-
-
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(core): Add inheritOrSampleWith
helper to traceSampler
#15277
Conversation
sampleRate = options.tracesSampler(samplingContext); | ||
sampleRate = options.tracesSampler({ | ||
...samplingContext, | ||
inheritOrSampleWith(fallbackSampleRate) { |
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.
inheritOrSampleWith(fallbackSampleRate) { | |
inheritOrSampleWith: (fallbackSampleRate) => { |
is this maybe slightly cleaner regarding this
context? 🤔
|
||
// Fallback if parent sample rate is not on the incoming trace (e.g. if there is no baggage) | ||
if (typeof samplingContext.parentSampled === 'boolean') { | ||
return 1; |
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.
return 1; | |
return Number(samplingContext.parentSampled); |
!! otherwise this would propagate 1 for false
:D
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.
sweet!
Should we backport this? 🤔 I think it may be worth it, makes docs much easier in that regard..? |
We found in getsentry/sentry-docs#12544 that the proposed ux is horrible because nobody would understand what it does, so we're adding a helper that facilitates inheriting a sampling decision that also allows for catching more edge-cases without worsening ux and is self explanatory in what it does.