Fix Netty 4.1 server span completion for streaming responses#11941
Fix Netty 4.1 server span completion for streaming responses#11941ygree wants to merge 5 commits into
Conversation
|
🎯 Code Coverage (details) 🔗 Commit SHA: efe7144 | 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. |
5e8efd4 to
8a32be2
Compare
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.
8a32be2 to
f65d4a4
Compare
Keep Netty 4.1 server spans open until terminal response writes while preserving request-ended callback timing for AppSec/IAST. Track one-shot beforeFinish execution separately from span finish so streaming spans get correct duration without dropping request-end security tags.
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
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".
- Defer beforeFinish until the response actually completes. - Finish close-delimited responses without error. - Guard Content-Length parsing. - Skip AppSec response analysis for all interim 1xx responses in MaybeBlockResponseHandler, not just 100 Continue. Add a regression test for close-delimited responses.
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
What Does This Do
Updates Netty 4.1 server instrumentation to keep request spans open until the HTTP response is actually complete.
It tracks pending HTTP/1.1 request contexts in FIFO order, finishes streaming/chunked responses on
LastHttpContent, finishes header-complete responses such as204/205/304,HEAD,CONNECT,Content-Length: 0, and WebSocket upgrades at the response headers, keeps spans open across interim1xxresponses (e.g.100 Continue,103 Early Hints), finishes connection-close-delimited responses (noContent-Length, not chunked) normally when the channel closes, and marks a response as an error only when it declared a body that never completed before the channel closed.Motivation
Netty 4.1 server spans could finish when the first response headers/chunk were written instead of when the response completed. That made streaming response durations too short and could misattribute spans on keep-alive or pipelined HTTP/1.1 connections.
Additional Notes
The fix avoids relying on a single channel-level streaming context because that is not sufficient for pipelined requests. Response writes are matched to the queued request context instead.
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