Skip to content

Fix Netty 4.1 HTTP/1.1 pipelined response tracing#11937

Open
ygree wants to merge 3 commits into
masterfrom
ygree/fix-netty41-http11-pipelining-context-queue
Open

Fix Netty 4.1 HTTP/1.1 pipelined response tracing#11937
ygree wants to merge 3 commits into
masterfrom
ygree/fix-netty41-http11-pipelining-context-queue

Conversation

@ygree

@ygree ygree commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

What Does This Do

Fixes Netty 4.1 HTTP/1.1 pipelining by storing server request state in a per-channel queue instead of a single overwritten channel attribute.

Each queued entry now carries the tracing context, request headers, and AppSec response blocking state, so outbound responses are matched with the correct inbound request.

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 caused responses to finish or annotate the wrong span, losing trace context for pipelined requests.

Additional Notes

  • HTTP/1.1 pipelined responses are matched to request contexts in FIFO order.
  • The existing CONTEXT_ATTRIBUTE_KEY is still mirrored for generic Netty fire* span activation.
  • If a channel exceeds the pending server request context limit, pending contexts are closed and server tracing is disabled for that channel to avoid unbounded retention or response misattribution.
  • The per-channel state is removed on channelInactive.

Contributor Checklist

Jira ticket: [PROJ-IDENT]

@ygree ygree self-assigned this Jul 13, 2026
@ygree ygree added type: bug fix Bug fix inst: netty Netty instrumentation labels Jul 13, 2026
@datadog-datadog-us1-prod

datadog-datadog-us1-prod Bot commented Jul 13, 2026

Copy link
Copy Markdown

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 69.73% (+12.68%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 57f9d65 | Docs | Datadog PR Page | Give us feedback!

@dd-octo-sts

dd-octo-sts Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 13.96 s 13.90 s [-0.1%; +1.1%] (no difference)
startup:insecure-bank:tracing:Agent 12.94 s 12.94 s [-0.6%; +0.6%] (no difference)
startup:petclinic:appsec:Agent 16.98 s 16.59 s [+1.3%; +3.4%] (significantly worse)
startup:petclinic:iast:Agent 16.85 s 16.96 s [-1.6%; +0.2%] (no difference)
startup:petclinic:profiling:Agent 16.84 s 16.34 s [-1.5%; +7.6%] (no difference)
startup:petclinic:sca:Agent 16.89 s 16.35 s [-1.2%; +7.8%] (no difference)
startup:petclinic:tracing:Agent 16.05 s 16.20 s [-1.6%; -0.2%] (maybe better)

Commit: 57f9d657 · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

@ygree ygree force-pushed the ygree/fix-netty41-http11-pipelining-context-queue branch from 6f4df04 to 7f7fdf4 Compare July 13, 2026 23:23
@ygree ygree marked this pull request as ready for review July 14, 2026 00:10
@ygree ygree requested a review from a team as a code owner July 14, 2026 00:10
@ygree ygree requested review from jordan-wong and removed request for a team July 14, 2026 00:10

@datadog-datadog-us1-prod datadog-datadog-us1-prod Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Datadog Autotest: PASS

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.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Datadog Autotest · Commit 7f7fdf4 · What is Autotest? · Any feedback? Reach out in #autotest

@ygree ygree added the tag: ai generated Largely based on code generated by an AI or LLM label Jul 14, 2026
@mcculls mcculls requested a review from Copilot July 16, 2026 07:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ServerRequestContext to 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.


/** Per-request server state stored on the channel until the matching response is written. */
public final class ServerRequestContext {
/** Returns whether a new server request can be tracked on this channel. */
Comment on lines +68 to +75
final ServerRequestContext currentContext = contexts.peekLast();
if (currentContext == null) {
attributes.attr(CONTEXT_ATTRIBUTE_KEY).remove();
} else {
// Keep the legacy mirror pointed at the current inbound request after removing an older
// response context.
attributes.attr(CONTEXT_ATTRIBUTE_KEY).set(currentContext.tracingContext());
}
Comment on lines +154 to +165
private final Context tracingContext;
private final HttpHeaders requestHeaders;
private boolean responseAnalyzed;
private boolean responseBlocked;

public Context tracingContext() {
return tracingContext;
}

public HttpHeaders requestHeaders() {
return requestHeaders;
}
@mcculls

mcculls commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +49 to +50
if (serverContext.isResponseAnalyzed()) {
if (serverContext.isResponseBlocked()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep dropping writes after a response block

When AppSec blocks a streaming response, this state now lives only on the head ServerRequestContext; the replacement blocking FullHttpResponse is written through HttpServerResponseTracingHandler, which immediately removes that context while the channel close is still asynchronous. Any subsequent original response chunks written before the close then see no blocked context (or the next pipelined request's context) and are forwarded instead of being released, regressing the previous channel-level blocked flag behavior for non-FullHttpResponse responses.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inst: netty Netty instrumentation tag: ai generated Largely based on code generated by an AI or LLM type: bug fix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants