opencensus-cpp icon indicating copy to clipboard operation
opencensus-cpp copied to clipboard

Child Span doesn't inherit Parent's sampling decision?

Open jhferris opened this issue 5 years ago • 7 comments

The documentation says "A sampling decision from a ParentSpan is ALWAYS inherited by all its regardless of the trace configuration. This ensures continuity of traces – for example, if a parent were sampled but one of its children were not, we’d then lose parts of the trace, and vice versa if the parent weren’t sampled yet one of its children were, we wouldn’t know where the span began" https://opencensus.io/tracing/sampling/

However, looking at the code, the sampler doesn't take into consideration of parent span context. https://github.com/census-instrumentation/opencensus-cpp/blob/master/opencensus/trace/internal/sampler.cc#L57 So I don't think the rule is enforced.

jhferris avatar Mar 06 '19 18:03 jhferris

The child inherits the parent's trace options here: https://github.com/census-instrumentation/opencensus-cpp/blob/2c2e4c764755b54181a9649adf51e6fe65678ec0/opencensus/trace/internal/span.cc#L78

The sampler is called later (L83) if the parent wasn't sampled.

Does this help?

g-easy avatar Mar 08 '19 03:03 g-easy

Right, but the next line (https://github.com/census-instrumentation/opencensus-cpp/blob/2c2e4c764755b54181a9649adf51e6fe65678ec0/opencensus/trace/internal/span.cc#L80) will see that it's not sampled and then will call the Probability Sampler which doesn't check the parent trace.

One of 2 things needs to happen:

  1. the span.cc code needs to not call the sampler (thereby respecting the parent's sampled bit). That should be as easy as changing line 80 to only be true if the parent_ctx is null

  2. the sampler code needs to check the parent ctx and if its non-null then return the parent's sampled bit.

I think the right spot to do this in would be span.cc instead of having every sampler have to remember to handle this case

jhferris avatar Mar 08 '19 18:03 jhferris

IIUC: if the parent is sampled then the child is sampled.

g-easy avatar Mar 20 '19 03:03 g-easy

But if the parent is not sampled, the intended child may become a new root span. Should the sampling not be on by-trace instead of a by-span basis?

Oberon00 avatar Mar 20 '19 12:03 Oberon00

To summarize the discussion above, I think the question is what should be the correct behavior when a user pass in conflicting signals when constructing a child span? (since in the current API, sampling can be passed in by-span basis)

i.e. a child span is created with an always sampler, but it already has configs implicitly inherited from a parent span set with a never sampler. Should the child span obey the parent config then and not be sampled?

terencekwt avatar Mar 20 '19 18:03 terencekwt

I was also confused by this, I assumed the default behavior was to disable traces if the parent had the sample option set to false.

However the default Sampler that gets created as part of the failed if (!trace_options.IsSampled()) check in span.cc

Generate creates a ProbabilitySampler as part of MakeDefaultTraceParams. The ProbabilitySampler::ShouldSample currently does not use parent_context or has_remote_parent param at all.

I was expecting that if the parent_context.trace_options().IsSampled() was set to false the inherited span would not sample either.

curtpm avatar Sep 23 '19 19:09 curtpm

If you want to avoid "inflationary" sampling of child spans, set the sampling probability to zero. Children of sampled parents will still get sampled.

g-easy avatar Sep 24 '19 07:09 g-easy