Fail test if advice threw an exception (#9654)

laurit · trask · web-flow · commit 0de1dcff452d · 2023-10-11T19:17:20.000Z
Co-authored-by: Trask Stalnaker &lt;trask.stalnaker@gmail.com&gt;
diff --git a/instrumentation/aws-lambda/aws-lambda-core-1.0/javaagent/build.gradle.kts b/instrumentation/aws-lambda/aws-lambda-core-1.0/javaagent/build.gradle.kts
@@ -19,7 +19,6 @@ dependencies {
   library("com.amazonaws:aws-lambda-java-core:1.0.0")
 
   testImplementation(project(":instrumentation:aws-lambda:aws-lambda-core-1.0:testing"))
-  testInstrumentation(project(":instrumentation:aws-lambda:aws-lambda-events-2.2:javaagent"))
 }
 
 tasks.withType<Test>().configureEach {
diff --git a/instrumentation/aws-lambda/aws-lambda-core-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awslambdacore/v1_0/AwsLambdaInstrumentationModule.java b/instrumentation/aws-lambda/aws-lambda-core-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awslambdacore/v1_0/AwsLambdaInstrumentationModule.java
@@ -5,19 +5,28 @@
 
 package io.opentelemetry.javaagent.instrumentation.awslambdacore.v1_0;
 
+import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.hasClassesNamed;
 import static java.util.Collections.singletonList;
+import static net.bytebuddy.matcher.ElementMatchers.not;
 
 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 AwsLambdaInstrumentationModule extends InstrumentationModule {
   public AwsLambdaInstrumentationModule() {
     super("aws-lambda-core", "aws-lambda-core-1.0", "aws-lambda");
   }
 
+  @Override
+  public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
+    // aws-lambda-events-2.2 is used when SQSEvent is present
+    return not(hasClassesNamed("com.amazonaws.services.lambda.runtime.events.SQSEvent"));
+  }
+
   @Override
   public boolean isHelperClass(String className) {
     return className.startsWith("io.opentelemetry.contrib.awsxray.");
diff --git a/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaConsumerInstrumentation.java b/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaConsumerInstrumentation.java
@@ -24,6 +24,7 @@
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
 import java.time.Duration;
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Properties;
 import net.bytebuddy.asm.Advice;
@@ -61,7 +62,12 @@ public void transform(TypeTransformer transformer) {
   public static class ConstructorMapAdvice {
 
     @Advice.OnMethodEnter(suppress = Throwable.class)
-    public static void onEnter(@Advice.Argument(0) Map<String, Object> config) {
+    public static void onEnter(
+        @Advice.Argument(value = 0, readOnly = false) Map<String, Object> config) {
+      // ensure config is a mutable map
+      if (config.getClass() != HashMap.class) {
+        config = new HashMap<>(config);
+      }
       enhanceConfig(config);
     }
   }
diff --git a/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaProducerInstrumentation.java b/instrumentation/kafka/kafka-clients/kafka-clients-0.11/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaProducerInstrumentation.java
@@ -20,6 +20,7 @@
 import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Properties;
 import net.bytebuddy.asm.Advice;
@@ -57,7 +58,12 @@ public void transform(TypeTransformer transformer) {
   public static class ConstructorMapAdvice {
 
     @Advice.OnMethodEnter(suppress = Throwable.class)
-    public static void onEnter(@Advice.Argument(0) Map<String, Object> config) {
+    public static void onEnter(
+        @Advice.Argument(value = 0, readOnly = false) Map<String, Object> config) {
+      // ensure config is a mutable map
+      if (config.getClass() != HashMap.class) {
+        config = new HashMap<>(config);
+      }
       enhanceConfig(config);
     }
   }
diff --git a/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4/common/AbstractNettyChannelPipelineInstrumentation.java b/instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4/common/AbstractNettyChannelPipelineInstrumentation.java
@@ -71,7 +71,9 @@ public static void removeHandler(
           VirtualField.find(ChannelHandler.class, ChannelHandler.class);
       ChannelHandler ourHandler = virtualField.get(handler);
       if (ourHandler != null) {
-        pipeline.remove(ourHandler);
+        if (pipeline.context(ourHandler) != null) {
+          pipeline.remove(ourHandler);
+        }
         virtualField.set(handler, null);
       }
     }
@@ -92,7 +94,9 @@ public static void removeHandler(
           VirtualField.find(ChannelHandler.class, ChannelHandler.class);
       ChannelHandler ourHandler = virtualField.get(handler);
       if (ourHandler != null) {
-        pipeline.remove(ourHandler);
+        if (pipeline.context(ourHandler) != null) {
+          pipeline.remove(ourHandler);
+        }
         virtualField.set(handler, null);
       }
     }
@@ -114,7 +118,9 @@ public static void removeHandler(
           VirtualField.find(ChannelHandler.class, ChannelHandler.class);
       ChannelHandler ourHandler = virtualField.get(handler);
       if (ourHandler != null) {
-        pipeline.remove(ourHandler);
+        if (pipeline.context(ourHandler) != null) {
+          pipeline.remove(ourHandler);
+        }
         virtualField.set(handler, null);
       }
     }
@@ -130,7 +136,9 @@ public static void removeHandler(
           VirtualField.find(ChannelHandler.class, ChannelHandler.class);
       ChannelHandler ourHandler = virtualField.get(handler);
       if (ourHandler != null) {
-        pipeline.remove(ourHandler);
+        if (pipeline.context(ourHandler) != null) {
+          pipeline.remove(ourHandler);
+        }
         virtualField.set(handler, null);
       }
     }
diff --git a/instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2SpanNameExtractor.java b/instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2SpanNameExtractor.java
@@ -32,7 +32,7 @@ public String extract(ServletRequestContext<REQUEST> requestContext) {
     if (!knownMethods.contains(method)) {
       method = "HTTP";
     }
-    if (servletPath.isEmpty()) {
+    if (servletPath == null || servletPath.isEmpty()) {
       return method;
     }
     String contextPath = accessor.getRequestContextPath(request);
diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/ExceptionLogger.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/ExceptionLogger.java
@@ -7,6 +7,7 @@
 
 import static java.util.logging.Level.FINE;
 
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.logging.Logger;
 
 /**
@@ -17,11 +18,18 @@
 public final class ExceptionLogger {
 
   private static final Logger logger = Logger.getLogger(ExceptionLogger.class.getName());
+  private static final AtomicInteger counter = new AtomicInteger();
 
   /** See {@code io.opentelemetry.javaagent.tooling.ExceptionHandlers} for usages. */
   @SuppressWarnings("unused")
   public static void logSuppressedError(String message, Throwable error) {
     logger.log(FINE, message, error);
+    counter.incrementAndGet();
+  }
+
+  // only used by tests
+  public static int getAndReset() {
+    return counter.getAndSet(0);
   }
 
   private ExceptionLogger() {}
diff --git a/testing-common/build.gradle.kts b/testing-common/build.gradle.kts
@@ -46,6 +46,7 @@ dependencies {
   api("org.slf4j:slf4j-api")
 
   compileOnly(project(":testing:armeria-shaded-for-testing", configuration = "shadow"))
+  compileOnly(project(":javaagent-bootstrap"))
 
   compileOnly("com.google.auto.value:auto-value-annotations")
   annotationProcessor("com.google.auto.value:auto-value")
diff --git a/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/AgentTestRunner.java b/testing-common/src/main/java/io/opentelemetry/instrumentation/testing/AgentTestRunner.java
@@ -55,6 +55,8 @@ public void afterTestClass() {
     assert TestAgentListenerAccess.getInstrumentationErrorCount() == 0
         : TestAgentListenerAccess.getInstrumentationErrorCount()
             + " Instrumentation errors during test";
+    int adviceFailureCount = TestAgentListenerAccess.getAndResetAdviceFailureCount();
+    assert adviceFailureCount == 0 : adviceFailureCount + " Advice failures during test";
     int muzzleFailureCount = TestAgentListenerAccess.getAndResetMuzzleFailureCount();
     assert muzzleFailureCount == 0 : muzzleFailureCount + " Muzzle failures during test";
     // additional library ignores are ignored during tests, because they can make it really
diff --git a/testing-common/src/main/java/io/opentelemetry/javaagent/testing/common/TestAgentListenerAccess.java b/testing-common/src/main/java/io/opentelemetry/javaagent/testing/common/TestAgentListenerAccess.java
@@ -7,6 +7,7 @@
 
 import static java.lang.invoke.MethodType.methodType;
 
+import io.opentelemetry.javaagent.bootstrap.ExceptionLogger;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.util.List;
@@ -105,5 +106,9 @@ public static void addSkipErrorCondition(BiFunction<String, Throwable, Boolean>
     }
   }
 
+  public static int getAndResetAdviceFailureCount() {
+    return ExceptionLogger.getAndReset();
+  }
+
   private TestAgentListenerAccess() {}
 }

Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,6 @@ dependencies {`
`19`	`19`	`library("com.amazonaws:aws-lambda-java-core:1.0.0")`
`20`	`20`
`21`	`21`	`testImplementation(project(":instrumentation:aws-lambda:aws-lambda-core-1.0:testing"))`
`22`		`- testInstrumentation(project(":instrumentation:aws-lambda:aws-lambda-events-2.2:javaagent"))`
`23`	`22`	`}`
`24`	`23`
`25`	`24`	`tasks.withType<Test>().configureEach {`
Original file line number	Diff line number	Diff line change
`@@ -71,7 +71,9 @@ public static void removeHandler(`
`71`	`71`	`VirtualField.find(ChannelHandler.class, ChannelHandler.class);`
`72`	`72`	`ChannelHandler ourHandler = virtualField.get(handler);`
`73`	`73`	`if (ourHandler != null) {`
`74`		`- pipeline.remove(ourHandler);`
	`74`	`+ if (pipeline.context(ourHandler) != null) {`
	`75`	`+ pipeline.remove(ourHandler);`
	`76`	`+ }`
`75`	`77`	`virtualField.set(handler, null);`
`76`	`78`	`}`
`77`	`79`	`}`
`@@ -92,7 +94,9 @@ public static void removeHandler(`
`92`	`94`	`VirtualField.find(ChannelHandler.class, ChannelHandler.class);`
`93`	`95`	`ChannelHandler ourHandler = virtualField.get(handler);`
`94`	`96`	`if (ourHandler != null) {`
`95`		`- pipeline.remove(ourHandler);`
	`97`	`+ if (pipeline.context(ourHandler) != null) {`
	`98`	`+ pipeline.remove(ourHandler);`
	`99`	`+ }`
`96`	`100`	`virtualField.set(handler, null);`
`97`	`101`	`}`
`98`	`102`	`}`
`@@ -114,7 +118,9 @@ public static void removeHandler(`
`114`	`118`	`VirtualField.find(ChannelHandler.class, ChannelHandler.class);`
`115`	`119`	`ChannelHandler ourHandler = virtualField.get(handler);`
`116`	`120`	`if (ourHandler != null) {`
`117`		`- pipeline.remove(ourHandler);`
	`121`	`+ if (pipeline.context(ourHandler) != null) {`
	`122`	`+ pipeline.remove(ourHandler);`
	`123`	`+ }`
`118`	`124`	`virtualField.set(handler, null);`
`119`	`125`	`}`
`120`	`126`	`}`
`@@ -130,7 +136,9 @@ public static void removeHandler(`
`130`	`136`	`VirtualField.find(ChannelHandler.class, ChannelHandler.class);`
`131`	`137`	`ChannelHandler ourHandler = virtualField.get(handler);`
`132`	`138`	`if (ourHandler != null) {`
`133`		`- pipeline.remove(ourHandler);`
	`139`	`+ if (pipeline.context(ourHandler) != null) {`
	`140`	`+ pipeline.remove(ourHandler);`
	`141`	`+ }`
`134`	`142`	`virtualField.set(handler, null);`
`135`	`143`	`}`
`136`	`144`	`}`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ public String extract(ServletRequestContext<REQUEST> requestContext) {`
`32`	`32`	`if (!knownMethods.contains(method)) {`
`33`	`33`	`method = "HTTP";`
`34`	`34`	`}`
`35`		`- if (servletPath.isEmpty()) {`
	`35`	`+ if (servletPath == null \|\| servletPath.isEmpty()) {`
`36`	`36`	`return method;`
`37`	`37`	`}`
`38`	`38`	`String contextPath = accessor.getRequestContextPath(request);`
Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,7 @@`
`7`	`7`
`8`	`8`	`import static java.lang.invoke.MethodType.methodType;`
`9`	`9`
	`10`	`+import io.opentelemetry.javaagent.bootstrap.ExceptionLogger;`
`10`	`11`	`import java.lang.invoke.MethodHandle;`
`11`	`12`	`import java.lang.invoke.MethodHandles;`
`12`	`13`	`import java.util.List;`
`@@ -105,5 +106,9 @@ public static void addSkipErrorCondition(BiFunction<String, Throwable, Boolean>`
`105`	`106`	`}`
`106`	`107`	`}`
`107`	`108`
	`109`	`+ public static int getAndResetAdviceFailureCount() {`
	`110`	`+ return ExceptionLogger.getAndReset();`
	`111`	`+ }`
	`112`	`+`
`108`	`113`	`private TestAgentListenerAccess() {}`
`109`	`114`	`}`