[OpAMP] Snapshot profiling preparation for remote config#2944
Conversation
|
|
||
| if (snapshotProfiling.isEnabled()) { | ||
| initActiveSpansTracking(); | ||
| initStackTraceSampler(snapshotProfiling); |
There was a problem hiding this comment.
[for reviewer] StackTraceSampler is now initiated in StackTraceSamplerInitializer called by SnapshotProfilingSupervisor
| @Override | ||
| public Scope attach(Context toAttach) { | ||
| Scope scope = delegate.attach(toAttach); | ||
| if (!isEnabled()) { |
There was a problem hiding this comment.
[for reviewer] ActiveSpanTracker is always created during SDK initialization and can be turned on and off by SnapshotProfilingSupervisor
There was a problem hiding this comment.
This is the unfortunate world we will live in forever now. 💣
| if (snapshotProfilingEnabled()) { | ||
| builder.addSpanProcessor(new SdkShutdownHook()); | ||
| } | ||
| builder.addSpanProcessor(new SdkShutdownHook()); |
There was a problem hiding this comment.
[for reviewer] It is safe to call SdkShutdownHook regardless if snapshot profiling is on or off
There was a problem hiding this comment.
[for reviewer] Like in SnapshotProfilingConfigurationCustomizerProvider, we always preconfigure some components that later can be enabled or disabled by SnapshotProfilingSupervisor
| SnapshotProfilingConfiguration snapshotProfiling = getSnapshotProfilingConfig(model); | ||
| SnapshotProfilingConfiguration.SUPPLIER.configure(snapshotProfiling); | ||
|
|
||
| if (snapshotProfiling.isEnabled()) { |
There was a problem hiding this comment.
[for reviewer] We always setup some components that must be created during sdk initialization, and later on they are enabled or disabled by SnapshotProfilingSupervisor.
| private Function<ConfigProperties, Map<String, String>> startTrackingActiveSpans( | ||
| TraceRegistry registry) { | ||
| return properties -> { | ||
| if (snapshotProfilingEnabled()) { |
There was a problem hiding this comment.
[for reviewer] This check is replaced by enabled property in ActiveSpanTracker and TraceThreadChangeDetector that are created by ContextStorageWrapper
| private Function<ConfigProperties, Map<String, String>> setupStackTraceSampler() { | ||
| return properties -> { | ||
| if (snapshotProfilingEnabled()) { | ||
| StackTraceSampler sampler = samplerProvider.apply(properties); |
There was a problem hiding this comment.
[for reviewer] It is now created by StackTraceSamplerInitializer called by SnapshotProfilingSupervisor when profiling is enabled
|
|
||
| autoConfigurationCustomizer | ||
| .addTracerProviderCustomizer(snapshotProfilingSpanProcessor(registry)) | ||
| .addPropertiesCustomizer(setupStackTraceSampler()) |
There was a problem hiding this comment.
[for reviewer] StackTraceSampler is now initiated in StackTraceSamplerInitializer called by SnapshotProfilingSupervisor
| private boolean includeTraceContextPropagator(Set<String> configuredPropagators) { | ||
| return configuredPropagators.isEmpty(); | ||
| } | ||
| SnapshotProfilingSpanProcessor spanProcessor = |
There was a problem hiding this comment.
[for reviewer] SnapshotProfilingSpanProcessor is now enabled and disabled by SnapshotProfilingSupervisor
|
|
||
| configurationSupplier.get().log(); | ||
|
|
||
| StackTraceSamplerInitializer.setupStackTraceSampler(configurationSupplier.get()); |
There was a problem hiding this comment.
[for reviewer] I'll probably move these methods to SnapshotProfilingSupervisor class
| return; | ||
| } | ||
|
|
||
| updateJvmMemoryMetrics(); |
There was a problem hiding this comment.
wanna move it down below the jfr check too then?
There was a problem hiding this comment.
I was not sure. It can be moved if we are sure we do not want to gather these metrics in case JFR is not available.
Do you recommend to move it down?
There was a problem hiding this comment.
Anyway, I moved it down. In case we decide to keep it upper I can do revert it in next PR. If these metrics do not make sense without profiling data then it should be fine now.
| static void setupStackTraceExporter( | ||
| SnapshotProfilingConfiguration configuration, | ||
| Resource resource, | ||
| OtelLoggerFactory otelLoggerFactory) { | ||
| int maxDepth = configuration.getStackDepth(); | ||
| Logger otelLogger = | ||
| buildLogger(otelLoggerFactory, resource, configuration.getConfigProperties()); | ||
| AsyncStackTraceExporter exporter = new AsyncStackTraceExporter(otelLogger, maxDepth); | ||
| StackTraceExporter.SUPPLIER.configure(exporter); | ||
| } | ||
|
|
||
| private static StagingArea createStagingArea(SnapshotProfilingConfiguration configuration) { | ||
| Duration interval = configuration.getExportInterval(); | ||
| int capacity = configuration.getStagingCapacity(); | ||
| return new PeriodicallyExportingStagingArea(StackTraceExporter.SUPPLIER, interval, capacity); | ||
| } | ||
|
|
||
| private static Logger buildLogger( | ||
| OtelLoggerFactory otelLoggerFactory, Resource resource, Object configProperties) { | ||
| if (configProperties instanceof DeclarativeConfigProperties) { | ||
| DeclarativeConfigProperties exporterConfig = | ||
| DeclarativeConfigPropertiesUtil.getStructuredOrEmpty( | ||
| (DeclarativeConfigProperties) configProperties, "exporter"); | ||
| return otelLoggerFactory.build(exporterConfig, resource); | ||
| } | ||
| if (configProperties instanceof ConfigProperties) { | ||
| return otelLoggerFactory.build((ConfigProperties) configProperties, resource); | ||
| } | ||
| throw new IllegalArgumentException( | ||
| "Unsupported config properties type: " + configProperties.getClass().getName()); | ||
| } |
There was a problem hiding this comment.
It's a little weird to me now that a class whose name suggests that it initializes a stack trace sampler, now also builds a logger and sets up an exporter. Should that be a separate responsibility?
There was a problem hiding this comment.
Agreed. This class no longer serves its original purpose. I already refactored it in #2955 (moved to SnapshotProfilingSupervisor), but I think I can improve it even further.
| private final TraceRegistry registry; | ||
| private final Supplier<StackTraceSampler> sampler; | ||
|
|
||
| private boolean enabled; |
There was a problem hiding this comment.
Should this be volatile?
breedx-splk
left a comment
There was a problem hiding this comment.
Thanks for getting this started. I had a few small comments, but I think it's fine to proceed with this.
I think the tests here do a pretty good job of showing how much unfortunate static/global state we are introducing, especially with all of the SUPPLIER stuff everywhere, but that's just what we got now I guess.
I appreciate that it's mostly following the same patterns as the main profiler mess, which made it easier to review...but I was a little surprised that the supervisor wasn't "command" based. Maybe that is happening in a subsequent PR? Anyway, cheers.
Yes. I have a bit different idea which I'm going to discuss with you first. |
This is refactoring of snapshot profiler preparing it to dynamic enabling and disabling with remote config.
While PR touches a lot of files, most of them are unit tests.
Snapshot profiling was implemented using multiple components that were mostly initialized during SDK customization.
These components are:
StackTraceSamplerStagingAreaStackTraceExporterActiveSpanTrackerTraceThreadChangeDetectorSnapshotProfilingSpanProcessorWith this PR the following changes was introduced to enable snapshot profiling after agent started:
SnapshotProfilingAgentListener.afterAgent.StackTraceSampler,StagingAreaandStackTraceExporterare created when snapshot profiling is enabled.ActiveSpanTracker,TraceThreadChangeDetectorandSnapshotProfilingSpanProcessorare always created but they are disabled by default. They are enabled when snapshot profiling is enabled. When in disabled state, they add close to zero overhead.This PR does not implement support for remote config. It is necessary preparation and dynamic enable/disable will be delivered by subsequent PRs.