Uh oh!
There was an error while loading. Please reload this page.
feat: Pass parentSampleRate to tracesSampler - #15024
Conversation
size-limit report 📦
|
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| spans.forEach(span => { | ||
| delete span.data[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]; | ||
| delete span.data[SEMANTIC_ATTRIBUTE_SENTRY_OVERRIDE_TRACE_SAMPLE_RATE]; | ||
| span.data && delete span.data[SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]; |
There was a problem hiding this comment.
I think we can drop this check (span.data &&), as span data is not optional anymore? 🤔
| // This attribute is important for internal logic but should not leak into actual data | ||
| // eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
| delete spanJson.data[SEMANTIC_ATTRIBUTE_SENTRY_OVERRIDE_TRACE_SAMPLE_RATE]; |
There was a problem hiding this comment.
hmmm no strong feelings, but can we move this somewhere else than here? E.g. into sentrySpan (where we already delete the name attribute) and/or the span exporter (where we already clear some data that we do not want to send)? These feel like more correct places for this than the envelope, which should rather just send what it gets, IMHO.
There was a problem hiding this comment.
I put this here because there is no other really generic flow that runs for standalone spans. This is basically one-of logic. We already delete it in sentrySpan, but sentrySpan doesn't run for standalones, neither does the span exporter in browser. Do you have any other concrete ideas?
There was a problem hiding this comment.
I could also just delete the line. We also seem to be sending other garbage attributes through standalones.
There was a problem hiding this comment.
hmm, I see 🤔 you decide, it feels a bit "weird" to do this in envelope serialization step, but also OK to me!
There was a problem hiding this comment.
I know. For the lack of better placement I'll leave it. Removing it wouldn't be great since we'd just end up forgetting to do so. It's super easy to refactor once we flesh out standalone spans.
Uh oh!
There was an error while loading. Please reload this page.
| const frozenDsc = (rootSpan as SpanWithMaybeDsc)[FROZEN_DSC_FIELD]; | ||
| if (frozenDsc) { | ||
| return frozenDsc; | ||
| return applyTraceSampleRateOverrideToDsc(frozenDsc); |
There was a problem hiding this comment.
should this not be on the frozen DSC already in this scenario?
There was a problem hiding this comment.
This is effectively what this change is about. The sample_rate becomes "unfrozen" in certain situations so no matter what the dsc may be, the sample rate that was applied to the root span wins.
| if (dscOnTraceState) { | ||
| return dscOnTraceState; | ||
| return applyTraceSampleRateOverrideToDsc(dscOnTraceState); |
There was a problem hiding this comment.
why is this not already on the frozen dsc from the trace state here?
There was a problem hiding this comment.
Because for example when the sample rate returned from a tracesSampler is different than on the incoming DSC. The sample rate returned from the tracesSampler wins.
| context: Context; | ||
| spanAttributes: SpanAttributes; | ||
| sampleRand?: number; | ||
| sampleRateOverride?: number; |
There was a problem hiding this comment.
Maybe this is just the naming, but I am confused. This sounds to me as if we are overriding the used sample rate for this sampling decision? But if I read the comment below, I guess it means "propagate this sample rate to downstream spans"? Is this a correct interpretation? 😅
There was a problem hiding this comment.
well here we are in wrapSamplingDecision, meaning that the sampling decision was already made. In this context, it kinda means what you wrote: Override the sampling decision on the DSC for everything downstream with this value, no matter what the DSC had as a sample_rate before. Do you have any suggestions?
There was a problem hiding this comment.
Renamed it to shouldUpdateSampleRateOnDownstreamTrace for now.
Uh oh!
There was an error while loading. Please reload this page.
lforst
commented
Jan 21, 2025
|
Ref: #14932
Ref: getsentry/team-sdks#117
This PR mainly does 2 things:
sample_ratefrom the DSC and pass it to thetracesSamplerasparentSampleRatesample_ratefor a DSC of a given span whenever we applied a sample rate to the span's root span. Thesample_rateis equivalent to the sample rate the root span has been sampled with. This is to ensure that downstream services can reliably use thesample_ratein the DSC to pass it to thetracesSampler- like we do in this PR (kinda closing the loop).