Replace ALWAYS_PARENT with a composite ParentOrElse sampler

[bogdandrutu]

Fixes #492

[arminru]

LGTM!

I think we should revise the naming of the SamplingResults, however. NOT_RECORD and RECORD_AND_SAMPLED (mixed tenses) sounds a bit awkward IMHO.
Also, where and how is RECORD (but not sampled) used?

[Oberon00]

EDIT: I misread this, sorry. Please disregard this review.

I think you attempt to split something here that cannot be split. This is my interpretation of the current spec: 1. If there was no parent, then the spec says nothing about the behavior. This should be fixed. 2. If there was a parent, then the sampling decision equals the sampled flag on the parent decision (in other words the span is sampled if an only if its parent is sampled).

You do fix the first point in this PR, but the two new samplers cannot replace the behavior of the old ALWAYS_PARENT in the second point.

[Oberon00]

Maybe a just stared at this for too long but I am still not sure I'm reading this right. I'll try to restate this spec:

ALWAYS_PARENT_OR_ON, ALWAYS_PARENT_OR_OFF:

• If there is a parent, return NOT_RECORD if the parent's sampled flag is false, otherwise return RECORD_AND_SAMPLED.
• If there is no parent, return NOT_RECORD for the OR_OFF variant and RECORD_AND_SAMPLED for the OR_ON variant.

If this is correct, I suggest renaming ALWAYS_PARENT_OR_OFF to ONLY_PARENT.

Also, I would really love to see the "no-parent" case explicitly addressed. As it reads now, it's still ambiguous, whether the no-parent case falls under "otherwise" or "the writer of the spec did not consider that case".

[arminru]

@bogdandrutu you could add a table that shows the logic at one glance:

SampledFlag of parent's SpanContext	ALWAYS_PARENT_OR_ON Sampler	ALWAYS_PARENT_OR_OFF Sampler
true	RECORD_AND_SAMPLED	RECORD_AND_SAMPLED
false	NOT_RECORD	NOT_RECORD
no parent, i.e., for root spans	RECORD_AND_SAMPLED	NOT_RECORD

[arminru]

If this is correct, I suggest renaming ALWAYS_PARENT_OR_OFF to ONLY_PARENT.

@Oberon00 Personally I find the (ALWAYS_)PARENT_OR_OFF naming alright but ONLY_IF_PARENT would also be fine. To me, ONLY_PARENT sounds like only parents, i.e., root spans, are sampled but children would always be dropped.

Also, I would really love to see the "no-parent" case explicitly addressed. As it reads now, it's still ambiguous, whether the no-parent case falls under "otherwise" or "the writer of the spec did not consider that case".

I assumed the former implicitly but it certainly makes sense to spell this out clearly.

[bogdandrutu]

@Oberon00 / @arminru updated: added a table as suggested to clarify.

[carlosalberto]

Looks great to me. The table definitely helps a lot ;)

[Oberon00]

A final remark: Do we actually need both ALWAYS_PARENT_OR_* variants?

[arminru]

A final remark: Do we actually need both ALWAYS_PARENT_OR_* variants?

@Oberon00 Do you mean the feature of either default on or off if no parent is present in general or having it as two separate samplers?
For the latter I'd say no, because the same can be achieved by using a ProbabilitySampler(p=0) and a ProbabilitySampler(p=1), but it's a convenient shorthand. Implementations can still be kept simple by using ProbabilitySampler internally.

[Oberon00]

I forgot about using the ProbabilitySampler implementation! Makes sense. Maybe the spec should even state, normatively, that ALWAYS_PARENT_OR_ON is equivalent to a Probability(1) and _OR_OFF to (0).

[bogdandrutu]

@yurishkuro @arminru @Oberon00 updated to use a composite Sampler. I used RootOnly as the name, happy to change if you think of a better name.

[Oberon00]

Reposting my comment as the containing conversation was marked resolved:

I think the ALWAYS_PARENT pattern is still common enough to warrant it's own sampler. We should take care not to fall into the inner-platform trap here. After all, I can already write something like this today (using Java 8 functional interfaces, pseudo-code):

setSampler((parent, attrs) -> new Decision(
  !parent.isValid() || parent.getTraceFlags().isSampled()))

This could be written as setSampler(Samplers.alwaysParentOrOff()) or setSampler(samplers.respectParentOrElse(samplers.staticDecision(true))). I think we should try to keep it simple here.

[yurishkuro]

@Oberon00 the reason I am not a fan of ALWAYS_PARENT is because it conflates two different questions - WHETHER a sampling decision must be made, and WHAT that decision should be. For example, in Jaeger clients the former is never even an issue because it's enforced by the tracer itself, so the sampler is only every concerned with the decision itself. OTEL's model is more flexible as it allows up-sampling lower in the call graph. If we were to support ALWAYS_PARENT (which, btw, is named backwards because it works as parent-or-always, not at all how the name reads), then we'd also need to support PARENT_OR_PROBABILISTIC, PARENT_OR_RATELLIMITING, etc. - full denormalization of all possible sampler types. It's not clear to me why PARENT_OR_ALWAYS should be given more preference than any other sampler type.

[bogdandrutu]

@Oberon00 sorry for resolving that, my github did not get updated so I did not see the comment. If that happens again consider to press "Unresolve conversation" instead of duplicating the comment. Thanks and sorry for closing that with your comment.

[bogdandrutu]

@yurishkuro are you ok with the proposed name? We can later add RootOrRemoteParent for example

[Oberon00]

Hi @bogdandrutu, no problem, I already assumed that's what happened. 😃 But as I have no write-access and also did not create the PR, I cannot (un)resolve anything.

[Oberon00]

Apart from the RootOnly name, LGTM

[bogdandrutu]

The CI failure for the link https://marc.info/?l=apache-cvs&m=130928191414740 is transient, the problem is that the link takes a lot of time to load (probably something on their server side, I expect they need opentelemetry :) ).

[yurishkuro]

Should this have the "always" behavior instead of trusting parent, to be consistent? If trusting parent is needed, use Sampers.parentOrElse(Samplers.probabilistic(r)).

[yurishkuro]

I see you have added TODO, so will be in the separate PR?

[bogdandrutu]

Yes.