From 0556b38fa2a6e50ceaba4b6669f206a9f2a6f321 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 16 Nov 2023 14:08:38 +0200 Subject: [PATCH 1/3] Rewrite @Advice.Enter for indy advice --- .../v1_4/JedisInstrumentationModule.java | 10 - .../v3_0/JedisInstrumentationModule.java | 10 - .../v4_0/JedisInstrumentationModule.java | 10 - .../KafkaClientsInstrumentationModule.java | 6 - .../KafkaMetricsInstrumentationModule.java | 14 +- .../KafkaStreamsInstrumentationModule.java | 7 - ...ResteasyReactiveInstrumentationModule.java | 6 - .../ratpack/RatpackInstrumentationModule.java | 7 +- .../ReactorKafkaInstrumentationModule.java | 6 - .../ReactorNettyInstrumentationModule.java | 16 +- .../SpringKafkaInstrumentationModule.java | 6 - .../SpymemcachedInstrumentationModule.java | 6 - .../src/test/groovy/SpymemcachedTest.groovy | 7 +- .../v3_6/VertxKafkaInstrumentationModule.java | 6 - .../indy/AdviceTransformer.java | 239 +++++++++++++++--- 15 files changed, 226 insertions(+), 130 deletions(-) diff --git a/instrumentation/jedis/jedis-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v1_4/JedisInstrumentationModule.java b/instrumentation/jedis/jedis-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v1_4/JedisInstrumentationModule.java index 3e05e81255f0..ebbf2fbcfe79 100644 --- a/instrumentation/jedis/jedis-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v1_4/JedisInstrumentationModule.java +++ b/instrumentation/jedis/jedis-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v1_4/JedisInstrumentationModule.java @@ -32,14 +32,4 @@ public ElementMatcher.Junction classLoaderMatcher() { public List typeInstrumentations() { return asList(new JedisConnectionInstrumentation(), new JedisInstrumentation()); } - - @Override - public boolean isIndyModule() { - // java.lang.NoClassDefFoundError: - // io/opentelemetry/javaagent/instrumentation/jedis/JedisRequestContext - // at redis.clients.jedis.Jedis.flushAll(Jedis.java:367) - // at io.opentelemetry.javaagent.instrumentation.jedis.v1_4 - // .JedisClientTest.setup(JedisClientTest.java:49) - return false; - } } diff --git a/instrumentation/jedis/jedis-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v3_0/JedisInstrumentationModule.java b/instrumentation/jedis/jedis-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v3_0/JedisInstrumentationModule.java index 1e87a3c0ccda..22ddf9453448 100644 --- a/instrumentation/jedis/jedis-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v3_0/JedisInstrumentationModule.java +++ b/instrumentation/jedis/jedis-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v3_0/JedisInstrumentationModule.java @@ -34,14 +34,4 @@ public ElementMatcher.Junction classLoaderMatcher() { public List typeInstrumentations() { return asList(new JedisConnectionInstrumentation(), new JedisInstrumentation()); } - - @Override - public boolean isIndyModule() { - // java.lang.NoClassDefFoundError: - // io/opentelemetry/javaagent/instrumentation/jedis/JedisRequestContext - // at redis.clients.jedis.BinaryJedis.flushAll(BinaryJedis.java:595) - // at io.opentelemetry.javaagent.instrumentation.jedis.v3_0 - // .Jedis30ClientTest.setup(Jedis30ClientTest.java:50) - return false; - } } diff --git a/instrumentation/jedis/jedis-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v4_0/JedisInstrumentationModule.java b/instrumentation/jedis/jedis-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v4_0/JedisInstrumentationModule.java index 770c015ff2b2..d875cb0c1755 100644 --- a/instrumentation/jedis/jedis-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v4_0/JedisInstrumentationModule.java +++ b/instrumentation/jedis/jedis-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jedis/v4_0/JedisInstrumentationModule.java @@ -30,14 +30,4 @@ public ElementMatcher.Junction classLoaderMatcher() { public List typeInstrumentations() { return asList(new JedisConnectionInstrumentation(), new JedisInstrumentation()); } - - @Override - public boolean isIndyModule() { - // java.lang.NoClassDefFoundError: - // io/opentelemetry/javaagent/instrumentation/jedis/JedisRequestContext - // at redis.clients.jedis.Jedis.set(Jedis.java:4613) - // at io.opentelemetry.javaagent.instrumentation.jedis - // .v4_0.Jedis40ClientTest.getCommand(Jedis40ClientTest.java:78) - return false; - } } diff --git a/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaClientsInstrumentationModule.java b/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaClientsInstrumentationModule.java index 4b71df3df6ef..88ef08507e37 100644 --- a/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaClientsInstrumentationModule.java +++ b/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaClientsInstrumentationModule.java @@ -18,12 +18,6 @@ public KafkaClientsInstrumentationModule() { super("kafka-clients", "kafka-clients-0.11", "kafka"); } - @Override - public boolean isIndyModule() { - // OpenTelemetryMetricsReporter is not available in app class loader - return false; - } - @Override public List typeInstrumentations() { return asList( diff --git a/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/metrics/KafkaMetricsInstrumentationModule.java b/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/metrics/KafkaMetricsInstrumentationModule.java index ef14594ac551..6749eb2cfe7e 100644 --- a/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/metrics/KafkaMetricsInstrumentationModule.java +++ b/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/metrics/KafkaMetricsInstrumentationModule.java @@ -10,10 +10,14 @@ import com.google.auto.service.AutoService; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; +import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector; +import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.InjectionMode; import java.util.List; @AutoService(InstrumentationModule.class) -public class KafkaMetricsInstrumentationModule extends InstrumentationModule { +public class KafkaMetricsInstrumentationModule extends InstrumentationModule + implements ExperimentalInstrumentationModule { public KafkaMetricsInstrumentationModule() { super( "kafka-clients-metrics", @@ -24,9 +28,11 @@ public KafkaMetricsInstrumentationModule() { } @Override - public boolean isIndyModule() { - // OpenTelemetryMetricsReporter is not available in app class loader - return false; + public void injectClasses(ClassInjector injector) { + injector + .proxyBuilder( + "io.opentelemetry.instrumentation.kafka.internal.OpenTelemetryMetricsReporter") + .inject(InjectionMode.CLASS_ONLY); } @Override diff --git a/instrumentation/kafka/kafka-streams-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkastreams/KafkaStreamsInstrumentationModule.java b/instrumentation/kafka/kafka-streams-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkastreams/KafkaStreamsInstrumentationModule.java index 6fa0b9bd9e5b..c61cecfed349 100644 --- a/instrumentation/kafka/kafka-streams-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkastreams/KafkaStreamsInstrumentationModule.java +++ b/instrumentation/kafka/kafka-streams-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkastreams/KafkaStreamsInstrumentationModule.java @@ -18,13 +18,6 @@ public KafkaStreamsInstrumentationModule() { super("kafka-streams", "kafka-streams-0.11", "kafka"); } - @Override - public boolean isIndyModule() { - // java.lang.NoClassDefFoundError: - // io/opentelemetry/javaagent/instrumentation/kafkastreams/StateHolder - return false; - } - @Override public List typeInstrumentations() { return asList( diff --git a/instrumentation/quarkus-resteasy-reactive/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/quarkus/resteasy/reactive/QuarkusResteasyReactiveInstrumentationModule.java b/instrumentation/quarkus-resteasy-reactive/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/quarkus/resteasy/reactive/QuarkusResteasyReactiveInstrumentationModule.java index f02d8cab9b16..1a1be882bde4 100644 --- a/instrumentation/quarkus-resteasy-reactive/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/quarkus/resteasy/reactive/QuarkusResteasyReactiveInstrumentationModule.java +++ b/instrumentation/quarkus-resteasy-reactive/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/quarkus/resteasy/reactive/QuarkusResteasyReactiveInstrumentationModule.java @@ -19,12 +19,6 @@ public QuarkusResteasyReactiveInstrumentationModule() { super("quarkus", "jaxrs", "quarkus-resteasy-reactive", "quarkus-resteasy-reactive-3.0"); } - @Override - public boolean isIndyModule() { - // RunAdvice returns OtelRequestContext that is not available to the application class loader - return false; - } - @Override public List typeInstrumentations() { return asList( diff --git a/instrumentation/ratpack/ratpack-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/ratpack/RatpackInstrumentationModule.java b/instrumentation/ratpack/ratpack-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/ratpack/RatpackInstrumentationModule.java index 29bd0b6271cd..c0530aedfb79 100644 --- a/instrumentation/ratpack/ratpack-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/ratpack/RatpackInstrumentationModule.java +++ b/instrumentation/ratpack/ratpack-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/ratpack/RatpackInstrumentationModule.java @@ -20,8 +20,11 @@ public RatpackInstrumentationModule() { @Override public boolean isIndyModule() { - // StartAdvice uses both @Advice.Argument(readOnly = false) and return value from an - // @OnMethodEnter advice + // java.lang.ClassCastException: class + // io.opentelemetry.javaagent.shaded.instrumentation.netty.v4_1.internal.AutoValue_ServerContext + // cannot be cast to class + // io.opentelemetry.javaagent.shaded.instrumentation.netty.v4_1.internal.ServerContext + // (io.opentelemetry.javaagent.shaded.instrumentation.netty.v4_1.internal.AutoValue_ServerContext is in unnamed module of loader 'app'; io.opentelemetry.javaagent.shaded.instrumentation.netty.v4_1.internal.ServerContext is in unnamed module of loader io.opentelemetry.javaagent.tooling.instrumentation.indy.InstrumentationModuleClassLoader @7f088b5c) return false; } diff --git a/instrumentation/reactor/reactor-kafka-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactor/kafka/v1_0/ReactorKafkaInstrumentationModule.java b/instrumentation/reactor/reactor-kafka-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactor/kafka/v1_0/ReactorKafkaInstrumentationModule.java index bf1bfb8eea3a..47524c7fec23 100644 --- a/instrumentation/reactor/reactor-kafka-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactor/kafka/v1_0/ReactorKafkaInstrumentationModule.java +++ b/instrumentation/reactor/reactor-kafka-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactor/kafka/v1_0/ReactorKafkaInstrumentationModule.java @@ -19,12 +19,6 @@ public ReactorKafkaInstrumentationModule() { super("reactor-kafka", "reactor-kafka-1.0"); } - @Override - public boolean isIndyModule() { - // OpenTelemetryMetricsReporter is not available in app class loader - return false; - } - @Override public List typeInstrumentations() { return asList( diff --git a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyInstrumentationModule.java b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyInstrumentationModule.java index 6e9116ca3f34..2517d4ccbc69 100644 --- a/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyInstrumentationModule.java +++ b/instrumentation/reactor/reactor-netty/reactor-netty-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/ReactorNettyInstrumentationModule.java @@ -7,10 +7,12 @@ import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed; import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; import com.google.auto.service.AutoService; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; +import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule; import java.util.List; import java.util.function.BiConsumer; import java.util.function.Function; @@ -24,18 +26,13 @@ * HttpClient#doOnRequest(BiConsumer)} to pass context from the caller to Reactor to Netty. */ @AutoService(InstrumentationModule.class) -public class ReactorNettyInstrumentationModule extends InstrumentationModule { +public class ReactorNettyInstrumentationModule extends InstrumentationModule + implements ExperimentalInstrumentationModule { public ReactorNettyInstrumentationModule() { super("reactor-netty", "reactor-netty-1.0"); } - @Override - public boolean isIndyModule() { - // ResponseMonoAdvice uses both @Advice.Local and return value from an @OnMethodEnter advice - return false; - } - @Override public ElementMatcher.Junction classLoaderMatcher() { // Introduced in 1.0.0 @@ -47,6 +44,11 @@ public boolean isHelperClass(String className) { return className.startsWith("reactor.netty.http.client.HttpClientConfigBuddy"); } + @Override + public List injectedClassNames() { + return singletonList("reactor.netty.http.client.HttpClientConfigBuddy"); + } + @Override public List typeInstrumentations() { return asList( diff --git a/instrumentation/spring/spring-kafka-2.7/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/kafka/v2_7/SpringKafkaInstrumentationModule.java b/instrumentation/spring/spring-kafka-2.7/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/kafka/v2_7/SpringKafkaInstrumentationModule.java index 71b3503f98ed..332462d8e16b 100644 --- a/instrumentation/spring/spring-kafka-2.7/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/kafka/v2_7/SpringKafkaInstrumentationModule.java +++ b/instrumentation/spring/spring-kafka-2.7/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/kafka/v2_7/SpringKafkaInstrumentationModule.java @@ -18,12 +18,6 @@ public SpringKafkaInstrumentationModule() { super("spring-kafka", "spring-kafka-2.7"); } - @Override - public boolean isIndyModule() { - // OpenTelemetryMetricsReporter is not available in app class loader - return false; - } - @Override public List typeInstrumentations() { return asList( diff --git a/instrumentation/spymemcached-2.12/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spymemcached/SpymemcachedInstrumentationModule.java b/instrumentation/spymemcached-2.12/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spymemcached/SpymemcachedInstrumentationModule.java index 33c8165bcdf9..d7a6b20a6842 100644 --- a/instrumentation/spymemcached-2.12/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spymemcached/SpymemcachedInstrumentationModule.java +++ b/instrumentation/spymemcached-2.12/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spymemcached/SpymemcachedInstrumentationModule.java @@ -19,12 +19,6 @@ public SpymemcachedInstrumentationModule() { super("spymemcached", "spymemcached-2.12"); } - @Override - public boolean isIndyModule() { - // SyncOperationAdvice uses both @Advice.Local and return value from an @OnMethodEnter advice - return false; - } - @Override public List typeInstrumentations() { return singletonList(new MemcachedClientInstrumentation()); diff --git a/instrumentation/spymemcached-2.12/javaagent/src/test/groovy/SpymemcachedTest.groovy b/instrumentation/spymemcached-2.12/javaagent/src/test/groovy/SpymemcachedTest.groovy index fe4e91890977..1af5d70a4551 100644 --- a/instrumentation/spymemcached-2.12/javaagent/src/test/groovy/SpymemcachedTest.groovy +++ b/instrumentation/spymemcached-2.12/javaagent/src/test/groovy/SpymemcachedTest.groovy @@ -6,7 +6,6 @@ import com.google.common.util.concurrent.MoreExecutors import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification import io.opentelemetry.instrumentation.test.asserts.TraceAssert -import io.opentelemetry.javaagent.instrumentation.spymemcached.CompletionListener import io.opentelemetry.semconv.SemanticAttributes import net.spy.memcached.CASResponse import net.spy.memcached.ConnectionFactory @@ -627,15 +626,15 @@ class SpymemcachedTest extends AgentInstrumentationSpecification { "$SemanticAttributes.DB_OPERATION" operation if (error == "canceled") { - "${CompletionListener.DB_COMMAND_CANCELLED}" true + "spymemcached.command.cancelled" true } if (result == "hit") { - "${CompletionListener.MEMCACHED_RESULT}" CompletionListener.HIT + "spymemcached.result" "hit" } if (result == "miss") { - "${CompletionListener.MEMCACHED_RESULT}" CompletionListener.MISS + "spymemcached.result" "miss" } } } diff --git a/instrumentation/vertx/vertx-kafka-client-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/vertx/kafka/v3_6/VertxKafkaInstrumentationModule.java b/instrumentation/vertx/vertx-kafka-client-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/vertx/kafka/v3_6/VertxKafkaInstrumentationModule.java index a493a56c7e71..605351f87ee6 100644 --- a/instrumentation/vertx/vertx-kafka-client-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/vertx/kafka/v3_6/VertxKafkaInstrumentationModule.java +++ b/instrumentation/vertx/vertx-kafka-client-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/vertx/kafka/v3_6/VertxKafkaInstrumentationModule.java @@ -19,12 +19,6 @@ public VertxKafkaInstrumentationModule() { super("vertx-kafka-client", "vertx-kafka-client-3.6", "vertx"); } - @Override - public boolean isIndyModule() { - // OpenTelemetryMetricsReporter is not available in app class loader - return false; - } - @Override public List typeInstrumentations() { return asList( diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java index 6ed160573ff8..5103409d232f 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java @@ -7,6 +7,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Comparator; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -24,6 +25,7 @@ import org.objectweb.asm.commons.GeneratorAdapter; import org.objectweb.asm.commons.Method; import org.objectweb.asm.tree.AnnotationNode; +import org.objectweb.asm.tree.ClassNode; import org.objectweb.asm.tree.MethodNode; /** @@ -37,6 +39,29 @@ class AdviceTransformer { static byte[] transform(byte[] bytes) { ClassReader cr = new ClassReader(bytes); ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_MAXS); + ClassNode classNode = new ClassNode(); + cr.accept(classNode, ClassReader.EXPAND_FRAMES); + + // skip the class if there aren't any methods with advice annotations or these annotations have + // set inline = false + if (!hasInlineAdvice(classNode)) { + classNode.accept(cw); + return cw.toByteArray(); + } + + // sort enter advice method before exit advice + classNode.methods.sort( + Comparator.comparingInt( + (methodNode) -> { + if (isEnterAdvice(methodNode)) { + return 1; + } else if (isExitAdvice(methodNode)) { + return 2; + } + return 0; + })); + + TransformationContext context = new TransformationContext(); ClassVisitor cv = new ClassVisitor(Opcodes.ASM9, cw) { @@ -49,15 +74,40 @@ public MethodVisitor visitMethod( public void visitEnd() { super.visitEnd(); - instrument(this, classVisitor); + instrument(context, this, classVisitor); } }; } }; - cr.accept(cv, ClassReader.EXPAND_FRAMES); + classNode.accept(cv); return cw.toByteArray(); } + private static boolean hasInlineAdvice(ClassNode classNode) { + for (MethodNode mn : classNode.methods) { + if (hasInlineAdvice(mn)) { + return true; + } + } + + return false; + } + + private static boolean hasInlineAdvice(MethodNode methodNode) { + return hasInlineAdvice(methodNode, ADVICE_ON_METHOD_ENTER) + || hasInlineAdvice(methodNode, ADVICE_ON_METHOD_EXIT); + } + + private static boolean hasInlineAdvice(MethodNode methodNode, Type type) { + AnnotationNode annotationNode = getAnnotationNode(methodNode, type); + if (annotationNode != null) { + // delegating advice has attribute "inline" = false + // all other advice is inline + return !Boolean.FALSE.equals(getAttributeValue(annotationNode, "inline")); + } + return false; + } + // method argument annotated with Advice.Argument or Advice.Return private static class OutputArgument { // index of the method argument with the annotation @@ -128,6 +178,28 @@ private static OutputArgument getWritableReturnValue(MethodNode source) { return null; } + private static final Type ADVICE_ENTER = Type.getType(Advice.Enter.class); + + /** Argument annotated with {@code @Advice.Enter} or {@code null}. */ + private static OutputArgument getEnterArgument(MethodNode source) { + Type[] argumentTypes = Type.getArgumentTypes(source.desc); + if (source.visibleParameterAnnotations != null) { + int i = 0; + for (List list : source.visibleParameterAnnotations) { + for (AnnotationNode annotationNode : list) { + Type annotationType = Type.getType(annotationNode.desc); + if (ADVICE_ENTER.equals(annotationType) + && argumentTypes[i].getDescriptor().length() > 1) { + return new OutputArgument(i, -1); + } + } + i++; + } + } + + return null; + } + private static final Type ADVICE_LOCAL = Type.getType(Advice.Local.class); /** List of arguments annotated with {@code @Advice.Local}. */ @@ -152,29 +224,33 @@ private static List getLocals(MethodNode source) { return result; } - private static final Type ADVICE_ENTER = Type.getType(Advice.OnMethodEnter.class); + private static final Type ADVICE_ON_METHOD_ENTER = Type.getType(Advice.OnMethodEnter.class); private static boolean isEnterAdvice(MethodNode source) { - return hasAnnotation(source, ADVICE_ENTER); + return hasAnnotation(source, ADVICE_ON_METHOD_ENTER); } - private static final Type ADVICE_EXIT = Type.getType(Advice.OnMethodExit.class); + private static final Type ADVICE_ON_METHOD_EXIT = Type.getType(Advice.OnMethodExit.class); private static boolean isExitAdvice(MethodNode source) { - return hasAnnotation(source, ADVICE_EXIT); + return hasAnnotation(source, ADVICE_ON_METHOD_EXIT); } - private static boolean hasAnnotation(MethodNode source, Type type) { + private static AnnotationNode getAnnotationNode(MethodNode source, Type type) { if (source.visibleAnnotations != null) { for (AnnotationNode annotationNode : source.visibleAnnotations) { Type annotationType = Type.getType(annotationNode.desc); if (type.equals(annotationType)) { - return true; + return annotationNode; } } } - return false; + return null; + } + + private static boolean hasAnnotation(MethodNode source, Type type) { + return getAnnotationNode(source, type) != null; } /** @@ -304,10 +380,10 @@ public void visitInsn(int opcode) { } /** - * Transform arguments annotated with {@code @Advice.Local}. + * Transform arguments annotated with {@code @Advice.Local} and {@code @Advice.Enter}. * * 
{@code
-   * void foo(@Advice.Local("foo") T1 foo, @Advice.Local("bar") T1 bar)
+   * void foo(@Advice.Local("foo") T1 foo, @Advice.Local("bar") T2 bar)
    * }
* *

for enter advice is transformed to @@ -339,6 +415,7 @@ private static MethodVisitor instrumentAdviceLocals( MethodNode source, String originalDesc, List adviceLocals, + OutputArgument enterArgument, int returnIndex) { AtomicReference generatorRef = new AtomicReference<>(); AtomicInteger dataIndex = new AtomicInteger(); @@ -352,6 +429,12 @@ public AnnotationVisitor visitParameterAnnotation( if (Type.getDescriptor(Advice.Local.class).equals(descriptor)) { descriptor = Type.getDescriptor(Advice.Unused.class); } + // replace @Advice.Enter with @Advice.Unused + if (enterArgument != null + && enterArgument.adviceIndex == parameter + && Type.getDescriptor(Advice.Enter.class).equals(descriptor)) { + descriptor = Type.getDescriptor(Advice.Unused.class); + } return super.visitParameterAnnotation(parameter, descriptor, visible); } @@ -386,7 +469,7 @@ public void visitInsn(int opcode) { ga.invokeVirtual( hashMapType, Method.getMethod("java.lang.Object put(java.lang.Object, java.lang.Object)")); - // por return value of Map.put + // pop return value of Map.put ga.pop(); } // stack: array, array, array index for map, map @@ -404,7 +487,7 @@ public void visitCode() { Type[] argumentTypes = ga.getArgumentTypes(); if (isEnterAdvice) { - // we have change the type fo method arguments annotated with @Advice.Local to Object + // we have changed the type fo method arguments annotated with @Advice.Local to Object // here we'll load the argument, cast it to its actual type, and store it back for (AdviceLocal adviceLocal : adviceLocals) { ga.loadArg(adviceLocal.adviceIndex); @@ -414,18 +497,29 @@ public void visitCode() { return; } - // the index of last argument where Map is inserted (argumentTypes array does not - // contain the Map) + // the index of last argument where object array returned from enter advice is inserted + // (argumentTypes array does not contain the object array) int lastArgumentIndex = argumentTypes.length; AnnotationVisitor av = mv.visitParameterAnnotation( lastArgumentIndex, Type.getDescriptor(Advice.Enter.class), true); av.visitEnd(); - Type mapType = Type.getType(Map.class); // load object array ga.loadLocal(dataIndex.get(), OBJECT_ARRAY_TYPE); + if (enterArgument != null) { + // value for @Advice.Enter is stored as the first element + ga.dup(); + ga.push(0); + Type type = argumentTypes[enterArgument.adviceIndex]; + ga.arrayLoad(type); + ga.checkCast(type); + ga.storeArg(enterArgument.adviceIndex); + } + + // object array on stack ga.dup(); + Type mapType = Type.getType(Map.class); // we want the last element of the array ga.arrayLength(); ga.visitInsn(Opcodes.ICONST_1); @@ -459,27 +553,39 @@ public void visitCode() { return ga; } - private static void instrument(MethodNode methodNode, ClassVisitor classVisitor) { + private static void instrument( + TransformationContext context, MethodNode methodNode, ClassVisitor classVisitor) { String originalDescriptor = methodNode.desc; String[] exceptionsArray = methodNode.exceptions.toArray(new String[0]); - // only instrument if method returns void, in most of the instrumentations we need to change - // the return type of the method which will only work if the method returns void - if (Type.VOID_TYPE.equals(Type.getReturnType(methodNode.desc))) { - List writableArguments = getWritableArguments(methodNode); - OutputArgument writableReturn = getWritableReturnValue(methodNode); - List adviceLocals = getLocals(methodNode); - boolean isEnterAdvice = isEnterAdvice(methodNode); + List writableArguments = getWritableArguments(methodNode); + OutputArgument writableReturn = getWritableReturnValue(methodNode); + OutputArgument enterArgument = getEnterArgument(methodNode); + List adviceLocals = getLocals(methodNode); + boolean isEnterAdvice = isEnterAdvice(methodNode); + boolean isExitAdvice = isExitAdvice(methodNode); + Type returnType = Type.getReturnType(methodNode.desc); + + // currently we don't support rewriting enter advice returning a primitive type + if (isEnterAdvice && !(returnType.getSort() == Type.VOID || returnType.getSort() == Type.OBJECT || returnType.getSort() == Type.ARRAY)) { + context.disableReturnTypeChange(); + } + // context is shared by enter and exit advice, if entry advice was rejected don't attempt to + // rewrite usages of @Advice.Enter in the exit advice + if (!context.canChangeReturnType()) { + enterArgument = null; + } + if (context.canChangeReturnType() || (isExitAdvice && Type.VOID_TYPE.equals(returnType))) { if (!writableArguments.isEmpty() || writableReturn != null + || !Type.VOID_TYPE.equals(returnType) || (!adviceLocals.isEmpty() && isEnterAdvice)) { Type[] argumentTypes = Type.getArgumentTypes(methodNode.desc); if (!adviceLocals.isEmpty() && isEnterAdvice) { // Set type of arguments annotated with @Advice.Local to Object. These arguments are - // likely - // to be helper classes which currently breaks because the invokedynamic call in advised - // class needs access to the parameter types of the advice method. + // likely to be helper classes which currently breaks because the invokedynamic call in + // advised class needs access to the parameter types of the advice method. for (AdviceLocal adviceLocal : adviceLocals) { argumentTypes[adviceLocal.adviceIndex] = OBJECT_TYPE; } @@ -496,6 +602,7 @@ private static void instrument(MethodNode methodNode, ClassVisitor classVisitor) exceptionsArray); MethodVisitor mv = instrumentOurParameters( + context, tmp, methodNode, originalDescriptor, @@ -510,7 +617,7 @@ private static void instrument(MethodNode methodNode, ClassVisitor classVisitor) // this is the only transformation that does not change the return type of the advice method, // thus it is also the only transformation that can be applied on top of the other transforms - if (!adviceLocals.isEmpty() && isExitAdvice(methodNode)) { + if ((!adviceLocals.isEmpty() || enterArgument != null) && isExitAdvice) { // Set type of arguments annotated with @Advice.Local to Object. These arguments are likely // to be helper classes which currently breaks because the invokedynamic call in advised // class needs access to the parameter types of the advice method. @@ -518,6 +625,9 @@ private static void instrument(MethodNode methodNode, ClassVisitor classVisitor) for (AdviceLocal adviceLocal : adviceLocals) { newArgumentTypes[adviceLocal.adviceIndex] = OBJECT_TYPE; } + if (enterArgument != null) { + newArgumentTypes[enterArgument.adviceIndex] = OBJECT_TYPE; + } List typeList = new ArrayList<>(Arrays.asList(newArgumentTypes)); // add Object array as the last argument, this array is used to pass info from the enter // advice @@ -535,7 +645,8 @@ private static void instrument(MethodNode methodNode, ClassVisitor classVisitor) methodNode.signature, exceptionsArray); MethodVisitor mv = - instrumentAdviceLocals(false, tmp, methodNode, originalDescriptor, adviceLocals, -1); + instrumentAdviceLocals( + false, tmp, methodNode, originalDescriptor, adviceLocals, enterArgument, -1); methodNode.accept(mv); methodNode = tmp; @@ -549,12 +660,13 @@ private static void instrument(MethodNode methodNode, ClassVisitor classVisitor) methodNode.desc, methodNode.signature, exceptionsArray); - mv = delegateAdvice(mv); + mv = delegateAdvice(context, mv); methodNode.accept(mv); } private static MethodVisitor instrumentOurParameters( + TransformationContext context, MethodVisitor target, MethodNode source, String originalDesc, @@ -562,7 +674,9 @@ private static MethodVisitor instrumentOurParameters( OutputArgument writableReturn, List adviceLocals) { - int returnArraySize = 0; + // position 0 in enter advice is reserved for the return value of the method + // to avoid complicating things further we aren't going to figure out whether it is really used + int returnArraySize = isEnterAdvice(source) ? 1 : 0; if (writableReturn != null) { target = instrumentWritableReturn(target, source, writableReturn, returnArraySize); returnArraySize++; @@ -573,28 +687,34 @@ private static MethodVisitor instrumentOurParameters( } if (!adviceLocals.isEmpty() && isEnterAdvice(source)) { target = - instrumentAdviceLocals(true, target, source, originalDesc, adviceLocals, returnArraySize); + instrumentAdviceLocals( + true, target, source, originalDesc, adviceLocals, null, returnArraySize); returnArraySize++; } - target = addReturnArray(target, returnArraySize); + target = addReturnArray(context, target, returnArraySize); return target; } /** Return the value of the {@code readOnly} attribute of the annotation. */ private static boolean isWriteable(AnnotationNode annotationNode) { + Object value = getAttributeValue(annotationNode, "readOnly"); + return Boolean.FALSE.equals(value); + } + + private static Object getAttributeValue(AnnotationNode annotationNode, String attributeName) { if (annotationNode.values != null && !annotationNode.values.isEmpty()) { List values = annotationNode.values; for (int i = 0; i < values.size(); i += 2) { - String attributeName = (String) values.get(i); - Object attributeValue = values.get(i + 1); - if ("readOnly".equals(attributeName)) { - return Boolean.FALSE.equals(attributeValue); + String name = (String) values.get(i); + Object value = values.get(i + 1); + if (attributeName.equals(name)) { + return value; } } } - return false; + return null; } /** Return the value of the {@code value} attribute of the annotation. */ @@ -613,15 +733,34 @@ private static Object getAnnotationValue(AnnotationNode annotationNode) { return null; } - private static MethodVisitor addReturnArray(MethodVisitor target, int returnArraySize) { + private static MethodVisitor addReturnArray( + TransformationContext context, MethodVisitor target, int returnArraySize) { return new MethodVisitor(Opcodes.ASM9, target) { @Override public void visitInsn(int opcode) { if (Opcodes.RETURN == opcode) { + // change the return value of the method to Object[] GeneratorAdapter ga = new GeneratorAdapter(mv, 0, null, "()V"); ga.push(returnArraySize); ga.newArray(OBJECT_TYPE); opcode = Opcodes.ARETURN; + } else if (context.canChangeReturnType() && Opcodes.ARETURN == opcode) { + // change the return value of the method to Object[] that on the 0 index contains the + // original return value + + // stack: original return value + GeneratorAdapter ga = new GeneratorAdapter(mv, 0, null, "()V"); + ga.push(returnArraySize); + ga.newArray(OBJECT_TYPE); + // stack: original return value, array + ga.dupX1(); + // stack: array, original return value, array + ga.swap(); + // stack: array, array, original return value + ga.push(0); // original return value is stored as the first element + ga.swap(); + ga.arrayStore(OBJECT_TYPE); + // stack: array } super.visitInsn(opcode); } @@ -631,8 +770,10 @@ public void visitInsn(int opcode) { /** * If method is annotated with {@link Advice.OnMethodEnter} or {@link Advice.OnMethodExit} set * {@code inline} attribute on the annotation to {@code false}. + * If method is annotated with {@link Advice.OnMethodEnter} and has {@code skipOn} attribute set + * {@code skipOnIndex} attribute on the annotation to {@code 0}. */ - private static MethodVisitor delegateAdvice(MethodVisitor target) { + private static MethodVisitor delegateAdvice(TransformationContext context, MethodVisitor target) { return new MethodVisitor(Opcodes.ASM9, target) { @Override public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) { @@ -644,12 +785,15 @@ public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) { } return new AnnotationVisitor(api, av) { boolean hasInline = false; + boolean hasSkipOn = false; @Override public void visit(String name, Object value) { if ("inline".equals(name)) { value = Boolean.FALSE; hasInline = true; + } else if ("skipOn".equals(name) && value != void.class) { + hasSkipOn = true; } super.visit(name, value); } @@ -659,6 +803,9 @@ public void visitEnd() { if (!hasInline) { visit("inline", Boolean.FALSE); } + if (context.canChangeReturnType() && hasSkipOn) { + visit("skipOnIndex", 0); + } super.visitEnd(); } }; @@ -691,5 +838,17 @@ public void visit(String name, Object value) { }; } + private static class TransformationContext { + private boolean canChangeReturnType = true; + + void disableReturnTypeChange() { + canChangeReturnType = false; + } + + boolean canChangeReturnType() { + return canChangeReturnType; + } + } + private AdviceTransformer() {} } From 60c942430b76fae8bbb52242368cf7bfb88e0de5 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 16 Nov 2023 14:26:02 +0200 Subject: [PATCH 2/3] spotless --- .../instrumentation/indy/AdviceTransformer.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java index 5103409d232f..500e565b85bb 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java @@ -567,7 +567,10 @@ private static void instrument( Type returnType = Type.getReturnType(methodNode.desc); // currently we don't support rewriting enter advice returning a primitive type - if (isEnterAdvice && !(returnType.getSort() == Type.VOID || returnType.getSort() == Type.OBJECT || returnType.getSort() == Type.ARRAY)) { + if (isEnterAdvice + && !(returnType.getSort() == Type.VOID + || returnType.getSort() == Type.OBJECT + || returnType.getSort() == Type.ARRAY)) { context.disableReturnTypeChange(); } // context is shared by enter and exit advice, if entry advice was rejected don't attempt to @@ -769,9 +772,9 @@ public void visitInsn(int opcode) { /** * If method is annotated with {@link Advice.OnMethodEnter} or {@link Advice.OnMethodExit} set - * {@code inline} attribute on the annotation to {@code false}. - * If method is annotated with {@link Advice.OnMethodEnter} and has {@code skipOn} attribute set - * {@code skipOnIndex} attribute on the annotation to {@code 0}. + * {@code inline} attribute on the annotation to {@code false}. If method is annotated with {@link + * Advice.OnMethodEnter} and has {@code skipOn} attribute set {@code skipOnIndex} attribute on the + * annotation to {@code 0}. */ private static MethodVisitor delegateAdvice(TransformationContext context, MethodVisitor target) { return new MethodVisitor(Opcodes.ASM9, target) { From 30e9ef0bead9b5b39ff237be75a4c495c1f7c5a4 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Thu, 16 Nov 2023 20:34:39 +0200 Subject: [PATCH 3/3] update comment --- .../tooling/instrumentation/indy/AdviceTransformer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java index 500e565b85bb..26f3323c0de5 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java @@ -112,7 +112,7 @@ private static boolean hasInlineAdvice(MethodNode methodNode, Type type) { private static class OutputArgument { // index of the method argument with the annotation final int adviceIndex; - // value of the annotation or -1 if Advice.Return + // value of the annotation or -1 if Advice.Return or Advice.Enter final int methodIndex; OutputArgument(int adviceIndex, int methodIndex) {