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 10 commits into
Conversation
ygree
added
type: bug fix
This comment has been minimized.
This comment has been minimized.
🟢 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.
| ctx.write(msg, writePromise); | ||
| if (terminalResponse) { | ||
| // Run the request-ended callbacks (AppSec/IAST) here, while the context is attached | ||
| beforeFinish(serverContext, storedContext); | ||
| } |
There was a problem hiding this comment.
Resolved: terminal responses no longer call beforeFinish(...) immediately after ctx.write(...). The write-promise listener is now the only normal terminal completion path, so finishSpan(...) runs after the write completes and invokes beforeFinish(...) there.
|
@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".
| if (msg instanceof LastHttpContent | ||
| || isBodylessResponse(serverContext, response) | ||
| || isWebsocketUpgrade) { | ||
| return true; |
There was a problem hiding this comment.
When a Netty app writes a FullHttpResponse without Content-Length or chunked encoding, the HTTP/1.x response is still close-delimited, but this branch treats it as terminal just because FullHttpResponse implements LastHttpContent. That removes the context and finishes the span on the write promise before the !hasKnownBodyLength(response) path can mark it close-delimited, so these responses still get the too-short duration this change is meant to fix whenever completion is signaled by closing the connection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Changed HttpServerResponseTracingHandler.java so unknown-length responses are marked close-delimited before LastHttpContent can make them terminal. Close-delimited contexts now wait for channelInactive.
Added a regression in NettyChunkedResponseSpanTest.java for DefaultFullHttpResponse with no Content-Length and no chunked encoding.
| if (!hasKnownBodyLength(response)) { | ||
| serverContext.markResponseCloseDelimited(); |
There was a problem hiding this comment.
In the HTTP/2 server path wired through Http2StreamFrameToHttpObjectCodec, a response without Content-Length or chunked encoding is normally delimited by END_STREAM/LastHttpContent, not by closing the channel. Marking every such response as close-delimited means an HTTP/2 stream that starts a response and then closes before LastHttpContent is reported as a clean span instead of the incomplete-response error this change applies to HTTP/1.x chunked/known-length bodies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Restrict close-delimited response handling to HTTP/1.x only.
The Netty HTTP/2 server path now caches when Http2StreamFrameToHttpObjectCodec is present and avoids marking no-length responses as close-delimited there. This keeps HTTP/2 responses dependent on LastHttpContent/END_STREAM, so an early stream close is still reported as an incomplete-response error.
| private static boolean isWebsocketUpgrade(final HttpResponse response) { | ||
| return response.status() == HttpResponseStatus.SWITCHING_PROTOCOLS | ||
| && response | ||
| .headers() | ||
| .containsValue(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET, true); |
There was a problem hiding this comment.
If a server constructs a valid websocket upgrade with a distinct HttpResponseStatus instance for code 101, this identity check returns false even though the status code is Switching Protocols. The new informational-response branch then treats that terminal upgrade as an interim 1xx, skips onResponse, and leaves the request context open until the upgraded channel eventually closes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The Netty 4.1 websocket upgrade check is now value-based for status 101, and the regression test for a distinct HttpResponseStatus(101, ...) passes.
| if (!serverContext.isResponseCloseDelimited()) { | ||
| // The response declared a body via Content-Length or chunked encoding, but the channel closed | ||
| // before that body completed. Close-delimited responses, in contrast, end normally when the | ||
| // connection closes. | ||
| DECORATE.onError(span, new IllegalStateException(INCOMPLETE_RESPONSE_MESSAGE)); |
There was a problem hiding this comment.
When a server writes headers with Content-Length, streams the body as ByteBuf/FileRegion, and then closes the connection instead of sending a LastHttpContent sentinel, the response can be fully sent even though responseCloseDelimited is false. This close path will mark every such span as an incomplete-response error, which regresses valid non-keepalive/file responses that complete by closing after the known-length body is written.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It now treats known-length responses as complete once the declared body bytes are successfully written, so closing the connection afterward no longer marks the span as an incomplete-response error. Chunked/incomplete known-length closes still error as expected.
Tests passed for NettyChunkedResponseSpanTest; changes are not committed.
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.
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.
- 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.
immediately after `ctx.write(...)`. The write-promise listener is now the only normal terminal completion path, so `finishSpan(...)` runs after the write completes and invokes `beforeFinish(...)` there. The exception path is also guarded with `writePromise.isDone()`: if the terminal write promise has already completed, the listener has already handled finishing, so the catch block just rethrows and avoids a second finish/decorator pass.
responses are marked close-delimited before `LastHttpContent` can make them terminal. Close-delimited contexts now wait for `channelInactive`. Added a regression in NettyChunkedResponseSpanTest.java for `DefaultFullHttpResponse` with no `Content-Length` and no chunked encoding.
The Netty HTTP/2 server path now caches when `Http2StreamFrameToHttpObjectCodec` is present and avoids marking no-length responses as close-delimited there. This keeps HTTP/2 responses dependent on `LastHttpContent`/END_STREAM, so an early stream close is still reported as an incomplete-response error.
`101`, and the regression test for a distinct `HttpResponseStatus(101, ...)` passes.
bytes are successfully written, so closing the connection afterward no longer marks the span as an incomplete-response error. Chunked/incomplete known-length closes still error as expected. Tests passed for `NettyChunkedResponseSpanTest`; changes are not committed.
efe7144 to
cde82d0
Compare
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.
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