feat: SwiftUI time for initial display and time for full display #4596

brustolin · 2024-12-04T11:44:33Z

📜 Description

Track time for initial display and time for full display in SwiftUI

💚 How did you test it?

UI Test and sample

📝 Checklist

You have to check all boxes before merging:

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.
I updated the wizard 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 · 2024-12-04T11:45:27Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against c95d3ec

…ry-cocoa into feat/swiftui-ttid

github-actions · 2024-12-04T11:58:29Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1242.41 ms	1259.24 ms	16.84 ms
Size	22.32 KiB	761.55 KiB	739.23 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`b9a9ffd`	1201.61 ms	1215.06 ms	13.45 ms
`282cc99`	1238.27 ms	1253.46 ms	15.19 ms
`984eb2d`	1220.62 ms	1235.24 ms	14.62 ms
`4350d44`	1228.75 ms	1246.75 ms	18.00 ms
`279841c`	1237.29 ms	1254.12 ms	16.83 ms
`e145ca1`	1207.19 ms	1230.67 ms	23.48 ms
`9b9f2a0`	1233.36 ms	1244.04 ms	10.68 ms
`7b022df`	1220.53 ms	1227.56 ms	7.03 ms
`aa225e2`	1218.00 ms	1236.45 ms	18.45 ms
`9f0d9e0`	1226.31 ms	1242.70 ms	16.40 ms

App size

Revision	Plain	With Sentry	Diff
`b9a9ffd`	21.58 KiB	418.43 KiB	396.85 KiB
`282cc99`	22.85 KiB	414.09 KiB	391.24 KiB
`984eb2d`	20.76 KiB	425.77 KiB	405.01 KiB
`4350d44`	21.58 KiB	629.82 KiB	608.24 KiB
`279841c`	22.84 KiB	403.19 KiB	380.34 KiB
`e145ca1`	21.58 KiB	669.70 KiB	648.12 KiB
`9b9f2a0`	21.58 KiB	707.42 KiB	685.84 KiB
`7b022df`	22.85 KiB	412.95 KiB	390.10 KiB
`aa225e2`	21.58 KiB	694.47 KiB	672.89 KiB
`9f0d9e0`	21.58 KiB	424.28 KiB	402.70 KiB

Previous results on branch: feat/swiftui-ttid

Startup times

Revision	Plain	With Sentry	Diff
`51e85b4`	1228.22 ms	1249.19 ms	20.96 ms
`f583fd1`	1243.27 ms	1258.91 ms	15.65 ms
`f38e16a`	1237.13 ms	1262.76 ms	25.62 ms
`1ea8b74`	1229.67 ms	1251.90 ms	22.22 ms
`7ce0058`	1230.49 ms	1246.94 ms	16.45 ms
`9ef5a81`	1234.29 ms	1256.35 ms	22.07 ms
`fccfa3c`	1232.65 ms	1254.22 ms	21.57 ms
`11c93a2`	1227.00 ms	1246.31 ms	19.31 ms
`895b829`	1232.55 ms	1250.90 ms	18.35 ms
`f6a9e2b`	1234.94 ms	1245.60 ms	10.66 ms

App size

Revision	Plain	With Sentry	Diff
`51e85b4`	22.31 KiB	756.73 KiB	734.42 KiB
`f583fd1`	22.30 KiB	750.46 KiB	728.16 KiB
`f38e16a`	22.30 KiB	750.46 KiB	728.16 KiB
`1ea8b74`	22.30 KiB	760.46 KiB	738.16 KiB
`7ce0058`	22.30 KiB	750.46 KiB	728.16 KiB
`9ef5a81`	22.30 KiB	750.46 KiB	728.16 KiB
`fccfa3c`	22.31 KiB	756.70 KiB	734.39 KiB
`11c93a2`	22.31 KiB	760.41 KiB	738.10 KiB
`895b829`	22.30 KiB	750.46 KiB	728.16 KiB
`f6a9e2b`	22.30 KiB	750.46 KiB	728.16 KiB

codecov · 2024-12-04T15:46:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.669%. Comparing base (0cb72fd) to head (c95d3ec).

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #4596       +/-   ##
=============================================
+ Coverage   90.584%   90.669%   +0.084%     
=============================================
  Files          621       618        -3     
  Lines        71120     71060       -60     
  Branches     25306     25300        -6     
=============================================
+ Hits         64424     64430        +6     
+ Misses        6602      6537       -65     
+ Partials        94        93        -1

Files with missing lines	Coverage Δ
SentryTestUtils/ClearTestState.swift	`86.363% <100.000%> (ø)`
Sources/Sentry/SentryHub.m	`99.065% <100.000%> (-0.007%)`	⬇️
...rces/Sentry/SentryPerformanceTrackingIntegration.m	`100.000% <100.000%> (ø)`
Sources/Sentry/SentryProfiler.mm	`87.000% <100.000%> (+1.000%)`	⬆️
Sources/Sentry/SentryTimeToDisplayTracker.m	`100.000% <100.000%> (ø)`
.../Sentry/SentryUIViewControllerPerformanceTracker.m	`99.317% <100.000%> (+0.050%)`	⬆️
...yProfilerTests/SentryAppLaunchProfilingTests.swift	`100.000% <100.000%> (ø)`
...ce/SentryPerformanceTrackingIntegrationTests.swift	`100.000% <100.000%> (ø)`
...iewController/SentryTimeToDisplayTrackerTest.swift	`100.000% <100.000%> (ø)`
...entryUIViewControllerPerformanceTrackerTests.swift	`99.012% <100.000%> (ø)`
... and 2 more

... and 11 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 0cb72fd...c95d3ec. Read the comment docs.

Samples/iOS-SwiftUI/iOS-SwiftUI-UITests/SentryRedactModifierTests.swift

Sources/Sentry/SentryTimeToDisplayTracker.m

Sources/Sentry/SentryHub.m

Sources/SentrySwiftUI/TracingView.swift

philipphofmann

I have one important comment to address that could impact a rework of this PR. Therefore, I wait until we resolve it before giving this a more detailed review.

Sources/Sentry/SentryUIViewControllerPerformanceTracker.m

Sources/SentrySwiftUI/SentryTracedView.swift

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

Sources/SentrySwiftUI/SentryTracedView.swift

Sources/Sentry/SentryTimeToDisplayTracker.m

philipphofmann · 2024-12-17T13:09:39Z

Sources/SentrySwiftUI/SentryTracedView.swift

+        if !viewAppeared {
+            trace = ensureTransactionExists()
+            spanId = createAndPushBodySpan(transactionCreated: trace != nil)
+#if canImport(SwiftUI) && canImport(UIKit) && os(iOS) || os(tvOS)
+            if let trace = trace { startTTDTraker(for: trace) }
+#endif


m: I would prefer keeping the logic for creating the transaction or span and keeping track of the TTDTracker in the SentryUIViewControllerPerformanceTracker. Now, we have two different places that can easily get out of sync.

This specific piece of logic is particular to the SwiftUI lifecycle. I don’t think it makes sense to move it to SentryUIViewControllerPerformanceTracker, but I did move other parts related to TTID to SentryUIViewControllerPerformanceTracker to reduce the responsibilities of this class.

philipphofmann · 2024-12-17T13:11:24Z

Sources/SentrySwiftUI/SentryTracedView.swift

+    private func startTTDTraker(for trace: SentryTracer) {
+        let tracker = SentryTimeToDisplayTracker(name: self.name, waitForFullDisplay: self.waitforFullDisplay)
+        SentryUIViewControllerPerformanceTracker.shared.setTimeToDisplay(tracker)
+        tracker.start(for: trace)
+
+        viewModel.tracker = tracker
+    }
+#endif
+
+    private func ensureTransactionExists() -> SentryTracer? {
+        guard SentryPerformanceTracker.shared.activeSpanId() == nil else { return nil }
+
+        let transactionId = SentryPerformanceTracker.shared.startSpan(
+            withName: name,
+            nameSource: nameSource,
+            operation: "ui.load",
+            origin: traceOrigin
+        )
+        SentryPerformanceTracker.shared.pushActiveSpan(transactionId)
+
+        //According to Apple's documentation, the call to body needs to be fast
+        //and can be made many times in one frame. Therefore they don't use async code to process the view.
+        //Scheduling to finish the transaction at the end of the main loop seems the least hack solution right now.
+        //'onAppear' is not a suitable place to do this because it may happen before other view body property get called.
+        DispatchQueue.main.async {
+            self.finishSpan(transactionId)
        }
+
+        return SentryPerformanceTracker.shared.getSpan(transactionId) as? SentryTracer
+    }
+
+    private func createAndPushBodySpan(transactionCreated: Bool) -> SpanId {
+        let spanName = transactionCreated ? "\(name) - body" : name
+        let spanId = SentryPerformanceTracker.shared.startSpan(
+            withName: spanName,
+            nameSource: nameSource,
+            operation: "ui.load",
+            origin: traceOrigin
+        )
+        SentryPerformanceTracker.shared.pushActiveSpan(spanId)
+        return spanId
+    }
+
+    private func finishSpan(_ spanId: SpanId) {
+        SentryPerformanceTracker.shared.popActiveSpan()
+        SentryPerformanceTracker.shared.finishSpan(spanId)


m: There is so much logic in this code in here. I would appreciate it if we could extract the logic into an extra testable component and/or write tests for all the different types of edge cases. Currently, I don't know how I can trust this code without having a good test suite covering multiple edge cases.

philipphofmann

m: I think we must add to the code docs of the option enableTimeToFullDisplayTracing that this also has an impact on the SentryTracedView.

Almost LGTM.

philipphofmann · 2024-12-19T14:40:58Z

Sources/SentrySwiftUI/SentryTracedView.swift

+    /// Creates a view that measures the performance of its `content`.
+    ///
+    /// - Parameter viewName: The name that will be used for the span, if nil we try to get the name of the content class.
+    /// - Parameter waitForFullDisplay: Indicates whether this view transaction should wait for `SentrySDK.reportFullyDisplayed()`


m: I think we must document how this works in combination with the enableTimeToFullDisplayTracing option.

philipphofmann · 2024-12-19T14:43:50Z

Sources/SentrySwiftUI/SentryTracedView.swift

-        self.nameSource = viewName == nil ? .component : .custom
+        let name = viewName ?? SentryTracedView.extractName(content: Content.self)
+        let nameSource = viewName == nil ? SentryTransactionNameSource.component : SentryTransactionNameSource.custom
+        let waitforFullDisplay = waitForFullDisplay ?? SentrySDK.options?.enableTimeToFullDisplayTracing ?? false


m: It would be great to add a test validating that we pick up the value from the options.

True. The tests related to TTID are missing. Going to add it

philipphofmann · 2024-12-19T14:45:06Z

Sources/SentrySwiftUI/SentryTracedView.swift

+
+        if let transactionId = transactionId {
+            self.finishSpan(transactionId)
+            SentryPerformanceTracker.shared.popActiveSpan()


m: Why do we also need to call popActiveSpan() here? We already call it in finishSpan.

We need to pop the root transaction, but not always. There is a bug in here 🤦🏻

scripts/xcode-test.sh

brustolin and others added 4 commits December 2, 2024 16:25

wip

a689d58

more changes

fd905da

Update CHANGELOG.md

e024c6c

Merge branch 'main' into feat/swiftui-ttid

ec167f7

getsentry-bot and others added 3 commits December 4, 2024 11:45

Format code

671956f

Update CHANGELOG.md

5c531b6

Merge branch 'feat/swiftui-ttid' of https://github.com/getsentry/sent…

6bedfa7

…ry-cocoa into feat/swiftui-ttid

brustolin and others added 7 commits December 4, 2024 13:15

fix for macOS

e6c6f18

fix tests

d6884c5

Update SentryTracedView.swift

ebdbc79

Format code

78e6b66

Update SentryTimeToDisplayTrackerTest.swift

04c6fdd

Format code

818aa34

Update SentrySDKTests.swift

6bbebc7

UITests

a970eb3

brustolin marked this pull request as ready for review December 5, 2024 08:57

brustolin requested review from philipphofmann, armcknight and philprime as code owners December 5, 2024 08:57

brustolin added 2 commits December 5, 2024 14:48

Merge branch 'main' into feat/swiftui-ttid

9f92dd8

Update CHANGELOG.md

d51b0ee

armcknight reviewed Dec 5, 2024

View reviewed changes

Samples/iOS-SwiftUI/iOS-SwiftUI-UITests/SentryRedactModifierTests.swift Outdated Show resolved Hide resolved

Sources/Sentry/SentryTimeToDisplayTracker.m Show resolved Hide resolved

Update SentryRedactModifierTests.swift

45623f3

philprime reviewed Dec 11, 2024

View reviewed changes

Sources/Sentry/SentryHub.m Show resolved Hide resolved

Sources/SentrySwiftUI/TracingView.swift Outdated Show resolved Hide resolved

Merge branch 'main' into feat/swiftui-ttid

e572da7

philipphofmann reviewed Dec 12, 2024

View reviewed changes

Sources/Sentry/SentryUIViewControllerPerformanceTracker.m Outdated Show resolved Hide resolved

Sources/SentrySwiftUI/SentryTracedView.swift Outdated Show resolved Hide resolved

use onAppear

331d1df

getsentry-bot and others added 3 commits December 12, 2024 14:09

Format code

f1b6632

Update SentryTracedView.swift

6dc28e5

fix test

588764e

brustolin requested a review from philipphofmann December 13, 2024 09:50

brustolin and others added 2 commits December 13, 2024 11:08

Update Sources/Sentry/SentryUIViewControllerPerformanceTracker.m

15cfd35

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

Format code

edba1ba

brustolin requested a review from philprime December 16, 2024 09:18

philipphofmann reviewed Dec 17, 2024

View reviewed changes

brustolin and others added 7 commits December 18, 2024 10:16

refactoring

98e4c6e

Format code

facf4a3

move code to viewModel

d3bc4ca

Update SentryTracedView.swift

036a741

Update SentryTracedView.swift

b4ddf12

More change

4c749b4

Merge branch 'main' into feat/swiftui-ttid

25d4173

philipphofmann reviewed Dec 19, 2024

View reviewed changes

brustolin and others added 2 commits December 20, 2024 08:17

Update test.yml

951bab7

Merge branch 'main' into feat/swiftui-ttid

9f95dd8

brustolin commented Dec 20, 2024

View reviewed changes

scripts/xcode-test.sh Outdated Show resolved Hide resolved

Update scripts/xcode-test.sh

c95d3ec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: SwiftUI time for initial display and time for full display #4596

feat: SwiftUI time for initial display and time for full display #4596

brustolin commented Dec 4, 2024

github-actions bot commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/swiftui-ttid

Startup times

App size

codecov bot commented Dec 4, 2024 •

edited

Loading

philipphofmann left a comment

philipphofmann Dec 17, 2024

brustolin Dec 18, 2024

philipphofmann Dec 17, 2024

philipphofmann left a comment

philipphofmann Dec 19, 2024

philipphofmann Dec 19, 2024

brustolin Dec 19, 2024

philipphofmann Dec 19, 2024

brustolin Dec 19, 2024

feat: SwiftUI time for initial display and time for full display #4596

Are you sure you want to change the base?

feat: SwiftUI time for initial display and time for full display #4596

Conversation

brustolin commented Dec 4, 2024

📜 Description

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Dec 4, 2024 • edited Loading

github-actions bot commented Dec 4, 2024 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/swiftui-ttid

Startup times

App size

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

philipphofmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading