Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Dont create transaction for unused ViewControllers #4437

Closed
wants to merge 11 commits into from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ via the option `swizzleClassNameExclude`.
- Add TTID/TTFD spans when loadView gets skipped (#4415)
- Finish TTID correctly when viewWillAppear is skipped (#4417)
- Swizzling RootUIViewController if ignored by `swizzleClassNameExclude` (#4407)
- Do not create transaction for unused view controllers (#4437)
- Data race in SentrySwizzleInfo.originalCalled (#4434)

### Improvements
Expand Down
2 changes: 1 addition & 1 deletion Sources/Sentry/SentrySpan.m
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ - (instancetype)initWithContext:(SentrySpanContext *)context
_spanId = context.spanId;
_sampled = context.sampled;
_origin = context.origin;

_shouldIgnore = NO;
#if SENTRY_TARGET_PROFILING_SUPPORTED
_isContinuousProfiling = [SentrySDK.options isContinuousProfilingEnabled];
if (_isContinuousProfiling) {
Expand Down
27 changes: 20 additions & 7 deletions Sources/Sentry/SentryTracer.m
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ @implementation SentryTracer {
#endif // SENTRY_HAS_UIKIT
NSMutableDictionary<NSString *, SentryMeasurementValue *> *_measurements;
dispatch_block_t _idleTimeoutBlock;
NSMutableArray<id<SentrySpan>> *_children;
NSMutableArray<SentrySpan *> *_children;
BOOL _startTimeChanged;
NSObject *_idleTimeoutLock;

Expand Down Expand Up @@ -506,12 +506,12 @@ - (BOOL)hasUnfinishedChildSpansToWaitFor
}

@synchronized(_children) {
for (id<SentrySpan> span in _children) {
for (SentrySpan *span in _children) {
if (self.shouldIgnoreWaitForChildrenCallback != nil
&& self.shouldIgnoreWaitForChildrenCallback(span)) {
continue;
}
if (![span isFinished])
if (!span.shouldIgnore && ![span isFinished])
return YES;
}
return NO;
Expand Down Expand Up @@ -604,6 +604,9 @@ - (void)finishInternal
}

SentryTransaction *transaction = [self toTransaction];
if (transaction == nil) {
return;
}

#if SENTRY_TARGET_PROFILING_SUPPORTED
if (self.isProfiling) {
Expand Down Expand Up @@ -638,8 +641,9 @@ - (void)trimEndTimestamp
NSDate *oldest = self.startTimestamp;

@synchronized(_children) {
for (id<SentrySpan> childSpan in _children) {
if ([oldest compare:childSpan.timestamp] == NSOrderedAscending) {
for (SentrySpan *childSpan in _children) {
if (!childSpan.shouldIgnore &&
[oldest compare:childSpan.timestamp] == NSOrderedAscending) {
oldest = childSpan.timestamp;
}
}
Expand All @@ -656,8 +660,13 @@ - (void)updateStartTime:(NSDate *)startTime
_startTimeChanged = YES;
}

- (SentryTransaction *)toTransaction
- (nullable SentryTransaction *)toTransaction
{
if (self.shouldIgnore) {
// If this transaction was marked as "not used" we drop it.
SENTRY_LOG_WARN(@"Transaction '%@' was not used.", self.transactionContext.name);
return nil;
}

NSUInteger capacity;
#if SENTRY_HAS_UIKIT
Expand All @@ -672,7 +681,11 @@ - (SentryTransaction *)toTransaction
NSMutableArray<id<SentrySpan>> *spans = [[NSMutableArray alloc] initWithCapacity:capacity];

@synchronized(_children) {
[spans addObjectsFromArray:_children];
for (SentrySpan *child in _children) {
if (child.shouldIgnore == NO) {
[spans addObject:child];
}
Comment on lines +685 to +687
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h: If I'm not mistaken, this approach won't work for nested UIViewControllers, as this would only remove the spans for loadView and viewDidLoad and not the other spans, such as TTID/TTFD.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TTID and TTFD are not created for nested view controllers.
Im not sure if I understood your point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, TTID/TTFD was a bad example then. What about other spans, such as file IO, DB queries, manually added ones, or other future spans that we aren't yet aware of?

Copy link
Contributor Author

@brustolin brustolin Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can see where this could happen.
What do you think we should do to them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation idea mentioned in the top-level comment sounds promising 👍🏻

Copy link
Member

@philipphofmann philipphofmann Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brustolin, I would suggest a validation logic as described here #4437 (review). WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need to talk about this in person, maybe I didn't get it but your suggestion in the PR review comment does not solve the problem raised in here.

Also, I would not like to have a "scenario specific" logic inside the generic SentryTracer class, it feels more hacky than the one flag solution that indicates whether the span was validated by its creator or not.

}
}

#if SENTRY_HAS_UIKIT
Expand Down
7 changes: 5 additions & 2 deletions Sources/Sentry/SentryUIViewControllerPerformanceTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ - (void)createTransaction:(UIViewController *)controller
SENTRY_LOG_DEBUG(@"Started span with id %@ to track view controller %@.",
spanId.sentrySpanIdString, name);

SentrySpan *span = [self.tracker getSpan:spanId];
span.shouldIgnore = 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);
Expand Down Expand Up @@ -264,13 +267,13 @@ - (void)finishTransaction:(UIViewController *)controller
};

[self.tracker activateSpan:spanId duringBlock:duringBlock];
id<SentrySpan> vcSpan = [self.tracker getSpan:spanId];
SentrySpan *vcSpan = [self.tracker getSpan:spanId];
// If the current controller span has no parent,
// it means it is the root transaction and need to be pop from the queue.
if (vcSpan.parentSpanId == nil) {
[self.tracker popActiveSpan];
}

vcSpan.shouldIgnore = NO;
// If we are still tracking this UIViewController finish the transaction
// and remove associated span id.
[self.tracker finishSpan:spanId withStatus:status];
Expand Down
8 changes: 8 additions & 0 deletions Sources/Sentry/include/SentrySpan.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,14 @@ SENTRY_NO_INIT
*/
@property (nullable, nonatomic, strong) NSArray<SentryFrame *> *frames;

/**
* Indicates whether this span should be ignored when finishing a transaction.
* Ignored spans are not included in the transaction and not taken in account
* when trimming transaction timeout.
* Default value is @c NO
*/
@property (nonatomic) BOOL shouldIgnore;

/**
* Init a @c SentrySpan with given transaction and context.
* @param transaction The @c SentryTracer managing the transaction this span is associated with.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ class SentryUIViewControllerPerformanceTrackerTests: XCTestCase {
callbackExpectation.fulfill()
}
let tracer = try XCTUnwrap(transactionSpan as? SentryTracer)
XCTAssertTrue(tracer.shouldIgnore)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: It seems to me that these test additions don't fully cover the edge case described in GH-4427. I would love to see a test that emulates these edge cases.

Copy link
Contributor Author

@brustolin brustolin Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This addition are meant to check whether SentryUIViewControllerPerformanceTrackerTests.swift is properly setting this values.

The test to check whether we create or not the transaction can be found in SentryTracerTests.swift

XCTAssertEqual(tracer.transactionContext.name, fixture.viewControllerName)
XCTAssertEqual(tracer.transactionContext.nameSource, .component)
XCTAssertEqual(tracer.transactionContext.origin, origin)
Expand Down Expand Up @@ -193,6 +194,7 @@ class SentryUIViewControllerPerformanceTrackerTests: XCTestCase {

lifecycleEndingMethod(sut, viewController, tracker, callbackExpectation, tracer)

XCTAssertFalse(tracer.shouldIgnore)
XCTAssertEqual(Dynamic(transactionSpan).children.asArray!.count, 8)
XCTAssertTrue(tracer.isFinished)
XCTAssertEqual(finishStatus.rawValue, tracer.status.rawValue)
Expand Down
18 changes: 18 additions & 0 deletions Tests/SentryTests/Transaction/SentryTracerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1211,6 +1211,24 @@ class SentryTracerTests: XCTestCase {
}
}

func testRemoveChildWithShouldIgnoreTrue() throws {
let sut = fixture.getSut()
let child = try XCTUnwrap(sut.startChild(operation: fixture.transactionOperation) as? SentrySpan)
child.shouldIgnore = true

sut.finish()
let transaction = try XCTUnwrap(fixture.hub.capturedTransactionsWithScope.first?.transaction as? [String: Any])
let spans = try XCTUnwrap(transaction["spans"]! as? [[String: Any]])
XCTAssertEqual(spans.count, 0)
}

func testDontCaptureWithShouldIgnoreTrue() throws {
let sut = fixture.getSut()
sut.shouldIgnore = true
sut.finish()
XCTAssertEqual(fixture.hub.capturedTransactionsWithScope.count, 0)
}

#if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)

func testChangeStartTimeStamp_OnlyFramesDelayAdded() throws {
Expand Down
Loading