Cach all exceptions in advices (#380)

closes #351
elastic · Dec 11, 2018 · e7008f0 · e7008f0
1 parent bd6ff30
commit e7008f0
Show file tree

Hide file tree

Showing 25 changed files with 139 additions and 52 deletions.
diff --git a/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationTest.java b/apm-agent-core/src/test/java/co/elastic/apm/agent/bci/InstrumentationTest.java
@@ -34,10 +34,16 @@
 import org.junit.jupiter.api.Test;
 import org.stagemonitor.configuration.ConfigurationRegistry;
 
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
 import java.util.Collection;
 import java.util.Collections;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.Mockito.when;
 
 class InstrumentationTest {
@@ -72,6 +78,38 @@ void testDontInstrumentOldClassFileVersions() {
         assertThat(MathUtils.sign(-42)).isEqualTo(-1);
     }
 
+    @Test
+    void testSuppressException() {
+        ElasticApmAgent.initInstrumentation(MockTracer.create(),
+            ByteBuddyAgent.install(),
+            Collections.singletonList(new SuppressExceptionInstrumentation()));
+        assertThat(noExceptionPlease("foo")).isEqualTo("foo_no_exception");
+    }
+
+    @Test
+    void testRetainExceptionInUserCode() {
+        ElasticApmAgent.initInstrumentation(MockTracer.create(),
+            ByteBuddyAgent.install(),
+            Collections.singletonList(new SuppressExceptionInstrumentation()));
+        assertThatThrownBy(this::exceptionPlease).isInstanceOf(NullPointerException.class);
+    }
+
+    @Test
+    void testNonSuppressedException() {
+        ElasticApmAgent.initInstrumentation(MockTracer.create(),
+            ByteBuddyAgent.install(),
+            Collections.singletonList(new ExceptionInstrumentation()));
+        assertThatThrownBy(() -> noExceptionPlease("foo")).isInstanceOf(RuntimeException.class);
+    }
+
+    String noExceptionPlease(String s) {
+        return s + "_no_exception";
+    }
+
+    String exceptionPlease() {
+        throw null;
+    }
+
     private void init(ConfigurationRegistry config) {
         ElasticApmAgent.initInstrumentation(new ElasticApmTracerBuilder()
                 .configurationRegistry(config)
@@ -127,4 +165,49 @@ public Collection<String> getInstrumentationGroupNames() {
             return Collections.emptyList();
         }
     }
+
+    public static class ExceptionInstrumentation extends ElasticApmInstrumentation {
+        @Advice.OnMethodExit
+        public static void onMethodExit() {
+            throw new RuntimeException("This exception should not be suppressed");
+        }
+
+        @Override
+        public ElementMatcher<? super TypeDescription> getTypeMatcher() {
+            return ElementMatchers.named(InstrumentationTest.class.getName());
+        }
+
+        @Override
+        public ElementMatcher<? super MethodDescription> getMethodMatcher() {
+            return ElementMatchers.nameEndsWithIgnoreCase("exceptionPlease");
+        }
+
+        @Override
+        public Collection<String> getInstrumentationGroupNames() {
+            return Collections.emptyList();
+        }
+    }
+
+    public static class SuppressExceptionInstrumentation extends ElasticApmInstrumentation {
+        @Advice.OnMethodExit(suppress = Throwable.class)
+        @Advice.OnMethodEnter(suppress = Throwable.class)
+        public static void onMethodEnterAndExit() {
+            throw new RuntimeException("This exception should be suppressed");
+        }
+
+        @Override
+        public ElementMatcher<? super TypeDescription> getTypeMatcher() {
+            return ElementMatchers.named(InstrumentationTest.class.getName());
+        }
+
+        @Override
+        public ElementMatcher<? super MethodDescription> getMethodMatcher() {
+            return ElementMatchers.named("noExceptionPlease");
+        }
+
+        @Override
+        public Collection<String> getInstrumentationGroupNames() {
+            return Collections.emptyList();
+        }
+    }
 }
diff --git a/apm-agent-plugins/README.md b/apm-agent-plugins/README.md
@@ -17,3 +17,7 @@ follow these steps:
 1. Add a line in [`CHANGELOG.md`](../CHANGELOG.md) 
 
 
+NOTE: when adding advices, make sure to add `(suppress = Throwable.class)`
+to the `net.bytebuddy.asm.Advice.OnMethodEnter` and `net.bytebuddy.asm.Advice.OnMethodExit` annotations. 
+Search for the regex `@.*OnMethod(Enter|Exit)(?!\(s)` to find annotations with a missing `suppress` attribute. 
+
diff --git a/...plugin/src/main/java/co/elastic/apm/agent/httpclient/ApacheHttpClientInstrumentation.java b/...plugin/src/main/java/co/elastic/apm/agent/httpclient/ApacheHttpClientInstrumentation.java
@@ -49,7 +49,7 @@ public class ApacheHttpClientInstrumentation extends ElasticApmInstrumentation {
 
     private static final String SPAN_TYPE_APACHE_HTTP_CLIENT = HTTP_CLIENT_SPAN_TYPE_PREFIX + "apache-httpclient";
 
-    @Advice.OnMethodEnter
+    @Advice.OnMethodEnter(suppress = Throwable.class)
     private static void onBeforeExecute(@Advice.Argument(0) HttpRoute route,
                                         @Advice.Argument(1) HttpRequestWrapper request,
                                         @Advice.Local("span") Span span) {
@@ -66,7 +66,7 @@ private static void onBeforeExecute(@Advice.Argument(0) HttpRoute route,
         }
     }
 
-    @Advice.OnMethodExit(onThrowable = Throwable.class)
+    @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
     public static void onAfterExecute(@Advice.Return CloseableHttpResponse response,
                                       @Advice.Local("span") @Nullable Span span,
                                       @Advice.Thrown @Nullable Throwable t) {

diff --git a/...apm-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/ApiScopeInstrumentation.java b/...apm-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/ApiScopeInstrumentation.java
@@ -40,7 +40,7 @@ public ElementMatcher<? super MethodDescription> getMethodMatcher() {
         return named("close");
     }
 
-    @Advice.OnMethodEnter
+    @Advice.OnMethodEnter(suppress = Throwable.class)
     private static void close(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span) {
         span.deactivate();
     }

diff --git a/...plugin/src/main/java/co/elastic/apm/agent/plugin/api/CaptureExceptionInstrumentation.java b/...plugin/src/main/java/co/elastic/apm/agent/plugin/api/CaptureExceptionInstrumentation.java
@@ -33,7 +33,7 @@
 
 public class CaptureExceptionInstrumentation extends ApiInstrumentation {
 
-    @Advice.OnMethodEnter
+    @Advice.OnMethodEnter(suppress = Throwable.class)
     public static void captureException(@Advice.Argument(0) Throwable t) {
         if (tracer != null) {
             tracer.captureException(t);

diff --git a/...-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/CaptureSpanInstrumentation.java b/...-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/CaptureSpanInstrumentation.java
@@ -54,7 +54,7 @@ public class CaptureSpanInstrumentation extends ElasticApmInstrumentation {
 
     private StacktraceConfiguration config;
 
-    @Advice.OnMethodEnter(inline = true)
+    @Advice.OnMethodEnter(suppress = Throwable.class)
     public static void onMethodEnter(@SimpleMethodSignatureOffsetMappingFactory.SimpleMethodSignature String signature,
                                      @AnnotationValueOffsetMappingFactory.AnnotationValueExtractor(annotationClassName = "co.elastic.apm.api.CaptureSpan", method = "value") String spanName,
                                      @AnnotationValueOffsetMappingFactory.AnnotationValueExtractor(annotationClassName = "co.elastic.apm.api.CaptureSpan", method = "type") String type,
@@ -73,7 +73,7 @@ public static void onMethodEnter(@SimpleMethodSignatureOffsetMappingFactory.Simp
 
     }
 
-    @Advice.OnMethodExit(inline = true, onThrowable = Throwable.class)
+    @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
     public static void onMethodExit(@Nullable @Advice.Local("span") Span span,
                                     @Advice.Thrown Throwable t) {
         if (span != null) {

diff --git a/...ugin/src/main/java/co/elastic/apm/agent/plugin/api/CaptureTransactionInstrumentation.java b/...ugin/src/main/java/co/elastic/apm/agent/plugin/api/CaptureTransactionInstrumentation.java
@@ -53,7 +53,7 @@ public class CaptureTransactionInstrumentation extends ElasticApmInstrumentation
 
     private StacktraceConfiguration config;
 
-    @Advice.OnMethodEnter(inline = true)
+    @Advice.OnMethodEnter(suppress = Throwable.class)
     public static void onMethodEnter(@SimpleMethodSignature String signature,
                                      @AnnotationValueExtractor(annotationClassName = "co.elastic.apm.api.CaptureTransaction", method = "value") String transactionName,
                                      @AnnotationValueExtractor(annotationClassName = "co.elastic.apm.api.CaptureTransaction", method = "type") String type,
@@ -71,7 +71,7 @@ public static void onMethodEnter(@SimpleMethodSignature String signature,
         }
     }
 
-    @Advice.OnMethodExit(inline = true, onThrowable = Throwable.class)
+    @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
     public static void onMethodExit(@Nullable @Advice.Local("transaction") Transaction transaction,
                                     @Advice.Thrown Throwable t) {
         if (transaction != null) {

diff --git a/...pi-plugin/src/main/java/co/elastic/apm/agent/plugin/api/ElasticApmApiInstrumentation.java b/...pi-plugin/src/main/java/co/elastic/apm/agent/plugin/api/ElasticApmApiInstrumentation.java
@@ -60,7 +60,7 @@ public StartTransactionInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodExit
+        @Advice.OnMethodExit(suppress = Throwable.class)
         private static void doStartTransaction(@Advice.Return(readOnly = false) Object transaction) {
             if (tracer != null) {
                 transaction = tracer.startTransaction();
@@ -74,7 +74,7 @@ public CurrentTransactionInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodExit
+        @Advice.OnMethodExit(suppress = Throwable.class)
         private static void doGetCurrentTransaction(@Advice.Return(readOnly = false) Object transaction) {
             if (tracer != null) {
                 transaction = tracer.currentTransaction();
@@ -88,7 +88,7 @@ public CurrentSpanInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodExit
+        @Advice.OnMethodExit(suppress = Throwable.class)
         private static void doGetCurrentSpan(@Advice.Return(readOnly = false) Object span) {
             if (tracer != null) {
                 span = tracer.activeSpan();
@@ -102,7 +102,7 @@ public CaptureExceptionInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodEnter
+        @Advice.OnMethodEnter(suppress = Throwable.class)
         private static void captureException(@Advice.Argument(0) @Nullable Throwable e) {
             if (tracer != null) {
                 tracer.captureException(e);

diff --git a/...ins/apm-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/SpanInstrumentation.java b/...ins/apm-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/SpanInstrumentation.java
@@ -57,7 +57,7 @@ public SetNameInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodEnter
+        @Advice.OnMethodEnter(suppress = Throwable.class)
         public static void setName(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span,
                                    @Advice.Argument(0) String name) {
             span.setName(name);
@@ -70,7 +70,7 @@ public SetTypeInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodEnter
+        @Advice.OnMethodEnter(suppress = Throwable.class)
         public static void setType(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span,
                                    @Advice.Argument(0) String type) {
             span.withType(type);
@@ -83,7 +83,7 @@ public DoCreateSpanInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodExit
+        @Advice.OnMethodExit(suppress = Throwable.class)
         public static void doCreateSpan(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span,
                                         @Advice.Return(readOnly = false) Object result) {
             result = span.createSpan();
@@ -96,7 +96,7 @@ public EndInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodEnter
+        @Advice.OnMethodEnter(suppress = Throwable.class)
         public static void end(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span) {
             span.end();
         }
@@ -109,7 +109,7 @@ public CaptureExceptionInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodExit
+        @Advice.OnMethodExit(suppress = Throwable.class)
         public static void doCreateSpan(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span,
                                         @Advice.Argument(0) Throwable t) {
             span.captureException(t);
@@ -122,7 +122,7 @@ public GetIdInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodExit
+        @Advice.OnMethodExit(suppress = Throwable.class)
         public static void getId(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span,
                                         @Advice.Return(readOnly = false) String id) {
             if (tracer != null) {
@@ -137,7 +137,7 @@ public GetTraceIdInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodExit
+        @Advice.OnMethodExit(suppress = Throwable.class)
         public static void getTraceId(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span,
                                         @Advice.Return(readOnly = false) String traceId) {
             if (tracer != null) {
@@ -152,7 +152,7 @@ public AddTagInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodEnter
+        @Advice.OnMethodEnter(suppress = Throwable.class)
         public static void addTag(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span,
                                   @Advice.Argument(0) String key, @Advice.Argument(1) String value) {
             span.addTag(key, value);
@@ -165,7 +165,7 @@ public ActivateInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodEnter
+        @Advice.OnMethodEnter(suppress = Throwable.class)
         public static void addTag(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span) {
             span.activate();
         }
@@ -177,7 +177,7 @@ public IsSampledInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodExit
+        @Advice.OnMethodExit(suppress = Throwable.class)
         public static void addTag(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) AbstractSpan<?> span,
                                   @Advice.Return(readOnly = false) boolean sampled) {
             sampled = span.isSampled();

diff --git a/...-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/TransactionInstrumentation.java b/...-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/TransactionInstrumentation.java
@@ -57,7 +57,7 @@ public SetUserInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodEnter
+        @Advice.OnMethodEnter(suppress = Throwable.class)
         public static void setUser(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) Transaction transaction,
                                    @Advice.Argument(0) String id, @Advice.Argument(1) String email, @Advice.Argument(2) String username) {
             transaction.setUser(id, email, username);
@@ -70,7 +70,7 @@ public EnsureParentIdInstrumentation() {
         }
 
         @VisibleForAdvice
-        @Advice.OnMethodExit
+        @Advice.OnMethodExit(suppress = Throwable.class)
         public static void ensureParentId(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) Transaction transaction,
                                           @Advice.Return(readOnly = false) String spanId) {
             if (tracer != null) {
@@ -88,7 +88,7 @@ public SetResultInstrumentation() {
             super(named("setResult"));
         }
 
-        @Advice.OnMethodEnter()
+        @Advice.OnMethodEnter(suppress = Throwable.class)
         private static void ensureParentId(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) Transaction transaction,
                                            @Advice.Argument(0) String result) {
             transaction.withResult(result);

diff --git a/.../java/co/elastic/apm/agent/es/restclient/v5_6/ElasticsearchRestClientInstrumentation.java b/.../java/co/elastic/apm/agent/es/restclient/v5_6/ElasticsearchRestClientInstrumentation.java
@@ -55,7 +55,7 @@ public class ElasticsearchRestClientInstrumentation extends ElasticApmInstrument
     @VisibleForAdvice
     public static final String DB_CONTEXT_TYPE = "elasticsearch";
 
-    @Advice.OnMethodEnter
+    @Advice.OnMethodEnter(suppress = Throwable.class)
     private static void onBeforeExecute(@Advice.Argument(0) String method,
                                         @Advice.Argument(1) String endpoint,
                                         @Advice.Argument(3) @Nullable HttpEntity entity,
@@ -87,7 +87,7 @@ private static void onBeforeExecute(@Advice.Argument(0) String method,
         }
     }
 
-    @Advice.OnMethodExit(onThrowable = Throwable.class)
+    @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
     public static void onAfterExecute(@Advice.Return @Nullable Response response,
                                       @Advice.Local("span") @Nullable Span span,
                                       @Advice.Thrown @Nullable Throwable t) {

diff --git a/.../java/co/elastic/apm/agent/es/restclient/v6_4/ElasticsearchRestClientInstrumentation.java b/.../java/co/elastic/apm/agent/es/restclient/v6_4/ElasticsearchRestClientInstrumentation.java
@@ -54,7 +54,7 @@ public class ElasticsearchRestClientInstrumentation extends ElasticApmInstrument
     @VisibleForAdvice
     public static final String DB_CONTEXT_TYPE = "elasticsearch";
 
-    @Advice.OnMethodEnter
+    @Advice.OnMethodEnter(suppress = Throwable.class)
     private static void onBeforeExecute(@Advice.Argument(0) Request request,
                                         @Advice.Local("span") Span span) {
         if (tracer == null) {
@@ -85,7 +85,7 @@ private static void onBeforeExecute(@Advice.Argument(0) Request request,
         }
     }
 
-    @Advice.OnMethodExit(onThrowable = Throwable.class)
+    @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
     public static void onAfterExecute(@Advice.Return @Nullable Response response,
                                       @Advice.Local("span") @Nullable Span span,
                                       @Advice.Thrown @Nullable Throwable t) {

diff --git a/...-plugin/src/main/java/co/elastic/apm/agent/jaxrs/JaxRsTransactionNameInstrumentation.java b/...-plugin/src/main/java/co/elastic/apm/agent/jaxrs/JaxRsTransactionNameInstrumentation.java
@@ -47,7 +47,7 @@ public class JaxRsTransactionNameInstrumentation extends ElasticApmInstrumentati
 
     private Collection<String> applicationPackages = Collections.emptyList();
 
-    @Advice.OnMethodEnter
+    @Advice.OnMethodEnter(suppress = Throwable.class)
     private static void setTransactionName(@SimpleMethodSignature String signature) {
         if (tracer != null) {
             final Transaction transaction = tracer.currentTransaction();

diff --git a/...ns/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/ConnectionInstrumentation.java b/...ns/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/ConnectionInstrumentation.java
@@ -56,7 +56,7 @@ public class ConnectionInstrumentation extends ElasticApmInstrumentation {
     static final String JDBC_INSTRUMENTATION_GROUP = "jdbc";
 
     @VisibleForAdvice
-    @Advice.OnMethodExit
+    @Advice.OnMethodExit(suppress = Throwable.class)
     public static void storeSql(@Advice.Return final PreparedStatement statement, @Advice.Argument(0) String sql) {
         statementSqlMap.put(statement, sql);
     }