feat: Added screen name to app context #3346

brustolin · 2023-10-17T14:49:44Z

📜 Description

Add screens name to app context

💡 Motivation and Context

close #3140

💚 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 · 2023-10-17T14:51:09Z

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

Generated by 🚫 dangerJS against 85f4d9e

codecov · 2023-10-17T15:10:11Z

Codecov Report

Merging #3346 (85f4d9e) into main (734d507) will decrease coverage by 0.083%.
The diff coverage is 64.351%.

@@              Coverage Diff              @@
##              main     #3346       +/-   ##
=============================================
- Coverage   89.068%   88.986%   -0.083%     
=============================================
  Files          520       520               
  Lines        55884     56293      +409     
  Branches     19857     20259      +402     
=============================================
+ Hits         49775     50093      +318     
- Misses        5195      5283       +88     
- Partials       914       917        +3

Files	Coverage Δ
Sources/Sentry/SentryClient.m	`97.649% <100.000%> (+0.061%)`	⬆️
Sources/Sentry/SentryDispatchQueueWrapper.m	`100.000% <100.000%> (ø)`
Sources/Sentry/SentryTracer.m	`96.276% <100.000%> (+0.205%)`	⬆️
Tests/SentryTests/SentryClientTests.swift	`95.962% <94.736%> (-0.061%)`	⬇️
Sources/Sentry/SentryUIApplication.m	`54.166% <31.775%> (-39.276%)`	⬇️

... and 64 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 734d507...85f4d9e. Read the comment docs.

github-actions · 2023-10-17T15:10:13Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1210.71 ms	1221.37 ms	10.65 ms
Size	22.85 KiB	414.99 KiB	392.14 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`7a7de19`	1251.30 ms	1266.94 ms	15.64 ms
`46f5eb8`	1200.09 ms	1231.38 ms	31.29 ms
`7f14650`	1249.73 ms	1269.88 ms	20.14 ms
`69d8759`	1229.88 ms	1240.45 ms	10.57 ms
`39b1c35`	1244.71 ms	1248.60 ms	3.89 ms
`f4e0299`	1230.33 ms	1249.68 ms	19.35 ms
`83d2d84`	1211.31 ms	1227.34 ms	16.03 ms
`01a28a9`	1237.24 ms	1253.24 ms	16.00 ms
`621ba9b`	1190.66 ms	1230.84 ms	40.18 ms
`ab6fee4`	1227.63 ms	1239.80 ms	12.17 ms

App size

Revision	Plain	With Sentry	Diff
`7a7de19`	20.76 KiB	435.64 KiB	414.88 KiB
`46f5eb8`	20.76 KiB	432.37 KiB	411.61 KiB
`7f14650`	22.84 KiB	402.63 KiB	379.79 KiB
`69d8759`	20.76 KiB	393.05 KiB	372.29 KiB
`39b1c35`	22.85 KiB	408.88 KiB	386.03 KiB
`f4e0299`	20.76 KiB	427.54 KiB	406.78 KiB
`83d2d84`	20.76 KiB	419.66 KiB	398.90 KiB
`01a28a9`	22.85 KiB	405.39 KiB	382.54 KiB
`621ba9b`	20.76 KiB	414.45 KiB	393.69 KiB
`ab6fee4`	22.84 KiB	401.67 KiB	378.83 KiB

Previous results on branch: feat/screen-name-events

Startup times

Revision	Plain	With Sentry	Diff
`d0ecf51`	1230.45 ms	1242.73 ms	12.29 ms
`1e297a8`	1204.14 ms	1227.54 ms	23.40 ms
`6086f29`	1222.51 ms	1233.80 ms	11.29 ms
`6ef1209`	1245.68 ms	1250.68 ms	5.00 ms
`bc12022`	1232.84 ms	1259.57 ms	26.73 ms
`d94450c`	1234.14 ms	1251.32 ms	17.18 ms
`2c15262`	1236.59 ms	1240.00 ms	3.41 ms
`3ad9ba7`	1261.61 ms	1276.48 ms	14.87 ms
`750145f`	1262.98 ms	1276.68 ms	13.70 ms
`ad03b34`	1236.80 ms	1243.32 ms	6.52 ms

App size

Revision	Plain	With Sentry	Diff
`d0ecf51`	22.85 KiB	414.25 KiB	391.41 KiB
`1e297a8`	22.85 KiB	413.21 KiB	390.36 KiB
`6086f29`	22.85 KiB	414.99 KiB	392.14 KiB
`6ef1209`	22.85 KiB	414.37 KiB	391.52 KiB
`bc12022`	22.85 KiB	413.21 KiB	390.36 KiB
`d94450c`	22.85 KiB	414.20 KiB	391.35 KiB
`2c15262`	22.85 KiB	414.45 KiB	391.60 KiB
`3ad9ba7`	22.85 KiB	412.96 KiB	390.11 KiB
`750145f`	22.85 KiB	414.33 KiB	391.48 KiB
`ad03b34`	22.85 KiB	414.54 KiB	391.69 KiB

…-cocoa into feat/screen-name-events

philipphofmann

As mentioned on Slack, I'm unsure about the general approach of this PR, and we need to discuss it before continuing.

CHANGELOG.md

Sources/Sentry/Public/SentryScope.h

Sources/Sentry/SentryClient.m

Tests/SentryTests/SentryClientTests.swift

Sources/Sentry/SentryUIApplication.m

Co-authored-by: Philipp Hofmann <[email protected]>

…-cocoa into feat/screen-name-events

Co-authored-by: Philipp Hofmann <[email protected]>

…-cocoa into feat/screen-name-events

philipphofmann

We still miss some UITests or tests for the code in SentryUIApplication. Apart from that, this already looks great.

Sources/Sentry/SentryTracer.m

philipphofmann · 2023-11-02T14:16:59Z

Sources/Sentry/include/SentryUIApplication.h

+ * view controller for each screen. If the topmost view controller
+ * is a split view controller, all of its sides are reported.
+ */
+- (nullable NSArray<UIViewController *> *)relevantViewControllers;


m: I think this method can now be internal to SentryUIApplication. No other class is using it, they all use relevantViewControllersNames.

philipphofmann

LGTM with a couple of minor suggestions.

Samples/iOS-Swift/iOS-Swift/ViewControllers/PageViewController.swift

Samples/iOS-Swift/iOS-SwiftUITests/TopViewControllerTests.swift

philipphofmann · 2023-11-06T15:06:44Z

Samples/iOS-Swift/iOS-Swift/Tools/SentryUIApplication.h

+/**
+ * A helper tool to retrieve informations from the application instance.
+ */
+@interface SentryUIApplication : NSObject


m: Why do we need this header file here? Please add a short comment with the explanation or remove it.

If it is needed here for some reason, we should use the one already in /Sources. See how SentryDevice.h/.mm are included in iOS-Swift/tvOS-Swift.

Adding the comment.
This is exposing private API for testing.

I rather have a copy of the header instead of using the same .h file because we have a better control of what is being exposed, and any change in the #imports in the original file wont affect the sample.

armcknight · 2023-11-06T18:39:09Z

Sources/Sentry/SentryUIApplication.m

+    if ([containerVC isKindOfClass:UISplitViewController.class]) {
+        UISplitViewController *splitVC = (UISplitViewController *)containerVC;
+        if (splitVC.viewControllers.count > 0) {
+            return [splitVC viewControllers];


So are these false positives? I see the UI test for split VC. Maybe we're not generating coverage from sample app UI tests? Otherwise we might want to raise this with codecov.

Exactly, I don't think we generate coverage from UI Tests.

philipphofmann · 2023-11-09T16:06:24Z

One final question just popped into my head. How expensive is the traversing of the UIViewControllers? Could it be that we slow down apps if they have a complex hierarchy of UIViewControllers, @brustolin? Are there any other risks involved with that PR? Should we maybe do a beta or are you confident to ship this PR right away?

If all answers to my questions are no, let's ship this 🚢 @brustolin

armcknight · 2023-11-09T21:45:07Z

One final question just popped into my head. How expensive is the traversing of the UIViewControllers? Could it be that we slow down apps if they have a complex hierarchy of UIViewControllers, @brustolin? Are there any other risks involved with that PR? Should we maybe do a beta or are you confident to ship this PR right away?

If all answers to my questions are no, let's ship this 🚢 @brustolin

We could get a quick idea on this if we create a perf test that measures the routine and create a full tree with N children per node and check it with various depths. Some quick numbers:

5 children per VC
- 1 level: 5 vcs
- 2 levels: 25 vcs
- 3 levels: 125 vcs
- 4 levels: 625 vcs

I can't quite imagine a view hierarchy with that many children VCs and layers, but iterating through 625 pointer references shouldn't take very long.

It's not possible to push a nav controller onto itself, but what if someone accidentally adds a view controller as a child of itself? Infinite loop? Any other possibilities for that here?

brustolin · 2023-11-10T10:23:48Z

Its impossible to create a infinite loop of UI Elements.
The speed to navigate the view controller tree is as fast as accessing properties of an objects, and vc trees are not big.
I believe this is good to go

philipphofmann · 2023-11-10T14:52:19Z

I can't quite imagine a view hierarchy with that many children VCs and layers, but iterating through 625 pointer references shouldn't take very long.

That's also my assumption. I would be surprised if it takes very long.

brustolin added 2 commits October 17, 2023 10:11

get relevant viewcontroller

8b205d2

add to event

74c78e6

brustolin changed the title ~~Feat/screen name events~~ feat: Added screen name to app context Oct 17, 2023

brustolin and others added 8 commits October 18, 2023 14:36

Update CHANGELOG.md

0ce2c42

Update SentryUIApplication.m

ed6fbc3

Update SentryClient.m

9d1d844

Format code

3ba3f65

Update SentryClient.m

8645718

Merge branch 'feat/screen-name-events' of github.com:getsentry/sentry…

da8f82f

…-cocoa into feat/screen-name-events

Format code

bf6000d

tests

fc2a826

brustolin marked this pull request as ready for review October 20, 2023 09:50

brustolin requested review from philipphofmann and armcknight as code owners October 20, 2023 09:50

Update SentryClientTests.swift

7af95b0

philipphofmann reviewed Oct 24, 2023

View reviewed changes

brustolin and others added 11 commits October 24, 2023 11:49

Apply suggestions from code review

2f1fc14

Co-authored-by: Philipp Hofmann <[email protected]>

Update SentryUIApplication.m

1a36c7b

adjustments

824b7fd

Format code

7443043

refs

9bba499

Merge branch 'feat/screen-name-events' of github.com:getsentry/sentry…

db34456

…-cocoa into feat/screen-name-events

Merge branch 'main' into feat/screen-name-events

e4c1183

Update CHANGELOG.md

2718bdd

Update CHANGELOG.md

461418e

Update SentryClientTests.swift

2175931

Merge branch 'main' into feat/screen-name-events

fe6b84b

brustolin and others added 9 commits October 31, 2023 15:50

Apply suggestions from code review

a1181bf

Co-authored-by: Philipp Hofmann <[email protected]>

Format code

3b0b391

refactoring

b9ad542

Format code

ad71fa3

Update SentryUIApplication.m

c5bbfab

Format code

15d63f8

Update SentryUIApplication.m

2bb1f44

Merge branch 'feat/screen-name-events' of github.com:getsentry/sentry…

76471ac

…-cocoa into feat/screen-name-events

Update ProfilingUITests.swift

27262df

philipphofmann reviewed Nov 2, 2023

View reviewed changes

brustolin added 5 commits November 2, 2023 17:59

Merge branch 'main' into feat/screen-name-events

3f3d8a5

UI Tests

f576777

fix ios13 sample

8a7fc71

Update TopViewControllerInspector.swift

13b13b6

Update PageViewController.swift

2fc0b4c

brustolin requested a review from philipphofmann November 3, 2023 14:53

Merge branch 'main' into feat/screen-name-events

f7e2bd6

philipphofmann approved these changes Nov 6, 2023

View reviewed changes

armcknight reviewed Nov 6, 2023

View reviewed changes

brustolin and others added 5 commits November 8, 2023 13:16

Merge branch 'main' into feat/screen-name-events

497ef2c

Refs

65451a2

test

cbefdeb

Format code

cc98e64

Merge branch 'main' into feat/screen-name-events

85f4d9e

brustolin merged commit e4cc043 into main Nov 10, 2023
69 of 71 checks passed

brustolin deleted the feat/screen-name-events branch November 10, 2023 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added screen name to app context #3346

feat: Added screen name to app context #3346

brustolin commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/screen-name-events

Startup times

App size

philipphofmann left a comment

philipphofmann left a comment

philipphofmann Nov 2, 2023

philipphofmann left a comment

philipphofmann Nov 6, 2023

armcknight Nov 6, 2023

brustolin Nov 8, 2023

armcknight Nov 6, 2023

brustolin Nov 8, 2023

philipphofmann commented Nov 9, 2023

armcknight commented Nov 9, 2023

brustolin commented Nov 10, 2023

philipphofmann commented Nov 10, 2023

feat: Added screen name to app context #3346

feat: Added screen name to app context #3346

Conversation

brustolin commented Oct 17, 2023 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Oct 17, 2023 • edited Loading

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

github-actions bot commented Oct 17, 2023 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/screen-name-events

Startup times

App size

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Nov 2, 2023

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Nov 6, 2023

Choose a reason for hiding this comment

armcknight Nov 6, 2023

Choose a reason for hiding this comment

brustolin Nov 8, 2023

Choose a reason for hiding this comment

armcknight Nov 6, 2023

Choose a reason for hiding this comment

brustolin Nov 8, 2023

Choose a reason for hiding this comment

philipphofmann commented Nov 9, 2023

armcknight commented Nov 9, 2023

brustolin commented Nov 10, 2023

philipphofmann commented Nov 10, 2023

brustolin commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

github-actions bot commented Oct 17, 2023 •

edited

Loading