-
-
Notifications
You must be signed in to change notification settings - Fork 372
fix: Crash when initializing SentryHub manually #3374
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
Fix a crash when serializing the trace context after initializing the SentryHub manually.
Codecov Report
@@ Coverage Diff @@
## main #3374 +/- ##
=============================================
+ Coverage 89.053% 89.074% +0.021%
=============================================
Files 521 521
Lines 56045 56135 +90
Branches 20170 20212 +42
=============================================
+ Hits 49910 50002 +92
Misses 5220 5220
+ Partials 915 913 -2
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 279841c | 1250.80 ms | 1263.08 ms | 12.29 ms |
| c319795 | 1205.12 ms | 1231.20 ms | 26.08 ms |
| 89bc37d | 1228.20 ms | 1257.10 ms | 28.90 ms |
| 1437c68 | 1244.86 ms | 1254.18 ms | 9.32 ms |
| 5616e0a | 1237.00 ms | 1260.43 ms | 23.43 ms |
| 67460f4 | 1244.56 ms | 1255.96 ms | 11.40 ms |
| 3a31fc9 | 1237.35 ms | 1249.02 ms | 11.67 ms |
| 01a28a9 | 1200.78 ms | 1227.90 ms | 27.12 ms |
| 72c8d84 | 1238.96 ms | 1247.34 ms | 8.38 ms |
| e324230 | 1225.84 ms | 1250.40 ms | 24.57 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 279841c | 22.84 KiB | 403.19 KiB | 380.34 KiB |
| c319795 | 20.76 KiB | 431.99 KiB | 411.22 KiB |
| 89bc37d | 20.76 KiB | 401.53 KiB | 380.77 KiB |
| 1437c68 | 22.85 KiB | 410.96 KiB | 388.11 KiB |
| 5616e0a | 22.85 KiB | 407.45 KiB | 384.60 KiB |
| 67460f4 | 20.76 KiB | 426.15 KiB | 405.39 KiB |
| 3a31fc9 | 20.76 KiB | 414.45 KiB | 393.69 KiB |
| 01a28a9 | 22.85 KiB | 405.39 KiB | 382.54 KiB |
| 72c8d84 | 22.85 KiB | 408.88 KiB | 386.03 KiB |
| e324230 | 22.85 KiB | 408.87 KiB | 386.02 KiB |
Previous results on branch: fix/crash-when-initializing-manually
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c7bedd1 | 1223.18 ms | 1244.20 ms | 21.02 ms |
| 121d6d3 | 1211.39 ms | 1229.22 ms | 17.83 ms |
| 6ccf54e | 1238.06 ms | 1239.10 ms | 1.04 ms |
| 0519fc7 | 1237.80 ms | 1240.32 ms | 2.52 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| c7bedd1 | 22.85 KiB | 414.08 KiB | 391.23 KiB |
| 121d6d3 | 22.85 KiB | 411.85 KiB | 389.00 KiB |
| 6ccf54e | 22.85 KiB | 413.96 KiB | 391.11 KiB |
| 0519fc7 | 22.85 KiB | 414.08 KiB | 391.23 KiB |
| * @param options The current active options. | ||
| * @param userSegment You can retrieve this usually from the `scope.userObject.segment`. | ||
| */ | ||
| - (SentryTraceContext *)getTraceContext:(SentryOptions *)options |
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.
The way this is named makes me think it's retrieving something like from an ivar, but really it's creating a new thing every time it's called. Can we rename it to e.g. 'traceContextForOptions:userSegment:`?
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.
Really, I would prefer that we remove this method from SentryPropagationContext altogether and just call [[SentryTraceContext alloc] initWithTraceId:...] in the places where this method is called, I don't understand why this should be wrapped in SentryPropagationContext at all, the indirection at the call sites is confusing.
Now that most of the ivars are gone (except for self.traceId, which could just be accessed from the callsites and passed to the trace context init) it's just taking parameters and repackaging them.
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.
That is an excellent point. I fully I agree 🥇. I moved it.
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.
Honestly, objc is already verbose as it is, I don't think we should keeping add to it, which also increase maintenance, because if we need to change the initialisation to add/remove more attributes, we need to do it everywhere instead of one center place.
If getting traceContext from the propagation context was not good enough (because the scope is not always from the hub), we could move this function to the scope itself and get its traceContext from itself. Here is not big of a deal, this new initialisation is used only twice (for now), my fear is that we start doing this everywhere.
If the problem is the calculated property we change it to a function, no problem.
brustolin
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
armcknight
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, just needs a changelog update and one small fixup!
📜 Description
Fix a crash when serializing the trace context after initializing the SentryHub manually.
💡 Motivation and Context
A user reported a crash on Discord when running the following code
💚 How did you test it?
Unit tests.
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.🔮 Next steps