From e368334ba0df2ebcbae11c0cb782598f741c4855 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Mon, 2 Oct 2023 19:51:22 +0300 Subject: [PATCH] Make netty ChannelPipeline removeLast return user handler (#9584) --- ...bstractNettyChannelPipelineInstrumentation.java | 14 ++++++++++---- .../v4_0/NettyChannelPipelineInstrumentation.java | 3 +-- .../src/test/groovy/ChannelPipelineTest.groovy | 4 +++- .../v4_1/NettyChannelPipelineInstrumentation.java | 3 +-- .../src/test/groovy/ChannelPipelineTest.groovy | 4 +++- 5 files changed, 18 insertions(+), 10 deletions(-) diff --git a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4/common/AbstractNettyChannelPipelineInstrumentation.java b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4/common/AbstractNettyChannelPipelineInstrumentation.java index c2c65fb42d..eb530d6c2a 100644 --- a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4/common/AbstractNettyChannelPipelineInstrumentation.java +++ b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4/common/AbstractNettyChannelPipelineInstrumentation.java @@ -141,23 +141,29 @@ public abstract class AbstractNettyChannelPipelineInstrumentation implements Typ @Advice.OnMethodExit(suppress = Throwable.class) public static void removeHandler( - @Advice.This ChannelPipeline pipeline, @Advice.Return ChannelHandler handler) { + @Advice.This ChannelPipeline pipeline, + @Advice.Return(readOnly = false) ChannelHandler handler) { VirtualField virtualField = VirtualField.find(ChannelHandler.class, ChannelHandler.class); ChannelHandler ourHandler = virtualField.get(handler); if (ourHandler != null) { - pipeline.remove(ourHandler); + // Context is null when our handler has already been removed. This happens when calling + // removeLast first removed our handler and we called removeLast again to remove the http + // handler. + if (pipeline.context(ourHandler) != null) { + pipeline.remove(ourHandler); + } virtualField.set(handler, null); } else if (handler .getClass() .getName() .startsWith("io.opentelemetry.javaagent.instrumentation.netty.")) { - pipeline.removeLast(); + handler = pipeline.removeLast(); } else if (handler .getClass() .getName() .startsWith("io.opentelemetry.instrumentation.netty.")) { - pipeline.removeLast(); + handler = pipeline.removeLast(); } } } diff --git a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyChannelPipelineInstrumentation.java b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyChannelPipelineInstrumentation.java index f3dcd2eb46..d913369e31 100644 --- a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyChannelPipelineInstrumentation.java +++ b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyChannelPipelineInstrumentation.java @@ -48,8 +48,7 @@ public class NettyChannelPipelineInstrumentation /** * When certain handlers are added to the pipeline, we want to add our corresponding tracing - * handlers. If those handlers are later removed, we may want to remove our handlers. That is not - * currently implemented. + * handlers. If those handlers are later removed, we may want to remove our handlers. */ @SuppressWarnings("unused") public static class ChannelPipelineAddAdvice { diff --git a/instrumentation/netty/netty-4.0/javaagent/src/test/groovy/ChannelPipelineTest.groovy b/instrumentation/netty/netty-4.0/javaagent/src/test/groovy/ChannelPipelineTest.groovy index 8c32bd21f7..ac5a0cb6ce 100644 --- a/instrumentation/netty/netty-4.0/javaagent/src/test/groovy/ChannelPipelineTest.groovy +++ b/instrumentation/netty/netty-4.0/javaagent/src/test/groovy/ChannelPipelineTest.groovy @@ -125,10 +125,12 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { channelPipeline.last().getClass().getSimpleName() == "HttpClientTracingHandler" when: - channelPipeline.removeLast() + def removed = channelPipeline.removeLast() then: "there is no handler in pipeline" channelPipeline.size() == 0 + // removing tracing handler also removes the http handler and returns it + removed == httpHandler } private static class NoopChannelHandler extends ChannelHandlerAdapter { diff --git a/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyChannelPipelineInstrumentation.java b/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyChannelPipelineInstrumentation.java index e410a9a351..6f3fa24d26 100644 --- a/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyChannelPipelineInstrumentation.java +++ b/instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/NettyChannelPipelineInstrumentation.java @@ -50,8 +50,7 @@ public class NettyChannelPipelineInstrumentation /** * When certain handlers are added to the pipeline, we want to add our corresponding tracing - * handlers. If those handlers are later removed, we also remove our handlers. Support for - * replacing handlers and removeFirst/removeLast is currently not implemented. + * handlers. If those handlers are later removed, we also remove our handlers. */ @SuppressWarnings("unused") public static class ChannelPipelineAddAdvice { diff --git a/instrumentation/netty/netty-4.1/javaagent/src/test/groovy/ChannelPipelineTest.groovy b/instrumentation/netty/netty-4.1/javaagent/src/test/groovy/ChannelPipelineTest.groovy index 6d0d79060e..a6fe2b0b73 100644 --- a/instrumentation/netty/netty-4.1/javaagent/src/test/groovy/ChannelPipelineTest.groovy +++ b/instrumentation/netty/netty-4.1/javaagent/src/test/groovy/ChannelPipelineTest.groovy @@ -123,10 +123,12 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { channelPipeline.last().getClass().simpleName == "HttpClientTracingHandler" when: - channelPipeline.removeLast() + def removed = channelPipeline.removeLast() then: "there is no handler in pipeline" channelPipeline.size() == 0 + // removing tracing handler also removes the http handler and returns it + removed == httpHandler } private static class NoopChannelHandler extends ChannelHandlerAdapter {