-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Document how to continue a trace using baggage header #5617
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
TransactionNameSource.CUSTOM, | ||
"op", | ||
new SentryTraceHeader(sentryTrace), | ||
Baggage.fromHeader(baggageHeaders, hub.getOptions().getLogger()) |
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 need to expose a method fromHeader
that doesn't take a logger
.
Sentry.getCurrentHub()
is @ApiStatus.Internal
so people cannot use this.
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.
Or make it nullable, but I'd rather expose a new overload.
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.
How about passing in the options instead?
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.
That does not help either, there's no public API to retrieve the options
.
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.
Hmm so we're down to calling Sentry.getCurrentHub().getOptions().getLogger()
inside then to hide it from the user but still be able to log problems I presume.
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.
That's fine, that's what HubAdapter.getInstance()
stands for, at least to make it testable when calling internally.
I'd make a PR asap because otherwise, it's not usable, I don't expect people using Baggage.fromHeader
themselves any way, so it's not a p0 but create an issue, and don't merge this before doing it, this would just raise more questions around "how do I get the logger?".
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.
Made a couple wording edits
Co-authored-by: Isabel <[email protected]>
For people who want to manually use distributed tracing via
baggage
header.