Propose sampling API changes

[bogdandrutu]

This RFC is a collaboration between @lizthegrey and @bogdandrutu. Also reviewed by @c24t.

If accepted, this document will serve as a rough draft for the new Sampling API in the specification.

[c24t]

LGTM to move to proposed.

[reyang]

Looks good in general, I have some minor comments.

[bogdandrutu]

@lizthegrey @c24t @reyang please re-read because I made some large changes and things may have changed.

[lizthegrey]

We don't need a separate RFC to resolve the question of propagating the sampleRate if @tedsuo's hypothesis from open-telemetry/opentelemetry-specification#198 (comment) is true; instead, we can just incorporate that sampleRate, if it exists, will be read from the tracestate, multiplied by the current sampleRate of the current Sampler, and passed into the newly created tracestate using the internal SDK method.

Awaiting feedback from @iredelmeier there and here as a result

[yurishkuro]

I suggest adding clarification / examples when the user code would want to provide one of the 4 values.

[lizthegrey]

Continuing to approve. suggest that we go ahead and merge this as a proposed RFC and then open a separate PR to cover moving from proposed to approved now that most of the substantive feedback is addressed.

[tsloughter]

I just realized I don't think it is made clear. My take away is that sampling happens on every span creation. If this is the case it should be called out because it differs from opencensus which only runs on the root span or one with a remote parent.

[tsloughter]

Also, at first, since RecordEvents doesn't get used during a child's sampling decision I wasn't concerned that it was an attribute on a span instead of the span context. But thinking about it more, if it is to be used to know whether to skip adding events/attributes/etc to a span then it would be much better for us if it was kept in the span context.

My guess is the reason it isn't in the span context is because it isn't meant to be propagated? The issue for us is checking if a span is sampled is a very efficient operation because it simply checks the context, and so isn't an issue that it may get checked a lot if a lot of events are recorded. While if it is in the span then the table of spans has to be searched each time to find the value.

[bogdandrutu]

My guess is the reason it isn't in the span context is because it isn't meant to be propagated? The issue for us is checking if a span is sampled is a very efficient operation because it simply checks the context, and so isn't an issue that it may get checked a lot if a lot of events are recorded. While if it is in the span then the table of spans has to be searched each time to find the value.

@tsloughter I think you refer by "us" to erlang. Conceptually this property is a span level property and it does not get propagated to the child span (only the sampled-flag is propagated).

I just realized I don't think it is made clear. My take away is that sampling happens on every span creation. If this is the case it should be called out because it differs from opencensus which only runs on the root span or one with a remote parent.

Yes I think we should allow the sampler to be more configurable, we can always call the sampler and an implementation for example the probability sampler by default applies the sampling logic only if root or remote parent.

[tsloughter]

@bogdandrutu yes, "us" referring to Erlang/Elixir implementation. I get that it isn't propagated so it makes sense to not be in the context, but maybe it should be propagated and simply not taken into account for anything when it gets propagated.

I suppose the Erlang impl could also include it in the span context itself so that we can make the recording decisions efficiently. Since doing that won't change what is propagated, only what is in the context record for the Erlang process.

As for when sampling is run, the SDK must define when it is to run, right? Is that simply outside the scope of this RFC? If not then it would be good to define that in the SDK default implementation that sampling is run on all span starts.

[bogdandrutu]

@tsloughter I think the comment in the rfc does mention this:

* Allows users to configure if probability applies only for "root spans", "root spans and remote 
   parent", or "all spans".
     * Default is to apply only for "root spans and remote parent".
     * Remote parent property should be added to the SpanContext see specs [PR/216][specs-pr-216]

Do you want to add this in the TLDR section?

[tsloughter]

@bogdandrutu it implies it but it doesn't say it. I'm suggesting it be a clear requirement that in the SDK the default tracer's StartSpan must run the the sampler any time it is called.

[bogdandrutu]

@tsloughter added a clarification in the when does sampling happen section.

[tsloughter]

@bogdandrutu perfect, thanks.