From dc56a6ae2c69b141e5821983033380e30c954f75 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Mon, 13 May 2024 14:24:48 +0200 Subject: [PATCH] Report suppressed exceptions as exception group (#3396) * replace hub with scopes * Add Scopes * Introduce `IScopes` interface. * Replace `IHub` with `IScopes` in core * Replace `IHub` with `IScopes` in android core * Replace `IHub` with `IScopes` in android integrations * Replace `IHub` with `IScopes` in apollo integrations * Replace `IHub` with `IScopes` in okhttp integration * Replace `IHub` with `IScopes` in graphql integration * Replace `IHub` with `IScopes` in logging integrations * Replace `IHub` with `IScopes` in more integrations * Replace `IHub` with `IScopes` in OTel integration * Replace `IHub` with `IScopes` in Spring 5 / Spring Boot 2 integrations * Replace `IHub` with `IScopes` in Spring 6 / Spring Boot 3 integrations * Replace `IHub` with `IScopes` in samples * gitscopes -> github * Replace ThreadLocal with ScopesStorage * Move client and throwable to span map to scope * Add global scope * use global scope in Scopes * Implement pushScope popScope and withScope for Scopes * Add pushIsolationScope; add fork methods to ISCope * Use separate scopes for current, isolation and global scope; rename mainScopes to rootScopes * Allow controlling which scope configureScope uses * Combine scopes * Use new API for CRONS integrations * Add lifecycle helper * Change spring integrations to use new API * Use new API in servlet integrations * Use new API for kotlin coroutines and wrapers for Supplier/Callable * Discussion TODOs * Fix breadcrumb ordering * Mark TODOS with [HSM] * Add getGlobalScope and forkedRootScopes to IScopes * Fix EventProcessor ordering on scopes * Reuse code in Scopes * No longer replace global scope * Replace hub occurrences in comments, var names etc. * Implement ScopesTest * Implement CombinedScopeViewTest * Fix combined contexts * Use combined scopes for cross platform * Changes according to reviews of previous PRs * more * even more * isEnabled checks client instead of having a property on Scopes * Use SentryOptions.empty * Remove Hub * Report suppressed exceptions as exception group * api dump * add tests for suppressed exceptions * Format code * add additinoal test * Format code * apply suggestion * add changelog * fix changelog --------- Co-authored-by: Lukas Bloder Co-authored-by: Sentry Github Bot --- CHANGELOG.md | 1 + sentry/api/sentry.api | 9 + .../io/sentry/SentryExceptionFactory.java | 40 +++- .../java/io/sentry/protocol/Mechanism.java | 57 ++++++ .../io/sentry/SentryExceptionFactoryTest.kt | 187 ++++++++++++++++++ 5 files changed, 288 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b578ed17f..1c90c3589e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Features - Add support for Spring Rest Client ([#3199](https://github.com/getsentry/sentry-java/pull/3199)) +- Report exceptions returned by Throwable.getSuppressed() to Sentry as exception groups ([#3396] https://github.com/getsentry/sentry-java/pull/3396) ## 7.6.0 diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index b201a47d0c..5a68a92811 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -4472,18 +4472,24 @@ public final class io/sentry/protocol/Mechanism : io/sentry/JsonSerializable, io public fun (Ljava/lang/Thread;)V public fun getData ()Ljava/util/Map; public fun getDescription ()Ljava/lang/String; + public fun getExceptionId ()Ljava/lang/Integer; public fun getHelpLink ()Ljava/lang/String; public fun getMeta ()Ljava/util/Map; + public fun getParentId ()Ljava/lang/Integer; public fun getSynthetic ()Ljava/lang/Boolean; public fun getType ()Ljava/lang/String; public fun getUnknown ()Ljava/util/Map; + public fun isExceptionGroup ()Ljava/lang/Boolean; public fun isHandled ()Ljava/lang/Boolean; public fun serialize (Lio/sentry/ObjectWriter;Lio/sentry/ILogger;)V public fun setData (Ljava/util/Map;)V public fun setDescription (Ljava/lang/String;)V + public fun setExceptionGroup (Ljava/lang/Boolean;)V + public fun setExceptionId (Ljava/lang/Integer;)V public fun setHandled (Ljava/lang/Boolean;)V public fun setHelpLink (Ljava/lang/String;)V public fun setMeta (Ljava/util/Map;)V + public fun setParentId (Ljava/lang/Integer;)V public fun setSynthetic (Ljava/lang/Boolean;)V public fun setType (Ljava/lang/String;)V public fun setUnknown (Ljava/util/Map;)V @@ -4498,9 +4504,12 @@ public final class io/sentry/protocol/Mechanism$Deserializer : io/sentry/JsonDes public final class io/sentry/protocol/Mechanism$JsonKeys { public static final field DATA Ljava/lang/String; public static final field DESCRIPTION Ljava/lang/String; + public static final field EXCEPTION_ID Ljava/lang/String; public static final field HANDLED Ljava/lang/String; public static final field HELP_LINK Ljava/lang/String; + public static final field IS_EXCEPTION_GROUP Ljava/lang/String; public static final field META Ljava/lang/String; + public static final field PARENT_ID Ljava/lang/String; public static final field SYNTHETIC Ljava/lang/String; public static final field TYPE Ljava/lang/String; public fun ()V diff --git a/sentry/src/main/java/io/sentry/SentryExceptionFactory.java b/sentry/src/main/java/io/sentry/SentryExceptionFactory.java index 6652ebb504..2808df11c0 100644 --- a/sentry/src/main/java/io/sentry/SentryExceptionFactory.java +++ b/sentry/src/main/java/io/sentry/SentryExceptionFactory.java @@ -12,7 +12,7 @@ import java.util.Deque; import java.util.HashSet; import java.util.List; -import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -136,12 +136,20 @@ public List getSentryExceptions(final @NotNull Throwable throwa @TestOnly @NotNull Deque extractExceptionQueue(final @NotNull Throwable throwable) { - final Deque exceptions = new ArrayDeque<>(); - final Set circularityDetector = new HashSet<>(); + return extractExceptionQueueInternal( + throwable, new AtomicInteger(-1), new HashSet<>(), new ArrayDeque<>()); + } + + Deque extractExceptionQueueInternal( + final @NotNull Throwable throwable, + final @NotNull AtomicInteger exceptionId, + final @NotNull HashSet circularityDetector, + final @NotNull Deque exceptions) { Mechanism exceptionMechanism; Thread thread; Throwable currentThrowable = throwable; + int parentId = exceptionId.get(); // Stack the exceptions to send them in the reverse order while (currentThrowable != null && circularityDetector.add(currentThrowable)) { @@ -155,12 +163,11 @@ Deque extractExceptionQueue(final @NotNull Throwable throwable) thread = exceptionMechanismThrowable.getThread(); snapshot = exceptionMechanismThrowable.isSnapshot(); } else { - exceptionMechanism = null; + exceptionMechanism = new Mechanism(); thread = Thread.currentThread(); } - final boolean includeSentryFrames = - exceptionMechanism != null && Boolean.FALSE.equals(exceptionMechanism.isHandled()); + final boolean includeSentryFrames = Boolean.FALSE.equals(exceptionMechanism.isHandled()); final List frames = sentryStackTraceFactory.getStackFrames( currentThrowable.getStackTrace(), includeSentryFrames); @@ -168,7 +175,28 @@ Deque extractExceptionQueue(final @NotNull Throwable throwable) getSentryException( currentThrowable, exceptionMechanism, thread.getId(), frames, snapshot); exceptions.addFirst(exception); + + if (exceptionMechanism.getType() == null) { + exceptionMechanism.setType("chained"); + } + + if (exceptionId.get() >= 0) { + exceptionMechanism.setParentId(parentId); + } + + final int currentExceptionId = exceptionId.incrementAndGet(); + exceptionMechanism.setExceptionId(currentExceptionId); + + Throwable[] suppressed = currentThrowable.getSuppressed(); + if (suppressed != null && suppressed.length > 0) { + exceptionMechanism.setExceptionGroup(true); + for (Throwable suppressedThrowable : suppressed) { + extractExceptionQueueInternal( + suppressedThrowable, exceptionId, circularityDetector, exceptions); + } + } currentThrowable = currentThrowable.getCause(); + parentId = currentExceptionId; } return exceptions; diff --git a/sentry/src/main/java/io/sentry/protocol/Mechanism.java b/sentry/src/main/java/io/sentry/protocol/Mechanism.java index 648aed39c2..8fc9aedf77 100644 --- a/sentry/src/main/java/io/sentry/protocol/Mechanism.java +++ b/sentry/src/main/java/io/sentry/protocol/Mechanism.java @@ -67,6 +67,18 @@ public final class Mechanism implements JsonUnknown, JsonSerializable { * for grouping or display purposes. */ private @Nullable Boolean synthetic; + /** + * Exception ID. Used. e.g. for exception groups to build a hierarchy. This is referenced as + * parent by child exceptions which for Java SDK means Throwable.getSuppressed(). + */ + private @Nullable Integer exceptionId; + /** Parent exception ID. Used e.g. for exception groups to build a hierarchy. */ + private @Nullable Integer parentId; + /** + * Whether this is a group of exceptions. For Java SDK this means there were suppressed + * exceptions. + */ + private @Nullable Boolean exceptionGroup; @SuppressWarnings("unused") private @Nullable Map unknown; @@ -140,6 +152,30 @@ public void setSynthetic(final @Nullable Boolean synthetic) { this.synthetic = synthetic; } + public @Nullable Integer getExceptionId() { + return exceptionId; + } + + public void setExceptionId(final @Nullable Integer exceptionId) { + this.exceptionId = exceptionId; + } + + public @Nullable Integer getParentId() { + return parentId; + } + + public void setParentId(final @Nullable Integer parentId) { + this.parentId = parentId; + } + + public @Nullable Boolean isExceptionGroup() { + return exceptionGroup; + } + + public void setExceptionGroup(final @Nullable Boolean exceptionGroup) { + this.exceptionGroup = exceptionGroup; + } + // JsonKeys public static final class JsonKeys { @@ -150,6 +186,9 @@ public static final class JsonKeys { public static final String META = "meta"; public static final String DATA = "data"; public static final String SYNTHETIC = "synthetic"; + public static final String EXCEPTION_ID = "exception_id"; + public static final String PARENT_ID = "parent_id"; + public static final String IS_EXCEPTION_GROUP = "is_exception_group"; } // JsonUnknown @@ -191,6 +230,15 @@ public void serialize(final @NotNull ObjectWriter writer, final @NotNull ILogger if (synthetic != null) { writer.name(JsonKeys.SYNTHETIC).value(synthetic); } + if (exceptionId != null) { + writer.name(JsonKeys.EXCEPTION_ID).value(logger, exceptionId); + } + if (parentId != null) { + writer.name(JsonKeys.PARENT_ID).value(logger, parentId); + } + if (exceptionGroup != null) { + writer.name(JsonKeys.IS_EXCEPTION_GROUP).value(exceptionGroup); + } if (unknown != null) { for (String key : unknown.keySet()) { Object value = unknown.get(key); @@ -238,6 +286,15 @@ public static final class Deserializer implements JsonDeserializer { case JsonKeys.SYNTHETIC: mechanism.synthetic = reader.nextBooleanOrNull(); break; + case JsonKeys.EXCEPTION_ID: + mechanism.exceptionId = reader.nextIntegerOrNull(); + break; + case JsonKeys.PARENT_ID: + mechanism.parentId = reader.nextIntegerOrNull(); + break; + case JsonKeys.IS_EXCEPTION_GROUP: + mechanism.exceptionGroup = reader.nextBooleanOrNull(); + break; default: if (unknown == null) { unknown = new HashMap<>(); diff --git a/sentry/src/test/java/io/sentry/SentryExceptionFactoryTest.kt b/sentry/src/test/java/io/sentry/SentryExceptionFactoryTest.kt index 888f17e0a3..8eb7f0e42d 100644 --- a/sentry/src/test/java/io/sentry/SentryExceptionFactoryTest.kt +++ b/sentry/src/test/java/io/sentry/SentryExceptionFactoryTest.kt @@ -209,6 +209,193 @@ class SentryExceptionFactoryTest { assertEquals(777, frame.lineno) } + @Test + fun `when exception with mechanism suppressed exceptions, add them and show as group`() { + val exception = Exception("message") + val suppressedException = Exception("suppressed") + exception.addSuppressed(suppressedException) + + val mechanism = Mechanism() + mechanism.type = "ANR" + val thread = Thread() + val throwable = ExceptionMechanismException(mechanism, exception, thread) + + val queue = fixture.getSut().extractExceptionQueue(throwable) + + val suppressedInQueue = queue.pop() + val mainInQueue = queue.pop() + + assertEquals("suppressed", suppressedInQueue.value) + assertEquals(1, suppressedInQueue.mechanism?.exceptionId) + assertEquals(0, suppressedInQueue.mechanism?.parentId) + + assertEquals("message", mainInQueue.value) + assertEquals(0, mainInQueue.mechanism?.exceptionId) + assertEquals(true, mainInQueue.mechanism?.isExceptionGroup) + } + + @Test + fun `nested exception that contains suppressed exceptions is marked as group`() { + val exception = Exception("inner") + val suppressedException = Exception("suppressed") + exception.addSuppressed(suppressedException) + + val outerException = Exception("outer", exception) + + val queue = fixture.getSut().extractExceptionQueue(outerException) + + val suppressedInQueue = queue.pop() + val mainInQueue = queue.pop() + val outerInQueue = queue.pop() + + assertEquals("suppressed", suppressedInQueue.value) + assertEquals(2, suppressedInQueue.mechanism?.exceptionId) + assertEquals(1, suppressedInQueue.mechanism?.parentId) + + assertEquals("inner", mainInQueue.value) + assertEquals(1, mainInQueue.mechanism?.exceptionId) + assertEquals(0, mainInQueue.mechanism?.parentId) + assertEquals(true, mainInQueue.mechanism?.isExceptionGroup) + + assertEquals("outer", outerInQueue.value) + assertEquals(0, outerInQueue.mechanism?.exceptionId) + assertNull(outerInQueue.mechanism?.parentId) + assertNull(outerInQueue.mechanism?.isExceptionGroup) + } + + @Test + fun `nested exception within Mechanism that contains suppressed exceptions is marked as group`() { + val exception = Exception("inner") + val suppressedException = Exception("suppressed") + exception.addSuppressed(suppressedException) + + val mechanism = Mechanism() + mechanism.type = "ANR" + val thread = Thread() + + val outerException = ExceptionMechanismException(mechanism, Exception("outer", exception), thread) + + val queue = fixture.getSut().extractExceptionQueue(outerException) + + val suppressedInQueue = queue.pop() + val mainInQueue = queue.pop() + val outerInQueue = queue.pop() + + assertEquals("suppressed", suppressedInQueue.value) + assertEquals(2, suppressedInQueue.mechanism?.exceptionId) + assertEquals(1, suppressedInQueue.mechanism?.parentId) + + assertEquals("inner", mainInQueue.value) + assertEquals(1, mainInQueue.mechanism?.exceptionId) + assertEquals(0, mainInQueue.mechanism?.parentId) + assertEquals(true, mainInQueue.mechanism?.isExceptionGroup) + + assertEquals("outer", outerInQueue.value) + assertEquals(0, outerInQueue.mechanism?.exceptionId) + assertNull(outerInQueue.mechanism?.parentId) + assertNull(outerInQueue.mechanism?.isExceptionGroup) + } + + @Test + fun `nested exception with nested exception that contain suppressed exceptions are marked as group`() { + val innerMostException = Exception("innermost") + val innerMostSuppressed = Exception("innermostSuppressed") + innerMostException.addSuppressed(innerMostSuppressed) + + val innerException = Exception("inner", innerMostException) + val innerSuppressed = Exception("suppressed") + innerException.addSuppressed(innerSuppressed) + + val outerException = Exception("outer", innerException) + + val queue = fixture.getSut().extractExceptionQueue(outerException) + + val innerMostSuppressedInQueue = queue.pop() + val innerMostExceptionInQueue = queue.pop() + val innerSuppressedInQueue = queue.pop() + val innerExceptionInQueue = queue.pop() + val outerInQueue = queue.pop() + + assertEquals("innermostSuppressed", innerMostSuppressedInQueue.value) + assertEquals(4, innerMostSuppressedInQueue.mechanism?.exceptionId) + assertEquals(3, innerMostSuppressedInQueue.mechanism?.parentId) + assertNull(innerMostSuppressedInQueue.mechanism?.isExceptionGroup) + + assertEquals("innermost", innerMostExceptionInQueue.value) + assertEquals(3, innerMostExceptionInQueue.mechanism?.exceptionId) + assertEquals(1, innerMostExceptionInQueue.mechanism?.parentId) + assertEquals(true, innerMostExceptionInQueue.mechanism?.isExceptionGroup) + + assertEquals("suppressed", innerSuppressedInQueue.value) + assertEquals(2, innerSuppressedInQueue.mechanism?.exceptionId) + assertEquals(1, innerSuppressedInQueue.mechanism?.parentId) + assertNull(innerSuppressedInQueue.mechanism?.isExceptionGroup) + + assertEquals("inner", innerExceptionInQueue.value) + assertEquals(1, innerExceptionInQueue.mechanism?.exceptionId) + assertEquals(0, innerExceptionInQueue.mechanism?.parentId) + assertEquals(true, innerExceptionInQueue.mechanism?.isExceptionGroup) + + assertEquals("outer", outerInQueue.value) + assertEquals(0, outerInQueue.mechanism?.exceptionId) + assertNull(outerInQueue.mechanism?.parentId) + assertNull(outerInQueue.mechanism?.isExceptionGroup) + } + + @Test + fun `nested exception with nested exception that contain suppressed exceptions with a nested exception are marked as group`() { + val innerMostException = Exception("innermost") + + val innerMostSuppressedNestedException = Exception("innermostSuppressedNested") + val innerMostSuppressed = Exception("innermostSuppressed", innerMostSuppressedNestedException) + innerMostException.addSuppressed(innerMostSuppressed) + + val innerException = Exception("inner", innerMostException) + val innerSuppressed = Exception("suppressed") + innerException.addSuppressed(innerSuppressed) + + val outerException = Exception("outer", innerException) + + val queue = fixture.getSut().extractExceptionQueue(outerException) + + val innerMostSuppressedNestedExceptionInQueue = queue.pop() + val innerMostSuppressedInQueue = queue.pop() + val innerMostExceptionInQueue = queue.pop() + val innerSuppressedInQueue = queue.pop() + val innerExceptionInQueue = queue.pop() + val outerInQueue = queue.pop() + + assertEquals("innermostSuppressedNested", innerMostSuppressedNestedExceptionInQueue.value) + assertEquals(5, innerMostSuppressedNestedExceptionInQueue.mechanism?.exceptionId) + assertEquals(4, innerMostSuppressedNestedExceptionInQueue.mechanism?.parentId) + assertNull(innerMostSuppressedNestedExceptionInQueue.mechanism?.isExceptionGroup) + + assertEquals("innermostSuppressed", innerMostSuppressedInQueue.value) + assertEquals(4, innerMostSuppressedInQueue.mechanism?.exceptionId) + assertEquals(3, innerMostSuppressedInQueue.mechanism?.parentId) + assertNull(innerMostSuppressedInQueue.mechanism?.isExceptionGroup) + + assertEquals("innermost", innerMostExceptionInQueue.value) + assertEquals(3, innerMostExceptionInQueue.mechanism?.exceptionId) + assertEquals(1, innerMostExceptionInQueue.mechanism?.parentId) + assertEquals(true, innerMostExceptionInQueue.mechanism?.isExceptionGroup) + + assertEquals("suppressed", innerSuppressedInQueue.value) + assertEquals(2, innerSuppressedInQueue.mechanism?.exceptionId) + assertEquals(1, innerSuppressedInQueue.mechanism?.parentId) + assertNull(innerSuppressedInQueue.mechanism?.isExceptionGroup) + + assertEquals("inner", innerExceptionInQueue.value) + assertEquals(1, innerExceptionInQueue.mechanism?.exceptionId) + assertEquals(0, innerExceptionInQueue.mechanism?.parentId) + assertEquals(true, innerExceptionInQueue.mechanism?.isExceptionGroup) + + assertEquals("outer", outerInQueue.value) + assertEquals(0, outerInQueue.mechanism?.exceptionId) + assertNull(outerInQueue.mechanism?.parentId) + assertNull(outerInQueue.mechanism?.isExceptionGroup) + } + internal class InnerClassThrowable constructor(cause: Throwable? = null) : Throwable(cause) private val anonymousException = object : Exception() {