From b45b682b61206ac68c48c6be9749abcdcf11cb0c Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Thu, 17 Nov 2022 16:11:08 +0100 Subject: [PATCH] fix: Don't increase session's error count for dropped events (#2374) --- CHANGELOG.md | 6 ++ Sources/Sentry/SentryClient.m | 25 +++++-- Sources/Sentry/SentryHub.m | 13 ++-- Sources/Sentry/include/SentryClient+Private.h | 8 +-- .../Session/SentrySessionTrackerTests.swift | 10 +-- Tests/SentryTests/SentryClientTests.swift | 69 +++++++++++++------ Tests/SentryTests/SentryHubTests.swift | 46 +++++++++++-- Tests/SentryTests/TestClient.swift | 15 ++-- 8 files changed, 137 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c927e510d3..2f0ad6b784c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Don't update session for dropped events (#2374) + ## 7.31.1 ### Fixes diff --git a/Sources/Sentry/SentryClient.m b/Sources/Sentry/SentryClient.m index 3da3f62dc2e..4eecbf78456 100644 --- a/Sources/Sentry/SentryClient.m +++ b/Sources/Sentry/SentryClient.m @@ -184,12 +184,18 @@ - (SentryId *)captureException:(NSException *)exception withScope:(SentryScope * } - (SentryId *)captureException:(NSException *)exception - withSession:(SentrySession *)session withScope:(SentryScope *)scope + incrementSessionErrors:(SentrySession * (^)(void))sessionBlock { SentryEvent *event = [self buildExceptionEvent:exception]; event = [self prepareEvent:event withScope:scope alwaysAttachStacktrace:YES]; - return [self sendEvent:event withSession:session withScope:scope]; + + if (event != nil) { + SentrySession *session = sessionBlock(); + return [self sendEvent:event withSession:session withScope:scope]; + } + + return SentryId.empty; } - (SentryEvent *)buildExceptionEvent:(NSException *)exception @@ -215,12 +221,18 @@ - (SentryId *)captureError:(NSError *)error withScope:(SentryScope *)scope } - (SentryId *)captureError:(NSError *)error - withSession:(SentrySession *)session withScope:(SentryScope *)scope + incrementSessionErrors:(SentrySession * (^)(void))sessionBlock { SentryEvent *event = [self buildErrorEvent:error]; event = [self prepareEvent:event withScope:scope alwaysAttachStacktrace:YES]; - return [self sendEvent:event withSession:session withScope:scope]; + + if (event != nil) { + SentrySession *session = sessionBlock(); + return [self sendEvent:event withSession:session withScope:scope]; + } + + return SentryId.empty; } - (SentryEvent *)buildErrorEvent:(NSError *)error @@ -399,10 +411,9 @@ - (SentryId *)sendEvent:(SentryEvent *)event [self.transportAdapter sendEvent:event session:session attachments:attachments]; return event.eventId; - } else { - [self captureSession:session]; - return SentryId.empty; } + + return SentryId.empty; } - (void)captureSession:(SentrySession *)session diff --git a/Sources/Sentry/SentryHub.m b/Sources/Sentry/SentryHub.m index 1c3b7699fa7..6448a99eed2 100644 --- a/Sources/Sentry/SentryHub.m +++ b/Sources/Sentry/SentryHub.m @@ -438,11 +438,13 @@ - (SentryId *)captureError:(NSError *)error - (SentryId *)captureError:(NSError *)error withScope:(SentryScope *)scope { - SentrySession *currentSession = [self incrementSessionErrors]; + SentrySession *currentSession = _session; SentryClient *client = _client; if (nil != client) { if (nil != currentSession) { - return [client captureError:error withSession:currentSession withScope:scope]; + return [client captureError:error + withScope:scope + incrementSessionErrors:^(void) { return [self incrementSessionErrors]; }]; } else { return [client captureError:error withScope:scope]; } @@ -457,12 +459,13 @@ - (SentryId *)captureException:(NSException *)exception - (SentryId *)captureException:(NSException *)exception withScope:(SentryScope *)scope { - SentrySession *currentSession = [self incrementSessionErrors]; - + SentrySession *currentSession = _session; SentryClient *client = _client; if (nil != client) { if (nil != currentSession) { - return [client captureException:exception withSession:currentSession withScope:scope]; + return [client captureException:exception + withScope:scope + incrementSessionErrors:^(void) { return [self incrementSessionErrors]; }]; } else { return [client captureException:exception withScope:scope]; } diff --git a/Sources/Sentry/include/SentryClient+Private.h b/Sources/Sentry/include/SentryClient+Private.h index dc71e7e3c6c..016cefe9d21 100644 --- a/Sources/Sentry/include/SentryClient+Private.h +++ b/Sources/Sentry/include/SentryClient+Private.h @@ -26,12 +26,12 @@ SentryClient (Private) - (SentryFileManager *)fileManager; - (SentryId *)captureError:(NSError *)error - withSession:(SentrySession *)session - withScope:(SentryScope *)scope; + withScope:(SentryScope *)scope + incrementSessionErrors:(SentrySession * (^)(void))sessionBlock; - (SentryId *)captureException:(NSException *)exception - withSession:(SentrySession *)session - withScope:(SentryScope *)scope; + withScope:(SentryScope *)scope + incrementSessionErrors:(SentrySession * (^)(void))sessionBlock; - (SentryId *)captureCrashEvent:(SentryEvent *)event withScope:(SentryScope *)scope; diff --git a/Tests/SentryTests/Integrations/Session/SentrySessionTrackerTests.swift b/Tests/SentryTests/Integrations/Session/SentrySessionTrackerTests.swift index c84a0fd93ea..d9b96575462 100644 --- a/Tests/SentryTests/Integrations/Session/SentrySessionTrackerTests.swift +++ b/Tests/SentryTests/Integrations/Session/SentrySessionTrackerTests.swift @@ -217,16 +217,16 @@ class SentrySessionTrackerTests: XCTestCase { let startTime = fixture.currentDateProvider.date() sut.start() goToForeground() - + advanceTime(bySeconds: 1) captureError() advanceTime(bySeconds: 1) - + goToBackground() captureError() advanceTime(bySeconds: 10) goToForeground() - + assertEndSessionSent(started: startTime, duration: 2, errors: 2) } @@ -509,10 +509,10 @@ class SentrySessionTrackerTests: XCTestCase { var sessions = fixture.client.captureSessionInvocations.invocations + eventWithSessions + errorWithSessions + exceptionWithSessions - sessions.sort { first, second in return first.started < second.started } + sessions.sort { first, second in return first!.started < second!.started } if let session = sessions.last { - XCTAssertFalse(session.flagInit?.boolValue ?? false) + XCTAssertFalse(session?.flagInit?.boolValue ?? false) } } diff --git a/Tests/SentryTests/SentryClientTests.swift b/Tests/SentryTests/SentryClientTests.swift index a9acfa07fed..1a353a3697a 100644 --- a/Tests/SentryTests/SentryClientTests.swift +++ b/Tests/SentryTests/SentryClientTests.swift @@ -269,29 +269,31 @@ class SentryClientTest: XCTestCase { sut.add(processor) sut.capture(event: event) - let sendedAttachments = fixture.transportAdapter.sendEventWithTraceStateInvocations.first?.attachments ?? [] + let sentAttachments = fixture.transportAdapter.sendEventWithTraceStateInvocations.first?.attachments ?? [] wait(for: [expectProcessorCall], timeout: 1) - XCTAssertEqual(sendedAttachments.count, 1) - XCTAssertEqual(extraAttachment, sendedAttachments.first) + XCTAssertEqual(sentAttachments.count, 1) + XCTAssertEqual(extraAttachment, sentAttachments.first) } func test_AttachmentProcessor_CaptureError_WithSession() { let sut = fixture.getSut() let error = NSError(domain: "test", code: -1) let extraAttachment = Attachment(data: Data(), filename: "ExtraAttachment") - + let processor = TestAttachmentProcessor { atts, _ in var result = atts ?? [] result.append(extraAttachment) return result } - + sut.add(processor) - sut.captureError(error, with: fixture.session, with: Scope()) - + sut.captureError(error, with: Scope()) { + self.fixture.session + } + let sentAttachments = fixture.transportAdapter.sentEventsWithSessionTraceState.first?.attachments ?? [] - + XCTAssertEqual(sentAttachments.count, 1) XCTAssertEqual(extraAttachment, sentAttachments.first) } @@ -308,12 +310,14 @@ class SentryClientTest: XCTestCase { } sut.add(processor) - sut.captureError(error, with: SentrySession(releaseName: ""), with: Scope()) + sut.captureError(error, with: Scope()) { + return SentrySession(releaseName: "") + } - let sendedAttachments = fixture.transportAdapter.sendEventWithTraceStateInvocations.first?.attachments ?? [] + let sentAttachments = fixture.transportAdapter.sendEventWithTraceStateInvocations.first?.attachments ?? [] - XCTAssertEqual(sendedAttachments.count, 1) - XCTAssertEqual(extraAttachment, sendedAttachments.first) + XCTAssertEqual(sentAttachments.count, 1) + XCTAssertEqual(extraAttachment, sentAttachments.first) } func testCaptureEventWithDsnSetAfterwards() { @@ -445,8 +449,13 @@ class SentryClientTest: XCTestCase { } func testCaptureErrorWithSession() { - let eventId = fixture.getSut().captureError(error, with: fixture.session, with: Scope()) - + let sessionBlockExpectation = expectation(description: "session block gets called") + let eventId = fixture.getSut().captureError(error, with: Scope()) { + sessionBlockExpectation.fulfill() + return self.fixture.session + } + wait(for: [sessionBlockExpectation], timeout: 0.2) + eventId.assertIsNotEmpty() XCTAssertNotNil(fixture.transportAdapter.sentEventsWithSessionTraceState.last) if let eventWithSessionArguments = fixture.transportAdapter.sentEventsWithSessionTraceState.last { @@ -456,9 +465,17 @@ class SentryClientTest: XCTestCase { } func testCaptureErrorWithSession_WithBeforeSendReturnsNil() { + let sessionBlockExpectation = expectation(description: "session block does not get called") + sessionBlockExpectation.isInverted = true + let eventId = fixture.getSut(configureOptions: { options in options.beforeSend = { _ in return nil } - }).captureError(error, with: fixture.session, with: Scope()) + }).captureError(error, with: Scope()) { + // This should NOT be called + sessionBlockExpectation.fulfill() + return self.fixture.session + } + wait(for: [sessionBlockExpectation], timeout: 0.2) eventId.assertIsEmpty() assertLastSentEnvelopeIsASession() @@ -680,8 +697,10 @@ class SentryClientTest: XCTestCase { } func testCaptureExceptionWithSession() { - let eventId = fixture.getSut().capture(exception, with: fixture.session, with: fixture.scope) - + let eventId = fixture.getSut().capture(exception, with: fixture.scope) { + self.fixture.session + } + eventId.assertIsNotEmpty() XCTAssertNotNil(fixture.transportAdapter.sentEventsWithSessionTraceState.last) if let eventWithSessionArguments = fixture.transportAdapter.sentEventsWithSessionTraceState.last { @@ -694,7 +713,9 @@ class SentryClientTest: XCTestCase { func testCaptureExceptionWithSession_WithBeforeSendReturnsNil() { let eventId = fixture.getSut(configureOptions: { options in options.beforeSend = { _ in return nil } - }).capture(exception, with: fixture.session, with: fixture.scope) + }).capture(exception, with: fixture.scope) { + self.fixture.session + } eventId.assertIsEmpty() assertLastSentEnvelopeIsASession() @@ -731,7 +752,9 @@ class SentryClientTest: XCTestCase { let session = SentrySession(releaseName: "") fixture.getSut().capture(session: session) - fixture.getSut().capture(exception, with: session, with: Scope()) + fixture.getSut().capture(exception, with: Scope()) { + session + } .assertIsNotEmpty() fixture.getSut().captureCrash(fixture.event, with: session, with: Scope()) .assertIsNotEmpty() @@ -850,7 +873,9 @@ class SentryClientTest: XCTestCase { _ = SentryEnvelope(event: Event()) let eventId = fixture.getSut(configureOptions: { options in options.dsn = nil - }).capture(self.exception, with: fixture.session, with: fixture.scope) + }).capture(self.exception, with: fixture.scope) { + self.fixture.session + } eventId.assertIsEmpty() assertNothingSent() @@ -860,7 +885,9 @@ class SentryClientTest: XCTestCase { _ = SentryEnvelope(event: Event()) let eventId = fixture.getSut(configureOptions: { options in options.dsn = nil - }).captureError(self.error, with: fixture.session, with: fixture.scope) + }).captureError(self.error, with: fixture.scope) { + self.fixture.session + } eventId.assertIsEmpty() assertNothingSent() diff --git a/Tests/SentryTests/SentryHubTests.swift b/Tests/SentryTests/SentryHubTests.swift index c06bde20805..859b2d678ee 100644 --- a/Tests/SentryTests/SentryHubTests.swift +++ b/Tests/SentryTests/SentryHubTests.swift @@ -383,8 +383,8 @@ class SentryHubTests: XCTestCase { if let errorArguments = fixture.client.captureErrorWithSessionInvocations.first { XCTAssertEqual(fixture.error, errorArguments.error as NSError) - XCTAssertEqual(1, errorArguments.session.errors) - XCTAssertEqual(SentrySessionStatus.ok, errorArguments.session.status) + XCTAssertEqual(1, errorArguments.session?.errors) + XCTAssertEqual(SentrySessionStatus.ok, errorArguments.session?.status) XCTAssertEqual(fixture.scope, errorArguments.scope) } @@ -392,6 +392,23 @@ class SentryHubTests: XCTestCase { // only session init is sent XCTAssertEqual(1, fixture.client.captureSessionInvocations.count) } + + func testCaptureWithoutIncreasingErrorCount() { + let sut = fixture.getSut() + sut.startSession() + fixture.client.callSessionBlockWithIncrementSessionErrors = false + sut.capture(error: fixture.error, scope: fixture.scope).assertIsNotEmpty() + + XCTAssertEqual(1, fixture.client.captureErrorWithSessionInvocations.count) + if let errorArguments = fixture.client.captureErrorWithSessionInvocations.first { + XCTAssertEqual(fixture.error, errorArguments.error as NSError) + XCTAssertNil(errorArguments.session) + XCTAssertEqual(fixture.scope, errorArguments.scope) + } + + // only session init is sent + XCTAssertEqual(1, fixture.client.captureSessionInvocations.count) + } func testCaptureErrorWithoutScope() { fixture.getSut(fixture.options, fixture.scope).capture(error: fixture.error).assertIsNotEmpty() @@ -432,8 +449,8 @@ class SentryHubTests: XCTestCase { if let exceptionArguments = fixture.client.captureExceptionWithSessionInvocations.first { XCTAssertEqual(fixture.exception, exceptionArguments.exception) - XCTAssertEqual(1, exceptionArguments.session.errors) - XCTAssertEqual(SentrySessionStatus.ok, exceptionArguments.session.status) + XCTAssertEqual(1, exceptionArguments.session?.errors) + XCTAssertEqual(SentrySessionStatus.ok, exceptionArguments.session?.status) XCTAssertEqual(fixture.scope, exceptionArguments.scope) } @@ -441,6 +458,23 @@ class SentryHubTests: XCTestCase { // only session init is sent XCTAssertEqual(1, fixture.client.captureSessionInvocations.count) } + + func testCaptureExceptionWithoutIncreasingErrorCount() { + let sut = fixture.getSut() + sut.startSession() + fixture.client.callSessionBlockWithIncrementSessionErrors = false + sut.capture(exception: fixture.exception, scope: fixture.scope).assertIsNotEmpty() + + XCTAssertEqual(1, fixture.client.captureExceptionWithSessionInvocations.count) + if let exceptionArguments = fixture.client.captureExceptionWithSessionInvocations.first { + XCTAssertEqual(fixture.exception, exceptionArguments.exception) + XCTAssertNil(exceptionArguments.session) + XCTAssertEqual(fixture.scope, exceptionArguments.scope) + } + + // only session init is sent + XCTAssertEqual(1, fixture.client.captureSessionInvocations.count) + } @available(tvOS 10.0, *) @available(OSX 10.12, *) @@ -456,7 +490,7 @@ class SentryHubTests: XCTestCase { for i in 1...captureCount { // The session error count must not be in order as we use a concurrent DispatchQueue XCTAssertTrue( - invocations.contains { $0.session.errors == i }, + invocations.contains { $0.session!.errors == i }, "No session captured with \(i) amount of errors." ) } @@ -476,7 +510,7 @@ class SentryHubTests: XCTestCase { for i in 1..() - override func captureError(_ error: Error, with session: SentrySession, with scope: Scope) -> SentryId { - captureErrorWithSessionInvocations.record((error, session, scope)) + + var callSessionBlockWithIncrementSessionErrors = true + var captureErrorWithSessionInvocations = Invocations<(error: Error, session: SentrySession?, scope: Scope)>() + override func captureError(_ error: Error, with scope: Scope, incrementSessionErrors sessionBlock: @escaping () -> SentrySession) -> SentryId { + captureErrorWithSessionInvocations.record((error, callSessionBlockWithIncrementSessionErrors ? sessionBlock() : nil, scope)) return SentryId() } - var captureExceptionWithSessionInvocations = Invocations<(exception: NSException, session: SentrySession, scope: Scope)>() - override func capture(_ exception: NSException, with session: SentrySession, with scope: Scope) -> SentryId { - captureExceptionWithSessionInvocations.record((exception, session, scope)) + var captureExceptionWithSessionInvocations = Invocations<(exception: NSException, session: SentrySession?, scope: Scope)>() + override func capture(_ exception: NSException, with scope: Scope, incrementSessionErrors sessionBlock: @escaping () -> SentrySession) -> SentryId { + captureExceptionWithSessionInvocations.record((exception, callSessionBlockWithIncrementSessionErrors ? sessionBlock() : nil, scope)) return SentryId() }