Single Span Sampling#2128
Conversation
Co-authored-by: Ivo Anjo <[email protected]>
delner
left a comment
There was a problem hiding this comment.
Overall looking good. Some questions.
Only hard ask here is to do some performance testing. I'm particularly interested in:
100+ spans per trace, all traces dropped, rule configured to keep one span
vs
100+ spans per trace, all traces kept, no rules evaluated
This should give us the relative cost increase in a worst case.
| enabled: settings.tracing.enabled, | ||
| trace_flush: trace_flush, | ||
| sampler: sampler, | ||
| span_sampler: build_span_sampler(settings), |
There was a problem hiding this comment.
Don't love that span_sampler is its own component within the tracer, but it seems better to have smaller, simpler responsibilities than to have one sampler that's complex.
| # These rules allow a span to be kept when its encompassing trace is dropped. | ||
| # | ||
| # The syntax for single span sampling rules can be found here: | ||
| # TODO: <Single Span Sampling documentation URL here> |
There was a problem hiding this comment.
Good call out: let's update this when we can.
| def get_trace(trace_op) | ||
| trace_op.flush! | ||
| trace_op.flush! do |spans| | ||
| spans.select! { |span| single_sampled?(span) } unless trace_op.sampled? |
There was a problem hiding this comment.
Seems like this will add some cost: having to iterate over each span in each dropped trace. Off the top of my head, not sure how we avoid this, but it is worth noting in regards of possible performance impact.
Fine for now; let's just measure the performance of this before merging, if possible.
There was a problem hiding this comment.
Benchmarks show it's not measurably slower, unless single span sampling actually matches, which causes 3 set_tag operations, which are a bit slow.
Benchmarks have been added and attached results to this PR as a comment.
| # sampled. | ||
| # | ||
| # DEV-2.0: Allow for `sample_rate` zero (drop all) to be allowed. This eases | ||
| # DEV-2.0: usage for many consumers of the {RateSampler} class. |
There was a problem hiding this comment.
Interesting. Can you clarify?
There was a problem hiding this comment.
I expanded the comment in the PR, but here's the gist:
All internal users of RateSampler (RuleSampler and now Single Span Sampling) want sample_rate == 0 to mean "drop all", but they can't do that because of the validation that happens in the RateSampler initializer.
The way they get around it is to not set the value in the initializer, but call:
sampler = RateSampler.new
sampler.sample_rate = sample_rate # There's no validation hereThis bypasses the validation. Ideally, the RateSampler would respect any rate between 0.0 and 1.0.
| end | ||
| end | ||
|
|
||
| def ==(other) |
There was a problem hiding this comment.
Is a span really the same as another span if their service and name are the same? For sampling purposes?
Can you explain the logic behind this a little more?
There was a problem hiding this comment.
This equality method referrers to the Matcher class, not to spans.
If two matchers have the exact same instance variables, which is the only state they can have, they are the same.
Currently, the name and service matchers are all the instance variables a Matcher can have, so if two matchers have the same name and service they are effectively the same.
|
Here are the benchmark results. Memory usage does have any significant change. The following benchmarks are measuring execution time. The numbers TraceOperation is kept by trace-level sampling:1. And no single span sampling is configured (baseline):This code path does not consult the single span sampler. TraceOperation is reject by trace-level sampling:2. And no single span sampling is configured:This code path does not consult the single span sampler. 3. Simple span sampling is configured and all spans are rejected:The difference between this benchmark and the previous one is the cost to consult single span rules. 4. Simple span sampling is configured and all spans are kept:One side effect of being Single Span Sampled is that 3 tags are added to each span successfully being single sampled, thus more overhead is expected. ConclusionsThe only code path with meaningful performance impact is the The rules by themselves are not very expensive: the difference between Maybe not surprising, but dropped traces ( |
delner
left a comment
There was a problem hiding this comment.
Performance looks acceptable for a worst case. Tests are passing (minus one annoying MacOS test) so I think this should be good to merge as soon as its rebased.
Nice work!
This PR adds support for single span sampling to the tracer.
Single Span Sampling allows you to:
It is configured through the documented environment variables:
DD_SPAN_SAMPLING_RULES,ENV_SPAN_SAMPLING_RULES_FILEAll changes in this feature branch have been individually reviewed.