-
-
Notifications
You must be signed in to change notification settings - Fork 376
ref: Renames SentrySpan class to SentrySpanInternal
#7172
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
Conversation
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Metrics
Other
Bug Fixes 🐛
Build / dependencies / internal 🔧Deps
Other
Other
🤖 This preview updates automatically when you update the PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7172 +/- ##
=============================================
+ Coverage 84.795% 84.938% +0.143%
=============================================
Files 461 465 +4
Lines 27860 28039 +179
Branches 12324 12392 +68
=============================================
+ Hits 23624 23816 +192
+ Misses 4195 4178 -17
- Partials 41 45 +4
... and 24 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 50e7b3e | 1221.54 ms | 1250.81 ms | 29.27 ms |
| 3bf0d3f | 1202.12 ms | 1237.23 ms | 35.11 ms |
| 2f28bda | 1221.21 ms | 1255.09 ms | 33.88 ms |
| dbfeb41 | 1215.17 ms | 1237.41 ms | 22.23 ms |
| d29a425 | 1209.96 ms | 1239.00 ms | 29.04 ms |
| 2f4ddaa | 1227.26 ms | 1260.04 ms | 32.78 ms |
| 1d0feed | 1213.31 ms | 1247.44 ms | 34.14 ms |
| adeec82 | 1220.43 ms | 1254.94 ms | 34.51 ms |
| 9f7ef2b | 1213.53 ms | 1250.23 ms | 36.70 ms |
| 2e5230b | 1207.41 ms | 1240.41 ms | 33.00 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 50e7b3e | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 3bf0d3f | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 2f28bda | 24.14 KiB | 1.05 MiB | 1.03 MiB |
| dbfeb41 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| d29a425 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 2f4ddaa | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 1d0feed | 24.14 KiB | 1.05 MiB | 1.03 MiB |
| adeec82 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 9f7ef2b | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 2e5230b | 24.14 KiB | 1.04 MiB | 1.02 MiB |
philprime
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but might we worth to ask one of our coding agents to verify we caught all use cases of SentrySpan which need to be renamed
|
@sentry review |
Should be fine since if I missed any case, it shouldn't compile |
philipphofmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several replacements of id<SentrySpan> with SentrySpanInternal. I don't think these should be required.
There is no reason to use |
philipphofmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this @itaybre 💯
📜 Description
Renames the class
SentrySpantoSentrySpanInternal, keepingSentrySpanonly to the protocol.Only the protocol is a public API, so no breaking changes were made
💡 Motivation and Context
We have 2 types with identical names, one a protocol and another a class.
While converting the SentryCrashIntegration to Swift, I ran into issues with them having the same name in ObjC.
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.Closes #7176