feat(tracing): Propagate environment and release values in baggage Http headers #5193
Conversation
size-limit report 📦
|
| @@ -302,7 +311,14 @@ export class Span implements SpanInterface { | |||
| * @inheritdoc | |||
| */ | |||
| public getBaggage(): Baggage | undefined { | |||
| return this.transaction && this.transaction.metadata.baggage; | |||
| const existingBaggage = this.transaction && this.transaction.metadata.baggage; | |||
|
|
|||
There was a problem hiding this comment.
| // Add sentry baggage data to baggage, only when baggage is either empty or does not exist yet |
Took me a while to get what's happening here. Wdyt about adding a comment like this to speed things up for future readers?
| @@ -334,6 +350,24 @@ export class Span implements SpanInterface { | |||
| trace_id: this.traceId, | |||
| }); | |||
| } | |||
|
|
|||
| /** | |||
| * | |||
There was a problem hiding this comment.
I think we forgot to add a comment here ^^
| */ | ||
| private _getBaggageWithSentryValues(baggage: Baggage = createBaggage({})): Baggage { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any | ||
| const hub: Hub = ((this.transaction as any) && (this.transaction as any)._hub) || getCurrentHub(); |
There was a problem hiding this comment.
To me this seems a bit dangerous. I was wondering if we can get away with only having getCurrentHub() here, because in a node context, we'll get the right hub anyways because of domains, and in a browser context we only have one proper hub anyways (?).
I'll let you make the final call on this though. It's probably a bit dangerous but should also be fine.
There was a problem hiding this comment.
I think it should be fine if we take the Hub from the transaction. When creating a Transaction, users can pass in a hub as an option in the TransactionContext. Also for example when calling hub.startTransaction. People who use a multi-hub/client setup (which comes with its own set of issues) do this sometimes.
Do you think it's dangerous because there might be a "wrong" hub in the transaction or because of the casts?
| }, | ||
| }); | ||
| }); | ||
|
|
||
| test('Should assign `baggage` header which contains sentry trace baggage data of an outgoing request.', async () => { | ||
| test('Should assign `baggage` header which contains sentry trace baggage data to an outgoing request.', async () => { |
There was a problem hiding this comment.
Can we say in this test title explaining, that we don't overwrite baggage that's already on the outgoing request?
There was a problem hiding this comment.
Yes, thanks for bringing this up. Changed this title and the one below it
(this makes me think that I should add more tests covering different mutability cases here in #5205)
|
|
||
| describe('getBaggage and _getBaggageWithSentryValues', () => { | ||
| beforeEach(() => { | ||
| hub.getClient()!.getOptions = () => { |
There was a problem hiding this comment.
Do you think it's necessary to add a cleanup for this? Wanna avoid flakeyness in the future.
There was a problem hiding this comment.
I was thinking the same thing when I wrote the tests here but then I saw that at the top of this file there already is a beforeEach function. This means that the modifications on the hub from my beforeSend, are reverted as soon as we leave the closure and move to other tests.
Following #5133, this PR adds the
sentry-environmentandsentry-releasevalues to thebaggageHTTP headers of outgoing requests. We add these values to baggage as late as possible with the rationale that other baggage values (to be added in the future) will not be available right away, e.g. when intercepting baggage from incoming requests.As a result, the flow described below happens, when the first call to
span.getBaggageis made (e.g. in callbacks of instrumented outgoing requests):span.getBaggage, check if there is baggage present in the span (in which case it was intercepted from incoming headers/meta tags) and it is empty (or only includes 3rd party data) OR if there is no baggage yetFurthermore, the PR adds and improves tests to check correct handling and propagation of baggage
This PR does not yet sync the Http header baggage information with the one that's sent in the
traceenvelope header. This will be addressed in a follow-up PR.EDIT: As discussed with the team, this PR does not fully address all (im)mutability cases. Will open up a separate PR soon to fix this.
ref: https://getsentry.atlassian.net/browse/WEB-936