fix: Don't increase session's error count for dropped events (#2374)

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Fixes
+
+- Don't update session for dropped events (#2374)
+
 ## 7.31.1
 
 ### Fixes

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

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];
         }

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;
 

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)
         }
     }
 

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()

Tests/SentryTests/SentryHubTests.swift

@@ -383,15 +383,32 @@ 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)
         }
 
         // 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,15 +449,32 @@ 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)
         }
 
         // 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..<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."
             )
         }