From 20650376cfa6ff7b3c15f2bde87d1c83e06f4947 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 14 Feb 2024 10:34:35 +0100 Subject: [PATCH 1/7] Implemented redacted exception --- .../apm/agent/configuration/CoreConfiguration.java | 12 ++++++++++++ .../co/elastic/apm/agent/impl/ElasticApmTracer.java | 12 ++++++++++++ .../apm/agent/impl/error/RedactedException.java | 10 ++++++++++ .../co/elastic/apm/agent/tracer/GlobalTracer.java | 6 ++++++ .../java/co/elastic/apm/agent/tracer/NoopTracer.java | 6 ++++++ .../java/co/elastic/apm/agent/tracer/Tracer.java | 3 +++ 6 files changed, 49 insertions(+) create mode 100644 apm-agent-core/src/main/java/co/elastic/apm/agent/impl/error/RedactedException.java 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 index f09ced8cfa..4f8bb44e3c 100644 --- 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 @@ -517,6 +517,14 @@ public String toString(Collection value) { .tags("added[1.44.0]") .buildWithDefault(false); + + private final ConfigurationOption redactExceptions = ConfigurationOption.booleanOption() + .key("redact_exceptions") + .tags("internal") + .configurationCategory(CORE_CATEGORY) + .dynamic(true) + .buildWithDefault(true); + private final ConfigurationOption> classesExcludedFromInstrumentation = ConfigurationOption .builder(new ValueConverter>() { @@ -1163,6 +1171,10 @@ public boolean isContextPropagationOnly() { return contextPropagationOnly.get(); } + public boolean isRedactExceptions() { + return redactExceptions.get(); + } + public enum CloudProvider { AUTO, AWS, diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java index 6639e57be2..624bfc3f2f 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java @@ -26,6 +26,7 @@ import co.elastic.apm.agent.configuration.CoreConfiguration; import co.elastic.apm.agent.configuration.MetricsConfiguration; import co.elastic.apm.agent.configuration.ServerlessConfiguration; +import co.elastic.apm.agent.impl.error.RedactedException; import co.elastic.apm.agent.impl.metadata.ServiceFactory; import co.elastic.apm.agent.tracer.service.Service; import co.elastic.apm.agent.tracer.service.ServiceInfo; @@ -446,6 +447,8 @@ private ErrorCapture captureException(long epochMicros, @Nullable Throwable e, E return null; } + e = redactExceptionIfRequired(e); + while (e != null && WildcardMatcher.anyMatch(coreConfiguration.getUnnestExceptions(), e.getClass().getName()) != null) { e = e.getCause(); } @@ -995,4 +998,13 @@ public Service createService(String ephemeralId) { configurationRegistry.getConfig(ServerlessConfiguration.class).runsOnAwsLambda() ); } + + @Override + @Nullable + public Throwable redactExceptionIfRequired(@Nullable Throwable original) { + if (original != null && coreConfiguration.isRedactExceptions() && !(original instanceof RedactedException)) { + return new RedactedException(); + } + return original; + } } diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/error/RedactedException.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/error/RedactedException.java new file mode 100644 index 0000000000..86b88625f3 --- /dev/null +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/error/RedactedException.java @@ -0,0 +1,10 @@ +package co.elastic.apm.agent.impl.error; + +public class RedactedException extends Exception { + + public RedactedException() { + super("This exception is a placeholder for an exception which has occurred in the application." + + "The stacktrace corresponds to where elastic APM detected the exception."); + } + +} diff --git a/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/GlobalTracer.java b/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/GlobalTracer.java index 1ad88cd129..449622326e 100644 --- a/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/GlobalTracer.java +++ b/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/GlobalTracer.java @@ -150,4 +150,10 @@ public void reportLog(byte[] log) { public Service createService(String ephemeralId) { return tracer.createService(ephemeralId); } + + @Nullable + @Override + public Throwable redactExceptionIfRequired(@Nullable Throwable original) { + return tracer.redactExceptionIfRequired(original); + } } diff --git a/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/NoopTracer.java b/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/NoopTracer.java index 2c2727152d..5ee5d8a652 100644 --- a/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/NoopTracer.java +++ b/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/NoopTracer.java @@ -125,4 +125,10 @@ public void reportLog(byte[] log) { public Service createService(String ephemeralId) { return null; } + + @Nullable + @Override + public Throwable redactExceptionIfRequired(@Nullable Throwable original) { + return original; + } } diff --git a/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/Tracer.java b/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/Tracer.java index b7f9042142..1cd0529de8 100644 --- a/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/Tracer.java +++ b/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/Tracer.java @@ -88,4 +88,7 @@ public interface Tracer { @Nullable Service createService(String ephemeralId); + + @Nullable + Throwable redactExceptionIfRequired(@Nullable Throwable original); } From df0eba18f8d249414c83ffd4f85da0b19185cbbe Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 14 Feb 2024 11:50:57 +0100 Subject: [PATCH 2/7] Redact stored exceptions --- .../apm/agent/servlet/helper/JakartaApmAsyncListener.java | 4 ++-- .../agent/servlet/helper/JakartaAsyncContextAdviceHelper.java | 4 ++++ .../apm/agent/servlet/helper/JavaxApmAsyncListener.java | 4 ++-- .../agent/servlet/helper/JavaxAsyncContextAdviceHelper.java | 4 ++++ .../AbstractSpringExceptionHandlerInstrumentation.java | 4 +++- 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JakartaApmAsyncListener.java b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JakartaApmAsyncListener.java index 41996f1c26..fd9dfe1b41 100644 --- a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JakartaApmAsyncListener.java +++ b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JakartaApmAsyncListener.java @@ -79,7 +79,7 @@ public void onComplete(AsyncEvent event) { @Override public void onTimeout(AsyncEvent event) { - throwable = event.getThrowable(); + throwable = asyncContextAdviceHelperImpl.getTracer().redactExceptionIfRequired(event.getThrowable()); if (isJBossEap6(event)) { endTransaction(event); } @@ -98,7 +98,7 @@ public void onTimeout(AsyncEvent event) { @Override public void onError(AsyncEvent event) { - throwable = event.getThrowable(); + throwable = asyncContextAdviceHelperImpl.getTracer().redactExceptionIfRequired(event.getThrowable()); if (isJBossEap6(event)) { endTransaction(event); } diff --git a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JakartaAsyncContextAdviceHelper.java b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JakartaAsyncContextAdviceHelper.java index 28efcfa191..aad6a566f2 100644 --- a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JakartaAsyncContextAdviceHelper.java +++ b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JakartaAsyncContextAdviceHelper.java @@ -44,6 +44,10 @@ public JakartaAsyncContextAdviceHelper(Tracer tracer) { new JakartaAsyncContextAdviceHelper.JakartaApmAsyncListenerAllocator()); } + public Tracer getTracer() { + return tracer; + } + private final class JakartaApmAsyncListenerAllocator implements Allocator { @Override public JakartaApmAsyncListener createInstance() { diff --git a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JavaxApmAsyncListener.java b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JavaxApmAsyncListener.java index 32fb3bac4e..2f18ed1944 100644 --- a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JavaxApmAsyncListener.java +++ b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JavaxApmAsyncListener.java @@ -78,7 +78,7 @@ public void onComplete(AsyncEvent event) { @Override public void onTimeout(AsyncEvent event) { - throwable = event.getThrowable(); + throwable = asyncContextAdviceHelperImpl.getTracer().redactExceptionIfRequired(event.getThrowable()); if (isJBossEap6(event)) { endTransaction(event); } @@ -97,7 +97,7 @@ public void onTimeout(AsyncEvent event) { @Override public void onError(AsyncEvent event) { - throwable = event.getThrowable(); + throwable = asyncContextAdviceHelperImpl.getTracer().redactExceptionIfRequired(event.getThrowable()); if (isJBossEap6(event)) { endTransaction(event); } diff --git a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JavaxAsyncContextAdviceHelper.java b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JavaxAsyncContextAdviceHelper.java index 4809a726df..cc11be4565 100644 --- a/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JavaxAsyncContextAdviceHelper.java +++ b/apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/helper/JavaxAsyncContextAdviceHelper.java @@ -45,6 +45,10 @@ public JavaxAsyncContextAdviceHelper(Tracer tracer) { new JavaxAsyncContextAdviceHelper.ApmAsyncListenerAllocator()); } + public Tracer getTracer() { + return tracer; + } + private final class ApmAsyncListenerAllocator implements Allocator { @Override public JavaxApmAsyncListener createInstance() { diff --git a/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/main/java/co/elastic/apm/agent/springwebmvc/AbstractSpringExceptionHandlerInstrumentation.java b/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/main/java/co/elastic/apm/agent/springwebmvc/AbstractSpringExceptionHandlerInstrumentation.java index 331f8131a4..c4fd27b15c 100644 --- a/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/main/java/co/elastic/apm/agent/springwebmvc/AbstractSpringExceptionHandlerInstrumentation.java +++ b/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/main/java/co/elastic/apm/agent/springwebmvc/AbstractSpringExceptionHandlerInstrumentation.java @@ -21,6 +21,7 @@ import co.elastic.apm.agent.sdk.ElasticApmInstrumentation; import co.elastic.apm.agent.servlet.Constants; import co.elastic.apm.agent.servlet.adapter.ServletRequestAdapter; +import co.elastic.apm.agent.tracer.GlobalTracer; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -63,7 +64,8 @@ public static void captureRequestError(ServletRequestAdapte @Nullable HttpServletRequest request, @Nullable Exception e) { if (request != null && e != null) { - adapter.setAttribute(request, "co.elastic.apm.exception", e); + Throwable maybeRedacted = GlobalTracer.get().redactExceptionIfRequired(e); + adapter.setAttribute(request, "co.elastic.apm.exception", maybeRedacted); } } } From e5e584962ad29c0ec9049e7184b6dc03e4a1d2f9 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 14 Feb 2024 11:55:11 +0100 Subject: [PATCH 3/7] Added test --- .../example/stacktrace/ErrorCaptureTest.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/apm-agent-core/src/test/java/org/example/stacktrace/ErrorCaptureTest.java b/apm-agent-core/src/test/java/org/example/stacktrace/ErrorCaptureTest.java index 91e2c267c8..5137144396 100644 --- a/apm-agent-core/src/test/java/org/example/stacktrace/ErrorCaptureTest.java +++ b/apm-agent-core/src/test/java/org/example/stacktrace/ErrorCaptureTest.java @@ -24,6 +24,7 @@ import co.elastic.apm.agent.impl.ElasticApmTracer; import co.elastic.apm.agent.impl.context.Request; import co.elastic.apm.agent.impl.error.ErrorCapture; +import co.elastic.apm.agent.impl.error.RedactedException; import co.elastic.apm.agent.impl.stacktrace.StacktraceConfiguration; import co.elastic.apm.agent.impl.transaction.Transaction; import org.junit.jupiter.api.BeforeEach; @@ -107,6 +108,27 @@ void testNonConfiguredWrappingConfigured() { assertThat(errorCapture.getException()).isInstanceOf(WrapperException.class); } + @Test + void testExceptionRedaction() { + doReturn(true).when(coreConfiguration).isRedactExceptions(); + assertThat(tracer.redactExceptionIfRequired(null)).isNull(); + + Exception exception = new CustomException(); + Throwable redacted = tracer.redactExceptionIfRequired(exception); + assertThat(redacted).isInstanceOf(RedactedException.class); + + assertThat(tracer.redactExceptionIfRequired(redacted)).isSameAs(redacted); + + ErrorCapture errorCapture = tracer.captureException(exception, tracer.currentContext(), null); + assertThat(errorCapture).isNotNull(); + assertThat(errorCapture.getException()).isInstanceOf(RedactedException.class); + + ErrorCapture alreadyRedacted = tracer.captureException(redacted, tracer.currentContext(), null); + assertThat(alreadyRedacted).isNotNull(); + assertThat(alreadyRedacted.getException()).isSameAs(redacted); + } + + private static class NestedException extends Exception { public NestedException(Throwable cause) { super(cause); From 897304896077dd6089c56754f1d878c862902393 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 14 Feb 2024 12:27:24 +0100 Subject: [PATCH 4/7] Fix config option default value --- .../agent/configuration/CoreConfiguration.java | 2 +- .../agent/impl/error/RedactedException.java | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) 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 index 4f8bb44e3c..67fb9b0656 100644 --- 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 @@ -523,7 +523,7 @@ public String toString(Collection value) { .tags("internal") .configurationCategory(CORE_CATEGORY) .dynamic(true) - .buildWithDefault(true); + .buildWithDefault(false); private final ConfigurationOption> classesExcludedFromInstrumentation = ConfigurationOption .builder(new ValueConverter>() { diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/error/RedactedException.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/error/RedactedException.java index 86b88625f3..19ca4d2e31 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/error/RedactedException.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/error/RedactedException.java @@ -1,3 +1,21 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ package co.elastic.apm.agent.impl.error; public class RedactedException extends Exception { From cc76842f69cefebe770d1229a92c068214c88d89 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 14 Feb 2024 13:04:08 +0100 Subject: [PATCH 5/7] Fix and add changelog --- CHANGELOG.asciidoc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 4d5a7a312e..2da4e73f32 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -31,6 +31,10 @@ Use subheadings with the "=====" level for adding notes for unreleased changes: === Unreleased +[float] +===== Features +* Added internal `redact_exceptions` config option to workaround JVM bugs related to touching exceptions - {pull}3528[#3528] + [[release-notes-1.x]] === Java Agent version 1.x @@ -40,9 +44,6 @@ Use subheadings with the "=====" level for adding notes for unreleased changes: [float] ===== Features * Added a configuration option to use queues in names of spring-rabbit transactions - {pull}3424[#3424] - -[float] -===== Features * Add option to retry JMX metrics capture in case of exception - {pull}3511[#3511] [float] From 54b66179d40b7171182183f11730ebd6a8142c14 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Wed, 14 Feb 2024 14:20:59 +0100 Subject: [PATCH 6/7] Added workaround for not using attributes for exception propagation --- .../agent/configuration/CoreConfiguration.java | 11 ++++++++--- .../apm/agent/impl/transaction/Transaction.java | 16 ++++++++++++++++ .../apm/agent/servlet/ServletApiAdvice.java | 6 ++++++ ...ctSpringExceptionHandlerInstrumentation.java | 17 +++++++++++++++-- ...nstrumentationWithExceptionResolverTest.java | 12 ++++++++++-- .../elastic/apm/agent/tracer/Transaction.java | 5 +++++ .../tracer/configuration/CoreConfiguration.java | 2 ++ 7 files changed, 62 insertions(+), 7 deletions(-) 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 index 67fb9b0656..a240a9b0c6 100644 --- 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 @@ -518,12 +518,12 @@ public String toString(Collection value) { .buildWithDefault(false); - private final ConfigurationOption redactExceptions = ConfigurationOption.booleanOption() + private final ConfigurationOption redactExceptions = ConfigurationOption.integerOption() .key("redact_exceptions") .tags("internal") .configurationCategory(CORE_CATEGORY) .dynamic(true) - .buildWithDefault(false); + .buildWithDefault(0); private final ConfigurationOption> classesExcludedFromInstrumentation = ConfigurationOption .builder(new ValueConverter>() { @@ -1172,7 +1172,12 @@ public boolean isContextPropagationOnly() { } public boolean isRedactExceptions() { - return redactExceptions.get(); + return (redactExceptions.get() & 1) != 0; + } + + @Override + public boolean isNotUseServletAttributesForExceptionPropagation() { + return (redactExceptions.get() & 2) != 0; } public enum CloudProvider { diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Transaction.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Transaction.java index aabac69aed..423a17a6ad 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Transaction.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Transaction.java @@ -104,6 +104,8 @@ public class Transaction extends AbstractSpan implements co.elastic @Nullable private String frameworkVersion; + private Throwable pendingException; + /** * Faas *

@@ -337,6 +339,7 @@ public void resetState() { frameworkVersion = null; faas.resetState(); wasActivated.set(false); + pendingException = null; // don't clear timerBySpanTypeAndSubtype map (see field-level javadoc) } @@ -535,4 +538,17 @@ private void trackMetrics() { phaser.readerUnlock(); } } + + + @Override + public void setPendingTransactionException(Throwable exception) { + this.pendingException = exception; + } + + @Nullable + @Override + public Throwable getPendingTransactionException() { + return this.pendingException; + } + } 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 index fa41a89bb0..f203f89272 100644 --- 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,12 @@ public static void captureRequestError(ServletRequestAdapte @Nullable HttpServletRequest request, @Nullable Exception e) { if (request != null && e != null) { - Throwable maybeRedacted = GlobalTracer.get().redactExceptionIfRequired(e); - adapter.setAttribute(request, "co.elastic.apm.exception", maybeRedacted); + Tracer tracer = GlobalTracer.get(); + boolean notUseAttribs = tracer.getConfig(CoreConfiguration.class).isNotUseServletAttributesForExceptionPropagation(); + Throwable maybeRedacted = tracer.redactExceptionIfRequired(e); + + if (notUseAttribs) { + Transaction transaction = tracer.currentContext().getTransaction(); + if(transaction != null) { + transaction.setPendingTransactionException(maybeRedacted); + } + } else { + adapter.setAttribute(request, "co.elastic.apm.exception", maybeRedacted); + } } } } diff --git a/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/test/java/co/elastic/apm/agent/springwebmvc/exception/AbstractExceptionHandlerInstrumentationWithExceptionResolverTest.java b/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/test/java/co/elastic/apm/agent/springwebmvc/exception/AbstractExceptionHandlerInstrumentationWithExceptionResolverTest.java index 1f62d500d2..a9b1a90e98 100644 --- a/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/test/java/co/elastic/apm/agent/springwebmvc/exception/AbstractExceptionHandlerInstrumentationWithExceptionResolverTest.java +++ b/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/test/java/co/elastic/apm/agent/springwebmvc/exception/AbstractExceptionHandlerInstrumentationWithExceptionResolverTest.java @@ -18,15 +18,19 @@ */ package co.elastic.apm.agent.springwebmvc.exception; +import co.elastic.apm.agent.configuration.CoreConfiguration; import co.elastic.apm.agent.springwebmvc.exception.testapp.exception_resolver.ExceptionResolverController; import co.elastic.apm.agent.springwebmvc.exception.testapp.exception_resolver.ExceptionResolverRuntimeException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.web.servlet.MvcResult; import org.springframework.test.web.servlet.ResultActions; import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.doReturn; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; /** @@ -39,8 +43,12 @@ }) public abstract class AbstractExceptionHandlerInstrumentationWithExceptionResolverTest extends AbstractExceptionHandlerInstrumentationTest { - @Test - public void testCallApiWithExceptionThrown() throws Exception { + @ParameterizedTest + @ValueSource(booleans = {true,false}) + public void testCallApiWithExceptionThrown(boolean useAttributeBasedPropagation) throws Exception { + CoreConfiguration coreConfig = config.getConfig(CoreConfiguration.class); + doReturn(useAttributeBasedPropagation).when(coreConfig).isNotUseServletAttributesForExceptionPropagation(); + ResultActions resultActions = this.mockMvc.perform(get("/exception-resolver/throw-exception")); MvcResult result = resultActions.andReturn(); MockHttpServletResponse response = result.getResponse(); diff --git a/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/Transaction.java b/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/Transaction.java index 1424da3766..1f6ade713c 100644 --- a/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/Transaction.java +++ b/apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/Transaction.java @@ -56,4 +56,9 @@ public interface Transaction> extends AbstractSpan { void setFrameworkName(@Nullable String frameworkName); void setFrameworkVersion(@Nullable String frameworkVersion); + + void setPendingTransactionException(Throwable exception); + + @Nullable + Throwable getPendingTransactionException(); } 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 index 3d166ca595..094c2d6550 100644 --- 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 isNotUseServletAttributesForExceptionPropagation(); + enum EventType { /** * Request bodies will never be reported From d1432b81b0b887242aced3e1df2473dece6b873c Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 15 Feb 2024 10:15:06 +0100 Subject: [PATCH 7/7] Review fixes --- CHANGELOG.asciidoc | 2 +- .../apm/agent/configuration/CoreConfiguration.java | 10 +++++----- .../apm/agent/impl/transaction/Transaction.java | 3 ++- .../AbstractSpringExceptionHandlerInstrumentation.java | 9 ++++----- ...andlerInstrumentationWithExceptionResolverTest.java | 3 +-- .../agent/tracer/configuration/CoreConfiguration.java | 2 +- 6 files changed, 14 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 2da4e73f32..72d6d651d2 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -33,7 +33,7 @@ Use subheadings with the "=====" level for adding notes for unreleased changes: [float] ===== Features -* Added internal `redact_exceptions` config option to workaround JVM bugs related to touching exceptions - {pull}3528[#3528] +* Added internal `safe_exceptions` config option to workaround JVM bugs related to touching exceptions - {pull}3528[#3528] [[release-notes-1.x]] === Java Agent version 1.x 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 index a240a9b0c6..c185bbc314 100644 --- 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 @@ -518,8 +518,8 @@ public String toString(Collection value) { .buildWithDefault(false); - private final ConfigurationOption redactExceptions = ConfigurationOption.integerOption() - .key("redact_exceptions") + private final ConfigurationOption safeExceptions = ConfigurationOption.integerOption() + .key("safe_exceptions") .tags("internal") .configurationCategory(CORE_CATEGORY) .dynamic(true) @@ -1172,12 +1172,12 @@ public boolean isContextPropagationOnly() { } public boolean isRedactExceptions() { - return (redactExceptions.get() & 1) != 0; + return (safeExceptions.get() & 1) != 0; } @Override - public boolean isNotUseServletAttributesForExceptionPropagation() { - return (redactExceptions.get() & 2) != 0; + public boolean isUseServletAttributesForExceptionPropagation() { + return (safeExceptions.get() & 2) == 0; } public enum CloudProvider { diff --git a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Transaction.java b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Transaction.java index 423a17a6ad..aab38dddd2 100644 --- a/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Transaction.java +++ b/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Transaction.java @@ -104,6 +104,7 @@ public class Transaction extends AbstractSpan implements co.elastic @Nullable private String frameworkVersion; + @Nullable private Throwable pendingException; /** @@ -541,7 +542,7 @@ private void trackMetrics() { @Override - public void setPendingTransactionException(Throwable exception) { + public void setPendingTransactionException(@Nullable Throwable exception) { this.pendingException = exception; } diff --git a/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/main/java/co/elastic/apm/agent/springwebmvc/AbstractSpringExceptionHandlerInstrumentation.java b/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/main/java/co/elastic/apm/agent/springwebmvc/AbstractSpringExceptionHandlerInstrumentation.java index c83f1d879f..8d2faed51f 100644 --- a/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/main/java/co/elastic/apm/agent/springwebmvc/AbstractSpringExceptionHandlerInstrumentation.java +++ b/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/main/java/co/elastic/apm/agent/springwebmvc/AbstractSpringExceptionHandlerInstrumentation.java @@ -68,16 +68,15 @@ public static void captureRequestError(ServletRequestAdapte @Nullable Exception e) { if (request != null && e != null) { Tracer tracer = GlobalTracer.get(); - boolean notUseAttribs = tracer.getConfig(CoreConfiguration.class).isNotUseServletAttributesForExceptionPropagation(); + boolean useAttribs = tracer.getConfig(CoreConfiguration.class).isUseServletAttributesForExceptionPropagation(); Throwable maybeRedacted = tracer.redactExceptionIfRequired(e); - - if (notUseAttribs) { + if (useAttribs) { + adapter.setAttribute(request, "co.elastic.apm.exception", maybeRedacted); + } else { Transaction transaction = tracer.currentContext().getTransaction(); if(transaction != null) { transaction.setPendingTransactionException(maybeRedacted); } - } else { - adapter.setAttribute(request, "co.elastic.apm.exception", maybeRedacted); } } } diff --git a/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/test/java/co/elastic/apm/agent/springwebmvc/exception/AbstractExceptionHandlerInstrumentationWithExceptionResolverTest.java b/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/test/java/co/elastic/apm/agent/springwebmvc/exception/AbstractExceptionHandlerInstrumentationWithExceptionResolverTest.java index a9b1a90e98..b15205fdf4 100644 --- a/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/test/java/co/elastic/apm/agent/springwebmvc/exception/AbstractExceptionHandlerInstrumentationWithExceptionResolverTest.java +++ b/apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/test/java/co/elastic/apm/agent/springwebmvc/exception/AbstractExceptionHandlerInstrumentationWithExceptionResolverTest.java @@ -21,7 +21,6 @@ import co.elastic.apm.agent.configuration.CoreConfiguration; import co.elastic.apm.agent.springwebmvc.exception.testapp.exception_resolver.ExceptionResolverController; import co.elastic.apm.agent.springwebmvc.exception.testapp.exception_resolver.ExceptionResolverRuntimeException; -import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import org.springframework.mock.web.MockHttpServletResponse; @@ -47,7 +46,7 @@ public abstract class AbstractExceptionHandlerInstrumentationWithExceptionResolv @ValueSource(booleans = {true,false}) public void testCallApiWithExceptionThrown(boolean useAttributeBasedPropagation) throws Exception { CoreConfiguration coreConfig = config.getConfig(CoreConfiguration.class); - doReturn(useAttributeBasedPropagation).when(coreConfig).isNotUseServletAttributesForExceptionPropagation(); + doReturn(useAttributeBasedPropagation).when(coreConfig).isUseServletAttributesForExceptionPropagation(); ResultActions resultActions = this.mockMvc.perform(get("/exception-resolver/throw-exception")); MvcResult result = resultActions.andReturn(); 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 index 094c2d6550..28af7bd574 100644 --- 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,7 +50,7 @@ public interface CoreConfiguration { TimeDuration getSpanMinDuration(); - boolean isNotUseServletAttributesForExceptionPropagation(); + boolean isUseServletAttributesForExceptionPropagation(); enum EventType { /**