-
-
Notifications
You must be signed in to change notification settings - Fork 363
fix: Type check SentrySpan #5982
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
b54498b
to
339fac0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5982 +/- ##
========================================
Coverage ? 86.716%
========================================
Files ? 424
Lines ? 36716
Branches ? 17366
========================================
Hits ? 31839
Misses ? 4829
Partials ? 48
Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
cc7f629 | 1226.00 ms | 1245.51 ms | 19.51 ms |
80a5166 | 1224.49 ms | 1251.29 ms | 26.80 ms |
82f60cf | 1218.65 ms | 1238.52 ms | 19.87 ms |
b9aacb6 | 1230.42 ms | 1251.00 ms | 20.58 ms |
2691350 | 1224.92 ms | 1255.82 ms | 30.90 ms |
9080e6c | 1221.17 ms | 1247.87 ms | 26.71 ms |
326984b | 1235.06 ms | 1252.75 ms | 17.69 ms |
2a36c3f | 1227.27 ms | 1246.89 ms | 19.63 ms |
916edbe | 1243.86 ms | 1256.11 ms | 12.25 ms |
324c109 | 1228.35 ms | 1252.47 ms | 24.12 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
cc7f629 | 23.75 KiB | 878.48 KiB | 854.73 KiB |
80a5166 | 23.75 KiB | 904.53 KiB | 880.78 KiB |
82f60cf | 23.75 KiB | 913.63 KiB | 889.88 KiB |
b9aacb6 | 23.75 KiB | 913.64 KiB | 889.89 KiB |
2691350 | 23.75 KiB | 850.73 KiB | 826.98 KiB |
9080e6c | 23.75 KiB | 926.80 KiB | 903.05 KiB |
326984b | 23.74 KiB | 926.64 KiB | 902.90 KiB |
2a36c3f | 23.75 KiB | 874.45 KiB | 850.71 KiB |
916edbe | 23.75 KiB | 908.41 KiB | 884.66 KiB |
324c109 | 23.75 KiB | 919.91 KiB | 896.16 KiB |
Previous results on branch: fixSentrySpanTypeChecks
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a74512d | 1219.81 ms | 1252.22 ms | 32.42 ms |
1e63689 | 1225.76 ms | 1243.73 ms | 17.97 ms |
cf7575c | 1229.65 ms | 1253.28 ms | 23.64 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a74512d | 23.75 KiB | 926.63 KiB | 902.88 KiB |
1e63689 | 23.75 KiB | 926.63 KiB | 902.88 KiB |
cf7575c | 23.75 KiB | 926.63 KiB | 902.88 KiB |
339fac0
to
e172fae
Compare
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
In a number of places took a value of type protocol SentrySpan and tried to use it as the SentrySpan class or the SentryTransaction class. ObjC doesn't really care about this, only Swift would tell you it is unsafe. ObjC seems to be particularly bad at giving a warning for this when there are multiple levels of protocols (as there are with SentrySpan and SentrySerializable).
This PR converts SentrySpan protocol in V9 to not use SentrySerializable, and instead just define the method directly. This causes the ObjC compiler to give us a warning every time we did this unsafe operation, which I've added explicit casts for. It doesn't change any behavior, but it will help us catch this next time because CI will emit an error in the V9 check, helping us to avoid introducing more of these unsafe casts.
#skip-changelog