Port span correlation #2251

tduncan · 2025-03-27T16:46:22Z

This PR add span correlation snapshot profiling stack traces. Span correlation is done by intercepting OpenTelemetry context changes via a ContextStorage that maps trace IDs to SpanContexts as new Contexts are attached and detached.

…aces selected for snapshotting.

…ble.

…ogger.

…tests to reliably inject the snapshot profiling active span tracker into the OpenTelemetry SDK runtime.

…ker to be implemented as a Singleton.

…her or not to track it.

…tting class in favor of direct use of OpenTelemetry's IdGenerator.

…ContextBuilder obtained from Snapshotting.

…her spans.

…ace instances so to always have a trace ID associated even if span tracking is offline.

…n ever be null.

…to a parent span upon scope closing.

tduncan · 2025-03-27T17:39:28Z

.../splunk/opentelemetry/profiler/snapshot/InterceptingContextStorageSpanTrackingActivator.java

This class primarily exists to ease testing since ContextStorage.addWrapper must be called prior to the ContextStorage being initialized. Putting this behind a Supplier offers tests a way to hook in (via the SettableContextStorageProvider) without trying to coordinate the ContextStorage initialization across every single test.

tduncan · 2025-03-27T17:46:36Z

profiler/src/main/java/com/splunk/opentelemetry/profiler/snapshot/SpanTrackerProvider.java

Early designs had ActiveSpanTracker as a singleton object. That worked but was quite intricate to use correctly. Pushing the singleton behavior into a provider removes a lot of the complexities with a minor inconvenience in ScheduledExecutorStackTraceSampler where the SpanTracker is needed.

profiler/src/main/java/com/splunk/opentelemetry/profiler/snapshot/ActiveSpanTracker.java

…ces that span multiple threads.

laurit · 2025-04-01T12:56:15Z

profiler/src/main/java/com/splunk/opentelemetry/profiler/snapshot/ActiveSpanTracker.java

+  }
+
+  private boolean doNotTrack(SpanContext spanContext) {
+    return !spanContext.isSampled() || !registry.isRegistered(spanContext);


perhaps should also add !spanContext.isValid()

I've added a test for invalid spans but it did not require adding a explicit check. I think the isSampled check doubles as a check for isValid. I'll leave the test to make that scenario documented.

true, Span.wrap(SpanContext.getInvalid()) creates a span where sampled flag is not set. I thought a bit more about this and since for registry.isRegistered(spanContext) to pass span must be registered and since the register is called from a span processor it should be impossible for an invalid span to pass this method. Since SnapshotProfilingSpanProcessor calls register only for the root span only root spans will be reported as active spans. Is this true? If it is perhaps it would make sense to spell this out somewhere.

I missed that the trace registry only checks for the trace id.

splunk-otel-java/profiler/src/main/java/com/splunk/opentelemetry/profiler/snapshot/TraceRegistry.java

Line 24 in 7bb828a

private final Map<String, SpanContext> traceIds = new ConcurrentHashMap<>();

could be a set as the map value is not used

I think you answered your question, however:

Since SnapshotProfilingSpanProcessor calls register only for the root span only root spans will be reported as active spans. Is this true?

No. Its true that only a root span will trigger the trace registration process but once a trace is registered the ActiveSpanTracker will keep track of child spans as they are added to the OpenTelemetry Context.

splunk-otel-java/profiler/src/main/java/com/splunk/opentelemetry/profiler/snapshot/TraceRegistry.java

Line 24 in 7bb828a

private final Map<String, SpanContext> traceIds = new ConcurrentHashMap<>();

could be a set as the map value is not used

Yes it can! I will submit a separate PR to update it.

I think you answered your question, however:

Since SnapshotProfilingSpanProcessor calls register only for the root span only root spans will be reported as active spans. Is this true?

No. Its true that only a root span will trigger the trace registration process but once a trace is registered the ActiveSpanTracker will keep track of child spans as they are added to the OpenTelemetry Context.

One minor -- but important -- detail I left out: trace selection only happens for root spans (see SnapshotVolumePropagator) but trace registration happens for service entry spans (see SnapshotProfilingSpanProcessor).

laurit · 2025-04-01T13:02:48Z

...main/java/com/splunk/opentelemetry/profiler/snapshot/ScheduledExecutorStackTraceSampler.java

@@ -88,13 +91,19 @@ public void run() {
      try {
        Instant now = Instant.now();
        ThreadInfo threadInfo = threadMXBean.getThreadInfo(threadId, Integer.MAX_VALUE);
-        StackTrace stackTrace = StackTrace.from(now, samplingPeriod, traceId, threadInfo);
+        SpanContext spanContext = retrieveActiveSpan(threadId);


Since there is race between taking the thread dump and asking the active span it is possible that the stack trace and the span won't match (stack trace won't include the method where the span is active). Is that an issue?

Theoretically, yes, but in practice I would suspect the risk is likely quite low. Low enough that I don't think we would want to block the trace thread(s) every time the snapshot profiler wants to take a sample.

I think it is fine for now. Continuous profiler has the same issue, there the dump is taken by jfr and we can't really do anything to synchronize with that. If blocking is of concern then keep in mind that, as far as I know, taking the stack trace suspends all threads. You could reduce the blocking by having only one scheduled task that takes the dump for all threads that you care about in one operation.

profiler/src/main/java/com/splunk/opentelemetry/profiler/snapshot/ActiveSpanTracker.java

laurit · 2025-04-01T14:00:09Z

profiler/src/test/java/com/splunk/opentelemetry/profiler/snapshot/ActiveSpanTrackerTest.java

+    var executor = Executors.newFixedThreadPool(2);
+    try (var scope1 = executor.submit(attach(span1)).get();
+        var scope2 = executor.submit(attach(span2)).get()) {
+      assertEquals(
+          Optional.of(span1.getSpanContext()), spanTracker.getActiveSpan(scope1.getThreadId()));
+      assertEquals(
+          Optional.of(span2.getSpanContext()), spanTracker.getActiveSpan(scope2.getThreadId()));
+    } finally {
+      executor.shutdown();
+    }


I found it hard to reason why both actions should be executed on separate threads. Looks line an implementation detail of ThreadPoolExecutor. I guess it is fine as long as it works reliably.

ActiveSpanTracker records the thread ID using Thread.currentThread().getId() so if both spans are attached to the OpenTelemetry Context on the same thread only one would be considered the active thread.

Let me rephrase. If you change Executors.newFixedThreadPool(2) to Executors.newFixedThreadPool(1) the test will fail. As far as I can tell the fact that when using Executors.newFixedThreadPool(2) both of the actions run on separate threads is an implementation detail of ThreadPoolExecutor not a specified behavior.

I understand now. You are technically correct, there aren't any documented guarantees that the two operations will actually be performed on separate threads so the test is relying on the first task not finishing prior to the second task actually being submitted. I've used this technique quite a few times over the years and it hasn't caused issues but I can add an explicit release step so the test ensures that the first submitted operation doesn't end to quickly to ensure that both threads in the thread pool much be used.

so the test is relying on the first task not finishing prior to the second task actually being submitted

I think this is not necessary. Looking at ThreadPoolExecutor I got the impression that it creates new threads for tasks as long as the thread pool has reached the requested size. So even when the first task is completed it will create a new thread for the second task to reach the pool size.

profiler/src/test/java/com/splunk/opentelemetry/profiler/snapshot/ActiveSpanTrackerTest.java

…eak reference keys in ActiveSpanTracker to ensure than map entries are automatically removed if there are a thread terminates without the OpenTelemetry scope being closed properly.

…tasks to ensure those tasks execute on multiple threads.

tduncan added 27 commits March 19, 2025 05:09

Add a custom ContextStorage to track the currently active span for tr…

47ce35f

…aces selected for snapshotting.

Configure active span tracking in SnapshotProfilingSdkCustomizer.

3104bc0

Add the SpanTracker interface.

70860c5

Include the active span details on collected stack traces when availa…

cdff346

…ble.

Activate span stracking in SnapshotProfilingSdkCustomizer.

0d33147

Merging 'main' into 'port-span-correlation'.

4086a26

Remove missed debugging statements in test.

b02605d

Remove unnecessary log record span context setting from InMemoryOtelL…

acab402

…ogger.

Include the currently active span ID on collected stack traces.

0a98127

Introduce the SpanTrackingActivator interface to add a mechanism for …

c201446

…tests to reliably inject the snapshot profiling active span tracker into the OpenTelemetry SDK runtime.

Introduce a SpanTrackerProvider to remove the need for ActiveSpanTrac…

2b4cc6d

…ker to be implemented as a Singleton.

Remove unnecessary 'null' check in ActiveSpanTracker.

7156c0b

Add tests for InterceptingContextStorageSpanTrackingActivator.

7eb843d

Add tests for SpanTrackerProvider.

8a71d81

Consider whether a span is sampled, not recording, when deciding whet…

cd45fd5

…her or not to track it.

Remove unnecessary fake ContextStorage in ActiveSpanTrackerTest.

7294c2c

Add a builder subclass for FakeSpan in ActiveSpanTrackerTest.

60d6c5a

Move SpanContext builder into Snapshotting test helper class.

785f3cd

Remove helper method for generating random trace IDs from the Snapsho…

a89916d

…tting class in favor of direct use of OpenTelemetry's IdGenerator.

Restore unnecessary changes to InMemoryOtelLogger.

d052116

Remove FakeSpan builder in ActiveSpanTrackerTest in favor of the Span…

3b3d20c

…ContextBuilder obtained from Snapshotting.

Add easier way to build SpanContext instances using trace IDs from ot…

c996820

…her spans.

Do not use traceId from the active span tracker when creating StackTr…

5c7cfc0

…ace instances so to always have a trace ID associated even if span tracking is offline.

Remove the possibility that the SpanTracker in SpanTrackerProvider ca…

23f9b32

…n ever be null.

Add ActiveSpanTrackerTest asserting that the active span is restored …

9b69551

…to a parent span upon scope closing.

Renaming class.

0b546bc

Expect span ID on exported logs.

ef218e9

tduncan requested review from a team as code owners March 27, 2025 16:46

Apply spotless code formatting.

6292ec3

tduncan commented Mar 27, 2025

View reviewed changes

laurit reviewed Mar 28, 2025

View reviewed changes

profiler/src/main/java/com/splunk/opentelemetry/profiler/snapshot/ActiveSpanTracker.java Outdated Show resolved Hide resolved

profiler/src/main/java/com/splunk/opentelemetry/profiler/snapshot/ActiveSpanTracker.java Outdated Show resolved Hide resolved

Track active spans per thread rather than trace ID to account for tra…

91e7ad5

…ces that span multiple threads.

laurit reviewed Apr 1, 2025

View reviewed changes

profiler/src/main/java/com/splunk/opentelemetry/profiler/snapshot/ActiveSpanTracker.java Outdated Show resolved Hide resolved

laurit reviewed Apr 1, 2025

View reviewed changes

profiler/src/test/java/com/splunk/opentelemetry/profiler/snapshot/ActiveSpanTrackerTest.java Outdated Show resolved Hide resolved

tduncan added 5 commits April 1, 2025 09:25

Replace FakeSpan in ActiveSpanTrackerTest with Span.wrap.

cc43350

Add ActiveSpanTracker test to ensure that invalid spans are not tracked.

65ec8d8

Track active spans by Thread rather than thread ID and use a map of w…

e0d40aa

…eak reference keys in ActiveSpanTracker to ensure than map entries are automatically removed if there are a thread terminates without the OpenTelemetry scope being closed properly.

Add a CountDownLatch to allow tests to explicitly release background …

1e810c2

…tasks to ensure those tasks execute on multiple threads.

Apply spotless code formatting.

2cd9e65

laurit approved these changes Apr 2, 2025

View reviewed changes

tduncan mentioned this pull request Apr 2, 2025

Update TraceRegistry To Use A Set #2256

Merged

laurit merged commit c05c5b3 into signalfx:main Apr 3, 2025
26 checks passed

github-actions bot locked and limited conversation to collaborators Apr 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port span correlation #2251

Port span correlation #2251

tduncan commented Mar 27, 2025

tduncan Mar 27, 2025

tduncan Mar 27, 2025

laurit Apr 1, 2025

tduncan Apr 1, 2025

laurit Apr 2, 2025

laurit Apr 2, 2025

laurit Apr 2, 2025

tduncan Apr 2, 2025

tduncan Apr 2, 2025

laurit Apr 1, 2025

tduncan Apr 1, 2025 •

edited

Loading

laurit Apr 2, 2025

laurit Apr 1, 2025

tduncan Apr 1, 2025

laurit Apr 1, 2025

tduncan Apr 1, 2025 •

edited

Loading

laurit Apr 2, 2025

Port span correlation #2251

Port span correlation #2251

Conversation

tduncan commented Mar 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tduncan Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tduncan Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tduncan Apr 1, 2025 •

edited

Loading

tduncan Apr 1, 2025 •

edited

Loading