Remove noopScope() usage from vertx-redis-client advice#12008
Conversation
mcculls
added
tag: no release notes
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR updates the Vert.x Redis client (3.9) instrumentation advice to avoid using AgentTracer.noopScope() as a sentinel and to ensure the RedisAPI call-depth guard is always balanced, preventing call-depth leaks when spans are intentionally not created (e.g., when the parent span is already REDIS_COMMAND).
Changes:
- Move
CallDepthThreadLocalMapincrement/decrement to the first statements of enter/exit advice inRedisSendAdviceandRedisFutureSendAdvice, returningnullinstead ofnoopScope(). - Simplify
RedisAPICallAdviceby returning theAgentScopedirectly via@Advice.Enter(and guarding the now-possiblenullscope), plus cleaning the context store viaremove. - Add a regression assertion in the Vertx Redis test base to ensure the RedisAPI call-depth counter is balanced after each test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisSendAdvice.java | Ensures call-depth is incremented/decremented in a balanced way regardless of early returns; removes noopScope() usage. |
| dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisFutureSendAdvice.java | Same call-depth balancing and noopScope() removal for Future-based send path; simplifies connection-decoration logic accordingly. |
| dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisAPICallAdvice.java | Returns AgentScope via @Advice.Enter instead of a vestigial boolean/local, guards null scope, and removes the stored handler from context on exit. |
| dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/test/groovy/VertxRedisTestBase.groovy | Adds a per-test call-depth regression assertion (but currently overrides base cleanup() without calling super). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 4afb44d346
ℹ️ 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".
788d7b7 to
7330a9c
Compare
|
🎯 Code Coverage (details) 🔗 Commit SHA: 9d3e164 | Docs | Datadog PR Page | Give us feedback! |
1b85dab to
78a595e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
🟢 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. |
78a595e to
44bfa56
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Increment/decrement the RedisAPI call-depth guard as the first statement of enter/exit advice instead of returning AgentTracer.noopScope() mid-method. This fixes a call-depth leak on the "parent span already REDIS_COMMAND" branch. Also drops RedisAPICallAdvice's vestigial always-true boolean return and @Advice.Local in favor of returning the AgentScope directly via @Advice.Enter, guarding the now-possible null scope on close/cleanup. Since the depth increment now happens before the Request-handled dedup check, mark the Request as handled unconditionally on the nested path too, instead of skipping it — otherwise a later async re-send of the same Request (e.g. via a pooled connection) could be mistaken for a brand-new command and get double-spanned. Guard the activeSpan() fallback used for connection decoration so it only tags a span that is actually a REDIS_COMMAND span, rather than whatever happens to be active — this avoids mistagging an unrelated span with Redis peer/connection info when the fallback resolves to something other than the intended command span. Adds a regression test asserting the call-depth counter is balanced after each test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
44bfa56 to
9d3e164
Compare
dd-octo-sts
Bot
added
the
tag: ai generated
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
PR fixes a call-depth leak in Redis instrumentation by moving the call-depth increment to the first line of each advice method and making the corresponding decrement unconditional. The change prevents counter imbalance when early-returning on the "parent span already REDIS_COMMAND" branch. A new regression test asserts call-depth is balanced after each test, protecting against future leaks.
📊 Validated against 5 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 9d3e164 · What is Autotest? · Any feedback? Reach out in #autotest
What Does This Do
Remove
AgentTracer.noopScope()usage from vertx-redis-client advice.Increment/decrement the RedisAPI call-depth guard as the first statement of enter/exit advice instead of returning AgentTracer.noopScope() mid-method. This fixes a call-depth leak on the "parent span already REDIS_COMMAND" branch.
Also drops RedisAPICallAdvice's vestigial always-true boolean return and @Advice.Local in favor of returning the AgentScope directly via @Advice.Enter, guarding the now-possible null scope on close/cleanup.
Adds a regression test asserting the call-depth counter is balanced after each test.
Motivation
Aligns this advice with the common pattern of doing call-depth increment/decrement first, to avoid accidental imbalances.
This also removes a non-obvious use of
AgentTracer.noopScope()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]