wankai123 opened a new pull request, #1234: URL: https://github.com/apache/skywalking-banyandb/pull/1234
### Add first-party trace-retention sampler plugins for SkyWalking's two trace schemas - [x] If this is non-trivial feature, paste the links/URLs to the design doc. - [x] Update the documentation to include this new feature. - [x] Tests(including UT, IT, E2E) are added to verify the new feature. - [ ] If it's UI related, attach the screenshots below. — N/A - [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. — no existing issue - [x] Update the [`CHANGES` log](https://github.com/apache/skywalking-banyandb/blob/main/CHANGES.md). --- Design doc: [`docs/design/post-trace-pipeline.md`](https://github.com/apache/skywalking-banyandb/blob/main/docs/design/post-trace-pipeline.md), Scenarios 6.1 (SkyWalking segments) and 6.2 (Zipkin). Those sections are synced in this PR — see *Design-doc corrections* below. ## What Adds `sw-trace-sampler.so` and `zipkin-trace-sampler.so` under `plugins/skywalking/`. Both compile the shared `plugins/skywalking/internal/tracesampler` engine and differ only in a `Schema` value naming where each schema keeps the rule inputs, so the two `main.go` files stay a few lines each and cannot drift apart. | Input | `sw-trace-sampler` | `zipkin-trace-sampler` | |---|---|---| | Searchable tags | `tags` | `query` | | Error signal | `is_error` column | `error` key inside `query` | | Per-row start | `start_time` | `timestamp_millis` | | Per-row duration | `latency` (ms) | `duration` (µs) | Config keys: `durationThresholdMs`, `keepErrors` (+ `errorTag` override), `keepTagRules`, `healthySampleRate`. Full reference in [`plugins/README.md`](https://github.com/apache/skywalking-banyandb/blob/main/plugins/README.md). ## Why The existing `_example/segment-tail-sampler` projects tag names the real schemas do not have (`duration`, `status`), so it cannot filter live SkyWalking data — it is a teaching example, not a usable sampler. These plugins read the columns OAP actually writes. The key fact they encode: searchable tags are **not** first-class columns. OAP flattens every one of them into a single string-array column (`tags` for segments, `query` for Zipkin) holding `key=value` entries, so every `keepTagRules` entry resolves there. A rule naming a real column could never match, and is rejected at admission rather than silently never firing. ## Notable decisions - **Duration is an end-to-end envelope**, `max(start + duration) − min(start)` over the trace's rows — so a trace slow only through sequential spans is caught. It is deliberately not the intrinsic `MaxTS − MinTS`, which is the spread of per-row *start* timestamps and `0` for a single-row trace. - **Unevaluatable predicates keep the trace.** Absent duration columns mean "can't tell", not "not slow": those columns are schema-declared, so their absence implies the block was written under a different schema (typically the wrong plugin attached to the group), and dropping there would silently discard exactly what the operator configured the sampler to keep. An absent tag array is deliberately *not* treated this way — a trace with no searchable tags is ordinary data. - **Strict config decoding.** Every option is a keep rule, so a key that silently missed would leave a sampler with no rules that drops the whole group. Unknown keys and a wholly empty config are rejected. Note the reference `_example` plugin uses `snake_case`, so a config copied from it is refused outright instead of quietly retaining nothing. - **The flattened array is decoded once per trace, from a copy.** The SDK's string-array decode rewrites its source in place, and the engine hands the same `*TraceBatch` to every link of a chain, so decoding the shared buffer corrupted the value for later links — making a rule's verdict depend on its position in the chain, and only for values containing `|` or `\`. Covered by `TestDecide_EscapedEntriesSurviveChainedLinks`. ## Testing Unit tests run offline against the `pkg/pipeline/sdk/sdktest` fixture kit (no `.so`, no cluster), per `plugins/README.md`. `go test ./plugins/...` is green and `golangci-lint` (pinned v1.64.8, repo config) reports no findings. End-to-end coverage lives in the companion SkyWalking change, since it needs an OAP server to push the `TracePipelineConfig` and write real trace data: four cases (`sw`, `sw-keep`, `zipkin`, `zipkin-keep`) forming drop/keep pairs on both schemas — identical data and thresholds, differing only by a `keepTagRules` entry, asserting opposite verdicts. Locally those pass 20/20 against a carrier image built from this branch, verifying the whole chain: OAP resolves the config → pushes it onto the BanyanDB group → the data node loads the `.so` from the trusted dir with `sampler_load_failed == 0` → data is queryable, then dropped once past the merge grace. That SkyWalking PR cannot merge until a carrier image containing these plugins is published, so this PR goes first. ## Design-doc corrections While syncing §6.1/§6.2 to what was actually built: - The stage-retention chains referenced `segment-stage-retention.so` / `zipkin-stage-retention.so`, which do not exist, configured with `minDurationMs`, which no plugin reads. They now reference these plugins with `durationThresholdMs`. - The scenarios claimed the merge raw fast path is preserved. It is not: `forceSlow` is `len(projection.Tags) > 0`, so projecting any tag disables the raw-copy path and the block is decoded in full. This contradicted the document's own "Filter contract" section, which had it right. `Spans: false` bounds what the *plugin* reads, not what the merge decodes. - A rule on `local_endpoint_service_name` was removed — it is a first-class column, so it could never match. -- 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]
