Fix: Dont create transaction for unused ViewControllers

[brustolin]

📜 Description

Its possible to create a view controller but never add it to the view hierarchy.
This will create a transaction with data that is not helpful

💚 How did you test it?

Unit tests

📝 Checklist

You have to check all boxes before merging:

• I reviewed the submitted code.
• I added tests to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1218.71 ms	1238.00 ms	19.29 ms
Size	21.58 KiB	707.59 KiB	686.01 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
b2c9166	1246.86 ms	1255.28 ms	8.42 ms
02e1163	1199.86 ms	1211.78 ms	11.92 ms
ee8b48f	1196.80 ms	1213.48 ms	16.68 ms
533c68f	1236.54 ms	1256.68 ms	20.14 ms
10ee2ce	1250.90 ms	1258.57 ms	7.67 ms
1f9387b	1229.00 ms	1256.49 ms	27.49 ms
bef2003	1248.18 ms	1258.86 ms	10.68 ms
6943de0	1237.67 ms	1247.12 ms	9.45 ms
326b7eb	1204.76 ms	1234.96 ms	30.20 ms
50b058e	1212.29 ms	1231.16 ms	18.88 ms

App size

Revision	Plain	With Sentry	Diff
b2c9166	21.58 KiB	630.26 KiB	608.68 KiB
02e1163	21.58 KiB	418.82 KiB	397.24 KiB
ee8b48f	21.58 KiB	418.70 KiB	397.11 KiB
533c68f	21.58 KiB	630.28 KiB	608.70 KiB
10ee2ce	20.76 KiB	427.77 KiB	407.00 KiB
1f9387b	21.58 KiB	654.26 KiB	632.68 KiB
bef2003	22.85 KiB	407.73 KiB	384.88 KiB
6943de0	20.76 KiB	393.33 KiB	372.57 KiB
326b7eb	20.76 KiB	432.31 KiB	411.55 KiB
50b058e	21.58 KiB	714.30 KiB	692.72 KiB

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.344%. Comparing base (f273312) to head (e2cba58).
Report is 11 commits behind head on main.

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #4437       +/-   ##
=============================================
+ Coverage   91.333%   91.344%   +0.010%     
=============================================
  Files          610       610               
  Lines        49927     49954       +27     
  Branches     18033     18056       +23     
=============================================
+ Hits         45600     45630       +30     
+ Misses        4235      4232        -3     
  Partials        92        92               

Files with missing lines	Coverage Δ
Sources/Sentry/SentrySpan.m	98.709% <100.000%> (+0.008%)	⬆️
Sources/Sentry/SentryTracer.m	97.202% <100.000%> (+0.053%)	⬆️
.../Sentry/SentryUIViewControllerPerformanceTracker.m	99.099% <100.000%> (+0.012%)	⬆️
...entryUIViewControllerPerformanceTrackerTests.swift	98.409% <100.000%> (+0.005%)	⬆️
...ts/SentryTests/Transaction/SentryTracerTests.swift	98.407% <100.000%> (+0.023%)	⬆️

... and 4 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f273312...e2cba58. Read the comment docs.

[philipphofmann]

I'm unsure if this approach will work for nested UIViewControllers. I'm not a big fan of the shouldIgnore flag approach. The logic for covering the edge case of unused view controllers is spread across multiple classes. Flipping some flags on the spans looks like something that can break easily.

What about a transaction validation/span filtering logic? We could have a method looking all the spans with some metadata and validating that it is a correct transaction. In our case, the logic checks if it's an auto-generated transaction with a ui.load operation. It then validates that the transaction has a loadView or a viewDidLoad spans, and also spans for viewWillDisappear or viewDidAppear. The logic would discard the transaction or filter spans for nested view controllers that never get loaded.
The max transaction duration logic should also be in that validation logic

sentry-cocoa/Sources/Sentry/SentryTracer.m

Lines 560 to 570 in 79eb67d

	// Prewarming can execute code up to viewDidLoad of a UIViewController, and keep the app in the
	// background. This can lead to auto-generated transactions lasting for minutes or even hours.
	// Therefore, we drop transactions lasting longer than SENTRY_AUTO_TRANSACTION_MAX_DURATION.
	NSTimeInterval transactionDuration = [self.timestamp timeIntervalSinceDate:self.startTimestamp];
	if ([self isAutoGeneratedTransaction]
	&& transactionDuration >= SENTRY_AUTO_TRANSACTION_MAX_DURATION) {
	SENTRY_LOG_INFO(@"Auto generated transaction exceeded the max duration of %f seconds. Not "
	@"capturing transaction.",
	SENTRY_AUTO_TRANSACTION_MAX_DURATION);
	return;
	}

[philipphofmann]

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.

[brustolin]

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

[philipphofmann]

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?

[brustolin]

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

[armcknight]

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

[philipphofmann]

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

[brustolin]

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.

[philipphofmann]

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.

[brustolin]

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

[brustolin]

I'm not a big fan of the shouldIgnore flag approach. The logic for covering the edge case of unused view controllers is spread across multiple classes

I think you’re looking at this from the wrong perspective. Don’t think about shouldIgnore as a solution for unused view controller transactions. Instead, consider it a broader solution: a flag indicating that a particular span was not ‘approved’ by the mechanism that created it.

[philipphofmann]

The SentryUIViewControllerPerformanceTracker calls finish on the tracer when the UIViewController is added to the view hierarchy / appears on the screen. Only when this happens can we keep the transaction. So, instead of putting a flag on the span, @brustolin and I discussed adding a new config option to the tracer. This flag will be called something finishMustBeCalled. When this flag is set to true, the transaction must be finished before the deadline timer fires. Otherwise, the tracer discards the transaction.

[brustolin]

Since you're using a different approach I prefer to start from scratch.