diff --git a/CHANGELOG.md b/CHANGELOG.md index d6cc28882a..c9fd18124a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Keep dropped tombstone and ANR events dropped, instead of reporting the same app exit again at every app start ([#6002](https://github.com/getsentry/sentry-java/pull/6002)) + ## 8.54.0 ### Features diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index 65bf072f0a..4b8b41d41c 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -638,6 +638,7 @@ public class io/sentry/android/core/TombstoneIntegration$TombstonePolicy : io/se public fun getLabel ()Ljava/lang/String; public fun getLastReportedTimestamp ()Ljava/lang/Long; public fun getTargetReason ()I + public fun markReported (J)V public fun shouldReportHistorical ()Z } @@ -749,6 +750,8 @@ public final class io/sentry/android/core/cache/AndroidEnvelopeCache : io/sentry public static fun hasStartupCrashMarker (Lio/sentry/SentryOptions;)Z public static fun lastReportedAnr (Lio/sentry/SentryOptions;)Ljava/lang/Long; public static fun lastReportedTombstone (Lio/sentry/SentryOptions;)Ljava/lang/Long; + public static fun markAnrReported (Lio/sentry/SentryOptions;J)V + public static fun markTombstoneReported (Lio/sentry/SentryOptions;J)V public fun store (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)V public fun storeEnvelope (Lio/sentry/SentryEnvelope;Lio/sentry/Hint;)Z } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java b/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java index 285c3b77ad..e28a08f7e8 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AnrV2Integration.java @@ -132,6 +132,11 @@ public boolean shouldReportHistorical() { return AndroidEnvelopeCache.lastReportedAnr(options); } + @Override + public void markReported(final long timestamp) { + AndroidEnvelopeCache.markAnrReported(options, timestamp); + } + @Override public @Nullable ApplicationExitInfoHistoryDispatcher.Report buildReport( final @NotNull ApplicationExitInfo exitInfo, final boolean shouldEnrich) { diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ApplicationExitInfoHistoryDispatcher.java b/sentry-android-core/src/main/java/io/sentry/android/core/ApplicationExitInfoHistoryDispatcher.java index 79c3f19c6e..155c9fab27 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ApplicationExitInfoHistoryDispatcher.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ApplicationExitInfoHistoryDispatcher.java @@ -14,6 +14,7 @@ import io.sentry.hints.BlockingFlushHint; import io.sentry.protocol.SentryId; import io.sentry.transport.ICurrentDateProvider; +import io.sentry.util.HintUtils; import java.util.ArrayList; import java.util.Collections; import java.util.Iterator; @@ -177,6 +178,7 @@ private void reportHistorical( } } + @RequiresApi(api = Build.VERSION_CODES.R) private void report(final @NotNull ApplicationExitInfo exitInfo, final boolean enrich) { final @Nullable Report report = policy.buildReport(exitInfo, enrich); @@ -186,7 +188,28 @@ private void report(final @NotNull ApplicationExitInfo exitInfo, final boolean e final @NotNull SentryId sentryId = scopes.captureEvent(report.getEvent(), report.getHint()); final boolean isEventDropped = sentryId.equals(SentryId.EMPTY_ID); - if (!isEventDropped) { + if (isEventDropped) { + // A dropped event never reaches the envelope disk cache, which is where the last reported + // marker is normally written. Without writing it here, the very same exit would be turned + // into an event again on the next app start, no matter why it was dropped. Only a technical + // failure to hand the event over keeps the exit eligible for another attempt. + if (HintUtils.isCaptureFailed(report.getHint())) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Capturing the %s event failed, leaving the exit for the next app start.", + policy.getLabel()); + } else { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "%s event was dropped, marking the exit as reported.", + policy.getLabel()); + policy.markReported(exitInfo.getTimestamp()); + } + } else { final @Nullable BlockingFlushHint flushHint = report.getFlushHint(); if (flushHint != null && !flushHint.waitFlush()) { options @@ -211,6 +234,9 @@ interface ApplicationExitInfoPolicy { @Nullable Long getLastReportedTimestamp(); + /** Records {@code timestamp} as the last reported exit, so it is not reported again. */ + void markReported(long timestamp); + @Nullable Report buildReport(@NotNull ApplicationExitInfo exitInfo, boolean enrich); } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java index 2663051f7e..d6d6a7767c 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java @@ -147,6 +147,11 @@ public boolean shouldReportHistorical() { return AndroidEnvelopeCache.lastReportedTombstone(options); } + @Override + public void markReported(final long timestamp) { + AndroidEnvelopeCache.markTombstoneReported(options, timestamp); + } + @RequiresApi(api = Build.VERSION_CODES.R) @Override public @Nullable ApplicationExitInfoHistoryDispatcher.Report buildReport( diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/cache/AndroidEnvelopeCache.java b/sentry-android-core/src/main/java/io/sentry/android/core/cache/AndroidEnvelopeCache.java index 1ef02dfdd2..0373c39dee 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/cache/AndroidEnvelopeCache.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/cache/AndroidEnvelopeCache.java @@ -85,7 +85,7 @@ private boolean storeInternalAndroid(@NotNull SentryEnvelope envelope, @NotNull } for (TimestampMarkerHandler handler : TIMESTAMP_MARKER_HANDLERS) { - handler.handle(this, hint, options); + handler.handle(hint, options); } return didStore; @@ -182,7 +182,8 @@ public static boolean hasStartupCrashMarker(final @NotNull SentryOptions options return null; } - private void writeLastReportedMarker( + private static void writeLastReportedMarker( + final @NotNull SentryOptions options, final @Nullable Long timestamp, @NotNull String reportFilename, @NotNull String markerCategory) { @@ -215,6 +216,15 @@ private void writeLastReportedMarker( return lastReportedMarker(options, LAST_TOMBSTONE_REPORT, LAST_TOMBSTONE_MARKER_LABEL); } + public static void markAnrReported(final @NotNull SentryOptions options, final long timestamp) { + writeLastReportedMarker(options, timestamp, LAST_ANR_REPORT, LAST_ANR_MARKER_LABEL); + } + + public static void markTombstoneReported( + final @NotNull SentryOptions options, final long timestamp) { + writeLastReportedMarker(options, timestamp, LAST_TOMBSTONE_REPORT, LAST_TOMBSTONE_MARKER_LABEL); + } + private static final class TimestampMarkerHandler { interface TimestampExtractor { @NotNull @@ -237,10 +247,7 @@ interface TimestampExtractor { this.timestampProvider = timestampProvider; } - void handle( - final @NotNull AndroidEnvelopeCache cache, - final @NotNull Hint hint, - final @NotNull SentryAndroidOptions options) { + void handle(final @NotNull Hint hint, final @NotNull SentryAndroidOptions options) { HintUtils.runIfHasType( hint, type, @@ -253,7 +260,7 @@ void handle( "Writing last reported %s marker with timestamp %d", label, timestamp); - cache.writeLastReportedMarker(timestamp, reportFilename, label); + writeLastReportedMarker(options, timestamp, reportFilename, label); }); } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ApplicationExitIntegrationTestBase.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ApplicationExitIntegrationTestBase.kt index edb2ce1df2..82cf1c25c9 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ApplicationExitIntegrationTestBase.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ApplicationExitIntegrationTestBase.kt @@ -187,6 +187,33 @@ abstract class ApplicationExitIntegrationTestBase { .log(any(), argThat { startsWith(config.flushLogPrefix) }, any()) } + @Test + fun `when latest event was dropped, marks the exit as reported`() { + val integration = + fixture.getSut(tmpDir, lastReportedTimestamp = oldTimestamp, lastEventId = SentryId.EMPTY_ID) + fixture.addAppExitInfo(timestamp = newTimestamp) + + integration.register(fixture.scopes, fixture.options) + + assertEquals(newTimestamp.toString(), fixture.lastReportedFile.readText()) + } + + @Test + fun `when capturing the latest event failed, does not mark the exit as reported`() { + val integration = + fixture.getSut( + tmpDir, + lastReportedTimestamp = oldTimestamp, + lastEventId = SentryId.EMPTY_ID, + captureFailed = true, + ) + fixture.addAppExitInfo(timestamp = newTimestamp) + + integration.register(fixture.scopes, fixture.options) + + assertEquals(oldTimestamp.toString(), fixture.lastReportedFile.readText()) + } + @Test fun `historical exits are reported non-enriched`() { val integration = fixture.getSut(tmpDir, lastReportedTimestamp = oldTimestamp) @@ -403,6 +430,7 @@ abstract class ApplicationExitIntegrationTestBase { sessionFlushTimeoutMillis: Long = 0L, lastReportedTimestamp: Long? = null, lastEventId: SentryId = SentryId(), + captureFailed: Boolean = false, sessionTrackingEnabled: Boolean = true, reportHistorical: Boolean = true, extraOptions: (SentryAndroidOptions) -> Unit = {}, @@ -426,7 +454,12 @@ abstract class ApplicationExitIntegrationTestBase { lastReportedFile = File(cacheDir, config.lastReportedFileName) lastReportedFile.writeText(lastReportedTimestamp.toString()) } - whenever(scopes.captureEvent(any(), anyOrNull())).thenReturn(lastEventId) + whenever(scopes.captureEvent(any(), anyOrNull())).thenAnswer { invocation -> + if (captureFailed) { + HintUtils.setCaptureFailed(invocation.getArgument(1)) + } + lastEventId + } return config.createIntegration(context) } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index ff75e82d0a..ec756beb11 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -4737,6 +4737,7 @@ public final class io/sentry/TypeCheckHint { public static final field OPEN_FEIGN_REQUEST Ljava/lang/String; public static final field OPEN_FEIGN_RESPONSE Ljava/lang/String; public static final field REPLAY_FRAME_BITMAP Ljava/lang/String; + public static final field SENTRY_CAPTURE_FAILED Ljava/lang/String; public static final field SENTRY_DART_SDK_NAME Ljava/lang/String; public static final field SENTRY_DOTNET_SDK_NAME Ljava/lang/String; public static final field SENTRY_EVENT_DROP_REASON Ljava/lang/String; @@ -7762,11 +7763,13 @@ public final class io/sentry/util/HintUtils { public static fun getEventDropReason (Lio/sentry/Hint;)Lio/sentry/hints/EventDropReason; public static fun getSentrySdkHint (Lio/sentry/Hint;)Ljava/lang/Object; public static fun hasType (Lio/sentry/Hint;Ljava/lang/Class;)Z + public static fun isCaptureFailed (Lio/sentry/Hint;)Z public static fun isFromHybridSdk (Lio/sentry/Hint;)Z public static fun runIfDoesNotHaveType (Lio/sentry/Hint;Ljava/lang/Class;Lio/sentry/util/HintUtils$SentryNullableConsumer;)V public static fun runIfHasType (Lio/sentry/Hint;Ljava/lang/Class;Lio/sentry/util/HintUtils$SentryConsumer;)V public static fun runIfHasType (Lio/sentry/Hint;Ljava/lang/Class;Lio/sentry/util/HintUtils$SentryConsumer;Lio/sentry/util/HintUtils$SentryHintFallback;)V public static fun runIfHasTypeLogIfNot (Lio/sentry/Hint;Ljava/lang/Class;Lio/sentry/ILogger;Lio/sentry/util/HintUtils$SentryConsumer;)V + public static fun setCaptureFailed (Lio/sentry/Hint;)V public static fun setEventDropReason (Lio/sentry/Hint;Lio/sentry/hints/EventDropReason;)V public static fun setIsFromHybridSdk (Lio/sentry/Hint;Ljava/lang/String;)V public static fun setTypeCheckHint (Lio/sentry/Hint;Ljava/lang/Object;)V diff --git a/sentry/src/main/java/io/sentry/Scopes.java b/sentry/src/main/java/io/sentry/Scopes.java index 936a331e3d..d3926d5699 100644 --- a/sentry/src/main/java/io/sentry/Scopes.java +++ b/sentry/src/main/java/io/sentry/Scopes.java @@ -152,8 +152,10 @@ public boolean isEnabled() { .getLogger() .log( SentryLevel.WARNING, "Instance is disabled and this 'captureEvent' call is a no-op."); + HintUtils.setCaptureFailed(hint); } else if (event == null) { getOptions().getLogger().log(SentryLevel.WARNING, "captureEvent called with null parameter."); + HintUtils.setCaptureFailed(hint); } else { try { assignTraceContext(event); @@ -166,6 +168,7 @@ public boolean isEnabled() { .getLogger() .log( SentryLevel.ERROR, "Error while capturing event with id: " + event.getEventId(), e); + HintUtils.setCaptureFailed(hint); } } return sentryId; diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 18e247c510..0e44812a49 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -287,6 +287,7 @@ private boolean shouldApplyScopeData(final @NotNull CheckIn event, final @NotNul options.getLogger().log(SentryLevel.WARNING, e, "Capturing event %s failed.", sentryId); // if there was an error capturing the event, we return an emptyId + HintUtils.setCaptureFailed(hint); sentryId = SentryId.EMPTY_ID; } diff --git a/sentry/src/main/java/io/sentry/TypeCheckHint.java b/sentry/src/main/java/io/sentry/TypeCheckHint.java index b3b061e847..3260b46f16 100644 --- a/sentry/src/main/java/io/sentry/TypeCheckHint.java +++ b/sentry/src/main/java/io/sentry/TypeCheckHint.java @@ -13,6 +13,8 @@ public final class TypeCheckHint { @ApiStatus.Internal public static final String SENTRY_EVENT_DROP_REASON = "sentry:eventDropReason"; + @ApiStatus.Internal public static final String SENTRY_CAPTURE_FAILED = "sentry:captureFailed"; + @ApiStatus.Internal public static final String SENTRY_REPLAY_NETWORK_DETAILS = "sentry:replayNetworkDetails"; diff --git a/sentry/src/main/java/io/sentry/util/HintUtils.java b/sentry/src/main/java/io/sentry/util/HintUtils.java index da568c614b..4ccbce3c33 100644 --- a/sentry/src/main/java/io/sentry/util/HintUtils.java +++ b/sentry/src/main/java/io/sentry/util/HintUtils.java @@ -1,5 +1,6 @@ package io.sentry.util; +import static io.sentry.TypeCheckHint.SENTRY_CAPTURE_FAILED; import static io.sentry.TypeCheckHint.SENTRY_DART_SDK_NAME; import static io.sentry.TypeCheckHint.SENTRY_DOTNET_SDK_NAME; import static io.sentry.TypeCheckHint.SENTRY_EVENT_DROP_REASON; @@ -45,6 +46,21 @@ public static EventDropReason getEventDropReason(final @NotNull Hint hint) { return hint.getAs(SENTRY_EVENT_DROP_REASON, EventDropReason.class); } + /** + * Marks the event as not captured because of a technical failure, as opposed to being dropped on + * purpose. Callers that keep an event around for a later attempt use this to tell the two apart: + * an empty event id alone does not, because almost every drop also returns one. + */ + public static void setCaptureFailed(final @Nullable Hint hint) { + if (hint != null) { + hint.set(SENTRY_CAPTURE_FAILED, true); + } + } + + public static boolean isCaptureFailed(final @NotNull Hint hint) { + return Boolean.TRUE.equals(hint.getAs(SENTRY_CAPTURE_FAILED, Boolean.class)); + } + public static Hint createWithTypeCheckHint(Object typeCheckHint) { Hint hint = new Hint(); setTypeCheckHint(hint, typeCheckHint); diff --git a/sentry/src/test/java/io/sentry/ScopesTest.kt b/sentry/src/test/java/io/sentry/ScopesTest.kt index 9d598aec88..d1cb38c649 100644 --- a/sentry/src/test/java/io/sentry/ScopesTest.kt +++ b/sentry/src/test/java/io/sentry/ScopesTest.kt @@ -344,6 +344,17 @@ class ScopesTest { verify(mockClient, never()).captureEvent(any(), any()) } + @Test + fun `when captureEvent is called on disabled client, the hint is marked as capture failed`() { + val (sut, _) = getEnabledScopes() + sut.close() + + val hint = Hint() + sut.captureEvent(SentryEvent(), hint) + + assertTrue(HintUtils.isCaptureFailed(hint)) + } + @Test fun `when captureEvent is called with a valid argument, captureEvent on the client should be called`() { val (sut, mockClient) = getEnabledScopes() diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index e4c4b447cf..3066d2c1b5 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -1635,6 +1635,28 @@ class SentryClientTest { verify(fixture.transport).send(check { assertNotNull(it.header.sdkVersion) }, anyOrNull()) } + @Test + fun `when capturing the event throws, the hint is marked as capture failed`() { + whenever(fixture.transport.send(any(), anyOrNull())).thenThrow(IOException()) + + val hint = Hint() + val sentryId = fixture.getSut().captureEvent(SentryEvent(), hint) + + assertEquals(SentryId.EMPTY_ID, sentryId) + assertTrue(HintUtils.isCaptureFailed(hint)) + } + + @Test + fun `when the event is dropped, the hint is not marked as capture failed`() { + fixture.sentryOptions.setBeforeSend { _, _ -> null } + + val hint = Hint() + val sentryId = fixture.getSut().captureEvent(SentryEvent(), hint) + + assertEquals(SentryId.EMPTY_ID, sentryId) + assertFalse(HintUtils.isCaptureFailed(hint)) + } + @Test fun `when captureEnvelope and thres an exception, returns empty sentryId`() { whenever(fixture.transport.send(any(), anyOrNull())).thenThrow(IOException())