Use USER_KEEP/USER_REJECT for RuleSampler decisions #1769
Conversation
There was a problem hiding this comment.
Overall it seems good... but I think I don't have enough context to evaluate if this change makes sense.
Is there any design doc I could reference to confirm that this is the common behavior we want? E.g., it's a bit unclear to me why we're changing this now.
lib/ddtrace/sampler.rb
Outdated
| if span.sampled | ||
| # If priority sampling has already been applied upstream, use that, otherwise... | ||
| unless priority_assigned_upstream?(span) | ||
| # Roll the dice and determine whether how we set the priority. | ||
| priority = priority_sample!(span) ? Datadog::Ext::Priority::AUTO_KEEP : Datadog::Ext::Priority::AUTO_REJECT | ||
| # If priority sampling has already been applied upstream, use that value. | ||
| return span.sampled if priority_assigned?(span) | ||
|
|
||
| assign_priority!(span, priority) | ||
| end | ||
| # Check with post sampler how we set the priority. | ||
| sample = priority_sample!(span) | ||
|
|
||
| # Check if post sampler has already assigned a priority. | ||
| return span.sampled if priority_assigned?(span) |
There was a problem hiding this comment.
Minor: Can the value of span.sampled be mutated as this method is being executed? If not, could we simply the early returns with return true if priority_assigned?(...) to clarify what we expect from it?
There was a problem hiding this comment.
It cannot, I simplified it was suggested. 👍
Oops just realized that you DID had already dropped a link to the design doc in slack. Thanks. What I did also notice is that I believe our docs still need to be updated, e.g. this section. |
Codecov Report
@@ Coverage Diff @@
## master #1769 +/- ##
=======================================
Coverage 98.21% 98.21%
=======================================
Files 931 931
Lines 44870 44904 +34
=======================================
+ Hits 44069 44103 +34
Misses 801 801
Continue to review full report at Codecov.
|
| # NOTE: We do not advise using a pre-sampler. It can save resources, | ||
| # but pre-sampling at rates < 100% may result in partial traces, unless | ||
| # the pre-sampler knows exactly how to drop a span without dropping its ancestors. | ||
| # | ||
| # Additionally, as service metrics are calculated in the Datadog Agent, | ||
| # the service's throughput will be underestimated. | ||
| attr_reader :pre_sampler, :priority_sampler |
There was a problem hiding this comment.
I'm actually curious, are there a lot of customers using this feature? Since it ends up being a very sharp edge...
There was a problem hiding this comment.
I'm not sure it's used in Ruby, but it is used by other language clients.
Because pre-smapler is the only way for the user to reduce traffic between ddtrace and the agent, it's used by clients with super high throughput, as their infra wouldn't handle the trace traffic otherwise (mostly due to high load on the trace agent).
This reverts commit dad5da9.
This PR changes changes how the sampling decisions yielded by the
Datadog::Sampling::RuleSampleraffect the span's_sampling_priority_v1tag.Before,
0(for rejecting) and1(for keeping) were used forRuleSamplerdecisions. The issue with this approach is that0and1are default sampling values that allow for the sampling decision to be altered downstream, either by the Agent or by the Datadog backend.0and1communicate intent, not a strong decision. These works great when the sampling rate is not strictly important: for example, it is normally desirable to keep an error span, even if it's marked with a sampling decision of0, as that span will have valuable debugging information.The issue with
0and1is that they don't allow for strict control of sampling rates. When limiting the sampling rate is very important (e.g. network traffic constrains, strictly limiting billable costs), such sampling was not possible with theRuleSampler, custom code snippets were required (e.g.Datadog::ForcedTracing.drop(span)).After the changes in this PR, decisions that are originated from any user configuration to the
RuleSampleruse priority values-1(for forced rejection) and2(for forced keeping). This means thatRuleSamplingsampling configurations will always be strictly respected by the whole Datadog processing pipeline.It is possible for the
RuleSamplerto analyze a span and not make any user-configured decision (e.g. when no rules match, or not sampling rate applies). In this case, the priority is not altered.