diff --git a/CHANGELOG.md b/CHANGELOG.md index eaed5ce3eb..7b48c92798 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -124,6 +124,7 @@ - Fix SDK callback error handling ([#6140](https://github.com/getsentry/sentry-java/pull/6140)) - Add `DiscardReason.CALLBACK_ERROR` and use it for telemetry dropped when a `beforeSend*` callback throws. `OnDiscardCallback` can now receive this value. + - Drop telemetry and record `callback_error` when an event processor throws instead of continuing with a potentially partially processed item. - Disable URL caching when reading `META-INF/MANIFEST.MF` files during version detection so that the SDK no longer keeps jar file handles open for the life of the process ([#6124](https://github.com/getsentry/sentry-java/pull/6124) - Keep the `EventListener` wrapped by `SentryOkHttpEventListener` per `Call` ([#6003](https://github.com/getsentry/sentry-java/pull/6003)) diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index a9cc392e71..cebc37a393 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -506,6 +506,10 @@ private SentryEvent processEvent( e, "An exception occurred while processing event by processor: %s", processor.getClass().getName()); + options + .getClientReportRecorder() + .recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Error); + return null; } if (event == null) { @@ -557,6 +561,8 @@ private SentryLogEvent processLogEvent( e, "An exception occurred while processing log event by processor: %s", processor.getClass().getName()); + recordLostLogEvent(DiscardReason.CALLBACK_ERROR, eventBeforeProcessor); + return null; } if (event == null) { @@ -590,6 +596,8 @@ private SentryMetricsEvent processMetricsEvent( e, "An exception occurred while processing metrics event by processor: %s", processor.getClass().getName()); + recordLostMetricsEvent(DiscardReason.CALLBACK_ERROR, eventBeforeProcessor); + return null; } if (event == null) { @@ -622,6 +630,14 @@ private SentryMetricsEvent processMetricsEvent( e, "An exception occurred while processing transaction by processor: %s", processor.getClass().getName()); + options + .getClientReportRecorder() + .recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Transaction); + options + .getClientReportRecorder() + .recordLostEvent( + DiscardReason.CALLBACK_ERROR, DataCategory.Span, spanCountBeforeProcessor + 1); + return null; } final int spanCountAfterProcessor = transaction == null ? 0 : transaction.getSpans().size(); @@ -675,6 +691,10 @@ private SentryReplayEvent processReplayEvent( e, "An exception occurred while processing replay event by processor: %s", processor.getClass().getName()); + options + .getClientReportRecorder() + .recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Replay); + return null; } if (replayEvent == null) { @@ -709,6 +729,10 @@ private SentryEvent processFeedbackEvent( e, "An exception occurred while processing feedback event by processor: %s", processor.getClass().getName()); + options + .getClientReportRecorder() + .recordLostEvent(DiscardReason.CALLBACK_ERROR, DataCategory.Feedback); + return null; } if (feedbackEvent == null) { diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 7b907df1e8..ff6fc57900 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -1,5 +1,6 @@ package io.sentry +import com.google.common.truth.Truth.assertThat import io.sentry.Scope.IWithPropagationContext import io.sentry.SentryLevel.WARNING import io.sentry.Session.State.Crashed @@ -474,6 +475,48 @@ class SentryClientTest { ) } + @Test + fun `throwing log processor drops log and stops callbacks`() { + val scope = createScope() + val logEvent = SentryLogEvent(SentryId(), SentryNanotimeDate(), "message", SentryLogLevel.WARN) + val logEventNumberOfBytes = + JsonSerializationUtils.byteSizeOf( + fixture.sentryOptions.serializer, + fixture.sentryOptions.logger, + logEvent, + ) + val throwingProcessor = mock() + val nextProcessor = mock() + val beforeSend = mock() + val onDiscard = mock() + whenever(throwingProcessor.process(any())) + .thenThrow(IllegalStateException("test")) + scope.addEventProcessor(throwingProcessor) + scope.addEventProcessor(nextProcessor) + fixture.sentryOptions.logs.beforeSend = beforeSend + fixture.sentryOptions.onDiscard = onDiscard + + fixture.getSut().captureLog(logEvent, scope) + + verify(nextProcessor, never()).process(any()) + verify(beforeSend, never()).execute(any()) + verify(fixture.loggerBatchProcessor, never()).add(any()) + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf( + DiscardedEvent(DiscardReason.CALLBACK_ERROR.reason, DataCategory.LogItem.category, 1), + DiscardedEvent( + DiscardReason.CALLBACK_ERROR.reason, + DataCategory.LogByte.category, + logEventNumberOfBytes, + ), + ), + ) + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.LogItem, 1) + verify(onDiscard) + .execute(DiscardReason.CALLBACK_ERROR, DataCategory.LogByte, logEventNumberOfBytes) + } + @Test fun `when beforeSendLog is returns new instance, new instance is sent`() { val scope = createScope() @@ -595,6 +638,56 @@ class SentryClientTest { ) } + @Test + fun `throwing metric processor drops metric and stops callbacks`() { + val scope = createScope() + val metricsEvent = SentryMetricsEvent(SentryId(), SentryNanotimeDate(), "name", "gauge", 123.0) + val metricsEventNumberOfBytes = + JsonSerializationUtils.byteSizeOf( + fixture.sentryOptions.serializer, + fixture.sentryOptions.logger, + metricsEvent, + ) + val throwingProcessor = mock() + val nextProcessor = mock() + val beforeSend = mock() + val onDiscard = mock() + whenever(throwingProcessor.process(any(), anyOrNull())) + .thenThrow(IllegalStateException("test")) + scope.addEventProcessor(throwingProcessor) + scope.addEventProcessor(nextProcessor) + fixture.sentryOptions.metrics.beforeSend = beforeSend + fixture.sentryOptions.onDiscard = onDiscard + + fixture.getSut().captureMetric(metricsEvent, scope, null) + + verify(nextProcessor, never()).process(any(), anyOrNull()) + verify(beforeSend, never()).execute(any(), anyOrNull()) + verify(fixture.metricsBatchProcessor, never()).add(any()) + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf( + DiscardedEvent( + DiscardReason.CALLBACK_ERROR.reason, + DataCategory.TraceMetric.category, + 1, + ), + DiscardedEvent( + DiscardReason.CALLBACK_ERROR.reason, + DataCategory.TraceMetricByte.category, + metricsEventNumberOfBytes, + ), + ), + ) + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.TraceMetric, 1) + verify(onDiscard) + .execute( + DiscardReason.CALLBACK_ERROR, + DataCategory.TraceMetricByte, + metricsEventNumberOfBytes, + ) + } + @Test fun `when beforeSendMetric is returns new instance, new instance is sent`() { val scope = createScope() @@ -1242,6 +1335,42 @@ class SentryClientTest { ) } + @Test + fun `throwing transaction processor drops transaction and stops callbacks`() { + val throwingProcessor = mock() + val nextProcessor = mock() + val beforeSend = mock() + val onDiscard = mock() + whenever(throwingProcessor.process(any(), anyOrNull())) + .thenThrow(IllegalStateException("test")) + fixture.sentryOptions.addEventProcessor(throwingProcessor) + fixture.sentryOptions.addEventProcessor(nextProcessor) + fixture.sentryOptions.beforeSendTransaction = beforeSend + fixture.sentryOptions.onDiscard = onDiscard + + val id = + fixture + .getSut() + .captureTransaction( + SentryTransaction(fixture.sentryTracer), + fixture.sentryTracer.traceContext(), + ) + + assertThat(id).isEqualTo(SentryId.EMPTY_ID) + verify(nextProcessor, never()).process(any(), anyOrNull()) + verify(beforeSend, never()).execute(any(), anyOrNull()) + verify(fixture.transport, never()).send(any(), anyOrNull()) + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf( + DiscardedEvent(DiscardReason.CALLBACK_ERROR.reason, DataCategory.Transaction.category, 1), + DiscardedEvent(DiscardReason.CALLBACK_ERROR.reason, DataCategory.Span.category, 2), + ), + ) + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Transaction, 1) + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Span, 2) + } + @Test fun `transaction dropped by ignoredTransactions is recorded`() { fixture.sentryOptions.setIgnoredTransactions(listOf("a-transaction")) @@ -1927,10 +2056,29 @@ class SentryClientTest { } @Test - fun `exception thrown by an event processor is handled gracefully`() { - fixture.sentryOptions.addEventProcessor(eventProcessorThrows()) - val sut = fixture.getSut() - sut.captureEvent(SentryEvent()) + fun `exception thrown by an event processor drops event and stops callbacks`() { + val throwingProcessor = mock() + val nextProcessor = mock() + val beforeSend = mock() + val onDiscard = mock() + whenever(throwingProcessor.process(any(), anyOrNull())) + .thenThrow(IllegalStateException("test")) + fixture.sentryOptions.addEventProcessor(throwingProcessor) + fixture.sentryOptions.addEventProcessor(nextProcessor) + fixture.sentryOptions.beforeSend = beforeSend + fixture.sentryOptions.onDiscard = onDiscard + + val id = fixture.getSut().captureEvent(SentryEvent()) + + assertThat(id).isEqualTo(SentryId.EMPTY_ID) + verify(nextProcessor, never()).process(any(), anyOrNull()) + verify(beforeSend, never()).execute(any(), anyOrNull()) + verify(fixture.transport, never()).send(any(), anyOrNull()) + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf(DiscardedEvent(DiscardReason.CALLBACK_ERROR.reason, DataCategory.Error.category, 1)), + ) + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Error, 1) } @Test @@ -3524,6 +3672,32 @@ class SentryClientTest { verify(onDiscardMock, times(1)).execute(DiscardReason.EVENT_PROCESSOR, DataCategory.Replay, 1) } + @Test + fun `throwing replay processor drops replay and stops callbacks`() { + val throwingProcessor = mock() + val nextProcessor = mock() + val beforeSend = mock() + val onDiscard = mock() + whenever(throwingProcessor.process(any(), anyOrNull())) + .thenThrow(IllegalStateException("test")) + fixture.sentryOptions.addEventProcessor(throwingProcessor) + fixture.sentryOptions.addEventProcessor(nextProcessor) + fixture.sentryOptions.beforeSendReplay = beforeSend + fixture.sentryOptions.onDiscard = onDiscard + + val id = fixture.getSut().captureReplayEvent(createReplayEvent(), createScope(), null) + + assertThat(id).isEqualTo(SentryId.EMPTY_ID) + verify(nextProcessor, never()).process(any(), anyOrNull()) + verify(beforeSend, never()).execute(any(), anyOrNull()) + verify(fixture.transport, never()).send(any(), anyOrNull()) + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf(DiscardedEvent(DiscardReason.CALLBACK_ERROR.reason, DataCategory.Replay.category, 1)), + ) + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Replay, 1) + } + @Test fun `calls captureReplay on replay controller for error events`() { var called = false @@ -4086,6 +4260,34 @@ class SentryClientTest { verify(onDiscardMock, times(1)).execute(DiscardReason.EVENT_PROCESSOR, DataCategory.Feedback, 1) } + @Test + fun `throwing feedback processor drops feedback and stops callbacks`() { + val throwingProcessor = mock() + val nextProcessor = mock() + val beforeSend = mock() + val onDiscard = mock() + whenever(throwingProcessor.process(any(), anyOrNull())) + .thenThrow(IllegalStateException("test")) + fixture.sentryOptions.addEventProcessor(throwingProcessor) + fixture.sentryOptions.addEventProcessor(nextProcessor) + fixture.sentryOptions.beforeSendFeedback = beforeSend + fixture.sentryOptions.onDiscard = onDiscard + + val id = fixture.getSut().captureFeedback(Feedback("message"), null, createScope()) + + assertThat(id).isEqualTo(SentryId.EMPTY_ID) + verify(nextProcessor, never()).process(any(), anyOrNull()) + verify(beforeSend, never()).execute(any(), anyOrNull()) + verify(fixture.transport, never()).send(any(), anyOrNull()) + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf( + DiscardedEvent(DiscardReason.CALLBACK_ERROR.reason, DataCategory.Feedback.category, 1) + ), + ) + verify(onDiscard).execute(DiscardReason.CALLBACK_ERROR, DataCategory.Feedback, 1) + } + // endregion private fun givenScopeWithStartedSession( @@ -4352,14 +4554,6 @@ class SentryClientTest { override fun timestamp(): Long? = null } - private fun eventProcessorThrows(): EventProcessor { - return object : EventProcessor { - override fun process(event: SentryEvent, hint: Hint): SentryEvent? { - throw Throwable() - } - } - } - private class BackfillableHint : Backfillable { override fun shouldEnrich(): Boolean = false }