Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24279fbd2a
ℹ️ 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.
The new matcher has no Netty bytecode reference. Unsupported versions pass Muzzle validation, so the inverse compatibility task fails and blocks CI.
🤖 Datadog Autotest · Commit 24279fb · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
🟢 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 |
|
@codex review |
There was a problem hiding this comment.
More details
Netty 4.1.44 and later keep the captured context until runTask() starts. Older Netty versions continue to activate the context in RunnableFuture.run().
🤖 Datadog Autotest · Commit a5ba81c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5ba81ce3d
ℹ️ 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
Regular and shaded delayed tasks retain context at execution time. Older Netty tasks keep the existing run path.
🤖 Datadog Autotest · Commit 3a9cabf · 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a9cabf941
ℹ️ 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".
|
@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. What shall we delve into next? 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". |
sarahchen6
left a comment
There was a problem hiding this comment.
Left a small comment about scope but otherwise looks reasonable to me!
btw - looks like the failing CI jobs can be fixed by adding
testImplementation project(':dd-java-agent:instrumentation:netty:netty-concurrent-4.0')
to dd-java-agent/instrumentation/armeria/armeria-grpc-0.84/build.gradle. Seems like Armeria's use of java-concurrent now depend on NettyPromiseTaskInstrumentation and require netty-concurrent-4.0..... "Armeria’s tests load java-concurrent, which still captures continuations when Netty scheduled tasks are constructed. This PR makes that instrumentation skip ScheduledFutureTask.run(). So, Activation and release now depend on NettyPromiseTaskInstrumentation in netty-concurrent-4.0."
| : null; | ||
| } | ||
|
|
||
| @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) |
There was a problem hiding this comment.
from Codex review and seems reasonable to me - this closes the scope when runTask() returns or throws. Netty subsequently calls setSuccessInternal() or setFailureInternal(), which notify completion listeners. Those callbacks now execute after the scheduling context has been removed. - so it seems like we can lose span context / disconnect traces here
What Does This Do
Fixes context propagation for delayed Netty scheduled tasks while moving the Netty-specific execution handling out of the generic
RunnableFutureadvice.Starting with Netty 4.1.44,
ScheduledFutureTask.run()may be called once while a delayed task is being enqueued and again when the delay expires. Activating the task scope during the enqueue-time call consumes the captured continuation, leaving no context available when the task actually executes.For Netty versions that expose
PromiseTask.runTask(), the genericRunnableFutureinstrumentation no longer activates context aroundScheduledFutureTask.run(). Thenetty-concurrentinstrumentation insteadactivates the existing
RunnableFuturestate aroundPromiseTask.runTask(), which is the point where the task body actually executes.Older Netty versions without
runTask()continue using the existing genericRunnableFuture.run()Instrumentation.This approach:
RunnableFuture -> Statecontext store.Motivation
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]