feat: Allow IScopeObserver to observe trace
#4026
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
bruno-garcia
reviewed
Mar 8, 2025
bitsandfoxes
commented
Mar 28, 2025
bitsandfoxes
force-pushed
the
feat/observe-trace
branch
from
77cfd29 to
2df0cd9
Compare
bruno-garcia
approved these changes
Apr 1, 2025
There was a problem hiding this comment.
I'm approving to unblock but give @jamescrosswell and @aritchie a chance to review this if they have time in the next day or two unless you need this out there asap
|
|
||
| public void SetTrace(SentryId traceId, SpanId parentSpanId) | ||
| { | ||
| // TODO: This requires sentry-java 8.4.0 |
There was a problem hiding this comment.
Do we need to follow up later with this once this is added to the Java and Apple SDKs?
If so ideally we create a ticket so folks here can add it later
| @@ -16,8 +16,14 @@ | |||
|
|
|||
| <PropertyGroup Condition="'$(SolutionName)' == 'Sentry.Unity'"> | |||
| <TargetFrameworks>netstandard2.0;netstandard2.1</TargetFrameworks> | |||
| <!-- To be able to make the internals visible, since we're not signing the assembly for Unity --> | |||
| <SignAssembly>false</SignAssembly> | |||
There was a problem hiding this comment.
Does this affect things like assembly aliasing? Since before we did have signing and now we wouldn't?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Problem
Currently, errors on different layers - i.e. Unity C# -> Java -> C/C++ are not connected via trace ID. The Java and the native SDK allow for Hybrid SDKs to
SetTrace(SentryId traceId, SpanId parentSpanId). From the Unity SDK's point of view, there is no mechanism right now that allows the .NET SDK to automatically propagate the trace to the native layers.Proposal
Since the trace is held on the scope's PropagationContext we can leverage the already existing TwP functionality. Additionally, we can use the
IScopeObserverto "observe" the relevantTraceIDandParentSpanID.Allow the Unity SDK to
SetTraceThere are multiple ways I can think of achieving this:
MakePropagationContextpublicAdd an accessors to theScopeto update the `PropagationContextAdd anInternalSentrySdkclass where we can putSetTrace(and other things that regular users are not supposed to go). We have similar solutions likePrivateSentrySDKOnlyfor Cocoa andInternalSentrySdkin Java.InternalsVisibletoSentry.Unity.Update the
IScopeObserverThis
IScopeObservercan then be implemented by the respective native scope observer on the Unity SDK.getsentry/sentry-unity#1997
This will also work for Android and iOS when building MAUI apps since. We'll just have to bump the native SDKs there.
Result
Unity SDK
The end result looks like this
.NET SDK
This also works on platforms where the .NET SDK brings support via
sentry-native.Since the SDK also has observers for iOS and Android, once those SDKs are updated, it'll work there as well.
Context
Relates to getsentry/sentry-unity#1997
Relevant change to
sentry-java: getsentry/sentry-java#4137Relevant change to
sentry-native: getsentry/sentry-native#1137