Fail test if advice threw an exception (#9654)

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

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 {

instrumentation/aws-lambda/aws-lambda-core-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awslambdacore/v1_0/AwsLambdaInstrumentationModule.java

@@ -5,12 +5,15 @@
 
 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 {
@@ -18,6 +21,12 @@ public class AwsLambdaInstrumentationModule extends InstrumentationModule {
     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.");

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.bootstrap.kafka.KafkaClientsConsumerProcessTra
 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 class KafkaConsumerInstrumentation implements TypeInstrumentation {
   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);
     }
   }

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.instrumentation.kafka.internal.KafkaPropagation;
 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 class KafkaProducerInstrumentation implements TypeInstrumentation {
   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);
     }
   }

instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4/common/AbstractNettyChannelPipelineInstrumentation.java

@@ -71,7 +71,9 @@ public abstract class AbstractNettyChannelPipelineInstrumentation implements Typ
           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 abstract class AbstractNettyChannelPipelineInstrumentation implements Typ
           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 abstract class AbstractNettyChannelPipelineInstrumentation implements Typ
           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 abstract class AbstractNettyChannelPipelineInstrumentation implements Typ
           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);
       }
     }

instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2SpanNameExtractor.java

@@ -32,7 +32,7 @@ public class Servlet2SpanNameExtractor<REQUEST, RESPONSE>
     if (!knownMethods.contains(method)) {
       method = "HTTP";
     }
-    if (servletPath.isEmpty()) {
+    if (servletPath == null || servletPath.isEmpty()) {
       return method;
     }
     String contextPath = accessor.getRequestContextPath(request);

javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/ExceptionLogger.java

@@ -7,6 +7,7 @@ package io.opentelemetry.javaagent.bootstrap;
 
 import static java.util.logging.Level.FINE;
 
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.logging.Logger;
 
 /**
@@ -17,11 +18,18 @@ import java.util.logging.Logger;
 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() {}

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")

testing-common/src/main/java/io/opentelemetry/instrumentation/testing/AgentTestRunner.java

@@ -55,6 +55,8 @@ public final class AgentTestRunner extends InstrumentationTestRunner {
     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

testing-common/src/main/java/io/opentelemetry/javaagent/testing/common/TestAgentListenerAccess.java

@@ -7,6 +7,7 @@ package io.opentelemetry.javaagent.testing.common;
 
 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 final class TestAgentListenerAccess {
     }
   }
 
+  public static int getAndResetAdviceFailureCount() {
+    return ExceptionLogger.getAndReset();
+  }
+
   private TestAgentListenerAccess() {}
 }