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: append launch profile data to app start transactions/spans #3736

Merged

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Mar 14, 2024

Following on app launch profiling that only used a dedicated span to attach the profiles, we will now attach the profile data to an automatic app start transaction if one will be recorded, otherwise continue with the same dedicated txn.

Closes #3725

Changes

  • Change SentrySDK.startWithOptions from always stopping the launch profile to only stopping it if SentryUIViewControllerPerformanceTracker.shared.enableWaitForFullDisplay is NO, otherwise, that trace will eventually end and get all the profile data. Then, SentryTimeToDisplayTracker will call stopLaunchProfile passing nil as the hub argument which will stop the dedicated tracer without actually sending a duplicate dedicated transaction, since the profile data we previously attached to it is now on the app start txn.
  • Records the system time SentrySysctl.load is called and keeps that as the runtimeInitSystemTimestamp to report to SentryAppStartMeasurement, which is used when SentryTracer requests a slice of profile data to attach to the app start transaction. Profile slicing needs a system timestamp which it uses to keep relevant samples and compute relative timestamps. it's set as the SentryTransaction.startSystemTime for app start transactions.
  • We no longer need to keep track of originalStartTimestamp in SentryTracer. We now take either the actual start time of the tracer (which was always the case outside of app start transactions, which modifies SentryTracer.startTimestamp) or SentryAppStartMeasurement.runtimeInitTimestamp in the case of app start txns, since this corresponds to the time that we're recording SentrySysctl.processStartSystemTimestamp as mentioned above.
    • Note that for prewarmed launches, we don't record a span using runtimeInitTimestamp. But, we have no reliable way to test what will happen for these launch types, so we are going to postpone any further treatment of that edge case.

Before (the profile data starts at the extreme right end):

image

After:

image

@armcknight armcknight changed the title test: disable all launch arguments by default feat: append launch profile data to app start transactions/spans Mar 14, 2024
Copy link

github-actions bot commented Mar 14, 2024

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- append launch profile data to app start transactions/spans ([#3736](https://github.com/getsentry/sentry-cocoa/pull/3736))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against b34fe84

Copy link

github-actions bot commented Mar 14, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1241.53 ms 1251.67 ms 10.14 ms
Size 21.58 KiB 548.09 KiB 526.50 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
7054676 1229.46 ms 1246.40 ms 16.94 ms
861d361 1227.90 ms 1231.45 ms 3.55 ms
01a28a9 1237.78 ms 1259.04 ms 21.27 ms
72c8d84 1218.69 ms 1241.08 ms 22.39 ms
282cc99 1232.59 ms 1245.88 ms 13.29 ms
d0deebd 1235.17 ms 1255.54 ms 20.37 ms
c2acec5 1243.04 ms 1254.15 ms 11.10 ms
8c50edb 1212.98 ms 1233.72 ms 20.74 ms
ecd9ecd 1215.77 ms 1238.70 ms 22.93 ms
7cd187e 1243.04 ms 1244.79 ms 1.75 ms

App size

Revision Plain With Sentry Diff
7054676 21.58 KiB 542.28 KiB 520.70 KiB
861d361 20.76 KiB 435.65 KiB 414.89 KiB
01a28a9 22.85 KiB 405.39 KiB 382.54 KiB
72c8d84 22.85 KiB 408.88 KiB 386.03 KiB
282cc99 22.85 KiB 414.09 KiB 391.24 KiB
d0deebd 21.58 KiB 542.28 KiB 520.69 KiB
c2acec5 22.85 KiB 408.88 KiB 386.03 KiB
8c50edb 20.76 KiB 432.31 KiB 411.55 KiB
ecd9ecd 20.76 KiB 420.23 KiB 399.47 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB

Previous results on branch: armcknight/feat/launch-profile-data-in-app-start-spans

Startup times

Revision Plain With Sentry Diff
82b16fe 1230.27 ms 1244.67 ms 14.40 ms
07e5edc 1244.73 ms 1253.24 ms 8.51 ms
c32b818 1237.06 ms 1251.63 ms 14.56 ms
9f00605 1233.63 ms 1246.52 ms 12.90 ms
389fb12 1228.85 ms 1249.57 ms 20.72 ms
0dd6fa2 1237.14 ms 1255.69 ms 18.55 ms
9d7d5a1 1207.23 ms 1230.02 ms 22.79 ms

App size

Revision Plain With Sentry Diff
82b16fe 21.58 KiB 546.50 KiB 524.92 KiB
07e5edc 21.58 KiB 545.19 KiB 523.61 KiB
c32b818 21.58 KiB 546.60 KiB 525.02 KiB
9f00605 21.58 KiB 542.87 KiB 521.29 KiB
389fb12 21.58 KiB 542.88 KiB 521.29 KiB
0dd6fa2 21.58 KiB 546.57 KiB 524.99 KiB
9d7d5a1 21.58 KiB 546.58 KiB 525.00 KiB

@armcknight armcknight force-pushed the armcknight/feat/launch-profile-data-in-app-start-spans branch from 5da3e24 to 90c0bf1 Compare March 15, 2024 21:22
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 89.437%. Comparing base (59c1b97) to head (b34fe84).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3736       +/-   ##
=============================================
- Coverage   89.446%   89.437%   -0.010%     
=============================================
  Files          556       557        +1     
  Lines        60045     60240      +195     
  Branches     21560     21668      +108     
=============================================
+ Hits         53708     53877      +169     
- Misses        5303      5437      +134     
+ Partials      1034       926      -108     
Files Coverage Δ
Sources/Sentry/SentryAppStartMeasurement.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryAppStartTracker.m 97.633% <100.000%> (+0.014%) ⬆️
Sources/Sentry/SentryProfiler.mm 79.466% <100.000%> (+0.876%) ⬆️
Sources/Sentry/SentrySysctl.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
...SentryProfilerTests/SentryProfilerSwiftTests.swift 97.760% <100.000%> (+0.029%) ⬆️
...iewController/SentryTimeToDisplayTrackerTest.swift 99.436% <100.000%> (ø)
Tests/SentryTests/Protocol/TestData.swift 98.936% <100.000%> (ø)
...ts/SentryTests/Transaction/SentryTracerTests.swift 97.691% <100.000%> (+0.003%) ⬆️
Sources/Sentry/SentrySDK.m 90.579% <81.818%> (-0.398%) ⬇️
... and 2 more

... and 43 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 59c1b97...b34fe84. Read the comment docs.

@armcknight armcknight force-pushed the armcknight/feat/launch-profile-data-in-app-start-spans branch from 90c0bf1 to 646ede9 Compare March 15, 2024 21:43
@armcknight armcknight force-pushed the armcknight/feat/launch-profile-data-in-app-start-spans branch from 1534090 to 69fcf6f Compare March 19, 2024 21:39
@armcknight armcknight marked this pull request as ready for review March 19, 2024 22:14
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.

Looking good.
Just a couple of comments.

Sources/Sentry/include/SentrySpan.h Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.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.

I found a few issues.

CHANGELOG.md Outdated Show resolved Hide resolved
Sources/Sentry/SentrySysctl.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryAppStartTracker.m Outdated Show resolved Hide resolved
Sources/Sentry/Public/SentrySpanProtocol.h Outdated Show resolved Hide resolved
Sources/Sentry/include/SentryLaunchProfiling.h Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Show resolved Hide resolved
Sources/Sentry/SentryTimeToDisplayTracker.m Outdated Show resolved Hide resolved
Sources/Sentry/SentrySDK.m Show resolved Hide resolved
Sources/Sentry/SentryTracer.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.

Thanks for including most of the feedback 😃

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.

Thanks, @armcknight. LGTM

@armcknight armcknight merged commit 69d2789 into main Mar 26, 2024
69 of 70 checks passed
@armcknight armcknight deleted the armcknight/feat/launch-profile-data-in-app-start-spans branch March 26, 2024 22:34
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
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.

Integrate launch profile data into app start transactions/spans
3 participants