feat: Include unfinished spans in transactions (#1592)

Instead of removing unfinished spans from the transaction, we close all unfinished spans with deadline_exceeded status. Fixes GH-1303 Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
getsentry · Dec 29, 2021 · 229c663 · 229c663
1 parent 6c4db1d
commit 229c663
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## Unreleased
 
+- feat: Include unfinished spans in transactions (#1592)
 - build: Disable NSAssertions for Release Builds (#1545)
 
 ## 7.7.0

diff --git a/Sources/Sentry/SentrySpan.m b/Sources/Sentry/SentrySpan.m
@@ -99,16 +99,16 @@ - (BOOL)isFinished
 
 - (void)finish
 {
-    self.timestamp = [SentryCurrentDate date];
-    if (self.transaction != nil) {
-        [self.transaction spanFinished:self];
-    }
+    [self finishWithStatus:kSentrySpanStatusOk];
 }
 
 - (void)finishWithStatus:(SentrySpanStatus)status
 {
     self.context.status = status;
-    [self finish];
+    self.timestamp = [SentryCurrentDate date];
+    if (self.transaction != nil) {
+        [self.transaction spanFinished:self];
+    }
 }
 
 - (SentryTraceHeader *)toTraceHeader

diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m
@@ -251,6 +251,12 @@ - (BOOL)hasUnfinishedChildren
 
 - (void)canBeFinished
 {
+    // Transaction already finished and captured.
+    // Sending another transaction and spans with
+    // the same SentryId would be an error.
+    if (self.rootSpan.isFinished)
+        return;
+
     if (!self.isWaitingForChildren || (_waitForChildren && [self hasUnfinishedChildren]))
         return;
 
@@ -263,6 +269,18 @@ - (void)captureTransaction
     if (_hub == nil)
         return;
 
+    @synchronized(_children) {
+        for (id<SentrySpan> span in _children) {
+            if (!span.isFinished) {
+                [span finishWithStatus:kSentrySpanStatusDeadlineExceeded];
+
+                // Unfinished children should have the same
+                // end timestamp as their parent transaction
+                span.timestamp = self.timestamp;
+            }
+        }
+    }
+
     [_hub.scope useSpan:^(id<SentrySpan> _Nullable span) {
         if (span == self) {
             [self->_hub.scope setSpan:nil];
@@ -280,15 +298,8 @@ - (SentryTransaction *)toTransaction
 
     NSArray<id<SentrySpan>> *spans;
     @synchronized(_children) {
-
         [_children addObjectsFromArray:appStartSpans];
-
-        spans = [_children
-            filteredArrayUsingPredicate:[NSPredicate
-                                            predicateWithBlock:^BOOL(id<SentrySpan> _Nullable span,
-                                                NSDictionary<NSString *, id> *_Nullable bindings) {
-                                                return span.isFinished;
-                                            }]];
+        spans = [_children copy];
     }
 
     if (appStartMeasurement != nil) {

diff --git a/Tests/SentryTests/Performance/SentryTracerTests.swift b/Tests/SentryTests/Performance/SentryTracerTests.swift
@@ -235,6 +235,28 @@ class SentryTracerTests: XCTestCase {
         assertAppStartMeasurementNotPutOnTransaction()
     }
 
+    func testFinish_WithUnfinishedChildren() {
+        CurrentDate.setCurrentDateProvider(DefaultCurrentDateProvider.sharedInstance())
+        let sut = fixture.getSut(waitForChildren: false)
+        let child1 = sut.startChild(operation: fixture.transactionOperation)
+        let child2 = sut.startChild(operation: fixture.transactionOperation)
+        let child3 = sut.startChild(operation: fixture.transactionOperation)
+        child2.finish()
+        sut.finish()
+
+        XCTAssertTrue(child1.isFinished)
+        XCTAssertEqual(child1.context.status, .deadlineExceeded)
+        XCTAssertEqual(sut.timestamp, child1.timestamp)
+
+        XCTAssertTrue(child2.isFinished)
+        XCTAssertEqual(child2.context.status, .ok)
+        XCTAssertNotEqual(sut.timestamp, child2.timestamp)
+
+        XCTAssertTrue(child3.isFinished)
+        XCTAssertEqual(child3.context.status, .deadlineExceeded)
+        XCTAssertEqual(sut.timestamp, child3.timestamp)
+    }
+
     // Although we only run this test above the below specified versions, we expect the
     // implementation to be thread safe
     @available(tvOS 10.0, *)

diff --git a/Tests/SentryTests/Transaction/SentrySpanTests.swift b/Tests/SentryTests/Transaction/SentrySpanTests.swift
@@ -55,6 +55,7 @@ class SentrySpanTests: XCTestCase {
         XCTAssertEqual(span.startTimestamp, TestData.timestamp)
         XCTAssertEqual(span.timestamp, TestData.timestamp)
         XCTAssertTrue(span.isFinished)
+        XCTAssertEqual(span.context.status, .ok)
 
         let lastEvent = client.captureEventWithScopeInvocations.invocations[0].event
         XCTAssertEqual(lastEvent.transaction, fixture.someTransaction)
@@ -65,11 +66,11 @@ class SentrySpanTests: XCTestCase {
 
     func testFinishWithStatus() {
         let span = fixture.getSut()
-        span.finish(status: .ok)
+        span.finish(status: .cancelled)
 
         XCTAssertEqual(span.startTimestamp, TestData.timestamp)
         XCTAssertEqual(span.timestamp, TestData.timestamp)
-        XCTAssertEqual(span.context.status, .ok)
+        XCTAssertEqual(span.context.status, .cancelled)
         XCTAssertTrue(span.isFinished)
     }
 
@@ -91,20 +92,6 @@ class SentrySpanTests: XCTestCase {
         XCTAssertEqual(serializedChild["parent_span_id"] as? String, span.context.spanId.sentrySpanIdString)
     }
 
-    func testFinishWithUnfinishedSpanDropsSpan() {
-        let client = TestClient(options: fixture.options)!
-        let span = fixture.getSut(client: client)
-        span.startChild(operation: fixture.someOperation)
-
-        span.finish()
-        let lastEvent = client.captureEventWithScopeInvocations.invocations[0].event
-        let serializedData = lastEvent.serialize()
-
-        let spans = serializedData["spans"] as! [Any]
-
-        XCTAssertEqual(spans.count, 0)
-    }
-
     func testStartChildWithNameOperation() {
         let span = fixture.getSut()