Skip to content

Commit 9d8242c

Browse files
robsundayrobsunday
authored
[OpAMP] Remote config profiler call stack interval support (signalfx#2905)
* Profiler configuration refactoring * Licenses refreshed * Feedback after remote config sent. * Naming improved. Test improved * Fix after merge * spotless * Profiler reinitialization implemented * Unit test added * Optimized profiler restart * Test fixed * Tests improved * PeriodicRecordingFlusher and tests improved * Further tests improvements * More changes related to JFR Recording Sequencer -> JFR Recording Flusher rename * Reinitialization simplified * fix after merge * Code review followup
1 parent 277922e commit 9d8242c

12 files changed

Lines changed: 316 additions & 141 deletions

File tree

‎custom/src/main/java/com/splunk/opentelemetry/logging/SplunkSlf4jLoggingCustomizer.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ private static void addCustomLoggingConfiguration() {
5959

6060
// Silence the following warning on jdk21, this warning should go away with an update to jfr
6161
// parser
62-
// [otel.javaagent 2023-10-09 14:38:17:665 +0000] [JFR Recording Sequencer] WARN
62+
// [otel.javaagent 2023-10-09 14:38:17:665 +0000] [JFR Recording Flusher] WARN
6363
// org.openjdk.jmc.flightrecorder.internal.parser.v1.ValueReaders$ReflectiveReader - Could not
6464
// find field with name 'virtual' in reader for 'thread'
6565
setLogLevelIfNotSet(

‎opamp/src/main/java/com/splunk/opentelemetry/opamp/RemoteConfigProcessor.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.io.ByteArrayInputStream;
3030
import java.util.Map;
3131
import java.util.Objects;
32+
import java.util.logging.Level;
3233
import java.util.logging.Logger;
3334
import okio.ByteString;
3435
import opamp.proto.AgentConfigFile;
@@ -79,16 +80,16 @@ public void applyConfig(AgentRemoteConfig remoteConfig, OpampClient opampClient)
7980
ProfilerDeclarativeConfigurationFactory.create(
8081
distributionRemoteConfigProperties.getStructured(PROFILING_NODE_NAME, empty()));
8182

83+
ProfilerConfiguration currentProfilerConfiguration = ProfilerConfiguration.SUPPLIER.get();
8284
ProfilerConfiguration updatedProfilerConfig =
83-
ProfilerConfiguration.SUPPLIER.get().toBuilder()
85+
currentProfilerConfiguration.toBuilder()
8486
.setEnabled(receivedProfilerConfig.isEnabled())
87+
.setCallStackInterval(receivedProfilerConfig.getCallStackInterval())
8588
.build();
86-
ProfilerConfiguration.SUPPLIER.configure(updatedProfilerConfig);
8789

88-
if (updatedProfilerConfig.isEnabled()) {
89-
profilingSupervisor.requestStartProfiling();
90-
} else {
91-
profilingSupervisor.requestStopProfiling();
90+
if (!currentProfilerConfiguration.equals(updatedProfilerConfig)) {
91+
ProfilerConfiguration.SUPPLIER.configure(updatedProfilerConfig);
92+
profilingSupervisor.requestReinitializeProfiling();
9293
}
9394
}
9495

@@ -100,12 +101,12 @@ public void applyConfig(AgentRemoteConfig remoteConfig, OpampClient opampClient)
100101
opampClient);
101102

102103
} catch (Exception e) {
104+
logger.log(Level.WARNING, "Remote configuration not applied due to exception.", e);
103105
reportRemoteConfigStatus(
104106
remoteConfig.config_hash,
105107
RemoteConfigStatuses.RemoteConfigStatuses_FAILED,
106108
"Exception occurred: " + e.getMessage(),
107109
opampClient);
108-
throw e;
109110
}
110111

111112
// TODO: Maybe should be postponed after profiler is enabled/disabled?

‎opamp/src/test/java/com/splunk/opentelemetry/opamp/RemoteConfigProcessorTest.java‎

Lines changed: 64 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import static org.mockito.Mockito.never;
2121
import static org.mockito.Mockito.verify;
2222
import static org.mockito.Mockito.verifyNoInteractions;
23+
import static org.mockito.Mockito.verifyNoMoreInteractions;
2324

2425
import com.splunk.opentelemetry.opamp.effectiveconfig.EffectiveConfigReporter;
2526
import com.splunk.opentelemetry.profiler.ProfilerConfiguration;
@@ -108,14 +109,74 @@ void shouldStartProfilingWhenRemoteConfigEnablesProfiler() {
108109
assertThat(status.status).isEqualTo(RemoteConfigStatuses.RemoteConfigStatuses_APPLIED);
109110
assertThat(status.error_message).isEmpty();
110111
assertThat(ProfilerConfiguration.SUPPLIER.get().isEnabled()).isTrue();
111-
verify(profilingSupervisor).requestStartProfiling();
112-
verify(profilingSupervisor, never()).requestStopProfiling();
112+
verify(profilingSupervisor).requestReinitializeProfiling();
113+
verifyNoMoreInteractions(profilingSupervisor);
114+
verify(effectiveConfigReporter).reportEffectiveConfigIfChanged();
115+
}
116+
117+
@Test
118+
void shouldReportErrorWhenRemoteConfigProcessingFailed() {
119+
// given
120+
String remoteConfigYaml =
121+
"""
122+
distribution:
123+
splunk:
124+
profiling:
125+
INVALID YAML HERE
126+
""";
127+
ByteString configHash = ByteString.encodeUtf8("test-config-hash");
128+
AgentRemoteConfig remoteConfig = createRemoteConfig(configHash, remoteConfigYaml);
129+
130+
// when
131+
handler.applyConfig(remoteConfig, opampClient);
132+
133+
// then
134+
RemoteConfigStatus status = getReportedRemoteConfigStatus();
135+
assertThat(status.last_remote_config_hash).isEqualTo(configHash);
136+
assertThat(status.status).isEqualTo(RemoteConfigStatuses.RemoteConfigStatuses_FAILED);
137+
assertThat(status.error_message).startsWith("Exception occurred:");
138+
assertThat(ProfilerConfiguration.SUPPLIER.get().isEnabled()).isFalse();
139+
assertThat(ProfilerConfiguration.SUPPLIER.get().getCallStackInterval().toMillis())
140+
.isEqualTo(10000);
141+
verifyNoInteractions(profilingSupervisor);
142+
verify(effectiveConfigReporter).reportEffectiveConfigIfChanged();
143+
}
144+
145+
@Test
146+
void shouldUpdateProfilerConfigWhileRunning() {
147+
// given
148+
ProfilerConfiguration.SUPPLIER.configure(
149+
ProfilerConfiguration.builder().setEnabled(true).build());
150+
151+
String remoteConfigYaml =
152+
"""
153+
distribution:
154+
splunk:
155+
profiling:
156+
always_on:
157+
cpu_profiler:
158+
sampling_interval: 123
159+
""";
160+
ByteString configHash = ByteString.encodeUtf8("test-config-hash");
161+
AgentRemoteConfig remoteConfig = createRemoteConfig(configHash, remoteConfigYaml);
162+
163+
// when
164+
handler.applyConfig(remoteConfig, opampClient);
165+
166+
// then
167+
assertThat(ProfilerConfiguration.SUPPLIER.get().isEnabled()).isTrue();
168+
assertThat(ProfilerConfiguration.SUPPLIER.get().getCallStackInterval().toMillis())
169+
.isEqualTo(123);
170+
verify(profilingSupervisor).requestReinitializeProfiling();
171+
verifyNoMoreInteractions(profilingSupervisor);
113172
verify(effectiveConfigReporter).reportEffectiveConfigIfChanged();
114173
}
115174

116175
@Test
117176
void shouldStopProfilingWhenRemoteConfigDisablesProfiler() {
118177
// given
178+
ProfilerConfiguration.SUPPLIER.configure(
179+
ProfilerConfiguration.builder().setEnabled(true).build());
119180
String remoteConfigYaml =
120181
"""
121182
distribution:
@@ -134,7 +195,7 @@ void shouldStopProfilingWhenRemoteConfigDisablesProfiler() {
134195
assertThat(status.status).isEqualTo(RemoteConfigStatuses.RemoteConfigStatuses_APPLIED);
135196
assertThat(status.error_message).isEmpty();
136197
assertThat(ProfilerConfiguration.SUPPLIER.get().isEnabled()).isFalse();
137-
verify(profilingSupervisor).requestStopProfiling();
198+
verify(profilingSupervisor).requestReinitializeProfiling();
138199
verify(profilingSupervisor, never()).requestStartProfiling();
139200
verify(effectiveConfigReporter).reportEffectiveConfigIfChanged();
140201
}

‎profiler/src/main/java/com/splunk/opentelemetry/profiler/PeriodicRecordingFlusher.java‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020

2121
import com.google.common.annotations.VisibleForTesting;
2222
import com.splunk.opentelemetry.profiler.util.HelpfulExecutors;
23-
import io.opentelemetry.sdk.resources.Resource;
2423
import java.time.Duration;
2524
import java.util.concurrent.ScheduledExecutorService;
2625
import java.util.concurrent.ScheduledFuture;
@@ -78,9 +77,4 @@ void handleInterval() {
7877
logger.log(SEVERE, "Profiler periodic task failed.", throwable);
7978
}
8079
}
81-
82-
public static PeriodicRecordingFlusherBuilder builder(
83-
ProfilerConfiguration config, Resource resource) {
84-
return new PeriodicRecordingFlusherBuilder(config, resource);
85-
}
8680
}

profiler/src/main/java/com/splunk/opentelemetry/profiler/PeriodicRecordingFlusherBuilder.java renamed to profiler/src/main/java/com/splunk/opentelemetry/profiler/PeriodicRecordingFlusherFactory.java

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -38,25 +38,11 @@
3838
import java.util.Collections;
3939
import java.util.Map;
4040

41-
class PeriodicRecordingFlusherBuilder {
41+
class PeriodicRecordingFlusherFactory {
4242
private static final java.util.logging.Logger logger =
43-
java.util.logging.Logger.getLogger(PeriodicRecordingFlusherBuilder.class.getName());
44-
private final ProfilerConfiguration config;
45-
private final Resource resource;
43+
java.util.logging.Logger.getLogger(PeriodicRecordingFlusherFactory.class.getName());
4644

47-
private JFR jfr;
48-
49-
public PeriodicRecordingFlusherBuilder(ProfilerConfiguration config, Resource resource) {
50-
this.config = config;
51-
this.resource = resource;
52-
}
53-
54-
PeriodicRecordingFlusherBuilder jfr(JFR jfr) {
55-
this.jfr = jfr;
56-
return this;
57-
}
58-
59-
PeriodicRecordingFlusher build() {
45+
PeriodicRecordingFlusher create(ProfilerConfiguration config, Resource resource, JFR jfr) {
6046
if (jfr == null) {
6147
jfr = JFR.getInstance();
6248
}

‎profiler/src/main/java/com/splunk/opentelemetry/profiler/ProfilerConfiguration.java‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,54 @@ public Object getConfigProperties() {
170170
return configProperties;
171171
}
172172

173+
@Override
174+
public boolean equals(Object other) {
175+
if (this == other) {
176+
return true;
177+
}
178+
if (!(other instanceof ProfilerConfiguration)) {
179+
return false;
180+
}
181+
ProfilerConfiguration that = (ProfilerConfiguration) other;
182+
return enabled == that.enabled
183+
&& memoryEnabled == that.memoryEnabled
184+
&& memoryEventRateLimitEnabled == that.memoryEventRateLimitEnabled
185+
&& useAllocationSampleEvent == that.useAllocationSampleEvent
186+
&& includeAgentInternalStacks == that.includeAgentInternalStacks
187+
&& includeJvmInternalStacks == that.includeJvmInternalStacks
188+
&& tracingStacksOnly == that.tracingStacksOnly
189+
&& stackDepth == that.stackDepth
190+
&& keepFiles == that.keepFiles
191+
&& Objects.equals(ingestUrl, that.ingestUrl)
192+
&& Objects.equals(otlpProtocol, that.otlpProtocol)
193+
&& Objects.equals(memoryEventRate, that.memoryEventRate)
194+
&& Objects.equals(callStackInterval, that.callStackInterval)
195+
&& Objects.equals(profilerDirectory, that.profilerDirectory)
196+
&& Objects.equals(recordingDuration, that.recordingDuration)
197+
&& Objects.equals(configProperties, that.configProperties);
198+
}
199+
200+
@Override
201+
public int hashCode() {
202+
return Objects.hash(
203+
enabled,
204+
ingestUrl,
205+
otlpProtocol,
206+
memoryEnabled,
207+
memoryEventRateLimitEnabled,
208+
memoryEventRate,
209+
useAllocationSampleEvent,
210+
callStackInterval,
211+
includeAgentInternalStacks,
212+
includeJvmInternalStacks,
213+
tracingStacksOnly,
214+
stackDepth,
215+
keepFiles,
216+
profilerDirectory,
217+
recordingDuration,
218+
configProperties);
219+
}
220+
173221
public static int getJavaVersion() {
174222
String javaSpecVersion = System.getProperty("java.specification.version");
175223
if ("1.8".equals(javaSpecVersion)) {

‎profiler/src/main/java/com/splunk/opentelemetry/profiler/ProfilingSupervisor.java‎

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ public class ProfilingSupervisor {
4545
private final JFR jfr;
4646
private final AutoConfiguredOpenTelemetrySdk sdk;
4747
private final BlockingQueue<ProfilingCommand> commandQueue;
48+
private final PeriodicRecordingFlusherFactory recordingFlusherFactory;
4849
private final AtomicReference<PeriodicRecordingFlusher> recordingFlusher =
4950
new AtomicReference<>();
5051
private static final AtomicReference<JfrContextStorage> jfrContextStorage =
@@ -57,10 +58,21 @@ public class ProfilingSupervisor {
5758
JFR jfr,
5859
AutoConfiguredOpenTelemetrySdk sdk,
5960
BlockingQueue<ProfilingCommand> commandQueue) {
61+
this(configSupplier, jfr, sdk, commandQueue, new PeriodicRecordingFlusherFactory());
62+
}
63+
64+
@VisibleForTesting
65+
ProfilingSupervisor(
66+
OptionalConfigurableSupplier<ProfilerConfiguration> configSupplier,
67+
JFR jfr,
68+
AutoConfiguredOpenTelemetrySdk sdk,
69+
BlockingQueue<ProfilingCommand> commandQueue,
70+
PeriodicRecordingFlusherFactory recordingFlusherFactory) {
6071
this.configSupplier = configSupplier;
6172
this.jfr = jfr;
6273
this.sdk = sdk;
6374
this.commandQueue = commandQueue;
75+
this.recordingFlusherFactory = recordingFlusherFactory;
6476
}
6577

6678
static ProfilingSupervisor createAndStart(AutoConfiguredOpenTelemetrySdk sdk) {
@@ -105,6 +117,10 @@ public void requestStopProfiling() {
105117
commandQueue.add(ProfilingCommand.STOP);
106118
}
107119

120+
public void requestReinitializeProfiling() {
121+
commandQueue.add(ProfilingCommand.REINITIALIZE);
122+
}
123+
108124
private void handleCommand(ProfilingCommand command) {
109125
switch (command) {
110126
case START:
@@ -113,6 +129,9 @@ private void handleCommand(ProfilingCommand command) {
113129
case STOP:
114130
tryStop();
115131
break;
132+
case REINITIALIZE:
133+
tryReinitialize();
134+
break;
116135
}
117136
}
118137

@@ -129,7 +148,7 @@ private void setJfrContextStorageEnabled(boolean enabled) {
129148
*/
130149
private void tryStart() {
131150
if (isJfrRecordingActive()) {
132-
logger.warning("JFR is already running, not starting again.");
151+
logger.fine("JFR is already running, not starting again.");
133152
return;
134153
}
135154
if (!jfr.isAvailable()) {
@@ -145,21 +164,29 @@ private void tryStart() {
145164

146165
private void tryStop() {
147166
if (!isJfrRecordingActive()) {
148-
logger.warning("JFR is not running already, not stopping again.");
167+
logger.fine("JFR is not running already, not stopping again.");
149168
return;
150169
}
151170
setJfrContextStorageEnabled(false);
152171
deactivateJfrRecording();
153172
logger.info("Profiler is deactivated.");
154173
}
155174

175+
private void tryReinitialize() {
176+
tryStop();
177+
// Start the profiler with current settings if it is enabled. New settings will be applied.
178+
if (configSupplier.get().isEnabled()) {
179+
tryStart();
180+
}
181+
}
182+
156183
private boolean isJfrRecordingActive() {
157184
return recordingFlusher.get() != null;
158185
}
159186

160187
private void activateJfrRecording(Resource resource) {
161188
PeriodicRecordingFlusher recordingFlusher =
162-
makeRecordingFlusherBuilder(resource).jfr(jfr).build();
189+
recordingFlusherFactory.create(configSupplier.get(), resource, jfr);
163190
if (this.recordingFlusher.compareAndSet(null, recordingFlusher)) {
164191
recordingFlusher.start();
165192
}
@@ -172,11 +199,6 @@ private void deactivateJfrRecording() {
172199
}
173200
}
174201

175-
// Exists for testing
176-
PeriodicRecordingFlusherBuilder makeRecordingFlusherBuilder(Resource resource) {
177-
return PeriodicRecordingFlusher.builder(configSupplier.get(), resource);
178-
}
179-
180202
static void setupJfrContextStorage() {
181203
if (!contextStorageSetup.compareAndSet(false, true)) {
182204
return;
@@ -192,6 +214,7 @@ static void setupJfrContextStorage() {
192214

193215
enum ProfilingCommand {
194216
START,
195-
STOP
217+
STOP,
218+
REINITIALIZE
196219
}
197220
}

‎profiler/src/main/java/com/splunk/opentelemetry/profiler/StackTraceFilter.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public class StackTraceFilter {
3333
"\"JFR Recorder Thread\"",
3434
"\"JFR Periodic Tasks\"",
3535
"\"JFR Recording Scheduler\"",
36-
"\"JFR Recording Sequencer\"",
36+
"\"JFR Recording Flusher\"",
3737
"\"Reference Handler\"",
3838
"\"Finalizer\"",
3939
"\"C1 CompilerThread",

0 commit comments

Comments
 (0)