From 8650fb5a79beff4bfd8549661aaecc162dfb610b Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 10 Nov 2021 00:10:31 +0200 Subject: [PATCH 1/4] Add a ClassAndMethod class to Instrumentation API --- docs/contributing/writing-instrumentation.md | 16 +++++++++++++++ .../instrumentation/api/tracer/SpanNames.java | 9 +++++++++ .../api/util}/ClassAndMethod.java | 2 +- .../extannotations/ClassAndMethod.java | 20 ------------------- ...ExternalAnnotationAttributesExtractor.java | 1 + .../ExternalAnnotationInstrumentation.java | 1 + .../ExternalAnnotationSingletons.java | 1 + .../methods/MethodInstrumentation.java | 18 ++++++++++------- .../methods/MethodSingletons.java | 10 +++++----- .../WithSpanInstrumentation.java | 18 +++++++++++++---- .../server/RemoteServerInstrumentation.java | 1 + .../server/RmiServerAttributesExtractor.java | 1 + .../rmi/server/RmiServerSingletons.java | 1 + .../v5_0/response/ResponseSendAdvice.java | 17 ++++++++++------ .../v5_0/response/ResponseSingletons.java | 8 ++++---- .../HttpServletResponseAdviceHelper.java | 6 +++--- .../javax/response/ResponseSendAdvice.java | 16 +++++++++------ .../javax/response/ResponseSingletons.java | 8 ++++---- 18 files changed, 94 insertions(+), 60 deletions(-) rename {instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server => instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/util}/ClassAndMethod.java (87%) delete mode 100644 instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ClassAndMethod.java diff --git a/docs/contributing/writing-instrumentation.md b/docs/contributing/writing-instrumentation.md index 7f6d52f95f0a..6303a2aeaf29 100644 --- a/docs/contributing/writing-instrumentation.md +++ b/docs/contributing/writing-instrumentation.md @@ -384,3 +384,19 @@ dependencies { testImplementation(project(":instrumentation:yarpc-1.0:javaagent")) } ``` + +## Why we don't use ByteBuddy @Advice.Origin Method + +Instead of +``` +@Advice.Origin Method method +``` +we prefer to use +``` +@Advice.Origin("#t") Class declaringClass, +@Advice.Origin("#m") String methodName +``` +because the former inserts a call to `Class.getMethod(...)` in transformed class. In contrast, +getting the declaring class and method name is just loading constants from constant pool, which is +a much simpler operation. Considering that the majority of our use cases only need declaring class +and method name we can avoid calling `Class.getMethod(...)`. diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/SpanNames.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/SpanNames.java index 41a3861598d0..d1fd88c00505 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/SpanNames.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/SpanNames.java @@ -5,6 +5,7 @@ package io.opentelemetry.instrumentation.api.tracer; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; import java.lang.reflect.Method; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -37,6 +38,14 @@ public static String fromMethod(Class clazz, @Nullable Method method) { return fromMethod(clazz, method == null ? "" : method.getName()); } + /** + * This method is used to generate a span name based on a method. Anonymous classes are named + * based on their parent. + */ + public static String fromMethod(ClassAndMethod classAndMethod) { + return fromMethod(classAndMethod.declaringClass(), classAndMethod.methodName()); + } + /** * This method is used to generate a span name based on a method. Anonymous classes are named * based on their parent. diff --git a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/ClassAndMethod.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/util/ClassAndMethod.java similarity index 87% rename from instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/ClassAndMethod.java rename to instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/util/ClassAndMethod.java index 0e47d1f3926a..1497853ff999 100644 --- a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/ClassAndMethod.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/util/ClassAndMethod.java @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.javaagent.instrumentation.rmi.server; +package io.opentelemetry.instrumentation.api.util; import com.google.auto.value.AutoValue; diff --git a/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ClassAndMethod.java b/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ClassAndMethod.java deleted file mode 100644 index f1b2ac65ff77..000000000000 --- a/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ClassAndMethod.java +++ /dev/null @@ -1,20 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.extannotations; - -import com.google.auto.value.AutoValue; - -@AutoValue -public abstract class ClassAndMethod { - - public static ClassAndMethod create(Class declaringClass, String methodName) { - return new AutoValue_ClassAndMethod(declaringClass, methodName); - } - - public abstract Class declaringClass(); - - public abstract String methodName(); -} diff --git a/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationAttributesExtractor.java b/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationAttributesExtractor.java index 965aa2403f69..d9eaa4b15d9c 100644 --- a/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationAttributesExtractor.java +++ b/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationAttributesExtractor.java @@ -6,6 +6,7 @@ package io.opentelemetry.javaagent.instrumentation.extannotations; import io.opentelemetry.instrumentation.api.instrumenter.code.CodeAttributesExtractor; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; import javax.annotation.Nullable; final class ExternalAnnotationAttributesExtractor diff --git a/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationInstrumentation.java b/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationInstrumentation.java index 5945253dd0f5..0709690ef643 100644 --- a/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationInstrumentation.java +++ b/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationInstrumentation.java @@ -18,6 +18,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.config.Config; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; diff --git a/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationSingletons.java b/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationSingletons.java index 5cc7a2477320..d7c0fe19683c 100644 --- a/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationSingletons.java +++ b/instrumentation/external-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/extannotations/ExternalAnnotationSingletons.java @@ -8,6 +8,7 @@ import io.opentelemetry.api.GlobalOpenTelemetry; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.instrumentation.api.instrumenter.code.CodeSpanNameExtractor; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; public final class ExternalAnnotationSingletons { diff --git a/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentation.java b/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentation.java index 115a336d9a15..365e568bee3f 100644 --- a/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentation.java +++ b/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentation.java @@ -15,9 +15,9 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.annotation.support.async.AsyncOperationEndSupport; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; -import java.lang.reflect.Method; import java.util.Set; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; @@ -55,21 +55,25 @@ public static class MethodAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter( - @Advice.Origin Method method, + @Advice.Origin("#t") Class declaringClass, + @Advice.Origin("#m") String methodName, + @Advice.Local("otelMethod") ClassAndMethod classAndMethod, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { Context parentContext = currentContext(); - if (!instrumenter().shouldStart(parentContext, method)) { + classAndMethod = ClassAndMethod.create(declaringClass, methodName); + if (!instrumenter().shouldStart(parentContext, classAndMethod)) { return; } - context = instrumenter().start(parentContext, method); + context = instrumenter().start(parentContext, classAndMethod); scope = context.makeCurrent(); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( - @Advice.Origin Method method, + @Advice.Origin("#r") Class returnType, + @Advice.Local("otelMethod") ClassAndMethod classAndMethod, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope, @Advice.Return(typing = Assigner.Typing.DYNAMIC, readOnly = false) Object returnValue, @@ -77,8 +81,8 @@ public static void stopSpan( scope.close(); returnValue = - AsyncOperationEndSupport.create(instrumenter(), Void.class, method.getReturnType()) - .asyncEnd(context, method, returnValue, throwable); + AsyncOperationEndSupport.create(instrumenter(), Void.class, returnType) + .asyncEnd(context, classAndMethod, returnValue, throwable); } } } diff --git a/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodSingletons.java b/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodSingletons.java index 5d6a030f07c9..6cd3cd826d32 100644 --- a/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodSingletons.java +++ b/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodSingletons.java @@ -10,23 +10,23 @@ import io.opentelemetry.instrumentation.api.instrumenter.SpanKindExtractor; import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor; import io.opentelemetry.instrumentation.api.tracer.SpanNames; -import java.lang.reflect.Method; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; public final class MethodSingletons { private static final String INSTRUMENTATION_NAME = "io.opentelemetry.methods"; - private static final Instrumenter INSTRUMENTER; + private static final Instrumenter INSTRUMENTER; static { - SpanNameExtractor spanName = SpanNames::fromMethod; + SpanNameExtractor spanName = SpanNames::fromMethod; INSTRUMENTER = - Instrumenter.builder( + Instrumenter.builder( GlobalOpenTelemetry.get(), INSTRUMENTATION_NAME, spanName) .newInstrumenter(SpanKindExtractor.alwaysInternal()); } - public static Instrumenter instrumenter() { + public static Instrumenter instrumenter() { return INSTRUMENTER; } diff --git a/instrumentation/opentelemetry-annotations-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/otelannotations/WithSpanInstrumentation.java b/instrumentation/opentelemetry-annotations-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/otelannotations/WithSpanInstrumentation.java index 21b3c5dd1e9e..3f63a52ae713 100644 --- a/instrumentation/opentelemetry-annotations-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/otelannotations/WithSpanInstrumentation.java +++ b/instrumentation/opentelemetry-annotations-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/otelannotations/WithSpanInstrumentation.java @@ -115,12 +115,17 @@ public static class WithSpanAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter( - @Advice.Origin Method method, + @Advice.Origin Method originMethod, + @Advice.Local("otelMethod") Method method, @Advice.Local("otelOperationEndSupport") AsyncOperationEndSupport operationEndSupport, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { + // Every usage of @Advice.Origin Method is replaced with a call to Class.getMethod, copy it + // to local variable so that there would be only one call to Class.getMethod. + method = originMethod; + Instrumenter instrumenter = instrumenter(); Context current = Java8BytecodeBridge.currentContext(); @@ -134,7 +139,7 @@ public static void onEnter( @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( - @Advice.Origin Method method, + @Advice.Local("otelMethod") Method method, @Advice.Local("otelOperationEndSupport") AsyncOperationEndSupport operationEndSupport, @Advice.Local("otelContext") Context context, @@ -154,7 +159,8 @@ public static class WithSpanAttributesAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter( - @Advice.Origin Method method, + @Advice.Origin Method originMethod, + @Advice.Local("otelMethod") Method method, @Advice.AllArguments(typing = Assigner.Typing.DYNAMIC) Object[] args, @Advice.Local("otelOperationEndSupport") AsyncOperationEndSupport operationEndSupport, @@ -162,6 +168,10 @@ public static void onEnter( @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { + // Every usage of @Advice.Origin Method is replaced with a call to Class.getMethod, copy it + // to local variable so that there would be only one call to Class.getMethod. + method = originMethod; + Instrumenter instrumenter = instrumenterWithAttributes(); Context current = Java8BytecodeBridge.currentContext(); request = new MethodRequest(method, args); @@ -176,7 +186,7 @@ public static void onEnter( @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( - @Advice.Origin Method method, + @Advice.Local("otelMethod") Method method, @Advice.Local("otelOperationEndSupport") AsyncOperationEndSupport operationEndSupport, @Advice.Local("otelRequest") MethodRequest request, diff --git a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RemoteServerInstrumentation.java b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RemoteServerInstrumentation.java index 06da7049cf43..1331a31f56e9 100644 --- a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RemoteServerInstrumentation.java +++ b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RemoteServerInstrumentation.java @@ -16,6 +16,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import io.opentelemetry.javaagent.instrumentation.api.CallDepth; diff --git a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RmiServerAttributesExtractor.java b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RmiServerAttributesExtractor.java index 49c6cfce71e9..4d3315ecd43f 100644 --- a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RmiServerAttributesExtractor.java +++ b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RmiServerAttributesExtractor.java @@ -6,6 +6,7 @@ package io.opentelemetry.javaagent.instrumentation.rmi.server; import io.opentelemetry.instrumentation.api.instrumenter.rpc.RpcAttributesExtractor; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; final class RmiServerAttributesExtractor extends RpcAttributesExtractor { diff --git a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RmiServerSingletons.java b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RmiServerSingletons.java index 85627c412b0c..ded3ebba11ee 100644 --- a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RmiServerSingletons.java +++ b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/server/RmiServerSingletons.java @@ -9,6 +9,7 @@ import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.instrumentation.api.instrumenter.SpanKindExtractor; import io.opentelemetry.instrumentation.api.instrumenter.rpc.RpcSpanNameExtractor; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; public final class RmiServerSingletons { diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/response/ResponseSendAdvice.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/response/ResponseSendAdvice.java index 42d5b664c619..98a2b88dd01f 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/response/ResponseSendAdvice.java +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/response/ResponseSendAdvice.java @@ -9,11 +9,11 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; import io.opentelemetry.javaagent.instrumentation.api.CallDepth; import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.instrumentation.servlet.common.response.HttpServletResponseAdviceHelper; import jakarta.servlet.http.HttpServletResponse; -import java.lang.reflect.Method; import net.bytebuddy.asm.Advice; @SuppressWarnings("unused") @@ -21,7 +21,10 @@ public class ResponseSendAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void start( - @Advice.Origin Method method, + @Advice.This Object response, + @Advice.Origin("#t") Class declaringClass, + @Advice.Origin("#m") String methodName, + @Advice.Local("otelMethod") ClassAndMethod classAndMethod, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope, @Advice.Local("otelCallDepth") CallDepth callDepth) { @@ -33,8 +36,9 @@ public static void start( Context parentContext = Java8BytecodeBridge.currentContext(); // Don't want to generate a new top-level span if (Java8BytecodeBridge.spanFromContext(parentContext).getSpanContext().isValid()) { - if (instrumenter().shouldStart(parentContext, method)) { - context = instrumenter().start(parentContext, method); + classAndMethod = ClassAndMethod.create(declaringClass, methodName); + if (instrumenter().shouldStart(parentContext, classAndMethod)) { + context = instrumenter().start(parentContext, classAndMethod); scope = context.makeCurrent(); } } @@ -42,14 +46,15 @@ public static void start( @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( - @Advice.Origin Method method, @Advice.Thrown Throwable throwable, + @Advice.Local("otelMethod") ClassAndMethod classAndMethod, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope, @Advice.Local("otelCallDepth") CallDepth callDepth) { if (callDepth.decrementAndGet() > 0) { return; } - HttpServletResponseAdviceHelper.stopSpan(instrumenter(), throwable, context, scope, method); + HttpServletResponseAdviceHelper.stopSpan( + instrumenter(), throwable, context, scope, classAndMethod); } } diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/response/ResponseSingletons.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/response/ResponseSingletons.java index 1074fe5ea8e3..c8ad094fdd0c 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/response/ResponseSingletons.java +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/response/ResponseSingletons.java @@ -8,20 +8,20 @@ import io.opentelemetry.api.GlobalOpenTelemetry; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.instrumentation.api.tracer.SpanNames; -import java.lang.reflect.Method; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; public class ResponseSingletons { - private static final Instrumenter INSTRUMENTER; + private static final Instrumenter INSTRUMENTER; static { INSTRUMENTER = - Instrumenter.builder( + Instrumenter.builder( GlobalOpenTelemetry.get(), "io.opentelemetry.servlet-5.0", SpanNames::fromMethod) .newInstrumenter(); } - public static Instrumenter instrumenter() { + public static Instrumenter instrumenter() { return INSTRUMENTER; } } diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/response/HttpServletResponseAdviceHelper.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/response/HttpServletResponseAdviceHelper.java index 1dbb71a43626..f097e96ebf75 100644 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/response/HttpServletResponseAdviceHelper.java +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/response/HttpServletResponseAdviceHelper.java @@ -8,15 +8,15 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; -import java.lang.reflect.Method; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; public class HttpServletResponseAdviceHelper { public static void stopSpan( - Instrumenter instrumenter, + Instrumenter instrumenter, Throwable throwable, Context context, Scope scope, - Method request) { + ClassAndMethod request) { if (scope != null) { scope.close(); diff --git a/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/response/ResponseSendAdvice.java b/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/response/ResponseSendAdvice.java index 5343ace25d8c..beebfc0b9526 100644 --- a/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/response/ResponseSendAdvice.java +++ b/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/response/ResponseSendAdvice.java @@ -9,10 +9,10 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; import io.opentelemetry.javaagent.instrumentation.api.CallDepth; import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.instrumentation.servlet.common.response.HttpServletResponseAdviceHelper; -import java.lang.reflect.Method; import javax.servlet.http.HttpServletResponse; import net.bytebuddy.asm.Advice; @@ -21,7 +21,9 @@ public class ResponseSendAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void start( - @Advice.Origin Method method, + @Advice.Origin("#t") Class declaringClass, + @Advice.Origin("#m") String methodName, + @Advice.Local("otelMethod") ClassAndMethod classAndMethod, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope, @Advice.Local("otelCallDepth") CallDepth callDepth) { @@ -32,8 +34,9 @@ public static void start( Context parentContext = Java8BytecodeBridge.currentContext(); // Don't want to generate a new top-level span if (Java8BytecodeBridge.spanFromContext(parentContext).getSpanContext().isValid()) { - if (instrumenter().shouldStart(parentContext, method)) { - context = instrumenter().start(parentContext, method); + classAndMethod = ClassAndMethod.create(declaringClass, methodName); + if (instrumenter().shouldStart(parentContext, classAndMethod)) { + context = instrumenter().start(parentContext, classAndMethod); scope = context.makeCurrent(); } } @@ -41,14 +44,15 @@ public static void start( @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( - @Advice.Origin Method method, @Advice.Thrown Throwable throwable, + @Advice.Local("otelMethod") ClassAndMethod classAndMethod, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope, @Advice.Local("otelCallDepth") CallDepth callDepth) { if (callDepth.decrementAndGet() > 0) { return; } - HttpServletResponseAdviceHelper.stopSpan(instrumenter(), throwable, context, scope, method); + HttpServletResponseAdviceHelper.stopSpan( + instrumenter(), throwable, context, scope, classAndMethod); } } diff --git a/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/response/ResponseSingletons.java b/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/response/ResponseSingletons.java index a8c460faaaeb..83f84e788de2 100644 --- a/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/response/ResponseSingletons.java +++ b/instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/response/ResponseSingletons.java @@ -8,22 +8,22 @@ import io.opentelemetry.api.GlobalOpenTelemetry; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.instrumentation.api.tracer.SpanNames; -import java.lang.reflect.Method; +import io.opentelemetry.instrumentation.api.util.ClassAndMethod; public class ResponseSingletons { - private static final Instrumenter INSTRUMENTER; + private static final Instrumenter INSTRUMENTER; static { INSTRUMENTER = - Instrumenter.builder( + Instrumenter.builder( GlobalOpenTelemetry.get(), "io.opentelemetry.servlet-javax-common", SpanNames::fromMethod) .newInstrumenter(); } - public static Instrumenter instrumenter() { + public static Instrumenter instrumenter() { return INSTRUMENTER; } } From b1965ab39ba50ef98a3f5df16329dbf9159a8b9e Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 10 Nov 2021 00:14:47 +0200 Subject: [PATCH 2/4] remove sentence --- docs/contributing/writing-instrumentation.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/contributing/writing-instrumentation.md b/docs/contributing/writing-instrumentation.md index 6303a2aeaf29..17add9f29e0a 100644 --- a/docs/contributing/writing-instrumentation.md +++ b/docs/contributing/writing-instrumentation.md @@ -398,5 +398,4 @@ we prefer to use ``` because the former inserts a call to `Class.getMethod(...)` in transformed class. In contrast, getting the declaring class and method name is just loading constants from constant pool, which is -a much simpler operation. Considering that the majority of our use cases only need declaring class -and method name we can avoid calling `Class.getMethod(...)`. +a much simpler operation. From 505e1b3aaf7272e7b18672153b6cfbdeaeb2fd56 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 10 Nov 2021 12:45:03 +0200 Subject: [PATCH 3/4] Update docs/contributing/writing-instrumentation.md Co-authored-by: Trask Stalnaker --- docs/contributing/writing-instrumentation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/contributing/writing-instrumentation.md b/docs/contributing/writing-instrumentation.md index 17add9f29e0a..58e64d1e76c1 100644 --- a/docs/contributing/writing-instrumentation.md +++ b/docs/contributing/writing-instrumentation.md @@ -396,6 +396,6 @@ we prefer to use @Advice.Origin("#t") Class declaringClass, @Advice.Origin("#m") String methodName ``` -because the former inserts a call to `Class.getMethod(...)` in transformed class. In contrast, +because the former inserts a call to `Class.getMethod(...)` in transformed method. In contrast, getting the declaring class and method name is just loading constants from constant pool, which is a much simpler operation. From 0d75caa83399c3accacbdd7ea7741e654aaa9fef Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 10 Nov 2021 12:49:53 +0200 Subject: [PATCH 4/4] address review comment --- .../writing-instrumentation-module.md | 15 +++++++++++++++ docs/contributing/writing-instrumentation.md | 15 --------------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/docs/contributing/writing-instrumentation-module.md b/docs/contributing/writing-instrumentation-module.md index bc5b33ffb36f..d08ccb5c09ab 100644 --- a/docs/contributing/writing-instrumentation-module.md +++ b/docs/contributing/writing-instrumentation-module.md @@ -340,3 +340,18 @@ bytecode tweaks to optimize it. Because of this, retrieving a `VirtualField` ins limited: the `VirtualField#get()` method must receive class references as its parameters; it won't work with variables, method params, etc. Both the owner class and the field class must be known at compile time for it to work. + +### Why we don't use ByteBuddy @Advice.Origin Method + +Instead of +``` +@Advice.Origin Method method +``` +we prefer to use +``` +@Advice.Origin("#t") Class declaringClass, +@Advice.Origin("#m") String methodName +``` +because the former inserts a call to `Class.getMethod(...)` in transformed method. In contrast, +getting the declaring class and method name is just loading constants from constant pool, which is +a much simpler operation. diff --git a/docs/contributing/writing-instrumentation.md b/docs/contributing/writing-instrumentation.md index 58e64d1e76c1..7f6d52f95f0a 100644 --- a/docs/contributing/writing-instrumentation.md +++ b/docs/contributing/writing-instrumentation.md @@ -384,18 +384,3 @@ dependencies { testImplementation(project(":instrumentation:yarpc-1.0:javaagent")) } ``` - -## Why we don't use ByteBuddy @Advice.Origin Method - -Instead of -``` -@Advice.Origin Method method -``` -we prefer to use -``` -@Advice.Origin("#t") Class declaringClass, -@Advice.Origin("#m") String methodName -``` -because the former inserts a call to `Class.getMethod(...)` in transformed method. In contrast, -getting the declaring class and method name is just loading constants from constant pool, which is -a much simpler operation.