ref: Make dynamic sampling context mutable #5710
Conversation
size-limit report 📦
|
…no sentry baggage
There was a problem hiding this comment.
We'll have to take care that this won't affect https://github.com/getsentry/sentry-react-native - @krystofwoldrich mind taking a look at this to see if any APIs we are changing will affect y'all?
I'll keep reading this through, gimme like an hour to think through it all
| initialProps._sentryTraceData = requestTransaction.toTraceparent(); | ||
| initialProps._sentryBaggage = serializeBaggage(requestTransaction.getBaggage()); | ||
| initialProps._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); |
There was a problem hiding this comment.
This logic is repeated 4x - can we extract into a helper func? Can do this in another PR.
There was a problem hiding this comment.
We intentionally kept it this way for the ongoing Next.js changes and said we were gonna extract duplicate logic later, since we don't know yet if extracting it now will complicate things
|
|
||
| const newTransaction = startTransaction({ | ||
| op: 'nextjs.data.server', | ||
| name: options.requestedRouteName, | ||
| ...traceparentData, | ||
| metadata: { | ||
| source: 'route', | ||
| baggage, | ||
| dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, |
There was a problem hiding this comment.
if we have an invalid traceparentData, do we want to even include a dynamicSamplingContext? Setting the dynamicSamplingContext to be an empty object will freeze this, which doesn't make a ton of sense to me.
We could either:
a) not freeze baggage if it comes in as an empty object (invalid - didn't have a trace_id or public_key)
b) make sure that we only define dynamicSamplingContext if traceparentData is defined. This means we still have the possibility of an empty object, but should be fine since that means the parent sdk just messed something up.
I'm leaning toward b - so I would do something like so:
...(traceparentData && dynamicSamplingContext && { dynamicSamplingContext }),We could also construct a helper for this, since we repeat this logic in some other places.
There was a problem hiding this comment.
The reason for this ternary here was to freeze baggage if there is a sentry-trace header, but no baggage header. The freezing is intentional and to handle requests of old SDKs correctly. I confirmed with Jan that we want to keep this behavior. Sadly, this means we can't do the thing you suggested.
In normal circumstances, it shouldn't happen that there is a baggage header with sentry- values but no sentry-trace header. This is why I didn't put any handling for that case here.
Do you have any other thoughts regarding this, otherwise I would leave it as is. Having a helper seems overkill to me for a simple if statement (even though I admit it is repeated in a lot of places).
What I can do, is add a comment where we do this to explain that DSC needs to be frozen when there is trace parent data. Wdyt?
There was a problem hiding this comment.
The reason for this ternary here was to freeze baggage if there is a sentry-trace header, but no baggage header
Ahhhh I see - this makes sense. Let me try playing around with the helper to clean it up a little.
There was a problem hiding this comment.
Ok I give up - I think we keep this for now then.
Ref: #5679
This PR makes Dynamic Sampling Context in Head-SDKs mutable.
List of changes:
Baggageabstraction (keeping track of foreign baggage and mutability is not necessary)Baggageabstraction with functions that convertbaggageheaders into DSC and vice-versagetBaggage()method on theTransactionclass withgetDynamicSamplingContext()which doesn't freeze DSC upon retrievalbaggageitems so that we're (almost) always adding an additional independentbaggageheader, in addition to any that were already there. This was done to reduce code complexity and bundle size. An exception here is in the browser where there is a case where I don't know if adding an additional header might break things.transactionContext.metadata.dynamicSamplingContext- used in non-head-SDKsextractTraceparentDataso that it returnsundefined(ie. "invalid") when passed an empty string - also added a test for that.Resolves #5652