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): Add transaction.setContext
method
#6154
Conversation
This is needed for the OpenTelemetry Span Processor.
transaction.setContext
method
packages/tracing/src/transaction.ts
Outdated
// eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
delete this._contexts[key]; | ||
} else { | ||
this._contexts = { ...this._contexts, [key]: context }; |
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.
l: Is it important to make this a new object every time? Or could we just do:
this._contexts[key] = context;
? To avoid re-creating objects when we don't really need to do so.
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 copied this from
sentry-javascript/packages/core/src/scope.ts
Lines 257 to 270 in 351be06
/** | |
* @inheritDoc | |
*/ | |
public setContext(key: string, context: Context | null): this { | |
if (context === null) { | |
// eslint-disable-next-line @typescript-eslint/no-dynamic-delete | |
delete this._contexts[key]; | |
} else { | |
this._contexts = { ...this._contexts, [key]: context }; | |
} | |
this._notifyScopeListeners(); | |
return this; | |
} |
I agree though, will change. Perhaps we can update the scope file as well then?
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 that makes sense as well 👍
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.
size-limit report 📦
|
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.
LGTM! When this is merged I can adjust the SpanProcessor
to use this.
This is needed for the OpenTelemetry Span Processor. Same behaviour as
scope.setContext
here:sentry-javascript/packages/core/src/scope.ts
Lines 257 to 270 in 351be06
One thing to note, we don't want users to override
trace
context, so we set that here.