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

feat(app launch profiling): add app launch profiling #3529

Merged
merged 56 commits into from
Feb 16, 2024

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Jan 3, 2024

This PR and those that follow will add the ability of the SDK to start the profiler as early as possible from our code in order to record profiling information during an app launch..

Internal design doc

Followed by #3558
Docs PR: getsentry/sentry-docs#8876

Major changes:

  • persist traces/sampling rates to a file we can check on class load, before SentrySDK.startWithOptions is called

    • we store this in a new sentry path under Application Support, not caches as is used by SentryFileManager.sentryPath
    • starts a dedicated launch trace to attach the profile to
  • add logic to SentryTracer to check the config file and control starting the profiler for launch; because SentryTracer is already the thing that manages SentryProfiler today, this seemed like the best location

  • notes on some decisions

    • we decided not to use plist keys to enable this feature for now; we can iterate on it if we think profiling a post install/upgrade cold launch will be valuable (I think it would be) and after battle testing the existing logic
    • we considered using a root superclass to force a +[load] call order so we could start the profiler as early as possible, but decided against it for now, see ref: create root object as superclass for all SDK classes #3532
    • I considered exposing a method up through SentrySDK like startAppProfile so customers could do it from their own class load methods, but will save that for iterating; there's a timing experiment I'd like to do first
    • we tried using preexisting automatic transactions like ui.load but there are edge cases involved with it, and it doesn't cover all use cases like SwiftUI; so we decided instead to use dedicated traces for launch

This comment was marked as outdated.

@philipphofmann

This comment was marked as outdated.

@armcknight

This comment was marked as outdated.

@armcknight armcknight force-pushed the armcknight/feat/launch-profiling branch from 655b9d3 to f3e920a Compare January 4, 2024 02:00
@armcknight armcknight changed the base branch from main to armcknight/ref/sentry-root-object January 4, 2024 02:00
@armcknight armcknight force-pushed the armcknight/ref/sentry-root-object branch from 182d7e3 to 5f17e83 Compare January 4, 2024 02:01
@philipphofmann

This comment was marked as outdated.

@armcknight armcknight force-pushed the armcknight/feat/launch-profiling branch from f3e920a to 9572d11 Compare January 5, 2024 00:52
@armcknight armcknight changed the base branch from armcknight/ref/sentry-root-object to main January 5, 2024 00:53
@armcknight armcknight force-pushed the armcknight/feat/launch-profiling branch from 9572d11 to 26c750c Compare January 5, 2024 02:02
@armcknight armcknight changed the base branch from main to armcknight/test/sample-app-tweaks January 5, 2024 02:02
Copy link

github-actions bot commented Jan 5, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1217.18 ms 1238.24 ms 21.06 ms
Size 21.58 KiB 422.66 KiB 401.08 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
189b629 1211.16 ms 1224.30 ms 13.14 ms
98752f3 1240.61 ms 1259.80 ms 19.18 ms
06548c0 1225.58 ms 1244.70 ms 19.12 ms
f8fc36d 1226.31 ms 1247.80 ms 21.49 ms
dfde86d 1215.12 ms 1221.30 ms 6.18 ms
7c5d161 1224.38 ms 1249.66 ms 25.28 ms
0dfdaaa 1226.88 ms 1248.82 ms 21.94 ms
04c0c68 1223.22 ms 1251.24 ms 28.02 ms
2ce582e 1238.98 ms 1259.66 ms 20.68 ms
f2daa68 1211.14 ms 1217.53 ms 6.39 ms

App size

Revision Plain With Sentry Diff
189b629 20.76 KiB 399.69 KiB 378.93 KiB
98752f3 20.76 KiB 435.09 KiB 414.33 KiB
06548c0 20.76 KiB 427.35 KiB 406.59 KiB
f8fc36d 20.76 KiB 419.70 KiB 398.94 KiB
dfde86d 21.58 KiB 414.57 KiB 392.99 KiB
7c5d161 20.76 KiB 414.44 KiB 393.68 KiB
0dfdaaa 20.76 KiB 434.56 KiB 413.79 KiB
04c0c68 22.85 KiB 414.09 KiB 391.24 KiB
2ce582e 22.85 KiB 410.97 KiB 388.12 KiB
f2daa68 21.58 KiB 418.40 KiB 396.82 KiB

Previous results on branch: armcknight/feat/launch-profiling

Startup times

Revision Plain With Sentry Diff
55af12d 1229.06 ms 1244.85 ms 15.79 ms
97fc3fc 1203.63 ms 1211.80 ms 8.16 ms
a4122ec 1205.48 ms 1229.02 ms 23.54 ms
8512126 1232.21 ms 1253.60 ms 21.39 ms
7d8f64b 1214.90 ms 1230.56 ms 15.67 ms
2f41ea8 1194.29 ms 1204.35 ms 10.07 ms
a2498ae 1202.59 ms 1213.83 ms 11.24 ms
af0dbfd 1202.61 ms 1209.63 ms 7.02 ms
54c42ec 1208.63 ms 1224.35 ms 15.71 ms

App size

Revision Plain With Sentry Diff
55af12d 21.58 KiB 421.23 KiB 399.65 KiB
97fc3fc 21.58 KiB 420.05 KiB 398.46 KiB
a4122ec 21.58 KiB 422.04 KiB 400.46 KiB
8512126 21.58 KiB 422.65 KiB 401.06 KiB
7d8f64b 21.58 KiB 422.15 KiB 400.57 KiB
2f41ea8 21.58 KiB 427.08 KiB 405.50 KiB
a2498ae 21.58 KiB 422.10 KiB 400.51 KiB
af0dbfd 21.58 KiB 422.10 KiB 400.52 KiB
54c42ec 21.58 KiB 422.66 KiB 401.08 KiB

Base automatically changed from armcknight/test/sample-app-tweaks to main January 8, 2024 20:00
@armcknight armcknight linked an issue Jan 9, 2024 that may be closed by this pull request
@armcknight armcknight force-pushed the armcknight/feat/launch-profiling branch from 26c750c to d8b5c71 Compare January 11, 2024 00:57
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 75 lines in your changes are missing coverage. Please review.

Comparison is base (6e1452d) 89.348% compared to head (bb83d27) 89.223%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3529       +/-   ##
=============================================
- Coverage   89.348%   89.223%   -0.126%     
=============================================
  Files          530       532        +2     
  Lines        58291     58691      +400     
  Branches     20917     21066      +149     
=============================================
+ Hits         52082     52366      +284     
- Misses        5183      5288      +105     
- Partials      1026      1037       +11     
Files Coverage Δ
Sources/Sentry/Profiling/SentryProfilerState.mm 98.484% <100.000%> (+0.110%) ⬆️
Sources/Sentry/SentryHub.m 98.504% <100.000%> (-0.016%) ⬇️
Sources/Sentry/SentryLog.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryOptions.m 97.613% <100.000%> (-0.036%) ⬇️
Sources/Sentry/SentryProfileTimeseries.mm 48.076% <100.000%> (+1.018%) ⬆️
Sources/Sentry/SentrySDK.m 90.697% <100.000%> (+0.259%) ⬆️
Sources/Sentry/SentrySamplerDecision.m 100.000% <100.000%> (ø)
Sources/Sentry/include/SentryInternalDefines.h 50.000% <100.000%> (+7.894%) ⬆️
...entryProfilerTests/SentryAppLaunchProfilingTests.m 100.000% <100.000%> (ø)
...SentryProfilerTests/SentryProfilerSwiftTests.swift 97.730% <100.000%> (-0.010%) ⬇️
... and 10 more

... and 15 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 6e1452d...bb83d27. Read the comment docs.

Sources/Sentry/SentryLog.m Outdated Show resolved Hide resolved
Sources/Sentry/SentrySDK.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Show resolved Hide resolved
@armcknight armcknight marked this pull request as ready for review January 13, 2024 01:43
@armcknight
Copy link
Member Author

I fixed a build error by adding SentryProfilingConditionals.h to iOS-SwiftUI/TestHeaders, and Xcode made a whole bunch of changes in its project file. Not sure what's up with it. See 4866ffa

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

I'm excited to see app start profiling moving forward 💪.

As this feature has a high risk of breaking things, I took a close look and added plenty of comments. I haven't completely reviewed all the code, as some comments could lead to a few changes that could make some of my comments outdated.

I will wait to review #3558 as changes here could also bubble up to it.

Sources/Sentry/Public/SentryOptions.h Outdated Show resolved Hide resolved
Sources/Sentry/Public/SentryOptions.h Outdated Show resolved Hide resolved
Sources/Sentry/Public/SentryOptions.h Outdated Show resolved Hide resolved
Sources/Sentry/SentrySDK.m Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryHub.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

Philipp did most of the heavy lifting in reviewing this, and it looks good.
I will give it another pass on Monday to understand everything and double-check.

CHANGELOG.md Outdated
@@ -17,6 +17,7 @@

- Add visionOS as device family (#3548)
- Add VisionOS Support for Carthage (#3565)
- Automatically profile app launches (#3529)

This comment was marked as resolved.

Sources/Sentry/include/SentryLaunchProfiling.h Outdated Show resolved Hide resolved
@armcknight

This comment was marked as outdated.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

I found one more important issue. Apart from that LGTM.

Sources/Sentry/SentryFileManager.m Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM

armcknight and others added 6 commits February 16, 2024 09:26
Add the possibility to add multiple transports in the TransportAdapter,
which is internal. This is a preparation for adding support for
Spotlight.
@armcknight armcknight changed the base branch from main to armcknight/test/reset-file-manager-state February 16, 2024 01:49
Base automatically changed from armcknight/test/reset-file-manager-state to main February 16, 2024 12:26
@armcknight armcknight merged commit f8833c4 into main Feb 16, 2024
70 of 74 checks passed
@armcknight armcknight deleted the armcknight/feat/launch-profiling branch February 16, 2024 13:10
philipphofmann added a commit that referenced this pull request Feb 20, 2024
The with GH-3529 added test method writeProfileFile crashed when a
profile file already existed. Now, the writeProfileFile method exists
and prints a debug message when a profiling file already exists and logs
an error when it can't write the profile.
philipphofmann added a commit that referenced this pull request Feb 20, 2024
The with GH-3529 added test method writeProfileFile crashed when a
profile file already existed. Now, the writeProfileFile method exists
and prints a debug message when a profiling file already exists and logs
an error when it can't write the profile.

Fixes GH-3661
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App Launch Profiling
3 participants