[Single Span Sampling] Span Sampler#2098
Conversation
| return if trace_op.sampled? | ||
|
|
||
| # Return as soon as one rule returns non-nil | ||
| # DEV: `all?{|x|x.nil?}` is faster than `any?{|x|!x.nil?}` |
There was a problem hiding this comment.
Cool! I don't know about all?{|x|x.nil?} is faster than any?{|x|!x.nil?}
| events.span_finished.subscribe do |event_span, event_trace_op| | ||
| sample_span(event_trace_op, event_span) |
There was a problem hiding this comment.
Minor: There's a bit of a trade-off here in terms of where logic lives, but have you considered having the logic of "we don't even call the span sampler if the trace has already been sampled" here?
There was a problem hiding this comment.
I like hiding the logic that is specific to Single Span Sampling away, inside the @span_sampler. I see it as the tracer doesn't now, it just calls the @span_sampler and the sampler has to figure it out, as the tracer doesn't really care.
There was a problem hiding this comment.
I guess. All these components are quite inter-related anyway -- e.g. the span sampler "knows" things about the tracer and traces, e.g. when traces are going to be sampled anyway or not. +1 if you prefer this option I think it's reasonable; we can always adjust laster.
| def sample_span(trace_op, span) | ||
| begin | ||
| @span_sampler.sample!(trace_op, span) | ||
| rescue StandardError => e | ||
| Datadog.logger.warn { "Failed to sample span: #{e.class.name} #{e} at #{Array(e.backtrace).first}" } | ||
| end |
There was a problem hiding this comment.
Minor: On the other hand of the spectrum of "what should live in the Tracer class vs the Sampler", would it make sense to have the error handling live inside the sampler?
Since the Tracer is already quite complex, I think having the lower-level component being resilient would free a bit the Tracer to care more about orchestration. (On the other hand, it seems like a common pattern to have all the begin/rescues in the Tracer so... up to you as usual :D ).
There was a problem hiding this comment.
I can move this to the sampler, yeah, it seems good to hide it there.
| context 'with matching rules' do | ||
| let(:rules) { [Datadog::Tracing::Sampling::Span::Rule.new(match_all, sample_rate: 1.0, rate_limit: 3)] } | ||
|
|
||
| context 'a kept trace' do | ||
| before { trace_op.keep! } | ||
|
|
||
| it_behaves_like 'does not modify span' | ||
| end | ||
|
|
||
| context 'a rejected trace' do | ||
| before { trace_op.reject! } | ||
|
|
||
| it 'sets mechanism, rule rate and rate limit metrics' do | ||
| sample! | ||
|
|
||
| expect(span_op.get_metric('_dd.span_sampling.mechanism')).to eq(8) | ||
| expect(span_op.get_metric('_dd.span_sampling.rule_rate')).to eq(1.0) | ||
| expect(span_op.get_metric('_dd.span_sampling.max_per_second')).to eq(3) | ||
| end | ||
|
|
||
| context 'multiple rules' do | ||
| let(:rules) do | ||
| [ | ||
| Datadog::Tracing::Sampling::Span::Rule.new(match_all, sample_rate: 1.0, rate_limit: 3), | ||
| Datadog::Tracing::Sampling::Span::Rule.new(match_all, sample_rate: 0.5, rate_limit: 2), | ||
| ] | ||
| end | ||
|
|
||
| it 'applies the first matching rule' do | ||
| sample! | ||
|
|
||
| expect(span_op.get_metric('_dd.span_sampling.mechanism')).to eq(8) | ||
| expect(span_op.get_metric('_dd.span_sampling.rule_rate')).to eq(1.0) | ||
| expect(span_op.get_metric('_dd.span_sampling.max_per_second')).to eq(3) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
More of a duplication thing, but these tests are a lot more about the matcher/rule than the Sampler. It's not particularly bad, but perhaps it's not worth asserting that every metric gets set, for instance, since we already have test coverage for that.
There was a problem hiding this comment.
Always a though judgment call, let me see how it looks with more simplified checks.
There was a problem hiding this comment.
I removed a few of the internal assumptions.
e5ba346 to
de3b4a3
Compare
2b23264 to
8dc706f
Compare
3 participants
Follow up from #2095
This PR adds a new Single Span Sampling class that ties up all the previous Single Span Sampling work.
This class alters span tags when spans are finished, based on the provided rules.
At this moment, this is functionally a no-op, as the interface to configure rules will be done in a following PR.