hanahmily commented on PR #1144:
URL:
https://github.com/apache/skywalking-banyandb/pull/1144#issuecomment-4562117375
Addressed all 9 Copilot review comments. Branch was force-pushed; commit
count stays at 3 (proto / design / backlog) as originally planned, with the
fixes folded into the relevant commits.
**Proto validation (4 comments):**
- `TracePipelineConfig.metadata` — now `[(validate.rules).message =
{required: true}]` so empty configs are rejected at parse time rather than
reaching the registry.
- `StageRule.stage`, `TagSamplingRule.tag_key`, `TagMatcher.tag_key` — all
now `[(validate.rules).string.min_len = 1]`, matching the project convention
(e.g. `common.v1.LifecycleStage.name`). An empty stage name or tag key would
have been silently un-matchable.
**Backlog doc inconsistency (1 comment):**
- The "field 2 and field 7 are reserved" claim was stale: those reservations
were dropped during the "remove all scoring references" pass earlier in the
branch, and the gaps were closed (StageRule field 2 is now `apply_at`;
TracePipelineConfig field 7 is now `merge_grace`). Updated the backlog doc to
say so explicitly — any future revival must pick new field numbers. The
renumber is wire-safe because the proto has never been released.
**Section numbering (3 comments):**
- Jumped from §4.2 straight to §6.1 because the original §5 was a
one-paragraph intro that got folded into §6 during an earlier rewrite, leaving
subsection labels off by one. Renumbered to keep the sequence contiguous: §6.x
→ §5.x, §7.x → §6.x, §8.x → §7.x — applied to both subsection headings and
every cross-reference (in main doc, proto comments, and backlog doc). No `§8.x`
or "Scenario 7/8" left anywhere.
**Finalization composition semantics (1 comment):**
- The §7.3 design-decision blockquote previously said "traces failing
**both** the gate and any FINALIZE `StageRule` predicates are dropped", which
could be read as `StageRule` being able to override a tail-sampling drop.
Rewrote it to be explicit: `tail_sampling` is the **absolute gate**, evaluated
first — traces failing it are dropped immediately and never reach any
`StageRule`. Surviving traces are then evaluated against any FINALIZE-targeted
`StageRule`; failing those is also a drop. To survive finalization a trace must
pass **both**. This now matches §1.1's "decides whether the trace survives at
all" framing.
`make generate / build / lint / check` all pass locally; CI was clean on the
previous push, the regenerated `docs/api-reference.md` picked up the new
validation comments only.
--
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]
