wu-sheng commented on PR #1234:
URL:
https://github.com/apache/skywalking-banyandb/pull/1234#issuecomment-5120020809
Review findings. The whole-trace policy (keep on any errored span/segment,
or on a long end-to-end duration) is implemented correctly against the SDK
contract — `hasErrorColumn` is existential over rows and the duration envelope
`max(start+duration) − min(start)` is the right measure. The items below are
the gaps.
I verified the schema assumptions against the OAP write path and they hold:
`start_time` / `timestamp_millis` are the `@BanyanDB.TimestampColumn` for their
models and become `TAG_TYPE_TIMESTAMP` (`MetadataRegistry.java:293-296`),
stored as unix ns (`banyand/trace/write_standalone.go:533-540`); `latency` is
ms and Zipkin `duration` is µs, matching `DurationTagNanosPerUnit`; `is_error`
is an int column; OAP writes both the bare key and `key=value` into Zipkin's
`query` (`SpanForward.java:150-151`). Build and tests are green: `make
generate` / `make build`, `make build-plugins` emits all three `.so`, `go test
./plugins/...` passes.
## 1. `keepErrors` fails closed where `durationThresholdMs` fails open
`plugins/skywalking/internal/tracesampler/sampler.go:677-681`:
```go
col := b.Tag(s.errorTag)
if col == nil {
return false, nil // no keep
}
```
`hasSlowTrace` in the same situation returns `errNoDurationEnvelope` and the
trace is kept, on the reasoning that a schema-declared column that is absent
means the block came from a different schema — "can't tell", not "not slow"
(`sampler.go:541-551`). That argument applies unchanged to `is_error`.
Consequence: `{"keepErrors": true}` on a mis-paired group drops **every**
trace — the same catastrophe the empty-config guard at `sampler.go:338-342`
exists to prevent, reached by a different route.
`TestDecide_DurationMissingColumnsFailOpen` covers the duration side; there is
no error-side counterpart.
## 2. The never-match guard covers 3 columns out of ~20
`Schema.firstClassColumn` (`sampler.go:115-128`) only knows `DurationTag`,
`StartTimeTag` and the error column. Every other first-class column is accepted
at admission and then silently never matches, because rules resolve against the
flattened array:
- segment: `service_id`, `endpoint_id`, `service_instance_id`, `segment_id`,
`trace_id`
- zipkin: `name`, `kind`, `local_endpoint_service_name`,
`remote_endpoint_service_name`, …
"Always keep traces from the payment service", written as `{"tagKey":
"service_id", "equals": "..."}`, admits cleanly and then drops exactly those
traces — the precise failure mode this PR set out to eliminate, and the reason
it removed a `local_endpoint_service_name` rule from the design doc. The
comment at `sampler.go:111-114` already concedes the gap.
Both schemas' column sets are static and known; carrying them as a
`FirstClassColumns []string` on `Schema` closes it.
## 3. A row with a start but no duration does not contribute its start
`sampler.go:580-582` skips the row when **either** value is null, so its
start never reaches `minStart`. Zipkin spans without a `duration` (incomplete /
one-way) are common; if the earliest span is one, the envelope is measured from
a later start and underestimates. Tracking `minStart` from any non-null start,
independently of duration, is more faithful to "whole-trace duration".
## 4. The SDK contract still contradicts this PR's premise
The PR correctly establishes that `MaxTS − MinTS` is not the trace duration
— confirmed at `banyand/trace/block.go:91-93`, where both come from the per-row
timestamp column, i.e. span/segment **start** times. It fixes
`docs/design/post-trace-pipeline.md`, but leaves:
- `pkg/pipeline/sdk/sdk.go:158` — "MaxTS is the latest span end in unix
nanoseconds; trace duration is MaxTS - MinTS"
- `pkg/pipeline/sdk/_example/segment-tail-sampler/main.go:191-192` —
computes duration that way, commented "free from the intrinsic MinTS/MaxTS — no
decode"
`sdk.go` is the contract the next plugin author reads; leaving it wrong
re-creates the bug this work exists to fix.
## 5. `latencystatussampler` should be removed or deprecated
`plugins/skywalking/latencystatussampler/main.go:109` projects `duration`
and `status` — the same criticism this PR levels at
`_example/segment-tail-sampler`. `status` exists in neither schema, and
`duration` exists only in Zipkin (µs, so a `thresholdMs` there is off by
1000×). It fails open, so it is inert rather than destructive, but it ships in
the same `/plugins` image as the two new plugins, leaving three first-party
samplers of which one silently keeps everything.
## 6. Zipkin `keepErrors` misses `error` values longer than 256 chars
Upstream, not fixable here, but the plugin depends on it.
`SpanForward.java:143-152` skips **both** `query.add(tag.getKey())` and
`query.add(key=value)` when the value or `key=value` exceeds `Tag.TAG_LENGTH`
(256):
```java
if (tag.getValue().length() > Tag.TAG_LENGTH || tagString.length() >
Tag.TAG_LENGTH) {
continue; // the bare key the plugin matches on is never
written
}
query.add(tag.getKey());
query.add(tagString);
```
Zipkin `error` tags routinely carry exception messages past 256 chars, so
`keepErrors: true` on the Zipkin schema silently misses the loudest errors.
Worth an OAP-side fix (add the bare key before the length check) and a note in
the plugin header meanwhile, which currently documents only the
`http.status_code` / `otel.status_code` gap.
---
Separately, two framework-level issues affect whether this policy holds end
to end, but are not these plugins' responsibility — filed against the pipeline
engine as apache/skywalking#13962 (a split trace is decided on its first block
only) and apache/skywalking#13963 (the MERGE-event verdict is fragment-local).
--
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]
