From d4c58ecde2a648cee433e00ef02cec0170362001 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Mon, 7 Dec 2020 12:33:34 +0900 Subject: [PATCH 1/4] Switch gRPC instrumentation to instrument public ServerBuilder class. --- .../javaagent/grpc-1.5-javaagent.gradle | 8 +-- .../grpc/v1_5/GrpcInstrumentationModule.java | 7 ++ .../GrpcServerBuilderInstrumentation.java | 68 ++++++++++++++----- 3 files changed, 59 insertions(+), 24 deletions(-) diff --git a/instrumentation/grpc-1.5/javaagent/grpc-1.5-javaagent.gradle b/instrumentation/grpc-1.5/javaagent/grpc-1.5-javaagent.gradle index 8cac1ce4ec56..603cb3f08159 100644 --- a/instrumentation/grpc-1.5/javaagent/grpc-1.5-javaagent.gradle +++ b/instrumentation/grpc-1.5/javaagent/grpc-1.5-javaagent.gradle @@ -20,7 +20,7 @@ muzzle { pass { group = "io.grpc" module = "grpc-core" - versions = "[1.5.0, 1.33.0)" // see https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/1453 + versions = "[1.5.0,)" } } @@ -35,11 +35,5 @@ dependencies { testLibrary group: 'io.grpc', name: 'grpc-protobuf', version: grpcVersion testLibrary group: 'io.grpc', name: 'grpc-stub', version: grpcVersion - // see https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/1453 - latestDepTestLibrary group: 'io.grpc', name: 'grpc-core', version: '1.32.+' - latestDepTestLibrary group: 'io.grpc', name: 'grpc-netty', version: '1.32.+' - latestDepTestLibrary group: 'io.grpc', name: 'grpc-protobuf', version: '1.32.+' - latestDepTestLibrary group: 'io.grpc', name: 'grpc-stub', version: '1.32.+' - testImplementation project(':instrumentation:grpc-1.5:testing') } \ No newline at end of file diff --git a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcInstrumentationModule.java b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcInstrumentationModule.java index 50e016e5d789..ac5d861ede60 100644 --- a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcInstrumentationModule.java +++ b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcInstrumentationModule.java @@ -10,7 +10,9 @@ import com.google.auto.service.AutoService; import io.opentelemetry.javaagent.tooling.InstrumentationModule; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; +import java.util.Collections; import java.util.List; +import java.util.Map; @AutoService(InstrumentationModule.class) public class GrpcInstrumentationModule extends InstrumentationModule { @@ -23,4 +25,9 @@ public List typeInstrumentations() { return asList( new GrpcClientBuilderBuildInstrumentation(), new GrpcServerBuilderInstrumentation()); } + + @Override + protected Map contextStore() { + return Collections.singletonMap("io.grpc.ServerBuilder", Boolean.class.getName()); + } } diff --git a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java index 21b7fe46af09..0d8064c6c716 100644 --- a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java +++ b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java @@ -5,14 +5,21 @@ package io.opentelemetry.javaagent.instrumentation.grpc.v1_5; -import static java.util.Collections.singletonMap; +import static io.opentelemetry.javaagent.tooling.ClassLoaderMatcher.hasClassesNamed; +import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.AgentElementMatchers.safeHasSuperType; import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; +import io.grpc.ServerBuilder; import io.grpc.ServerInterceptor; import io.opentelemetry.instrumentation.grpc.v1_5.server.TracingServerInterceptor; +import io.opentelemetry.javaagent.instrumentation.api.ContextStore; +import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; -import java.util.List; +import java.util.HashMap; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -21,33 +28,60 @@ public class GrpcServerBuilderInstrumentation implements TypeInstrumentation { + @Override + public ElementMatcher classLoaderOptimization() { + return hasClassesNamed("io.grpc.ServerBuilder"); + } + @Override public ElementMatcher typeMatcher() { - return named("io.grpc.internal.AbstractServerImplBuilder"); + return safeHasSuperType(named("io.grpc.ServerBuilder")); } @Override public Map, String> transformers() { - return singletonMap( - isMethod().and(named("build")), - GrpcServerBuilderInstrumentation.class.getName() + "$AddInterceptorAdvice"); + Map, String> transformers = new HashMap<>(); + transformers.put( + isMethod() + .and(isPublic()) + .and(named("intercept")) + .and(takesArgument(0, named("io.grpc.ServerInterceptor"))), + GrpcServerBuilderInstrumentation.class.getName() + "$InterceptAdvice"); + transformers.put( + isMethod().and(isPublic()).and(named("build")).and(takesArguments(0)), + GrpcServerBuilderInstrumentation.class.getName() + "$BuildAdvice"); + return transformers; } - public static class AddInterceptorAdvice { + public static class InterceptAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static void addInterceptor( - @Advice.FieldValue("interceptors") List interceptors) { - boolean shouldRegister = true; - for (ServerInterceptor interceptor : interceptors) { - if (interceptor instanceof TracingServerInterceptor) { - shouldRegister = false; - break; - } + public static void onEnter( + @Advice.This ServerBuilder serverBuilder, + @Advice.Argument(0) ServerInterceptor interceptor) { + // Check against unshaded name. + if (interceptor + .getClass() + .getName() + .equals("io.opentelemetry.instrumentation.grpc.v1_5.server.TracingServerInterceptor")) { + @SuppressWarnings("rawtypes") + ContextStore instrumentationContext = + InstrumentationContext.get(ServerBuilder.class, Boolean.class); + instrumentationContext.put(serverBuilder, true); } - if (shouldRegister) { - interceptors.add(TracingServerInterceptor.newInterceptor()); + } + } + + public static class BuildAdvice { + + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void onEnter(@Advice.This ServerBuilder serverBuilder) { + ContextStore instrumentationContext = + InstrumentationContext.get(ServerBuilder.class, Boolean.class); + if (Boolean.TRUE.equals(instrumentationContext.get(serverBuilder))) { + return; } + serverBuilder.intercept(TracingServerInterceptor.newInterceptor()); } } } From 58e54bc42f2e3818c51cdb9f9741ce01e522cc6d Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Mon, 7 Dec 2020 12:35:40 +0900 Subject: [PATCH 2/4] A bit more readability (hopefully) --- .../grpc/v1_5/GrpcServerBuilderInstrumentation.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java index 0d8064c6c716..0aa4e80de2ab 100644 --- a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java +++ b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java @@ -67,7 +67,7 @@ public static void onEnter( @SuppressWarnings("rawtypes") ContextStore instrumentationContext = InstrumentationContext.get(ServerBuilder.class, Boolean.class); - instrumentationContext.put(serverBuilder, true); + instrumentationContext.put(serverBuilder, /* alreadyRegistered= */ true); } } } @@ -78,7 +78,8 @@ public static class BuildAdvice { public static void onEnter(@Advice.This ServerBuilder serverBuilder) { ContextStore instrumentationContext = InstrumentationContext.get(ServerBuilder.class, Boolean.class); - if (Boolean.TRUE.equals(instrumentationContext.get(serverBuilder))) { + boolean alreadyRegistered = instrumentationContext.get(serverBuilder); + if (Boolean.TRUE.equals(alreadyRegistered)) { return; } serverBuilder.intercept(TracingServerInterceptor.newInterceptor()); From 37dc02410a20673b2573311b1954c9ae9cfe67e5 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Mon, 7 Dec 2020 15:48:25 +0900 Subject: [PATCH 3/4] hack --- .../grpc/v1_5/GrpcServerBuilderInstrumentation.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java index 0aa4e80de2ab..55b5a3032c7e 100644 --- a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java +++ b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java @@ -28,6 +28,11 @@ public class GrpcServerBuilderInstrumentation implements TypeInstrumentation { + // Computing the name at runtime is the simplest way to make sure the String doesn't get shaded. + private static final String LIBRARY_INSTRUMENTATION_INTERCEPTOR_NAME = + "library.io.opentelemetry.instrumentation.grpc.v1_5.server.TracingServerInterceptor" + .substring("library.".length()); + @Override public ElementMatcher classLoaderOptimization() { return hasClassesNamed("io.grpc.ServerBuilder"); @@ -60,10 +65,7 @@ public static void onEnter( @Advice.This ServerBuilder serverBuilder, @Advice.Argument(0) ServerInterceptor interceptor) { // Check against unshaded name. - if (interceptor - .getClass() - .getName() - .equals("io.opentelemetry.instrumentation.grpc.v1_5.server.TracingServerInterceptor")) { + if (interceptor.getClass().getName().equals(LIBRARY_INSTRUMENTATION_INTERCEPTOR_NAME)) { @SuppressWarnings("rawtypes") ContextStore instrumentationContext = InstrumentationContext.get(ServerBuilder.class, Boolean.class); From e34e557cc6fc2750f7597c7cddb52fee6457dd15 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Mon, 7 Dec 2020 15:56:40 +0900 Subject: [PATCH 4/4] Remove library dedupe for now since we need a better story. --- .../grpc/v1_5/GrpcInstrumentationModule.java | 7 --- .../GrpcServerBuilderInstrumentation.java | 52 ++++--------------- 2 files changed, 11 insertions(+), 48 deletions(-) diff --git a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcInstrumentationModule.java b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcInstrumentationModule.java index ac5d861ede60..50e016e5d789 100644 --- a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcInstrumentationModule.java +++ b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcInstrumentationModule.java @@ -10,9 +10,7 @@ import com.google.auto.service.AutoService; import io.opentelemetry.javaagent.tooling.InstrumentationModule; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; -import java.util.Collections; import java.util.List; -import java.util.Map; @AutoService(InstrumentationModule.class) public class GrpcInstrumentationModule extends InstrumentationModule { @@ -25,9 +23,4 @@ public List typeInstrumentations() { return asList( new GrpcClientBuilderBuildInstrumentation(), new GrpcServerBuilderInstrumentation()); } - - @Override - protected Map contextStore() { - return Collections.singletonMap("io.grpc.ServerBuilder", Boolean.class.getName()); - } } diff --git a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java index 55b5a3032c7e..d5c21f08d662 100644 --- a/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java +++ b/instrumentation/grpc-1.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grpc/v1_5/GrpcServerBuilderInstrumentation.java @@ -10,16 +10,13 @@ import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.named; -import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import static net.bytebuddy.matcher.ElementMatchers.takesArguments; import io.grpc.ServerBuilder; -import io.grpc.ServerInterceptor; import io.opentelemetry.instrumentation.grpc.v1_5.server.TracingServerInterceptor; -import io.opentelemetry.javaagent.instrumentation.api.ContextStore; -import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext; +import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; -import java.util.HashMap; +import java.util.Collections; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -28,11 +25,6 @@ public class GrpcServerBuilderInstrumentation implements TypeInstrumentation { - // Computing the name at runtime is the simplest way to make sure the String doesn't get shaded. - private static final String LIBRARY_INSTRUMENTATION_INTERCEPTOR_NAME = - "library.io.opentelemetry.instrumentation.grpc.v1_5.server.TracingServerInterceptor" - .substring("library.".length()); - @Override public ElementMatcher classLoaderOptimization() { return hasClassesNamed("io.grpc.ServerBuilder"); @@ -45,46 +37,24 @@ public ElementMatcher typeMatcher() { @Override public Map, String> transformers() { - Map, String> transformers = new HashMap<>(); - transformers.put( - isMethod() - .and(isPublic()) - .and(named("intercept")) - .and(takesArgument(0, named("io.grpc.ServerInterceptor"))), - GrpcServerBuilderInstrumentation.class.getName() + "$InterceptAdvice"); - transformers.put( + return Collections.singletonMap( isMethod().and(isPublic()).and(named("build")).and(takesArguments(0)), GrpcServerBuilderInstrumentation.class.getName() + "$BuildAdvice"); - return transformers; - } - - public static class InterceptAdvice { - - @Advice.OnMethodEnter(suppress = Throwable.class) - public static void onEnter( - @Advice.This ServerBuilder serverBuilder, - @Advice.Argument(0) ServerInterceptor interceptor) { - // Check against unshaded name. - if (interceptor.getClass().getName().equals(LIBRARY_INSTRUMENTATION_INTERCEPTOR_NAME)) { - @SuppressWarnings("rawtypes") - ContextStore instrumentationContext = - InstrumentationContext.get(ServerBuilder.class, Boolean.class); - instrumentationContext.put(serverBuilder, /* alreadyRegistered= */ true); - } - } } public static class BuildAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter(@Advice.This ServerBuilder serverBuilder) { - ContextStore instrumentationContext = - InstrumentationContext.get(ServerBuilder.class, Boolean.class); - boolean alreadyRegistered = instrumentationContext.get(serverBuilder); - if (Boolean.TRUE.equals(alreadyRegistered)) { - return; + int callDepth = CallDepthThreadLocalMap.incrementCallDepth(ServerBuilder.class); + if (callDepth == 0) { + serverBuilder.intercept(TracingServerInterceptor.newInterceptor()); } - serverBuilder.intercept(TracingServerInterceptor.newInterceptor()); + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void onExit(@Advice.This ServerBuilder serverBuilder) { + CallDepthThreadLocalMap.decrementCallDepth(ServerBuilder.class); } } }