wu-sheng opened a new issue, #13962:
URL: https://github.com/apache/skywalking/issues/13962
> Code paths in this issue are in `apache/skywalking-banyandb`. Umbrella:
#13634.
Found while reviewing apache/skywalking-banyandb#1234, which adds the first
first-party samplers that implement a whole-trace keep policy ("keep if any
span/segment has the error flag, or if the whole-trace duration is too long").
## Problem
`pkg/pipeline/sdk/sdk.go:137-138` promises the plugin:
> `Traces` holds one block per trace_id. The verdict's keep-mask is aligned
to this slice.
and documents `Decide` as a per-trace-id verdict. A tail sampler written
against that contract is entitled to treat a `TraceBlock` as the complete trace.
The merge path does not honor it. When a trace's spans exceed
`maxUncompressedSpanSize` (2 MiB, `banyand/trace/trace.go:45`), `mergeBlocks`
stages that trace as **several** `stagedTrace` entries
(`banyand/trace/merger.go:1012` and `:1040` — the
`pendingBlock.block.spanSize() >= maxUncompressedSpanSize` disjunct fires with
the same trace id).
`flushStaged` then builds the Decide batch from **only the first** staged
block per trace id and applies that single verdict to all of them:
```go
// banyand/trace/merger.go:836-847
seenEval := make(map[string]struct{})
for i := range staged {
if _, seen := seenEval[staged[i].traceID]; seen {
continue // <-- later blocks of the same trace
never reach Decide
}
seenEval[staged[i].traceID] = struct{}{}
...
}
```
So the plugin receives a **prefix** of the trace while the SDK tells it the
block is the trace.
## Impact
Any existential whole-trace predicate silently mis-fires on large traces:
- an error flag on a segment that landed in the second block is invisible →
the trace is dropped;
- a duration envelope (`max(start+duration) − min(start)`) is computed over
the prefix → underestimated → a slow trace is dropped.
Traces above 2 MiB of span bodies are disproportionately the slow ones and
the ones carrying errors, so this hits precisely the traces a tail sampler
exists to keep.
## Suggested fix
Union the same-trace staged blocks into one `TraceBlock` before `Decide`
(concatenating the projected tag columns row-wise is enough — every predicate
is existential over rows), keeping the per-block staging for the write path.
Alternatively, if the split is to remain visible to plugins, the SDK contract
must say so explicitly instead of promising one block per trace_id.
Related: apache/skywalking-banyandb#1234.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail:
[email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
