keyviz: label sub-ranges in the SPA and document the K tradeoff - #1230
keyviz: label sub-ranges in the SPA and document the K tradeoff#1230bootjp wants to merge 2 commits into
Conversation
Closes the two polish follow-ups the sub-range sampling design left open, plus the help-text gap it explicitly flagged. The wire gains sub_bucket / sub_bucket_count so the SPA can say "sub-range i/K" instead of leaving an operator to parse the "#i" suffix out of bucket_id. Both are omitted unless the route is genuinely sub-divided, mirroring bucketIDFor's own test, so at the K=1 default and for aggregate rows the payload is byte-identical and an older SPA sees no change. RowDetail also renames its Start/End rows to Sub-range start/end when the row is a sub-range. Those bounds are the narrowed ones, and labelling them the same as a whole-route row invites reading a hot sub-range as a hot route. One subtlety worth naming: the label keys off sub_bucket_count, not sub_bucket. Bucket zero of a sub-divided route has index 0, which omitempty strips from the JSON — testing the index would have hidden the first sub-range of every route. Both the Go and the SPA tests pin that case specifically. The --keyvizKeyBucketsPerRoute help text now states the breadth/depth tradeoff the design said it should: raising K makes each route emit up to K sub-rows competing for the same row budget, so fewer distinct routes fit in one response. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@claude review |
|
Warning Review limit reachedNext included review available in 39 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @bootjp's task in 1m 43s —— View job Review: keyviz sub-range labels
Summary: Small, well-scoped change that wires already-shipped What I verified:
Minor/optional nits (non-blocking):
Not independently re-run: No changes requested — this is ready as-is from a correctness standpoint. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ab7912b53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SubBucket int `json:"sub_bucket,omitempty"` | ||
| SubBucketCount int `json:"sub_bucket_count,omitempty"` |
There was a problem hiding this comment.
When --keyvizFanoutNodes yields at least one successful peer, mergeKeyVizMatrices takes its multi-matrix path, but rowMergeAcc and resolveRowMergeAcc in internal/admin/keyviz_fanout.go do not retain either of these new fields. Every merged response therefore serializes them as omitted, causing subRangeLabel to return null and disabling the new sub-range label and narrowed Start/End captions specifically in the cluster-wide view. Preserve the metadata through the accumulator, preferably accepting a nonzero value from any modern peer for rolling-upgrade compatibility.
Useful? React with 👍 / 👎.
rowMergeAcc retained neither SubBucket nor SubBucketCount, so whenever --keyvizFanoutNodes produced at least one successful peer and the multi-matrix merge path ran, every merged row serialized both as omitted. subRangeLabel therefore returned null and the sub-range label plus the narrowed Start/End captions disappeared specifically in the cluster-wide view -- the view where an operator is most likely to be looking at a sub-divided route. A nonzero value from ANY peer wins, rather than the first peer's value. During a rolling upgrade a bucket is reported both by peers that omit the fields and by peers that populate them, and seeding from whichever happened to be first would let one legacy peer blank the identity for the whole cluster. A route that is not sub-divided still serializes neither field, so an older SPA sees no change. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
Confirmed and fixed in the commit above.
Both fields are now carried through the accumulator, and I took your rolling-upgrade suggestion literally: a nonzero value from any peer wins, not the first peer's. Seeding from whichever peer happened to be first would let a single legacy peer — which omits the fields — blank the identity for the whole cluster even though the upgraded peers reported it. A test puts the legacy peer first specifically to pin that ordering. A route that is not sub-divided still serializes neither field, so the "byte-identical for an older SPA" property the row's doc comment promises is preserved; there is a test for that too. Verification
|
|
To use Codex here, create a Codex account and connect to github. |
|
Claude encountered an error after 35s —— View job Review in progress
|
1 participant
How this was found
Auditing
*_implemented_*design docs for parked follow-ups.2026_05_25_implemented_keyviz_subrange_sampling.md§5.3 and §8:What
sub_bucket/sub_bucket_counton the keyviz row.RowDetailshows asub-range i/Kpill with an explanatory tooltip, and renames Start/End to Sub-range start/end when the row is a sub-range.--keyvizKeyBucketsPerRoutehelp text now states the breadth/depth tradeoff.The subtlety worth reviewing
The label keys off
sub_bucket_count, neversub_bucket. Bucket zero of a sub-divided route has index0, whichomitemptystrips from the JSON — testing the index would have silently hidden the first sub-range of every route. Both the Go and the SPA tests pin that specific case.Why rename Start/End. Those bounds are the narrowed ones. Labelling them identically to a whole-route row invites reading a hot sub-range as a hot route, which is the exact misreading sub-range sampling exists to prevent.
Behavior change / risk
At the
K=1default, and for aggregate rows, the payload is byte-identical — the new fields mirrorbucketIDFor's own "genuinely sub-divided" test via a shared predicate, so abucket_idcarrying#ialways has the matching fields and one without always omits them. An older SPA against a newer server sees no change.Test evidence
go test ./internal/admin/ -race -count=1— passgolangci-lint run(full repo) — 0 issues, no//nolintisSubDividedRowreturn true unconditionally failsTestSubBucketFieldsStayOffTheWireUnlessSubDividedon both theK=1andaggregatecasestsc -breports no errors in the changed files. Note the repo'snpm run lintscript is broken independently of this change (tsc -b --noEmitis rejected by this TS version), andvitest/@testing-libraryare absent from the local install, so the three new SPA test cases are verified by CI rather than locally — calling that out rather than implying I ran them.Self-review (five passes)
bucket_idsuffix share one predicate, so they cannot disagree about whether a row is a sub-range.https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE