[codex] Fix java.net.http async trace propagation#1214
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f44867b53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .onMethod("getUserHeaders") | ||
| .interceptedBy("org.bithon.agent.plugin.httpclient.javanethttp.interceptor.HttpRequestImpl$GetUserHeaders") |
There was a problem hiding this comment.
For sendAsync with a user executor that is not instrumented to carry TraceContextHolder (for example an executor that starts a raw new thread), this new getUserHeaders hook can run on the HTTP worker with no current trace context, causing HttpRequestImpl$GetUserHeaders to return without injecting traceparent. The previous headers() hook was hit while HttpClientImpl copied the request on the caller thread, so propagation did not depend on executor instrumentation; this drops distributed tracing for those async clients.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks. We evaluated this path and intentionally removed the fallback in e4a9ede. For now the expected model is that getUserHeaders propagates from the current worker TraceContextHolder, so the downstream server remains under the HTTP client's thread-pool/async-task span when executor context propagation is installed. Supporting uninstrumented executors would require extra request-copy handoff state on HttpRequestImpl, which we decided is too intrusive for the current scope.
There was a problem hiding this comment.
Pull request overview
This PR updates the java.net.http client instrumentation to correctly propagate trace context across HttpClient.sendAsync scheduling/execution boundaries, ensuring outbound header injection occurs at send-time under the correct worker/async-task context.
Changes:
- Switch
sendAsyncto create an attached client span and then detach it for safe async completion from the returnedCompletableFuture. - Move header propagation interception from
headers()toHttpRequestImpl#getUserHeaders()so injection happens at send-time. - Add/extend tests to validate async trace propagation and the new span-detach behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| agent/agent-plugins/httpclient/java-net-http/src/main/java/org/bithon/agent/plugin/httpclient/javanethttp/JavaNetHttpClientPlugin.java | Updates interceptor wiring to propagate headers via HttpRequestImpl#getUserHeaders() instead of headers(). |
| agent/agent-plugins/httpclient/java-net-http/src/main/java/org/bithon/agent/plugin/httpclient/javanethttp/interceptor/HttpRequestImpl$GetUserHeaders.java | Implements send-time header propagation by mutating returned HttpHeaders. |
| agent/agent-plugins/httpclient/java-net-http/src/main/java/org/bithon/agent/plugin/httpclient/javanethttp/interceptor/HttpClientImpl$SendAsync.java | Changes span lifecycle for sendAsync: create attached span, detach after submission, finish via CompletableFuture completion. |
| agent/agent-plugins-test/src/test/java/org/bithon/agent/plugins/test/httpclient/JavaNetHttpClientThreadPropagationInterceptorTest.java | Adds coverage to validate worker/async-task context propagation and correct parent/child span relationships. |
| agent/agent-observability/src/test/java/org/bithon/agent/observability/tracing/context/TraceSpanDetachTest.java | Adds tests around ITraceSpan.detach() behavior. |
| agent/agent-observability/src/main/java/org/bithon/agent/observability/tracing/context/ITraceSpan.java | Introduces detach() API on spans. |
| agent/agent-observability/src/main/java/org/bithon/agent/observability/tracing/context/impl/TracingSpan.java | Implements detach() by creating an equivalent span in a copied context. |
| agent/agent-observability/src/main/java/org/bithon/agent/observability/tracing/context/impl/TracingContext.java | Adds stack-detach support used by TracingSpan.detach(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Intercept the JDK's send-time user headers access to propagate tracing headers | ||
| forClass("jdk.internal.net.http.HttpRequestImpl") | ||
| .onMethod("headers") | ||
| .interceptedBy("org.bithon.agent.plugin.httpclient.javanethttp.interceptor.HttpRequest$Headers") | ||
| .build(), | ||
| forClass("jdk.internal.net.http.ImmutableHttpRequest") | ||
| .onMethod("headers") | ||
| .interceptedBy("org.bithon.agent.plugin.httpclient.javanethttp.interceptor.HttpRequest$Headers") | ||
| .onMethod("getUserHeaders") | ||
| .interceptedBy("org.bithon.agent.plugin.httpclient.javanethttp.interceptor.HttpRequestImpl$GetUserHeaders") | ||
| .build() |
There was a problem hiding this comment.
Done in de9e536: getUserHeaders now has an explicit zero-arg matcher via andArgsSize(0).
| LoggerFactory.getLogger(TracingContext.class).warn("Try to detach a span which is not in the stack. This IS a bug.\n" | ||
| + "Current span: \n{}, Unfinished Spans:\n{}", | ||
| span, | ||
| spanStack); | ||
| return false; |
There was a problem hiding this comment.
Done in de9e536: the warning now says the span is not the current stack top and includes the current stack-top span in the log message.
| public void testDetachReturnsCopiedContextEvenWhenSpanIsNotStackTop() { | ||
| ITraceContext context = TraceContextFactory.newContext(SamplingMode.FULL); | ||
| ITraceSpan rootSpan = context.currentSpan().name("root"); | ||
| ITraceSpan childSpan = rootSpan.newChildSpan("child"); | ||
| ITraceSpan grandchildSpan = childSpan.newChildSpan("grandchild"); | ||
|
|
||
| ITraceSpan detachedSpan = childSpan.detach(); | ||
|
|
||
| Assertions.assertNotSame(childSpan, detachedSpan); | ||
| Assertions.assertNotSame(childSpan.context(), detachedSpan.context()); | ||
| Assertions.assertEquals(childSpan.traceId(), detachedSpan.traceId()); | ||
| Assertions.assertEquals(childSpan.spanId(), detachedSpan.spanId()); | ||
| Assertions.assertEquals(childSpan.parentSpanId(), detachedSpan.parentSpanId()); | ||
| Assertions.assertSame(detachedSpan, detachedSpan.context().currentSpan()); | ||
|
|
||
| detachedSpan.finish(); | ||
| detachedSpan.context().finish(); | ||
|
|
||
| grandchildSpan.finish(); | ||
| childSpan.finish(); | ||
| rootSpan.finish(); | ||
| context.finish(); | ||
| } |
There was a problem hiding this comment.
Done in de9e536: the test now detaches only the current child span and finishes the returned detached span, avoiding finishing the original child span after detach.
| /** | ||
| * Detach this span from its current trace context and return a span with the same identity | ||
| * in a copied context so it can be finished asynchronously. | ||
| */ |
There was a problem hiding this comment.
Done in de9e536: the detach javadoc now documents the stack-top precondition and the lifecycle rule that callers should finish the returned span rather than the original reference.
| Object source = aopContext.getArgAs(0); | ||
| ITraceContext context = HttpRequestPropagationContext.get(source); | ||
| if (context != null) { | ||
| ((IBithonObject) aopContext.getTarget()).setInjectedObject(context); | ||
| } |
There was a problem hiding this comment.
This hook was removed in e4a9ede along with the fallback propagation path, so the cast no longer exists.
Summary
sendAsyncas an attached client span while JDKHttpClientschedules internal work.CompletableFuturewithout leaving it on the caller thread.HttpRequestImpl#getUserHeaders, so the remote server span is linked under the HTTP client worker/async-task context.Root Cause
The previous
sendAsyncspan used an async span that was not attached to the current trace context during the client executor submission path. Header propagation also happened atheaders(), which could run before the worker context existed.Validation
mvn -pl agent/agent-plugins-test -Dtest=JavaNetHttpClientThreadPropagationInterceptorTest,JavaNetHttpClientPluginInterceptorTest testmvn -pl agent/agent-observability -Dtest=TraceSpanDetachTest testmvn -pl agent/agent-distribution -am -DskipTests packagegit diff --check