From d53c276b96db7262a2d849a9960b2e01560f4d8c Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 11 Nov 2021 18:38:56 +0200 Subject: [PATCH] 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 --- .../netty-3.8/javaagent/build.gradle.kts | 3 ++ .../v3_8/NettyInstrumentationModule.java | 7 +++++ .../netty-4.0/javaagent/build.gradle.kts | 4 +++ .../netty/v4_0/BootstrapInstrumentation.java | 5 +++- .../v4_0/NettyInstrumentationModule.java | 8 +++-- .../netty-4.1/javaagent/build.gradle.kts | 3 ++ .../ReferenceCollectingClassVisitor.java | 29 +++++++++++++++++-- .../muzzle/ReferenceCollectorTest.java | 19 ++++++++++++ muzzle/src/test/java/muzzle/TestClasses.java | 1 + 9 files changed, 72 insertions(+), 7 deletions(-) diff --git a/instrumentation/netty/netty-3.8/javaagent/build.gradle.kts b/instrumentation/netty/netty-3.8/javaagent/build.gradle.kts index def7c6d8ef12..47dafa705960 100644 --- 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/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/NettyInstrumentationModule.java b/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/NettyInstrumentationModule.java index 77d66eb821cb..54b65990b13f 100644 --- a/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/NettyInstrumentationModule.java +++ b/instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/NettyInstrumentationModule.java @@ -5,12 +5,14 @@ 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 { @@ -18,6 +20,11 @@ public NettyInstrumentationModule() { super("netty", "netty-3.8"); } + @Override + public ElementMatcher.Junction classLoaderMatcher() { + return hasClassesNamed("org.jboss.netty.handler.codec.http.HttpMessage"); + } + @Override public List 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 index 165aa43bde60..967270148905 100644 --- 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/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java index 4e9943a5140f..7dfd926a2629 100644 --- a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java +++ b/instrumentation/netty/netty-4.0/javaagent/src/main/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 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/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyInstrumentationModule.java b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyInstrumentationModule.java index f17c49c0394a..2ebeefa066d5 100644 --- a/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyInstrumentationModule.java +++ b/instrumentation/netty/netty-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/NettyInstrumentationModule.java @@ -24,9 +24,11 @@ public NettyInstrumentationModule() { @Override public ElementMatcher.Junction 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 index 35740cdfa2a5..5d3a8586e074 100644 --- 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/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCollectingClassVisitor.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCollectingClassVisitor.java index 3da3746c95fe..03c99ada9c30 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/ReferenceCollectingClassVisitor.java +++ b/muzzle/src/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 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 index 198206d591e3..3607a0f47186 100644 --- 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 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"), + "", + "()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 index b3516dbd2dab..32f41f82bcbc 100644 --- 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; } }