Fix Netty 4.1 HTTP/1.1 pipelined response tracing#11937
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: 31ca152 | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
6f4df04 to
7f7fdf4
Compare
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The PR correctly fixes HTTP/1.1 pipelining by switching from single-attribute storage to a per-channel queue, allowing multiple in-flight requests to maintain independent tracing contexts. Code review of request queuing, response FIFO matching, error handling, and channel cleanup pathways reveals no behavioral regressions—the fix is architecturally sound.
🤖 Datadog Autotest · Commit 7f7fdf4 · What is Autotest? · Any feedback? Reach out in #autotest
ygree
added
the
tag: ai generated
There was a problem hiding this comment.
Pull request overview
Fixes Netty 4.1 HTTP/1.1 pipelined response tracing by tracking per-request server state in a per-channel FIFO queue, so each outbound response is matched to the correct inbound request span/context (including AppSec response blocking state and request header context).
Changes:
- Introduces
ServerRequestContextto queue per-request tracing + AppSec state on the channel and safely disable tracing when backlog grows beyond a limit. - Updates Netty 4.1 server handlers to consume/advance this queue when writing responses and when blocking responses.
- Adds regression tests for HTTP/1.1 pipelining span creation and for the pending-context limit behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dd-java-agent/instrumentation/netty/netty-common/src/main/java/datadog/trace/instrumentation/netty41/ServerRequestContext.java | New per-channel FIFO queue of per-request server tracing/AppSec state for pipelined matching. |
| dd-java-agent/instrumentation/netty/netty-common/src/main/java/datadog/trace/instrumentation/netty41/AttributeKeys.java | Removes now-obsolete per-channel request/response state keys; exposes attributeKey for queue key creation. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/MaybeBlockResponseHandler.java | Switches AppSec response analysis/blocking bookkeeping to per-request queued context. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/HttpServerResponseTracingHandler.java | Finishes/removes the correct request span/context for each response via queued context. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/HttpServerRequestTracingHandler.java | Enqueues per-request context and drains all pending contexts on channel close. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyChannelPipelineInstrumentation.java | Ensures ServerRequestContext is injected as a helper alongside Netty 4.1 server/client helpers. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/NettyChannelHandlerContextInstrumentation.java | Adds helper injection for ServerRequestContext and minor refactor in fire* activation. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/ChannelFutureListenerInstrumentation.java | Adds helper injection for ServerRequestContext in the module helper set. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/ServerRequestContextTest.java | Unit test validating tracing is disabled and contexts are drained when the pending limit is exceeded. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/server/NettyHttp11PipeliningTest.java | Regression test asserting spans are created correctly for pipelined HTTP/1.1 requests/responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
![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: 57f9d65709
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Track server request contexts per pipelined response, snapshot only the Accept header, and keep AppSec response-block state channel-level so late streaming chunks are dropped until close.
|
@code review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6173156b9e
ℹ️ 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".
vandonr
left a comment
vandonr
left a comment
There was a problem hiding this comment.
looks good from afar, though do check codex's review comments because I'm not knowledgeable enough on netty or appsec to really tell if they are relevant
Fallback to the channel context mirror when no queued ServerRequestContext exists, so HTTP/2 multiplex child-channel responses still decorate and finish propagated server spans. Add regression coverage for the no-queue context-mirror path and document why CONTEXT_ATTRIBUTE_KEY is maintained.
65df490 to
88da30a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88da30a8cd
ℹ️ 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".
Defer blocked responses until the active HTTP/1.1 response is complete, including chunked/streaming responses that finish on LastHttpContent. Handle header-only responses such as HEAD, 204, 205, 304, and explicit Content-Length: 0 without requiring an application LastHttpContent. Also keep interim 1xx responses from completing the server span and add coverage for chunked, header-only, HEAD, interim, and malformed Content-Length cases.
Fall back to the mirrored channel context when HTTP/2 stream channels have no request context queue. Ignore informational responses until the final response and add regression coverage for response blocking.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 349a4d0054
ℹ️ 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".
vandonr
left a comment
vandonr
left a comment
There was a problem hiding this comment.
lol no issue before merging main, 2 comments afterwards 🙃
anyway, I didn't see issues myself, approving to unblock, I'll let you review the AI's comments if they make sense.
This removes HEAD-specific state, header parsing, and synthetic outbound messages while making pipelined response attribution follow Netty’s actual response boundaries.
f80bce1 to
78ebedf
Compare
Mark channels when a request block is committed so later pipelined requests are forwarded to the existing blocking handler and discarded. Serialize response-function handler installation on the event loop and clean up partially installed handlers on failure.
Snapshot request metadata before scheduling blocking responses instead of retaining and replaying reference-counted HTTP requests. Reject scheduled blocking responses when their server request context has already completed, preventing stale commits from blocking keep-alive connections.
7dd79a7 to
b81803e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
What Does This Do
Fixes Netty 4.1 HTTP/1.1 pipelining by storing server request state in a bounded, per-channel FIFO queue instead of a single overwritten channel attribute.
Each queued entry carries the tracing context, captured
Acceptheader, response-analysis state, and any deferred AppSec blocking response. Outbound responses are matched with the correct inbound requests and contexts are removed only when their final responses complete.Response completion follows Netty's outbound message lifecycle. Full responses complete directly because they implement
LastHttpContent; split responses complete when their terminalLastHttpContentis written. HEAD, bodyless, and zero-length response headers do not complete a request early, and informational 1xx responses do not start or finish the final response.The change preserves the existing mirrored channel context used for generic Netty
fire*span activation and HTTP/2 multiplex stream-channel propagation.Motivation
HTTP/1.1 pipelining can place multiple in-flight requests on the same Netty channel. The previous instrumentation kept only one server tracing context per connection, so later requests overwrote earlier request state before their responses were written. This could cause responses to finish or annotate the wrong span.
AppSec blocking responses also need to follow pipelined response ordering so a blocked request cannot overtake an earlier pending response.
Additional Notes
CONTEXT_ATTRIBUTE_KEYremains mirrored for generic Nettyfire*span activation and HTTP/2 stream-channel context propagation.channelInactive.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]