Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

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

Open
jhferris opened this issue Mar 6, 2019 · 7 comments
Open

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

jhferris opened this issue Mar 6, 2019 · 7 comments

Comments

@jhferris
Copy link
Contributor

jhferris commented Mar 6, 2019

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.

@g-easy
Copy link
Contributor

g-easy commented Mar 8, 2019

The child inherits the parent's trace options here:

trace_options = parent_ctx->trace_options();

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

Does this help?

@jhferris
Copy link
Contributor Author

jhferris commented Mar 8, 2019

Right, but the next line (

if (!trace_options.IsSampled()) {
) 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

@g-easy
Copy link
Contributor

g-easy commented Mar 20, 2019

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

@Oberon00
Copy link

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?

@terencekwt
Copy link
Contributor

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?

@curtpm
Copy link

curtpm commented Sep 23, 2019

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.

@g-easy
Copy link
Contributor

g-easy commented Sep 24, 2019

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants