Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ref(profiling): public methods for new API #4995

Open
wants to merge 13 commits into
base: armcknight/git-chain/profiling/new-continuous-apis/6-launch-profiling
Choose a base branch
from

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Mar 18, 2025

per some discussion about API terminology, we will not introduce new methods named startProfileSession/stopProfileSession, instead continuing with startProfiler/stopProfiler. also explain session a bit better in the SentryProfileOptions headerdocs

#skip-changelog; for #4851

@armcknight armcknight changed the title Armcknight/profiling/new continuous apis/7 combine public methods ref(profiling): public methods for new API Mar 18, 2025
Copy link

codecov bot commented Mar 18, 2025

Codecov Report

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

Project coverage is 92.609%. Comparing base (a65fcde) to head (d1dfcd0).

Files with missing lines Patch % Lines
Sources/Sentry/SentrySDK.m 75.000% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                                             Coverage Diff                                              @@
##           armcknight/git-chain/profiling/new-continuous-apis/6-launch-profiling     #4995        +/-   ##
============================================================================================================
+ Coverage                                                                  8.901%   92.609%   +83.707%     
============================================================================================================
  Files                                                                        358       673       +315     
  Lines                                                                      25680     82876     +57196     
  Branches                                                                      94     30021     +29927     
============================================================================================================
+ Hits                                                                        2286     76751     +74465     
+ Misses                                                                     23394      6033     -17361     
- Partials                                                                       0        92        +92     
Files with missing lines Coverage Δ
...ntegrations/Performance/SentryProfileOptions.swift 69.230% <ø> (+69.230%) ⬆️
...ts/SentryAppStartProfilingConfigurationTests.swift 72.826% <ø> (ø)
...yProfilerTests/SentryProfilingPublicAPITests.swift 99.261% <100.000%> (ø)
Sources/Sentry/SentrySDK.m 90.190% <75.000%> (+81.658%) ⬆️

... and 661 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 a65fcde...d1dfcd0. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

github-actions bot commented Mar 18, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1221.33 ms 1243.73 ms 22.41 ms
Size 22.30 KiB 843.38 KiB 821.07 KiB

Baseline results on branch: armcknight/git-chain/profiling/new-continuous-apis/6-launch-profiling

Startup times

Revision Plain With Sentry Diff
2fd0f0e 1225.94 ms 1247.47 ms 21.53 ms
cf7f7c2 1215.34 ms 1239.89 ms 24.55 ms
9839a68 1228.24 ms 1247.98 ms 19.73 ms

App size

Revision Plain With Sentry Diff
2fd0f0e 22.30 KiB 842.05 KiB 819.75 KiB
cf7f7c2 22.30 KiB 842.86 KiB 820.56 KiB
9839a68 22.30 KiB 843.83 KiB 821.52 KiB

Previous results on branch: armcknight/profiling/new-continuous-apis/7-combine-public-methods

Startup times

Revision Plain With Sentry Diff
541710b 1219.14 ms 1238.58 ms 19.44 ms
784d936 1219.02 ms 1251.24 ms 32.22 ms
7edc56a 1224.47 ms 1250.91 ms 26.45 ms
7606482 1229.80 ms 1250.92 ms 21.12 ms

App size

Revision Plain With Sentry Diff
541710b 22.30 KiB 843.37 KiB 821.07 KiB
784d936 22.30 KiB 841.48 KiB 819.18 KiB
7edc56a 22.30 KiB 843.38 KiB 821.07 KiB
7606482 22.30 KiB 841.48 KiB 819.17 KiB

@armcknight armcknight force-pushed the armcknight/profiling/new-continuous-apis/7-combine-public-methods branch from 49166aa to 0d7a452 Compare March 19, 2025 01:39
@armcknight armcknight marked this pull request as ready for review March 19, 2025 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

> - `SentryOptions.profilesSampler`
> - `SentryOptions.enableLaunchProfiling`
>
> Additionally, note that the behavior of `SentrySDK.startProfiler()` will change once the above API are removed, as follows: before adding the new configuration API (`SentryProfileOptions`), `SentrySDK.startProfiler()` would unconditionally start a continuous profile if both `SentryOptions.profilesSampleRate` and `SentryOptions.profilesSampler` were `nil`, or no-op if either was non-`nil` (meaning the SDK would operate under original, transaction-based, profiling model). In the next major version, `SentryOptions.profilesSampleRate` and `SentryOptions.profilesSampler` will be removed, and `SentrySDK.startProfile()` will become a no-op unless you configure `SentryProfileOptions.sessionSampleRate` to a value greater than zero (which is it's default). If you already have calls to `SentrySDK.startProfiler()` in your code, ensure you properly configure `SentryProfileOptions` via `SentryOptions.configureProfiling` to avoid losing profiling coverage.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: Two small typos

Suggested change
> Additionally, note that the behavior of `SentrySDK.startProfiler()` will change once the above API are removed, as follows: before adding the new configuration API (`SentryProfileOptions`), `SentrySDK.startProfiler()` would unconditionally start a continuous profile if both `SentryOptions.profilesSampleRate` and `SentryOptions.profilesSampler` were `nil`, or no-op if either was non-`nil` (meaning the SDK would operate under original, transaction-based, profiling model). In the next major version, `SentryOptions.profilesSampleRate` and `SentryOptions.profilesSampler` will be removed, and `SentrySDK.startProfile()` will become a no-op unless you configure `SentryProfileOptions.sessionSampleRate` to a value greater than zero (which is it's default). If you already have calls to `SentrySDK.startProfiler()` in your code, ensure you properly configure `SentryProfileOptions` via `SentryOptions.configureProfiling` to avoid losing profiling coverage.
> Additionally, note that the behavior of `SentrySDK.startProfiler()` will change once the above APIs are removed, as follows: before adding the new configuration API (`SentryProfileOptions`), `SentrySDK.startProfiler()` would unconditionally start a continuous profile if both `SentryOptions.profilesSampleRate` and `SentryOptions.profilesSampler` were `nil`, or no-op if either was non-`nil` (meaning the SDK would operate under original, transaction-based, profiling model). In the next major version, `SentryOptions.profilesSampleRate` and `SentryOptions.profilesSampler` will be removed, and `SentrySDK.startProfile()` will become a no-op unless you configure `SentryProfileOptions.sessionSampleRate` to a value greater than zero (which is its default). If you already have calls to `SentrySDK.startProfiler()` in your code, ensure you properly configure `SentryProfileOptions` via `SentryOptions.configureProfiling` to avoid losing profiling coverage.


// MARK: continuous profiling v2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes V1 and V2 not old and new 🙏 😃

@armcknight armcknight force-pushed the armcknight/git-chain/profiling/new-continuous-apis/6-launch-profiling branch from 7b6e093 to a65fcde Compare March 21, 2025 08:34
@armcknight armcknight force-pushed the armcknight/profiling/new-continuous-apis/7-combine-public-methods branch from 5a35e76 to d1dfcd0 Compare March 21, 2025 08:34
…unch-profiling' into armcknight/profiling/new-continuous-apis/7-combine-public-methods
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants