Skip to content

[OpAMP] Snapshot profiling preparation for remote config#2944

Merged
robsunday merged 24 commits into
signalfx:mainfrom
robsunday:opamp-call-graphs
Jul 24, 2026
Merged

[OpAMP] Snapshot profiling preparation for remote config#2944
robsunday merged 24 commits into
signalfx:mainfrom
robsunday:opamp-call-graphs

Conversation

@robsunday

@robsunday robsunday commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

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:

  • StackTraceSampler
  • StagingArea
  • StackTraceExporter
  • ActiveSpanTracker
  • TraceThreadChangeDetector
  • SnapshotProfilingSpanProcessor

With this PR the following changes was introduced to enable snapshot profiling after agent started:

  • Snapshot profiling is initiated after SDK is initialized, from SnapshotProfilingAgentListener.afterAgent.
  • StackTraceSampler, StagingArea and StackTraceExporter are created when snapshot profiling is enabled.
  • ActiveSpanTracker, TraceThreadChangeDetector and SnapshotProfilingSpanProcessor are 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.

@robsunday robsunday changed the title Opamp call graphs [OpAMP] Enabling and disabling call graphs via remote config Jul 15, 2026

if (snapshotProfiling.isEnabled()) {
initActiveSpansTracking();
initStackTraceSampler(snapshotProfiling);

@robsunday robsunday Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] StackTraceSampler is now initiated in StackTraceSamplerInitializer called by SnapshotProfilingSupervisor

@Override
public Scope attach(Context toAttach) {
Scope scope = delegate.attach(toAttach);
if (!isEnabled()) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] ActiveSpanTracker is always created during SDK initialization and can be turned on and off by SnapshotProfilingSupervisor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the unfortunate world we will live in forever now. 💣

if (snapshotProfilingEnabled()) {
builder.addSpanProcessor(new SdkShutdownHook());
}
builder.addSpanProcessor(new SdkShutdownHook());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] It is safe to call SdkShutdownHook regardless if snapshot profiling is on or off

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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()) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] We always setup some components that must be created during sdk initialization, and later on they are enabled or disabled by SnapshotProfilingSupervisor.

@robsunday
robsunday marked this pull request as ready for review July 21, 2026 11:17
@robsunday
robsunday requested review from a team as code owners July 21, 2026 11:17
@robsunday robsunday changed the title [OpAMP] Enabling and disabling call graphs via remote config [OpAMP] Snapshot profiling preparation for remote config Jul 21, 2026
private Function<ConfigProperties, Map<String, String>> startTrackingActiveSpans(
TraceRegistry registry) {
return properties -> {
if (snapshotProfilingEnabled()) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] It is now created by StackTraceSamplerInitializer called by SnapshotProfilingSupervisor when profiling is enabled


autoConfigurationCustomizer
.addTracerProviderCustomizer(snapshotProfilingSpanProcessor(registry))
.addPropertiesCustomizer(setupStackTraceSampler())

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] StackTraceSampler is now initiated in StackTraceSamplerInitializer called by SnapshotProfilingSupervisor

private boolean includeTraceContextPropagator(Set<String> configuredPropagators) {
return configuredPropagators.isEmpty();
}
SnapshotProfilingSpanProcessor spanProcessor =

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] SnapshotProfilingSpanProcessor is now enabled and disabled by SnapshotProfilingSupervisor


configurationSupplier.get().log();

StackTraceSamplerInitializer.setupStackTraceSampler(configurationSupplier.get());

@robsunday robsunday Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] I'll probably move these methods to SnapshotProfilingSupervisor class

return;
}

updateJvmMemoryMetrics();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanna move it down below the jfr check too then?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +40 to +70
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());
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@robsunday robsunday Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be volatile?

@breedx-splk breedx-splk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@robsunday

Copy link
Copy Markdown
Contributor Author

but I was a little surprised that the supervisor wasn't "command" based. Maybe that is happening in a subsequent PR?

Yes. I have a bit different idea which I'm going to discuss with you first.

@robsunday
robsunday merged commit ba88e5d into signalfx:main Jul 24, 2026
28 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants