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

fix(profiling): prefix some functions with "sentry_" #3862

Merged
merged 7 commits into from
Apr 19, 2024

Conversation

armcknight
Copy link
Member

We should've been prefixing any functions in the global namespace to avoid potential collisions with other apps' functions. Fixes the immediate problem reported in #3856, but we may need to do more here with all other functions. These generic functions names present the most likely possibility for collisions, though, so this should practically fix most of the issue.

Closes #3856

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.

I believe prefixing is good enough, thanks for solve this one. The changes looks good to me but CI is not happy.

Copy link

codecov bot commented Apr 18, 2024

Codecov Report

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

Project coverage is 90.584%. Comparing base (953e914) to head (c111d44).
Report is 1 commits behind head on armcknight/test/fix-crash-recovery-test.

Additional details and impacted files

Impacted file tree graph

@@                              Coverage Diff                              @@
##           armcknight/test/fix-crash-recovery-test     #3862       +/-   ##
=============================================================================
+ Coverage                                   90.536%   90.584%   +0.047%     
=============================================================================
  Files                                          578       579        +1     
  Lines                                        45206     45296       +90     
  Branches                                     16070     16114       +44     
=============================================================================
+ Hits                                         40928     41031      +103     
- Misses                                        4098      4195       +97     
+ Partials                                       180        70      -110     
Files Coverage Δ
Sources/Sentry/SentryHub.m 98.533% <100.000%> (ø)
Sources/Sentry/SentryProfiler.mm 85.333% <100.000%> (ø)
Sources/Sentry/SentrySDK.m 86.470% <100.000%> (ø)
Sources/Sentry/SentrySampling.m 92.682% <ø> (ø)
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryTracer.m 96.897% <100.000%> (ø)
...entryProfilerTests/SentryAppLaunchProfilingTests.m 100.000% <100.000%> (ø)
Sources/Sentry/Profiling/SentryLaunchProfiling.m 61.842% <57.692%> (-0.825%) ⬇️

... and 35 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 953e914...c111d44. Read the comment docs.

@armcknight armcknight force-pushed the armcknight/fix/crash-naming-collision branch from adb906d to c111d44 Compare April 18, 2024 19:23
Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1228.90 ms 1247.79 ms 18.90 ms
Size 21.58 KiB 614.74 KiB 593.15 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
afce5bf 1199.94 ms 1213.27 ms 13.33 ms
4b08ceb 1237.75 ms 1249.61 ms 11.86 ms
addcf42 1247.33 ms 1253.58 ms 6.25 ms
3297d6e 1206.80 ms 1213.86 ms 7.06 ms
d60f70a 1219.63 ms 1228.54 ms 8.91 ms
9f0d9e0 1206.55 ms 1219.41 ms 12.86 ms
d8eb419 1221.91 ms 1253.62 ms 31.71 ms
340fb46 1224.60 ms 1239.27 ms 14.67 ms
1d11695 1219.57 ms 1243.52 ms 23.95 ms
e0f077c 1224.65 ms 1243.52 ms 18.87 ms

App size

Revision Plain With Sentry Diff
afce5bf 21.58 KiB 542.18 KiB 520.60 KiB
4b08ceb 20.76 KiB 431.99 KiB 411.23 KiB
addcf42 22.85 KiB 413.42 KiB 390.57 KiB
3297d6e 21.58 KiB 418.44 KiB 396.86 KiB
d60f70a 20.76 KiB 430.97 KiB 410.21 KiB
9f0d9e0 21.58 KiB 424.28 KiB 402.70 KiB
d8eb419 22.85 KiB 408.87 KiB 386.02 KiB
340fb46 21.58 KiB 418.78 KiB 397.20 KiB
1d11695 20.76 KiB 401.60 KiB 380.84 KiB
e0f077c 22.85 KiB 412.59 KiB 389.74 KiB

@armcknight armcknight changed the base branch from main to armcknight/test/fix-crash-recovery-test April 19, 2024 22:33
@armcknight armcknight changed the title fix(profiling): prefix some functions with "sentry_" and move other to a static namespace fix(profiling): prefix some functions with "sentry_" Apr 19, 2024
Base automatically changed from armcknight/test/fix-crash-recovery-test to main April 19, 2024 23:04
@armcknight armcknight merged commit 94e1968 into main Apr 19, 2024
72 of 74 checks passed
@armcknight armcknight deleted the armcknight/fix/crash-naming-collision branch April 19, 2024 23:04
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- prefix some functions with "sentry_" ([#3862](https://github.com/getsentry/sentry-cocoa/pull/3862))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against c111d44

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.

Update to 8.21.0 causes a crash in my app in runtime
4 participants