Ensure that netty 4.0 instrumentation is not applied to 4.1 (#4626)

* Enusre that netty 4.0 instrumentation is not applied to 4.1 * formatting * cross test netty instrumentation
open-telemetry · Nov 11, 2021 · d53c276 · d53c276
1 parent 90e2a8c
commit d53c276
Show file tree

Hide file tree

Showing 9 changed files with 72 additions and 7 deletions.
diff --git a/instrumentation/netty/netty-3.8/javaagent/build.gradle.kts b/instrumentation/netty/netty-3.8/javaagent/build.gradle.kts
@@ -25,6 +25,9 @@ dependencies {
 
   compileOnly("io.netty:netty:3.8.0.Final")
 
+  testInstrumentation(project(":instrumentation:netty:netty-4.0:javaagent"))
+  testInstrumentation(project(":instrumentation:netty:netty-4.1:javaagent"))
+
   testLibrary("io.netty:netty:3.8.0.Final")
   testLibrary("com.ning:async-http-client:1.8.0")
 

diff --git a/...ava/io/opentelemetry/javaagent/instrumentation/netty/v3_8/NettyInstrumentationModule.java b/...ava/io/opentelemetry/javaagent/instrumentation/netty/v3_8/NettyInstrumentationModule.java
@@ -5,19 +5,26 @@
 
 package io.opentelemetry.javaagent.instrumentation.netty.v3_8;
 
+import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
 import static java.util.Arrays.asList;
 
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import java.util.List;
+import net.bytebuddy.matcher.ElementMatcher;
 
 @AutoService(InstrumentationModule.class)
 public class NettyInstrumentationModule extends InstrumentationModule {
   public NettyInstrumentationModule() {
     super("netty", "netty-3.8");
   }
 
+  @Override
+  public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
+    return hasClassesNamed("org.jboss.netty.handler.codec.http.HttpMessage");
+  }
+
   @Override
   public List<TypeInstrumentation> typeInstrumentations() {
     return asList(

diff --git a/instrumentation/netty/netty-4.0/javaagent/build.gradle.kts b/instrumentation/netty/netty-4.0/javaagent/build.gradle.kts
@@ -26,6 +26,10 @@ muzzle {
 dependencies {
   library("io.netty:netty-codec-http:4.0.0.Final")
   implementation(project(":instrumentation:netty:netty-4-common:javaagent"))
+
+  testInstrumentation(project(":instrumentation:netty:netty-3.8:javaagent"))
+  testInstrumentation(project(":instrumentation:netty:netty-4.1:javaagent"))
+
   latestDepTestLibrary("io.netty:netty-codec-http:4.0.56.Final")
 }
 

diff --git a/.../java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java b/.../java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java
@@ -7,6 +7,7 @@
 
 import static io.opentelemetry.javaagent.instrumentation.netty.v4_0.client.NettyClientSingletons.connectionInstrumenter;
 import static net.bytebuddy.matcher.ElementMatchers.named;
+import static net.bytebuddy.matcher.ElementMatchers.returns;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 
 import io.netty.channel.ChannelFuture;
@@ -31,7 +32,9 @@ public ElementMatcher<TypeDescription> typeMatcher() {
   @Override
   public void transform(TypeTransformer transformer) {
     transformer.applyAdviceToMethod(
-        named("doConnect").and(takesArgument(0, SocketAddress.class)),
+        named("doConnect")
+            .and(takesArgument(0, SocketAddress.class))
+            .and(returns(named("io.netty.channel.ChannelFuture"))),
         BootstrapInstrumentation.class.getName() + "$ConnectAdvice");
   }
 

diff --git a/...ava/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyInstrumentationModule.java b/...ava/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyInstrumentationModule.java
@@ -24,9 +24,11 @@ public NettyInstrumentationModule() {
 
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
-    // Class added in 4.1.0 and not in 4.0.56 to avoid resolving this instrumentation completely
-    // when using 4.1.
-    return not(hasClassesNamed("io.netty.handler.codec.http.CombinedHttpHeaders"));
+    return hasClassesNamed("io.netty.handler.codec.http.HttpMessage")
+        .and(
+            // Class added in 4.1.0 and not in 4.0.56 to avoid resolving this instrumentation
+            // completely when using 4.1.
+            not(hasClassesNamed("io.netty.handler.codec.http.CombinedHttpHeaders")));
   }
 
   @Override

diff --git a/instrumentation/netty/netty-4.1/javaagent/build.gradle.kts b/instrumentation/netty/netty-4.1/javaagent/build.gradle.kts
@@ -28,6 +28,9 @@ dependencies {
   api(project(":instrumentation:netty:netty-4.1:library"))
   implementation(project(":instrumentation:netty:netty-4-common:javaagent"))
 
+  testInstrumentation(project(":instrumentation:netty:netty-3.8:javaagent"))
+  testInstrumentation(project(":instrumentation:netty:netty-4.0:javaagent"))
+
   // Contains logging handler
   testLibrary("io.netty:netty-handler:4.1.0.Final")
   testLibrary("io.netty:netty-transport-native-epoll:4.1.0.Final:linux-x86_64")

diff --git a/.../main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCollectingClassVisitor.java b/.../main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCollectingClassVisitor.java
@@ -8,6 +8,7 @@
 import com.google.common.collect.EvictingQueue;
 import io.opentelemetry.instrumentation.api.field.VirtualField;
 import io.opentelemetry.javaagent.tooling.muzzle.references.ClassRef;
+import io.opentelemetry.javaagent.tooling.muzzle.references.ClassRefBuilder;
 import io.opentelemetry.javaagent.tooling.muzzle.references.Flag;
 import io.opentelemetry.javaagent.tooling.muzzle.references.Flag.ManifestationFlag;
 import io.opentelemetry.javaagent.tooling.muzzle.references.Flag.MinimumVisibilityFlag;
@@ -435,13 +436,35 @@ public void visitInvokeDynamicInsn(
       for (Object arg : bootstrapMethodArguments) {
         if (arg instanceof Handle) {
           Handle handle = (Handle) arg;
-          addReference(
+          ClassRefBuilder classRefBuilder =
               ClassRef.builder(Utils.getClassName(handle.getOwner()))
                   .addSource(refSourceClassName, currentLineNumber)
                   .addFlag(
                       computeMinimumClassAccess(
-                          refSourceType, Type.getObjectType(handle.getOwner())))
-                  .build());
+                          refSourceType, Type.getObjectType(handle.getOwner())));
+
+          if (handle.getTag() == Opcodes.H_INVOKEVIRTUAL
+              || handle.getTag() == Opcodes.H_INVOKESTATIC
+              || handle.getTag() == Opcodes.H_INVOKESPECIAL
+              || handle.getTag() == Opcodes.H_NEWINVOKESPECIAL
+              || handle.getTag() == Opcodes.H_INVOKEINTERFACE) {
+            Type methodType = Type.getMethodType(handle.getDesc());
+            Type ownerType = Type.getObjectType(handle.getOwner());
+            List<Flag> methodFlags = new ArrayList<>();
+            methodFlags.add(
+                handle.getTag() == Opcodes.H_INVOKESTATIC
+                    ? OwnershipFlag.STATIC
+                    : OwnershipFlag.NON_STATIC);
+            methodFlags.add(computeMinimumMethodAccess(refSourceType, ownerType));
+
+            classRefBuilder.addMethod(
+                new Source[] {new Source(refSourceClassName, currentLineNumber)},
+                methodFlags.toArray(new Flag[0]),
+                handle.getName(),
+                methodType.getReturnType(),
+                methodType.getArgumentTypes());
+            addReference(classRefBuilder.build());
+          }
         }
       }
       super.visitInvokeDynamicInsn(

diff --git a/muzzle/src/test/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCollectorTest.java b/muzzle/src/test/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCollectorTest.java
@@ -132,7 +132,26 @@ public void invokedynamicCreatesReferences() {
     Map<String, ClassRef> references = collector.getReferences();
 
     assertThat(references).containsKey("muzzle.TestClasses$MethodBodyAdvice$SomeImplementation");
+    assertMethod(
+        references.get("muzzle.TestClasses$MethodBodyAdvice$SomeImplementation"),
+        "someMethod",
+        "()V",
+        PROTECTED_OR_HIGHER,
+        OwnershipFlag.NON_STATIC);
     assertThat(references).containsKey("muzzle.TestClasses$MethodBodyAdvice$B");
+    assertMethod(
+        references.get("muzzle.TestClasses$MethodBodyAdvice$B"),
+        "staticMethod",
+        "()V",
+        PROTECTED_OR_HIGHER,
+        OwnershipFlag.STATIC);
+    assertThat(references).containsKey("muzzle.TestClasses$MethodBodyAdvice$A");
+    assertMethod(
+        references.get("muzzle.TestClasses$MethodBodyAdvice$A"),
+        "<init>",
+        "()V",
+        PROTECTED_OR_HIGHER,
+        OwnershipFlag.NON_STATIC);
   }
 
   @Test

diff --git a/muzzle/src/test/java/muzzle/TestClasses.java b/muzzle/src/test/java/muzzle/TestClasses.java
@@ -104,6 +104,7 @@ public static class InvokeDynamicAdvice {
     public static MethodBodyAdvice.SomeInterface invokeDynamicMethod(
         MethodBodyAdvice.SomeImplementation a) {
       Runnable staticMethod = MethodBodyAdvice.B::staticMethod;
+      Runnable constructorMethod = MethodBodyAdvice.A::new;
       return a::someMethod;
     }
   }