-
-
Notifications
You must be signed in to change notification settings - Fork 345
feat: Allow Hybrid SDKs to setTrace
#5081
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
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c59914b | 1342.08 ms | 1375.73 ms | 33.65 ms |
89491ad | 1222.12 ms | 1231.96 ms | 9.83 ms |
fc163f5 | 1198.05 ms | 1227.76 ms | 29.71 ms |
e324230 | 1230.35 ms | 1236.45 ms | 6.10 ms |
983de17 | 1260.57 ms | 1263.68 ms | 3.11 ms |
1ce939e | 1222.22 ms | 1243.26 ms | 21.04 ms |
a9ac2b2 | 1241.22 ms | 1256.50 ms | 15.28 ms |
6d541bc | 1223.06 ms | 1242.38 ms | 19.32 ms |
8f397a7 | 1230.10 ms | 1253.88 ms | 23.77 ms |
9f57953 | 1232.83 ms | 1249.87 ms | 17.04 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c59914b | 21.58 KiB | 671.90 KiB | 650.32 KiB |
89491ad | 21.58 KiB | 417.89 KiB | 396.30 KiB |
fc163f5 | 20.76 KiB | 436.30 KiB | 415.54 KiB |
e324230 | 22.85 KiB | 408.87 KiB | 386.02 KiB |
983de17 | 22.84 KiB | 403.19 KiB | 380.34 KiB |
1ce939e | 22.85 KiB | 412.98 KiB | 390.13 KiB |
a9ac2b2 | 21.58 KiB | 631.19 KiB | 609.61 KiB |
6d541bc | 21.58 KiB | 616.71 KiB | 595.13 KiB |
8f397a7 | 20.76 KiB | 420.55 KiB | 399.79 KiB |
9f57953 | 22.31 KiB | 773.84 KiB | 751.53 KiB |
Previous results on branch: feat/hybrid-sdk-set-trace
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d88b671 | 1210.06 ms | 1230.88 ms | 20.82 ms |
ce2eef8 | 1220.65 ms | 1242.49 ms | 21.84 ms |
921c24c | 1216.94 ms | 1239.98 ms | 23.04 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d88b671 | 22.30 KiB | 846.64 KiB | 824.33 KiB |
ce2eef8 | 22.30 KiB | 847.83 KiB | 825.53 KiB |
921c24c | 22.30 KiB | 846.63 KiB | 824.32 KiB |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5081 +/- ##
=============================================
+ Coverage 92.720% 92.738% +0.018%
=============================================
Files 674 675 +1
Lines 83522 83719 +197
Branches 30443 30488 +45
=============================================
+ Hits 77442 77640 +198
Misses 5981 5981
+ Partials 99 98 -1
... and 20 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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.
Please add tests for the SentryPropagationContext.initWithTraceId
to ensure that the traceId
and spanId
is actually set and not falling back to randomly generated ones (could be annoying to debug).
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.
Almost LGTM, please add a final test case and this should be ready for merge
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, thanks for the addition 🎉
📜 Description
This PR allows Hybrid SDKs to explicitly set the
PropagationContext
on the scope via thePrivateSentrySDKOnly
.💡 Motivation and Context
The basic idea is that the Cocoa SDK, when brought in via a Hybrid SDK, should accept a trace from the Head SDK to connect errors on all layers of the application. This PR makes use of the fact that the Hybrid SDKs rely on their own performance instrumentation right now, allowing us to leverage the propagation context to achieve this.
Originates from the Unity SDK: getsentry/sentry-unity#1998
Unblocks the React Native SDK: getsentry/sentry-react-native#3918
Mirror-PR on the Java SDK: getsentry/sentry-java#4137
💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.#skip-changelog