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 profile serialization schema #3932

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented May 2, 2024

#skip-changelog, for #3555

Implements the necessary schema changes for how continuous profiling chunks will be serialized, and their envelope structure. This was cribbed from the current functions that do the same for legacy profiles (sentry_serializedProfileDataLegacy and sentry_profileEnvelopeItemLegacy) to create sentry_serializedContinuousProfileChunk and sentry_continuousProfileChunkEnvelope.

Also implements the timer logic to transmit those envelopes every time it fires in SentryProfiler.mm.

Added assertions for the new schema to SentryContinuousProfilerTests.swift. Those tests will simulate sending multiple chunks per test, validating the contents of each, which have unique data per chunk to ensure the same data isn't carrying over, and that the profiler is still running after transmitting each until the profiler is explicitly stopped.

…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
@armcknight armcknight requested a review from brustolin as a code owner May 2, 2024 22:24
@armcknight armcknight marked this pull request as draft May 2, 2024 22:24
Copy link

github-actions bot commented May 2, 2024

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

Generated by 🚫 dangerJS against 122be7a

Copy link

github-actions bot commented May 2, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1222.35 ms 1231.27 ms 8.92 ms
Size 21.58 KiB 629.71 KiB 608.13 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
7bb0873 1360.94 ms 1362.24 ms 1.30 ms
dc0db9e 1222.10 ms 1240.90 ms 18.80 ms
5e65d2b 1216.08 ms 1236.08 ms 20.00 ms
62c15d4 1231.80 ms 1248.86 ms 17.06 ms
b35ccd0 1238.67 ms 1260.35 ms 21.67 ms
98713e6 1211.48 ms 1244.42 ms 32.94 ms
3f366ee 1244.49 ms 1257.28 ms 12.79 ms
b2f82fa 1237.78 ms 1256.02 ms 18.24 ms
42ef6ba 1219.58 ms 1245.37 ms 25.78 ms
3db3e35 1248.02 ms 1258.35 ms 10.33 ms

App size

Revision Plain With Sentry Diff
7bb0873 22.85 KiB 407.09 KiB 384.24 KiB
dc0db9e 20.76 KiB 419.62 KiB 398.86 KiB
5e65d2b 21.58 KiB 616.67 KiB 595.09 KiB
62c15d4 22.85 KiB 411.14 KiB 388.29 KiB
b35ccd0 21.58 KiB 573.13 KiB 551.55 KiB
98713e6 20.76 KiB 435.22 KiB 414.46 KiB
3f366ee 20.76 KiB 427.84 KiB 407.08 KiB
b2f82fa 20.76 KiB 419.62 KiB 398.86 KiB
42ef6ba 21.58 KiB 417.86 KiB 396.28 KiB
3db3e35 21.58 KiB 419.21 KiB 397.63 KiB

Previous results on branch: armcknight/feat/3555-continuous-profiling/5-implementation/2-schema-serialization

Startup times

Revision Plain With Sentry Diff
d9b4c67 1239.04 ms 1253.81 ms 14.77 ms
e78fbaf 1234.37 ms 1249.00 ms 14.63 ms
33bd2ca 1227.50 ms 1245.12 ms 17.62 ms

App size

Revision Plain With Sentry Diff
d9b4c67 21.58 KiB 620.30 KiB 598.72 KiB
e78fbaf 21.58 KiB 620.26 KiB 598.68 KiB
33bd2ca 21.58 KiB 620.40 KiB 598.82 KiB

Copy link

codecov bot commented May 4, 2024

Codecov Report

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

Project coverage is 90.855%. Comparing base (e072ad1) to head (122be7a).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3932       +/-   ##
=============================================
+ Coverage   90.844%   90.855%   +0.010%     
=============================================
  Files          593       594        +1     
  Lines        45974     46290      +316     
  Branches     16383     16554      +171     
=============================================
+ Hits         41765     42057      +292     
- Misses        4139      4163       +24     
  Partials        70        70               
Files Coverage Δ
Sources/Sentry/PrivateSentrySDKOnly.mm 91.549% <100.000%> (ø)
Sources/Sentry/Profiling/SentryLegacyProfiler.mm 95.000% <ø> (ø)
Sources/Sentry/Profiling/SentryProfilerState.mm 99.264% <100.000%> (+0.022%) ⬆️
Sources/Sentry/SentryBacktrace.cpp 89.705% <ø> (-0.150%) ⬇️
Sources/Sentry/SentryDataCategoryMapper.m 97.142% <100.000%> (+0.267%) ⬆️
Sources/Sentry/SentryTracer.m 96.889% <100.000%> (+0.239%) ⬆️
...yProfilerTests/SentryContinuousProfilerTests.swift 100.000% <100.000%> (ø)
...entryProfilerTests/SentryLegacyProfilerTests.swift 98.333% <100.000%> (ø)
...SentryProfilerTests/SentryProfileTestFixture.swift 99.397% <100.000%> (-0.018%) ⬇️
Tests/SentryProfilerTests/SentryProfilerTests.mm 68.000% <100.000%> (ø)
... and 5 more

... and 22 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 e072ad1...122be7a. Read the comment docs.

Base automatically changed from armcknight/feat/3555-continuous-profiling/5-implementation/1-continuous-profiling to main May 8, 2024 18:56
…continuous-profiling/5-implementation/2-schema-serialization
@armcknight armcknight marked this pull request as ready for review May 8, 2024 19:04
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.

A couple of issues, but looking good already 👍

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, thanks 👍

@armcknight armcknight merged commit c838244 into main May 14, 2024
69 of 70 checks passed
@armcknight armcknight deleted the armcknight/feat/3555-continuous-profiling/5-implementation/2-schema-serialization branch May 14, 2024 19:28
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.

3 participants