Fixed some locations to not touch exceptions when safe_exception is configured (#3543)

SylvainJuge · JonasKunz · web-flow · commit 33bf365f9c2c · 2024-03-05T12:32:14.000+01:00
---------

Co-authored-by: Jonas Kunz &lt;jonas.kunz@elastic.co&gt;
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
@@ -39,6 +39,7 @@ Use subheadings with the "=====" level for adding notes for unreleased changes:
 [float]
 ===== Bug fixes
 * Added missing support for TracerBuilder in OpenTelemetry bridge - {pull}3535[#3535]
+* Fixed some locations to not touch exceptions when `safe_exception` is configured - {pull}3543[#3543]
 
 [float]
 ===== Potentially breaking changes
diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/CoreConfiguration.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/CoreConfiguration.java
@@ -1175,6 +1175,11 @@ public boolean isRedactExceptions() {
         return (safeExceptions.get() & 1) != 0;
     }
 
+    @Override
+    public boolean isAvoidTouchingExceptions() {
+        return isRedactExceptions() || !captureExceptionDetails();
+    }
+
     @Override
     public boolean isUseServletAttributesForExceptionPropagation() {
         return (safeExceptions.get() & 2) == 0;
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
@@ -603,7 +603,7 @@ public static String onMethodEnter() {
 
             @Advice.AssignReturned.ToReturned
             @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)
-            public static String onMethodExit(@Advice.Thrown Throwable throwable) {
+            public static String onMethodExit() {
                 throw new RuntimeException("This exception should be suppressed");
             }
         }
diff --git a/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient3-plugin/src/main/java/co/elastic/apm/agent/httpclient/v3/HttpClient3Instrumentation.java b/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient3-plugin/src/main/java/co/elastic/apm/agent/httpclient/v3/HttpClient3Instrumentation.java
@@ -28,6 +28,7 @@
 import co.elastic.apm.agent.tracer.Outcome;
 import co.elastic.apm.agent.tracer.Span;
 import co.elastic.apm.agent.tracer.Tracer;
+import co.elastic.apm.agent.tracer.configuration.CoreConfiguration;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.method.MethodDescription;
 import net.bytebuddy.description.type.TypeDescription;
@@ -146,8 +147,10 @@ public static void onExit(@Advice.Thrown @Nullable Throwable thrown,
                 span.getContext().getHttp().withStatusCode(statusLine.getStatusCode());
             }
 
-            if (thrown instanceof CircularRedirectException) {
-                span.withOutcome(Outcome.FAILURE);
+            if(thrown != null && !tracer.getConfig(CoreConfiguration.class).isAvoidTouchingExceptions()) {
+                if (thrown instanceof CircularRedirectException) {
+                    span.withOutcome(Outcome.FAILURE);
+                }
             }
 
             span.captureException(thrown)
diff --git a/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient4-plugin/src/main/java/co/elastic/apm/agent/httpclient/v4/LegacyApacheHttpClientInstrumentation.java b/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient4-plugin/src/main/java/co/elastic/apm/agent/httpclient/v4/LegacyApacheHttpClientInstrumentation.java
@@ -23,6 +23,7 @@
 import co.elastic.apm.agent.tracer.ElasticContext;
 import co.elastic.apm.agent.tracer.Outcome;
 import co.elastic.apm.agent.tracer.Span;
+import co.elastic.apm.agent.tracer.configuration.CoreConfiguration;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.NamedElement;
 import net.bytebuddy.description.method.MethodDescription;
@@ -123,10 +124,13 @@ public static void onAfterExecute(@Advice.Return @Nullable HttpResponse response
                 }
                 span.captureException(t);
             } finally {
-                // in case of circular redirect, we get an exception but status code won't be available without response
-                // thus we have to deal with span outcome directly
-                if (t instanceof CircularRedirectException) {
-                    span.withOutcome(Outcome.FAILURE);
+
+                if(t != null && !tracer.getConfig(CoreConfiguration.class).isAvoidTouchingExceptions()) {
+                    // in case of circular redirect, we get an exception but status code won't be available without response
+                    // thus we have to deal with span outcome directly
+                    if (t instanceof CircularRedirectException) {
+                        span.withOutcome(Outcome.FAILURE);
+                    }
                 }
 
                 span.deactivate().end();
diff --git a/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient4-plugin/src/main/java/co/elastic/apm/agent/httpclient/v4/helper/ApacheHttpClient4ApiAdapter.java b/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient4-plugin/src/main/java/co/elastic/apm/agent/httpclient/v4/helper/ApacheHttpClient4ApiAdapter.java
@@ -20,6 +20,9 @@
 
 
 import co.elastic.apm.agent.httpclient.common.ApacheHttpClientApiAdapter;
+import co.elastic.apm.agent.tracer.GlobalTracer;
+import co.elastic.apm.agent.tracer.Tracer;
+import co.elastic.apm.agent.tracer.configuration.CoreConfiguration;
 import org.apache.http.HttpHost;
 import org.apache.http.HttpRequest;
 import org.apache.http.StatusLine;
@@ -32,6 +35,8 @@
 public class ApacheHttpClient4ApiAdapter implements ApacheHttpClientApiAdapter<HttpRequest, HttpRequestWrapper, HttpHost, CloseableHttpResponse> {
     private static final ApacheHttpClient4ApiAdapter INSTANCE = new ApacheHttpClient4ApiAdapter();
 
+    private final Tracer tracer = GlobalTracer.get();
+
     private ApacheHttpClient4ApiAdapter() {
     }
 
@@ -65,6 +70,9 @@ public int getResponseCode(CloseableHttpResponse closeableHttpResponse) {
 
     @Override
     public boolean isCircularRedirectException(Throwable t) {
+        if (t == null || tracer.getConfig(CoreConfiguration.class).isAvoidTouchingExceptions()) {
+            return false;
+        }
         return t instanceof CircularRedirectException;
     }
 
diff --git a/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient5-plugin/src/main/java/co/elastic/apm/agent/httpclient/v5/helper/ApacheHttpClient5ApiAdapter.java b/apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient5-plugin/src/main/java/co/elastic/apm/agent/httpclient/v5/helper/ApacheHttpClient5ApiAdapter.java
@@ -21,6 +21,9 @@
 import co.elastic.apm.agent.httpclient.common.ApacheHttpClientApiAdapter;
 import co.elastic.apm.agent.sdk.logging.Logger;
 import co.elastic.apm.agent.sdk.logging.LoggerFactory;
+import co.elastic.apm.agent.tracer.GlobalTracer;
+import co.elastic.apm.agent.tracer.Tracer;
+import co.elastic.apm.agent.tracer.configuration.CoreConfiguration;
 import org.apache.hc.client5.http.CircularRedirectException;
 import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
 import org.apache.hc.client5.http.routing.RoutingSupport;
@@ -38,6 +41,8 @@ public class ApacheHttpClient5ApiAdapter implements ApacheHttpClientApiAdapter<H
 
     private static final Logger logger = LoggerFactory.getLogger(ApacheHttpClient5ApiAdapter.class);
 
+    private final Tracer tracer = GlobalTracer.get();
+
     private ApacheHttpClient5ApiAdapter() {
     }
 
@@ -78,6 +83,9 @@ public int getResponseCode(CloseableHttpResponse closeableHttpResponse) {
 
     @Override
     public boolean isCircularRedirectException(Throwable t) {
+        if (t == null || tracer.getConfig(CoreConfiguration.class).isAvoidTouchingExceptions()) {
+            return false;
+        }
         return t instanceof CircularRedirectException;
     }
 
diff --git a/apm-agent-plugins/apm-cassandra/apm-cassandra3-plugin/src/main/java/co/elastic/apm/agent/cassandra3/Cassandra3Instrumentation.java b/apm-agent-plugins/apm-cassandra/apm-cassandra3-plugin/src/main/java/co/elastic/apm/agent/cassandra3/Cassandra3Instrumentation.java
@@ -106,7 +106,7 @@ public static void onEnter() {
             }
 
             @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)
-            public static void onExit(@Advice.Thrown @Nullable Throwable thrown, @Advice.Return @Nullable Object returnValue) {
+            public static void onExit() {
                 CassandraHelper.inSyncExecute(false);
             }
         }
diff --git a/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-5_6/src/main/java/co/elastic/apm/agent/esrestclient/v5_6/ElasticsearchClientAsyncInstrumentation.java b/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-5_6/src/main/java/co/elastic/apm/agent/esrestclient/v5_6/ElasticsearchClientAsyncInstrumentation.java
@@ -86,8 +86,7 @@ public static Object[] onBeforeExecute(@Advice.Argument(0) String method,
         }
 
         @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)
-        public static void onAfterExecute(@Advice.Thrown @Nullable Throwable t,
-                                          @Advice.Enter @Nullable Object[] entryArgs) {
+        public static void onAfterExecute(@Advice.Enter @Nullable Object[] entryArgs) {
             if (entryArgs != null) {
                 final Span<?> span = (Span<?>) entryArgs[0];
                 if (span != null) {
diff --git a/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-6_4/src/main/java/co/elastic/apm/agent/esrestclient/v6_4/ElasticsearchClientAsyncInstrumentation.java b/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-6_4/src/main/java/co/elastic/apm/agent/esrestclient/v6_4/ElasticsearchClientAsyncInstrumentation.java
@@ -77,8 +77,7 @@ public static Object[] onBeforeExecute(@Advice.Argument(0) Request request,
         }
 
         @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)
-        public static void onAfterExecute(@Advice.Thrown @Nullable Throwable t,
-                                          @Advice.Enter @Nullable Object[] entryArgs) {
+        public static void onAfterExecute(@Advice.Enter @Nullable Object[] entryArgs) {
             if (entryArgs != null) {
                 final Span<?> span = (Span<?>) entryArgs[0];
                 if (span != null) {
diff --git a/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-common/src/main/java/co/elastic/apm/agent/esrestclient/ElasticsearchRestClientInstrumentationHelper.java b/apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-common/src/main/java/co/elastic/apm/agent/esrestclient/ElasticsearchRestClientInstrumentationHelper.java
@@ -30,6 +30,7 @@
 import co.elastic.apm.agent.tracer.Outcome;
 import co.elastic.apm.agent.tracer.Span;
 import co.elastic.apm.agent.tracer.Tracer;
+import co.elastic.apm.agent.tracer.configuration.CoreConfiguration;
 import co.elastic.apm.agent.tracer.pooling.Allocator;
 import co.elastic.apm.agent.tracer.pooling.ObjectPool;
 import org.apache.http.HttpEntity;
@@ -154,16 +155,18 @@ public void finishClientSpan(@Nullable Response response, Span<?> span, @Nullabl
                 cluster = response.getHeader("x-found-handling-cluster");
 
             } else if (t != null) {
-                if (t instanceof ResponseException) {
-                    ResponseException esre = (ResponseException) t;
-                    HttpHost host = esre.getResponse().getHost();
-                    address = host.getHostName();
-                    port = host.getPort();
-                    url = host.toURI();
-                    statusCode = esre.getResponse().getStatusLine().getStatusCode();
-                } else if (t instanceof CancellationException) {
-                    // We can't tell whether a cancelled search is related to a failure or not
-                    span.withOutcome(Outcome.UNKNOWN);
+                if (!tracer.getConfig(CoreConfiguration.class).isAvoidTouchingExceptions()) {
+                    if (t instanceof ResponseException) {
+                        ResponseException esre = (ResponseException) t;
+                        HttpHost host = esre.getResponse().getHost();
+                        address = host.getHostName();
+                        port = host.getPort();
+                        url = host.toURI();
+                        statusCode = esre.getResponse().getStatusLine().getStatusCode();
+                    } else if (t instanceof CancellationException) {
+                        // We can't tell whether a cancelled search is related to a failure or not
+                        span.withOutcome(Outcome.UNKNOWN);
+                    }
                 }
                 span.captureException(t);
             }
diff --git a/apm-agent-plugins/apm-grpc/apm-grpc-plugin/src/main/java/co/elastic/apm/agent/grpc/GrpcHelper.java b/apm-agent-plugins/apm-grpc/apm-grpc-plugin/src/main/java/co/elastic/apm/agent/grpc/GrpcHelper.java
@@ -27,6 +27,7 @@
 import co.elastic.apm.agent.tracer.Span;
 import co.elastic.apm.agent.tracer.Tracer;
 import co.elastic.apm.agent.tracer.Transaction;
+import co.elastic.apm.agent.tracer.configuration.CoreConfiguration;
 import co.elastic.apm.agent.tracer.dispatch.AbstractHeaderGetter;
 import co.elastic.apm.agent.tracer.dispatch.TextHeaderGetter;
 import co.elastic.apm.agent.tracer.dispatch.TextHeaderSetter;
@@ -257,7 +258,11 @@ public void exitServerListenerMethod(@Nullable Throwable thrown,
         if (null != thrown) {
             // when there is a runtime exception thrown in one of the listener methods the calling code will catch it
             // and make this the last listener method called
-            terminateStatus = Status.fromThrowable(thrown);
+            if (tracer.getConfig(CoreConfiguration.class).isAvoidTouchingExceptions()) {
+                terminateStatus = Status.UNKNOWN;
+            } else {
+                terminateStatus = Status.fromThrowable(thrown);
+            }
             setTerminateStatus = true;
 
         } else if (transaction.getOutcome() == Outcome.UNKNOWN) {
diff --git a/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/RunnableCallableForkJoinTaskInstrumentation.java b/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/RunnableCallableForkJoinTaskInstrumentation.java
@@ -79,8 +79,7 @@ public static Object onEnter(@Advice.This Object thiz) {
         }
 
         @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)
-        public static void onExit(@Advice.Thrown Throwable thrown,
-                                  @Nullable @Advice.Enter Object context) {
+        public static void onExit(@Nullable @Advice.Enter Object context) {
             if (context != null) {
                 ((ElasticContext<?>) context).deactivate();
             }
diff --git a/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/main/java/co/elastic/apm/agent/kafka/KafkaProducerHeadersInstrumentation.java b/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/main/java/co/elastic/apm/agent/kafka/KafkaProducerHeadersInstrumentation.java
@@ -23,6 +23,7 @@
 import co.elastic.apm.agent.sdk.logging.Logger;
 import co.elastic.apm.agent.sdk.logging.LoggerFactory;
 import co.elastic.apm.agent.tracer.Span;
+import co.elastic.apm.agent.tracer.configuration.CoreConfiguration;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.asm.Advice.AssignReturned.ToArguments.ToArgument;
 import net.bytebuddy.description.method.MethodDescription;
@@ -112,7 +113,9 @@ public static Object[] afterSend(@Advice.Enter @Nullable Object[] enter,
                 return null;
             }
             Object[] overrideThrowable = null;
-            if (throwable != null && throwable.getMessage().contains("Magic v1 does not support record headers")) {
+            if (throwable != null
+                && !tracer.getConfig(CoreConfiguration.class).isAvoidTouchingExceptions()
+                && throwable.getMessage().contains("Magic v1 does not support record headers")) {
                 // Probably our fault - ignore span and retry. May happen when using a new client with an old (< 0.11.0)
                 // broker. In such cases we DO check the version, but the first version check may be not yet up to date.
                 logger.info("Adding header to Kafka record is not allowed with the used broker, attempting to resend record");
diff --git a/apm-agent-plugins/apm-process-plugin/src/main/java/co/elastic/apm/agent/process/ProcessExitInstrumentation.java b/apm-agent-plugins/apm-process-plugin/src/main/java/co/elastic/apm/agent/process/ProcessExitInstrumentation.java
@@ -18,6 +18,7 @@
  */
 package co.elastic.apm.agent.process;
 
+import co.elastic.apm.agent.tracer.configuration.CoreConfiguration;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.method.MethodDescription;
 import net.bytebuddy.description.type.TypeDescription;
@@ -145,7 +146,8 @@ public static void onExit(@Advice.This Process process, @Advice.Thrown Throwable
                     return;
                 }
 
-                if (thrown instanceof IllegalThreadStateException) {
+                if (!tracer.getConfig(CoreConfiguration.class).isAvoidTouchingExceptions()
+                    && thrown instanceof IllegalThreadStateException) {
                     // this call to exitValue was invoked before the process had terminated
                     return;
                 }
diff --git a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/ServletApiAdvice.java b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/ServletApiAdvice.java
@@ -239,6 +239,10 @@ public static <HttpServletRequest, HttpServletResponse, ServletContext, ServletC
                     for (int i = 0; i < size; i++) {
                         String attributeName = requestExceptionAttributes.get(i);
                         Object throwable = adapter.getAttribute(httpServletRequest, attributeName);
+                        // We don't check tracer.getConfig(CoreConfiguration.class).isAvoidTouchingExceptions() here,
+                        // because in that case the corrupt pointer is already stored in the request attributes
+                        // In that case we already lost, because the GC will complain about the broken pointer
+                        // when the request attributes entry is GCed
                         if (throwable instanceof Throwable) {
                             t2 = (Throwable) throwable;
 
diff --git a/apm-agent-plugins/apm-vertx/apm-vertx3-plugin/src/main/java/co/elastic/apm/agent/vertx/v3/TaskQueueInstrumentation.java b/apm-agent-plugins/apm-vertx/apm-vertx3-plugin/src/main/java/co/elastic/apm/agent/vertx/v3/TaskQueueInstrumentation.java
@@ -58,7 +58,7 @@ public static void onEnter() {
         }
 
         @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)
-        public static void onExit(@Advice.Thrown @Nullable Throwable thrown) {
+        public static void onExit() {
             JavaConcurrent.allowContextPropagationOnCurrentThread();
         }
 
diff --git a/apm-agent-plugins/apm-vertx/apm-vertx4-plugin/src/main/java/co/elastic/apm/agent/vertx/v4/TaskQueueInstrumentation.java b/apm-agent-plugins/apm-vertx/apm-vertx4-plugin/src/main/java/co/elastic/apm/agent/vertx/v4/TaskQueueInstrumentation.java
@@ -60,7 +60,7 @@ public static void onEnter() {
         }
 
         @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)
-        public static void onExit(@Advice.Thrown @Nullable Throwable thrown) {
+        public static void onExit() {
             JavaConcurrent.allowContextPropagationOnCurrentThread();
         }
 
diff --git a/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/configuration/CoreConfiguration.java b/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/configuration/CoreConfiguration.java
@@ -50,6 +50,8 @@ public interface CoreConfiguration {
 
     TimeDuration getSpanMinDuration();
 
+    boolean isAvoidTouchingExceptions();
+
     boolean isUseServletAttributesForExceptionPropagation();
 
     enum EventType {

Original file line number	Diff line number	Diff line change
`@@ -603,7 +603,7 @@ public static String onMethodEnter() {`
`603`	`603`
`604`	`604`	`@Advice.AssignReturned.ToReturned`
`605`	`605`	`@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)`
`606`		`- public static String onMethodExit(@Advice.Thrown Throwable throwable) {`
	`606`	`+ public static String onMethodExit() {`
`607`	`607`	`throw new RuntimeException("This exception should be suppressed");`
`608`	`608`	`}`
`609`	`609`	`}`
Original file line number	Diff line number	Diff line change
`@@ -106,7 +106,7 @@ public static void onEnter() {`
`106`	`106`	`}`
`107`	`107`
`108`	`108`	`@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)`
`109`		`- public static void onExit(@Advice.Thrown @Nullable Throwable thrown, @Advice.Return @Nullable Object returnValue) {`
	`109`	`+ public static void onExit() {`
`110`	`110`	`CassandraHelper.inSyncExecute(false);`
`111`	`111`	`}`
`112`	`112`	`}`
Original file line number	Diff line number	Diff line change
`@@ -86,8 +86,7 @@ public static Object[] onBeforeExecute(@Advice.Argument(0) String method,`
`86`	`86`	`}`
`87`	`87`
`88`	`88`	`@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)`
`89`		`- public static void onAfterExecute(@Advice.Thrown @Nullable Throwable t,`
`90`		`- @Advice.Enter @Nullable Object[] entryArgs) {`
	`89`	`+ public static void onAfterExecute(@Advice.Enter @Nullable Object[] entryArgs) {`
`91`	`90`	`if (entryArgs != null) {`
`92`	`91`	`final Span<?> span = (Span<?>) entryArgs[0];`
`93`	`92`	`if (span != null) {`
Original file line number	Diff line number	Diff line change
`@@ -77,8 +77,7 @@ public static Object[] onBeforeExecute(@Advice.Argument(0) Request request,`
`77`	`77`	`}`
`78`	`78`
`79`	`79`	`@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class, inline = false)`
`80`		`- public static void onAfterExecute(@Advice.Thrown @Nullable Throwable t,`
`81`		`- @Advice.Enter @Nullable Object[] entryArgs) {`
	`80`	`+ public static void onAfterExecute(@Advice.Enter @Nullable Object[] entryArgs) {`
`82`	`81`	`if (entryArgs != null) {`
`83`	`82`	`final Span<?> span = (Span<?>) entryArgs[0];`
`84`	`83`	`if (span != null) {`