diff --git a/CHANGELOG.md b/CHANGELOG.md index 754646ddc5..1e5bee9034 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Report process init time as a span for app start performance ([#3159](https://github.com/getsentry/sentry-java/pull/3159)) - (perf-v2): Calculate frame delay on a span level ([#3197](https://github.com/getsentry/sentry-java/pull/3197)) - Resolve spring properties in @SentryCheckIn annotation ([#3194](https://github.com/getsentry/sentry-java/pull/3194)) +- Improve exception grouping for Android RuntimeExceptions ([#3184](https://github.com/getsentry/sentry-java/pull/3184)) ### Fixes diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java index 8dcfe196c2..97d5a042a5 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java @@ -3,6 +3,7 @@ import android.content.Context; import android.content.pm.PackageInfo; import android.content.pm.PackageManager; +import androidx.annotation.NonNull; import io.sentry.DateUtils; import io.sentry.EventProcessor; import io.sentry.Hint; @@ -15,11 +16,16 @@ import io.sentry.android.core.performance.TimeSpan; import io.sentry.protocol.App; import io.sentry.protocol.OperatingSystem; +import io.sentry.protocol.SentryException; +import io.sentry.protocol.SentryStackFrame; +import io.sentry.protocol.SentryStackTrace; import io.sentry.protocol.SentryThread; import io.sentry.protocol.SentryTransaction; import io.sentry.protocol.User; import io.sentry.util.HintUtils; import io.sentry.util.Objects; +import java.util.Collections; +import java.util.List; import java.util.Locale; import java.util.Map; import java.util.concurrent.ExecutorService; @@ -68,9 +74,51 @@ public DefaultAndroidEventProcessor( setCommons(event, true, applyScopeData); + fixExceptionOrder(event); + return event; } + /** + * The last exception is usually used for picking the issue title, but the convention is to send + * inner exceptions first, e.g. [inner, outer] This doesn't work very well on Android, as some + * hooks like Application.onCreate is wrapped by Android framework with a RuntimeException. Thus, + * if the last exception is a RuntimeInit$MethodAndArgsCaller, reverse the order to get a better + * issue title. This is a quick fix, for more details see: #64074 #59679 #64088 + * + * @param event the event to process + */ + private static void fixExceptionOrder(final @NonNull SentryEvent event) { + boolean reverseExceptions = false; + + final @Nullable List exceptions = event.getExceptions(); + if (exceptions != null && exceptions.size() > 1) { + final @NotNull SentryException lastException = exceptions.get(exceptions.size() - 1); + if ("java.lang".equals(lastException.getModule())) { + final @Nullable SentryStackTrace stacktrace = lastException.getStacktrace(); + if (stacktrace != null) { + final @Nullable List frames = stacktrace.getFrames(); + if (frames != null) { + for (SentryStackFrame frame : frames) { + if ("com.android.internal.os.RuntimeInit$MethodAndArgsCaller" + .equals(frame.getModule())) { + reverseExceptions = true; + break; + } + } + } + } + } + } + + if (reverseExceptions) { + Collections.reverse(exceptions); + } + } + private void setCommons( final @NotNull SentryBaseEvent event, final boolean errorEvent, diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/DefaultAndroidEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/DefaultAndroidEventProcessorTest.kt index 70b20e2ab2..80954f67a5 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/DefaultAndroidEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/DefaultAndroidEventProcessorTest.kt @@ -16,6 +16,9 @@ import io.sentry.TypeCheckHint.SENTRY_DART_SDK_NAME import io.sentry.android.core.internal.util.CpuInfoUtils import io.sentry.protocol.OperatingSystem import io.sentry.protocol.SdkVersion +import io.sentry.protocol.SentryException +import io.sentry.protocol.SentryStackFrame +import io.sentry.protocol.SentryStackTrace import io.sentry.protocol.SentryThread import io.sentry.protocol.SentryTransaction import io.sentry.protocol.User @@ -573,4 +576,87 @@ class DefaultAndroidEventProcessorTest { assertNull(thread.isMain) } } + + @Test + fun `the exception list is reversed in case there's an RuntimeException`() { + val sut = fixture.getSut(context) + val event = SentryEvent().apply { + exceptions = listOf( + SentryException().apply { + type = "IllegalStateException" + module = "com.example" + stacktrace = SentryStackTrace( + listOf( + SentryStackFrame().apply { + function = "onCreate" + module = "com.example.Application" + filename = "Application.java" + } + ) + ) + }, + SentryException().apply { + type = "RuntimeException" + value = "Unable to create application com.example.Application: java.lang.IllegalStateException" + module = "java.lang" + stacktrace = SentryStackTrace( + listOf( + SentryStackFrame().apply { + function = "run" + module = "com.android.internal.os.RuntimeInit\$MethodAndArgsCaller" + filename = "RuntimeInit.java" + } + ) + ) + } + ) + } + val processedEvent = sut.process(event, Hint()) + assertNotNull(processedEvent) { + assertEquals(2, it.exceptions!!.size) + assertEquals("RuntimeException", it.exceptions!![0].type) + assertEquals("IllegalStateException", it.exceptions!![1].type) + } + } + + @Test + fun `the exception list is kept as-is in case there's no RuntimeException`() { + val sut = fixture.getSut(context) + val event = SentryEvent().apply { + exceptions = listOf( + SentryException().apply { + type = "IllegalStateException" + module = "com.example" + stacktrace = SentryStackTrace( + listOf( + SentryStackFrame().apply { + function = "onCreate" + module = "com.example.Application" + filename = "Application.java" + } + ) + ) + }, + SentryException().apply { + type = "IllegalArgumentException" + module = "com.example" + stacktrace = SentryStackTrace( + listOf( + SentryStackFrame().apply { + function = "onCreate" + module = "com.example.Application" + filename = "Application.java" + } + ) + ) + } + ) + } + val processedEvent = sut.process(event, Hint()) + assertNotNull(processedEvent) { + assertEquals(2, it.exceptions!!.size) + assertEquals("IllegalStateException", it.exceptions!![0].type) + assertEquals("IllegalArgumentException", it.exceptions!![1].type) + } + } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index da727346a8..b89af252d9 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -3812,8 +3812,11 @@ 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 getIsExceptionGroup ()Ljava/lang/Boolean; 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; @@ -3821,9 +3824,12 @@ public final class io/sentry/protocol/Mechanism : io/sentry/JsonSerializable, io 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 setExceptionId (Ljava/lang/Integer;)V public fun setHandled (Ljava/lang/Boolean;)V public fun setHelpLink (Ljava/lang/String;)V + public fun setIsExceptionGroup (Ljava/lang/Boolean;)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 @@ -3838,9 +3844,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..d22ee0ba23 100644 --- a/sentry/src/main/java/io/sentry/SentryExceptionFactory.java +++ b/sentry/src/main/java/io/sentry/SentryExceptionFactory.java @@ -143,6 +143,11 @@ Deque extractExceptionQueue(final @NotNull Throwable throwable) Throwable currentThrowable = throwable; + boolean handled = false; + String type = null; + + int currentExceptionId = 0; + // Stack the exceptions to send them in the reverse order while (currentThrowable != null && circularityDetector.add(currentThrowable)) { boolean snapshot = false; @@ -151,14 +156,26 @@ Deque extractExceptionQueue(final @NotNull Throwable throwable) ExceptionMechanismException exceptionMechanismThrowable = (ExceptionMechanismException) currentThrowable; exceptionMechanism = exceptionMechanismThrowable.getExceptionMechanism(); + handled = Boolean.TRUE.equals(exceptionMechanism.isHandled()); + type = exceptionMechanism.getType(); + currentThrowable = exceptionMechanismThrowable.getThrowable(); thread = exceptionMechanismThrowable.getThread(); snapshot = exceptionMechanismThrowable.isSnapshot(); } else { - exceptionMechanism = null; + exceptionMechanism = new Mechanism(); + exceptionMechanism.setHandled(handled); + exceptionMechanism.setType(type); + thread = Thread.currentThread(); } + exceptionMechanism.setExceptionId(currentExceptionId); + if (currentExceptionId > 0) { + exceptionMechanism.setParentId(currentExceptionId - 1); + } + currentExceptionId++; + final boolean includeSentryFrames = exceptionMechanism != null && Boolean.FALSE.equals(exceptionMechanism.isHandled()); final List frames = diff --git a/sentry/src/main/java/io/sentry/protocol/Mechanism.java b/sentry/src/main/java/io/sentry/protocol/Mechanism.java index 648aed39c2..33cd9bc22d 100644 --- a/sentry/src/main/java/io/sentry/protocol/Mechanism.java +++ b/sentry/src/main/java/io/sentry/protocol/Mechanism.java @@ -68,6 +68,12 @@ public final class Mechanism implements JsonUnknown, JsonSerializable { */ private @Nullable Boolean synthetic; + private @Nullable Boolean isExceptionGroup; + + private @Nullable Integer parentId; + + private @Nullable Integer exceptionId; + @SuppressWarnings("unused") private @Nullable Map unknown; @@ -140,6 +146,33 @@ public void setSynthetic(final @Nullable Boolean synthetic) { this.synthetic = synthetic; } + @Nullable + public Boolean getIsExceptionGroup() { + return isExceptionGroup; + } + + public void setIsExceptionGroup(@Nullable final Boolean exceptionGroup) { + isExceptionGroup = exceptionGroup; + } + + @Nullable + public Integer getParentId() { + return parentId; + } + + public void setParentId(@Nullable final Integer parentId) { + this.parentId = parentId; + } + + @Nullable + public Integer getExceptionId() { + return exceptionId; + } + + public void setExceptionId(@Nullable Integer exceptionId) { + this.exceptionId = exceptionId; + } + // JsonKeys public static final class JsonKeys { @@ -150,6 +183,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 IS_EXCEPTION_GROUP = "is_exception_group"; + public static final String EXCEPTION_ID = "exception_id"; + public static final String PARENT_ID = "parent_id"; } // JsonUnknown @@ -191,6 +227,15 @@ public void serialize(final @NotNull ObjectWriter writer, final @NotNull ILogger if (synthetic != null) { writer.name(JsonKeys.SYNTHETIC).value(synthetic); } + if (isExceptionGroup != null) { + writer.name(JsonKeys.IS_EXCEPTION_GROUP).value(isExceptionGroup); + } + if (exceptionId != null) { + writer.name(JsonKeys.EXCEPTION_ID).value(exceptionId); + } + if (parentId != null) { + writer.name(JsonKeys.PARENT_ID).value(parentId); + } if (unknown != null) { for (String key : unknown.keySet()) { Object value = unknown.get(key); @@ -238,6 +283,15 @@ public static final class Deserializer implements JsonDeserializer { case JsonKeys.SYNTHETIC: mechanism.synthetic = reader.nextBooleanOrNull(); break; + case JsonKeys.IS_EXCEPTION_GROUP: + mechanism.isExceptionGroup = reader.nextBooleanOrNull(); + break; + case JsonKeys.EXCEPTION_ID: + mechanism.exceptionId = reader.nextIntegerOrNull(); + break; + case JsonKeys.PARENT_ID: + mechanism.parentId = reader.nextIntegerOrNull(); + 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..1f8f56ac77 100644 --- a/sentry/src/test/java/io/sentry/SentryExceptionFactoryTest.kt +++ b/sentry/src/test/java/io/sentry/SentryExceptionFactoryTest.kt @@ -177,6 +177,20 @@ class SentryExceptionFactoryTest { assertTrue(exceptions.isEmpty()) } + @Test + fun `the relationship between exceptions is set`() { + val exception = IllegalStateException("outer", IllegalStateException("inner")) + val exceptions = fixture.getSut().getSentryExceptions(exception) + + assertFalse(exceptions.isEmpty()) + assertEquals("inner", exceptions[0].value) + assertEquals(1, exceptions[0].mechanism!!.exceptionId) + assertEquals(0, exceptions[0].mechanism!!.parentId) + + assertEquals("outer", exceptions[1].value) + assertEquals(0, exceptions[1].mechanism!!.exceptionId) + } + @Test fun `returns proper exception backfilled from SentryThread`() { val thread = SentryThread().apply { diff --git a/sentry/src/test/java/io/sentry/protocol/MechanismSerializationTest.kt b/sentry/src/test/java/io/sentry/protocol/MechanismSerializationTest.kt index 89c0f24b9b..7ef80af6a9 100644 --- a/sentry/src/test/java/io/sentry/protocol/MechanismSerializationTest.kt +++ b/sentry/src/test/java/io/sentry/protocol/MechanismSerializationTest.kt @@ -28,6 +28,9 @@ class MechanismSerializationTest { "0275caba-1fd8-4de3-9ead-b6c8dcdd5666" to "669cc6ad-1435-4233-b199-2800f901bbcd" ) synthetic = false + isExceptionGroup = false + exceptionId = 1 + parentId = 0 } } private val fixture = Fixture() diff --git a/sentry/src/test/resources/json/mechanism.json b/sentry/src/test/resources/json/mechanism.json index 33348ea745..c13fe9ca22 100644 --- a/sentry/src/test/resources/json/mechanism.json +++ b/sentry/src/test/resources/json/mechanism.json @@ -11,5 +11,8 @@ { "0275caba-1fd8-4de3-9ead-b6c8dcdd5666": "669cc6ad-1435-4233-b199-2800f901bbcd" }, - "synthetic": false + "synthetic": false, + "is_exception_group": false, + "exception_id": 1, + "parent_id": 0 }