Support BanyanDB's group-scoped trace retention pipeline - #13965
Merged
wu-sheng merged 5 commits intoJul 31, 2026
Conversation
BanyanDB can now run a sampler plugin inside the data node during compaction, deciding per whole trace whether to keep it. This wires OAP up to configure that: the trace and zipkinTrace groups gain a pipeline block in bydb.yml, which OAP pushes onto the BanyanDB group as a TracePipelineConfig. Unlike the ingest-side server-side trace sampling, this runs AFTER storage, so it reclaims space already written and sees the whole trace rather than one segment as it arrives. The two are independent gates, so enabling both multiplies the drop rate. Disabled by default. Config notes worth knowing: - The grace windows use -1 for "inherit the data node default" (30s merge, 5m finalize), because the node treats any non-positive grace as unset, so 0 cannot mean "no grace" here. - enabledEvents accepts a comma-separated string as well as a YAML block list, so it can be set from the environment. Handling only the list form left the events empty, and the node reads an empty list as MERGE for backward compatibility, so asking for FINALIZE would silently have got MERGE with no error to point at. - A plugin's config is passed through verbatim as a protobuf Struct. Nested lists and objects, such as keepTagRules, now survive the loader as real ListValue/Struct instead of being flattened to a string by String.valueOf, and scalars keep their JSON type so a float such as healthySampleRate is not coerced back to a string. - A blank value in bydb.yml (a key with nothing after it) aborted startup with an opaque NullPointerException from java.util.Properties, which rejects nulls. Blank entries are now skipped and leave the field at its default, matching what omitting the line does. The proto submodule moves to 8fb4409, which is where TracePipelineConfig and SamplerPlugin live; without it the storage plugin does not compile. Four e2e cases cover the feature as two controlled pairs, one per schema: the same mocked data and thresholds, differing only by a keepTagRules entry, asserting opposite verdicts. Each case walks the whole chain — OAP resolves the placeholders, pushes the config onto the group, the data node loads the .so from the carrier's trusted dir with no load failures, the data is queryable, and then it is dropped (or kept) once past the merge grace. Locally 20/20 against the published d552528a carrier.
wankai123
added
feature
There was a problem hiding this comment.
Pull request overview
Adds support in SkyWalking OAP’s BanyanDB storage plugin for configuring BanyanDB’s group-scoped trace retention pipeline (native sampler plugins executed inside BanyanDB during compaction/finalization), and validates the end-to-end behavior with new e2e cases plus documentation updates.
Changes:
- Extend BanyanDB
bydb.yml(trace + zipkinTrace groups) with apipelineblock and wire OAP to push it to BanyanDB asTracePipelineConfig. - Enhance BanyanDB config loading / struct building to better preserve nested plugin config structures for protobuf
Structpayloads. - Add 4 e2e cases (SW + Zipkin, each as drop/keep controlled pairs), update CI workflow matrix, and update docs + changelog.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e-v2/script/env | Bumps pinned BanyanDB commit used by e2e images (incl. plugin-capable images). |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin/expected/traces-present.yml | Zipkin pipeline e2e assertion for “data present before merge grace elapses”. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin/expected/traces-dropped.yml | Zipkin pipeline e2e assertion for “data dropped after grace + merge”. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin/expected/sampler-loaded.yml | Zipkin sampler “loaded/active” sentinel assertion. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin/expected/group-pipeline.yml | Zipkin group schema assertion that pipeline config was pushed. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin/expected/config-dump-pipeline.yml | Zipkin assertion that OAP config dump includes expected pipeline keys. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin/e2e.yaml | Zipkin “drop” end-to-end pipeline verification. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin/docker-compose.yml | Zipkin pipeline test compose (plugin carrier + distroless plugin host + sidecar proxy + mocker). |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin-keep/expected/traces-present.yml | Zipkin “keep” test precondition assertion (data arrives). |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin-keep/expected/traces-kept.yml | Zipkin “keepTagRules causes retention” assertion. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin-keep/expected/sampler-loaded.yml | Zipkin keep-case sampler load sentinel. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin-keep/expected/group-pipeline.yml | Zipkin keep-case group pipeline echo assertion. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin-keep/expected/config-dump-pipeline.yml | Zipkin keep-case config dump assertion. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin-keep/e2e.yaml | Zipkin “keep” end-to-end pipeline verification. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/zipkin-keep/docker-compose.yml | Zipkin keep-case compose wiring + keepTagRules env override. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw/expected/traces-list.yml | SW schema trace list “arrives and queryable” assertion. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw/expected/traces-dropped.yml | SW schema “dropped after grace + merge” assertion. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw/expected/sampler-loaded.yml | SW schema sampler “loaded/active” sentinel assertion. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw/expected/group-pipeline.yml | SW group schema assertion that pipeline config was pushed. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw/expected/config-dump-pipeline.yml | SW config dump assertion for pipeline keys. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw/e2e.yaml | SW “drop” end-to-end pipeline verification. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw/docker-compose.yml | SW pipeline test compose (plugin carrier + proxy + trace mocker). |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw-keep/expected/traces-present.yml | SW keep-case precondition assertion (data arrives). |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw-keep/expected/traces-kept.yml | SW keep-case retention assertion (keepTagRules prevents drop). |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw-keep/expected/sampler-loaded.yml | SW keep-case sampler load sentinel. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw-keep/expected/group-pipeline.yml | SW keep-case group pipeline echo assertion. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw-keep/expected/config-dump-pipeline.yml | SW keep-case config dump assertion. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw-keep/e2e.yaml | SW “keep” end-to-end pipeline verification. |
| test/e2e-v2/cases/storage/banyandb/pipeline-plugin/sw-keep/docker-compose.yml | SW keep-case compose wiring + keepTagRules env override. |
| oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/resources/bydb.yml | Adds pipeline blocks to test fixture bydb.yml (incl. null/blank value case). |
| oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBTracePipelineConfigTest.java | Unit tests for placeholder resolution, enabledEvents parsing shapes, and nested config handling. |
| oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBStorageConfig.java | Adds model classes for trace pipeline + sampler plugin config under group resource. |
| oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBIndexInstaller.java | Builds and pushes TracePipelineConfig into BanyanDB group definitions/updates; adds protobuf Struct serialization helpers. |
| oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBConfigLoader.java | Loads pipeline from raw YAML map; resolves placeholders while attempting to preserve nested config shapes/types; skips null YAML values. |
| oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBConfigDumpExtension.java | Adds pipeline flattening into /debugging/config/dump output. |
| oap-server/server-starter/src/main/resources/bydb.yml | Documents and exposes new pipeline settings for trace + zipkinTrace groups. |
| docs/en/setup/backend/trace-sampling.md | Adds section explaining additional sampling/retention mechanisms (Zipkin receiver + BanyanDB pipeline) and how they combine. |
| docs/en/setup/backend/storages/banyandb.md | Documents pipeline settings and first-party sampler options and behavior. |
| docs/en/changes/changes.md | Changelog entries for pipeline support + blank-YAML-value startup fix + docs additions. |
| .github/workflows/skywalking.yaml | Adds the 4 new e2e pipeline cases to CI. |
Comments suppressed due to low confidence (1)
oap-server/server-starter/src/main/resources/bydb.yml:185
zipkinTrace.pipeline.mergeGraceSecondsis documented just above as “-1 leaves the node defaults in effect”, but the placeholder default is1800. This makes the shipped config inconsistent with its own comment and the documented-1sentinel behavior used elsewhere in this PR.
# -1 leaves the node defaults in effect (30s merge / 5m finalize); see the trace group above.
mergeGraceSeconds: ${SW_STORAGE_BANYANDB_ZIPKIN_TRACE_PIPELINE_MERGE_GRACE_SECONDS:1800}
finalizeGraceSeconds: ${SW_STORAGE_BANYANDB_ZIPKIN_TRACE_PIPELINE_FINALIZE_GRACE_SECONDS:-1}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nyandb-trace-retention-pipeline
wu-sheng
reviewed
Jul 31, 2026
wu-sheng
reviewed
Jul 31, 2026
Enable the pipeline in both the trace and zipkinTrace groups. This is inert for
most deployments: a data node only acts on the config when it runs the
plugin-capable image with -trace-pipeline-native-plugin-enabled=true and the
sampler .so in its trusted directory. A node without that support ignores the
config entirely, and one that has it but cannot load the .so logs an error, keeps
its previous sampler set and merges unfiltered, so nothing is dropped by accident.
Where it IS active, traces matching no keep rule are deleted at merge, so the
default is pinned by a test rather than left to be flipped unnoticed.
The -1 graces are omitted from the message rather than sent as negative Durations,
so the proto's gt:{seconds:0} validation cannot trip on a group registration.
Review fixes:
- The settings table in the BanyanDB storage guide was split in two: a prose
paragraph sat between the rows, terminating the table and leaving the plugins[]
rows as a headerless orphan. Rows are contiguous now, prose after.
- Drop the claim that a blank YAML value fails startup. It did once; the loader now
skips blanks and leaves the field unset.
- mergeGraceSeconds ships an explicit 1800 rather than -1, which contradicted the
surrounding comment. The value is intended, so both the comment and the guide now
say why: a trace is usually read soon after it is written, and the node's own 30s
default would let the sampler drop traces while someone is still looking at them.
- Import Struct/Value/ListValue/NullValue instead of writing them fully qualified.
com.google.protobuf.Duration stays qualified because java.time.Duration is already
imported and the simple name is taken; a comment says so.
- Correct the claim that float scalars keep their JSON type. They do not when
written as a ${ENV:default} placeholder, which is how the shipped bydb.yml writes
healthySampleRate: the shared resolver preserves only String/Integer/Long/Boolean,
so a Double falls through to its original string. The samplers accept a quoted
number for exactly this reason. The test fixture used a literal, which stayed a
Double and hid the real shape; it now uses the placeholder form and asserts the
string. The zipkin group keeps a literal so both shapes stay covered.
Revert the default to false in both the trace and zipkinTrace groups. Enabling it deletes stored traces, and with the shipped sampler config that is most of them: a trace is dropped unless it is slow, errored, or matches a keep rule, after which healthySampleRate retains 10% of the remainder (5% on zipkin). Turning that on for every deployment is not something an upgrade should do silently, so it stays opt-in. The guard added with the previous default is inverted rather than dropped: shouldBeDisabledByDefault now fails if the shipped value flips to true, so the default cannot change without a deliberate edit in the same commit. Everything else from the enable-by-default change is kept, because it is true either way: the note that the config only has any effect on a data node running the plugin-capable image with the sampler .so mounted, and the explanation of why merge ships an explicit 1800 rather than inheriting the node's 30s.
wu-sheng
previously approved these changes
Jul 31, 2026
The trace retention pipeline had its configuration documented in the BanyanDB storage guide, but nothing explained how a trace is actually judged. Add a page under "BanyanDB Exclusive Setup" covering it: the OR-ed rule chain and why order affects cost but never the verdict, the end-to-end duration envelope and why it is not the intrinsic MaxTS - MinTS, the deterministic trace-ID hash behind healthySampleRate and the two consequences of hashing rather than drawing at random, what each schema stores where, MERGE versus FINALIZE and their grace windows, the fail-open behaviour when a plugin is absent or unloadable, and the metrics to watch. Split the material rather than duplicate it. The storage guide keeps the keys, the environment-override guidance and the merge CPU cost, since those are what you need while deciding what to set, and its 54 lines of semantics move to the new guide. Both now cross-reference, and each fact lives in one place — the earlier duplication is where the broken settings table and the stale "a blank YAML value fails startup" claim came from. trace-sampling.md gains a See also, so a reader looking at ingest-side sampling can find the post-storage mechanism instead of assuming ingest is all there is. On the Zipkin limitations: say what keepErrors actually reads rather than calling it a convention and leaving it there. It looks for the error tag among the flattened query entries, which OAP writes both as a bare key and as key=value. That matters for the second limitation, because OAP drops BOTH forms once the value passes 256 characters, so a long exception message makes the loudest errors invisible to keepErrors.
wu-sheng
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BanyanDB can now run a sampler plugin inside the data node during compaction, deciding per whole trace whether to keep it. This wires OAP up to configure that: the trace and zipkinTrace groups gain a pipeline block in bydb.yml, which OAP pushes onto the BanyanDB group as a TracePipelineConfig.
Unlike the ingest-side server-side trace sampling, this runs AFTER storage, so it reclaims space already written and sees the whole trace rather than one segment as it arrives. The two are independent gates, so enabling both multiplies the drop rate. Disabled by default.
Config notes worth knowing:
The proto submodule moves to 8fb4409, which is where TracePipelineConfig and SamplerPlugin live; without it the storage plugin does not compile.
Four e2e cases cover the feature as two controlled pairs, one per schema: the same mocked data and thresholds, differing only by a keepTagRules entry, asserting opposite verdicts. Each case walks the whole chain — OAP resolves the placeholders, pushes the config onto the group, the data node loads the .so from the carrier's trusted dir with no load failures, the data is queryable, and then it is dropped (or kept) once past the merge grace. Locally 20/20 against the published d552528a carrier.
The BanyanDB plugin:apache/skywalking-banyandb#1234
If this is non-trivial feature, paste the links/URLs to the design doc.
Update the documentation to include this new feature.
Tests(including UT, IT, E2E) are added to verify the new feature.
If it's UI related, attach the screenshots below.
If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
Update the
CHANGESlog.