wankai123 commented on code in PR #1234:
URL:
https://github.com/apache/skywalking-banyandb/pull/1234#discussion_r3670730770
##########
docs/design/post-trace-pipeline.md:
##########
@@ -124,7 +124,7 @@ The contract is designed around three hard requirements.
- The plugin re-exports an `ABIVersion` constant; the engine refuses to load
on mismatch with its own compiled `sdk.ABIVersion`, turning a silent miscompile
into a clear, fail-fast error. Configuration is a structured
`google.protobuf.Struct` (`SamplerPlugin.config`) set directly in the pipeline
config; the engine serializes it to canonical JSON and the plugin unmarshals
those `[]byte` into its own typed config — so the wire form is structured and
inspectable while the `.so` boundary stays a plain `[]byte`, with no shared
config struct.
- **Distribution:** operators build plugins against the released,
version-tagged `pkg/pipeline/sdk` using the **same CI image / Go version /
`-trimpath` / CGO flags** as the data node. (Background constraints,
well-documented but outside this design's verified scope: Go plugins are
**Linux/macOS only**, **cannot be unloaded** — so changing a plugin requires a
node restart, there is no hot-reload — and a plugin **panic crashes the host**
unless contained; see fail-open below.)
-**(3) Projection / column selection — spans optional, more than tags.** The
plugin declares the columns it needs up front via `Project()`, which returns a
`Projection{ Tags []string; SpanIDs bool; Spans bool }`. The engine turns
`Tags` into the **same `model.TagProjection`** the block reader already honors
(`blockMetadata.tagProjection`), so only those tag columns are decoded into
`block.tags` — literally the query engine's tag-projection path
(`trace/v1/query.proto`), not a new mechanism. Two things are **opt-in and
default off**: the span-id column (`Projection.SpanIDs`) and the heavy
span-body column (`Projection.Spans`). A subtlety in the native layout makes
this matter: `spanIDs` and `spans` are encoded **together** in one data block
(`mustWriteSpansTo`/`mustReadSpansFrom`, `banyand/trace/block.go`), so reading
span ids is **not** free — requesting either one forces a read of the spans
stream. Only `trace_id` and `MinTS`/`MaxTS` are genuinely intrinsic (they come
from `bl
ockMetadata` with no decode). So the tiers are: intrinsic-always (`trace_id`,
`minTS`/`maxTS`), opt-in-by-name (tags), and opt-in-and-default-off (the spans
stream — span ids and/or span bodies) — which is what makes spans *more*
optional than tags. The declare-up-front handshake matches the
projection-pushdown contracts in [DuckDB's C table
API](https://duckdb.org/docs/stable/clients/c/table_functions)
(`duckdb_init_get_column_index`) and [DataFusion's
`TableProvider::scan`](https://datafusion.apache.org/library-user-guide/custom-table-providers.html)
(`projection: Option<&Vec<usize>>`). `min_duration`-style checks are free from
`minTS`/`maxTS`, and an error predicate is just a projected tag (e.g.
`is_error`), so a plugin that requests neither tags, span ids, nor span bodies
stays on the merge raw fast path (`mustReadRaw` → `mustWriteRawBlock`, §7.1)
and pays no decode at all.
+**(3) Projection / column selection — spans optional, more than tags.** The
plugin declares the columns it needs up front via `Project()`, which returns a
`Projection{ Tags []string; SpanIDs bool; Spans bool }`. The engine turns
`Tags` into the **same `model.TagProjection`** the block reader already honors
(`blockMetadata.tagProjection`), so only those tag columns are decoded into
`block.tags` — literally the query engine's tag-projection path
(`trace/v1/query.proto`), not a new mechanism. Two things are **opt-in and
default off**: the span-id column (`Projection.SpanIDs`) and the heavy
span-body column (`Projection.Spans`). A subtlety in the native layout makes
this matter: `spanIDs` and `spans` are encoded **together** in one data block
(`mustWriteSpansTo`/`mustReadSpansFrom`, `banyand/trace/block.go`), so reading
span ids is **not** free — requesting either one forces a read of the spans
stream. Only `trace_id` and `MinTS`/`MaxTS` are genuinely intrinsic (they come
from `bl
ockMetadata` with no decode). So the tiers are: intrinsic-always (`trace_id`,
`minTS`/`maxTS`), opt-in-by-name (tags), and opt-in-and-default-off (the spans
stream — span ids and/or span bodies) — which is what makes spans *more*
optional than tags. The declare-up-front handshake matches the
projection-pushdown contracts in [DuckDB's C table
API](https://duckdb.org/docs/stable/clients/c/table_functions)
(`duckdb_init_get_column_index`) and [DataFusion's
`TableProvider::scan`](https://datafusion.apache.org/library-user-guide/custom-table-providers.html)
(`projection: Option<&Vec<usize>>`). an error predicate is just a projected
tag (e.g. `is_error`), and while `minTS`/`maxTS` are free, they are the spread
of per-row *start* timestamps — **not** a trace duration (they are 0 for a
single-row trace), so a duration predicate must project the schema's own
start/duration tags, so a plugin that requests neither tags, span ids, nor span
bodies stays on the merge raw fast path (`mustRea
dRaw` → `mustWriteRawBlock`, §7.1) and pays no decode at all.
Review Comment:
Good catch, and it was worse than the capitalization. Fixed in eb1e7d69.
The paragraph previously claimed `min_duration`-style checks were free from
`minTS`/`maxTS`, which is wrong — those are the spread of per-row *start*
timestamps, not a duration (and `0` for a single-row trace). Replacing that
clause left the next one starting lowercase mid-paragraph *and* chained two
`so` clauses that no longer followed from each other:
> ...). an error predicate is just a projected tag (e.g. `is_error`), and
while `minTS`/`maxTS` are free, they are the spread of per-row *start*
timestamps — **not** a trace duration (they are 0 for a single-row trace), so a
duration predicate must project the schema's own start/duration tags, so a
plugin that requests neither tags...
Now split into two sentences that each say one thing:
> An error predicate is just a projected tag (e.g. `is_error`), while a
duration predicate is **not** free: `minTS`/`maxTS` are the spread of per-row
*start* timestamps, not a trace duration (they are 0 for a single-row trace),
so it has to project the schema's own start and duration tags. A plugin that
requests neither tags, span ids, nor span bodies stays on the merge raw fast
path...
I also swept the rest of the diff in that file for the same splice pattern
(lowercase after a sentence break, doubled connectives) — this was the only
instance.
--
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]
