-
-
Notifications
You must be signed in to change notification settings - Fork 371
feat: Expose attachment type on SentryAttachment #6521
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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6521 +/- ##
=============================================
+ Coverage 85.443% 85.666% +0.222%
=============================================
Files 451 452 +1
Lines 27424 27655 +231
Branches 11933 12122 +189
=============================================
+ Hits 23432 23691 +259
+ Misses 3948 3917 -31
- Partials 44 47 +3 see 145 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fccc4e5 | 1237.80 ms | 1264.16 ms | 26.37 ms |
| d157d83 | 1228.02 ms | 1252.47 ms | 24.45 ms |
| 570f725 | 1206.00 ms | 1238.96 ms | 32.96 ms |
| 649265b | 1235.16 ms | 1264.59 ms | 29.43 ms |
| 3319d58 | 1226.46 ms | 1256.56 ms | 30.10 ms |
| c8dd5e4 | 1217.67 ms | 1242.90 ms | 25.23 ms |
| 4267263 | 1233.13 ms | 1266.13 ms | 33.00 ms |
| d637379 | 1226.43 ms | 1250.77 ms | 24.34 ms |
| d3e7aa6 | 1226.06 ms | 1248.87 ms | 22.81 ms |
| 7aaa0b6 | 1230.49 ms | 1263.78 ms | 33.29 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| fccc4e5 | 23.75 KiB | 974.94 KiB | 951.19 KiB |
| d157d83 | 23.75 KiB | 928.85 KiB | 905.10 KiB |
| 570f725 | 23.74 KiB | 913.38 KiB | 889.63 KiB |
| 649265b | 23.75 KiB | 980.81 KiB | 957.06 KiB |
| 3319d58 | 23.75 KiB | 1021.48 KiB | 997.73 KiB |
| c8dd5e4 | 23.75 KiB | 913.48 KiB | 889.72 KiB |
| 4267263 | 23.75 KiB | 988.03 KiB | 964.28 KiB |
| d637379 | 23.75 KiB | 855.38 KiB | 831.63 KiB |
| d3e7aa6 | 23.75 KiB | 913.16 KiB | 889.41 KiB |
| 7aaa0b6 | 23.75 KiB | 989.97 KiB | 966.23 KiB |
Previous results on branch: itay/view_hierarchy_downstream_2
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 331866b | 1226.74 ms | 1261.31 ms | 34.57 ms |
| ada6c87 | 1191.10 ms | 1216.51 ms | 25.41 ms |
| 658951f | 1222.57 ms | 1261.89 ms | 39.32 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 331866b | 23.75 KiB | 1.00 MiB | 1005.94 KiB |
| ada6c87 | 23.75 KiB | 1.01 MiB | 1016.25 KiB |
| 658951f | 23.75 KiB | 1.02 MiB | 1016.47 KiB |
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.
Is it not entirely possible to achieve the same behavior today without adding to this private API? You can call SentrySDK.configureScope { ... } Wouldn't that allow the Godot SDK to do the same thing?
IMO we should try to have fewer hybrid SDK specific APIs, and move towards a version where anything that needs to be visible to an SDK should be part of the public API. So my suggestion is just to double check, do we really need to add an API for this?
The current blocker is SentryAttachment's attachmentType being private only |
|
Makes sense thanks for explaining @itaybre Maybe we should make the attachment type property visible to hybrid SDKs, or visible to everyone? That way the extra logic that would never get called from regular users of the cocoa SDK doesn't have to be included in every native app and can instead be in the Godot repo, seems more scalable/better abstracted that way |
We didn't decide on this yet, but I think we should come up with proper APIs also for hybrid. This visible only to hybrid only causes problems. We quite often break them. We should treat hybrid as any other SDK user. So IMO we should make the things they need public with a proper API and only change these APIs when doing a major. |
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 @itaybre.
This PR adds a new method in PrivateSentrySDKOnly for SDKs which use our SDK can add a custom view hierarchy.
Fixes: #6491