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(profiling): continuous profiling from launch #3938

Conversation

armcknight
Copy link
Member

for #3555; #skip-changelog

This PR makes the necessary changes to app launch profiling logic to work with the continuous profiler, since we'll no longer be using SentryTracer to manage starting the profiler, instead starting it directly.

Copy link

github-actions bot commented May 5, 2024

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

Generated by 🚫 dangerJS against 86b8815

Copy link

github-actions bot commented May 5, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1236.24 ms 1251.63 ms 15.38 ms
Size 21.58 KiB 630.72 KiB 609.14 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d9cd5f1 1234.14 ms 1257.69 ms 23.54 ms
257c2a9 1231.45 ms 1252.12 ms 20.67 ms
6001822 1234.49 ms 1265.20 ms 30.71 ms
e24290f 1239.94 ms 1248.28 ms 8.34 ms
973d574 1228.92 ms 1246.79 ms 17.87 ms
8aba9c4 1236.94 ms 1248.29 ms 11.35 ms
6943de0 1235.98 ms 1246.88 ms 10.90 ms
7419285 1209.53 ms 1244.72 ms 35.19 ms
408f43e 1229.31 ms 1247.52 ms 18.21 ms
69d2789 1224.14 ms 1247.17 ms 23.02 ms

App size

Revision Plain With Sentry Diff
d9cd5f1 21.58 KiB 544.73 KiB 523.14 KiB
257c2a9 20.76 KiB 401.36 KiB 380.60 KiB
6001822 22.85 KiB 410.98 KiB 388.13 KiB
e24290f 22.84 KiB 403.51 KiB 380.66 KiB
973d574 21.58 KiB 542.38 KiB 520.80 KiB
8aba9c4 21.58 KiB 544.72 KiB 523.14 KiB
6943de0 20.76 KiB 393.33 KiB 372.57 KiB
7419285 20.76 KiB 432.99 KiB 412.22 KiB
408f43e 21.58 KiB 573.17 KiB 551.59 KiB
69d2789 21.58 KiB 548.09 KiB 526.50 KiB

Previous results on branch: armcknight/feat/3555-continuous-profiling/5-implementation/4-launch-profiling

Startup times

Revision Plain With Sentry Diff
21d1971 1246.59 ms 1255.06 ms 8.47 ms
1f9a916 1212.57 ms 1241.27 ms 28.69 ms
4cc09ac 1245.08 ms 1257.56 ms 12.48 ms
db1c44f 1236.31 ms 1241.31 ms 4.99 ms

App size

Revision Plain With Sentry Diff
21d1971 21.58 KiB 630.71 KiB 609.13 KiB
1f9a916 21.58 KiB 630.20 KiB 608.62 KiB
4cc09ac 21.58 KiB 621.09 KiB 599.51 KiB
db1c44f 21.58 KiB 630.11 KiB 608.53 KiB

Copy link

codecov bot commented May 8, 2024

Codecov Report

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

Project coverage is 90.955%. Comparing base (b2c9166) to head (86b8815).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3938       +/-   ##
=============================================
+ Coverage   90.856%   90.955%   +0.099%     
=============================================
  Files          594       594               
  Lines        46392     46481       +89     
  Branches     16620     16661       +41     
=============================================
+ Hits         42150     42277      +127     
+ Misses        4062      4026       -36     
+ Partials       180       178        -2     
Files Coverage Δ
SentryTestUtils/ClearTestState.swift 100.000% <100.000%> (ø)
...urces/Sentry/Profiling/SentryProfilerTestHelpers.m 65.384% <100.000%> (-2.263%) ⬇️
Sources/Sentry/SentryOptions.m 99.202% <100.000%> (ø)
Sources/Sentry/SentryProfiler.mm 90.967% <100.000%> (-0.058%) ⬇️
Sources/Sentry/SentrySampling.m 92.682% <100.000%> (ø)
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryTracer.m 96.889% <ø> (+0.231%) ⬆️
...yProfilerTests/SentryAppLaunchProfilingTests.swift 100.000% <100.000%> (ø)
...entryProfilerTests/SentryLegacyProfilerTests.swift 98.337% <100.000%> (+0.003%) ⬆️
Sources/Sentry/Profiling/SentryLaunchProfiling.m 93.333% <84.210%> (+31.491%) ⬆️

... and 6 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 b2c9166...86b8815. Read the comment docs.

@armcknight armcknight marked this pull request as ready for review May 14, 2024 19:43
Base automatically changed from armcknight/feat/3555-continuous-profiling/5-implementation/3-automatic-start-stop to main May 14, 2024 21:09
@armcknight armcknight force-pushed the armcknight/feat/3555-continuous-profiling/5-implementation/4-launch-profiling branch from 8a829a6 to a24c529 Compare May 15, 2024 06:39
@armcknight armcknight changed the base branch from main to armcknight/ref/move-testhub-to-testutils May 15, 2024 07:23
@armcknight armcknight force-pushed the armcknight/feat/3555-continuous-profiling/5-implementation/4-launch-profiling branch from f91f24f to b102627 Compare May 15, 2024 07:26
@armcknight armcknight force-pushed the armcknight/feat/3555-continuous-profiling/5-implementation/4-launch-profiling branch from b102627 to 363cb2c Compare May 15, 2024 07:42
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, almost LGTM 😸

Base automatically changed from armcknight/ref/move-testhub-to-testutils to main May 15, 2024 18:30
@armcknight armcknight force-pushed the armcknight/feat/3555-continuous-profiling/5-implementation/4-launch-profiling branch from 1fa9daf to 8b6a180 Compare May 15, 2024 22:39
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 armcknight merged commit f1d1166 into main May 16, 2024
69 of 70 checks passed
@armcknight armcknight deleted the armcknight/feat/3555-continuous-profiling/5-implementation/4-launch-profiling branch May 16, 2024 17:55
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.

2 participants