From 38b36f5735cfc98c4645f4c06a30a3442a858dbd Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Mon, 29 Apr 2024 11:06:48 +0200 Subject: [PATCH] impr: Capture transactions on a bg thread (#3892) Instead of capturing and serializing the transaction on the calling thread, the SDK now does this on a background thread. Fixes GH-3826 --- CHANGELOG.md | 4 +++ .../TraceTestViewController.swift | 8 ++--- Sources/Sentry/SentryHub.m | 30 ++++++++++++------- Sources/Sentry/include/SentryHub+Private.h | 8 ++--- .../Session/SentrySessionGeneratorTests.swift | 2 +- .../Session/SentrySessionTrackerTests.swift | 2 +- ...WatchdogTerminationsIntegrationTests.swift | 2 +- ...ntryWatchdogTerminationsTrackerTests.swift | 2 +- Tests/SentryTests/SentryHubTests.swift | 20 ++++++++++--- .../SentrySDKIntegrationTestsBase.swift | 2 +- Tests/SentryTests/SentrySDKTests.swift | 2 +- Tests/SentryTests/State/SentryHub+Test.h | 4 ++- Tests/SentryTests/State/TestHub.swift | 4 +-- .../Transaction/SentrySpanTests.swift | 2 +- 14 files changed, 58 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4eff4fe564..8b6ba8dcb46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ - Don't transmit device boot time in envelopes enriched with crash data (#3912) +### Improvements + +- Capture transactions on a background thread (#3892) + ## 8.25.0-alpha.0 ### Features diff --git a/Samples/iOS-Swift/iOS-Swift/ViewControllers/TraceTestViewController.swift b/Samples/iOS-Swift/iOS-Swift/ViewControllers/TraceTestViewController.swift index fc549d3e995..1aabf5974e2 100644 --- a/Samples/iOS-Swift/iOS-Swift/ViewControllers/TraceTestViewController.swift +++ b/Samples/iOS-Swift/iOS-Swift/ViewControllers/TraceTestViewController.swift @@ -68,15 +68,11 @@ class TraceTestViewController: UIViewController { return } - guard let child = children.first(where: { $0.operation == "http.client" }) else { - UIAssert.fail("Did not found http request child") + guard let child = children.first(where: { $0.operation == "http.client" && $0.data["url"] as? String == "https://sentry-brand.storage.googleapis.com/sentry-logo-black.png" && $0.data["http.response.status_code"] as? String == "200" }) else { + UIAssert.fail("Did not find child span for HTTP for retrieving the sentry brand logo.") return } - UIAssert.isEqual(child.data["url"] as? String, "https://sentry-brand.storage.googleapis.com/sentry-logo-black.png", "Could not read url data value") - - UIAssert.isEqual(child.data["http.response.status_code"] as? String, "200", "Could not read status_code tag value") - UIAssert.checkForViewControllerLifeCycle(span, viewController: "TraceTestViewController", stepsToCheck: self.lifeCycleSteps) } } diff --git a/Sources/Sentry/SentryHub.m b/Sources/Sentry/SentryHub.m index b2caf814c49..b1a0e032cdc 100644 --- a/Sources/Sentry/SentryHub.m +++ b/Sources/Sentry/SentryHub.m @@ -40,6 +40,7 @@ @property (nullable, nonatomic, strong) SentryClient *client; @property (nullable, nonatomic, strong) SentryScope *scope; +@property (nonatomic) SentryDispatchQueueWrapper *dispatchQueue; @property (nonatomic, strong) SentryCrashWrapper *crashWrapper; @property (nonatomic, strong) NSMutableSet *installedIntegrationNames; @property (nonatomic) NSUInteger errorsBeforeSession; @@ -57,6 +58,7 @@ - (instancetype)initWithClient:(nullable SentryClient *)client if (self = [super init]) { _client = client; _scope = scope; + _dispatchQueue = SentryDependencyContainer.sharedInstance.dispatchQueueWrapper; SentryStatsdClient *statsdClient = [[SentryStatsdClient alloc] initWithClient:client]; SentryMetricsClient *metricsClient = [[SentryMetricsClient alloc] initWithClient:statsdClient]; @@ -64,7 +66,7 @@ - (instancetype)initWithClient:(nullable SentryClient *)client initWithEnabled:client.options.enableMetrics client:metricsClient currentDate:SentryDependencyContainer.sharedInstance.dateProvider - dispatchQueue:SentryDependencyContainer.sharedInstance.dispatchQueueWrapper + dispatchQueue:_dispatchQueue random:SentryDependencyContainer.sharedInstance.random beforeEmitMetric:client.options.beforeEmitMetric]; [_metrics setDelegate:self]; @@ -85,9 +87,11 @@ - (instancetype)initWithClient:(nullable SentryClient *)client - (instancetype)initWithClient:(nullable SentryClient *)client andScope:(nullable SentryScope *)scope andCrashWrapper:(SentryCrashWrapper *)crashWrapper + andDispatchQueue:(SentryDispatchQueueWrapper *)dispatchQueue { self = [self initWithClient:client andScope:scope]; _crashWrapper = crashWrapper; + _dispatchQueue = dispatchQueue; return self; } @@ -268,25 +272,31 @@ - (void)captureCrashEvent:(SentryEvent *)event withScope:(SentryScope *)scope } } -- (SentryId *)captureTransaction:(SentryTransaction *)transaction withScope:(SentryScope *)scope +- (void)captureTransaction:(SentryTransaction *)transaction withScope:(SentryScope *)scope { - return [self captureTransaction:transaction withScope:scope additionalEnvelopeItems:@[]]; + [self captureTransaction:transaction withScope:scope additionalEnvelopeItems:@[]]; } -- (SentryId *)captureTransaction:(SentryTransaction *)transaction - withScope:(SentryScope *)scope - additionalEnvelopeItems:(NSArray *)additionalEnvelopeItems +- (void)captureTransaction:(SentryTransaction *)transaction + withScope:(SentryScope *)scope + additionalEnvelopeItems:(NSArray *)additionalEnvelopeItems { SentrySampleDecision decision = transaction.trace.sampled; if (decision != kSentrySampleDecisionYes) { [self.client recordLostEvent:kSentryDataCategoryTransaction reason:kSentryDiscardReasonSampleRate]; - return SentryId.empty; + return; } - return [self captureEvent:transaction - withScope:scope - additionalEnvelopeItems:additionalEnvelopeItems]; + // When a user calls finish on a transaction, which calls captureTransaction, the calling thread + // here could be the main thread, which we only want to block as long as required. Therefore, we + // capture the transaction on a background thread. + __weak SentryHub *weakSelf = self; + [self.dispatchQueue dispatchAsyncWithBlock:^{ + [weakSelf captureEvent:transaction + withScope:scope + additionalEnvelopeItems:additionalEnvelopeItems]; + }]; } - (SentryId *)captureEvent:(SentryEvent *)event diff --git a/Sources/Sentry/include/SentryHub+Private.h b/Sources/Sentry/include/SentryHub+Private.h index 9d81842c32c..f443ff3b13d 100644 --- a/Sources/Sentry/include/SentryHub+Private.h +++ b/Sources/Sentry/include/SentryHub+Private.h @@ -54,11 +54,11 @@ SentryHub () additionalEnvelopeItems:(NSArray *)additionalEnvelopeItems NS_SWIFT_NAME(capture(event:scope:additionalEnvelopeItems:)); -- (SentryId *)captureTransaction:(SentryTransaction *)transaction withScope:(SentryScope *)scope; +- (void)captureTransaction:(SentryTransaction *)transaction withScope:(SentryScope *)scope; -- (SentryId *)captureTransaction:(SentryTransaction *)transaction - withScope:(SentryScope *)scope - additionalEnvelopeItems:(NSArray *)additionalEnvelopeItems; +- (void)captureTransaction:(SentryTransaction *)transaction + withScope:(SentryScope *)scope + additionalEnvelopeItems:(NSArray *)additionalEnvelopeItems; - (void)captureEnvelope:(SentryEnvelope *)envelope; diff --git a/Tests/SentryTests/Integrations/Session/SentrySessionGeneratorTests.swift b/Tests/SentryTests/Integrations/Session/SentrySessionGeneratorTests.swift index 04c9fc7288f..d9e2d8bc297 100644 --- a/Tests/SentryTests/Integrations/Session/SentrySessionGeneratorTests.swift +++ b/Tests/SentryTests/Integrations/Session/SentrySessionGeneratorTests.swift @@ -142,7 +142,7 @@ class SentrySessionGeneratorTests: NotificationCenterTestCase { sentryCrash = TestSentryCrashWrapper.sharedInstance() let client = SentrySDK.currentHub().getClient() - let hub = SentryHub(client: client, andScope: nil, andCrashWrapper: self.sentryCrash) + let hub = SentryHub(client: client, andScope: nil, andCrashWrapper: self.sentryCrash, andDispatchQueue: SentryDispatchQueueWrapper()) SentrySDK.setCurrentHub(hub) crashIntegration = SentryCrashIntegration(crashAdapter: sentryCrash, andDispatchQueueWrapper: TestSentryDispatchQueueWrapper()) diff --git a/Tests/SentryTests/Integrations/Session/SentrySessionTrackerTests.swift b/Tests/SentryTests/Integrations/Session/SentrySessionTrackerTests.swift index 21fe8accef1..e8c55836252 100644 --- a/Tests/SentryTests/Integrations/Session/SentrySessionTrackerTests.swift +++ b/Tests/SentryTests/Integrations/Session/SentrySessionTrackerTests.swift @@ -34,7 +34,7 @@ class SentrySessionTrackerTests: XCTestCase { } func setNewHubToSDK() { - let hub = SentryHub(client: client, andScope: nil, andCrashWrapper: self.sentryCrash) + let hub = SentryHub(client: client, andScope: nil, andCrashWrapper: self.sentryCrash, andDispatchQueue: SentryDispatchQueueWrapper()) SentrySDK.setCurrentHub(hub) } } diff --git a/Tests/SentryTests/Integrations/WatchdogTerminations/SentryWatchdogTerminationsIntegrationTests.swift b/Tests/SentryTests/Integrations/WatchdogTerminations/SentryWatchdogTerminationsIntegrationTests.swift index 786cb130e96..4600de49051 100644 --- a/Tests/SentryTests/Integrations/WatchdogTerminations/SentryWatchdogTerminationsIntegrationTests.swift +++ b/Tests/SentryTests/Integrations/WatchdogTerminations/SentryWatchdogTerminationsIntegrationTests.swift @@ -22,7 +22,7 @@ class SentryWatchdogTerminationIntegrationTests: XCTestCase { SentryDependencyContainer.sharedInstance().crashWrapper = crashWrapper SentryDependencyContainer.sharedInstance().fileManager = try! SentryFileManager(options: options, dispatchQueueWrapper: TestSentryDispatchQueueWrapper()) - let hub = SentryHub(client: client, andScope: nil, andCrashWrapper: crashWrapper) + let hub = SentryHub(client: client, andScope: nil, andCrashWrapper: crashWrapper, andDispatchQueue: SentryDispatchQueueWrapper()) SentrySDK.setCurrentHub(hub) fileManager = try! SentryFileManager(options: options, dispatchQueueWrapper: dispatchQueue) diff --git a/Tests/SentryTests/Integrations/WatchdogTerminations/SentryWatchdogTerminationsTrackerTests.swift b/Tests/SentryTests/Integrations/WatchdogTerminations/SentryWatchdogTerminationsTrackerTests.swift index e122421e938..8ac5c8771c8 100644 --- a/Tests/SentryTests/Integrations/WatchdogTerminations/SentryWatchdogTerminationsTrackerTests.swift +++ b/Tests/SentryTests/Integrations/WatchdogTerminations/SentryWatchdogTerminationsTrackerTests.swift @@ -29,7 +29,7 @@ class SentryWatchdogTerminationTrackerTests: NotificationCenterTestCase { crashWrapper = TestSentryCrashWrapper.sharedInstance() - let hub = SentryHub(client: client, andScope: nil, andCrashWrapper: crashWrapper) + let hub = SentryHub(client: client, andScope: nil, andCrashWrapper: crashWrapper, andDispatchQueue: SentryDispatchQueueWrapper()) SentrySDK.setCurrentHub(hub) } diff --git a/Tests/SentryTests/SentryHubTests.swift b/Tests/SentryTests/SentryHubTests.swift index 18cf26b0931..04b59bacb96 100644 --- a/Tests/SentryTests/SentryHubTests.swift +++ b/Tests/SentryTests/SentryHubTests.swift @@ -26,6 +26,7 @@ class SentryHubTests: XCTestCase { let traceOrigin = "auto" let random = TestRandom(value: 0.5) let queue = DispatchQueue(label: "SentryHubTests", qos: .utility, attributes: [.concurrent]) + let dispatchQueueWrapper = TestSentryDispatchQueueWrapper() init() { options = Options() @@ -52,7 +53,7 @@ class SentryHubTests: XCTestCase { } func getSut(_ options: Options, _ scope: Scope? = nil) -> SentryHub { - let hub = SentryHub(client: client, andScope: scope, andCrashWrapper: sentryCrash) + let hub = SentryHub(client: client, andScope: scope, andCrashWrapper: sentryCrash, andDispatchQueue: self.dispatchQueueWrapper) hub.bindClient(client) return hub } @@ -354,12 +355,23 @@ class SentryHubTests: XCTestCase { XCTAssertEqual(span.sampled, .no) } - func testCaptureSampledTransaction_ReturnsEmptyId() { + func testCaptureTransaction_CapturesEventAsync() { + let transaction = sut.startTransaction(transactionContext: TransactionContext(name: fixture.transactionName, operation: fixture.transactionOperation, sampled: .yes)) + + let trans = Dynamic(transaction).toTransaction().asAnyObject + sut.capture(trans as! Transaction, with: Scope()) + + expect(self.fixture.client.captureEventWithScopeInvocations.count) == 1 + expect(self.fixture.dispatchQueueWrapper.dispatchAsyncInvocations.count) == 1 + } + + func testCaptureSampledTransaction_DoesNotCaptureEvent() { let transaction = sut.startTransaction(transactionContext: TransactionContext(name: fixture.transactionName, operation: fixture.transactionOperation, sampled: .no)) let trans = Dynamic(transaction).toTransaction().asAnyObject - let id = sut.capture(trans as! Transaction, with: Scope()) - id.assertIsEmpty() + sut.capture(trans as! Transaction, with: Scope()) + + expect(self.fixture.client.captureEventWithScopeInvocations.count) == 0 } func testCaptureSampledTransaction_RecordsLostEvent() { diff --git a/Tests/SentryTests/SentrySDKIntegrationTestsBase.swift b/Tests/SentryTests/SentrySDKIntegrationTestsBase.swift index cef6df91dc8..7a54a6c47db 100644 --- a/Tests/SentryTests/SentrySDKIntegrationTestsBase.swift +++ b/Tests/SentryTests/SentrySDKIntegrationTestsBase.swift @@ -26,7 +26,7 @@ class SentrySDKIntegrationTestsBase: XCTestCase { func givenSdkWithHub(_ options: Options? = nil, scope: Scope = Scope()) { let client = TestClient(options: options ?? self.options) - let hub = SentryHub(client: client, andScope: scope, andCrashWrapper: TestSentryCrashWrapper.sharedInstance()) + let hub = SentryHub(client: client, andScope: scope, andCrashWrapper: TestSentryCrashWrapper.sharedInstance(), andDispatchQueue: SentryDispatchQueueWrapper()) SentrySDK.setCurrentHub(hub) } diff --git a/Tests/SentryTests/SentrySDKTests.swift b/Tests/SentryTests/SentrySDKTests.swift index 50dec90fff0..ab48269348d 100644 --- a/Tests/SentryTests/SentrySDKTests.swift +++ b/Tests/SentryTests/SentrySDKTests.swift @@ -47,7 +47,7 @@ class SentrySDKTests: XCTestCase { options.releaseName = "1.0.0" client = TestClient(options: options)! - hub = SentryHub(client: client, andScope: scope, andCrashWrapper: TestSentryCrashWrapper.sharedInstance()) + hub = SentryHub(client: client, andScope: scope, andCrashWrapper: TestSentryCrashWrapper.sharedInstance(), andDispatchQueue: SentryDispatchQueueWrapper()) userFeedback = UserFeedback(eventId: SentryId()) userFeedback.comments = "Again really?" diff --git a/Tests/SentryTests/State/SentryHub+Test.h b/Tests/SentryTests/State/SentryHub+Test.h index 9fce2d64ca3..94f4f8a02a0 100644 --- a/Tests/SentryTests/State/SentryHub+Test.h +++ b/Tests/SentryTests/State/SentryHub+Test.h @@ -2,6 +2,7 @@ @class SentryClient; @class SentryCrashWrapper; +@class SentryDispatchQueueWrapper; @protocol SentryIntegrationProtocol; NS_ASSUME_NONNULL_BEGIN @@ -11,7 +12,8 @@ SentryHub () - (instancetype)initWithClient:(SentryClient *_Nullable)client andScope:(SentryScope *_Nullable)scope - andCrashWrapper:(SentryCrashWrapper *)crashAdapter; + andCrashWrapper:(SentryCrashWrapper *)crashAdapter + andDispatchQueue:(SentryDispatchQueueWrapper *)dispatchQueue; - (NSArray> *)installedIntegrations; - (NSSet *)installedIntegrationNames; diff --git a/Tests/SentryTests/State/TestHub.swift b/Tests/SentryTests/State/TestHub.swift index c66c452063f..56affc451c7 100644 --- a/Tests/SentryTests/State/TestHub.swift +++ b/Tests/SentryTests/State/TestHub.swift @@ -40,8 +40,8 @@ class TestHub: SentryHub { } var capturedTransactionsWithScope = Invocations<(transaction: [String: Any], scope: Scope)>() - override func capture(_ transaction: Transaction, with scope: Scope) -> SentryId { + override func capture(_ transaction: Transaction, with scope: Scope) { capturedTransactionsWithScope.record((transaction.serialize(), scope)) - return super.capture(transaction, with: scope) + super.capture(transaction, with: scope) } } diff --git a/Tests/SentryTests/Transaction/SentrySpanTests.swift b/Tests/SentryTests/Transaction/SentrySpanTests.swift index 59a3a4a575a..fa6a08e94ea 100644 --- a/Tests/SentryTests/Transaction/SentrySpanTests.swift +++ b/Tests/SentryTests/Transaction/SentrySpanTests.swift @@ -34,7 +34,7 @@ class SentrySpanTests: XCTestCase { } func getSut(client: SentryClient) -> Span { - let hub = SentryHub(client: client, andScope: nil, andCrashWrapper: TestSentryCrashWrapper.sharedInstance()) + let hub = SentryHub(client: client, andScope: nil, andCrashWrapper: TestSentryCrashWrapper.sharedInstance(), andDispatchQueue: TestSentryDispatchQueueWrapper()) return hub.startTransaction(name: someTransaction, operation: someOperation) }