-
-
Notifications
You must be signed in to change notification settings - Fork 340
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
feat: Deserialize SentryBreadcrumb #4785
feat: Deserialize SentryBreadcrumb #4785
Conversation
Add Decodable/Deserializing of SentryBreadcrumb.
|
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, even though I do not fully understand the "unknown" field
When you call initWithDict on the breadcrumb class we keep unknown properties and serialize them. This gives you extra flexibility for hybrid SDKs. They could add a breadcrumb property without having the Cocoa SDK actually adding the properly. I don't see any strong use cases for this and as other classes don't have it on Cocoa, I would actually vote for removing it for simplicity. |
Did you align this with the Hybrid SDK maintainers? We should not break expected behavior. |
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
378d4bd | 1216.53 ms | 1241.19 ms | 24.66 ms |
04e8394 | 1223.24 ms | 1244.33 ms | 21.08 ms |
74e111a | 1229.31 ms | 1244.63 ms | 15.33 ms |
8dae1f6 | 1212.60 ms | 1226.52 ms | 13.93 ms |
4dc265c | 1221.31 ms | 1238.55 ms | 17.24 ms |
8a562cc | 1228.57 ms | 1245.18 ms | 16.60 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
378d4bd | 22.31 KiB | 784.05 KiB | 761.73 KiB |
04e8394 | 22.31 KiB | 771.93 KiB | 749.62 KiB |
74e111a | 22.31 KiB | 800.12 KiB | 777.80 KiB |
8dae1f6 | 22.31 KiB | 772.51 KiB | 750.20 KiB |
4dc265c | 22.31 KiB | 798.21 KiB | 775.90 KiB |
8a562cc | 22.31 KiB | 773.33 KiB | 751.02 KiB |
…ry-breadcrumb # Conflicts: # Sentry.xcodeproj/project.pbxproj
I will do before merging #4724. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/deserializing-events #4785 +/- ##
===============================================================
+ Coverage 91.435% 91.439% +0.004%
===============================================================
Files 642 643 +1
Lines 75593 75652 +59
Branches 26542 26558 +16
===============================================================
+ Hits 69119 69176 +57
- Misses 6379 6382 +3
+ Partials 95 94 -1
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
d408cb1
into
feat/deserializing-events
Add Decodable/Deserializing of SentryBreadcrumb.
This PR is based on #4724.
According to @denrase he added unknown in #2820 because we have this pattern also in Java and Dart. We don't have this pattern anywhere else in Cocoa. So, IMO we should either remove it for SentryBreadcrumb or also add it for all other classes. This decision shouldn't block this PR at the moment. I added an item to #4724, so we don't forget about it.
#skip-changelog