From d823ffed0b17b78a49deba37f6f99b2d6f634a83 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Tue, 6 Feb 2024 08:43:46 +0200 Subject: [PATCH] Netty: don't expose tracing handler in handlers map (#10410) --- ...ctNettyChannelPipelineInstrumentation.java | 23 ++++++++++++ .../test/groovy/ChannelPipelineTest.groovy | 35 ++++++++++++++++--- .../test/groovy/ChannelPipelineTest.groovy | 35 ++++++++++++++++--- 3 files changed, 85 insertions(+), 8 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 41ab08d0c8..6dc633f6e8 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 @@ -18,6 +18,8 @@ import io.netty.channel.ChannelPipeline; import io.opentelemetry.instrumentation.api.util.VirtualField; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; +import java.util.Iterator; +import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -59,6 +61,9 @@ public abstract class AbstractNettyChannelPipelineInstrumentation implements Typ .and(takesArgument(1, String.class)) .and(takesArguments(4)), AbstractNettyChannelPipelineInstrumentation.class.getName() + "$AddAfterAdvice"); + transformer.applyAdviceToMethod( + isMethod().and(named("toMap")).and(takesArguments(0)).and(returns(Map.class)), + AbstractNettyChannelPipelineInstrumentation.class.getName() + "$ToMapAdvice"); } @SuppressWarnings("unused") @@ -194,4 +199,22 @@ public abstract class AbstractNettyChannelPipelineInstrumentation implements Typ } } } + + @SuppressWarnings("unused") + public static class ToMapAdvice { + + @Advice.OnMethodExit(suppress = Throwable.class) + public static void wrapIterator(@Advice.Return Map map) { + VirtualField virtualField = + VirtualField.find(ChannelHandler.class, ChannelHandler.class); + for (Iterator iterator = map.values().iterator(); iterator.hasNext(); ) { + ChannelHandler handler = iterator.next(); + String handlerClassName = handler.getClass().getName(); + if (handlerClassName.startsWith("io.opentelemetry.javaagent.instrumentation.netty.") + || handlerClassName.startsWith("io.opentelemetry.instrumentation.netty.")) { + iterator.remove(); + } + } + } + } } 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 ac5a0cb6ce..cbe819dc5b 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 @@ -70,7 +70,7 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { replaceMethod(channelPipeline, "test", noopHandler, "http", httpHandler) then: "noop handler was removed; http and instrumentation handlers were added" - channelPipeline.size() == 2 + channelPipeline.size() == 1 channelPipeline.first() == httpHandler channelPipeline.last().getClass().getSimpleName() == "HttpClientTracingHandler" @@ -103,7 +103,7 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { channelPipeline.addLast("http", httpHandler) then: "add http and instrumentation handlers" - channelPipeline.size() == 2 + channelPipeline.size() == 1 channelPipeline.first() == httpHandler channelPipeline.last().getClass().getSimpleName() == "HttpClientTracingHandler" @@ -112,7 +112,7 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { channelPipeline.addAfter("http", "noop", noopHandler) then: "instrumentation handler is between with http and noop" - channelPipeline.size() == 3 + channelPipeline.size() == 2 channelPipeline.first() == httpHandler channelPipeline.last() == noopHandler @@ -120,7 +120,7 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { channelPipeline.removeLast() then: "http and instrumentation handlers will be remained" - channelPipeline.size() == 2 + channelPipeline.size() == 1 channelPipeline.first() == httpHandler channelPipeline.last().getClass().getSimpleName() == "HttpClientTracingHandler" @@ -133,6 +133,33 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { removed == httpHandler } + // regression test for https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/10377 + def "our handler not in handlers map"() { + setup: + def channel = new EmbeddedChannel(new NoopChannelHandler()) + def channelPipeline = new DefaultChannelPipeline(channel) + def handler = new HttpClientCodec() + + when: + // no handlers + channelPipeline.first() == null + + then: + // add handler + channelPipeline.addLast("http", handler) + channelPipeline.first() == handler + // our handler was also added + channelPipeline.last().getClass().simpleName == "HttpClientTracingHandler" + // our handler not counted + channelPipeline.size() == 1 + // our handler is not in handlers map + channelPipeline.toMap().size() == 1 + // our handler is not in handlers iterator + def list = [] + channelPipeline.iterator().forEachRemaining {list.add(it) } + list.size() == 1 + } + private static class NoopChannelHandler extends ChannelHandlerAdapter { } } 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 a6fe2b0b73..6093df1187 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 @@ -68,7 +68,7 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { replaceMethod(channelPipeline, "test", noopHandler, "http", httpHandler) then: "noop handler was removed; http and instrumentation handlers were added" - channelPipeline.size() == 2 + channelPipeline.size() == 1 channelPipeline.first() == httpHandler channelPipeline.last().getClass().simpleName == "HttpClientTracingHandler" @@ -101,7 +101,7 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { channelPipeline.addLast("http", httpHandler) then: "add http and instrumentation handlers" - channelPipeline.size() == 2 + channelPipeline.size() == 1 channelPipeline.first() == httpHandler channelPipeline.last().getClass().simpleName == "HttpClientTracingHandler" @@ -110,7 +110,7 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { channelPipeline.addAfter("http", "noop", noopHandler) then: "instrumentation handler is between with http and noop" - channelPipeline.size() == 3 + channelPipeline.size() == 2 channelPipeline.first() == httpHandler channelPipeline.last() == noopHandler @@ -118,7 +118,7 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { channelPipeline.removeLast() then: "http and instrumentation handlers will be remained" - channelPipeline.size() == 2 + channelPipeline.size() == 1 channelPipeline.first() == httpHandler channelPipeline.last().getClass().simpleName == "HttpClientTracingHandler" @@ -131,6 +131,33 @@ class ChannelPipelineTest extends AgentInstrumentationSpecification { removed == httpHandler } + // regression test for https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/10377 + def "our handler not in handlers map"() { + setup: + def channel = new EmbeddedChannel() + def channelPipeline = new DefaultChannelPipeline(channel) + def handler = new HttpClientCodec() + + when: + // no handlers + channelPipeline.first() == null + + then: + // add handler + channelPipeline.addLast("http", handler) + channelPipeline.first() == handler + // our handler was also added + channelPipeline.last().getClass().simpleName == "HttpClientTracingHandler" + // our handler not counted + channelPipeline.size() == 1 + // our handler is not in handlers map + channelPipeline.toMap().size() == 1 + // our handler is not in handlers iterator + def list = [] + channelPipeline.iterator().forEachRemaining {list.add(it) } + list.size() == 1 + } + private static class NoopChannelHandler extends ChannelHandlerAdapter { } }