Uh oh!
There was an error while loading. Please reload this page.
ref: Use getCurrentScope() / getClient instead of hub.xxx - #9862
Conversation
| export function getTracingMetaTags(span: Span | undefined, hub: Hub): { sentryTrace: string; baggage?: string } { | ||
| const scope = hub.getScope(); | ||
| const client = hub.getClient(); | ||
| export function getTracingMetaTags( |
There was a problem hiding this comment.
@Lms24 Refactored this to take a scope & client directly, this is not exported as far as I can tell?
| const stackParser = options.stackParser; | ||
| const client = getClient(); | ||
| const options = client && client.getOptions(); | ||
| const stackParser = options && options.stackParser; |
There was a problem hiding this comment.
Refactored this a bit to be a bit more straightforward IMHO.
| function finishAndSetSpan(): void { | ||
| activeSpan && activeSpan.finish(); | ||
| hub.getScope().setSpan(parentSpan); | ||
| scope.setSpan(parentSpan); |
There was a problem hiding this comment.
@AbhiPrasad I was not 100% sure, but it is correct that we reset the span on the current scope of the hub, not the one that we started it on? 🤔 Or possibly we should reset it on both? (same for below) - wydt?
There was a problem hiding this comment.
Thinking about this a bit we should probably fork the scope inside of this function otherwise it is a bit leaky...
Besides that I think the most correct behaviour is imo to only set the span on the scope that we started on.
There was a problem hiding this comment.
jup, agree! I'll "fix" this in a follow up PR.
size-limit report 📦
|
Except for places where we are really passing a `hub` in, not using `getCurrentHub()`.
| function finishAndSetSpan(): void { | ||
| activeSpan && activeSpan.finish(); | ||
| hub.getScope().setSpan(parentSpan); | ||
| scope.setSpan(parentSpan); |
There was a problem hiding this comment.
Thinking about this a bit we should probably fork the scope inside of this function otherwise it is a bit leaky...
Besides that I think the most correct behaviour is imo to only set the span on the scope that we started on.
To align this with OpenTelemetry and make some things possible that are currently not easily doable without `pushScope` / `popScope`. Noticed this because currently it's not easily possible to e.g. use `withScope` in places like [this](#9862 (comment)). This should be backwards compatible because any code that previously relied on this returning `void` should still work.
Except for places where we are really passing a
hubin, not usinggetCurrentHub().