Netty: don't expose tracing handler in handlers map (#10410)

Author: laurit

instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4/common/AbstractNettyChannelPipelineInstrumentation.java

@@ -18,6 +18,8 @@
 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 void transform(TypeTransformer transformer) {
             .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 static void addAfterHandler(
       }
     }
   }
+
+  @SuppressWarnings("unused")
+  public static class ToMapAdvice {
+
+    @Advice.OnMethodExit(suppress = Throwable.class)
+    public static void wrapIterator(@Advice.Return Map<String, ChannelHandler> map) {
+      VirtualField<ChannelHandler, ChannelHandler> virtualField =
+          VirtualField.find(ChannelHandler.class, ChannelHandler.class);
+      for (Iterator<ChannelHandler> 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();
+        }
+      }
+    }
+  }
 }

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,15 +112,15 @@ 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
 
     when:
     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 {
   }
 }

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,15 +110,15 @@ 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
 
     when:
     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 {
   }
 }