fix: Normalize profiling cpu usage to percent#7798
Conversation
Fix SentrySystemWrapper CPU usage calculation by dividing Mach thread cpu_usage by TH_USAGE_SCALE, then normalizing by processor count and converting to 0..100 percent. Updates docs and adds test for the normalization logic.
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧Deps
🤖 This preview updates automatically when you update the PR. |
denrase
requested review from
itaybre,
noahsmartin,
philipphofmann and
philprime
as code owners
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7798 +/- ##
=============================================
- Coverage 85.434% 85.433% -0.002%
=============================================
Files 487 487
Lines 29261 29265 +4
Branches 12671 12658 -13
=============================================
+ Hits 24999 25002 +3
- Misses 4212 4214 +2
+ Partials 50 49 -1
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
📲 Install BuildsiOS
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0e4b033 | 1203.74 ms | 1249.71 ms | 45.97 ms |
| bbee1ba | 1197.79 ms | 1215.42 ms | 17.63 ms |
| 59981b9 | 1207.25 ms | 1240.71 ms | 33.46 ms |
| 64a365a | 1225.60 ms | 1255.49 ms | 29.89 ms |
| 93d7fdf | 1225.77 ms | 1259.79 ms | 34.02 ms |
| e03f459 | 1222.56 ms | 1255.94 ms | 33.37 ms |
| ffac605 | 1217.10 ms | 1256.91 ms | 39.81 ms |
| 2f4ddaa | 1227.26 ms | 1260.04 ms | 32.78 ms |
| 778dadf | 1207.69 ms | 1246.09 ms | 38.40 ms |
| daf8b80 | 1233.78 ms | 1259.44 ms | 25.66 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0e4b033 | 24.14 KiB | 1.11 MiB | 1.09 MiB |
| bbee1ba | 24.14 KiB | 1.11 MiB | 1.09 MiB |
| 59981b9 | 24.14 KiB | 1.09 MiB | 1.06 MiB |
| 64a365a | 24.14 KiB | 1.09 MiB | 1.06 MiB |
| 93d7fdf | 24.14 KiB | 1.11 MiB | 1.08 MiB |
| e03f459 | 24.14 KiB | 1.11 MiB | 1.09 MiB |
| ffac605 | 24.14 KiB | 1.13 MiB | 1.10 MiB |
| 2f4ddaa | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 778dadf | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| daf8b80 | 24.14 KiB | 1.11 MiB | 1.09 MiB |
philprime
left a comment
philprime
left a comment
There was a problem hiding this comment.
LGTM but I have one concern: Isn't this technically speaking a breaking change because the reported data and any related dashboards will report completely different numbers from now on?
itaybre
left a comment
itaybre
left a comment
There was a problem hiding this comment.
@philprime concern seems valid, at this time we might not be able to update this untils a new major
|
@philprime @itaybre Should we consider it a breaking change when it was incorrect before? |
Co-authored-by: Itay Brenner <[email protected]>
If a user created a dashboard based on this, it would change behavior. So even if it was incorrect before, it is a breaking change (the dashboard wouldn't work with old and new data) |
|
@denrase after a discussion in the Team Apple SDK sync we believe that even though this is is a bug fix, it's a breaking change to any queries or dashboards built around this invalid value. Therefore we must postpone this to the next major release of v10, which is not going to happen until fall 2026 We have two options:
I believe option 2 is easier to do right now, as a conditional compilation flag would also require us to extend CI to make sure the SDK is still functional for v10 too. |
|
@philprime Sounds good to me, thank you! Closing for now and will re-visit once we have v10 out. |
3 participants
📜 Description
Fix SentrySystemWrapper CPU usage calculation by dividing Mach thread cpu_usage by TH_USAGE_SCALE, then normalizing by processor count and converting to 0..100 percent. Updates docs and adds test for the normalization logic.
💡 Motivation and Context
Closes #7456
💚 How did you test it?
Unit Tests
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.