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): change how continuous profiling is enabled in options #4024

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented May 30, 2024

After some team discussion, we decided to change how continuous profiling is enabled. Remove SentryOptions.enableContinuousProfiling, and make setting SentryOptions.profilesSampleRate = nil the way to do it.

ETA (3 Jun 2024): a profilesSampleRate of 0 also enables continuous profiling. If a sampler function is defined, always disable continuous profiling.

Second edit: we decided to go back on the decision to overload the 0 value. Now, only setting it to nil is the way to enable continuous profiling again.

#skip-changelog; for #3555

Copy link

github-actions bot commented May 30, 2024

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

Generated by 🚫 dangerJS against ca21101

Copy link

codecov bot commented May 30, 2024

Codecov Report

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

Project coverage is 91.139%. Comparing base (643853e) to head (9cdbaf8).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4024       +/-   ##
=============================================
+ Coverage   91.076%   91.139%   +0.062%     
=============================================
  Files          606       607        +1     
  Lines        47515     47716      +201     
  Branches     17023     17150      +127     
=============================================
+ Hits         43275     43488      +213     
+ Misses        4146      4135       -11     
+ Partials        94        93        -1     
Files Coverage Δ
Sources/Sentry/Profiling/SentryLaunchProfiling.m 90.000% <100.000%> (-3.334%) ⬇️
Sources/Sentry/SentryOptions.m 99.240% <100.000%> (+0.013%) ⬆️
Sources/Sentry/SentrySampling.m 92.682% <ø> (ø)
Sources/Sentry/SentrySpan.m 99.358% <100.000%> (ø)
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryTracer.m 96.666% <100.000%> (+0.255%) ⬆️
Sources/Sentry/SentryTransaction.m 90.140% <100.000%> (+0.910%) ⬆️
...rding/Monitors/SentryCrashMonitor_CPPException.cpp 20.408% <ø> (ø)
...yProfilerTests/SentryAppLaunchProfilingTests.swift 100.000% <100.000%> (ø)
...yProfilerTests/SentryContinuousProfilerTests.swift 97.000% <100.000%> (+0.092%) ⬆️
... and 6 more

... and 20 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 643853e...9cdbaf8. Read the comment docs.

Copy link

github-actions bot commented May 30, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1216.84 ms 1239.45 ms 22.61 ms
Size 21.58 KiB 655.73 KiB 634.15 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
6943de0 1215.83 ms 1253.22 ms 37.39 ms
9cc7e7c 1228.90 ms 1237.96 ms 9.06 ms
0559a8f 1239.78 ms 1252.60 ms 12.83 ms
ef284b2 1211.76 ms 1229.69 ms 17.93 ms
b9a9ffd 1201.61 ms 1215.06 ms 13.45 ms
add9550 1228.15 ms 1247.58 ms 19.44 ms
e84bc3f 1196.62 ms 1218.86 ms 22.24 ms
736495a 1245.16 ms 1254.42 ms 9.26 ms
32ac934 1224.46 ms 1243.16 ms 18.70 ms
326b7eb 1223.41 ms 1235.66 ms 12.25 ms

App size

Revision Plain With Sentry Diff
6943de0 20.76 KiB 393.33 KiB 372.57 KiB
9cc7e7c 22.84 KiB 403.13 KiB 380.29 KiB
0559a8f 21.58 KiB 419.81 KiB 398.23 KiB
ef284b2 20.76 KiB 401.36 KiB 380.60 KiB
b9a9ffd 21.58 KiB 418.43 KiB 396.85 KiB
add9550 21.58 KiB 418.37 KiB 396.79 KiB
e84bc3f 20.76 KiB 434.72 KiB 413.96 KiB
736495a 20.76 KiB 435.22 KiB 414.46 KiB
32ac934 21.58 KiB 616.72 KiB 595.14 KiB
326b7eb 20.76 KiB 432.31 KiB 411.55 KiB

Previous results on branch: armcknight/feat/3555-continuous-profiling/5-implementation/12-rework-continuous-profiling-options

Startup times

Revision Plain With Sentry Diff
3e1f4ee 1234.98 ms 1253.38 ms 18.40 ms
8364b13 1244.82 ms 1262.10 ms 17.29 ms
73665c4 1217.96 ms 1242.72 ms 24.76 ms

App size

Revision Plain With Sentry Diff
3e1f4ee 21.58 KiB 656.80 KiB 635.21 KiB
8364b13 21.58 KiB 643.31 KiB 621.73 KiB
73665c4 21.58 KiB 655.80 KiB 634.21 KiB

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.

PR looks about right. I'm going to give this a full review once it's marked as ready for review.

…g tests; add new private SentryOptions.isContinuousProfiling method, and note to isProfilingEnabled prop
@armcknight armcknight changed the base branch from armcknight/feat/3555-continuous-profiling/5-implementation/11-expose-continuous-profiling to main June 5, 2024 04:44
@armcknight armcknight force-pushed the armcknight/feat/3555-continuous-profiling/5-implementation/12-rework-continuous-profiling-options branch from 90d0fd0 to fc7f58b Compare June 5, 2024 04:44
@armcknight armcknight marked this pull request as ready for review June 5, 2024 04:44
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

Sources/Sentry/Public/SentryOptions.h Show resolved Hide resolved
Sources/Sentry/SentryTimeToDisplayTracker.m Outdated Show resolved Hide resolved
Tests/SentryTests/SentrySDKTests.swift 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.

Again, LGTM.

…continuous-profiling/5-implementation/12-rework-continuous-profiling-options
@armcknight armcknight merged commit e70a2e1 into main Jun 11, 2024
33 of 35 checks passed
@armcknight armcknight deleted the armcknight/feat/3555-continuous-profiling/5-implementation/12-rework-continuous-profiling-options branch June 11, 2024 20:53
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