Override sampling decision#639
Conversation
| """ | ||
| return self.span_context.trace_options.enabled \ | ||
| or self.sampler.should_sample(self.span_context.trace_id) | ||
| return self.sampler.should_sample(self.span_context) |
There was a problem hiding this comment.
This is the important change: we used to short-circuit here, which meant we couldn't refuse to sample (from e.g. the AlwaysOffSampler) if the sampling bit was already set.
Note that we're reading span_context.trace_options.enabled in the sampler... and then using the sampling decision to set it. This works because we only use the sampler once at init time to set tracer.tracer (and the sampling bit). This is potentially confusing, but refactoring Tracer to fix this is outside the scope of this PR.
| raise NotImplementedError | ||
|
|
||
|
|
||
| class AlwaysOnSampler(Sampler): |
There was a problem hiding this comment.
Having samplers.FoobarSampler is much much better and more intuitive than samplers.foobar_sampler.FoobarSampler! Wish we could have that for propagators and other stuff.
| @@ -116,8 +116,11 @@ There are several things you can customize in OpenCensus: | |||
| other exporters are provided as `extensions <#trace-exporter>`__. | |||
|
|
|||
| * **Sampler**, which determines how traces are sampled. | |||
There was a problem hiding this comment.
One thing we probably need to consider - update the README file so first time users wouldn't be surprised when there is no trace dumped to the console due to the default low sample rate:
from opencensus.trace.tracer import Tracer
tracer = Tracer()
with tracer.span('foo'):
pass
There was a problem hiding this comment.
Added in 014b906, it does make the docs a bit uglier though.
reyang
left a comment
There was a problem hiding this comment.
The change looks great! And the namespace is much more cleaner!
I have a question regarding the expectation, do we expect the majority of people would use 0.01% as the default sample rate? If we do expect people to put the sample rate, then having 100% makes more sense since it gives a clear signal to the user that "you should customize the sample rate".
Another minor question is why do we need AlwaysOffSampler, or we can use ProbabilitySampler with rate=0 (I understand they have slight difference since AlwaysOffSampler ignores the sampling flag in the context, would like to understand why we want to take this approach versus another one)?
Another thing to consider: going forward we would need the agent to be able to pass in sampling policy, which could override the sampler.
I expect that a lot of people won't change the default settings no matter what they are (we have some evidence that people leave the
Without
We may need to allow setting per-span samplers for this. |
Fixes #481.
This PR changes the sampler API so that
should_samplenow takes the span context, not just the trace ID. This moves the responsibility for checking the sampling bit from the tracer to the sampler.This PR also:
AlwaysOnSamplertoProbabilitySamplerProbabilitySampler's default sampling rate to match the other clientsNote that we don't allow the user to override the sampler for a single span (as e.g. the java client does in
SpanBuilder). Overriding the tracer's sampler solves the problem in #481, but we may want to add a span-specific sampling option later.