From 69d88a24c95ce927695bf108e6ec3db9e53c2ea1 Mon Sep 17 00:00:00 2001 From: Dhiogo Brustolin Date: Wed, 16 Oct 2024 16:31:26 +0200 Subject: [PATCH 1/6] Fix: Dont create transaction for unused ViewControllers --- Sources/Sentry/SentryTracer.m | 14 +++++++++++++- .../SentryUIViewControllerPerformanceTracker.m | 6 ++++++ Sources/Sentry/include/SentryTracer.h | 7 +++++++ .../Transaction/SentryTracerTests.swift | 9 +++++++++ 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m index 427590ceb4..61f1ca4299 100644 --- a/Sources/Sentry/SentryTracer.m +++ b/Sources/Sentry/SentryTracer.m @@ -96,6 +96,7 @@ @implementation SentryTracer { dispatch_block_t _idleTimeoutBlock; NSMutableArray> *_children; BOOL _startTimeChanged; + BOOL _timeout; NSObject *_idleTimeoutLock; #if SENTRY_HAS_UIKIT @@ -148,6 +149,8 @@ - (instancetype)initWithTransactionContext:(SentryTransactionContext *)transacti self.wasFinishCalled = NO; _measurements = [[NSMutableDictionary alloc] init]; self.finishStatus = kSentrySpanStatusUndefined; + self.finishMustBeCalled = NO; + _timeout = YES; if (_configuration.timerFactory == nil) { _configuration.timerFactory = [[SentryNSTimerFactory alloc] init]; @@ -289,6 +292,8 @@ - (void)startDeadlineTimer - (void)deadlineTimerFired { SENTRY_LOG_DEBUG(@"Sentry tracer deadline fired"); + _timeout = YES; + @synchronized(self) { // This try to minimize a race condition with a proper call to `finishInternal`. if (self.isFinished) { @@ -303,7 +308,8 @@ - (void)deadlineTimerFired } } - [self finishWithStatus:kSentrySpanStatusDeadlineExceeded]; + _finishStatus = kSentrySpanStatusDeadlineExceeded; + [self finishInternal]; } - (void)cancelDeadlineTimer @@ -581,6 +587,12 @@ - (void)finishInternal } }]; + if (self.finishMustBeCalled && !self.wasFinishCalled) { + SENTRY_LOG_DEBUG( + @"Not capturing transaction because finish was not called before timing out."); + return; + } + @synchronized(_children) { if (_configuration.idleTimeout > 0.0 && _children.count == 0) { SENTRY_LOG_DEBUG(@"Was waiting for timeout for UI event trace but it had no children, " diff --git a/Sources/Sentry/SentryUIViewControllerPerformanceTracker.m b/Sources/Sentry/SentryUIViewControllerPerformanceTracker.m index c7d051e15b..c8eeb1aa24 100644 --- a/Sources/Sentry/SentryUIViewControllerPerformanceTracker.m +++ b/Sources/Sentry/SentryUIViewControllerPerformanceTracker.m @@ -125,6 +125,12 @@ - (void)createTransaction:(UIViewController *)controller SENTRY_LOG_DEBUG(@"Started span with id %@ to track view controller %@.", spanId.sentrySpanIdString, name); + id vcSpan = [self.tracker getSpan:spanId]; + if ([vcSpan isKindOfClass:SentryTracer.class]) { + SentryTracer *vcTracer = (SentryTracer *)vcSpan; + vcTracer.finishMustBeCalled = YES; + } + // Use the target itself to store the spanId to avoid using a global mapper. objc_setAssociatedObject(controller, &SENTRY_UI_PERFORMANCE_TRACKER_SPAN_ID, spanId, OBJC_ASSOCIATION_RETAIN_NONATOMIC); diff --git a/Sources/Sentry/include/SentryTracer.h b/Sources/Sentry/include/SentryTracer.h index 33dfd76ccb..d2e40a4d72 100644 --- a/Sources/Sentry/include/SentryTracer.h +++ b/Sources/Sentry/include/SentryTracer.h @@ -43,6 +43,13 @@ static const NSTimeInterval SENTRY_AUTO_TRANSACTION_MAX_DURATION = 500.0; @property (nullable, nonatomic, copy) BOOL (^shouldIgnoreWaitForChildrenCallback)(id); +/** + * This flag indicates whether the trace should be captured when the timeout triggers. + * If Yes, this tracer will be discarced in case the timeout triggers. + * Default @c NO + */ +@property (nonatomic) BOOL finishMustBeCalled; + /** * All the spans that where created with this tracer but rootSpan. */ diff --git a/Tests/SentryTests/Transaction/SentryTracerTests.swift b/Tests/SentryTests/Transaction/SentryTracerTests.swift index 105c47712f..44f3808805 100644 --- a/Tests/SentryTests/Transaction/SentryTracerTests.swift +++ b/Tests/SentryTests/Transaction/SentryTracerTests.swift @@ -1292,6 +1292,15 @@ class SentryTracerTests: XCTestCase { } #endif + func testFinishShouldBeCalled_Timeout_NotCaptured() { + fixture.dispatchQueue.blockBeforeMainBlock = { true } + + let sut = fixture.getSut() + sut.finishMustBeCalled = true + fixture.timerFactory.fire() + assertTransactionNotCaptured(sut) + } + @available(*, deprecated) func testSetExtra_ForwardsToSetData() { let sut = fixture.getSut() From d03c4f9f77248b404448f9eed158a086ee234567 Mon Sep 17 00:00:00 2001 From: Dhiogo Brustolin Date: Wed, 16 Oct 2024 16:36:21 +0200 Subject: [PATCH 2/6] Update SentryUIViewControllerPerformanceTrackerTests.swift --- .../SentryUIViewControllerPerformanceTrackerTests.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerPerformanceTrackerTests.swift b/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerPerformanceTrackerTests.swift index abae8db883..641b5d3e2d 100644 --- a/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerPerformanceTrackerTests.swift +++ b/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerPerformanceTrackerTests.swift @@ -136,6 +136,7 @@ class SentryUIViewControllerPerformanceTrackerTests: XCTestCase { XCTAssertEqual(tracer.transactionContext.nameSource, .component) XCTAssertEqual(tracer.transactionContext.origin, origin) XCTAssertFalse(tracer.isFinished) + XCTAssertTrue(tracer.finishMustBeCalled) sut.viewControllerViewDidLoad(viewController) { if let blockSpan = self.getStack(tracker).last { From 7780ec8f16b9af2028539363f01cda89e255be59 Mon Sep 17 00:00:00 2001 From: Dhiogo Brustolin Date: Wed, 16 Oct 2024 16:37:27 +0200 Subject: [PATCH 3/6] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69022a5853..e90b4ffba9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ via the option `swizzleClassNameExclude`. - Swizzling RootUIViewController if ignored by `swizzleClassNameExclude` (#4407) - Data race in SentrySwizzleInfo.originalCalled (#4434) - Thread running at user-initiated quality-of-service for session replay (#4439) - +- Do not create transaction for unused ViewControllers (#4448) ### Improvements From 6dad4b9d1c2dec3c7b1501ed8409f3c120895c0a Mon Sep 17 00:00:00 2001 From: Dhiogo Brustolin Date: Thu, 17 Oct 2024 14:29:33 +0200 Subject: [PATCH 4/6] Apply suggestions from code review Co-authored-by: Philipp Hofmann --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e90b4ffba9..789d7861ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ via the option `swizzleClassNameExclude`. - Swizzling RootUIViewController if ignored by `swizzleClassNameExclude` (#4407) - Data race in SentrySwizzleInfo.originalCalled (#4434) - Thread running at user-initiated quality-of-service for session replay (#4439) -- Do not create transaction for unused ViewControllers (#4448) +- Don't create transactions for unused UIViewControllers (#4448) ### Improvements From bad0bf496d5f2791eb3b0b13f65f5fb42d043621 Mon Sep 17 00:00:00 2001 From: Dhiogo Brustolin Date: Thu, 17 Oct 2024 14:49:07 +0200 Subject: [PATCH 5/6] ref --- Sources/Sentry/SentryPerformanceTracker.m | 2 ++ Sources/Sentry/SentryTracer.m | 7 +------ Sources/Sentry/SentryTracerConfiguration.m | 1 + Sources/Sentry/SentryUIViewControllerPerformanceTracker.m | 6 ------ Sources/Sentry/include/SentryTracer.h | 7 ------- Sources/Sentry/include/SentryTracerConfiguration.h | 7 +++++++ .../SentryUIViewControllerPerformanceTrackerTests.swift | 4 +++- Tests/SentryTests/Transaction/SentryTracerTests.swift | 6 +++--- 8 files changed, 17 insertions(+), 23 deletions(-) diff --git a/Sources/Sentry/SentryPerformanceTracker.m b/Sources/Sentry/SentryPerformanceTracker.m index ed24bf2538..f0a0908ada 100644 --- a/Sources/Sentry/SentryPerformanceTracker.m +++ b/Sources/Sentry/SentryPerformanceTracker.m @@ -7,6 +7,7 @@ #import "SentrySpanId.h" #import "SentrySpanProtocol.h" #import "SentryTracer.h" +#import "SentryTracerConfiguration.h" #import "SentryTransactionContext+Private.h" #if SENTRY_HAS_UIKIT @@ -86,6 +87,7 @@ - (SentrySpanId *)startSpanWithName:(NSString *)name configuration:[SentryTracerConfiguration configurationWithBlock:^( SentryTracerConfiguration *configuration) { configuration.waitForChildren = YES; + configuration.finishMustBeCalled = YES; }]]; [(SentryTracer *)newSpan setDelegate:self]; diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m index 61f1ca4299..f5d41be75f 100644 --- a/Sources/Sentry/SentryTracer.m +++ b/Sources/Sentry/SentryTracer.m @@ -96,7 +96,6 @@ @implementation SentryTracer { dispatch_block_t _idleTimeoutBlock; NSMutableArray> *_children; BOOL _startTimeChanged; - BOOL _timeout; NSObject *_idleTimeoutLock; #if SENTRY_HAS_UIKIT @@ -149,8 +148,6 @@ - (instancetype)initWithTransactionContext:(SentryTransactionContext *)transacti self.wasFinishCalled = NO; _measurements = [[NSMutableDictionary alloc] init]; self.finishStatus = kSentrySpanStatusUndefined; - self.finishMustBeCalled = NO; - _timeout = YES; if (_configuration.timerFactory == nil) { _configuration.timerFactory = [[SentryNSTimerFactory alloc] init]; @@ -292,8 +289,6 @@ - (void)startDeadlineTimer - (void)deadlineTimerFired { SENTRY_LOG_DEBUG(@"Sentry tracer deadline fired"); - _timeout = YES; - @synchronized(self) { // This try to minimize a race condition with a proper call to `finishInternal`. if (self.isFinished) { @@ -587,7 +582,7 @@ - (void)finishInternal } }]; - if (self.finishMustBeCalled && !self.wasFinishCalled) { + if (self.configuration.finishMustBeCalled && !self.wasFinishCalled) { SENTRY_LOG_DEBUG( @"Not capturing transaction because finish was not called before timing out."); return; diff --git a/Sources/Sentry/SentryTracerConfiguration.m b/Sources/Sentry/SentryTracerConfiguration.m index c14ec504f4..5fbb920fbd 100644 --- a/Sources/Sentry/SentryTracerConfiguration.m +++ b/Sources/Sentry/SentryTracerConfiguration.m @@ -21,6 +21,7 @@ - (instancetype)init if (self = [super init]) { self.idleTimeout = 0; self.waitForChildren = NO; + self.finishMustBeCalled = NO; } return self; } diff --git a/Sources/Sentry/SentryUIViewControllerPerformanceTracker.m b/Sources/Sentry/SentryUIViewControllerPerformanceTracker.m index c8eeb1aa24..c7d051e15b 100644 --- a/Sources/Sentry/SentryUIViewControllerPerformanceTracker.m +++ b/Sources/Sentry/SentryUIViewControllerPerformanceTracker.m @@ -125,12 +125,6 @@ - (void)createTransaction:(UIViewController *)controller SENTRY_LOG_DEBUG(@"Started span with id %@ to track view controller %@.", spanId.sentrySpanIdString, name); - id vcSpan = [self.tracker getSpan:spanId]; - if ([vcSpan isKindOfClass:SentryTracer.class]) { - SentryTracer *vcTracer = (SentryTracer *)vcSpan; - vcTracer.finishMustBeCalled = YES; - } - // Use the target itself to store the spanId to avoid using a global mapper. objc_setAssociatedObject(controller, &SENTRY_UI_PERFORMANCE_TRACKER_SPAN_ID, spanId, OBJC_ASSOCIATION_RETAIN_NONATOMIC); diff --git a/Sources/Sentry/include/SentryTracer.h b/Sources/Sentry/include/SentryTracer.h index d2e40a4d72..33dfd76ccb 100644 --- a/Sources/Sentry/include/SentryTracer.h +++ b/Sources/Sentry/include/SentryTracer.h @@ -43,13 +43,6 @@ static const NSTimeInterval SENTRY_AUTO_TRANSACTION_MAX_DURATION = 500.0; @property (nullable, nonatomic, copy) BOOL (^shouldIgnoreWaitForChildrenCallback)(id); -/** - * This flag indicates whether the trace should be captured when the timeout triggers. - * If Yes, this tracer will be discarced in case the timeout triggers. - * Default @c NO - */ -@property (nonatomic) BOOL finishMustBeCalled; - /** * All the spans that where created with this tracer but rootSpan. */ diff --git a/Sources/Sentry/include/SentryTracerConfiguration.h b/Sources/Sentry/include/SentryTracerConfiguration.h index 9fdb9c228c..76bc217337 100644 --- a/Sources/Sentry/include/SentryTracerConfiguration.h +++ b/Sources/Sentry/include/SentryTracerConfiguration.h @@ -28,6 +28,13 @@ NS_ASSUME_NONNULL_BEGIN */ @property (nonatomic) BOOL waitForChildren; +/** + * This flag indicates whether the trace should be captured when the timeout triggers. + * If Yes, this tracer will be discarced in case the timeout triggers. + * Default @c NO + */ +@property (nonatomic) BOOL finishMustBeCalled; + #if SENTRY_TARGET_PROFILING_SUPPORTED /** * Whether to sample a profile corresponding to this transaction diff --git a/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerPerformanceTrackerTests.swift b/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerPerformanceTrackerTests.swift index 641b5d3e2d..5408af4e39 100644 --- a/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerPerformanceTrackerTests.swift +++ b/Tests/SentryTests/Integrations/Performance/UIViewController/SentryUIViewControllerPerformanceTrackerTests.swift @@ -136,7 +136,9 @@ class SentryUIViewControllerPerformanceTrackerTests: XCTestCase { XCTAssertEqual(tracer.transactionContext.nameSource, .component) XCTAssertEqual(tracer.transactionContext.origin, origin) XCTAssertFalse(tracer.isFinished) - XCTAssertTrue(tracer.finishMustBeCalled) + + let config = try XCTUnwrap(Dynamic(tracer).configuration.asObject as? SentryTracerConfiguration) + XCTAssertTrue(config.finishMustBeCalled) sut.viewControllerViewDidLoad(viewController) { if let blockSpan = self.getStack(tracker).last { diff --git a/Tests/SentryTests/Transaction/SentryTracerTests.swift b/Tests/SentryTests/Transaction/SentryTracerTests.swift index 44f3808805..c4bba33464 100644 --- a/Tests/SentryTests/Transaction/SentryTracerTests.swift +++ b/Tests/SentryTests/Transaction/SentryTracerTests.swift @@ -95,7 +95,7 @@ class SentryTracerTests: XCTestCase { } #endif // os(iOS) || os(tvOS) || targetEnvironment(macCatalyst) - func getSut(waitForChildren: Bool = true, idleTimeout: TimeInterval = 0.0) -> SentryTracer { + func getSut(waitForChildren: Bool = true, idleTimeout: TimeInterval = 0.0, finishMustBeCalled: Bool = false) -> SentryTracer { let tracer = hub.startTransaction( with: transactionContext, bindToScope: false, @@ -104,6 +104,7 @@ class SentryTracerTests: XCTestCase { $0.waitForChildren = waitForChildren $0.timerFactory = self.timerFactory $0.idleTimeout = idleTimeout + $0.finishMustBeCalled = finishMustBeCalled })) return tracer } @@ -1295,8 +1296,7 @@ class SentryTracerTests: XCTestCase { func testFinishShouldBeCalled_Timeout_NotCaptured() { fixture.dispatchQueue.blockBeforeMainBlock = { true } - let sut = fixture.getSut() - sut.finishMustBeCalled = true + let sut = fixture.getSut(finishMustBeCalled: true) fixture.timerFactory.fire() assertTransactionNotCaptured(sut) } From 3260d344e87982dabcd18f6a01b16f5e6942bfb1 Mon Sep 17 00:00:00 2001 From: Dhiogo Brustolin Date: Thu, 17 Oct 2024 14:49:42 +0200 Subject: [PATCH 6/6] Update SentryPerformanceTracker.m --- Sources/Sentry/SentryPerformanceTracker.m | 1 - 1 file changed, 1 deletion(-) diff --git a/Sources/Sentry/SentryPerformanceTracker.m b/Sources/Sentry/SentryPerformanceTracker.m index f0a0908ada..cf1a3e98c0 100644 --- a/Sources/Sentry/SentryPerformanceTracker.m +++ b/Sources/Sentry/SentryPerformanceTracker.m @@ -7,7 +7,6 @@ #import "SentrySpanId.h" #import "SentrySpanProtocol.h" #import "SentryTracer.h" -#import "SentryTracerConfiguration.h" #import "SentryTransactionContext+Private.h" #if SENTRY_HAS_UIKIT