-
-
Notifications
You must be signed in to change notification settings - Fork 207
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: Tracing without performance #2493
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Tracing without performance ([#2493](https://github.com/getsentry/sentry-dotnet/pull/2493)) If none of the above apply, you can opt out of this check by adding |
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.
Great work! left some notes
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.
Looking good! Not too much to add to the comments Bruno left already.
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
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 didn't do a full review - just the files that have changed since I last checked a few days ago. All looking good.
My only question is what we plan to do with the EnableTracing option.
Otherwise, once the Check failures from CI are addressed, I think it's looking good.
string? operation = null) | ||
{ | ||
// Transactions from DisabledHub are always sampled out | ||
return new TransactionContext(string.Empty, string.Empty, false); |
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 think we could do the same thing we've done for the DisabledHub.Instance
here, to avoid creating a new object every time this method is called.
Not sure where to put that... maybe TransactionContext.Disabled
or TransactionContext.DisabledInstance
?
Resolves #2447
Implementations in other SDKs:
The idea here is:
The scope now has a
SentryPropagationContext
. The context contains minimally atraceID
and aspanID
.ContinueTrace
overwrites the context on the current scope, making it possible to propagate the IDs without the performance feature enabled.Relevant top-level methods:
GetTraceHeader
,GetBaggage
. The methods first look for active spans before falling back to creating their return value from the PropagationContext. The same is true forCaptureEvent
where we check for linked or active spans first.