-
Notifications
You must be signed in to change notification settings - Fork 337
fix(okhttp): capture scope at AsyncCall.<init> to keep traces intact under virtual-thread Dispatcher #11479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(okhttp): capture scope at AsyncCall.<init> to keep traces intact under virtual-thread Dispatcher #11479
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,9 +8,30 @@ muzzle { | |
| } | ||
|
|
||
| apply from: "$rootDir/gradle/java.gradle" | ||
| // Use slf4j-simple for tests; logback's synchronized appenders can pin virtual-thread carriers | ||
| // when many vthreads log concurrently, which deadlocks this module's vthread21Test suite. | ||
| apply from: "$rootDir/gradle/slf4j-simple.gradle" | ||
|
|
||
| addTestSuiteForDir('latestDepTest', 'test') | ||
|
|
||
| // Separate suite for tests that need the JDK 21+ virtual-thread API. Kept apart from the | ||
| // default test suite so that the existing OkHttp 3.0 tests keep their Java 1.8 baseline. | ||
| addTestSuite('vthread21Test') | ||
|
|
||
| tasks.named("compileVthread21TestJava", JavaCompile) { | ||
| configureCompiler(it, 21) | ||
| } | ||
|
|
||
| tasks.named("vthread21Test", Test) { | ||
| testJvmConstraints { | ||
| minJavaVersion = JavaVersion.VERSION_21 | ||
| } | ||
|
Comment on lines
+25
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This suite is forced to run with a JDK 21 launcher, but it does not declare a task-specific Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DataDog fix this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can only run on private repositories. |
||
| } | ||
|
|
||
| tasks.named("check") { | ||
| dependsOn "vthread21Test" | ||
| } | ||
|
Comment on lines
+31
to
+33
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this is needed? Normally you should use the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that doesn't quite work here because it is a mix of okhttp and virtual threads that we need to test. The fix is with okhttp, but using virtual threads requires Java 21. |
||
|
|
||
| dependencies { | ||
| compileOnly(group: 'com.squareup.okhttp3', name: 'okhttp', version: '3.0.0') | ||
|
|
||
|
|
@@ -38,4 +59,14 @@ dependencies { | |
|
|
||
| testRuntimeOnly(project(':dd-java-agent:instrumentation:datadog:asm:iast-instrumenter')) | ||
| testRuntimeOnly(project(':dd-java-agent:instrumentation:java:java-net:java-net-1.8')) | ||
|
|
||
| // vthread21Test inherits testImplementation via addTestSuite's extendsFrom wiring. | ||
| vthread21TestImplementation(group: 'com.squareup.okhttp3', name: 'okhttp', version: '3.11.0') | ||
| vthread21TestImplementation(group: 'com.squareup.okio', name: 'okio', version: '1.14.0') | ||
|
|
||
| // Pull in the JDK-21+ concurrent / lang instrumentations so the test installs the same | ||
| // TaskRunnerInstrumentation + VirtualThreadInstrumentation chain that profiling-backend | ||
| // exercises in production. | ||
| vthread21TestRuntimeOnly project(':dd-java-agent:instrumentation:java:java-concurrent:java-concurrent-21.0') | ||
| vthread21TestRuntimeOnly project(':dd-java-agent:instrumentation:java:java-lang:java-lang-21.0') | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| package datadog.trace.instrumentation.okhttp3; | ||
|
|
||
| import static datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils.capture; | ||
| import static java.util.Collections.singletonMap; | ||
| import static net.bytebuddy.matcher.ElementMatchers.isConstructor; | ||
|
|
||
| import com.google.auto.service.AutoService; | ||
| import datadog.trace.agent.tooling.Instrumenter; | ||
| import datadog.trace.agent.tooling.InstrumenterModule; | ||
| import datadog.trace.bootstrap.InstrumentationContext; | ||
| import datadog.trace.bootstrap.instrumentation.java.concurrent.State; | ||
| import java.util.Map; | ||
| import net.bytebuddy.asm.Advice; | ||
|
|
||
| /** | ||
| * Captures the active scope at {@code AsyncCall.<init>} (i.e. the moment {@code | ||
| * Call.enqueue(callback)} was invoked on the user's thread) and stores it in the {@code | ||
| * ContextStore<Runnable, State>} shared with the rest of the concurrent instrumentation. {@code | ||
| * RunnableInstrumentation} then re-activates that scope on {@code AsyncCall.run()} entry, which | ||
| * overrides whatever scope {@code TaskRunner.run()} (or {@code beforeExecute}) put in place from | ||
| * the dispatcher's worker thread. | ||
| * | ||
| * <p>Without this, {@code TaskRunnerInstrumentation} captures whatever scope happens to be active | ||
| * on the worker thread when {@code Dispatcher.promoteAndExecute()} dequeues and submits the call — | ||
| * and when promotion runs from inside {@code Dispatcher.finished()} (i.e. recursively from a | ||
| * <em>different</em> AsyncCall's run()), that scope belongs to the finishing call, not to the | ||
| * caller who actually enqueued this AsyncCall. Result: under concurrent OkHttp load, {@code | ||
| * okhttp.request} spans cross-contaminate between traces. | ||
| * | ||
| * <p>{@code AsyncCall} is an inner class of {@code RealCall} and transitively implements {@link | ||
| * Runnable}. | ||
| */ | ||
| @AutoService(InstrumenterModule.class) | ||
| public final class AsyncCallInstrumentation extends InstrumenterModule.Tracing | ||
| implements Instrumenter.ForKnownTypes, Instrumenter.HasMethodAdvice { | ||
|
|
||
| public AsyncCallInstrumentation() { | ||
| // Re-use the existing "okhttp" / "okhttp-3" instrumentation names so we don't introduce a | ||
| // separately-toggleable feature flag (DD_TRACE_OKHTTP_ASYNC_CALL_ENABLED). The capture here | ||
| // is conceptually part of the OkHttp instrumentation — if you disable OkHttp tracing, you | ||
| // also disable this capture, which is the right behavior. | ||
| super("okhttp", "okhttp-3"); | ||
| } | ||
|
|
||
| @Override | ||
| public String[] knownMatchingTypes() { | ||
| return new String[] { | ||
| "okhttp3.RealCall$AsyncCall", | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public Map<String, String> contextStore() { | ||
| // Same Runnable -> State store that RunnableInstrumentation reads from. | ||
| return singletonMap("java.lang.Runnable", State.class.getName()); | ||
| } | ||
|
|
||
| @Override | ||
| public void methodAdvice(MethodTransformer transformer) { | ||
| transformer.applyAdvice(isConstructor(), getClass().getName() + "$Construct"); | ||
| } | ||
|
|
||
| public static final class Construct { | ||
| @Advice.OnMethodExit(suppress = Throwable.class) | ||
| public static void captureScope(@Advice.This Runnable asyncCall) { | ||
| // AdviceUtils.capture is a no-op when async propagation is disabled or there's no active | ||
| // span — same behavior as the rest of the concurrent instrumentation. | ||
| capture(InstrumentationContext.get(Runnable.class, State.class), asyncCall); | ||
| } | ||
| } | ||
| } |
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.
the test can simply put under
test- no need to have a different test suite