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): continuous profiler basic implementation #3913

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Apr 27, 2024

For #3555; #skip-changelog

  • further refactor the current profiling implementation in order to make space for the new one as a parallel to the legacy logic
  • carry over some basic functionality from the legacy profiler controls so it can run
  • create a test case that starts and stops it to ensure that it at least doesn't crash

Starting and stopping automatically on given events, including launch profiling, will be handled in a subsequent PR.

This extracts the remaining class functions from SentryProfiler.mm to a new class SentryLegacyProfiler, to parallel the previously created and stubbed SentryContinuousProfiler.

All that then remains of SentryProfiler are the instance methods. Both the legacy and continuous classes will use an instance of SentryProfiler and control it with their respective specified behaviors. SentryProfiler is now initialized with an enum to specify whether it is running in legacy or continuous mode. It will use this to e.g. decide what to do when its timer expires (stop profiling in legacy, send a chunk in continuous–this part is also left for a later PR).

Also renamed SentryProfilerSwiftTests.swift to SentryLegacyProfilerTests and created a new SentryContinuousProfilerTests class, and factored out the fixture to a separate file in SentryProfileTestFixture.

…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
Copy link

github-actions bot commented Apr 27, 2024

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

Generated by 🚫 dangerJS against 8f6f361

Copy link

codecov bot commented Apr 27, 2024

Codecov Report

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

Project coverage is 90.806%. Comparing base (7bdf989) to head (8f6f361).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3913       +/-   ##
=============================================
- Coverage   90.806%   90.806%   -0.001%     
=============================================
  Files          588       591        +3     
  Lines        45879     45954       +75     
  Branches     16370     16410       +40     
=============================================
+ Hits         41661     41729       +68     
- Misses        4148      4155        +7     
  Partials        70        70               
Files Coverage Δ
SentryTestUtils/ClearTestState.swift 100.000% <100.000%> (ø)
Sources/Sentry/PrivateSentrySDKOnly.mm 91.549% <100.000%> (ø)
...entry/Profiling/SentryProfiledTracerConcurrency.mm 97.368% <ø> (ø)
Sources/Sentry/SentryFramesTracker.m 99.186% <100.000%> (+0.013%) ⬆️
Sources/Sentry/SentryProfiler.mm 89.600% <100.000%> (-0.144%) ⬇️
Sources/Sentry/SentryTracer.m 97.607% <100.000%> (+0.717%) ⬆️
...yProfilerTests/SentryContinuousProfilerTests.swift 100.000% <100.000%> (ø)
...entryProfilerTests/SentryLegacyProfilerTests.swift 98.333% <100.000%> (ø)
Tests/SentryProfilerTests/SentryProfilerTests.mm 68.000% <100.000%> (+2.355%) ⬆️
...ance/FramesTracking/SentryFramesTrackerTests.swift 99.491% <100.000%> (ø)
... and 3 more

... and 7 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 7bdf989...8f6f361. Read the comment docs.

Copy link

github-actions bot commented Apr 27, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1234.38 ms 1250.75 ms 16.37 ms
Size 21.58 KiB 617.83 KiB 596.25 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
eb41178 1228.06 ms 1248.37 ms 20.31 ms
25d925a 1232.89 ms 1248.41 ms 15.52 ms
ce4cfaf 1203.61 ms 1218.86 ms 15.25 ms
ee8b48f 1204.04 ms 1216.70 ms 12.66 ms
50bbebb 1234.76 ms 1246.66 ms 11.90 ms
369222e 1232.14 ms 1258.90 ms 26.76 ms
e1cd9e9 1203.93 ms 1239.12 ms 35.19 ms
83d84d7 1244.92 ms 1253.34 ms 8.42 ms
5f8ee7a 1253.96 ms 1264.98 ms 11.02 ms
9dbf743 1252.10 ms 1262.10 ms 10.00 ms

App size

Revision Plain With Sentry Diff
eb41178 21.58 KiB 544.86 KiB 523.28 KiB
25d925a 21.58 KiB 418.82 KiB 397.24 KiB
ce4cfaf 20.76 KiB 423.19 KiB 402.43 KiB
ee8b48f 21.58 KiB 418.70 KiB 397.11 KiB
50bbebb 21.58 KiB 414.96 KiB 393.38 KiB
369222e 20.76 KiB 419.67 KiB 398.91 KiB
e1cd9e9 22.85 KiB 412.95 KiB 390.10 KiB
83d84d7 22.84 KiB 402.56 KiB 379.72 KiB
5f8ee7a 22.85 KiB 411.93 KiB 389.08 KiB
9dbf743 20.76 KiB 434.94 KiB 414.18 KiB

Previous results on branch: armcknight/feat/3555-continuous-profiling/5-implementation/1-continuous-profiling

Startup times

Revision Plain With Sentry Diff
bcfe763 1238.71 ms 1252.82 ms 14.11 ms
69a92e8 1230.06 ms 1249.22 ms 19.16 ms

App size

Revision Plain With Sentry Diff
bcfe763 21.58 KiB 614.95 KiB 593.37 KiB
69a92e8 21.58 KiB 617.11 KiB 595.52 KiB

@armcknight armcknight marked this pull request as ready for review April 30, 2024 22:49
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

Looks good for me. Thanks!!

@armcknight armcknight changed the title feat(profiling): continuous profiling feat(profiling): continuous profiler basic implementation May 5, 2024
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, with mostly nitpicks.

…continuous-profiling/5-implementation/1-continuous-profiling
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

@armcknight armcknight merged commit ee8a0f4 into main May 8, 2024
69 of 70 checks passed
@armcknight armcknight deleted the armcknight/feat/3555-continuous-profiling/5-implementation/1-continuous-profiling branch May 8, 2024 18:56
threema-matteo pushed a commit to threema-ch/sentry-cocoa that referenced this pull request May 21, 2024
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.

None yet

3 participants