Conversation
amarziali
added
type: bug fix
amarziali
added
the
tag: ai generated
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
A custom body subscriber can throw from onSubscribe or onNext. The held continuation then has no release path, so it can keep trace context and stop trace reporting.
🤖 Datadog Autotest · Commit 257fe75 · What is Autotest? · @DataDog review to ask questions · 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: 257fe750c3
ℹ️ 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".
|
🎯 Code Coverage (details) 🔗 Commit SHA: d663073 | Docs | View more details | 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. |
|
@DataDog review |
There was a problem hiding this comment.
More details
The held continuation stays available for each response-body callback. Each terminal path releases it one time.
🤖 Datadog Autotest · Commit 72169ba · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
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". |
jordan-wong
left a comment
jordan-wong
left a comment
There was a problem hiding this comment.
LGTM, left one non-blocking comment about potential test coverage
| private int released; | ||
|
|
||
| @Override | ||
| public ContextContinuation hold() { |
There was a problem hiding this comment.
This RecordingContinuation test class for ContextContinuation doesn't track the core behavior that was changed with .hold() in this PR - may be worth looking into adding modeling for hold and release behavior in tests?
Claude suggests this for the test class:
private static final class RecordingContinuation implements ContextContinuation {
private boolean held;
private int released;
@Override
public ContextContinuation hold() {
held = true;
return this;
}
@Override
public Context context() {
return null;
}
@Override
public ContextScope resume() {
// Real contract: without hold(), the resumed scope's close() implicitly releases once.
// With hold(), close() does NOT release — release() must be called explicitly.
return new ContextScope() {
private boolean closed;
@Override
public Context context() {
return null;
}
@Override
public void close() {
if (!closed && !held) {
closed = true;
released++;
}
}
};
}
@Override
public void release() {
released++;
}
}
3 participants
What Does This Do
Keeps the Java HTTP client context available across all asynchronous response-body callbacks.
The body subscriber previously used a one-shot continuation. The first callback resolved it, so later callbacks could run without the expected context and the trace could be reported before response processing
completed.
The continuation is now held until a terminal event:
onCompleteonErrorA shared terminal guard ensures concurrent or repeated terminal signals release it exactly once.
sequenceDiagram participant Client as HTTP client participant Wrapper as BodySubscriber wrapper participant Context as Trace continuation participant Delegate as Body subscriber Client->>Wrapper: onSubscribe(subscription) Wrapper->>Delegate: onSubscribe(wrapped subscription) loop Response chunks Client->>Wrapper: onNext(chunk) Wrapper->>Context: resume() Wrapper->>Delegate: onNext(chunk) Wrapper->>Context: close resumed scope end alt Response completes Client->>Wrapper: onComplete() Wrapper->>Context: resume() Wrapper->>Delegate: onComplete() Wrapper->>Context: release once else Response fails Client->>Wrapper: onError(error) Wrapper->>Context: resume() Wrapper->>Delegate: onError(error) Wrapper->>Context: release once else Subscription is cancelled Delegate->>Wrapper: cancel() Wrapper->>Client: delegate cancel() Wrapper->>Context: release once endMotivation
Java HTTP response bodies can invoke
onNextmultiple times. Their tracing context must remain available until body processing actually terminates, including cancellation.Additional Notes
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]