Propagate Sampling Seed #3951
Propagate Sampling Seed #3951
Conversation
…sactionEndedAsCrashed.Net4_8.verified.txt
…sactionEndedAsCrashed.Net4_8.verified.txt
| @@ -61,6 +68,11 @@ private DynamicSamplingContext( | |||
| items.Add("sample_rate", sampleRate.Value.ToString(CultureInfo.InvariantCulture)); | |||
| } | |||
|
|
|||
| if (sampleRand is not null) | |||
| { | |||
| items.Add("sample_rand", sampleRand.Value.ToString("N4", CultureInfo.InvariantCulture)); | |||
There was a problem hiding this comment.
we want this even if we have sample_rate in? see line 68
There was a problem hiding this comment.
I believe so yes. The docs developer docs indicate:
If sample_rate (inside baggage) and the sampling decision (trailing -1 or -0 from the sentry-trace header) are present in the incoming trace, create sample_rand so that when compared to the incoming sample_rate it would lead to the same sampling decision that is in the sentry-trace header.
Which we're doing... and then we propagate the sample_rand so that if any downstream nodes can use this in their sampling decision.
| @@ -111,6 +123,27 @@ private DynamicSamplingContext( | |||
| return null; | |||
| } | |||
|
|
|||
| // See https://develop.sentry.dev/sdk/telemetry/traces/#propagated-random-value | |||
| if (items.TryGetValue("sample_rand", out var sampleRand)) | |||
There was a problem hiding this comment.
odd that we validate this to be within a range as a float but then we make it a string then try to parse it back out.
Can we rely on the original value and avoid the parsing instead?
There was a problem hiding this comment.
I'm not sure I follow. It may or may not be present as a string in the baggage header. If it's present then here we try to cast it to a float and ensure it's in the correct range.
If it's present but malformed then we don't create the DSC from the baggage header.
| if (items.TryGetValue("sample_rand", out var sampleRand)) | ||
| { | ||
| if (!double.TryParse(sampleRand, NumberStyles.Float, CultureInfo.InvariantCulture, out var rand) || | ||
| rand is < 0.0 or >= 1.0) |
There was a problem hiding this comment.
didn't we validate this above before adding it to items? or it could already be in items when the method was caled? in which case wouldn't we be adding it twice? (I add Add so assume it's not a dictionary)
There was a problem hiding this comment.
This method is creating a DSC from the baggage header. The BaggageHeader is a pretty simple class that just pulls stuff directly off a certain HTTP header, without any strong opinions about what the items in that header should be. Other vendors might be putting things on that header so we just propagate it, pretty much verbatim (we might add things to it but we never remove things from it).
But we can't control what will be on that baggage header in this SDK. A request could be made to our SDK with a malformed sentry-sample_rand item in the BaggageHeader.
|
added @cleptric and @buenaflor to check alignment with the other SDKs |
Resolves #3921
Implementation in other SDKs:
sample_randin DSC sentry-python#4016Progress
sentry-sample_randis read from the DSC and PropagationContext (TwP)sample_randwhen none is provided in BaggageHeadersample_randwhen creating new Transactionssample_randin sampling decisionsentry-sample_randgets propagated in trace headers