Expose OTel thread/process context without requiring profiling to be enabled #12546
+1,054
−39
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| package datadog.trace.bootstrap; | ||
|
|
||
| import static java.util.concurrent.TimeUnit.MILLISECONDS; | ||
|
|
||
| import datadog.context.Context; | ||
| import datadog.trace.api.EndpointTracker; | ||
| import datadog.trace.api.Stateful; | ||
| import datadog.trace.api.profiling.ProfilingContextAttribute; | ||
| import datadog.trace.api.profiling.ProfilingScope; | ||
| import datadog.trace.api.profiling.Timing; | ||
| import datadog.trace.bootstrap.instrumentation.api.AgentSpan; | ||
| import datadog.trace.bootstrap.instrumentation.api.ProfilerContext; | ||
| import datadog.trace.bootstrap.instrumentation.api.ProfilingContextIntegration; | ||
| import datadog.trace.util.AgentTaskScheduler; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.concurrent.Callable; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * A {@link ProfilingContextIntegration} handed out synchronously during {@code premain} while the | ||
| * real ddprof-based integration is constructed later, off the premain thread, to avoid loading the | ||
| * ddprof native library (and touching {@code java.nio.file}) before {@code main} gets a chance to | ||
| * set its own {@code java.nio.file.spi.DefaultFileSystemProvider}. Delegates to {@link | ||
| * ProfilingContextIntegration.NoOp} until the swap happens; stays a no-op forever if construction | ||
| * fails. | ||
| */ | ||
| final class DeferredProfilingContextIntegration implements ProfilingContextIntegration { | ||
| private static final Logger log = | ||
| LoggerFactory.getLogger(DeferredProfilingContextIntegration.class); | ||
|
|
||
| /** | ||
| * Delay before the deferred construction runs, giving {@code main} a chance to install its own | ||
| * {@code java.nio.file.spi.DefaultFileSystemProvider} first; not user-tunable since losing the | ||
| * first second of context exposure is not observable. | ||
| */ | ||
| private static final long INITIALIZATION_DELAY_MILLIS = 1_000; | ||
|
|
||
| private final String name; | ||
| private final Callable<ProfilingContextIntegration> factory; | ||
|
|
||
| /** | ||
| * Swapped to the real integration once construction succeeds; volatile since scopes may already | ||
| * be running when the swap happens. | ||
| */ | ||
| private volatile ProfilingContextIntegration delegate = ProfilingContextIntegration.NoOp.INSTANCE; | ||
|
|
||
| /** | ||
| * Callbacks queued via {@link #whenAvailable(Runnable)} before the swap; guarded by {@code this} | ||
| * together with the {@link #delegate} write so none is run twice or dropped. | ||
| */ | ||
| private final List<Runnable> pendingAvailabilityCallbacks = new ArrayList<>(1); | ||
|
|
||
| /** | ||
| * @param name the name reported by {@link #name()}, i.e. the name of the integration being | ||
| * deferred. | ||
| * @param factory creates the real integration; invoked at most once, off the premain thread. | ||
| */ | ||
| DeferredProfilingContextIntegration( | ||
| final String name, final Callable<ProfilingContextIntegration> factory) { | ||
| this.name = name; | ||
| this.factory = factory; | ||
| } | ||
|
|
||
| /** | ||
| * Schedules the deferred construction to run off this (premain) thread, after {@link | ||
| * #INITIALIZATION_DELAY_MILLIS}. | ||
| */ | ||
| void scheduleInitialization() { | ||
| AgentTaskScheduler.get().schedule(this::initialize, INITIALIZATION_DELAY_MILLIS, MILLISECONDS); | ||
|
jandro996 marked this conversation as resolved.
jandro996 marked this conversation as resolved.
jandro996 marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** | ||
| * Runs the deferred construction; called exactly once per instance, from {@link | ||
| * #scheduleInitialization()}. On failure this instance keeps behaving as {@link | ||
| * ProfilingContextIntegration.NoOp} forever; a background failure must never propagate. | ||
| */ | ||
| void initialize() { | ||
| try { | ||
| final ProfilingContextIntegration integration = factory.call(); | ||
|
jandro996 marked this conversation as resolved.
|
||
| if (integration == null) { | ||
|
Contributor
There was a problem hiding this comment. [Sphinx Review — LOW] Mutation candidate (null-guard) on the integration==null check in initialize() has no test exercising a factory that returns null. Deleting the guard (mutant) would set delegate=null and make pass-through methods throw NPE after initialize(); no existing test constructs DeferredProfilingContextIntegration with a null-returning factory and then calls a pass-through method, so the mutant survives. Suggestion: Add a test with a factory returning null; assert initialize() completes and pass-through calls still behave as NoOp.
Member
Author
There was a problem hiding this comment. Good catch - added |
||
| return; | ||
| } | ||
| final List<Runnable> callbacks; | ||
| synchronized (this) { | ||
| delegate = integration; | ||
| callbacks = new ArrayList<>(pendingAvailabilityCallbacks); | ||
| pendingAvailabilityCallbacks.clear(); | ||
| } | ||
| for (final Runnable callback : callbacks) { | ||
| try { | ||
| callback.run(); | ||
| } catch (final Throwable t) { | ||
| log.debug("Availability callback for {} profiling context failed.", name, t); | ||
| } | ||
| } | ||
| } catch (final Throwable t) { | ||
| // toString() because failures here (UnsatisfiedLinkError etc.) often carry no message. | ||
| log.info("Deferred {} profiling context labeling not available. {}", name, t.toString()); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Runs {@code callback} once the real integration is swapped in, or immediately if it already is; | ||
| * never runs it if the deferred construction failed. | ||
| */ | ||
| @Override | ||
| public void whenAvailable(final Runnable callback) { | ||
| // double-checked: delegate is volatile, so a post-swap caller never takes the lock | ||
| if (delegate == ProfilingContextIntegration.NoOp.INSTANCE) { | ||
| synchronized (this) { | ||
| if (delegate == ProfilingContextIntegration.NoOp.INSTANCE) { | ||
| pendingAvailabilityCallbacks.add(callback); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| callback.run(); | ||
| } | ||
|
|
||
| /** | ||
| * The name of the deferred integration, not of the current delegate: read once at tracer build | ||
| * time, possibly before the deferred construction completes. | ||
| */ | ||
| @Override | ||
| public String name() { | ||
| return name; | ||
| } | ||
|
|
||
| @Override | ||
| public void onStart() { | ||
| delegate.onStart(); | ||
| } | ||
|
|
||
| @Override | ||
| public void onAttach() { | ||
| delegate.onAttach(); | ||
| } | ||
|
|
||
| @Override | ||
| public void onDetach() { | ||
| delegate.onDetach(); | ||
| } | ||
|
jandro996 marked this conversation as resolved.
|
||
|
|
||
| @Override | ||
| public boolean isThreadContextBindingRequired() { | ||
| return delegate.isThreadContextBindingRequired(); | ||
| } | ||
|
|
||
| @Override | ||
| public void setContext(final Context context) { | ||
| delegate.setContext(context); | ||
| } | ||
|
|
||
| @Override | ||
| public Stateful newScopeState(final ProfilerContext profilerContext) { | ||
| return delegate.newScopeState(profilerContext); | ||
| } | ||
|
|
||
| @Override | ||
| public int encode(final CharSequence constant) { | ||
| return delegate.encode(constant); | ||
| } | ||
|
|
||
| @Override | ||
| public int encodeOperationName(final CharSequence constant) { | ||
| return delegate.encodeOperationName(constant); | ||
| } | ||
|
|
||
| @Override | ||
| public int encodeResourceName(final CharSequence constant) { | ||
| return delegate.encodeResourceName(constant); | ||
| } | ||
|
|
||
| @Override | ||
| public ProfilingContextAttribute createContextAttribute(final String attribute) { | ||
| return delegate.createContextAttribute(attribute); | ||
| } | ||
|
|
||
| @Override | ||
| public ProfilingScope newScope() { | ||
| return delegate.newScope(); | ||
| } | ||
|
|
||
| @Override | ||
| public void onRootSpanFinished(final AgentSpan rootSpan, final EndpointTracker tracker) { | ||
| delegate.onRootSpanFinished(rootSpan, tracker); | ||
| } | ||
|
|
||
| @Override | ||
| public EndpointTracker onRootSpanStarted(final AgentSpan rootSpan) { | ||
| return delegate.onRootSpanStarted(rootSpan); | ||
| } | ||
|
|
||
| @Override | ||
| public Timing start(final TimerType type) { | ||
| return delegate.start(type); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package datadog.trace.bootstrap; | ||
|
|
||
| import static datadog.trace.api.config.AppSecConfig.APPSEC_ENABLED; | ||
| import static datadog.trace.api.config.ProfilingConfig.PROFILING_DATADOG_PROFILER_ENABLED; | ||
| import static datadog.trace.api.config.ProfilingConfig.PROFILING_ENABLED; | ||
| import static org.junit.jupiter.api.Assertions.assertSame; | ||
| import static org.junit.jupiter.api.Assumptions.assumeTrue; | ||
|
|
||
| import datadog.trace.api.Config; | ||
| import datadog.trace.bootstrap.instrumentation.api.ProfilingContextIntegration; | ||
| import datadog.trace.test.junit.utils.config.WithConfig; | ||
| import datadog.trace.test.junit.utils.config.WithConfigExtension; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
|
|
||
| /** | ||
| * Profiling is explicitly unsupported in AWS Lambda runtimes ({@code Agent#startProfilingAgent} | ||
| * bails out there). The AppSec-driven OTel context exposure path must honour the same exclusion, so | ||
| * that enabling AppSec inside a Lambda function never loads the ddprof native library. | ||
| * | ||
| * <p>Forked because {@link WithConfigExtension} swaps the process-wide environment variable | ||
| * provider. | ||
| */ | ||
| @ExtendWith(WithConfigExtension.class) | ||
| class AgentLambdaProfilingContextForkedTest { | ||
|
|
||
| @Test | ||
| @WithConfig(key = APPSEC_ENABLED, value = "true") | ||
| @WithConfig(key = PROFILING_ENABLED, value = "false") | ||
| @WithConfig(key = PROFILING_DATADOG_PROFILER_ENABLED, value = "true") | ||
| @WithConfig( | ||
| key = "AWS_LAMBDA_FUNCTION_NAME", | ||
| value = "my-function", | ||
| env = true, | ||
| addPrefix = false) | ||
| void doesNotCreateTheDdprofIntegrationInAwsLambda() { | ||
| // The exclusion is only observable when the configuration would otherwise have triggered the | ||
| // ddprof context integration; the Datadog profiler is vetoed on some platforms and JVMs. | ||
| assumeTrue( | ||
| Config.get().isOtelThreadContextEnabled(), | ||
| "OTel context exposure is unavailable on this platform/JVM version"); | ||
|
|
||
| // AGENT_CLASSLOADER is null in this unit test, so reaching the ddprof branch at all would fail | ||
| // loudly rather than silently return the no-op integration. | ||
| assertSame( | ||
| ProfilingContextIntegration.NoOp.INSTANCE, Agent.createProfilingContextIntegration()); | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read this code correctly, when the 'ddprof' profiler library is disabled and the otel thread context is enabled, we short-circuit back to ddprof context integration, effectively disabling the fall-back profiler context integration based on JFR events for timeline and trace-to-profile, breaking those features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I’m wrong, but the new branch only kicks in when
isProfilingEnabled() == false, while the JFR fallback requiresisProfilingEnabled() == true. So they’re mutually exclusive, and we can never lose the JFR fallback, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch kicks in for
isDatadogProfilerEnabled() == falsenotisProfilingEnabled() == false- at least I can not find that guard anywhere.The
isOtelThreadContextEnabledmay becometrueonly ifisProfilingEnabled() == true, so ifisProfilingEnabled() == falsewe would not be entering this branch at all.My point is - we have profiling enabled, but not the 'ddprof' native library based implementation. Therefore, in the original code we did fall back to JFR event based context representation so our timeline and trace-to-profile can still work, even though with lowered precission.
But if we return unconditinally the wrapper from this branch, the JFR event based context representation will stay disabled and break our timeline and trace-to-profile.
Sadly, the JVM JFR can not use the otel/native thread context and because of that we need to have the alternative, JFR event based implementation available.