From f8355d1cb7792f1244445f06db7d26057364297f Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 10 May 2022 09:24:33 +0200 Subject: [PATCH 1/2] Change order of event filtering mechanisms... ... and only send session update for dropped events if session state changed --- .../src/main/java/io/sentry/SentryClient.java | 85 ++++- .../test/java/io/sentry/SentryClientTest.kt | 330 +++++++++++++++++- 2 files changed, 394 insertions(+), 21 deletions(-) diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index b7fb87f4906..d446afb1215 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -73,6 +73,19 @@ private boolean shouldApplyScopeData( options.getLogger().log(SentryLevel.DEBUG, "Capturing event: %s", event.getEventId()); + if (event != null) { + final Throwable eventThrowable = event.getThrowable(); + if (eventThrowable != null && options.containsIgnoredExceptionForType(eventThrowable)) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Event was dropped as the exception %s is ignored", + eventThrowable.getClass()); + return SentryId.EMPTY_ID; + } + } + if (shouldApplyScopeData(event, hint)) { // Event has already passed through here before it was cached // Going through again could be reading data that is no longer relevant @@ -87,7 +100,22 @@ private boolean shouldApplyScopeData( event = processEvent(event, hint, options.getEventProcessors()); - Session session = null; + if (event != null) { + event = executeBeforeSend(event, hint); + + if (event == null) { + options.getLogger().log(SentryLevel.DEBUG, "Event was dropped by beforeSend"); + } + } + + if (event == null) { + return SentryId.EMPTY_ID; + } + + @Nullable + Session sessionBeforeUpdate = + scope != null ? scope.withSession((@Nullable Session session) -> {}) : null; + @Nullable Session session = null; if (event != null) { session = updateSessionData(event, hint, scope); @@ -104,22 +132,16 @@ private boolean shouldApplyScopeData( } } - if (event != null) { - if (event.getThrowable() != null - && options.containsIgnoredExceptionForType(event.getThrowable())) { - options - .getLogger() - .log( - SentryLevel.DEBUG, - "Event was dropped as the exception %s is ignored", - event.getThrowable().getClass()); - return SentryId.EMPTY_ID; - } - event = executeBeforeSend(event, hint); + final boolean shouldSendSessionUpdate = + shouldSendSessionUpdateForDroppedEvent(sessionBeforeUpdate, session); - if (event == null) { - options.getLogger().log(SentryLevel.DEBUG, "Event was dropped by beforeSend"); - } + if (event == null && !shouldSendSessionUpdate) { + options + .getLogger() + .log( + SentryLevel.DEBUG, + "Not sending session update for dropped event as it did not cause the session health to change."); + return SentryId.EMPTY_ID; } SentryId sentryId = SentryId.EMPTY_ID; @@ -132,8 +154,9 @@ private boolean shouldApplyScopeData( scope != null && scope.getTransaction() != null ? scope.getTransaction().traceState() : null; - final SentryEnvelope envelope = - buildEnvelope(event, getAttachmentsFromScope(scope), session, traceState); + final boolean shouldSendAttachments = event != null; + List attachments = shouldSendAttachments ? getAttachmentsFromScope(scope) : null; + final SentryEnvelope envelope = buildEnvelope(event, attachments, session, traceState); if (envelope != null) { transport.send(envelope, hint); @@ -148,6 +171,32 @@ private boolean shouldApplyScopeData( return sentryId; } + private boolean shouldSendSessionUpdateForDroppedEvent( + @Nullable Session sessionBeforeUpdate, @Nullable Session sessionAfterUpdate) { + if (sessionAfterUpdate == null) { + return false; + } + + if (sessionBeforeUpdate == null) { + return true; + } + + final boolean didSessionMoveToCrashedState = + sessionAfterUpdate.getStatus() == Session.State.Crashed + && sessionBeforeUpdate.getStatus() != Session.State.Crashed; + if (didSessionMoveToCrashedState) { + return true; + } + + final boolean didSessionMoveToErroredState = + sessionAfterUpdate.errorCount() > 0 && sessionBeforeUpdate.errorCount() <= 0; + if (didSessionMoveToErroredState) { + return true; + } + + return false; + } + private @Nullable List getAttachmentsFromScope(@Nullable Scope scope) { if (scope != null) { return scope.getAttachments(); diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index c7975f1b904..69848ff7181 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -2,12 +2,16 @@ package io.sentry import com.nhaarman.mockitokotlin2.any import com.nhaarman.mockitokotlin2.anyOrNull +import com.nhaarman.mockitokotlin2.argumentCaptor import com.nhaarman.mockitokotlin2.check +import com.nhaarman.mockitokotlin2.doAnswer import com.nhaarman.mockitokotlin2.eq +import com.nhaarman.mockitokotlin2.inOrder import com.nhaarman.mockitokotlin2.isNull import com.nhaarman.mockitokotlin2.mock import com.nhaarman.mockitokotlin2.mockingDetails import com.nhaarman.mockitokotlin2.never +import com.nhaarman.mockitokotlin2.times import com.nhaarman.mockitokotlin2.verify import com.nhaarman.mockitokotlin2.verifyNoMoreInteractions import com.nhaarman.mockitokotlin2.whenever @@ -75,7 +79,10 @@ class SentryClientTest { var attachment = Attachment("hello".toByteArray(), "hello.txt", "text/plain", true) - fun getSut() = SentryClient(sentryOptions) + fun getSut(optionsCallback: ((SentryOptions) -> Unit)? = null): SentryClient { + optionsCallback?.invoke(sentryOptions) + return SentryClient(sentryOptions) + } } private val fixture = Fixture() @@ -1156,9 +1163,308 @@ class SentryClientTest { sut.captureException(IllegalStateException()) verify(fixture.transport, never()).send(any(), anyOrNull()) } + @Test + fun `capturing an error updates session and sends event + session`() { + val sut = fixture.getSut() + val scope = givenScopeWithStartedSession() + + sut.captureEvent(SentryEvent().apply { exceptions = createHandledException() }, scope) + + thenSessionIsErrored(scope) + thenEnvelopeIsSentWith(eventCount = 1, sessionCount = 1) + } + + @Test + fun `dropping a captured error from beforeSend has no effect on session and does not send anything`() { + val sut = fixture.getSut { options -> + options.beforeSend = SentryOptions.BeforeSendCallback { _, _ -> null } + } + val scope = givenScopeWithStartedSession() + + sut.captureEvent(SentryEvent().apply { exceptions = createHandledException() }, scope) + + thenSessionIsStillOK(scope) + thenNothingIsSent() + } + + @Test + fun `dropping a captured error from eventProcessor has no effect on session and does not send anything`() { + val sut = fixture.getSut { options -> + options.addEventProcessor(DropEverythingEventProcessor()) + } + val scope = givenScopeWithStartedSession() + + sut.captureEvent(SentryEvent().apply { exceptions = createHandledException() }, scope) + + thenSessionIsStillOK(scope) + thenNothingIsSent() + } + + @Test + fun `dropping a captured error via sampling updates the session and only sends the session for a new session`() { + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + } + val scope = givenScopeWithStartedSession() + + sut.captureEvent(SentryEvent().apply { exceptions = createHandledException() }, scope) + + thenSessionIsErrored(scope) + thenEnvelopeIsSentWith(eventCount = 0, sessionCount = 1) + } - private fun createScope(): Scope { - return Scope(SentryOptions()).apply { + @Test + fun `dropping a captured error via sampling updates the session and does not send anything for an errored session`() { + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + } + val scope = givenScopeWithStartedSession(errored = true) + + sut.captureEvent(SentryEvent().apply { exceptions = createHandledException() }, scope) + + thenSessionIsErrored(scope) + thenNothingIsSent() + } + + @Test + fun `dropping a captured error via sampling updates the session and does not send anything for a crashed session`() { + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + } + val scope = givenScopeWithStartedSession(crashed = true) + + sut.captureEvent(SentryEvent().apply { exceptions = createHandledException() }, scope) + + thenSessionIsCrashed(scope) + thenNothingIsSent() + } + + @Test + fun `dropping a captured crash via sampling updates the session and only sends the session for a new session`() { + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + } + val scope = givenScopeWithStartedSession() + + sut.captureEvent(SentryEvent().apply { exceptions = createNonHandledException() }, scope) + + thenSessionIsCrashed(scope) + thenEnvelopeIsSentWith(eventCount = 0, sessionCount = 1) + } + + @Test + fun `dropping a captured crash via sampling updates the session and sends the session for an errored session`() { + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + } + val scope = givenScopeWithStartedSession(errored = true) + + sut.captureEvent(SentryEvent().apply { exceptions = createNonHandledException() }, scope) + + thenSessionIsCrashed(scope) + thenEnvelopeIsSentWith(eventCount = 0, sessionCount = 1) + } + + @Test + fun `dropping a captured crash via sampling updates the session and does not send anything for a crashed session`() { + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + } + val scope = givenScopeWithStartedSession(crashed = true) + + sut.captureEvent(SentryEvent().apply { exceptions = createNonHandledException() }, scope) + + thenSessionIsCrashed(scope) + thenNothingIsSent() + } + + @Test + fun `ignored exceptions are checked before other filter mechanisms`() { + val beforeSendMock = mock() + val scopedEventProcessorMock = mock() + val globalEventProcessorMock = mock() + + whenever(scopedEventProcessorMock.process(any(), anyOrNull())).thenReturn(null) + whenever(globalEventProcessorMock.process(any(), anyOrNull())).thenReturn(null) + whenever(beforeSendMock.execute(any(), anyOrNull())).thenReturn(null) + + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + options.addIgnoredExceptionForType(NegativeArraySizeException::class.java) + options.beforeSend = beforeSendMock + options.addEventProcessor(globalEventProcessorMock) + } + val scope = givenScopeWithStartedSession() + scope.addEventProcessor(scopedEventProcessorMock) + + sut.captureException(NegativeArraySizeException(), scope) + + verify(scopedEventProcessorMock, never()).process(any(), anyOrNull()) + verify(globalEventProcessorMock, never()).process(any(), anyOrNull()) + verify(beforeSendMock, never()).execute(any(), anyOrNull()) + } + + @Test + fun `sampling is last filter mechanism`() { + val beforeSendMock = mock() + val scopedEventProcessorMock = mock() + val globalEventProcessorMock = mock() + + whenever(scopedEventProcessorMock.process(any(), anyOrNull())).doAnswer { it.arguments.first() as SentryEvent } + whenever(globalEventProcessorMock.process(any(), anyOrNull())).doAnswer { it.arguments.first() as SentryEvent } + whenever(beforeSendMock.execute(any(), anyOrNull())).doAnswer { it.arguments.first() as SentryEvent } + + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + options.addIgnoredExceptionForType(NegativeArraySizeException::class.java) + options.beforeSend = beforeSendMock + options.addEventProcessor(globalEventProcessorMock) + } + val scope = givenScopeWithStartedSession() + scope.addEventProcessor(scopedEventProcessorMock) + + sut.captureException(IllegalStateException(), scope) + + val order = inOrder(scopedEventProcessorMock, globalEventProcessorMock, beforeSendMock) + + order.verify(scopedEventProcessorMock, times(1)).process(any(), anyOrNull()) + order.verify(globalEventProcessorMock, times(1)).process(any(), anyOrNull()) + order.verify(beforeSendMock, times(1)).execute(any(), anyOrNull()) + } + + @Test + fun `filter mechanism order check for beforeSend`() { + val beforeSendMock = mock() + val scopedEventProcessorMock = mock() + val globalEventProcessorMock = mock() + + whenever(scopedEventProcessorMock.process(any(), anyOrNull())).doAnswer { it.arguments.first() as SentryEvent } + whenever(globalEventProcessorMock.process(any(), anyOrNull())).doAnswer { it.arguments.first() as SentryEvent } + whenever(beforeSendMock.execute(any(), anyOrNull())).thenReturn(null) + + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + options.addIgnoredExceptionForType(NegativeArraySizeException::class.java) + options.beforeSend = beforeSendMock + options.addEventProcessor(globalEventProcessorMock) + } + val scope = givenScopeWithStartedSession() + scope.addEventProcessor(scopedEventProcessorMock) + + sut.captureException(IllegalStateException(), scope) + + val order = inOrder(scopedEventProcessorMock, globalEventProcessorMock, beforeSendMock) + + order.verify(scopedEventProcessorMock, times(1)).process(any(), anyOrNull()) + order.verify(globalEventProcessorMock, times(1)).process(any(), anyOrNull()) + order.verify(beforeSendMock, times(1)).execute(any(), anyOrNull()) + } + + @Test + fun `filter mechanism order check for scoped eventProcessor`() { + val beforeSendMock = mock() + val scopedEventProcessorMock = mock() + val globalEventProcessorMock = mock() + + whenever(scopedEventProcessorMock.process(any(), anyOrNull())).thenReturn(null) + whenever(globalEventProcessorMock.process(any(), anyOrNull())).thenReturn(null) + whenever(beforeSendMock.execute(any(), anyOrNull())).thenReturn(null) + + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + options.addIgnoredExceptionForType(NegativeArraySizeException::class.java) + options.beforeSend = beforeSendMock + options.addEventProcessor(globalEventProcessorMock) + } + val scope = givenScopeWithStartedSession() + scope.addEventProcessor(scopedEventProcessorMock) + + sut.captureException(IllegalStateException(), scope) + + val order = inOrder(scopedEventProcessorMock, globalEventProcessorMock, beforeSendMock) + + order.verify(scopedEventProcessorMock, times(1)).process(any(), anyOrNull()) + order.verify(globalEventProcessorMock, never()).process(any(), anyOrNull()) + order.verify(beforeSendMock, never()).execute(any(), anyOrNull()) + } + + @Test + fun `filter mechanism order check for global eventProcessor`() { + val beforeSendMock = mock() + val scopedEventProcessorMock = mock() + val globalEventProcessorMock = mock() + + whenever(scopedEventProcessorMock.process(any(), anyOrNull())).doAnswer { it.arguments.first() as SentryEvent } + whenever(globalEventProcessorMock.process(any(), anyOrNull())).thenReturn(null) + whenever(beforeSendMock.execute(any(), anyOrNull())).thenReturn(null) + + val sut = fixture.getSut { options -> + options.sampleRate = 0.000000000001 + options.addIgnoredExceptionForType(NegativeArraySizeException::class.java) + options.beforeSend = beforeSendMock + options.addEventProcessor(globalEventProcessorMock) + } + val scope = givenScopeWithStartedSession() + scope.addEventProcessor(scopedEventProcessorMock) + + sut.captureException(IllegalStateException(), scope) + + val order = inOrder(scopedEventProcessorMock, globalEventProcessorMock, beforeSendMock) + + order.verify(scopedEventProcessorMock, times(1)).process(any(), anyOrNull()) + order.verify(globalEventProcessorMock, times(1)).process(any(), anyOrNull()) + order.verify(beforeSendMock, never()).execute(any(), anyOrNull()) + } + + private fun givenScopeWithStartedSession(errored: Boolean = false, crashed: Boolean = false): Scope { + val scope = createScope(fixture.sentryOptions) + scope.startSession() + + if (errored) { + scope.withSession { it?.update(Session.State.Ok, "some-user-agent", true) } + } + + if (crashed) { + scope.withSession { it?.update(Session.State.Crashed, "some-user-agent", true) } + } + + return scope + } + + private fun thenNothingIsSent() { + verify(fixture.transport, never()).send(anyOrNull(), anyOrNull()) + } + + private fun thenEnvelopeIsSentWith(eventCount: Int, sessionCount: Int) { + val argumentCaptor = argumentCaptor() + verify(fixture.transport, times(1)).send(argumentCaptor.capture(), anyOrNull()) + + val envelope = argumentCaptor.firstValue + val envelopeItemTypes = envelope.items.map { it.header.type } + assertEquals(eventCount, envelopeItemTypes.count { it == SentryItemType.Event }) + assertEquals(sessionCount, envelopeItemTypes.count { it == SentryItemType.Session }) + } + + private fun thenSessionIsStillOK(scope: Scope) { + val sessionAfterCapture = scope.withSession { }!! + assertEquals(0, sessionAfterCapture.errorCount()) + assertEquals(Session.State.Ok, sessionAfterCapture.status) + } + + private fun thenSessionIsErrored(scope: Scope) { + val sessionAfterCapture = scope.withSession { }!! + assertTrue(sessionAfterCapture.errorCount() > 0) + assertEquals(Session.State.Ok, sessionAfterCapture.status) + } + + private fun thenSessionIsCrashed(scope: Scope) { + val sessionAfterCapture = scope.withSession { }!! + assertTrue(sessionAfterCapture.errorCount() > 0) + assertEquals(Session.State.Crashed, sessionAfterCapture.status) + } + + private fun createScope(options: SentryOptions = SentryOptions()): Scope { + return Scope(options).apply { addBreadcrumb( Breadcrumb().apply { message = "message" @@ -1242,6 +1548,10 @@ class SentryClientTest { return listOf(exception) } + private fun createHandledException(): List { + return listOf(SentryException()) + } + private fun getEventFromData(data: ByteArray): SentryEvent { val inputStream = InputStreamReader(ByteArrayInputStream(data)) return fixture.sentryOptions.serializer.deserialize(inputStream, SentryEvent::class.java)!! @@ -1303,3 +1613,17 @@ class SentryClientTest { } } } + +class DropEverythingEventProcessor : EventProcessor { + + override fun process(event: SentryEvent, hint: Any?): SentryEvent? { + return null + } + + override fun process( + transaction: SentryTransaction, + hint: Any? + ): SentryTransaction? { + return null + } +} From 346de300783a5af8ddc5c9386c55ccb78cafb356 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 10 May 2022 09:41:52 +0200 Subject: [PATCH 2/2] Add changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bd99d0f290..a798967ab2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Fixes + +* Change order of event filtering mechanisms and only send session update for dropped events if session state changed (#2028) + ## 5.7.3 * Fix: Sentry Timber integration throws an exception when using args (#1986)