From e91f3979280062ecf38433ac910237f36d84bf76 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 3 May 2022 12:07:54 +0200 Subject: [PATCH] Only send session update for dropped events if state changed (#2002) * Change order of filtering mechanisms and add early return * Only send session update for dropped events if state changed * Extract variable for throwable * Add changelog * Add changelog * Rename method * Rename things * Make var final * Apply suggestions from code review Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> * Update sentry/src/main/java/io/sentry/SentryClient.java Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> * Add tests to verify order, session updates and sending * Add debug log for dropped events ... ... where the session update is not sent because it does not change the health of the sesssion * Fix merge mistake Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> --- CHANGELOG.md | 1 + .../src/main/java/io/sentry/SentryClient.java | 48 ++- .../test/java/io/sentry/SentryClientTest.kt | 342 +++++++++++++++++- 3 files changed, 385 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd7ff496ff..6c63fe7673 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fix * Change order of event filtering mechanisms (#2001) +* Only send session update for dropped events if state changed (#2002) ## 6.0.0-beta.2 diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 7b094f6d37..269574ca64 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -126,7 +126,10 @@ private boolean shouldApplyScopeData( return SentryId.EMPTY_ID; } - Session session = null; + @Nullable + Session sessionBeforeUpdate = + scope != null ? scope.withSession((@Nullable Session session) -> {}) : null; + @Nullable Session session = null; if (event != null) { session = updateSessionData(event, hint, scope); @@ -146,6 +149,18 @@ private boolean shouldApplyScopeData( } } + final boolean shouldSendSessionUpdate = + shouldSendSessionUpdateForDroppedEvent(sessionBeforeUpdate, session); + + 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; if (event != null && event.getEventId() != null) { sentryId = event.getEventId(); @@ -156,8 +171,9 @@ private boolean shouldApplyScopeData( scope != null && scope.getTransaction() != null ? scope.getTransaction().traceState() : null; - final SentryEnvelope envelope = - buildEnvelope(event, getAttachments(scope, hint), session, traceState, null); + final boolean shouldSendAttachments = event != null; + List attachments = shouldSendAttachments ? getAttachments(scope, hint) : null; + final SentryEnvelope envelope = buildEnvelope(event, attachments, session, traceState, null); if (envelope != null) { transport.send(envelope, hint); @@ -172,6 +188,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 getAttachments( final @Nullable Scope scope, final @NotNull Map hint) { List attachments = null; diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 59b4e78832..853f4de4b7 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -2,11 +2,15 @@ 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.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 @@ -83,7 +87,10 @@ class SentryClientTest { var profilingTraceData = ProfilingTraceData(profilingTraceFile, sentryTracer) var profilingNonExistingTraceData = ProfilingTraceData(File("non_existent.trace"), sentryTracer) - fun getSut() = SentryClient(sentryOptions) + fun getSut(optionsCallback: ((SentryOptions) -> Unit)? = null): SentryClient { + optionsCallback?.invoke(sentryOptions) + return SentryClient(sentryOptions) + } } private val fixture = Fixture() @@ -1306,6 +1313,331 @@ class SentryClientTest { ) } + @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) + } + + @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()) + + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf(DiscardedEvent(DiscardReason.EVENT_PROCESSOR.reason, DataCategory.Error.category, 1)) + ) + } + + @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()) + + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf(DiscardedEvent(DiscardReason.SAMPLE_RATE.reason, DataCategory.Error.category, 1)) + ) + } + + @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()) + + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf(DiscardedEvent(DiscardReason.BEFORE_SEND.reason, DataCategory.Error.category, 1)) + ) + } + + @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()) + + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf(DiscardedEvent(DiscardReason.EVENT_PROCESSOR.reason, DataCategory.Error.category, 1)) + ) + } + + @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()) + + assertClientReport( + fixture.sentryOptions.clientReportRecorder, + listOf(DiscardedEvent(DiscardReason.EVENT_PROCESSOR.reason, DataCategory.Error.category, 1)) + ) + } + + 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) + } + class CustomBeforeSendCallback : SentryOptions.BeforeSendCallback { override fun execute(event: SentryEvent, hint: MutableMap?): SentryEvent? { hint?.remove(SENTRY_SCREENSHOT) @@ -1314,8 +1646,8 @@ class SentryClientTest { } } - private fun createScope(): Scope { - return Scope(SentryOptions()).apply { + private fun createScope(options: SentryOptions = SentryOptions()): Scope { + return Scope(options).apply { addBreadcrumb( Breadcrumb().apply { message = "message" @@ -1399,6 +1731,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)!!