Skip to content

Commit 25ac777

Browse files
committed
keyviz admin: precompute row totals + deterministic budget tie-break
Round-1 review fixes for PR #660 (Gemini medium): The rows-budget sort previously called rowActivityTotal inside the comparator, paying O(M) per pair × O(N log N) comparisons — quadratic-ish in column count for the cost of one HTTP request. Worse, the lack of a tie-breaker made truncation non-deterministic: two refreshes against identical data could return different row sets, surfacing as flicker in the SPA heatmap. Accumulate a precomputed row.total during the pivot phase (json:"-" field, internal only) and sort on it in the budget step. Add BucketID as the tie-breaker so identical data → identical response. Test TestKeyVizHandlerRowsBudgetTieBreakDeterministic stages three routes with equal Writes, requests rows=2 three times in a row, and asserts the same two routes (lowest BucketID) survive every time.
1 parent 78200a5 commit 25ac777

2 files changed

Lines changed: 48 additions & 10 deletions

File tree

internal/admin/keyviz_handler.go

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,11 @@ type KeyVizRow struct {
7373
RouteIDsTruncated bool `json:"route_ids_truncated,omitempty"`
7474
RouteCount uint64 `json:"route_count"`
7575
Values []uint64 `json:"values"`
76+
// total accumulates the sum of Values during pivot so the
77+
// rowBudget sort is O(N log N) on a precomputed key rather
78+
// than O(N log N × M) recomputing the sum per comparison.
79+
// Not on the wire — clients read activity off Values directly.
80+
total uint64
7681
}
7782

7883
// KeyVizHandler serves GET /admin/api/v1/keyviz/matrix.
@@ -262,7 +267,9 @@ func pivotKeyVizColumns(cols []keyviz.MatrixColumn, series KeyVizSeries, rowBudg
262267
rowsByID[mr.RouteID] = row
263268
order = append(order, mr.RouteID)
264269
}
265-
row.Values[j] = pick(mr)
270+
v := pick(mr)
271+
row.Values[j] = v
272+
row.total += v
266273
}
267274
}
268275
matrix.Rows = make([]KeyVizRow, len(order))
@@ -320,24 +327,24 @@ func bucketIDFor(mr keyviz.MatrixRow) string {
320327
// activity-descending truncation rather than design §5.5's lexicographic
321328
// merge. Future work should swap in the spec'd merge once the
322329
// virtual-bucket plumbing supports synthesis at the response layer.
330+
//
331+
// Sort uses the precomputed row.total (accumulated during pivot) so
332+
// the comparator is O(1), not O(M). BucketID breaks activity ties
333+
// deterministically — the SPA refresh on the same data must yield the
334+
// same row set.
323335
func applyKeyVizRowBudget(rows []KeyVizRow, budget int) []KeyVizRow {
324336
if budget <= 0 || len(rows) <= budget {
325337
return rows
326338
}
327339
sort.Slice(rows, func(i, j int) bool {
328-
return rowActivityTotal(rows[i]) > rowActivityTotal(rows[j])
340+
if rows[i].total != rows[j].total {
341+
return rows[i].total > rows[j].total
342+
}
343+
return rows[i].BucketID < rows[j].BucketID
329344
})
330345
return rows[:budget]
331346
}
332347

333-
func rowActivityTotal(r KeyVizRow) uint64 {
334-
var sum uint64
335-
for _, v := range r.Values {
336-
sum += v
337-
}
338-
return sum
339-
}
340-
341348
func sortKeyVizRowsByStart(rows []KeyVizRow) {
342349
sort.Slice(rows, func(i, j int) bool {
343350
if c := bytes.Compare(rows[i].Start, rows[j].Start); c != 0 {

internal/admin/keyviz_handler_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,3 +256,34 @@ func TestKeyVizHandlerEncodesAggregateBucket(t *testing.T) {
256256
require.True(t, r.RouteIDsTruncated)
257257
require.Equal(t, []uint64{2, 3}, r.RouteIDs)
258258
}
259+
260+
// TestKeyVizHandlerRowsBudgetTieBreakDeterministic pins Gemini round-1
261+
// nit: when two rows tie on activity total, the rows-budget truncation
262+
// must pick the same set every refresh. Tie-break is BucketID
263+
// ascending so a re-poll on identical data yields identical rows.
264+
func TestKeyVizHandlerRowsBudgetTieBreakDeterministic(t *testing.T) {
265+
t.Parallel()
266+
srv := newKeyVizTestServer(t, &fakeKeyVizSource{cols: []keyviz.MatrixColumn{
267+
{
268+
At: time.Unix(1_700_000_000, 0),
269+
Rows: []keyviz.MatrixRow{
270+
{RouteID: 3, Start: []byte("c"), End: []byte("d"), Writes: 10},
271+
{RouteID: 1, Start: []byte("a"), End: []byte("b"), Writes: 10},
272+
{RouteID: 2, Start: []byte("b"), End: []byte("c"), Writes: 10},
273+
},
274+
},
275+
}})
276+
defer srv.Close()
277+
278+
for i := 0; i < 3; i++ {
279+
resp := keyVizGet(t, srv.URL+"?rows=2")
280+
var matrix KeyVizMatrix
281+
require.NoError(t, json.NewDecoder(resp.Body).Decode(&matrix))
282+
resp.Body.Close()
283+
require.Len(t, matrix.Rows, 2, "iteration %d", i)
284+
// BucketID tie-break: route:1, route:2 win over route:3.
285+
// After the budget cap they sort by Start, giving "a" then "b".
286+
require.Equal(t, "route:1", matrix.Rows[0].BucketID, "iteration %d", i)
287+
require.Equal(t, "route:2", matrix.Rows[1].BucketID, "iteration %d", i)
288+
}
289+
}

0 commit comments

Comments
 (0)