-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 additional Dynamic Sampling Context items to baggage and envelope headers #5292
Conversation
…ge and envelope headers
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.
Looking good! One minor thing and one I'm not entirely sure on.
@@ -101,27 +100,24 @@ function createEventEnvelopeHeaders( | |||
tunnel: string | undefined, | |||
dsn: DsnComponents, | |||
): EventEnvelopeHeaders { | |||
const baggage = event.contexts && (event.contexts.baggage as BaggageObj); | |||
const { environment, release, transaction, userid, usersegment } = baggage || {}; | |||
const baggage = event.sdkProcessingMetadata && event.sdkProcessingMetadata.baggage; |
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.
Yup, having this in the sdkProcessingMetadata
makes so much more sense.
packages/tracing/src/span.ts
Outdated
const { publicKey } = (client && client.getDsn()) || {}; | ||
|
||
const metadata = this.transaction && this.transaction.metadata; | ||
const sampelRate = metadata && metadata.transactionSampling && metadata.transactionSampling.rate; |
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.
s/sampelRate/sampleRate/ :D
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.
Whoops 😅
packages/hub/src/scope.ts
Outdated
// Since we're storing dynamic sampling context data in the event.sdkProcessingMetadata | ||
// field We have to re-apply it after we applied the Scope's field. | ||
// (This is because we're storing this data on the span and not on the scope) | ||
const baggage = event.sdkProcessingMetadata && event.sdkProcessingMetadata.baggage; | ||
event.sdkProcessingMetadata = this._sdkProcessingMetadata || {}; | ||
event.sdkProcessingMetadata.baggage = baggage; |
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'm wondering. This already seemed kinda sketchy before you changed it. Did this line just overwrite/wipe the entire metadata from before? Would it be an option here to just merge them instead (like we do with the breadcrumbs and basically everything else (extra, tags, user, ...) above)?
Like this for example:
event.sdkProcessingMetadata = { ...event.sdkProcessingMetadata, ...this._sdkProcessingMetadata }; // We probably want this ordering
Wdyt?
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.
Did this line just overwrite/wipe the entire metadata from before?
Yes, everything that was on event.sdkProcessingMetadata
.
I had the same feeling when writing this hack. Decided to go with it anyway because I thought that there was a reason for why we do this.
However, I just tried your suggestion and our tests still seem to pass, with it in, so I'll take it. Thanks :D
'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-samplerate=1,' + | ||
'sentry-publickey=public,sentry-traceid=', |
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.
Doesn't really matter here and it's very subjective but I personally avoid breaking up strings this way, simply because it's less "searchable". No action required though - just wanted to get this thought out ^^
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.
Yup but I also don't like super long strings that heavily exceed the characters-per-line limit. Very annoying when working with split editor windows 😅
@@ -367,15 +374,46 @@ export class Span implements SpanInterface { | |||
* | |||
* @returns modified and immutable baggage object | |||
*/ | |||
private _getBaggageWithSentryValues(baggage: Baggage = createBaggage({})): Baggage { | |||
private _populateBaggageWithSentryValues(baggage: Baggage = createBaggage({})): Baggage { | |||
// Because of a cicular dependency, we cannot import the Transaction class here, hence the type casts |
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.
As discussed: Root cause of the circular dependency is, that we have some of the Dynamic Sampling Context functionality on the Span
class, while it should probably all live on the Transaction
class.
We will clean this up in a follow-up PR so no required action on this for now.
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.
Almost done with it already. Will make a PR soon
Simplify applyToEvent logic
This PR adds several additional Dynamic Sampling Context items to be collected, populated and propagated via baggage or sent via envelope headers. The propagated/sent data now conforms with the specs proposed in getsentry/develop#613.
With this PR, the following fields were added to DSC:
Note that trace id and public key were already previously sent with envelope headers but the SDK would send its own values instead of the trace's values in case it is not the head-of-trace SDK. This PR also fixes that, in the sense that all
trace
envelope header data items are directly taken from the DSC we either received from an incoming baggage header or from the one we populate instead.In addition to these fields, this PR makes the following changes:
event.sdkProcessingMetadata
(was previously stored inevent.contexts.baggage
which was wrong, caused a bug and caused the baggage to even be sent to Sentry (which we do not want in this form)._getBaggageDataWithSentryValues
to_populateBaggageDataWithSentryValues
because it better describes the purpose of the functionThere are still more open tasks to address to improve DSC propagation (e.g. new handling of 3rd party baggage, adjusting naming to DSC rather than baggage) but this will be done in a follow-up PR.
Fixes #5290