From 1ab7912b5354ebacbc46cecc68de72689e79dc13 Mon Sep 17 00:00:00 2001 From: bootjp Date: Thu, 10 Sep 2026 15:14:17 +0900 Subject: [PATCH 1/2] keyviz: label sub-ranges in the SPA and document the K tradeoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...25_implemented_keyviz_subrange_sampling.md | 14 ++-- internal/admin/keyviz_handler.go | 35 ++++++++++ internal/admin/keyviz_subbucket_test.go | 67 +++++++++++++++++++ main.go | 2 +- web/admin/src/api/client.ts | 6 ++ web/admin/src/pages/KeyViz.test.ts | 33 ++++++++- web/admin/src/pages/KeyViz.tsx | 25 ++++++- 7 files changed, 173 insertions(+), 9 deletions(-) create mode 100644 internal/admin/keyviz_subbucket_test.go diff --git a/docs/design/2026_05_25_implemented_keyviz_subrange_sampling.md b/docs/design/2026_05_25_implemented_keyviz_subrange_sampling.md index 707957e7f..d1c30112b 100644 --- a/docs/design/2026_05_25_implemented_keyviz_subrange_sampling.md +++ b/docs/design/2026_05_25_implemented_keyviz_subrange_sampling.md @@ -402,10 +402,13 @@ gains a sub-range discriminator keyed on `SubBucketCount` (§4.3): The SPA heatmap renders rows by `Start`/`End` and already supports an arbitrary number of rows under the row budget, so **no structural SPA -change is required** to see hot sub-ranges. Two small polish items -(optional, can be a follow-up PR): the `RowDetail` panel labels a -sub-row with its narrowed range, and tooltips show "route N · sub-range -i/K". The fan-out wire fields (`conflicts[]`, `raft_group_ids[]`, +change is required** to see hot sub-ranges. Two small polish items, **now implemented**: the `RowDetail` panel +labels a sub-row with `sub-range i/K` and renames its Start/End rows to +`Sub-range start`/`Sub-range end` so the narrowed bounds are not read as +the whole route's, with a tooltip explaining the split. The wire carries +`sub_bucket` / `sub_bucket_count` for this, both omitted unless the +route is genuinely sub-divided so the payload is byte-identical at the +`K=1` default. The fan-out wire fields (`conflicts[]`, `raft_group_ids[]`, `leader_terms[]`) are unaffected — they are per-column and travel on each sub-row unchanged. @@ -453,7 +456,8 @@ of 1024). Two consequences to document for operators: - The per-request `rows` budget (`keyVizRowBudgetCap`, 1024) now buys roughly `budget / active_subbuckets_per_route` *routes* shown at full resolution — i.e. raising `K` trades route breadth for intra-route - depth at a fixed payload size. The doc/flag help text should say so. + depth at a fixed payload size. **The `--keyvizKeyBucketsPerRoute` help + text now states this.** - The intermediate 160k-row materialisation is bounded and transient (one column build), but if it proves heavy, `Flush` can apply a cheap per-slot top-sub-bucket cap before the global budget. Noted as diff --git a/internal/admin/keyviz_handler.go b/internal/admin/keyviz_handler.go index db54f812b..2cd9b9859 100644 --- a/internal/admin/keyviz_handler.go +++ b/internal/admin/keyviz_handler.go @@ -103,6 +103,17 @@ type KeyVizRow struct { // merged row) so omitempty keeps it off the wire; otherwise // len == len(Values). Conflict is the OR of this slice. Conflicts []bool `json:"conflicts,omitempty"` + // SubBucket / SubBucketCount expose the order-preserving sub-range + // position within a route, so the SPA can label a sub-row as + // "sub-range i/K" instead of leaving the operator to parse the + // "#i" suffix out of BucketID. + // + // Both are omitted unless the route is genuinely sub-divided + // (SubBucketCount > 1), matching bucketIDFor: at the K=1 default, + // for aggregate rows, and for degenerate slots the wire is byte + // -identical to before, so an older SPA sees no change. + SubBucket int `json:"sub_bucket,omitempty"` + SubBucketCount int `json:"sub_bucket_count,omitempty"` // RaftGroupIDs[j] and LeaderTerms[j] carry the route's Raft // identity at the time column j was flushed (parallel to // Values[]). Phase 2-C+ fan-out uses @@ -409,6 +420,8 @@ func newKeyVizRowFrom(mr keyviz.MatrixRow, numCols int) *KeyVizRow { } row := &KeyVizRow{ BucketID: bucketIDFor(mr), + SubBucket: subBucketIndexFor(mr), + SubBucketCount: subBucketCountFor(mr), Label: string(mr.Label), Start: append([]byte(nil), mr.Start...), End: append([]byte(nil), mr.End...), @@ -428,6 +441,28 @@ func newKeyVizRowFrom(mr keyviz.MatrixRow, numCols int) *KeyVizRow { return row } +// subBucketIndexFor and subBucketCountFor mirror bucketIDFor's +// "genuinely sub-divided" test, so the two cannot disagree about +// whether a row is a sub-range: a bucket_id carrying "#i" always has +// the matching fields, and one without always omits them. +func subBucketIndexFor(mr keyviz.MatrixRow) int { + if !isSubDividedRow(mr) { + return 0 + } + return mr.SubBucket +} + +func subBucketCountFor(mr keyviz.MatrixRow) int { + if !isSubDividedRow(mr) { + return 0 + } + return mr.SubBucketCount +} + +func isSubDividedRow(mr keyviz.MatrixRow) bool { + return !mr.Aggregate && mr.SubBucketCount > 1 +} + func bucketIDFor(mr keyviz.MatrixRow) string { if mr.Aggregate { return "virtual:" + strconv.FormatUint(mr.RouteID, 10) diff --git a/internal/admin/keyviz_subbucket_test.go b/internal/admin/keyviz_subbucket_test.go new file mode 100644 index 000000000..22c770051 --- /dev/null +++ b/internal/admin/keyviz_subbucket_test.go @@ -0,0 +1,67 @@ +package admin + +import ( + "encoding/json" + "testing" + + "github.com/bootjp/elastickv/keyviz" + "github.com/stretchr/testify/require" +) + +// TestSubBucketFieldsStayOffTheWireUnlessSubDivided pins that the new +// fields are inert at the K=1 default. The keyviz response is consumed +// by an SPA that ships separately from the server, so a field that +// appeared on every row would change the payload for every existing +// deployment that never enabled sub-bucketing. +func TestSubBucketFieldsStayOffTheWireUnlessSubDivided(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + row keyviz.MatrixRow + }{ + {name: "K=1 route", row: keyviz.MatrixRow{RouteID: 7, SubBucketCount: 1}}, + {name: "unset count", row: keyviz.MatrixRow{RouteID: 7}}, + {name: "aggregate row", row: keyviz.MatrixRow{RouteID: 7, Aggregate: true, SubBucketCount: 4, SubBucket: 2}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + encoded, err := json.Marshal(KeyVizRow{ + BucketID: bucketIDFor(tc.row), + SubBucket: subBucketIndexFor(tc.row), + SubBucketCount: subBucketCountFor(tc.row), + }) + require.NoError(t, err) + require.NotContains(t, string(encoded), "sub_bucket", + "the sub-range fields must not appear for a non-sub-divided row") + }) + } +} + +// TestSubBucketFieldsAgreeWithTheBucketID pins that the fields and the +// "#i" suffix cannot disagree about whether a row is a sub-range — +// they share one predicate, so an SPA can trust either. +func TestSubBucketFieldsAgreeWithTheBucketID(t *testing.T) { + t.Parallel() + + subdivided := keyviz.MatrixRow{RouteID: 42, SubBucket: 3, SubBucketCount: 8} + require.Equal(t, "route:42#3", bucketIDFor(subdivided)) + require.Equal(t, 3, subBucketIndexFor(subdivided)) + require.Equal(t, 8, subBucketCountFor(subdivided)) + + encoded, err := json.Marshal(KeyVizRow{ + BucketID: bucketIDFor(subdivided), + SubBucket: subBucketIndexFor(subdivided), + SubBucketCount: subBucketCountFor(subdivided), + }) + require.NoError(t, err) + require.Contains(t, string(encoded), `"sub_bucket_count":8`) + + // Bucket zero of a subdivided route: the index is legitimately 0, + // so only the count keeps it on the wire — which is why the count + // is what the SPA must test, not the index. + first := keyviz.MatrixRow{RouteID: 42, SubBucket: 0, SubBucketCount: 8} + require.Equal(t, "route:42#0", bucketIDFor(first)) + require.Equal(t, 8, subBucketCountFor(first)) +} diff --git a/main.go b/main.go index 87aae6b0c..3acd7b008 100644 --- a/main.go +++ b/main.go @@ -257,7 +257,7 @@ var ( keyvizMaxTrackedRoutes = flag.Int("keyvizMaxTrackedRoutes", keyviz.DefaultMaxTrackedRoutes, "Maximum routes tracked individually before excess routes coarsen into virtual buckets") keyvizMaxMemberRoutesPerSlot = flag.Int("keyvizMaxMemberRoutesPerSlot", keyviz.DefaultMaxMemberRoutesPerSlot, "Maximum members listed on a virtual bucket; excess routes still drive the bucket counters") keyvizHistoryColumns = flag.Int("keyvizHistoryColumns", keyviz.DefaultHistoryColumns, "Maximum matrix columns retained in the keyviz ring buffer (each column = one Step)") - keyvizKeyBucketsPerRoute = flag.Int("keyvizKeyBucketsPerRoute", keyviz.DefaultKeyBucketsPerRoute, "Order-preserving sub-range buckets per individual route for the hot-key heatmap; 1 disables sub-bucketing (route-granular, today's behaviour). Capped at 256; memory is ~K*32 bytes/route, so K_max ~= memBudget/(32*keyvizMaxTrackedRoutes)") + keyvizKeyBucketsPerRoute = flag.Int("keyvizKeyBucketsPerRoute", keyviz.DefaultKeyBucketsPerRoute, "Order-preserving sub-range buckets per individual route for the hot-key heatmap; 1 disables sub-bucketing (route-granular, today's behaviour). Raising K trades route BREADTH for intra-route DEPTH at a fixed payload size: each route emits up to K sub-rows that compete for the same row budget, so fewer distinct routes fit in one response. Capped at 256; memory is ~K*32 bytes/route, so K_max ~= memBudget/(32*keyvizMaxTrackedRoutes)") keyvizLabelsEnabled = flag.Bool("keyvizLabelsEnabled", false, "Enable per-adapter KeyViz row labels. Default false keeps legacy route-only rows during rolling upgrades") // Hot-key drill-down (Phase 2-A++; design 2026_05_28_implemented_keyviz_hot_key_topk). diff --git a/web/admin/src/api/client.ts b/web/admin/src/api/client.ts index a401c0d64..88d85536f 100644 --- a/web/admin/src/api/client.ts +++ b/web/admin/src/api/client.ts @@ -349,6 +349,12 @@ export interface KeyVizRow { route_ids?: number[]; route_ids_truncated?: boolean; route_count: number; + // Present only when the route is genuinely sub-divided + // (sub_bucket_count > 1). Test sub_bucket_count, never sub_bucket: + // bucket zero of a sub-divided route legitimately has index 0 and + // is omitted from the JSON by omitempty. + sub_bucket?: number; + sub_bucket_count?: number; values: number[]; // Row-level conflict flag — the OR of conflicts[]. True when ≥2 // nodes reported a different non-zero value for the same cell, diff --git a/web/admin/src/pages/KeyViz.test.ts b/web/admin/src/pages/KeyViz.test.ts index 40d4ba492..03d7c688d 100644 --- a/web/admin/src/pages/KeyViz.test.ts +++ b/web/admin/src/pages/KeyViz.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; -import { parseBucketID } from "./KeyViz"; +import type { KeyVizRow } from "../api/client"; +import { parseBucketID, subRangeLabel } from "./KeyViz"; describe("parseBucketID", () => { it("parses legacy and labeled route bucket IDs", () => { @@ -16,3 +17,33 @@ describe("parseBucketID", () => { expect(parseBucketID("route:7:redis#")).toBeNull(); }); }); + +describe("subRangeLabel", () => { + const row = (over: Partial): KeyVizRow => ({ + bucket_id: "route:7", + start: "", + end: "", + aggregate: false, + route_count: 1, + values: [], + ...over, + }); + + it("labels a sub-divided route as 1-based i/K", () => { + expect(subRangeLabel(row({ sub_bucket: 0, sub_bucket_count: 8 }))).toBe("sub-range 1/8"); + expect(subRangeLabel(row({ sub_bucket: 7, sub_bucket_count: 8 }))).toBe("sub-range 8/8"); + }); + + // The index is omitted from the JSON for bucket zero, so the label + // must key off the COUNT. Testing sub_bucket instead would hide the + // first sub-range of every route. + it("labels bucket zero even though the index is absent from the wire", () => { + expect(subRangeLabel(row({ sub_bucket_count: 4 }))).toBe("sub-range 1/4"); + }); + + it("returns null when the route is not sub-divided", () => { + expect(subRangeLabel(row({}))).toBeNull(); + expect(subRangeLabel(row({ sub_bucket_count: 0 }))).toBeNull(); + expect(subRangeLabel(row({ sub_bucket_count: 1 }))).toBeNull(); + }); +}); diff --git a/web/admin/src/pages/KeyViz.tsx b/web/admin/src/pages/KeyViz.tsx index 0281224b7..5b1d01996 100644 --- a/web/admin/src/pages/KeyViz.tsx +++ b/web/admin/src/pages/KeyViz.tsx @@ -534,7 +534,20 @@ interface RowDetailProps { index: number; } +// subRangeLabel renders "sub-range i/K" for a sub-divided route, or +// null when the route is not sub-divided. +// +// It keys off sub_bucket_count rather than sub_bucket because bucket +// zero of a sub-divided route has index 0, which omitempty strips from +// the JSON — testing the index would hide exactly the first sub-range. +export function subRangeLabel(row: KeyVizRow): string | null { + const count = row.sub_bucket_count ?? 0; + if (count <= 1) return null; + return `sub-range ${(row.sub_bucket ?? 0) + 1}/${count}`; +} + function RowDetail({ row, index }: RowDetailProps) { + const subRange = subRangeLabel(row); const total = row.values.reduce((a, b) => a + b, 0); return (
@@ -542,6 +555,14 @@ function RowDetail({ row, index }: RowDetailProps) { Row {index} {row.bucket_id} {row.aggregate && aggregate} + {subRange && ( + + {subRange} + + )} {row.conflict && (
-
Start
+
{subRange ? "Sub-range start" : "Start"}
{decodePreview(row.start)}
-
End
+
{subRange ? "Sub-range end" : "End"}
{decodePreview(row.end)}
Routes
From 289ba94334c349ce974ff8f7144aecf26474cddf Mon Sep 17 00:00:00 2001 From: bootjp Date: Sun, 13 Sep 2026 14:47:50 +0900 Subject: [PATCH 2/2] keyviz: carry sub-range metadata through fan-out merging 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 --- internal/admin/keyviz_fanout.go | 24 ++++++- internal/admin/keyviz_subrange_test.go | 91 ++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/internal/admin/keyviz_fanout.go b/internal/admin/keyviz_fanout.go index 6e3dfa7af..af26466cf 100644 --- a/internal/admin/keyviz_fanout.go +++ b/internal/admin/keyviz_fanout.go @@ -552,7 +552,19 @@ type rowMergeAcc struct { routeIDsTruncated bool routeCount uint64 label string - cells []cellMergeAcc + // subBucket / subBucketCount carry the §K sub-range identity through the + // merge. Without them every merged response omitted both, so + // subRangeLabel returned null and the sub-range label and narrowed + // Start/End captions vanished specifically in the cluster-wide view -- + // the one view where an operator is most likely to be looking. + // + // A nonzero value from ANY peer wins: during a rolling upgrade the + // bucket is reported by both old peers (which omit the fields) and new + // ones, and taking the first peer's zero would discard what the upgraded + // peers know. + subBucket int + subBucketCount int + cells []cellMergeAcc } // cellMergeAcc tracks merge state for one (bucket, column) cell. @@ -630,11 +642,19 @@ func mergeRowInto( routeIDsTruncated: row.RouteIDsTruncated, routeCount: row.RouteCount, label: row.Label, + subBucket: row.SubBucket, + subBucketCount: row.SubBucketCount, cells: make([]cellMergeAcc, mergedWidth), } accByBucket[row.BucketID] = acc *bucketOrder = append(*bucketOrder, row.BucketID) } + // Accept sub-range metadata from any peer that reports it, so one legacy + // peer in the fan-out cannot blank it for the whole row. + if acc.subBucketCount == 0 && row.SubBucketCount != 0 { + acc.subBucket = row.SubBucket + acc.subBucketCount = row.SubBucketCount + } for j, ts := range srcColumns { idx, ok := indexByColumn[ts] if !ok || j >= len(row.Values) { @@ -748,6 +768,8 @@ func resolveRowMergeAcc(acc *rowMergeAcc, useGroupTermDedupe bool) KeyVizRow { row := KeyVizRow{ BucketID: acc.bucketID, Label: acc.label, + SubBucket: acc.subBucket, + SubBucketCount: acc.subBucketCount, Start: acc.start, End: acc.end, Aggregate: acc.aggregate, diff --git a/internal/admin/keyviz_subrange_test.go b/internal/admin/keyviz_subrange_test.go index ae9374078..f107171a6 100644 --- a/internal/admin/keyviz_subrange_test.go +++ b/internal/admin/keyviz_subrange_test.go @@ -116,3 +116,94 @@ func TestMergeKeyVizMatricesMixedKCoexist(t *testing.T) { require.Equal(t, []uint64{25}, byID["route:1#0"]) require.Equal(t, []uint64{15}, byID["route:1#1"]) } + +// TestMergeKeyVizMatricesCarriesSubRangeMetadata is the fan-out regression. +// +// rowMergeAcc retained neither SubBucket nor SubBucketCount, so every merged +// response serialized both as omitted: subRangeLabel returned null and the +// sub-range label plus the narrowed Start/End captions disappeared specifically +// in the cluster-wide view — the one view where an operator is most likely to be +// looking at sub-divided routes. +func TestMergeKeyVizMatricesCarriesSubRangeMetadata(t *testing.T) { + t.Parallel() + + col := []int64{1_700_000_000_000} + subRow := func(id string, sub int, values []uint64) KeyVizRow { + return KeyVizRow{ + BucketID: id, Start: []byte{byte(sub * 0x10)}, End: []byte{byte((sub + 1) * 0x10)}, + SubBucket: sub, SubBucketCount: 2, Values: values, + } + } + peerA := KeyVizMatrix{ + ColumnUnixMs: col, Series: keyVizSeriesWrites, + Rows: []KeyVizRow{subRow("route:1#0", 0, []uint64{5}), subRow("route:1#1", 1, []uint64{7})}, + } + peerB := KeyVizMatrix{ + ColumnUnixMs: col, Series: keyVizSeriesWrites, + Rows: []KeyVizRow{subRow("route:1#0", 0, []uint64{3}), subRow("route:1#1", 1, []uint64{2})}, + } + + merged := mergeKeyVizMatrices([]KeyVizMatrix{peerA, peerB}, keyVizSeriesWrites) + require.Len(t, merged.Rows, 2) + + byID := map[string]KeyVizRow{} + for _, r := range merged.Rows { + byID[r.BucketID] = r + } + require.Equal(t, 0, byID["route:1#0"].SubBucket) + require.Equal(t, 2, byID["route:1#0"].SubBucketCount, + "a merged row must keep its sub-range identity, or the SPA renders no label") + require.Equal(t, 1, byID["route:1#1"].SubBucket) + require.Equal(t, 2, byID["route:1#1"].SubBucketCount) +} + +// TestMergeKeyVizMatricesTakesSubRangeMetadataFromAnyPeer covers the rolling +// upgrade: during one, a bucket is reported by peers that omit the fields and +// peers that populate them. Taking the first peer's zero would blank the row for +// the whole cluster, so a nonzero value from any peer wins. +func TestMergeKeyVizMatricesTakesSubRangeMetadataFromAnyPeer(t *testing.T) { + t.Parallel() + + col := []int64{1_700_000_000_000} + // The legacy peer is FIRST, so the accumulator is seeded from it. + legacyPeer := KeyVizMatrix{ + ColumnUnixMs: col, Series: keyVizSeriesWrites, + Rows: []KeyVizRow{{ + BucketID: "route:1#1", Start: []byte{0x10}, End: []byte{0x20}, Values: []uint64{4}, + }}, + } + upgradedPeer := KeyVizMatrix{ + ColumnUnixMs: col, Series: keyVizSeriesWrites, + Rows: []KeyVizRow{{ + BucketID: "route:1#1", Start: []byte{0x10}, End: []byte{0x20}, + SubBucket: 1, SubBucketCount: 2, Values: []uint64{6}, + }}, + } + + merged := mergeKeyVizMatrices([]KeyVizMatrix{legacyPeer, upgradedPeer}, keyVizSeriesWrites) + require.Len(t, merged.Rows, 1) + require.Equal(t, 1, merged.Rows[0].SubBucket) + require.Equal(t, 2, merged.Rows[0].SubBucketCount, + "one legacy peer must not blank the sub-range identity the upgraded peers report") +} + +// A route that is not sub-divided must stay byte-identical on the wire, so an +// older SPA sees no change. +func TestMergeKeyVizMatricesOmitsSubRangeMetadataWhenAbsent(t *testing.T) { + t.Parallel() + + col := []int64{1_700_000_000_000} + peer := func(v uint64) KeyVizMatrix { + return KeyVizMatrix{ + ColumnUnixMs: col, Series: keyVizSeriesWrites, + Rows: []KeyVizRow{{ + BucketID: "route:1", Start: []byte{0x00}, End: []byte{0x20}, Values: []uint64{v}, + }}, + } + } + + merged := mergeKeyVizMatrices([]KeyVizMatrix{peer(1), peer(2)}, keyVizSeriesWrites) + require.Len(t, merged.Rows, 1) + require.Zero(t, merged.Rows[0].SubBucket) + require.Zero(t, merged.Rows[0].SubBucketCount) +}