Ensure tracer resources cleaned up in exit advice#11905
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7dc8d5123
ℹ️ 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".
|
🎯 Code Coverage (details) 🔗 Commit SHA: 01796fc | 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. |
fa80293 to
32a9fed
Compare
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR updates multiple ByteBuddy @Advice.OnMethodExit hooks across instrumentations so cleanup/finishing logic still runs when the instrumented method exits by throwing, preventing leaked scopes/spans and stale context-store entries.
Changes:
- Add
onThrowable = Throwable.classto various@Advice.OnMethodExitannotations so exit advice executes on exceptional control flow. - Capture
@Advice.Thrownin selected advices and record errors on spans (with special-case filtering where appropriate, e.g., KafkaWakeupException). - Ensure scope/context cleanup is performed from exit advice paths that previously only ran on normal returns.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dd-java-agent/instrumentation/servicetalk/servicetalk-0.42.0/.../ContextPreservingInstrumentation.java | Run scope-close exit advice even when the target method throws. |
| dd-java-agent/instrumentation/rs/jax-rs/jax-rs-annotations/jax-rs-annotations-2.0/.../JaxRsAsyncResponseInstrumentation.java | Run stop-span logic on exceptional exits; report thrown errors via decorator. |
| dd-java-agent/instrumentation/rs/jakarta-rs-annotations-3.0/.../JakartaRsAsyncResponseInstrumentation.java | Same as JAX-RS 2.x variant for Jakarta RS. |
| dd-java-agent/instrumentation/opensearch/opensearch-transport-1.0/.../ShadowExistingScopeAdvice.java | Ensure shadow scope is closed even if the method throws. |
| dd-java-agent/instrumentation/lettuce/lettuce-5.0/.../rx/RedisSubscriptionSubscribeAdvice.java | Ensure exit advice runs on throwable; (needs follow-up to finish/clear spans on thrown for response-expected commands). |
| dd-java-agent/instrumentation/lettuce/lettuce-5.0/.../rx/RedisSubscriptionCommandOnCompleteAdvice.java | Run completion cleanup on exceptional exit as well. |
| dd-java-agent/instrumentation/lettuce/lettuce-5.0/.../rx/RedisSubscriptionCommandErrorAdvice.java | Ensure error cleanup runs even if the error handler throws. |
| dd-java-agent/instrumentation/lettuce/lettuce-5.0/.../rx/RedisSubscriptionCommandCompleteAdvice.java | Run completion/cancel cleanup even on exceptional exit. |
| dd-java-agent/instrumentation/lettuce/lettuce-5.0/.../CommandHandlerInstrumentation.java | Ensure endTaskScope runs on exceptional exit from decode. |
| dd-java-agent/instrumentation/kafka/kafka-clients-3.8/.../RecordsAdvice.java | Finish/close scope on exceptional exit; record thrown error except WakeupException. |
| dd-java-agent/instrumentation/kafka/kafka-clients-0.11/.../KafkaConsumerInfoInstrumentation.java | Same as above for Kafka 0.11 instrumentation. |
| dd-java-agent/instrumentation/java/java-concurrent/java-concurrent-1.8/.../AsyncTaskInstrumentation.java | Ensure task scopes are ended even if run()/cancel() throw. |
| dd-java-agent/instrumentation/grizzly/grizzly-http-2.3.20/.../GrizzlyCharBodyInstrumentation.java | Ensure context-store cleanup runs even if recycle throws. |
| dd-java-agent/instrumentation/grizzly/grizzly-http-2.3.20/.../GrizzlyByteBodyInstrumentation.java | Same as above for byte-body storage. |
| dd-java-agent/instrumentation/gax-1.4/.../CallbackChainRetryingFutureInstrumentation.java | Run superseded-listener cancellation on exceptional exit. |
| dd-java-agent/instrumentation/elasticsearch/.../ShadowExistingScopeAdvice.java | Ensure shadow scope is closed even if the method throws. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32a9fed1bb
ℹ️ 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".
…p even if the patched method throws an exception
32a9fed to
01796fc
Compare
What Does This Do
Ensure tracer resources cleaned up in exit advice are still cleaned up even if the patched method throws an exception
Motivation
Discovered while using Claude to check codebase consistency.
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]