-
-
Notifications
You must be signed in to change notification settings - Fork 226
feat: Observe transaction on scope #4153
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
|
| if (ReferenceEquals(_transaction.Value, expectedCurrentTransaction)) | ||
| { | ||
| _transaction.Value = null; | ||
| if (Options.EnableScopeSync) |
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.
Instead, this could also look like this
internal void ResetTransaction(ITransactionTracer? expectedCurrentTransaction)
{
if (ReferenceEquals(Transaction, expectedCurrentTransaction))
{
Transaction = null;
}
}
and have the property take care of locking and synching. But I'm not fully confident in this looking at the way it's set up right originally. cc @jamescrosswell
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.
Hm, I wonder if we need the locking at all. It's an AsyncLocal<ITransactionTracer?> so in what scenarios would we see contention for reading/writing this? It has to be code from the same AsyncLocal context, which implicitly means it's on the same thread right? We shouldn't ever see multiple threads trying to access this concurrently?
@bruno-garcia sanity check?
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.
if it's always AsyncLocal we don't need any locking
| if (_transaction.Value != null) | ||
| { | ||
| // If there is a transaction set we propagate the trace to the native layer | ||
| Options.ScopeObserver?.SetTrace(_transaction.Value.TraceId, _transaction.Value.SpanId); |
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 just the TraceId and SpanId that need to be synced? Should the sample rate and sample rand also be synced (we normally put that on the DSC to ensure consistent sampling decisions)? Or will this just be used to create spans (not transactions) and so no sampling decisions need to be made?
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.
In the future, they should. But both the iOS and Android ScopeObservers are missing their implementation. See #4074
It needs bumping the native SDKs to specific versions and then mess around with the bindings (especially, since the API is in private/internal parts of the Cocoa/Java SDK)
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 raise a ticket in the relevant repos so we don't forget about this
Problem Statement
Underlying SDKs should be able to observe changes to the transaction, so they use the same trace context.
Proposal
SetTraceand call it whenever a transaction gets set/unset on the scope.