Keep Netty 4.1 server spans open until streaming responses complete - #11941
Keep Netty 4.1 server spans open until streaming responses complete#11941ygree wants to merge 42 commits into
Conversation
ygree
added
type: bug fix
|
🎯 Code Coverage (details) 🔗 Commit SHA: a5af354 | 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. |
8a32be2 to
f65d4a4
Compare
![datadog-datadog-prod-us1-2[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 refactors Netty 4.1 server span lifecycle to keep spans open until response completion instead of finishing on first headers. A FIFO queue ensures pipelined requests match responses correctly, and guards protect against double-finishing. All critical scenarios (chunked responses, bodyless responses, pipelined requests, connection drops) are tested and pass validation. No behavioral regressions found in diff analysis or test coverage review.
📊 Validated against 7 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit de23316 · What is Autotest? · Any feedback? Reach out in #autotest
![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: de233160c2
ℹ️ 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".
There was a problem hiding this comment.
More details
The PR's isTerminalResponse logic was traced through 15 concrete response type/method combinations (FullHttpResponse, chunked, 1xx, 204/205/304, HEAD, CONNECT, WebSocket, close-delimited, Content-Length:0) and the channelInactive handler was verified against 6 channel-close scenarios. No path produces a span leak, a double-reported span under normal conditions, or a misclassified terminal/non-terminal response. The PR also silently fixes two pre-existing bugs: 103 Early Hints was incorrectly treated as terminal in the old code (consuming the one-shot AppSec analysis slot), and Upgrade: WebSocket with a capital W was not recognised as a WebSocket upgrade due to a case-sensitive string comparison.
📊 Validated against 26 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit efe7144 · What is Autotest? · Any feedback? Reach out in #autotest
|
BTW, @ygree , |
There was a problem hiding this comment.
Pull request overview
Updates the Netty 4.1 server instrumentation span lifecycle so request spans stay open until the HTTP response is actually complete (including chunked/streaming bodies and connection-close-delimited responses), with added tests covering pipelining and multiple completion edge cases.
Changes:
- Extend
ServerRequestContextto track request method and response/finish state needed to decide when a span can be completed. - Update Netty 4.1 request/response handlers to match responses to queued request contexts and finish spans on
LastHttpContent, header-only terminals, or channel close (with error only for incomplete declared bodies). - Add/refactor JUnit tests to validate chunked streaming, interim 1xx handling, header-only responses, close-delimited completion, and pipelined request ordering.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dd-java-agent/instrumentation/netty/netty-common/src/main/java/datadog/trace/instrumentation/netty41/ServerRequestContext.java | Adds request method + response lifecycle flags and supports removing all queued contexts on channel close. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/HttpServerResponseTracingHandler.java | Defers span completion to response terminal conditions (incl. LastHttpContent), and supports close-delimited responses. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/HttpServerRequestTracingHandler.java | Enqueues full HttpRequest context (method-aware) and finishes/publishes pending spans on channel close with incomplete-body error handling. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/server/MaybeBlockResponseHandler.java | Avoids consuming one-shot response analysis on interim 1xx responses (except 101). |
| dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/server/NettyHttpServerTestSupport.java | New shared Netty server + raw socket HTTP parsing utilities for tests. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/server/NettyHttp11PipeliningTest.java | Refactors pipelining test to reuse shared server/socket helpers. |
| dd-java-agent/instrumentation/netty/netty-4.1/src/test/java/datadog/trace/instrumentation/netty41/server/NettyChunkedResponseSpanTest.java | New tests asserting span lifecycle correctness for chunked/streaming, interim 1xx, header-only, websocket upgrade, and close-delimited responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efe7144263
ℹ️ 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.
Keep server spans open until terminal response writes, finish incomplete responses on channel close, and use queued request contexts to avoid keep-alive/pipelining misattribution. Cover chunked, bodyless, HEAD, CONNECT, interim, and WebSocket upgrade response cases.
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.
… ygree/fix-netty41-chunked-tracing
Finish server spans and dequeue pipelined request contexts only when Netty writes LastHttpContent. Use the terminal write promise so asynchronous failures retain error reporting and request context. Remove Content-Length, bodyless-response, request-method, and close-delimited completion inference, along with synthetic terminators and the HTTP/2 marker used by that model. Preserve channel-close cleanup for responses that never reach terminal content. Update pipelining, AppSec, HTTP/2, framing, and asynchronous write tests for the terminal-content contract.
5a106cf to
ffb560d
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
@gtukmachev Could you test the latest snapshot? There have been a few changes since the last one. |
3332358 to
ffb560d
Compare
… ygree/fix-netty41-chunked-tracing
ygree
changed the title
The agent jar exceeds the previous budget by about 1 KiB. Raise the budget to the next 16 KiB boundary to pass the verification check.
There was a problem hiding this comment.
More details
The request-context FIFO and terminal-write lifecycle changes were reviewed across pipelined, interim-response, close-delimited, header-only, websocket, deferred-blocking, and delayed-write-failure paths. No clear behavioral regression was identified; local execution could not start because the sandbox has no Java 25 toolchain.
🤖 Datadog Autotest · Commit 952e59b · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
964c376 to
df8095a
Compare
| # Max agent jar size in bytes. Raise only when the size growth is intentional (~33.02 MiB). | ||
| jar.size.budget = 34619392 | ||
| # Max agent jar size in bytes. Raise only when the size growth is intentional (~33.03 MiB). | ||
| jar.size.budget = 34635776 |
There was a problem hiding this comment.
The limit was exceeded due to a normal size increase: Jar size 34621751 B exceeds budget 34619392 B.
Increased the limit by 16Kb.
4565ac1 to
3191577
Compare
3191577 to
670d5c4
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
vandonr
left a comment
vandonr
left a comment
There was a problem hiding this comment.
looks ok, idk if we want to wait for customer's feedback after your latest changes before merging ?
What Does This Do
Updates Netty 4.1 server instrumentation to finish server spans when the final
LastHttpContentwrite completes. It preserves spans across interim1xxresponses, reports asynchronous terminal-write failures as errors, and finishes remaining response spans normally when the channel closes without inferring framing errors fromContent-Length.Motivation
Netty response writes can complete asynchronously. Finishing a server span when the final response content is submitted, rather than when its write completes, can underreport response duration and miss asynchronous write failures.
Additional Notes
This change builds on the FIFO request-context tracking introduced by #11937.
Based on: #11937
Supersedes: #10734
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: APMS-20050