-
-
Notifications
You must be signed in to change notification settings - Fork 318
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): add API to SentrySDK to start/stop a continuous profiling session #3834
feat(profiling): add API to SentrySDK to start/stop a continuous profiling session #3834
Conversation
…ing new public API - there was already a SentryProfiler+Private.h; split up what was in there between the new one (nee SentryProfiler.h) and SentryProfiler+Test.h, depending on where the declarations are actually needed - there were also two SentryProfiler+Test.h files: one in the SDK, and one in the test utils lib; remove the one in the SDK, and move its declarations to the one in the test lib along with the appropriate ones moved from the old SentryProfiler+Private.h that was deleted to make way for renaming SentryProfiler.h to SentryProfiler+Private.h
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3834 +/- ##
=============================================
- Coverage 90.957% 90.627% -0.331%
=============================================
Files 560 579 +19
Lines 44271 45240 +969
Branches 15776 16098 +322
=============================================
+ Hits 40268 41000 +732
- Misses 3822 4060 +238
+ Partials 181 180 -1
... and 40 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
4b1a58e | 1237.39 ms | 1265.63 ms | 28.23 ms |
d10ae0c | 1250.02 ms | 1253.74 ms | 3.72 ms |
0559a8f | 1239.78 ms | 1252.60 ms | 12.83 ms |
dcec216 | 1238.94 ms | 1261.06 ms | 22.12 ms |
4bca912 | 1252.42 ms | 1260.06 ms | 7.64 ms |
42ef6ba | 1234.35 ms | 1252.29 ms | 17.94 ms |
ca91a5c | 1234.53 ms | 1249.86 ms | 15.33 ms |
aeec206 | 1211.31 ms | 1229.18 ms | 17.87 ms |
83887af | 1196.94 ms | 1206.82 ms | 9.88 ms |
4259afd | 1222.12 ms | 1249.74 ms | 27.62 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
4b1a58e | 22.85 KiB | 407.02 KiB | 384.17 KiB |
d10ae0c | 20.76 KiB | 419.86 KiB | 399.10 KiB |
0559a8f | 21.58 KiB | 419.81 KiB | 398.23 KiB |
dcec216 | 20.76 KiB | 432.88 KiB | 412.11 KiB |
4bca912 | 22.85 KiB | 411.14 KiB | 388.29 KiB |
42ef6ba | 21.58 KiB | 417.86 KiB | 396.28 KiB |
ca91a5c | 22.84 KiB | 403.19 KiB | 380.34 KiB |
aeec206 | 20.76 KiB | 434.88 KiB | 414.12 KiB |
83887af | 21.58 KiB | 419.64 KiB | 398.06 KiB |
4259afd | 20.76 KiB | 419.70 KiB | 398.94 KiB |
…continuous-profiling/2-privatize-existing-api
…xisting-api' into armcknight/feat/3555-continuous-profiling/3-new-public-api
…continuous-profiling/3-new-public-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
And thread it through to a stub for new implementation for the continuous profiler.
Add a check for where the legacy profiler is started from SentryTracer for the new feature flag.
TODO
- [ ] Haven't decided what to do yet with the launch profiler; I think it will also need the feature flag at the spots that automatically stop the tracer managing it, so it is left running throughout the app session or until SentrySDK.stopProfiler is called.- [ ] just learned (April 5) that we will automatically start profiling from SentrySDK.startWithOptions (if launch profiling isn't enabled) which will respect profiles sample rate, so this needs to be added here or in a subsequent PRI am just going to leave this PR as the stub for the new continuous profiler and leave it to a future PR to deliver the implementation of automatic/manual start/stop.
#skip-changelog
for #3555