Questions about new tailsampling span ingest strategy

[john-morales]

Hi folks!

I've been following along on recent changes to the tailsampling processor (and was preparing to offer a couple low-priority bugfixes that I've come across).

However I recently came upon the work by @carsonip on storage support via pebble extension, as well as the new "span ingest" strategy mode of evaluating a batch of spans on the ingestion path for any terminal decisions that can be made eagerly, rather than strictly waiting for the full decision_wait time to make a decision on whole traces.

Something I was looking for clarification on is whether the "span ingest" behavior is actually backward compatible with the existing evaluation order precedence?
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/tailsamplingprocessor/README.md#policy-decision-flow

My read of the code is that a decision of Sampled from a partial trace batch results in an eager decision to sample and forward those traces:

opentelemetry-collector-contrib/processor/tailsamplingprocessor/processor.go

Lines 948 to 961 in 171d78e

	if decision == samplingpolicy.Sampled || decision == samplingpolicy.Dropped {
	// Release all accumulated spans (prior pending batches + current batch)
	// without writing the current batch to storage first.
	merged := ptrace.NewTraces()
	if allSpans, ok := tsp.tailStorage.Take(id); ok {
	appendAllTraces(merged, allSpans)
	}
	appendAllTraces(merged, spanIngestTraceData.ReceivedBatches)
	actualData.ReceivedBatches = merged
	actualData.FinalDecision = decision
	actualData.PolicyName = policyName
	if decision == samplingpolicy.Sampled {
	tsp.releaseSampledTrace(tsp.ctx, id, actualData)

Now suppose the following simple tailsampling policy that aims for "sample all traces from service A, except if it's the test environment".

processors:
  tail_sampling:
    decision_wait: 10s
    num_traces: 500
    expected_new_traces_per_sec: 10
    policies:
      [
        {
          name: service-A,
          type: string_attribute,
          string_attribute:
            {
              key: rpc.service,
              values: [ my.example.ServiceA ],
            }
        },
        {
          name: test-environment,
          type: drop,
          drop: {
            drop_sub_policy:
              [
                {
                  name: stop-test-environment,
                  type: string_attribute,
                  string_attribute: {key: environment, values: [test]}
                }
              ]
          }
        }
      ]

I believe the current code could (incorrectly) cause a partial trace from ServiceA to still be Sampled and forwarded on, because it might take another later batch of spans to observe the "test" environment attribute that would cause the whole trace to be dropped?

Said differently, I think my question is whether perhaps "Sampled" cannot safely be used as a terminal decision. (I also see that the current implementation attempts to identify "stateful" policy Evaluator's, which are checked at startup to ensure the "span ingest" strategy cannot be used if a policy definition contains a stateful Evaluator. But note this example is simply using string_attribute, which is not stateful.)

I know I've spent quite some time squinting at the evaluation order rules, so apologies if I'm still off my reasoning here. But if the current code's behavior is expected, might I suggest this be more prominently documented.

Thanks for your time. (CC @csmarchbanks since I saw on another PR that you had some thoughts/experience here.)

[john-morales]

Looking at the current behavior a bit further, I was focused more on the case of separate batches of spans for the same trace, and the batch that contains the Dropped decision coming after the batch that resulted in a Sampled decision. However now I think I see that the mismatch can happen even within a single span batch if the drop policy comes later in the policies list than any policy that results in Sampled:

opentelemetry-collector-contrib/processor/tailsamplingprocessor/processor.go

Lines 848 to 858 in e064b15

	if decision == samplingpolicy.Dropped {
	metrics.decisionDropped++
	return samplingpolicy.Dropped, p.name
	}
	if decision == samplingpolicy.Sampled {
	metrics.decisionSampled++
	if tsp.recordPolicy {
	sampling.SetAttrOnScopeSpans(trace.ReceivedBatches, "tailsampling.policy", p.name)
	}
	return samplingpolicy.Sampled, p.name
	}

I think I'm more confident in this simpler case being a bug? Happy to file an issue and move any discussion to the issue if that's more the preferred workflow. Thanks again.

[csmarchbanks]

Hello, you are correct there are a few bugs in the current implementation, especially around drop policies, but also latency. I have a PR to fix them open: #47476. I'll try to find a maintainer to get it merged next week!

[john-morales]

Oh, great! Ha, I looked for existing open issues and didn't find any, but I did not look at open PRs. Sounds good, thanks!

[carsonip]

thank you both. As @csmarchbanks said, there's an edge case around drop that is incorrectly handled in the existing implementation. With #47476 , it should be better. That said, there may still be cases where span ingest mode does not interact correctly with certain combinations of policies. Feel free to open an issue and we'll sort that out.

[s3onghyun]

@john-morales Your reading was correct — this was a genuine bug, and it has since been fixed. @csmarchbanks (who you CC'd) addressed exactly this class of problem in #47476 — "[processor/tailsampling] Fix drop, latency, and span count policies in span ingest mode" — merged May 5 and shipped in v0.126.0.

Two mechanisms from that PR resolve your example:

1. Drop policies are always evaluated first, regardless of their position in policies. Policy construction splits drop policies out and concatenates them at the front (slices.Concat(dropPolicies, policies)), with the comment "Dropped decision takes precedence over all others, therefore we evaluate them first." So "the drop policy comes later in the list than a Sampled policy" can't happen at evaluation time — the drop is always checked before any sampling policy. That covers your simpler single-batch case.

2. A pending drop policy blocks any eager Sampled decision. In makeDecisionOnSpanIngest, a drop policy that returns NotSampled — i.e. it didn't match these spans, but a later span could still carry environment=test — sets dropPolicyPending = true:

// A drop policy returning NotSampled means it didn't match these spans but
// could still match future spans for this trace, so we must defer a Sampled
// decision to avoid forwarding spans that should ultimately be dropped.

The loop then refuses to return Sampled (guarded by decision == Sampled && !dropPolicyPending) and actually breaks out as soon as it's past the drop policies (if !p.isDrop && dropPolicyPending { break }), returning Pending instead. So a partial ServiceA batch without environment=test is not forwarded eagerly — it waits for the full decision_wait, exactly as you argued it should. In other words Sampled is never used as a terminal decision while an as-yet-unmatched drop policy exists, which is the invariant you were pointing at.

The same PR also closed the related "a positive decision could later flip" cases you'd expect to matter: latency policies with an upper_bound and span-count policies with max_spans are now reported as stateful, so span-ingest rejects them at startup (requires all policies to be stateless) rather than risking a wrong eager sample.

I believe that fully covers what you raised, so this can probably be closed as resolved (for >= v0.126.0). Nice catch on the original report.