fix: Use different fallback for when MetricKit does not have file path#7473
fix: Use different fallback for when MetricKit does not have file path#7473noahsmartin merged 2 commits intomainfrom
Conversation
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Bug Fixes 🐛
Internal Changes 🔧Deps
Other
Other
🤖 This preview updates automatically when you update the PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7473 +/- ##
=============================================
+ Coverage 85.240% 85.269% +0.029%
=============================================
Files 479 479
Lines 28591 28600 +9
Branches 12421 12422 +1
=============================================
+ Hits 24371 24387 +16
+ Misses 4174 4166 -8
- Partials 46 47 +1
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
382d56d to
0b34ddb
Compare
itaybre
left a comment
There was a problem hiding this comment.
After discussing with @noahsmartin, this LGTM
0b34ddb to
91e119a
Compare
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d0b4402 | 1218.62 ms | 1241.80 ms | 23.18 ms |
| ffe0649 | 1213.35 ms | 1248.64 ms | 35.29 ms |
| 1a887e2 | 1212.46 ms | 1241.33 ms | 28.87 ms |
| 5ca545a | 1219.06 ms | 1244.59 ms | 25.53 ms |
| d69379c | 1211.30 ms | 1234.77 ms | 23.47 ms |
| a1a9260 | 1192.15 ms | 1229.80 ms | 37.64 ms |
| d492bc8 | 1214.12 ms | 1242.19 ms | 28.06 ms |
| 9a34176 | 1227.24 ms | 1266.22 ms | 38.98 ms |
| 680923d | 1211.40 ms | 1241.00 ms | 29.60 ms |
| db9e223 | 1193.69 ms | 1213.56 ms | 19.87 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d0b4402 | 24.14 KiB | 1.11 MiB | 1.08 MiB |
| ffe0649 | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| 1a887e2 | 24.14 KiB | 1.09 MiB | 1.07 MiB |
| 5ca545a | 24.14 KiB | 1.06 MiB | 1.04 MiB |
| d69379c | 24.14 KiB | 1.11 MiB | 1.09 MiB |
| a1a9260 | 24.14 KiB | 1.08 MiB | 1.06 MiB |
| d492bc8 | 24.14 KiB | 1.11 MiB | 1.09 MiB |
| 9a34176 | 24.14 KiB | 1.10 MiB | 1.07 MiB |
| 680923d | 24.14 KiB | 1.10 MiB | 1.08 MiB |
| db9e223 | 24.14 KiB | 1.06 MiB | 1.03 MiB |
91e119a to
8df8b8a
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where MetricKit sometimes provides only a binaryUUID without an image name (binaryName), causing the in-app frame detection logic to fail. The fix changes the default fallback behavior from inApp: false to inApp: true when the package (binaryName) is nil, based on the observation that MetricKit only does this for in-app code, not system code.
Changes:
- Modified inApp logic in
SentryMXCallStackTree+Parsing.swiftto default totruewhen package is nil - Added test case to verify the new behavior when package is nil
- Added corresponding test resource JSON file for the nil package scenario
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Sources/Swift/Core/MetricKit/SentryMXCallStackTree+Parsing.swift | Updated inApp assignment logic to handle nil package with a fallback to true instead of false |
| Tests/SentryTests/Integrations/MetricKit/SentryMXCallStackTreeTests.swift | Added test case testInAppTrue_WhenPackageIsNil to verify the new behavior |
| Tests/Resources/MetricKitCallstacks/per-thread-nil-package.json | Added test resource file representing MetricKit data with missing binaryName |
| CHANGELOG.md | Added entry documenting the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sometimes metric kit only provides the binaryUUID and not the image name. This will break the in app logic and requires a full fix on the backend getsentry/sentry#108352
In the meantime though, I noticed this only ever occurring with frames that are in the app, so we can change this default to
truefor a slight improvement. It still won't work with overrides from the default in-app behavior.