From 9d3e164c93c9d527ae0c491c1210e22076740387 Mon Sep 17 00:00:00 2001 From: Stuart McCulloch Date: Mon, 20 Jul 2026 18:15:03 +0100 Subject: [PATCH] Remove noopScope() usage from vertx-redis-client advice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../RedisAPICallAdvice.java | 22 +++++++-------- .../RedisFutureSendAdvice.java | 28 ++++++++++--------- .../vertx_redis_client/RedisSendAdvice.java | 18 +++++++----- .../src/test/groovy/VertxRedisTestBase.groovy | 6 ++++ 4 files changed, 43 insertions(+), 31 deletions(-) diff --git a/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisAPICallAdvice.java b/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisAPICallAdvice.java index 179ef81e0ca..8fd7180aeee 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisAPICallAdvice.java +++ b/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisAPICallAdvice.java @@ -22,10 +22,9 @@ public class RedisAPICallAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static boolean beforeCall( + public static AgentScope beforeCall( @Advice.Origin final Method currentMethod, @Advice.This final RedisAPI self, - @Advice.Local("callScope") AgentScope scope, @Advice.Argument( value = 0, readOnly = false, @@ -62,7 +61,7 @@ public static boolean beforeCall( // either), so this seems to be the only way to communicate that we have already wrapped // the handler. :( if (CallDepthThreadLocalMap.incrementCallDepth(RedisAPI.class) > 0) { - return true; + return null; } // TODO what is the recreated for every read about in the @Advice.Origin javadoc? @@ -100,7 +99,7 @@ public static boolean beforeCall( } if (null == handler || handler instanceof ResponseHandlerWrapper) { - return true; + return null; } final AgentSpan parentSpan = activeSpan(); @@ -112,7 +111,7 @@ public static boolean beforeCall( The potential racy condition when the handler may be added to an already finished task is handled by RedisAPIImplSendAdvice. */ - scope = activateSpan(clientSpan); + AgentScope scope = activateSpan(clientSpan); ResponseHandlerWrapper respHandler = new ResponseHandlerWrapper(handler, clientSpan, parentContinuation); handler = respHandler; @@ -139,23 +138,24 @@ public static boolean beforeCall( Store the response handler in the context so that it can be retrieved in RedisAPIImplSendAdvice */ InstrumentationContext.get(RedisAPI.class, ResponseHandlerWrapper.class).put(self, respHandler); - return true; + return scope; } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void afterCall( @Advice.Thrown final Throwable throwable, @Advice.This final RedisAPI self, - @Advice.Local("callScope") AgentScope scope, - @Advice.Enter final boolean decrement) { - if (decrement) { - CallDepthThreadLocalMap.decrementCallDepth(RedisAPI.class); + @Advice.Enter final AgentScope scope) { + CallDepthThreadLocalMap.decrementCallDepth(RedisAPI.class); + + if (null == scope) { + return; } scope.close(); // Clean the response handler from the context - InstrumentationContext.get(RedisAPI.class, ResponseHandlerWrapper.class).put(self, null); + InstrumentationContext.get(RedisAPI.class, ResponseHandlerWrapper.class).remove(self); } // Only apply this advice for versions that we instrument 3.9.x diff --git a/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisFutureSendAdvice.java b/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisFutureSendAdvice.java index 9d963a7228b..4777bd3c4dc 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisFutureSendAdvice.java +++ b/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisFutureSendAdvice.java @@ -3,7 +3,6 @@ import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.captureSpan; -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopScope; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpan; import static datadog.trace.instrumentation.vertx_redis_client.VertxRedisClientDecorator.DECORATE; import static datadog.trace.instrumentation.vertx_redis_client.VertxRedisClientDecorator.REDIS_COMMAND; @@ -32,6 +31,10 @@ public static AgentScope beforeSend( @Advice.Argument(value = 0, readOnly = false) Request request, @Advice.Local("ddParentContinuation") ContextContinuation parentContinuation) throws Throwable { + // If we had already wrapped the innermost handler in the RedisAPI call, then we should + // not wrap it again here. See comment in RedisAPICallAdvice + boolean nested = CallDepthThreadLocalMap.incrementCallDepth(RedisAPI.class) > 0; + ContextStore ctxt = InstrumentationContext.get(Request.class, Boolean.class); Boolean handled = ctxt.get(request); if (null != handled && handled) { @@ -44,6 +47,12 @@ public static AgentScope beforeSend( } ctxt.put(request, Boolean.TRUE); + // Mark the request handled even when nested, so a later async re-send of the same + // Request (e.g. via a pooled connection) isn't mistaken for a brand-new command. + if (nested) { + return null; + } + AgentSpan parentSpan = activeSpan(); if (parentSpan != null && REDIS_COMMAND.equals(parentSpan.getOperationName())) { @@ -53,12 +62,6 @@ public static AgentScope beforeSend( parentContinuation = null == parentSpan ? captureSpan(noopSpan()) : captureSpan(parentSpan); - // If we had already wrapped the innermost handler in the RedisAPI call, then we should - // not wrap it again here. See comment in RedisAPICallAdvice - if (CallDepthThreadLocalMap.incrementCallDepth(RedisAPI.class) > 0) { - return noopScope(); - } - final AgentSpan clientSpan = DECORATE.startAndDecorateSpan( request.command(), InstrumentationContext.get(Command.class, UTF8BytesString.class)); @@ -72,20 +75,19 @@ public static void afterSend( @Advice.Local("ddParentContinuation") ContextContinuation parentContinuation, @Advice.Enter final AgentScope clientScope, @Advice.This final Object thiz) { + CallDepthThreadLocalMap.decrementCallDepth(RedisAPI.class); if (thiz instanceof RedisConnection) { final SocketAddress socketAddress = InstrumentationContext.get(RedisConnection.class, SocketAddress.class) .get((RedisConnection) thiz); final AgentSpan span = clientScope != null ? clientScope.span() : activeSpan(); - - if (socketAddress != null && span != null) { - final AgentSpan spanWithConnection = clientScope == noopScope() ? activeSpan() : span; - DECORATE.onConnection(spanWithConnection, socketAddress); - DECORATE.setPeerPort(spanWithConnection, socketAddress.port()); + // Verify the activeSpan() fallback is actually a REDIS_COMMAND span + if (socketAddress != null && span != null && REDIS_COMMAND.equals(span.getOperationName())) { + DECORATE.onConnection(span, socketAddress); + DECORATE.setPeerPort(span, socketAddress.port()); } } if (clientScope != null) { - CallDepthThreadLocalMap.decrementCallDepth(RedisAPI.class); Promise promise = Promise.promise(); responseFuture.onComplete( new ResponseHandler(promise, clientScope.span(), parentContinuation)); diff --git a/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisSendAdvice.java b/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisSendAdvice.java index 88acf8e8109..51bd22ddc38 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisSendAdvice.java +++ b/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/main/java/datadog/trace/instrumentation/vertx_redis_client/RedisSendAdvice.java @@ -3,7 +3,6 @@ import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.captureSpan; -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopScope; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.noopSpan; import static datadog.trace.instrumentation.vertx_redis_client.VertxRedisClientDecorator.DECORATE; import static datadog.trace.instrumentation.vertx_redis_client.VertxRedisClientDecorator.REDIS_COMMAND; @@ -33,6 +32,10 @@ public static AgentScope beforeSend( @Advice.Argument(value = 0, readOnly = false) Request request, @Advice.Argument(value = 1, readOnly = false) Handler> handler) throws Throwable { + // If we had already wrapped the innermost handler in the RedisAPI call, then we should + // not wrap it again here. See comment in RedisAPICallAdvice + boolean nested = CallDepthThreadLocalMap.incrementCallDepth(RedisAPI.class) > 0; + if (null == handler || handler instanceof ResponseHandlerWrapper) { return null; } @@ -49,10 +52,10 @@ public static AgentScope beforeSend( } ctxt.put(request, Boolean.TRUE); - // If we had already wrapped the innermost handler in the RedisAPI call, then we should - // not wrap it again here. See comment in RedisAPICallAdvice - if (CallDepthThreadLocalMap.incrementCallDepth(RedisAPI.class) > 0) { - return noopScope(); + // Mark the request handled even when nested, so a later async re-send of the same + // Request (e.g. via a pooled connection) isn't mistaken for a brand-new command. + if (nested) { + return null; } AgentSpan parentSpan = activeSpan(); @@ -74,18 +77,19 @@ public static AgentScope beforeSend( @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void afterSend( @Advice.Enter final AgentScope clientScope, @Advice.This final Object thiz) { + CallDepthThreadLocalMap.decrementCallDepth(RedisAPI.class); if (thiz instanceof RedisConnection) { final SocketAddress socketAddress = InstrumentationContext.get(RedisConnection.class, SocketAddress.class) .get((RedisConnection) thiz); final AgentSpan span = clientScope != null ? clientScope.span() : activeSpan(); - if (socketAddress != null && span != null) { + // Verify the activeSpan() fallback is actually a REDIS_COMMAND span + if (socketAddress != null && span != null && REDIS_COMMAND.equals(span.getOperationName())) { DECORATE.onConnection(span, socketAddress); DECORATE.setPeerPort(span, socketAddress.port()); } } if (null != clientScope) { - CallDepthThreadLocalMap.decrementCallDepth(RedisAPI.class); clientScope.close(); } } diff --git a/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/test/groovy/VertxRedisTestBase.groovy b/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/test/groovy/VertxRedisTestBase.groovy index ab3275561f0..8c26b18680a 100644 --- a/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/test/groovy/VertxRedisTestBase.groovy +++ b/dd-java-agent/instrumentation/vertx/vertx-redis-client/vertx-redis-client-3.9/src/test/groovy/VertxRedisTestBase.groovy @@ -3,6 +3,7 @@ import datadog.trace.agent.test.asserts.ListWriterAssert import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.naming.VersionedNamingTestBase import datadog.trace.api.DDSpanTypes +import datadog.trace.bootstrap.CallDepthThreadLocalMap import datadog.trace.bootstrap.instrumentation.api.Tags import datadog.trace.core.DDSpan import io.vertx.core.AsyncResult @@ -12,6 +13,7 @@ import io.vertx.core.Vertx import io.vertx.core.VertxOptions import io.vertx.redis.client.Command import io.vertx.redis.client.Redis +import io.vertx.redis.client.RedisAPI import io.vertx.redis.client.Request import io.vertx.redis.client.Response import org.testcontainers.containers.wait.strategy.Wait @@ -75,6 +77,10 @@ abstract class VertxRedisTestBase extends VersionedNamingTestBase { cleanUpTraces() } + def cleanup() { + assert CallDepthThreadLocalMap.getCallDepth(RedisAPI) == 0 + } + void cleanUpTraces() { def cleanupSpan = runUnderTrace("cleanup") { redis.send(Request.cmd(Command.FLUSHALL), Promise.promise())