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): starting and stopping continuous profiler #3937

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented May 5, 2024

for #3555; #skip-changelog

After some more discussion, we're only going to offer manually starting and stopping the profiler (besides app launch profiling starting it automatically before any app code can actually run; but that must at least be manually configured by customers by setting the SentryOption for it).

We originally planned to automatically start/stop with session start/stop that respected the profiles sampling definitions, but we won't move forward with that right now. Manual start/stop will not respect profiling sample rates, calls will always start the profiler if one isn't already running.

Closing the SDK will stop any profiler that's running.

This cleans up anything that tied profiles to sessions, and adds more tests around various start/stop scenarios.

…ure of profiler frequency; remove duplicate declaration of threadSanitizerIsPresent(); move some SentryProfiler.mm imports into SENTRY_HAS_UIKIT gate
…ctly from dep container in profile serialization
- move old profiler control logic to new SentryLegacyProfiler class,
    leaving SentryProfiler to only contain the state and internal
    reference to SamplingProfiler
…continuous-profiling/4-refactoring/3-renames
armcknight added 3 commits May 8, 2024 15:59
…ion/2-schema-serialization' into armcknight/feat/3555-continuous-profiling/5-implementation/3-automatic-start-stop
Copy link

codecov bot commented May 8, 2024

Codecov Report

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

Project coverage is 90.891%. Comparing base (c838244) to head (d0c8e78).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3937       +/-   ##
=============================================
+ Coverage   90.858%   90.891%   +0.032%     
=============================================
  Files          594       594               
  Lines        46328     46351       +23     
  Branches     16599     16610       +11     
=============================================
+ Hits         42093     42129       +36     
+ Misses        4165      4152       -13     
  Partials        70        70               
Files Coverage Δ
SentryTestUtils/ClearTestState.swift 100.000% <100.000%> (ø)
Sources/Sentry/Profiling/SentryLaunchProfiling.m 61.842% <100.000%> (ø)
Sources/Sentry/SentryHub.m 98.554% <100.000%> (ø)
Sources/Sentry/SentrySDK.m 86.585% <100.000%> (+0.249%) ⬆️
Sources/Sentry/SentrySampling.m 92.682% <ø> (ø)
Sources/Sentry/SentryTracer.m 96.897% <100.000%> (+0.007%) ⬆️
...yProfilerTests/SentryContinuousProfilerTests.swift 100.000% <100.000%> (ø)
...SentryProfilerTests/SentryProfileTestFixture.swift 99.401% <100.000%> (+0.003%) ⬆️
Sources/Sentry/SentryProfiler.mm 91.025% <92.857%> (+0.116%) ⬆️

... and 8 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 c838244...d0c8e78. Read the comment docs.

@armcknight armcknight marked this pull request as ready for review May 8, 2024 21:05
@armcknight armcknight marked this pull request as draft May 9, 2024 12:02
@armcknight armcknight changed the title feat(profiling): automatically starting and stopping continuous profiler feat(profiling): starting and stopping continuous profiler May 9, 2024
@armcknight armcknight marked this pull request as ready for review May 9, 2024 19:13
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

Base automatically changed from armcknight/feat/3555-continuous-profiling/5-implementation/2-schema-serialization to main May 14, 2024 19:28
Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1233.61 ms 1254.11 ms 20.50 ms
Size 21.58 KiB 629.82 KiB 608.24 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
deeb22c 1233.90 ms 1250.19 ms 16.29 ms
3a6495e 1227.61 ms 1239.22 ms 11.60 ms
742d4b6 1196.56 ms 1216.54 ms 19.98 ms
89491ad 1222.12 ms 1231.96 ms 9.83 ms
1a4da1b 1222.14 ms 1239.50 ms 17.36 ms
236d8a8 1232.33 ms 1255.55 ms 23.22 ms
983de17 1260.57 ms 1263.68 ms 3.11 ms
1b09e3f 1227.24 ms 1242.19 ms 14.95 ms
3cba0e8 1231.20 ms 1251.50 ms 20.30 ms
59c1b97 1230.70 ms 1257.12 ms 26.42 ms

App size

Revision Plain With Sentry Diff
deeb22c 21.58 KiB 612.11 KiB 590.53 KiB
3a6495e 21.58 KiB 422.66 KiB 401.08 KiB
742d4b6 21.58 KiB 546.20 KiB 524.62 KiB
89491ad 21.58 KiB 417.89 KiB 396.30 KiB
1a4da1b 21.58 KiB 418.33 KiB 396.75 KiB
236d8a8 21.58 KiB 418.70 KiB 397.12 KiB
983de17 22.84 KiB 403.19 KiB 380.34 KiB
1b09e3f 21.58 KiB 614.93 KiB 593.34 KiB
3cba0e8 22.84 KiB 403.18 KiB 380.34 KiB
59c1b97 21.58 KiB 547.63 KiB 526.05 KiB

@armcknight armcknight merged commit d914696 into main May 14, 2024
68 of 70 checks passed
@armcknight armcknight deleted the armcknight/feat/3555-continuous-profiling/5-implementation/3-automatic-start-stop branch May 14, 2024 21:09
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.

3 participants