-
-
Notifications
You must be signed in to change notification settings - Fork 450
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: Allow Hybrid SDK to setTrace
#4137
Conversation
|
0c5d7ff
to
a4b2d66
Compare
sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java
Outdated
Show resolved
Hide resolved
In JS the |
sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java
Show resolved
Hide resolved
e5c09c4
to
b427553
Compare
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
setTrace
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
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.
Left some thoughts after giving this a quick look
sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java
Outdated
Show resolved
Hide resolved
final @NotNull String traceId, | ||
final @NotNull String spanId, | ||
final @Nullable Double decisionSampleRate, | ||
final @Nullable Double decisionSampleRand) { |
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.
Can we add an optional Baggage
param here as well?
I would assume we want to sync that between the SDKs as well.
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.
I don't see the need or benefit of doing that from the Unity SDK's perspective. @krystofwoldrich would this help RN?
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.
At the moment, I don't see why we would need Baggage synced as well.
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.
I believe we pass replayId via baggage to downstream services
Might need that on React Native (which has Replay)
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.
We don't need that because the ReplayID is generated on native and synced up to RN Replay Integration which adds it to DSC.
The traceID is the opposite, hybrid (RN) generates it and we need to sync it down to native.
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
Performance metrics 🚀
|
🚨 Detected changes in high risk code 🚨High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:
|
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
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!
Resolves #3562
Follows #4188
Depends on
sentry-native
release: getsentry/sentry-native#1137Corresponding PR on Unity: getsentry/sentry-unity#1997
Context
Native events and crashes do not share the same trace ID as the rest of the layers. This is relevant in two ways:
Changes
Setting the trace from hybrid SDKs
The Java SDK provides an API to allow hybrid SDKs to set a trace outside of accepting trace-header & baggage. This is done in the
InternalSentrySdk
. It accepts atrace ID
, theparent span ID
, and optionally asample rate
, and asample rand
. Assuming thatenableTraceIdGeneration
and performance is disabled, writing these to thepropagation context
allows the SDK to effectively "continue the trace".Synching
trace
to nativeThe
NDKScopeObserver
is already getting invoked on changes to the propagation context viasetTrace
. The changes in getsentry/sentry-native#1137 provide a way to set theTrace
context on the native scope.Result