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

chore(profiling): place legacy compilation gates around profiling code that will need to be changed/removed #3808

Closed
wants to merge 1 commit into from

Conversation

armcknight
Copy link
Member

To kick off work for #3555, introduce two new preprocessor definitions to help transition the profiling implementation from the current form (henceforth known as "legacy") to continuous mode.

Gate declarations and implementation that will change or be removed with the legacy flag. This is not necessarily exhaustive, but serves as a guidepost for further work.

#skip-changelog

@armcknight armcknight changed the title chore: place legacy compilation gates around profiling code that will… chore: place legacy compilation gates around profiling code that will need to be changed/removed Mar 29, 2024
@armcknight armcknight changed the title chore: place legacy compilation gates around profiling code that will need to be changed/removed chore(profiling): place legacy compilation gates around profiling code that will need to be changed/removed Mar 29, 2024
Copy link

codecov bot commented Mar 29, 2024

Codecov Report

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

Project coverage is 89.541%. Comparing base (b15521e) to head (04e8a2f).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3808       +/-   ##
=============================================
- Coverage   89.588%   89.541%   -0.048%     
=============================================
  Files          560       560               
  Lines        60819     60809       -10     
  Branches     21876     21862       -14     
=============================================
- Hits         54487     54449       -38     
- Misses        5296      5318       +22     
- Partials      1036      1042        +6     
Files Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.mm 87.155% <ø> (ø)
...entry/Profiling/SentryProfiledTracerConcurrency.mm 94.736% <100.000%> (ø)
Sources/Sentry/SentryHub.m 98.613% <100.000%> (ø)
Sources/Sentry/SentryProfileTimeseries.mm 48.076% <ø> (ø)
Sources/Sentry/SentrySampling.m 93.333% <ø> (ø)
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryTracer.m 96.226% <100.000%> (-0.172%) ⬇️
Sources/Sentry/SentryTransactionContext.mm 100.000% <100.000%> (ø)
...entryProfilerTests/SentryAppLaunchProfilingTests.m 100.000% <100.000%> (ø)
Tests/SentryProfilerTests/SentryBacktraceTests.mm 94.904% <ø> (+0.099%) ⬆️
... and 8 more

... and 12 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 b15521e...04e8a2f. Read the comment docs.

Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1236.24 ms 1250.27 ms 14.02 ms
Size 21.58 KiB 573.18 KiB 551.60 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
c319795 1256.18 ms 1266.87 ms 10.69 ms
de033da 1206.55 ms 1217.08 ms 10.53 ms
97797b4 1243.47 ms 1252.84 ms 9.37 ms
d3630c3 1248.52 ms 1262.70 ms 14.18 ms
075a044 1237.26 ms 1255.36 ms 18.10 ms
ef5821b 1253.18 ms 1265.46 ms 12.28 ms
b9a9ffd 1201.61 ms 1215.06 ms 13.45 ms
e2abb0d 1235.08 ms 1257.00 ms 21.92 ms
7512778 1200.78 ms 1206.33 ms 5.55 ms
4af7581 1204.67 ms 1241.33 ms 36.66 ms

App size

Revision Plain With Sentry Diff
c319795 20.76 KiB 431.99 KiB 411.22 KiB
de033da 21.58 KiB 418.15 KiB 396.57 KiB
97797b4 22.85 KiB 408.72 KiB 385.88 KiB
d3630c3 22.84 KiB 403.19 KiB 380.34 KiB
075a044 20.76 KiB 420.41 KiB 399.65 KiB
ef5821b 21.58 KiB 414.96 KiB 393.37 KiB
b9a9ffd 21.58 KiB 418.43 KiB 396.85 KiB
e2abb0d 20.76 KiB 434.72 KiB 413.96 KiB
7512778 21.58 KiB 539.87 KiB 518.29 KiB
4af7581 22.84 KiB 401.62 KiB 378.78 KiB

philipphofmann

This comment was marked as off-topic.

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.

I don't want #3808 (review) to block this PR. If you would like to stick to compiler flags, which I don't think is a great idea, do so @armcknight.

@armcknight
Copy link
Member Author

OK, we're going to go with the feature flag approach in #3831

@armcknight armcknight closed this Apr 5, 2024
@armcknight armcknight deleted the armcknight/feat/continuous-profiling branch May 7, 2024 22:43
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