From 01d16b087bb9953e6f03252a30a4fdf3ff0a77ac Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 6 Sep 2026 10:06:22 -0400 Subject: [PATCH 1/8] feat: Add stacked-by-status and mean-with-whiskers usage renderers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the three views from #873. [g] cycles the grouping (none/status/method/plugin) and [t] gains a latency metric. Stacked bars break each bucket down by series. Colour is not the encoding: each series also gets a distinct texture (█ ▓ ▒ ░, then · for overflow), so the chart survives a terminal without colour, a colour-vision deficiency, and a screenshot pasted into an issue. Statuses >= 400 and denials render red, gated on group=status — "429" is a plausible model name, and a method chart must not turn red because a label looks like a status. Segments are whole cells while the ungrouped view keeps eighths, because a fractional top cell cannot also encode a segment boundary. That is why ungrouped is its own state rather than a special case of grouping. Latency gets marks and a range instead of bars: a bar encodes magnitude from a zero baseline, but mean latency has no meaningful zero and the spread is usually the more interesting half. Scaled to the tallest +1σ so no cap is clipped, with the lower cap clamped at the axis since a whisker below zero reads as negative latency. A window with no measured responses says so rather than drawing an empty grid that reads as zero latency. Grouping is ignored for latency — the aggregator carries no per-label latency, so a "by status" latency chart would show the bucket-wide mean under a heading implying otherwise. Colour is asserted through isErrorSeries rather than ANSI bytes: lipgloss strips colour with no TTY, which is always true under `go test`, so a byte-level assertion would pass vacuously and keep passing if the rule broke. Live rendering surfaced two defects the unit tests missed. Adjacent value labels touched ("1.2ms0") because a 5-column label exactly filled the 5-column stride; barGap is now 2, which keeps every label rather than alternating them — alternating dropped the newest bucket's value on a narrow terminal and hid the "0" that distinguishes an idle minute from a small one, and truncating was worse still ("1.2ms" clipped to "1.2m" reads as minutes). renderAxis now derives its tick spacing from barGap instead of assuming 1. And a small peak made every latency gridline round to the same string, so a repeated axis label is suppressed. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/help_overlay.go | 3 +- authbridge/cmd/abctl/tui/keys.go | 8 +- authbridge/cmd/abctl/tui/usage_pane.go | 63 ++++- authbridge/cmd/abctl/tui/usage_render.go | 44 +++- authbridge/cmd/abctl/tui/usage_stacked.go | 248 ++++++++++++++++++ .../cmd/abctl/tui/usage_stacked_test.go | 196 ++++++++++++++ authbridge/cmd/abctl/tui/usage_state_test.go | 69 ++++- authbridge/cmd/abctl/tui/usage_whiskers.go | 233 ++++++++++++++++ .../cmd/abctl/tui/usage_whiskers_test.go | 180 +++++++++++++ 9 files changed, 1025 insertions(+), 19 deletions(-) create mode 100644 authbridge/cmd/abctl/tui/usage_stacked.go create mode 100644 authbridge/cmd/abctl/tui/usage_stacked_test.go create mode 100644 authbridge/cmd/abctl/tui/usage_whiskers.go create mode 100644 authbridge/cmd/abctl/tui/usage_whiskers_test.go diff --git a/authbridge/cmd/abctl/tui/help_overlay.go b/authbridge/cmd/abctl/tui/help_overlay.go index 233f84df3..33d54a2cf 100644 --- a/authbridge/cmd/abctl/tui/help_overlay.go +++ b/authbridge/cmd/abctl/tui/help_overlay.go @@ -113,8 +113,9 @@ var paneKeys = map[paneID]keyGroup{ paneUsage: { title: "USAGE (this pane)", bindings: []keyBinding{ - {"t", "cycle metric (tokens/requests/errors)"}, + {"t", "cycle metric (tokens/requests/errors/latency)"}, {"w", "cycle window (10m/1h/6h)"}, + {"g", "cycle grouping (none/status/method/plugin)"}, {"s", "toggle session / all sessions"}, {"r", "refresh now"}, {"esc", "back"}, diff --git a/authbridge/cmd/abctl/tui/keys.go b/authbridge/cmd/abctl/tui/keys.go index c3bcd5b32..e6c205aea 100644 --- a/authbridge/cmd/abctl/tui/keys.go +++ b/authbridge/cmd/abctl/tui/keys.go @@ -77,6 +77,12 @@ func (m *model) handleKey(msg tea.KeyMsg) tea.Cmd { case "w": m.usage.cycleWindow() return m.beginFetch() + case "g": + // Refetch: the grouping is a server-side query parameter, not a + // client-side filter, so the current snapshot has no series for the + // newly selected dimension. + m.usage.cycleGroup() + return m.beginFetch() case "r": return m.beginFetch() case "s": @@ -612,7 +618,7 @@ func (m *model) helpView() string { } else if m.selectedSess != "" { scopeHint = " [s] this session" } - return "[t] metric [w] window" + scopeHint + + return "[t] metric [w] window [g] group" + scopeHint + " [r] refresh [esc] back [?] keys [q] quit" case paneCatalog: if m.catalog == nil { diff --git a/authbridge/cmd/abctl/tui/usage_pane.go b/authbridge/cmd/abctl/tui/usage_pane.go index b51a05dcb..8ba9f70ab 100644 --- a/authbridge/cmd/abctl/tui/usage_pane.go +++ b/authbridge/cmd/abctl/tui/usage_pane.go @@ -58,6 +58,9 @@ type usageState struct { loading bool lastFetch time.Time + // group is the active breakdown; GroupNone renders the ungrouped bars. + group usage.Group + // reqSeq is the id of the most recently ISSUED request. Any reply carrying a // smaller id is stale and dropped. reqSeq uint64 @@ -97,10 +100,31 @@ func (u *usageState) window() (window, resolution time.Duration) { return w.window, w.resolution } -// cycleMetric advances [t]. Only the three count metrics are cycled here; -// latency gets its own renderer (mean-with-whiskers), which is a follow-up. +// cycleMetric advances [t] across the count metrics and latency. Latency uses a +// different renderer (mean-with-whiskers) because a bar encodes magnitude from a +// zero baseline and mean latency has no meaningful zero. func (u *usageState) cycleMetric() { - u.metric = (u.metric + 1) % 3 + u.metric = (u.metric + 1) % usageMetricCount +} + +// cycleGroup advances [g] through the groupings. Ungrouped keeps sub-row +// precision via partial blocks; a grouped view trades that for the breakdown, +// since a fractional top cell cannot also encode a segment boundary. +func (u *usageState) cycleGroup() { + switch u.group { + // The zero value is "" and GroupNone is "none": both mean ungrouped, so both + // must advance to status. Matching only GroupNone sent a freshly opened pane + // to the default arm, which set GroupNone and made the first [g] press a + // no-op. + case "", usage.GroupNone: + u.group = usage.GroupStatus + case usage.GroupStatus: + u.group = usage.GroupMethod + case usage.GroupMethod: + u.group = usage.GroupPlugin + default: + u.group = usage.GroupNone + } } func (u *usageState) cycleWindow() { @@ -116,11 +140,12 @@ func (m *model) fetchUsage() tea.Cmd { client := m.client window, resolution := m.usage.window() session := m.usage.session + group := m.usage.group req := m.usage.reqSeq return func() tea.Msg { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - snap, err := client.GetUsage(ctx, window, resolution, session, usage.GroupNone) + snap, err := client.GetUsage(ctx, window, resolution, session, group) return usageLoadedMsg{snap: snap, req: req, err: err} } } @@ -157,6 +182,26 @@ func (m *model) resumeUsagePolling() tea.Cmd { return tea.Batch(m.beginFetch(), usageTick(m.usage.tickGen)) } +// renderUsageChart picks the form the data calls for. +// +// Three renderers rather than one parameterised one, because the forms differ in +// kind and not just in decoration: bars encode a magnitude from zero with +// sub-row precision; a stack trades that precision for a breakdown, since a +// fractional top cell cannot also encode a segment boundary; and latency is a +// distribution whose zero is meaningless, so it gets marks and a range instead. +func renderUsageChart(snap *usage.Snapshot, m usageMetric, group usage.Group, width int) []string { + if m.isLatency() { + // Grouping is ignored here: the aggregator carries no per-label latency, + // so a "by status" latency chart would silently show the bucket-wide mean + // under a heading implying otherwise. + return renderWhiskers(snap.Buckets, width) + } + if group != "" && group != usage.GroupNone { + return renderStackedBars(snap.Buckets, m, group, width) + } + return renderBars(snap.Buckets, m, width) +} + // renderUsage draws the pane. func (m *model) renderUsage(width, height int) string { var b strings.Builder @@ -166,8 +211,12 @@ func (m *model) renderUsage(width, height int) string { scope = "session: " + m.usage.session } window, resolution := m.usage.window() - b.WriteString(fmt.Sprintf(" USAGE — %s — %s @ %s — %s\n\n", - scope, window, resolution, m.usage.metric)) + grouping := "ungrouped" + if m.usage.group != "" && m.usage.group != usage.GroupNone { + grouping = "by " + string(m.usage.group) + } + b.WriteString(fmt.Sprintf(" USAGE — %s — %s @ %s — %s — %s\n\n", + scope, window, resolution, m.usage.metric, grouping)) switch { case m.usage.err != nil && errors.Is(m.usage.err, errUsageUnsupported): @@ -183,7 +232,7 @@ func (m *model) renderUsage(width, height int) string { case m.usage.snap == nil: b.WriteString(" (no data)\n") default: - for _, line := range renderBars(m.usage.snap.Buckets, m.usage.metric, width) { + for _, line := range renderUsageChart(m.usage.snap, m.usage.metric, m.usage.group, width) { b.WriteString(line) b.WriteString("\n") } diff --git a/authbridge/cmd/abctl/tui/usage_render.go b/authbridge/cmd/abctl/tui/usage_render.go index c5bd1bd34..75ce11922 100644 --- a/authbridge/cmd/abctl/tui/usage_render.go +++ b/authbridge/cmd/abctl/tui/usage_render.go @@ -17,8 +17,15 @@ import ( // for stacked segments — narrower and the block glyphs stop reading as distinct // textures, which is what carries the encoding when color is unavailable. const ( - barWidth = 4 - barGap = 1 + barWidth = 4 + // barGap is 2, not 1, so a value label always has a separating column. Labels + // are up to 5 wide ("48.2k", "1.2ms"); at a stride of 5 two adjacent labels + // touched and read as one nonsense number ("1.2ms0"). Widening the gap keeps + // every label — alternating them instead would have dropped the newest + // bucket's value on a narrow terminal and hidden the "0" that distinguishes an + // idle minute from a small one. Ten bars at stride 6 plus the gutter is 66 + // columns, still inside 80. + barGap = 2 barStride = barWidth + barGap plotRows = 10 // vertical resolution before partial blocks axisLabel = 6 // " 50k " gutter @@ -37,6 +44,11 @@ const ( metricTokens usageMetric = iota metricRequests metricErrors + metricLatency + + // usageMetricCount bounds the [t] cycle. Kept adjacent to the iota block so + // adding a metric means editing one line here. + usageMetricCount = iota ) func (m usageMetric) String() string { @@ -45,23 +57,37 @@ func (m usageMetric) String() string { return "requests" case metricErrors: return "errors" + case metricLatency: + return "latency" default: return "tokens" } } -// value extracts this metric from a bucket. -func (m usageMetric) value(b usage.Bucket) int64 { +// isLatency reports whether this metric needs the whiskers renderer rather than +// bars. Latency is a distribution, not a magnitude from zero, so it gets a +// different form — see the comment on the whisker glyphs. +func (m usageMetric) isLatency() bool { return m == metricLatency } + +// valueOf extracts this metric from a per-label Counts. Bucket embeds Counts, so +// value below delegates here — one place decides what each metric means, and a +// stacked segment can never disagree with the bar it sits inside. +func (m usageMetric) valueOf(c usage.Counts) int64 { switch m { case metricRequests: - return b.Requests + return c.Requests case metricErrors: - return b.Errors + return c.Errors default: - return b.Tokens + return c.Tokens } } +// value extracts this metric from a bucket. +func (m usageMetric) value(b usage.Bucket) int64 { + return m.valueOf(b.Counts) +} + // renderBars draws the ungrouped bar chart: a y-axis with humanized labels, one // bar per bucket using partial blocks for sub-row precision, a time axis, and a // value row. @@ -166,7 +192,11 @@ func renderAxis(n int) string { for i := 0; i < n; i++ { sb.WriteString(strings.Repeat("─", barWidth)) if i < n-1 { + // One tick plus dashes for the rest of the gap, so the tick stays on + // the bar boundary and the baseline remains continuous whatever barGap + // is set to. sb.WriteString("┴") + sb.WriteString(strings.Repeat("─", barGap-1)) } } return sb.String() diff --git a/authbridge/cmd/abctl/tui/usage_stacked.go b/authbridge/cmd/abctl/tui/usage_stacked.go new file mode 100644 index 000000000..e47441ed5 --- /dev/null +++ b/authbridge/cmd/abctl/tui/usage_stacked.go @@ -0,0 +1,248 @@ +package tui + +import ( + "fmt" + "sort" + "strings" + + "github.com/rossoctl/cortex/authbridge/authlib/usage" +) + +// segmentGlyphs are the textures a stacked bar cycles through, densest first. +// +// Redundant with color on purpose. Color alone fails three ways here: a terminal +// with no color support, a viewer with a colour-vision deficiency, and a +// screenshot pasted into an issue. A distinct glyph per series survives all +// three, so the chart is readable from texture alone and color is an accelerant +// rather than the encoding. +// +// Ordered densest-to-sparsest so the largest series (sorted first below) is also +// the visually heaviest, which matches what a reader expects from a stack. +var segmentGlyphs = [...]rune{'█', '▓', '▒', '░'} + +// overflowGlyph marks the series past len(segmentGlyphs) — including the +// aggregator's own "(other)" bucket. Distinct from the four above so "several +// small series folded together" never looks like a named one. +const overflowGlyph = '·' + +// seriesKey is one label's total across the window, used to decide segment order +// and which labels get their own glyph. +type seriesKey struct { + label string + total int64 +} + +// collectSeries totals each label across every bucket and returns them largest +// first, with ties broken by label so the legend and the stack order are stable +// between renders. An unstable order would make the chart appear to reshuffle on +// each 20s poll even when nothing changed. +func collectSeries(buckets []usage.Bucket, m usageMetric) []seriesKey { + totals := map[string]int64{} + for _, b := range buckets { + for label, c := range b.Series { + totals[label] += m.valueOf(c) + } + } + out := make([]seriesKey, 0, len(totals)) + for label, total := range totals { + out = append(out, seriesKey{label: label, total: total}) + } + sort.Slice(out, func(i, j int) bool { + if out[i].total != out[j].total { + return out[i].total > out[j].total + } + return out[i].label < out[j].label + }) + return out +} + +// glyphFor assigns each series its texture by rank. +func glyphFor(rank int) rune { + if rank < len(segmentGlyphs) { + return segmentGlyphs[rank] + } + return overflowGlyph +} + +// isErrorStatus reports whether a group=status label denotes a failure, so it can +// be drawn in red. +// +// Only meaningful for GroupStatus: a model name or plugin name has no status to +// read. Callers gate on the grouping rather than this returning false for +// everything else, because "429" is a plausible model name in a way that should +// not silently colour a method chart red. +func isErrorStatus(label string) bool { + if label == "denied" { + return true // a rejected request never got a status code + } + // Labels are produced by strconv.Itoa on the status code, so a leading 4 or 5 + // with three digits is the whole test. + if len(label) != 3 { + return false + } + return label[0] == '4' || label[0] == '5' +} + +// renderStackedBars draws one bar per bucket, split into segments by series. +// +// Segments are whole cells, not eighths: a fractional top cell cannot also encode +// a segment boundary, so the ungrouped renderer keeps sub-row precision and this +// one trades it for the breakdown. That is the reason "ungrouped" is its own +// cycle state rather than a special case of grouping. +func renderStackedBars(buckets []usage.Bucket, m usageMetric, group usage.Group, width int) []string { + if len(buckets) == 0 { + return []string{" (no data)"} + } + maxBars := (width - axisLabel) / barStride + if maxBars < 1 { + maxBars = 1 + } + if len(buckets) > maxBars { + buckets = buckets[len(buckets)-maxBars:] + } + + series := collectSeries(buckets, m) + if len(series) == 0 { + // Grouped view with no labelled traffic yet: fall back to the ungrouped + // bars rather than an empty frame, so the pane still shows the volume it + // does know about. + return renderBars(buckets, m, width) + } + + // Rank each label once so segment order is identical in every bucket. A stack + // whose layers reorder between adjacent bars is unreadable. + rank := make(map[string]int, len(series)) + for i, s := range series { + rank[s.label] = i + } + + var peak int64 + for _, b := range buckets { + if v := m.value(b); v > peak { + peak = v + } + } + + out := make([]string, 0, plotRows+4) + for row := plotRows; row >= 1; row-- { + var sb strings.Builder + if row%2 == 0 && peak > 0 { + sb.WriteString(fmt.Sprintf("%5s ", humanizeCount(peak*int64(row)/int64(plotRows)))) + } else { + sb.WriteString(strings.Repeat(" ", axisLabel)) + } + for _, b := range buckets { + sb.WriteString(stackedCell(b, m, group, series, rank, peak, row)) + sb.WriteString(strings.Repeat(" ", barGap)) + } + out = append(out, strings.TrimRight(sb.String(), " ")) + } + + out = append(out, renderAxis(len(buckets))) + out = append(out, renderTimeLabels(buckets)) + out = append(out, renderValues(buckets, m)) + out = append(out, "") + out = append(out, renderLegend(series, group, width)) + return out +} + +// stackedCell renders one bar's glyphs for one row, choosing the segment whose +// cumulative height covers this row. +func stackedCell(b usage.Bucket, m usageMetric, group usage.Group, + series []seriesKey, rank map[string]int, peak int64, row int) string { + + total := m.value(b) + if total <= 0 || peak <= 0 { + return strings.Repeat(" ", barWidth) + } + // Whole rows only — see the renderStackedBars comment on why segments cannot + // use partial blocks. + barRows := total * int64(plotRows) / peak + if barRows == 0 { + barRows = 1 // non-zero traffic must never render as an empty column + } + if int64(row) > barRows { + return strings.Repeat(" ", barWidth) + } + + // Walk the series in rank order, accumulating rows, until we pass this row. + // Rows are allotted proportionally to each series' share of the bucket. + var acc int64 + for _, s := range series { + v := m.valueOf(b.Series[s.label]) + if v <= 0 { + continue + } + segRows := v * barRows / total + if segRows == 0 { + segRows = 1 // a present series must occupy at least one cell + } + acc += segRows + if int64(row) <= acc { + cell := strings.Repeat(string(glyphFor(rank[s.label])), barWidth) + return paintSegment(cell, s.label, group) + } + } + // Rounding left this row uncovered: attribute it to the largest series rather + // than punching a hole in the middle of a bar. + cell := strings.Repeat(string(glyphFor(0)), barWidth) + return paintSegment(cell, series[0].label, group) +} + +// segmentStyle reports whether a series should be drawn as an error, separated +// from the rendering so tests can assert the DECISION rather than the bytes. +// +// lipgloss strips colour when it detects no TTY, which is always true under `go +// test`, so asserting on ANSI escapes cannot verify this. Returning the intent +// keeps the rule testable and leaves styling to one call site. +func isErrorSeries(label string, group usage.Group) bool { + return group == usage.GroupStatus && isErrorStatus(label) +} + +// paintSegment applies the error style when the series calls for it. +func paintSegment(text, label string, group usage.Group) string { + if isErrorSeries(label, group) { + return styleError.Render(text) + } + return text +} + +// renderLegend names each series with its glyph and total. Error statuses are +// coloured to match their segments, so the legend is the key to the chart rather +// than a separate vocabulary. +func renderLegend(series []seriesKey, group usage.Group, width int) string { + const sep = " " + var parts []string + // Track the printable width separately: paintSegment may add ANSI escapes, + // which occupy no columns but do inflate len(). Measuring the styled string + // would under-fill the line; measuring the plain text keeps it honest. + plain := 2 // leading indent + elided := 0 + + for i, s := range series { + entry := fmt.Sprintf("%c %s (%s)", glyphFor(i), s.label, humanizeCount(s.total)) + cost := len([]rune(entry)) + if len(parts) > 0 { + cost += len(sep) + } + // Reserve room for the "(+N more)" note so the line cannot overflow while + // admitting the very entry that would need eliding. + if plain+cost+len(" (+99 more)") > width && len(parts) > 0 { + elided = len(series) - i + break + } + parts = append(parts, paintSegment(entry, s.label, group)) + plain += cost + // Past the glyph set every series shares the overflow marker, so naming + // them individually stops being informative. + if i+1 >= len(segmentGlyphs) { + elided = len(series) - i - 1 + break + } + } + out := " " + strings.Join(parts, sep) + if elided > 0 { + out += fmt.Sprintf("%s(+%d more)", sep, elided) + } + return out +} diff --git a/authbridge/cmd/abctl/tui/usage_stacked_test.go b/authbridge/cmd/abctl/tui/usage_stacked_test.go new file mode 100644 index 000000000..8d3180eb9 --- /dev/null +++ b/authbridge/cmd/abctl/tui/usage_stacked_test.go @@ -0,0 +1,196 @@ +package tui + +import ( + "strings" + "testing" + "time" + + "github.com/rossoctl/cortex/authbridge/authlib/usage" +) + +// mkSeriesBuckets builds buckets whose Series carry the given per-label request +// counts, one bucket per map in the slice. +func mkSeriesBuckets(perBucket []map[string]int64) []usage.Bucket { + base := time.Date(2026, 9, 6, 23, 24, 0, 0, time.UTC) + out := make([]usage.Bucket, 0, len(perBucket)) + for i, labels := range perBucket { + b := usage.Bucket{At: base.Add(time.Duration(i) * time.Minute)} + series := map[string]usage.Counts{} + for label, n := range labels { + series[label] = usage.Counts{Requests: n, Tokens: n * 10} + b.Counts.Requests += n + b.Counts.Tokens += n * 10 + } + if len(series) > 0 { + b.Series = series + } + out = append(out, b) + } + return out +} + +// stripANSI removes escape sequences so glyph assertions are not defeated by +// colour codes. +func stripANSI(s string) string { + var b strings.Builder + for i := 0; i < len(s); { + if s[i] == 0x1b { + for i < len(s) && s[i] != 'm' { + i++ + } + i++ // skip the 'm' + continue + } + b.WriteByte(s[i]) + i++ + } + return b.String() +} + +// Each series must get its own texture, so the chart is readable with no colour +// at all — a terminal without colour support, a colour-vision deficiency, or a +// screenshot in an issue. +func TestRenderStacked_DistinctGlyphsPerSeries(t *testing.T) { + buckets := mkSeriesBuckets([]map[string]int64{ + {"200": 10, "429": 5, "500": 2}, + }) + lines := renderStackedBars(buckets, metricRequests, usage.GroupStatus, 80) + plot := stripANSI(strings.Join(lines, "\n")) + + // Three series, so three distinct glyphs from the densest end. + for _, g := range []string{"█", "▓", "▒"} { + if !strings.Contains(plot, g) { + t.Errorf("expected glyph %q in the stack; got:\n%s", g, plot) + } + } +} + +// Which series render as errors. Asserted on the DECISION, not on ANSI bytes: +// lipgloss strips colour when it detects no TTY, which is always the case under +// `go test`, so a byte-level assertion would pass vacuously and keep passing if +// the rule broke. +func TestRenderStacked_ErrorSeriesDecision(t *testing.T) { + for _, tc := range []struct { + label string + group usage.Group + want bool + }{ + {"500", usage.GroupStatus, true}, + {"429", usage.GroupStatus, true}, + {"400", usage.GroupStatus, true}, + {"denied", usage.GroupStatus, true}, + {"200", usage.GroupStatus, false}, + {"304", usage.GroupStatus, false}, + // Gated on the grouping: "429" is a plausible model name, and a method + // chart must not turn red because a label happens to look like a status. + {"429", usage.GroupMethod, false}, + {"500", usage.GroupPlugin, false}, + {"claude-sonnet-5", usage.GroupMethod, false}, + } { + if got := isErrorSeries(tc.label, tc.group); got != tc.want { + t.Errorf("isErrorSeries(%q, %s) = %v, want %v", tc.label, tc.group, got, tc.want) + } + } +} + +// paintSegment must leave a non-error series byte-identical, so any styling it +// does apply is attributable to the error rule alone. +func TestPaintSegment_LeavesNonErrorsUntouched(t *testing.T) { + const text = "████" + if got := paintSegment(text, "200", usage.GroupStatus); got != text { + t.Errorf("paintSegment styled a 2xx series: %q", got) + } + if got := paintSegment(text, "429", usage.GroupMethod); got != text { + t.Errorf("paintSegment styled a method label: %q", got) + } +} + +func TestIsErrorStatus(t *testing.T) { + for _, tc := range []struct { + label string + want bool + }{ + {"200", false}, {"201", false}, {"304", false}, + {"400", true}, {"429", true}, {"500", true}, {"503", true}, + {"denied", true}, + {"claude-sonnet-5", false}, + {"4", false}, {"40", false}, {"4000", false}, // wrong length + {"", false}, + {"(other)", false}, + } { + if got := isErrorStatus(tc.label); got != tc.want { + t.Errorf("isErrorStatus(%q) = %v, want %v", tc.label, got, tc.want) + } + } +} + +// Segment order must be stable across renders, or the chart appears to reshuffle +// on every 20s poll even when nothing changed. +func TestRenderStacked_SeriesOrderIsStable(t *testing.T) { + buckets := mkSeriesBuckets([]map[string]int64{ + {"a": 5, "b": 5, "c": 5}, // equal totals: ties must break deterministically + }) + first := stripANSI(strings.Join(renderStackedBars(buckets, metricRequests, usage.GroupStatus, 80), "\n")) + for i := 0; i < 5; i++ { + again := stripANSI(strings.Join(renderStackedBars(buckets, metricRequests, usage.GroupStatus, 80), "\n")) + if again != first { + t.Fatal("render is not deterministic for equal-total series") + } + } +} + +// A grouped view with no labelled traffic must still show the volume it knows +// about rather than an empty frame. +func TestRenderStacked_NoSeriesFallsBackToBars(t *testing.T) { + base := time.Date(2026, 9, 6, 23, 24, 0, 0, time.UTC) + buckets := []usage.Bucket{ + {At: base, Counts: usage.Counts{Requests: 3, Tokens: 300}}, // no Series + } + lines := renderStackedBars(buckets, metricTokens, usage.GroupStatus, 80) + if !strings.ContainsAny(strings.Join(lines, "\n"), "▁▂▃▄▅▆▇█") { + t.Error("no bars drawn when Series is absent; the frame is empty") + } +} + +// Every line must fit the terminal, colour codes excluded — they occupy no +// columns but do inflate len(). +func TestRenderStacked_FitsWidth(t *testing.T) { + buckets := mkSeriesBuckets([]map[string]int64{ + {"200": 100, "429": 50, "500": 25, "503": 10, "418": 5}, + {"200": 80, "429": 40}, + }) + for _, width := range []int{80, 100, 60} { + for _, line := range renderStackedBars(buckets, metricRequests, usage.GroupStatus, width) { + if got := len([]rune(stripANSI(line))); got > width { + t.Errorf("width %d: line is %d columns:\n%q", width, got, stripANSI(line)) + } + } + } +} + +// Series past the glyph set fold into the overflow marker, and the legend says +// how many were elided rather than running off the terminal. +func TestRenderStacked_OverflowSeriesAreMarked(t *testing.T) { + labels := map[string]int64{} + for i := 0; i < 12; i++ { + labels[string(rune('a'+i))] = int64(12 - i) + } + lines := renderStackedBars(mkSeriesBuckets([]map[string]int64{labels}), metricRequests, usage.GroupMethod, 80) + legend := stripANSI(lines[len(lines)-1]) + if !strings.Contains(legend, "more)") { + t.Errorf("legend does not report elided series: %q", legend) + } +} + +// Non-zero traffic must never render as an empty column, matching the bar chart. +func TestRenderStacked_SmallBucketStillDraws(t *testing.T) { + buckets := mkSeriesBuckets([]map[string]int64{ + {"200": 10000}, + {"200": 1}, // 1/10000 of the peak + }) + lines := renderStackedBars(buckets, metricRequests, usage.GroupStatus, 80) + bottom := stripANSI(lines[plotRows-1]) // last plot row + if strings.Count(bottom, "█") < barWidth*2 { + t.Errorf("the tiny bucket drew no segment:\n%q", bottom) + } +} diff --git a/authbridge/cmd/abctl/tui/usage_state_test.go b/authbridge/cmd/abctl/tui/usage_state_test.go index ac0fbfae4..2a5b1ebe6 100644 --- a/authbridge/cmd/abctl/tui/usage_state_test.go +++ b/authbridge/cmd/abctl/tui/usage_state_test.go @@ -2,6 +2,8 @@ package tui import ( tea "github.com/charmbracelet/bubbletea" + "strings" + "time" "testing" @@ -155,12 +157,73 @@ func TestUsageState_CyclesWrap(t *testing.T) { } } seen := map[usageMetric]bool{} - for i := 0; i < 6; i++ { + for i := 0; i < usageMetricCount*2; i++ { u.cycleMetric() seen[u.metric] = true + if u.metric < 0 || u.metric >= usageMetricCount { + t.Fatalf("metric %d out of range after %d cycles", u.metric, i+1) + } + } + if len(seen) != usageMetricCount { + t.Errorf("metric cycle covered %d of %d metrics", len(seen), usageMetricCount) + } +} + +// [g] must visit every grouping and return to ungrouped, so an operator can +// always get back to the sub-row-precision view without leaving the pane. +func TestUsageState_GroupCycleVisitsAllAndReturns(t *testing.T) { + var u usageState // zero value is GroupNone ("") + + want := []usage.Group{usage.GroupStatus, usage.GroupMethod, usage.GroupPlugin, usage.GroupNone} + for i, w := range want { + u.cycleGroup() + if u.group != w { + t.Fatalf("cycle %d: group = %q, want %q", i+1, u.group, w) + } + } + // And it keeps cycling rather than sticking. + u.cycleGroup() + if u.group != usage.GroupStatus { + t.Errorf("group = %q after a full lap, want status", u.group) + } +} + +// Latency needs the whiskers renderer; everything else uses bars, stacked when a +// grouping is active. Dispatch is what makes the three forms reachable at all. +func TestRenderUsageChart_PicksTheRightForm(t *testing.T) { + const nl = "\n" + base := time.Date(2026, 9, 6, 23, 24, 0, 0, time.UTC) + snap := &usage.Snapshot{Buckets: []usage.Bucket{{ + At: base, + Counts: usage.Counts{Requests: 10, Tokens: 1000}, + LatMeanMs: 2000, + LatStdDevMs: 400, + LatSamples: 10, + Series: map[string]usage.Counts{"200": {Requests: 10, Tokens: 1000}}, + }}} + + // Latency -> whisker glyphs, never block glyphs. + latency := strings.Join(renderUsageChart(snap, metricLatency, usage.GroupNone, 80), nl) + if !strings.ContainsRune(latency, whiskerMean) { + t.Error("latency did not use the whiskers renderer") + } + if strings.ContainsAny(latency, "▁▂▃▄▅▆▇█") { + t.Error("latency drew bars") + } + // Latency ignores grouping: there is no per-label latency to break down. + grouped := strings.Join(renderUsageChart(snap, metricLatency, usage.GroupStatus, 80), nl) + if grouped != latency { + t.Error("grouping changed the latency chart, implying a breakdown that does not exist") + } + + // Grouped counts -> a legend; ungrouped -> none. + stacked := strings.Join(renderUsageChart(snap, metricTokens, usage.GroupStatus, 80), nl) + if !strings.Contains(stripANSI(stacked), "200 (") { + t.Error("grouped chart has no legend") } - if len(seen) != 3 { - t.Errorf("metric cycle covered %d of 3 metrics", len(seen)) + plain := strings.Join(renderUsageChart(snap, metricTokens, usage.GroupNone, 80), nl) + if strings.Contains(plain, "200 (") { + t.Error("ungrouped chart rendered a legend") } } diff --git a/authbridge/cmd/abctl/tui/usage_whiskers.go b/authbridge/cmd/abctl/tui/usage_whiskers.go new file mode 100644 index 000000000..6cfecdc20 --- /dev/null +++ b/authbridge/cmd/abctl/tui/usage_whiskers.go @@ -0,0 +1,233 @@ +package tui + +import ( + "fmt" + "strings" + + "github.com/rossoctl/cortex/authbridge/authlib/usage" +) + +// Whisker glyphs. The mean is a crossbar, the +/-1σ caps are tees, and the span +// between them is a vertical rule. +// +// A bar chart is the wrong form for latency and this is why: a bar encodes +// magnitude from a zero baseline, but mean latency has no meaningful zero — a +// response taking 0ms is not the absence of a response — and the spread is +// usually the more interesting half. Drawing the mean with its dispersion says +// "typical, plus how much it varies", which is the question an operator actually +// has. +const ( + whiskerMean = '┼' + whiskerCap = '┬' // upper cap, +1σ + whiskerFoot = '┴' // lower cap, -1σ + whiskerRule = '│' +) + +// latencyRow is one bucket reduced to the three numbers the chart draws. +type latencyRow struct { + bucket usage.Bucket + // mean and the sigma band, in milliseconds. lo is clamped at zero: a mean of + // 200ms with a 500ms sigma would otherwise draw a cap below the axis, which + // reads as negative latency. + mean, lo, hi float64 + // measured is how many requests carried a duration. Zero means this bucket + // has no latency to draw at all, which is not the same as zero latency. + measured int64 +} + +// latencyRows projects buckets into plottable rows. +func latencyRows(buckets []usage.Bucket) []latencyRow { + out := make([]latencyRow, 0, len(buckets)) + for _, b := range buckets { + r := latencyRow{bucket: b, measured: b.LatSamples} + if b.LatSamples > 0 && b.LatMeanMs > 0 { + r.mean = b.LatMeanMs + r.hi = b.LatMeanMs + b.LatStdDevMs + r.lo = b.LatMeanMs - b.LatStdDevMs + if r.lo < 0 { + r.lo = 0 + } + } + out = append(out, r) + } + return out +} + +// renderWhiskers draws mean-with-whiskers latency per bucket. +// +// Scaled to the highest +1σ rather than the highest mean, so a cap is never +// clipped off the top of the frame — a whisker that runs past the axis tells the +// reader less than one that fits. +func renderWhiskers(buckets []usage.Bucket, width int) []string { + if len(buckets) == 0 { + return []string{" (no data)"} + } + maxBars := (width - axisLabel) / barStride + if maxBars < 1 { + maxBars = 1 + } + if len(buckets) > maxBars { + buckets = buckets[len(buckets)-maxBars:] + } + + rows := latencyRows(buckets) + + var peak float64 + for _, r := range rows { + if r.hi > peak { + peak = r.hi + } + } + if peak <= 0 { + // No bucket carried a duration. Say so rather than draw an empty grid that + // looks like latency was zero. + return []string{ + " (no latency samples in this window)", + "", + " Latency is recorded per response; a window with no measured", + " responses has nothing to plot.", + } + } + + out := make([]string, 0, plotRows+4) + lastAxisLabel := "" + for row := plotRows; row >= 1; row-- { + var sb strings.Builder + if row%2 == 0 { + label := humanizeDurationMs(peak * float64(row) / float64(plotRows)) + // Suppress a repeat: when the peak is small every gridline rounds to + // the same string, and a column of identical labels reads as a bug + // rather than as a collapsed scale. + if label == lastAxisLabel { + sb.WriteString(strings.Repeat(" ", axisLabel)) + } else { + lastAxisLabel = label + sb.WriteString(fmt.Sprintf("%5s ", label)) + } + } else { + sb.WriteString(strings.Repeat(" ", axisLabel)) + } + for _, r := range rows { + sb.WriteString(whiskerCell(r, peak, row)) + sb.WriteString(strings.Repeat(" ", barGap)) + } + out = append(out, strings.TrimRight(sb.String(), " ")) + } + + out = append(out, renderAxis(len(rows))) + out = append(out, renderTimeLabels(buckets)) + out = append(out, renderLatencyValues(rows)) + out = append(out, "") + legend := fmt.Sprintf(" %c mean %c +1σ %c −1σ (0 = no measured responses)", + whiskerMean, whiskerCap, whiskerFoot) + // Drop the parenthetical before the glyph key: on a narrow terminal the key is + // what makes the chart legible, and a wrapped line breaks the layout outright. + if len([]rune(legend)) > width { + legend = fmt.Sprintf(" %c mean %c +1σ %c −1σ", whiskerMean, whiskerCap, whiskerFoot) + } + if r := []rune(legend); len(r) > width { + legend = string(r[:width]) + } + out = append(out, legend) + return out +} + +// whiskerCell renders one bucket's glyph for one row. Centred in the bar width so +// the marks line up with the bars in the other two views. +func whiskerCell(r latencyRow, peak float64, row int) string { + blank := strings.Repeat(" ", barWidth) + if r.measured == 0 || r.mean <= 0 { + return blank + } + + // Which plot row each of the three marks falls on. Ceil so a small non-zero + // value lands on row 1 rather than row 0 and disappears. + rowOf := func(v float64) int { + if v <= 0 { + return 1 + } + n := int((v/peak)*float64(plotRows) + 0.999) + if n < 1 { + n = 1 + } + if n > plotRows { + n = plotRows + } + return n + } + meanRow, hiRow, loRow := rowOf(r.mean), rowOf(r.hi), rowOf(r.lo) + + var g rune + switch { + case row == meanRow: + g = whiskerMean // mean wins where marks collide: it is the headline number + case row == hiRow: + g = whiskerCap + case row == loRow: + g = whiskerFoot + case row > loRow && row < hiRow: + g = whiskerRule + default: + return blank + } + + // Centre the mark: (barWidth-1)/2 leading spaces puts it under the middle of + // the bar column the other renderers fill. + lead := (barWidth - 1) / 2 + return strings.Repeat(" ", lead) + string(g) + strings.Repeat(" ", barWidth-lead-1) +} + +// renderLatencyValues prints each bucket's mean under its mark, or "0" where no +// response was measured — the same idle-versus-small distinction the bar chart's +// value row makes. +func renderLatencyValues(rows []latencyRow) string { + row := make([]byte, axisLabel+len(rows)*barStride+8) + for i := range row { + row[i] = ' ' + } + for i, r := range rows { + label := "0" + if r.measured > 0 && r.mean > 0 { + label = humanizeDurationMs(r.mean) + } + at := axisLabel - 1 + i*barStride + if at+len(label) <= len(row) { + copy(row[at:], label) + } + } + return strings.TrimRight(string(row), " ") +} + +// maxDurationLabelLen is the width humanizeDurationMs promises, matching +// humanizeCount so both renderers share one gutter geometry. +const maxDurationLabelLen = 5 + +// humanizeDurationMs renders a millisecond duration in at most +// maxDurationLabelLen characters. +// +// Every magnitude is covered rather than only the plausible ones — the same +// lesson as humanizeCount, which promised 5 characters and returned 7 above a +// billion because nobody revisited the fallthrough. +func humanizeDurationMs(ms float64) string { + switch { + case ms <= 0: + return "0" + case ms < 1: + return "<1ms" + case ms < 9.95: + // Bounded below 9.95, not 10: %.1f rounds 9.99 up to "10.0ms", which is + // six characters and breaks the width promise the gutter is laid out + // against. Same trap one branch down. + return fmt.Sprintf("%.1fms", ms) // 1.0ms..9.9ms + case ms < 1000: + return fmt.Sprintf("%dms", int64(ms)) // 10ms..999ms + case ms < 9_950: + return fmt.Sprintf("%.1fs", ms/1000) // 1.0s..9.9s + case ms < 600_000: + return fmt.Sprintf("%ds", int64(ms)/1000) // 10s..599s + case ms < 36_000_000: + return fmt.Sprintf("%dm", int64(ms)/60_000) // 10m..599m + default: + return ">10h" + } +} diff --git a/authbridge/cmd/abctl/tui/usage_whiskers_test.go b/authbridge/cmd/abctl/tui/usage_whiskers_test.go new file mode 100644 index 000000000..d92733525 --- /dev/null +++ b/authbridge/cmd/abctl/tui/usage_whiskers_test.go @@ -0,0 +1,180 @@ +package tui + +import ( + "strings" + "testing" + "time" + + "github.com/rossoctl/cortex/authbridge/authlib/usage" +) + +// mkLatencyBuckets builds buckets carrying mean/stddev/sample triples. +func mkLatencyBuckets(triples [][3]float64) []usage.Bucket { + base := time.Date(2026, 9, 6, 23, 24, 0, 0, time.UTC) + out := make([]usage.Bucket, 0, len(triples)) + for i, tr := range triples { + b := usage.Bucket{At: base.Add(time.Duration(i) * time.Minute)} + b.LatMeanMs, b.LatStdDevMs = tr[0], tr[1] + b.LatSamples = int64(tr[2]) + b.Counts.Requests = b.LatSamples + out = append(out, b) + } + return out +} + +// The three marks must all appear: mean, upper cap and lower cap, joined by a +// rule. A bar would encode only the mean, which is the reason this form exists. +func TestRenderWhiskers_DrawsMeanAndBothCaps(t *testing.T) { + lines := renderWhiskers(mkLatencyBuckets([][3]float64{{2000, 600, 10}}), 80) + joined := strings.Join(lines, "\n") + + for name, g := range map[string]rune{ + "mean": whiskerMean, "upper cap": whiskerCap, "lower cap": whiskerFoot, + } { + if !strings.ContainsRune(joined, g) { + t.Errorf("%s glyph %q missing:\n%s", name, string(g), joined) + } + } +} + +// A bucket with no measured response must print "0", not a mark at the baseline. +// Zero latency and no measurement are different facts, and only one of them +// means the service was fast. +func TestRenderWhiskers_UnmeasuredBucketIsStated(t *testing.T) { + lines := renderWhiskers(mkLatencyBuckets([][3]float64{ + {2000, 100, 5}, + {0, 0, 0}, // requests happened but none carried a duration + }), 80) + values := lines[plotRows+2] // the value row, after axis + time labels + + if !strings.Contains(values, "0") { + t.Errorf("value row does not mark the unmeasured bucket:\n%q", values) + } +} + +// A window with nothing measured must say so rather than draw an empty grid, +// which reads as "latency was zero". +func TestRenderWhiskers_NoSamplesExplainsItself(t *testing.T) { + lines := renderWhiskers(mkLatencyBuckets([][3]float64{{0, 0, 0}, {0, 0, 0}}), 80) + joined := strings.Join(lines, "\n") + if !strings.Contains(joined, "no latency samples") { + t.Errorf("an all-unmeasured window should explain itself, got:\n%s", joined) + } + if strings.ContainsRune(joined, whiskerMean) { + t.Error("drew a mean mark for a window with no samples") + } +} + +// A sigma wider than the mean must clamp the lower cap at the axis: a whisker +// below zero reads as negative latency. +func TestRenderWhiskers_LowerCapClampsAtZero(t *testing.T) { + rows := latencyRows(mkLatencyBuckets([][3]float64{{200, 500, 10}})) + if rows[0].lo < 0 { + t.Errorf("lo = %v, want clamped to 0 (mean 200 with sigma 500)", rows[0].lo) + } + if rows[0].hi != 700 { + t.Errorf("hi = %v, want 700", rows[0].hi) + } +} + +// The frame must scale to the tallest +1σ, not the tallest mean, or a cap is +// clipped off the top and tells the reader less than one that fits. +func TestRenderWhiskers_ScalesToUpperCap(t *testing.T) { + // Bucket 2's mean is lower but its sigma pushes its cap highest. + lines := renderWhiskers(mkLatencyBuckets([][3]float64{ + {1000, 50, 10}, + {800, 4000, 10}, // cap at 4800 + }), 80) + + // The top axis label must cover the highest cap. + top := "" + for _, l := range lines { + if strings.TrimSpace(l) != "" { + top = l + break + } + } + if !strings.Contains(top, "4") && !strings.Contains(top, "s") { + t.Errorf("top axis label %q does not reflect the 4800ms cap", top) + } + // And the tall bucket's cap must be inside the frame, not clipped. + joined := strings.Join(lines[:plotRows], "\n") + if !strings.ContainsRune(joined, whiskerCap) { + t.Error("upper cap was clipped out of the plot area") + } +} + +// Marks must centre under the same columns the bar renderers fill, so switching +// metrics does not shift the chart sideways. +func TestRenderWhiskers_MarksAlignWithBarColumns(t *testing.T) { + lines := renderWhiskers(mkLatencyBuckets([][3]float64{{2000, 100, 5}, {3000, 100, 5}}), 80) + + for _, l := range lines[:plotRows] { + for i, r := range []rune(l) { + if r != whiskerMean && r != whiskerCap && r != whiskerFoot && r != whiskerRule { + continue + } + // Mark column = axisLabel + k*barStride + (barWidth-1)/2. + off := i - axisLabel - (barWidth-1)/2 + if off < 0 || off%barStride != 0 { + t.Errorf("mark at column %d is not centred on a bar column", i) + } + } + } +} + +func TestRenderWhiskers_FitsWidth(t *testing.T) { + var triples [][3]float64 + for i := 0; i < 10; i++ { + triples = append(triples, [3]float64{float64(1000 * (i + 1)), 300, 5}) + } + for _, width := range []int{80, 100, 60, 40} { + for _, line := range renderWhiskers(mkLatencyBuckets(triples), width) { + if got := len([]rune(line)); got > width { + t.Errorf("width %d: line is %d columns:\n%q", width, got, line) + } + } + } +} + +func TestRenderWhiskers_EmptyInput(t *testing.T) { + if got := renderWhiskers(nil, 80); len(got) != 1 { + t.Errorf("renderWhiskers(nil) = %v, want one placeholder line", got) + } +} + +func TestHumanizeDurationMs(t *testing.T) { + for _, tc := range []struct { + in float64 + want string + }{ + {0, "0"}, {0.4, "<1ms"}, {1, "1.0ms"}, {9.9, "9.9ms"}, + {10, "10ms"}, {999, "999ms"}, + {1000, "1.0s"}, {9900, "9.9s"}, + {10_000, "10s"}, {599_000, "599s"}, + {600_000, "10m"}, + {36_000_000, ">10h"}, + } { + if got := humanizeDurationMs(tc.in); got != tc.want { + t.Errorf("humanizeDurationMs(%v) = %q, want %q", tc.in, got, tc.want) + } + } +} + +// The width promise is what the gutter is laid out against, so it must hold for +// every magnitude — the same lesson as humanizeCount, which returned 7 characters +// above a billion because nobody revisited its fallthrough. +func TestHumanizeDurationMs_NeverExceedsWidth(t *testing.T) { + vals := []float64{0, -1, 0.001, 0.999} + for _, base := range []float64{1, 4.44, 9.99} { + for mag := 1.0; mag <= 1e15; mag *= 10 { + vals = append(vals, base*mag) + } + } + for _, v := range vals { + if got := humanizeDurationMs(v); len([]rune(got)) > maxDurationLabelLen { + t.Errorf("humanizeDurationMs(%v) = %q (%d chars), cap is %d", + v, got, len([]rune(got)), maxDurationLabelLen) + } + } +} From f445ff851885b228c8cadad3e534a3a6ec698bb8 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 6 Sep 2026 11:02:38 -0400 Subject: [PATCH 2/8] fix: Make tiny series visible and replace shaded blocks with letter marks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two problems reported against `tokens — by method`, where the reporter's three models were 2.2M / 105k / 912 tokens. A series that is a rounding error of the bucket drew no band at all. The per-series floor existed but could not work: rows were allotted proportionally with a floor of 1 applied afterward, so with three series in a ten-row bar the largest took 9 rows and the two floored ones landed on rows 10 and 11 — the eleventh outside the bar, so its series vanished anyway. Guaranteed rows are now reserved FIRST and the remainder shared by proportion, so the allotment always sums to the bar height exactly. When there are more series than rows, as many as fit get a row each, largest first, and the legend still names the rest. The shaded blocks (█ ▓ ▒ ░) were the wrong encoding. They looked principled but █ against ▓ is nearly indistinguishable at a glance in most terminal fonts, so a reader could not tell which band was which without counting against the legend. Each series now gets a letter derived from its label on a coloured background: claude-sonnet-5 is `s`, claude-opus-5 `o`, claude-haiku-4-5-20251001 `h`. A band is self-describing, and the letter alone identifies the series — colour is an accelerant, so the chart still works in a monochrome terminal, with a colour-vision deficiency, and in a screenshot. Deriving the letter needs the vendor prefix skipped or every Claude model yields `c`, which is the collision the scheme exists to prevent. Leading vendor tokens are dropped repeatedly, not once, so "anthropic/claude- sonnet-5" also gives `s`. Letters are deduped in rank order, so the largest series keeps the intuitive one. Reds are absent from the series palette: red is reserved for the >= 400 status rule, and a series landing on red would read as an error. The legend now wraps instead of eliding. It is the only key to a band, and three model names do not fit 80 columns on one line — the previous version reserved room for a "(+N more)" note even on the last entry, which dropped a present model from the legend and left its band unidentifiable. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/styles.go | 7 + authbridge/cmd/abctl/tui/usage_glyphs.go | 153 ++++++++++++ authbridge/cmd/abctl/tui/usage_stacked.go | 230 ++++++++++++------ .../cmd/abctl/tui/usage_stacked_test.go | 165 ++++++++++++- 4 files changed, 474 insertions(+), 81 deletions(-) create mode 100644 authbridge/cmd/abctl/tui/usage_glyphs.go diff --git a/authbridge/cmd/abctl/tui/styles.go b/authbridge/cmd/abctl/tui/styles.go index 83fb48946..4a1ce7f75 100644 --- a/authbridge/cmd/abctl/tui/styles.go +++ b/authbridge/cmd/abctl/tui/styles.go @@ -17,6 +17,13 @@ var ( colorMuted = lipgloss.AdaptiveColor{Light: "#6B7280", Dark: "#9CA3AF"} colorInbound = lipgloss.AdaptiveColor{Light: "#1D4ED8", Dark: "#93C5FD"} colorOutbound = lipgloss.AdaptiveColor{Light: "#B45309", Dark: "#FCD34D"} + + // colorOnSeries is the foreground for a letter drawn on a series background + // (the usage pane's stacked bars). Inverted relative to the palette: those + // backgrounds are mid-tone in both themes, so the mark needs the opposite end + // of the ramp to stay legible — white on the darker light-theme grounds, near + // black on the lighter dark-theme ones. + colorOnSeries = lipgloss.AdaptiveColor{Light: "#FFFFFF", Dark: "#111827"} ) var ( diff --git a/authbridge/cmd/abctl/tui/usage_glyphs.go b/authbridge/cmd/abctl/tui/usage_glyphs.go new file mode 100644 index 000000000..52b009f46 --- /dev/null +++ b/authbridge/cmd/abctl/tui/usage_glyphs.go @@ -0,0 +1,153 @@ +package tui + +import ( + "strings" + "unicode" + + "github.com/charmbracelet/lipgloss" +) + +// Series marks: a letter per series, on a coloured background. +// +// Replaces the shaded blocks (█ ▓ ▒ ░). Shading looked principled but failed in +// practice: █ against ▓ is nearly indistinguishable at a glance in most terminal +// fonts, so a reader could not tell which segment was which without counting +// against the legend. A letter is unambiguous at any size, and one derived from +// the label is self-describing — an `s` segment against a legend entry reading +// `s claude-sonnet-5` needs no decoding. +// +// Colour is layered on top for scanning speed, never as the encoding: the letter +// alone identifies the series, so the chart survives a monochrome terminal, a +// colour-vision deficiency, and a screenshot. +// +// seriesPalette is ordered so adjacent series differ in hue as well as letter. +// Reds are deliberately absent: red is reserved for the >= 400 status rule, and a +// series that happened to land on red would read as an error. +var seriesPalette = []lipgloss.AdaptiveColor{ + {Light: "#1D4ED8", Dark: "#93C5FD"}, // blue + {Light: "#047857", Dark: "#6EE7B7"}, // green + {Light: "#7C3AED", Dark: "#C4B5FD"}, // violet + {Light: "#B45309", Dark: "#FCD34D"}, // amber + {Light: "#0E7490", Dark: "#67E8F9"}, // cyan + {Light: "#9D174D", Dark: "#F9A8D4"}, // magenta +} + +// vendorPrefixes are leading tokens that identify a provider rather than a model, +// so they are skipped when deriving a letter. Without this every Anthropic model +// yields "c" for claude — the letters would collide precisely where a reader most +// needs them to differ. +// +// Matched case-insensitively against the first "-"/"_"/"/"/"." separated token. +var vendorPrefixes = map[string]bool{ + "claude": true, "anthropic": true, "openai": true, "gpt": true, + "azure": true, "aws": true, "bedrock": true, "vertex": true, + "google": true, "gemini": true, "meta": true, "llama": true, + "mistral": true, "cohere": true, "ollama": true, "litellm": true, +} + +// seriesLetter derives a one-character mark from a label. +// +// The rules, in order: skip a vendor prefix so sibling models differ; prefer a +// letter over a digit so "claude-sonnet-5" is `s` and not `5`; fall back to the +// first usable character; and use '?' when nothing qualifies. Callers dedupe the +// result — see assignLetters. +func seriesLetter(label string) rune { + tokens := strings.FieldsFunc(label, func(r rune) bool { + return r == '-' || r == '_' || r == '/' || r == '.' + }) + if len(tokens) == 0 { + return '?' + } + // Drop leading vendor tokens, not just one: "anthropic/claude-sonnet-5" has + // two stacked, and stopping after the first still yields "c" for every Claude + // model — the collision this exists to prevent. Always keep the last token so + // a label that is nothing but vendor names still gets a mark. + for len(tokens) > 1 && vendorPrefixes[strings.ToLower(tokens[0])] { + tokens = tokens[1:] + } + // Prefer the first alphabetic character across the remaining tokens. + for _, tok := range tokens { + for _, r := range tok { + if unicode.IsLetter(r) { + return unicode.ToLower(r) + } + } + } + // No letters at all — a status code, say. Use its first character. + for _, r := range tokens[0] { + if unicode.IsPrint(r) { + return r + } + } + return '?' +} + +// assignLetters maps each series label to a unique mark, in rank order. +// +// Uniqueness matters more than the mnemonic: two series sharing a letter is the +// failure the shaded blocks already had. When a derived letter collides, later +// characters of the label are tried, then the alphabet, then a digit — so the +// first (largest) series keeps the intuitive letter and the collision cost falls +// on the smaller one. +func assignLetters(series []seriesKey) map[string]rune { + out := make(map[string]rune, len(series)) + used := make(map[rune]bool, len(series)) + + claim := func(label string, r rune) bool { + if r == 0 || used[r] { + return false + } + used[r] = true + out[label] = r + return true + } + + for _, s := range series { + if claim(s.label, seriesLetter(s.label)) { + continue + } + // Try the label's own remaining letters before falling back, so the mark + // stays connected to the name where possible. + done := false + for _, r := range strings.ToLower(s.label) { + if unicode.IsLetter(r) && claim(s.label, r) { + done = true + break + } + } + if done { + continue + } + for r := 'a'; r <= 'z'; r++ { + if claim(s.label, r) { + done = true + break + } + } + if done { + continue + } + for r := '0'; r <= '9'; r++ { + if claim(s.label, r) { + break + } + } + } + return out +} + +// seriesStyle returns the style for a series' mark: a coloured background with a +// readable foreground. +// +// An error series (>= 400 under group=status) always takes the error colour, +// overriding its palette slot — a 500 must look like a failure regardless of +// where it ranks. +func seriesStyle(rank int, isError bool) lipgloss.Style { + bg := seriesPalette[rank%len(seriesPalette)] + if isError { + bg = colorError + } + // Bold on a coloured ground: terminals vary in how they render dim text on a + // background, and the mark has to stay legible in all of them. + return lipgloss.NewStyle().Background(bg).Foreground(colorOnSeries).Bold(true) +} diff --git a/authbridge/cmd/abctl/tui/usage_stacked.go b/authbridge/cmd/abctl/tui/usage_stacked.go index e47441ed5..9a45bd339 100644 --- a/authbridge/cmd/abctl/tui/usage_stacked.go +++ b/authbridge/cmd/abctl/tui/usage_stacked.go @@ -8,22 +8,10 @@ import ( "github.com/rossoctl/cortex/authbridge/authlib/usage" ) -// segmentGlyphs are the textures a stacked bar cycles through, densest first. -// -// Redundant with color on purpose. Color alone fails three ways here: a terminal -// with no color support, a viewer with a colour-vision deficiency, and a -// screenshot pasted into an issue. A distinct glyph per series survives all -// three, so the chart is readable from texture alone and color is an accelerant -// rather than the encoding. -// -// Ordered densest-to-sparsest so the largest series (sorted first below) is also -// the visually heaviest, which matches what a reader expects from a stack. -var segmentGlyphs = [...]rune{'█', '▓', '▒', '░'} - -// overflowGlyph marks the series past len(segmentGlyphs) — including the -// aggregator's own "(other)" bucket. Distinct from the four above so "several -// small series folded together" never looks like a named one. -const overflowGlyph = '·' +// maxNamedSeries is how many series get their own mark and legend entry before +// the rest fold together. Bounded by the palette so no two named series share a +// colour. +var maxNamedSeries = len(seriesPalette) // seriesKey is one label's total across the window, used to decide segment order // and which labels get their own glyph. @@ -56,14 +44,6 @@ func collectSeries(buckets []usage.Bucket, m usageMetric) []seriesKey { return out } -// glyphFor assigns each series its texture by rank. -func glyphFor(rank int) rune { - if rank < len(segmentGlyphs) { - return segmentGlyphs[rank] - } - return overflowGlyph -} - // isErrorStatus reports whether a group=status label denotes a failure, so it can // be drawn in red. // @@ -115,6 +95,9 @@ func renderStackedBars(buckets []usage.Bucket, m usageMetric, group usage.Group, for i, s := range series { rank[s.label] = i } + // One mark per series, assigned once for the whole chart so a letter means the + // same thing in every bucket and in the legend. + letters := assignLetters(series) var peak int64 for _, b := range buckets { @@ -132,7 +115,7 @@ func renderStackedBars(buckets []usage.Bucket, m usageMetric, group usage.Group, sb.WriteString(strings.Repeat(" ", axisLabel)) } for _, b := range buckets { - sb.WriteString(stackedCell(b, m, group, series, rank, peak, row)) + sb.WriteString(stackedCell(b, m, group, series, rank, letters, peak, row)) sb.WriteString(strings.Repeat(" ", barGap)) } out = append(out, strings.TrimRight(sb.String(), " ")) @@ -142,14 +125,15 @@ func renderStackedBars(buckets []usage.Bucket, m usageMetric, group usage.Group, out = append(out, renderTimeLabels(buckets)) out = append(out, renderValues(buckets, m)) out = append(out, "") - out = append(out, renderLegend(series, group, width)) + out = append(out, renderLegend(series, group, letters, rank, width)...) return out } // stackedCell renders one bar's glyphs for one row, choosing the segment whose // cumulative height covers this row. func stackedCell(b usage.Bucket, m usageMetric, group usage.Group, - series []seriesKey, rank map[string]int, peak int64, row int) string { + series []seriesKey, rank map[string]int, letters map[string]rune, + peak int64, row int) string { total := m.value(b) if total <= 0 || peak <= 0 { @@ -165,28 +149,73 @@ func stackedCell(b usage.Bucket, m usageMetric, group usage.Group, return strings.Repeat(" ", barWidth) } - // Walk the series in rank order, accumulating rows, until we pass this row. - // Rows are allotted proportionally to each series' share of the bucket. + // Walk the allotment bottom-up until we pass this row. var acc int64 - for _, s := range series { - v := m.valueOf(b.Series[s.label]) - if v <= 0 { - continue - } - segRows := v * barRows / total - if segRows == 0 { - segRows = 1 // a present series must occupy at least one cell - } - acc += segRows + for _, alloc := range allotRows(b, m, series, barRows, total) { + acc += alloc.rows if int64(row) <= acc { - cell := strings.Repeat(string(glyphFor(rank[s.label])), barWidth) - return paintSegment(cell, s.label, group) + return paintMark(alloc.label, letters, rank, group) } } // Rounding left this row uncovered: attribute it to the largest series rather // than punching a hole in the middle of a bar. - cell := strings.Repeat(string(glyphFor(0)), barWidth) - return paintSegment(cell, series[0].label, group) + return paintMark(series[0].label, letters, rank, group) +} + +// rowAlloc is one series' share of a bar, in whole rows. +type rowAlloc struct { + label string + rows int64 +} + +// allotRows divides a bar's rows among the series present in it. +// +// Every present series gets at least one row, so a model with a rounding-error +// share of the traffic is still visible — "claude-haiku at 912 tokens against +// 2.2M" is exactly the case an operator wants to spot, and a segment floored to +// zero rows makes it indistinguishable from absent. +// +// The floor cannot simply be applied per series, which is what the previous +// version did: with three series in a ten-row bar the largest took 9 rows and the +// two floored ones landed on rows 10 and 11, so the eleventh fell outside the bar +// and its series vanished anyway. Guaranteed rows are reserved FIRST and the +// remainder shared out proportionally, so the total always fits. +func allotRows(b usage.Bucket, m usageMetric, series []seriesKey, barRows, total int64) []rowAlloc { + present := make([]rowAlloc, 0, len(series)) + for _, s := range series { + if m.valueOf(b.Series[s.label]) > 0 { + present = append(present, rowAlloc{label: s.label}) + } + } + if len(present) == 0 { + return nil + } + // More series than rows: the bar cannot show them all, so give a row each to + // as many as fit, largest first (series is already sorted). The legend still + // names the rest. + if int64(len(present)) >= barRows { + out := present[:barRows] + for i := range out { + out[i].rows = 1 + } + return out + } + + // One row reserved per series, the rest shared by proportion of the surplus. + surplus := barRows - int64(len(present)) + var assigned int64 + for i := range present { + v := m.valueOf(b.Series[present[i].label]) + extra := v * surplus / total + present[i].rows = 1 + extra + assigned += present[i].rows + } + // Integer division leaves rows unassigned; give them to the largest series so + // the bar reaches its full height. + if rem := barRows - assigned; rem > 0 { + present[0].rows += rem + } + return present } // segmentStyle reports whether a series should be drawn as an error, separated @@ -199,7 +228,24 @@ func isErrorSeries(label string, group usage.Group) bool { return group == usage.GroupStatus && isErrorStatus(label) } -// paintSegment applies the error style when the series calls for it. +// paintMark renders one series' cell: its letter, repeated across the bar width, +// on the series background. +// +// Repeated rather than centred so the segment reads as a solid band — a single +// letter floating in a 4-column cell looks like a data point, not a share of a +// stack. +func paintMark(label string, letters map[string]rune, rank map[string]int, group usage.Group) string { + r, ok := letters[label] + if !ok { + r = '?' + } + return seriesStyle(rank[label], isErrorSeries(label, group)). + Render(strings.Repeat(string(r), barWidth)) +} + +// paintSegment applies the error style to legend text when the series calls for +// it. Foreground only: a coloured background belongs on the chart marks, where it +// encodes the series, not on a line of prose. func paintSegment(text, label string, group usage.Group) string { if isErrorSeries(label, group) { return styleError.Render(text) @@ -210,39 +256,87 @@ func paintSegment(text, label string, group usage.Group) string { // renderLegend names each series with its glyph and total. Error statuses are // coloured to match their segments, so the legend is the key to the chart rather // than a separate vocabulary. -func renderLegend(series []seriesKey, group usage.Group, width int) string { +// renderLegend keys the chart: each series' mark, name and total. +// +// Returns one or more lines. Wrapping rather than eliding matters because the mark +// is the only way to identify a band — a series dropped from the legend leaves an +// unreadable segment on the chart, and model names are long enough +// ("claude-haiku-4-5-20251001") that three of them do not fit 80 columns on one +// line. Only series past maxNamedSeries fold into a count, and those share a +// colour anyway. +func renderLegend(series []seriesKey, group usage.Group, + letters map[string]rune, rank map[string]int, width int) []string { const sep = " " - var parts []string - // Track the printable width separately: paintSegment may add ANSI escapes, - // which occupy no columns but do inflate len(). Measuring the styled string - // would under-fill the line; measuring the plain text keeps it honest. - plain := 2 // leading indent + const indent = " " + + named := series elided := 0 + if len(named) > maxNamedSeries { + elided = len(named) - maxNamedSeries + named = named[:maxNamedSeries] + } - for i, s := range series { - entry := fmt.Sprintf("%c %s (%s)", glyphFor(i), s.label, humanizeCount(s.total)) - cost := len([]rune(entry)) + var lines []string + var parts []string + plain := len(indent) + + flush := func() { + if len(parts) > 0 { + lines = append(lines, indent+strings.Join(parts, sep)) + parts = nil + plain = len(indent) + } + } + + for _, s := range named { + mark := seriesStyle(rank[s.label], isErrorSeries(s.label, group)). + Render(string(letters[s.label])) + text := fmt.Sprintf(" %s (%s)", s.label, humanizeCount(s.total)) + // Measured on the plain text: the mark is one column however many bytes of + // escape sequence it carries. + cost := 1 + len([]rune(text)) if len(parts) > 0 { cost += len(sep) } - // Reserve room for the "(+N more)" note so the line cannot overflow while - // admitting the very entry that would need eliding. - if plain+cost+len(" (+99 more)") > width && len(parts) > 0 { - elided = len(series) - i - break + if plain+cost > width && len(parts) > 0 { + flush() + cost = 1 + len([]rune(text)) } - parts = append(parts, paintSegment(entry, s.label, group)) + parts = append(parts, mark+paintSegment(text, s.label, group)) plain += cost - // Past the glyph set every series shares the overflow marker, so naming - // them individually stops being informative. - if i+1 >= len(segmentGlyphs) { - elided = len(series) - i - 1 - break - } } - out := " " + strings.Join(parts, sep) + flush() + if elided > 0 { - out += fmt.Sprintf("%s(+%d more)", sep, elided) + note := fmt.Sprintf("%s(+%d more)", indent, elided) + // Append to the last line when it fits, so a single extra series does not + // cost a whole row. + if n := len(lines); n > 0 && len([]rune(stripANSIWidth(lines[n-1])))+len(note) <= width { + lines[n-1] += sep + strings.TrimPrefix(note, indent) + } else { + lines = append(lines, note) + } } - return out + if len(lines) == 0 { + return nil + } + return lines +} + +// stripANSIWidth returns text with escape sequences removed, for measuring how +// many columns a styled string occupies. +func stripANSIWidth(s string) string { + var b strings.Builder + for i := 0; i < len(s); { + if s[i] == 0x1b { + for i < len(s) && s[i] != 'm' { + i++ + } + i++ + continue + } + b.WriteByte(s[i]) + i++ + } + return b.String() } diff --git a/authbridge/cmd/abctl/tui/usage_stacked_test.go b/authbridge/cmd/abctl/tui/usage_stacked_test.go index 8d3180eb9..849c3a2d2 100644 --- a/authbridge/cmd/abctl/tui/usage_stacked_test.go +++ b/authbridge/cmd/abctl/tui/usage_stacked_test.go @@ -47,24 +47,78 @@ func stripANSI(s string) string { return b.String() } -// Each series must get its own texture, so the chart is readable with no colour +// Each series must get a distinct mark, so the chart is readable with no colour // at all — a terminal without colour support, a colour-vision deficiency, or a -// screenshot in an issue. -func TestRenderStacked_DistinctGlyphsPerSeries(t *testing.T) { +// screenshot in an issue. Shaded blocks failed this in practice: █ against ▓ is +// nearly indistinguishable in most terminal fonts. +func TestRenderStacked_DistinctMarksPerSeries(t *testing.T) { buckets := mkSeriesBuckets([]map[string]int64{ {"200": 10, "429": 5, "500": 2}, }) - lines := renderStackedBars(buckets, metricRequests, usage.GroupStatus, 80) - plot := stripANSI(strings.Join(lines, "\n")) + plot := stripANSI(strings.Join(renderStackedBars(buckets, metricRequests, usage.GroupStatus, 80), "\n")) - // Three series, so three distinct glyphs from the densest end. - for _, g := range []string{"█", "▓", "▒"} { - if !strings.Contains(plot, g) { - t.Errorf("expected glyph %q in the stack; got:\n%s", g, plot) + // Status labels have no letters, so their marks are the leading digits. + for _, want := range []string{"2", "4", "5"} { + if !strings.Contains(plot, strings.Repeat(want, barWidth)) { + t.Errorf("expected a %q band in the stack; got:\n%s", want, plot) } } } +// The mark must be derived from the label so a band is self-describing, and +// sibling models must not collide on a shared vendor prefix — every claude-* +// yielding "c" would defeat the point. +func TestSeriesLetter(t *testing.T) { + for _, tc := range []struct { + label string + want rune + }{ + {"claude-sonnet-5", 's'}, + {"claude-opus-5", 'o'}, + {"claude-haiku-4-5-20251001", 'h'}, + {"anthropic/claude-sonnet-5", 's'}, // provider prefix skipped too + {"gpt-4o", 'o'}, // gpt is a vendor token; 4 is not a letter + {"inference-parser", 'i'}, + {"tool-prune", 't'}, + {"denied", 'd'}, + {"200", '2'}, // no letters at all: fall back to the first character + {"429", '4'}, + {"(other)", 'o'}, + {"", '?'}, + } { + if got := seriesLetter(tc.label); got != tc.want { + t.Errorf("seriesLetter(%q) = %q, want %q", tc.label, string(got), string(tc.want)) + } + } +} + +// Two series sharing a mark is the failure the shaded blocks had. Uniqueness +// beats the mnemonic, and the largest series keeps the intuitive letter. +func TestAssignLetters_AreUnique(t *testing.T) { + series := []seriesKey{ + {"claude-opus-5", 100}, // wants 'o' + {"claude-sonnet-5", 90}, // wants 's' + {"(other)", 80}, // also wants 'o' — must yield + {"openai/gpt-4o", 70}, // 'o' taken as well + {"ollama-llama3", 60}, + } + letters := assignLetters(series) + if len(letters) != len(series) { + t.Fatalf("assigned %d marks for %d series", len(letters), len(series)) + } + seen := map[rune]string{} + for label, r := range letters { + if prev, dup := seen[r]; dup { + t.Errorf("mark %q assigned to both %q and %q", string(r), prev, label) + } + seen[r] = label + } + // The largest series keeps its derived letter. + if letters["claude-opus-5"] != 'o' { + t.Errorf("largest series lost its mnemonic: got %q", string(letters["claude-opus-5"])) + } +} + // Which series render as errors. Asserted on the DECISION, not on ANSI bytes: // lipgloss strips colour when it detects no TTY, which is always the case under // `go test`, so a byte-level assertion would pass vacuously and keep passing if @@ -176,9 +230,9 @@ func TestRenderStacked_OverflowSeriesAreMarked(t *testing.T) { labels[string(rune('a'+i))] = int64(12 - i) } lines := renderStackedBars(mkSeriesBuckets([]map[string]int64{labels}), metricRequests, usage.GroupMethod, 80) - legend := stripANSI(lines[len(lines)-1]) - if !strings.Contains(legend, "more)") { - t.Errorf("legend does not report elided series: %q", legend) + joined := stripANSI(strings.Join(lines, "\n")) + if !strings.Contains(joined, "more)") { + t.Errorf("legend does not report series past the palette: %q", joined) } } @@ -190,7 +244,92 @@ func TestRenderStacked_SmallBucketStillDraws(t *testing.T) { }) lines := renderStackedBars(buckets, metricRequests, usage.GroupStatus, 80) bottom := stripANSI(lines[plotRows-1]) // last plot row - if strings.Count(bottom, "█") < barWidth*2 { + if strings.Count(bottom, "2") < barWidth*2 { t.Errorf("the tiny bucket drew no segment:\n%q", bottom) } } + +// A series that is a rounding error of the bucket must still occupy a row. +// "claude-haiku at 912 tokens against 2.2M" is exactly the case an operator wants +// to spot, and a segment floored to zero rows is indistinguishable from absent. +// +// The per-series floor alone was not enough: with three series in a ten-row bar +// the largest took 9 rows and the two floored ones landed on rows 10 and 11, so +// the eleventh fell outside the bar and vanished anyway. +func TestRenderStacked_TinySeriesIsStillVisible(t *testing.T) { + buckets := mkSeriesBuckets([]map[string]int64{{ + "claude-sonnet-5": 22000, // 2.2M tokens at 100x + "claude-opus-5": 1050, + "claude-haiku-4-5-20251001": 9, // 0.04% of the bucket + }}) + plot := stripANSI(strings.Join(renderStackedBars(buckets, metricTokens, usage.GroupMethod, 80), "\n")) + + for _, want := range []string{"s", "o", "h"} { + if !strings.Contains(plot, strings.Repeat(want, barWidth)) { + t.Errorf("series %q drew no band despite being present:\n%s", want, plot) + } + } +} + +// Allotment must fill the bar exactly: overshooting pushes top segments outside +// the frame, undershooting leaves a gap at the top. +func TestAllotRows_SumsToBarHeight(t *testing.T) { + for _, tc := range []struct { + name string + values map[string]int64 + barRows int64 + }{ + {"tiny tail", map[string]int64{"a": 22000, "b": 1050, "c": 9}, 10}, + {"even split", map[string]int64{"a": 10, "b": 10, "c": 10}, 9}, + {"more series than rows", map[string]int64{"a": 5, "b": 4, "c": 3, "d": 2, "e": 1}, 3}, + {"single series", map[string]int64{"a": 7}, 10}, + {"one row", map[string]int64{"a": 7, "b": 3}, 1}, + } { + b := mkSeriesBuckets([]map[string]int64{tc.values})[0] + series := collectSeries([]usage.Bucket{b}, metricRequests) + got := allotRows(b, metricRequests, series, tc.barRows, b.Requests) + + var sum int64 + for _, a := range got { + if a.rows < 1 { + t.Errorf("%s: series %q allotted %d rows", tc.name, a.label, a.rows) + } + sum += a.rows + } + if sum != tc.barRows { + t.Errorf("%s: allotted %d rows, bar is %d tall", tc.name, sum, tc.barRows) + } + } +} + +// The legend is the only key to a band, so a present series must never be elided +// for width — it wraps instead. Model names are long enough that three do not fit +// 80 columns on one line. +func TestRenderLegend_WrapsRatherThanElidingPresentSeries(t *testing.T) { + series := []seriesKey{ + {"claude-sonnet-5", 4_600_000}, + {"claude-opus-5", 218_000}, + {"claude-haiku-4-5-20251001", 1900}, + } + letters := assignLetters(series) + rank := map[string]int{} + for i, s := range series { + rank[s.label] = i + } + + lines := renderLegend(series, usage.GroupMethod, letters, rank, 80) + joined := stripANSI(strings.Join(lines, "\n")) + for _, s := range series { + if !strings.Contains(joined, s.label) { + t.Errorf("legend dropped %q:\n%s", s.label, joined) + } + } + if strings.Contains(joined, "more)") { + t.Errorf("legend elided a named series instead of wrapping:\n%s", joined) + } + for _, l := range lines { + if got := len([]rune(stripANSI(l))); got > 80 { + t.Errorf("legend line is %d columns:\n%q", got, stripANSI(l)) + } + } +} From 2a4857ddee52c248f985469755d8b0e6fba11d99 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 6 Sep 2026 11:16:21 -0400 Subject: [PATCH 3/8] fix: Rebind usage-pane keys to first letters, drop the refresh key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three usability points, all fair. `t` for "metric" named one of the four values (tokens) rather than the axis, while every other binding in the pane is the first letter of what it changes. It is now `m`, which was free. `g` for "group" shadowed the global `g` (go to top) — worse than merely inconsistent, since it broke a vim-style motion inside one pane. It is now `b` for breakdown. Changing the breakdown while viewing latency did nothing, because the aggregator holds no per-label latency: there is no per-status or per-model mean to plot, only the bucket-wide one. Rather than accept a keypress with no visible effect, `b` is not handled under latency and the footer omits it there — a key advertised as available but inert reads as a broken binding. `r` to refresh had no rationale. The pane polls every 20s on its own, and since the catalog-return fix even resuming the chain is automatic, so a manual refresh bought nothing but a line of footer. Removed. Tests now assert that every key the footer advertises is one the pane handles, that the retired keys are gone from both the footer and the [?] overlay, and that the pane does not claim `g`. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/help_overlay.go | 5 +- authbridge/cmd/abctl/tui/help_overlay_test.go | 47 ++++++++++++++++++- authbridge/cmd/abctl/tui/keys.go | 34 +++++++++++--- 3 files changed, 75 insertions(+), 11 deletions(-) diff --git a/authbridge/cmd/abctl/tui/help_overlay.go b/authbridge/cmd/abctl/tui/help_overlay.go index 33d54a2cf..0c09d2677 100644 --- a/authbridge/cmd/abctl/tui/help_overlay.go +++ b/authbridge/cmd/abctl/tui/help_overlay.go @@ -113,11 +113,10 @@ var paneKeys = map[paneID]keyGroup{ paneUsage: { title: "USAGE (this pane)", bindings: []keyBinding{ - {"t", "cycle metric (tokens/requests/errors/latency)"}, + {"m", "cycle metric (tokens/requests/errors/latency)"}, {"w", "cycle window (10m/1h/6h)"}, - {"g", "cycle grouping (none/status/method/plugin)"}, + {"b", "cycle breakdown (none/status/method/plugin)"}, {"s", "toggle session / all sessions"}, - {"r", "refresh now"}, {"esc", "back"}, }, }, diff --git a/authbridge/cmd/abctl/tui/help_overlay_test.go b/authbridge/cmd/abctl/tui/help_overlay_test.go index 6a140b2de..c355192c4 100644 --- a/authbridge/cmd/abctl/tui/help_overlay_test.go +++ b/authbridge/cmd/abctl/tui/help_overlay_test.go @@ -273,11 +273,56 @@ func TestFooterHintsMentionUsageKey(t *testing.T) { // The usage pane's own footer must not fall through to the bare default. m.pane = paneUsage got := m.helpView() - for _, want := range []string{"[t] metric", "[w] window", "[r] refresh"} { + for _, want := range []string{"[m] metric", "[w] window", "[b] breakdown"} { if !strings.Contains(got, want) { t.Errorf("usage footer omits %q:\n %s", want, got) } } + + // Latency has no per-label breakdown, so the footer must not advertise [b] + // there — a key shown as available but inert reads as a broken binding. + m.usage.metric = metricLatency + got = m.helpView() + if strings.Contains(got, "[b]") { + t.Errorf("latency footer advertises the inert breakdown key:\n %s", got) + } + if !strings.Contains(got, "[m] metric") { + t.Errorf("latency footer lost the metric key:\n %s", got) + } +} + +// Every key the usage footer advertises must be one the pane actually handles, +// and every key it handles should be advertised. A binding nobody can discover +// and a hint that does nothing are the same class of bug. +func TestUsageFooterMatchesHandledKeys(t *testing.T) { + m := &model{pane: paneUsage, selectedSess: "s1"} + + // Keys the pane handles, per the paneUsage switch in handleKey. + handled := []string{"m", "w", "b", "s"} + footer := m.helpView() + for _, k := range handled { + if !strings.Contains(footer, "["+k+"]") { + t.Errorf("footer omits handled key %q:\n %s", k, footer) + } + } + // And the retired ones must be gone from both surfaces. + for _, k := range []string{"[t]", "[g]", "[r]"} { + if strings.Contains(footer, k) { + t.Errorf("footer still advertises retired key %s:\n %s", k, footer) + } + } + for _, kb := range paneKeys[paneUsage].bindings { + if kb.keys == "t" || kb.keys == "g" || kb.keys == "r" { + t.Errorf("help overlay still lists retired key %q", kb.keys) + } + } +} + +// `g` is globally "go to top". The usage pane must not shadow it. +func TestUsagePane_DoesNotShadowGlobalG(t *testing.T) { + if strings.Contains((&model{pane: paneUsage}).helpView(), "[g]") { + t.Error("usage footer claims [g], which is the global go-to-top motion") + } } // The overlay must stay inside the terminal, and must never push the diff --git a/authbridge/cmd/abctl/tui/keys.go b/authbridge/cmd/abctl/tui/keys.go index e6c205aea..95fa39517 100644 --- a/authbridge/cmd/abctl/tui/keys.go +++ b/authbridge/cmd/abctl/tui/keys.go @@ -71,20 +71,32 @@ func (m *model) handleKey(msg tea.KeyMsg) tea.Cmd { // the shared handling above already routed. if m.pane == paneUsage && !m.filtering { switch msg.String() { - case "t": + case "m": + // Metric. `t` (for "tokens") named one of the four values rather than + // the axis, and every other binding here is the first letter of what it + // changes. m.usage.cycleMetric() return nil case "w": m.usage.cycleWindow() return m.beginFetch() - case "g": - // Refetch: the grouping is a server-side query parameter, not a + case "b": + // Breakdown. NOT `g` for "group": `g` is globally "go to top" (see + // goTop below), and shadowing a vim-style motion inside one pane is + // worse than picking a second-choice mnemonic. + // + // Inert while viewing latency — the aggregator holds no per-label + // latency — so the key is not accepted there at all. Silently + // swallowing it would look like a broken binding; falling through lets + // `b` keep whatever meaning it has elsewhere. + if m.usage.metric.isLatency() { + break + } + // Refetch: the breakdown is a server-side query parameter, not a // client-side filter, so the current snapshot has no series for the // newly selected dimension. m.usage.cycleGroup() return m.beginFetch() - case "r": - return m.beginFetch() case "s": // Toggle scope between this session and all sessions. Only offered // when a session is selected; otherwise there is nothing to toggle to. @@ -618,8 +630,16 @@ func (m *model) helpView() string { } else if m.selectedSess != "" { scopeHint = " [s] this session" } - return "[t] metric [w] window [g] group" + scopeHint + - " [r] refresh [esc] back [?] keys [q] quit" + // [b] is omitted under latency rather than shown as a no-op: a footer that + // advertises an inert key is worse than a shorter footer. + breakdownHint := " [b] breakdown" + if m.usage.metric.isLatency() { + breakdownHint = "" + } + // No [r]: the pane polls every 20s on its own, so a manual refresh key + // bought nothing but a line of footer. + return "[m] metric [w] window" + breakdownHint + scopeHint + + " [esc] back [?] keys [q] quit" case paneCatalog: if m.catalog == nil { return "loading catalog… [esc] back [?] keys [q] quit" From b53267e409942433c9a04e9bfcf3f2be1549ecf4 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 6 Sep 2026 11:37:37 -0400 Subject: [PATCH 4/8] =?UTF-8?q?fix:=20Address=20PR=20review=20=E2=80=94=20?= =?UTF-8?q?duration=20rounding,=20legend=20width,=20latency=20header?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five CodeRabbit findings, all verified against the code before changing anything. humanizeDurationMs truncated instead of rounding, so 9.99ms rendered as "9ms" — rounding DOWN past a whole millisecond, which is a wrong number rather than an imprecise one. I introduced this when bounding the branch at 9.95 to stop "10.0ms" overflowing the label width; the fix was the wrong half of the problem. Every integer branch now rounds, and each branch bound is the value that would round out of it, so 999.6ms is "1.0s" rather than a six-character "1000ms". Tested by sweeping magnitudes rather than listing points, since the failures live exactly at branch boundaries and a hand-written list is what misses them. The legend's width bound only applied when something was already on the line, so a single long model name on a narrow terminal emitted 66 columns at width 10. A lone over-wide entry now truncates its NAME, keeping the mark and the total: those identify the band and say what it is worth, and a name is recognisable from a prefix while a truncated number is simply wrong. The elision note is appended as sep+note but the fit check measured the indented form, so it was three columns short — which happened to agree at width 80 and nowhere else. It now measures what it appends, checked across every width from 30 to 100. The header read "by status" while viewing latency, asserting a breakdown the chart does not show: the aggregator holds no per-label latency, so renderUsageChart ignores the group entirely. It now says "no breakdown for latency" and keeps the selection, so cycling back to a count metric restores it. The [?] overlay says the same of the b key, matching the footer, which already omits it there. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/help_overlay.go | 2 +- authbridge/cmd/abctl/tui/usage_pane.go | 10 ++- authbridge/cmd/abctl/tui/usage_stacked.go | 50 ++++++++++++-- .../cmd/abctl/tui/usage_stacked_test.go | 67 +++++++++++++++++++ authbridge/cmd/abctl/tui/usage_state_test.go | 37 ++++++++++ authbridge/cmd/abctl/tui/usage_whiskers.go | 21 +++--- .../cmd/abctl/tui/usage_whiskers_test.go | 41 ++++++++++-- 7 files changed, 206 insertions(+), 22 deletions(-) diff --git a/authbridge/cmd/abctl/tui/help_overlay.go b/authbridge/cmd/abctl/tui/help_overlay.go index 0c09d2677..842655956 100644 --- a/authbridge/cmd/abctl/tui/help_overlay.go +++ b/authbridge/cmd/abctl/tui/help_overlay.go @@ -115,7 +115,7 @@ var paneKeys = map[paneID]keyGroup{ bindings: []keyBinding{ {"m", "cycle metric (tokens/requests/errors/latency)"}, {"w", "cycle window (10m/1h/6h)"}, - {"b", "cycle breakdown (none/status/method/plugin)"}, + {"b", "cycle breakdown (none/status/method/plugin; not for latency)"}, {"s", "toggle session / all sessions"}, {"esc", "back"}, }, diff --git a/authbridge/cmd/abctl/tui/usage_pane.go b/authbridge/cmd/abctl/tui/usage_pane.go index 8ba9f70ab..a4c780588 100644 --- a/authbridge/cmd/abctl/tui/usage_pane.go +++ b/authbridge/cmd/abctl/tui/usage_pane.go @@ -211,8 +211,16 @@ func (m *model) renderUsage(width, height int) string { scope = "session: " + m.usage.session } window, resolution := m.usage.window() + // The header must not claim a breakdown the chart is not showing. Latency has + // no per-label data in the aggregator, so renderUsageChart ignores the group + // entirely — displaying "by status" over a bucket-wide mean would assert a + // breakdown that does not exist. The selection is kept, not cleared, so it is + // still there when the operator cycles back to a count metric. grouping := "ungrouped" - if m.usage.group != "" && m.usage.group != usage.GroupNone { + switch { + case m.usage.metric.isLatency(): + grouping = "no breakdown for latency" + case m.usage.group != "" && m.usage.group != usage.GroupNone: grouping = "by " + string(m.usage.group) } b.WriteString(fmt.Sprintf(" USAGE — %s — %s @ %s — %s — %s\n\n", diff --git a/authbridge/cmd/abctl/tui/usage_stacked.go b/authbridge/cmd/abctl/tui/usage_stacked.go index 9a45bd339..e2a8e9e23 100644 --- a/authbridge/cmd/abctl/tui/usage_stacked.go +++ b/authbridge/cmd/abctl/tui/usage_stacked.go @@ -302,19 +302,30 @@ func renderLegend(series []seriesKey, group usage.Group, flush() cost = 1 + len([]rune(text)) } + // A single entry can still exceed the width with nothing to wrap against — + // one long model name on a narrow terminal. Truncate the NAME rather than + // the whole entry, so the mark and the total survive: those are what + // identify the band and say how much it is, and a legend line wider than + // the terminal wraps and destroys the chart above it. + if plain+cost > width { + text = truncateLegendText(text, width-plain-1) + cost = 1 + len([]rune(text)) + } parts = append(parts, mark+paintSegment(text, s.label, group)) plain += cost } flush() if elided > 0 { - note := fmt.Sprintf("%s(+%d more)", indent, elided) - // Append to the last line when it fits, so a single extra series does not - // cost a whole row. - if n := len(lines); n > 0 && len([]rune(stripANSIWidth(lines[n-1])))+len(note) <= width { - lines[n-1] += sep + strings.TrimPrefix(note, indent) + bare := fmt.Sprintf("(+%d more)", elided) + // Measure what is actually appended — sep + bare — not the indented form. + // Counting the indent instead of the separator happened to agree at width + // 80 and was three columns short everywhere else. + suffix := len([]rune(sep)) + len([]rune(bare)) + if n := len(lines); n > 0 && len([]rune(stripANSIWidth(lines[n-1])))+suffix <= width { + lines[n-1] += sep + bare } else { - lines = append(lines, note) + lines = append(lines, indent+bare) } } if len(lines) == 0 { @@ -323,6 +334,33 @@ func renderLegend(series []seriesKey, group usage.Group, return lines } +// truncateLegendText shortens a legend entry's text to fit, preserving the +// trailing "(total)" so the entry still says how much the band is worth. +// +// The name is what gets cut, with an ellipsis marking it, because a name is +// recognisable from a prefix while a truncated number is simply wrong. +func truncateLegendText(text string, max int) string { + r := []rune(text) + if max <= 0 { + return "" + } + if len(r) <= max { + return text + } + // Keep the parenthesised total if there is room for it plus a token name. + if i := strings.LastIndex(text, " ("); i > 0 { + total := text[i:] + tr := []rune(total) + if keep := max - len(tr) - 1; keep > 1 { + return string(r[:keep]) + "…" + total + } + } + if max == 1 { + return "…" + } + return string(r[:max-1]) + "…" +} + // stripANSIWidth returns text with escape sequences removed, for measuring how // many columns a styled string occupies. func stripANSIWidth(s string) string { diff --git a/authbridge/cmd/abctl/tui/usage_stacked_test.go b/authbridge/cmd/abctl/tui/usage_stacked_test.go index 849c3a2d2..d684c80f7 100644 --- a/authbridge/cmd/abctl/tui/usage_stacked_test.go +++ b/authbridge/cmd/abctl/tui/usage_stacked_test.go @@ -1,6 +1,7 @@ package tui import ( + "fmt" "strings" "testing" "time" @@ -333,3 +334,69 @@ func TestRenderLegend_WrapsRatherThanElidingPresentSeries(t *testing.T) { } } } + +// A legend line wider than the terminal wraps and destroys the chart above it, so +// the width bound has to hold even for a single entry with nothing to wrap +// against — one long model name on a narrow terminal. +func TestRenderLegend_BoundsALoneOverWideEntry(t *testing.T) { + series := []seriesKey{{"anthropic/claude-haiku-4-5-20251001-preview-experimental", 912}} + letters := assignLetters(series) + rank := map[string]int{series[0].label: 0} + + for _, width := range []int{10, 16, 20, 40, 80} { + lines := renderLegend(series, usage.GroupMethod, letters, rank, width) + for _, l := range lines { + if got := len([]rune(stripANSI(l))); got > width { + t.Errorf("width %d: legend line is %d columns:\n%q", width, got, stripANSI(l)) + } + } + // The total identifies how much the band is worth, so it survives + // truncation wherever there is room for it at all. + if width >= 20 { + if !strings.Contains(stripANSI(strings.Join(lines, "")), "(912)") { + t.Errorf("width %d: truncation dropped the total: %q", width, stripANSI(lines[0])) + } + } + } +} + +// The elision note is appended as sep+note, so the fit check must measure that. +// Counting the indent instead happened to agree at width 80 and was three columns +// short everywhere else. +func TestRenderLegend_ElisionNoteRespectsWidth(t *testing.T) { + var series []seriesKey + for i := 0; i < 9; i++ { + series = append(series, seriesKey{fmt.Sprintf("series-%c-name", 'a'+i), int64(100 - i)}) + } + letters := assignLetters(series) + rank := map[string]int{} + for i, s := range series { + rank[s.label] = i + } + + for width := 30; width <= 100; width++ { + lines := renderLegend(series, usage.GroupMethod, letters, rank, width) + joined := stripANSI(strings.Join(lines, "\n")) + if !strings.Contains(joined, "more)") { + t.Errorf("width %d: nine series past the palette but no elision note", width) + } + for _, l := range lines { + if got := len([]rune(stripANSI(l))); got > width { + t.Errorf("width %d: line is %d columns:\n%q", width, got, stripANSI(l)) + } + } + } +} + +func TestTruncateLegendText(t *testing.T) { + const text = " claude-sonnet-5 (2.2M)" + for _, max := range []int{0, 1, 2, 5, 10, 22, 23, 40} { + got := truncateLegendText(text, max) + if len([]rune(got)) > max { + t.Errorf("truncateLegendText(%d) = %q (%d runes), over budget", max, got, len([]rune(got))) + } + } + if got := truncateLegendText(text, 40); got != text { + t.Errorf("a text that already fits was altered: %q", got) + } +} diff --git a/authbridge/cmd/abctl/tui/usage_state_test.go b/authbridge/cmd/abctl/tui/usage_state_test.go index 2a5b1ebe6..4a162b774 100644 --- a/authbridge/cmd/abctl/tui/usage_state_test.go +++ b/authbridge/cmd/abctl/tui/usage_state_test.go @@ -268,3 +268,40 @@ func TestUsage_CatalogRoundTripRestartsPolling(t *testing.T) { t.Error("the pre-catalog chain is still alive — two chains would double the poll rate") } } + +// The header must not claim a breakdown the chart is not showing. Latency has no +// per-label data, so renderUsageChart ignores the group — a header reading "by +// status" over a bucket-wide mean asserts a breakdown that does not exist. +func TestRenderUsage_HeaderDoesNotClaimLatencyBreakdown(t *testing.T) { + m := &model{pane: paneUsage, width: 80, bodyHeight: 24} + m.usage.group = usage.GroupStatus + m.usage.snap = &usage.Snapshot{Buckets: []usage.Bucket{{ + At: time.Date(2026, 9, 6, 23, 24, 0, 0, time.UTC), + }}} + + m.usage.metric = metricTokens + if got := m.renderUsage(80, 24); !strings.Contains(got, "by status") { + t.Error("count metric header omits the active breakdown") + } + + m.usage.metric = metricLatency + got := m.renderUsage(80, 24) + if strings.Contains(got, "by status") { + t.Errorf("latency header claims a breakdown the chart ignores:\n%s", firstLine(got)) + } + if !strings.Contains(got, "no breakdown for latency") { + t.Errorf("latency header does not say why there is no breakdown:\n%s", firstLine(got)) + } + + // The selection survives, so cycling back to a count metric restores it. + if m.usage.group != usage.GroupStatus { + t.Errorf("group = %q, want the selection preserved", m.usage.group) + } +} + +func firstLine(s string) string { + if i := strings.IndexByte(s, '\n'); i >= 0 { + return s[:i] + } + return s +} diff --git a/authbridge/cmd/abctl/tui/usage_whiskers.go b/authbridge/cmd/abctl/tui/usage_whiskers.go index 6cfecdc20..4f89a4c22 100644 --- a/authbridge/cmd/abctl/tui/usage_whiskers.go +++ b/authbridge/cmd/abctl/tui/usage_whiskers.go @@ -215,18 +215,21 @@ func humanizeDurationMs(ms float64) string { case ms < 1: return "<1ms" case ms < 9.95: - // Bounded below 9.95, not 10: %.1f rounds 9.99 up to "10.0ms", which is - // six characters and breaks the width promise the gutter is laid out - // against. Same trap one branch down. + // Bounded below 9.95, not 10: %.1f rounds 9.99 up to "10.0ms", which is six + // characters and breaks the width promise the gutter is laid out against. return fmt.Sprintf("%.1fms", ms) // 1.0ms..9.9ms - case ms < 1000: - return fmt.Sprintf("%dms", int64(ms)) // 10ms..999ms + case ms < 999.5: + // Rounded, not truncated. int64(9.99) is 9, so the previous version + // reported 9.99ms as "9ms" — a value rounding DOWN past a whole + // millisecond, which is a worse error than the wide label the 9.95 bound + // above exists to avoid. Same reasoning in each integer branch below. + return fmt.Sprintf("%dms", int64(ms+0.5)) // 10ms..999ms case ms < 9_950: return fmt.Sprintf("%.1fs", ms/1000) // 1.0s..9.9s - case ms < 600_000: - return fmt.Sprintf("%ds", int64(ms)/1000) // 10s..599s - case ms < 36_000_000: - return fmt.Sprintf("%dm", int64(ms)/60_000) // 10m..599m + case ms < 599_500: + return fmt.Sprintf("%ds", int64(ms/1000+0.5)) // 10s..599s + case ms < 35_970_000: + return fmt.Sprintf("%dm", int64(ms/60_000+0.5)) // 10m..599m default: return ">10h" } diff --git a/authbridge/cmd/abctl/tui/usage_whiskers_test.go b/authbridge/cmd/abctl/tui/usage_whiskers_test.go index d92733525..342e12728 100644 --- a/authbridge/cmd/abctl/tui/usage_whiskers_test.go +++ b/authbridge/cmd/abctl/tui/usage_whiskers_test.go @@ -1,6 +1,8 @@ package tui import ( + "fmt" + "math" "strings" "testing" "time" @@ -154,6 +156,13 @@ func TestHumanizeDurationMs(t *testing.T) { {10_000, "10s"}, {599_000, "599s"}, {600_000, "10m"}, {36_000_000, ">10h"}, + // Boundaries where a rounded label would leave its own branch. Truncating + // here reported 9.99ms as "9ms" — rounding DOWN past a whole millisecond, + // which is a worse error than the wide label the branch bound prevents. + {9.95, "10ms"}, {9.99, "10ms"}, + {999.4, "999ms"}, {999.6, "1.0s"}, + {9949, "9.9s"}, {9950, "10s"}, {9999, "10s"}, + {599_400, "599s"}, {599_600, "10m"}, } { if got := humanizeDurationMs(tc.in); got != tc.want { t.Errorf("humanizeDurationMs(%v) = %q, want %q", tc.in, got, tc.want) @@ -164,17 +173,39 @@ func TestHumanizeDurationMs(t *testing.T) { // The width promise is what the gutter is laid out against, so it must hold for // every magnitude — the same lesson as humanizeCount, which returned 7 characters // above a billion because nobody revisited its fallthrough. +// +// Swept densely rather than at listed points: the failures here live at branch +// boundaries, where a rounded value crosses out of the branch that formatted it, +// and a hand-written list is exactly what misses those. func TestHumanizeDurationMs_NeverExceedsWidth(t *testing.T) { - vals := []float64{0, -1, 0.001, 0.999} - for _, base := range []float64{1, 4.44, 9.99} { - for mag := 1.0; mag <= 1e15; mag *= 10 { - vals = append(vals, base*mag) + for e := -3.0; e < 10; e += 0.001 { + v := math.Pow(10, e) + if got := humanizeDurationMs(v); len([]rune(got)) > maxDurationLabelLen { + t.Fatalf("humanizeDurationMs(%v) = %q (%d chars), cap is %d", + v, got, len([]rune(got)), maxDurationLabelLen) } } - for _, v := range vals { + for _, v := range []float64{0, -1, 0.001, 0.999, math.MaxFloat64} { if got := humanizeDurationMs(v); len([]rune(got)) > maxDurationLabelLen { t.Errorf("humanizeDurationMs(%v) = %q (%d chars), cap is %d", v, got, len([]rune(got)), maxDurationLabelLen) } } } + +// No label may understate its value: reporting 9.99ms as "9ms" is a wrong number, +// not merely a rounded one. Checks that the rendered label never falls below the +// value it describes by more than the precision it shows. +func TestHumanizeDurationMs_NeverRoundsDownAcrossAUnit(t *testing.T) { + for _, v := range []float64{9.95, 9.99, 999.6, 9999, 599_600} { + got := humanizeDurationMs(v) + // A label ending in a bare unit must not show fewer whole units than the + // value has, once the unit is accounted for. + if strings.HasSuffix(got, "ms") && !strings.Contains(got, ".") { + var n float64 + if _, err := fmt.Sscanf(got, "%fms", &n); err == nil && n < v-1 { + t.Errorf("humanizeDurationMs(%v) = %q understates by more than 1ms", v, got) + } + } + } +} From 6c21b0fee3ba7668c3b4e77318c1eb604bd327f5 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 6 Sep 2026 12:02:18 -0400 Subject: [PATCH 5/8] fix: Correct stacked-bar allotment, fold unnamed series, drop stale comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five review findings, all verified against the code first. allotRows took each series' share against the BUCKET total, which is not the sum of what it draws — and is wrong in both directions. Under: a bucket can carry traffic no label claims, so the labelled series may sum to a fraction of the total. Every series was then under-allotted and the leftover rows all went to the largest, drawing a bucket that is 10% claude-sonnet-5 as a solid `s` bar — the height said "lots of traffic" and every row of it claimed to be sonnet. Shares are now taken against the sum of what is drawn, and the unclaimed remainder gets its own "(unlabelled)" band, so a bar's height and its segments agree. Over: per-plugin attribution counts one request once per plugin that ran, so byPlugin sub-totals intentionally sum to more than the bucket (the aggregator says so in foldInto). Rows were over-allotted, `acc` ran past the bar height, and whole series fell off the top of the chart — on every by-plugin bucket. Normalising against the drawn sum fixes both cases by construction. Series past maxNamedSeries were drawn with marks but named in no legend entry. Marks come from the palette and repeat once it wraps, so a seventh series could draw with the first one's mark while the legend named neither. They now fold into one "(other)" band before drawing, reusing the aggregator's own overflow name, and an existing "(other)" merges rather than producing two bands that both mean "the rest". Two comment corrections: a duplicated doc comment on renderLegend left from an earlier rewrite, and a fallthrough rationale for `b` under latency that did not hold — pageActivePane has no paneUsage case, so the key is simply dropped rather than reaching another handler. The previous tests passed through both allotment bugs. They now assert the allotment sums to the bar height for over- and under-attributed series, that unclaimed traffic draws its own band, and that every mark on the chart appears in the legend. Verified by reverting the fix: the over-attribution test reports 24 rows allotted for a 10-row bar. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/keys.go | 10 +- authbridge/cmd/abctl/tui/usage_glyphs.go | 8 + authbridge/cmd/abctl/tui/usage_stacked.go | 168 ++++++++++++++++-- .../cmd/abctl/tui/usage_stacked_test.go | 136 ++++++++++++++ 4 files changed, 302 insertions(+), 20 deletions(-) diff --git a/authbridge/cmd/abctl/tui/keys.go b/authbridge/cmd/abctl/tui/keys.go index 95fa39517..518d007e2 100644 --- a/authbridge/cmd/abctl/tui/keys.go +++ b/authbridge/cmd/abctl/tui/keys.go @@ -85,10 +85,12 @@ func (m *model) handleKey(msg tea.KeyMsg) tea.Cmd { // goTop below), and shadowing a vim-style motion inside one pane is // worse than picking a second-choice mnemonic. // - // Inert while viewing latency — the aggregator holds no per-label - // latency — so the key is not accepted there at all. Silently - // swallowing it would look like a broken binding; falling through lets - // `b` keep whatever meaning it has elsewhere. + // Ignored while viewing latency: the aggregator holds no per-label + // latency, so there is no per-status or per-model mean to plot. The + // footer and the [?] overlay both say so, which is what keeps this from + // reading as a broken binding — `b` reaches no other handler for this + // pane (pageActivePane has no paneUsage case), so breaking here simply + // drops it. if m.usage.metric.isLatency() { break } diff --git a/authbridge/cmd/abctl/tui/usage_glyphs.go b/authbridge/cmd/abctl/tui/usage_glyphs.go index 52b009f46..89e30ed63 100644 --- a/authbridge/cmd/abctl/tui/usage_glyphs.go +++ b/authbridge/cmd/abctl/tui/usage_glyphs.go @@ -51,7 +51,15 @@ var vendorPrefixes = map[string]bool{ // letter over a digit so "claude-sonnet-5" is `s` and not `5`; fall back to the // first usable character; and use '?' when nothing qualifies. Callers dedupe the // result — see assignLetters. +// unlabelledMark is the band for traffic no series claims. A middle dot rather +// than a letter: there is no name to abbreviate, and it should not read as one +// more model. +const unlabelledMark = '·' + func seriesLetter(label string) rune { + if label == unlabelledLabel { + return unlabelledMark + } tokens := strings.FieldsFunc(label, func(r rune) bool { return r == '-' || r == '_' || r == '/' || r == '.' }) diff --git a/authbridge/cmd/abctl/tui/usage_stacked.go b/authbridge/cmd/abctl/tui/usage_stacked.go index e2a8e9e23..92da23256 100644 --- a/authbridge/cmd/abctl/tui/usage_stacked.go +++ b/authbridge/cmd/abctl/tui/usage_stacked.go @@ -8,6 +8,11 @@ import ( "github.com/rossoctl/cortex/authbridge/authlib/usage" ) +// unlabelledLabel names the share of a bucket that no series claims. It reaches +// the chart as its own band and the legend as its own entry, so a bar whose +// height exceeds what its labels account for says so. +const unlabelledLabel = "(unlabelled)" + // maxNamedSeries is how many series get their own mark and legend entry before // the rest fold together. Bounded by the palette so no two named series share a // colour. @@ -20,6 +25,93 @@ type seriesKey struct { total int64 } +// tailLabel collects series past maxNamedSeries. Matches the aggregator's own +// overflow name (usage.overflowLabel) so an operator sees one vocabulary whether +// the folding happened server-side, from label-cardinality capping, or here for +// palette reasons. +const tailLabel = "(other)" + +// foldTailSeries collapses everything past keep into a single tailLabel series, +// rewriting the buckets to match. +// +// Folding before drawing, rather than only in the legend, is what makes every +// band decodable: marks come from the palette and repeat once it wraps, so a +// seventh series could draw with the same mark as the first while the legend +// named neither. Returns the buckets unchanged when nothing needs folding, so the +// common case allocates nothing. +func foldTailSeries(buckets []usage.Bucket, m usageMetric, series []seriesKey, keep int) ([]seriesKey, []usage.Bucket) { + if len(series) <= keep { + return series, buckets + } + tail := make(map[string]bool, len(series)-keep) + var tailTotal int64 + for _, s := range series[keep:] { + tail[s.label] = true + tailTotal += s.total + } + // Existing "(other)" from the aggregator's own capping merges in rather than + // colliding: two bands both meaning "the rest" would be indefensible. + kept := append([]seriesKey(nil), series[:keep]...) + for i := range kept { + if kept[i].label == tailLabel { + kept[i].total += tailTotal + tailTotal = 0 + } + } + if tailTotal > 0 { + kept = append(kept, seriesKey{label: tailLabel, total: tailTotal}) + } + + out := make([]usage.Bucket, len(buckets)) + for i, b := range buckets { + out[i] = b + if len(b.Series) == 0 { + continue + } + merged := make(map[string]usage.Counts, keep+1) + var acc usage.Counts + for label, c := range b.Series { + if tail[label] { + // Summed field-wise: usage.Counts.add is unexported, and every field + // must be carried or a folded band would under-report. + acc.Requests += c.Requests + acc.Errors += c.Errors + acc.Tokens += c.Tokens + acc.CostMicros += c.CostMicros + continue + } + merged[label] = c + } + if acc.Requests > 0 || acc.Tokens > 0 || acc.Errors > 0 { + cur := merged[tailLabel] + cur.Requests += acc.Requests + cur.Errors += acc.Errors + cur.Tokens += acc.Tokens + cur.CostMicros += acc.CostMicros + merged[tailLabel] = cur + } + out[i].Series = merged + } + return kept, out +} + +// unlabelledTotal is how much of the window no series claims, summed across +// buckets that actually show a remainder band. Returns 0 when the labels account +// for everything, or over-account for it (per-plugin attribution does). +func unlabelledTotal(buckets []usage.Bucket, m usageMetric, series []seriesKey) int64 { + var out int64 + for _, b := range buckets { + var sum int64 + for _, s := range series { + sum += m.valueOf(b.Series[s.label]) + } + if d := m.value(b) - sum; d > 0 { + out += d + } + } + return out +} + // collectSeries totals each label across every bucket and returns them largest // first, with ties broken by label so the legend and the stack order are stable // between renders. An unstable order would make the chart appear to reshuffle on @@ -89,15 +181,32 @@ func renderStackedBars(buckets []usage.Bucket, m usageMetric, group usage.Group, return renderBars(buckets, m, width) } + // Fold everything past maxNamedSeries into one band BEFORE drawing, so every + // band on the chart has a legend entry. Drawing each of them with its own mark + // while the legend named only the first few left bands nothing could decode — + // the marks repeat once the palette wraps, so two unrelated series could even + // share one. + series, buckets = foldTailSeries(buckets, m, series, maxNamedSeries) + + // The unlabelled remainder is drawn as a band but is not in `series`, so + // register it for marks, colour and the legend. Appended last so it ranks + // behind every named series and cannot take a palette slot from one. + legendSeries := series + if unlabelled := unlabelledTotal(buckets, m, series); unlabelled > 0 { + legendSeries = append(append([]seriesKey(nil), series...), + seriesKey{label: unlabelledLabel, total: unlabelled}) + } + // Rank each label once so segment order is identical in every bucket. A stack // whose layers reorder between adjacent bars is unreadable. - rank := make(map[string]int, len(series)) - for i, s := range series { + rank := make(map[string]int, len(legendSeries)) + for i, s := range legendSeries { rank[s.label] = i } + // One mark per series, assigned once for the whole chart so a letter means the // same thing in every bucket and in the legend. - letters := assignLetters(series) + letters := assignLetters(legendSeries) var peak int64 for _, b := range buckets { @@ -125,7 +234,7 @@ func renderStackedBars(buckets []usage.Bucket, m usageMetric, group usage.Group, out = append(out, renderTimeLabels(buckets)) out = append(out, renderValues(buckets, m)) out = append(out, "") - out = append(out, renderLegend(series, group, letters, rank, width)...) + out = append(out, renderLegend(legendSeries, group, letters, rank, width)...) return out } @@ -165,6 +274,7 @@ func stackedCell(b usage.Bucket, m usageMetric, group usage.Group, // rowAlloc is one series' share of a bar, in whole rows. type rowAlloc struct { label string + value int64 // this series' metric value in the bucket rows int64 } @@ -181,15 +291,30 @@ type rowAlloc struct { // and its series vanished anyway. Guaranteed rows are reserved FIRST and the // remainder shared out proportionally, so the total always fits. func allotRows(b usage.Bucket, m usageMetric, series []seriesKey, barRows, total int64) []rowAlloc { - present := make([]rowAlloc, 0, len(series)) + present := make([]rowAlloc, 0, len(series)+1) + var seriesSum int64 for _, s := range series { - if m.valueOf(b.Series[s.label]) > 0 { - present = append(present, rowAlloc{label: s.label}) + if v := m.valueOf(b.Series[s.label]); v > 0 { + present = append(present, rowAlloc{label: s.label, value: v}) + seriesSum += v } } - if len(present) == 0 { + if len(present) == 0 || seriesSum == 0 { return nil } + // Traffic no label claims gets its own band rather than being absorbed by the + // named series. The bar's height comes from the bucket total, so silently + // sharing the unlabelled remainder out drew a bucket that is 10% + // claude-sonnet-5 as a solid `s` bar — the height said "lots of traffic" and + // every row of it claimed to be sonnet. + // + // Only when the shortfall is large enough to occupy a row: rounding noise does + // not deserve a band, and a one-row remainder on every bar would be more + // misleading than omitting it. + if unlabelled := total - seriesSum; unlabelled > 0 && unlabelled*barRows/total > 0 { + present = append(present, rowAlloc{label: unlabelledLabel, value: unlabelled}) + seriesSum += unlabelled + } // More series than rows: the bar cannot show them all, so give a row each to // as many as fit, largest first (series is already sorted). The legend still // names the rest. @@ -201,13 +326,25 @@ func allotRows(b usage.Bucket, m usageMetric, series []seriesKey, barRows, total return out } - // One row reserved per series, the rest shared by proportion of the surplus. + // Proportions are taken against seriesSum, NOT the bucket total. The two are + // not the same number in either direction: + // + // - Under: a bucket can carry traffic no label claims, so the labelled + // series may sum to a fraction of the total. Dividing by the total then + // under-allots every series and the leftover rows all went to the largest, + // drawing a bucket that is 10% claude-sonnet-5 as a solid `s` bar. + // - Over: per-plugin attribution counts one request once per plugin that + // ran, so byPlugin sub-totals intentionally sum to MORE than the bucket + // (see the aggregator's foldInto). Dividing by the total then over-allotted + // rows, `acc` ran past barRows, and whole series fell off the top of the + // chart — by-plugin did this on every bucket. + // + // Normalising against the sum of what is actually drawn makes the shares add + // up by construction, whichever way the totals disagree. surplus := barRows - int64(len(present)) var assigned int64 for i := range present { - v := m.valueOf(b.Series[present[i].label]) - extra := v * surplus / total - present[i].rows = 1 + extra + present[i].rows = 1 + present[i].value*surplus/seriesSum assigned += present[i].rows } // Integer division leaves rows unassigned; give them to the largest series so @@ -253,10 +390,9 @@ func paintSegment(text, label string, group usage.Group) string { return text } -// renderLegend names each series with its glyph and total. Error statuses are -// coloured to match their segments, so the legend is the key to the chart rather -// than a separate vocabulary. -// renderLegend keys the chart: each series' mark, name and total. +// renderLegend keys the chart: each series' mark, name and total. Error statuses +// are coloured to match their segments, so the legend is the key to the chart +// rather than a separate vocabulary. // // Returns one or more lines. Wrapping rather than eliding matters because the mark // is the only way to identify a band — a series dropped from the legend leaves an diff --git a/authbridge/cmd/abctl/tui/usage_stacked_test.go b/authbridge/cmd/abctl/tui/usage_stacked_test.go index d684c80f7..5aa6857c6 100644 --- a/authbridge/cmd/abctl/tui/usage_stacked_test.go +++ b/authbridge/cmd/abctl/tui/usage_stacked_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" "time" + "unicode" "github.com/rossoctl/cortex/authbridge/authlib/usage" ) @@ -272,6 +273,141 @@ func TestRenderStacked_TinySeriesIsStillVisible(t *testing.T) { } } +// A bar's height comes from the bucket total, so traffic no label claims must get +// its own band rather than being absorbed by the named series. Absorbing it drew +// a bucket that is 10% claude-sonnet-5 as a solid `s` bar: the height said "lots +// of traffic" and every row claimed to be sonnet. +func TestAllotRows_UnlabelledTrafficGetsItsOwnBand(t *testing.T) { + b := mkSeriesBuckets([]map[string]int64{{"claude-sonnet-5": 100}})[0] + b.Counts.Requests = 1000 // only 10% of the bucket is labelled + series := collectSeries([]usage.Bucket{b}, metricRequests) + + got := allotRows(b, metricRequests, series, 10, b.Requests) + + rows := map[string]int64{} + var sum int64 + for _, a := range got { + rows[a.label] = a.rows + sum += a.rows + } + if sum != 10 { + t.Fatalf("allotted %d rows, bar is 10 tall", sum) + } + if rows["claude-sonnet-5"] != 1 && rows["claude-sonnet-5"] != 2 { + t.Errorf("sonnet has %d of 10 rows for 10%% of the bucket", rows["claude-sonnet-5"]) + } + if rows[unlabelledLabel] == 0 { + t.Error("the 90% no label claims drew no band") + } +} + +// Per-plugin attribution counts one request once per plugin, so byPlugin +// sub-totals intentionally sum to MORE than the bucket (see the aggregator's +// foldInto). Dividing by the bucket total over-allotted rows, `acc` ran past the +// bar height, and whole series fell off the top — on every by-plugin bucket. +func TestAllotRows_HandlesOverAttributedSeries(t *testing.T) { + b := mkSeriesBuckets([]map[string]int64{{"p1": 1000, "p2": 1000, "p3": 1000}})[0] + b.Counts.Requests = 1000 // each plugin credited the whole bucket + series := collectSeries([]usage.Bucket{b}, metricRequests) + + got := allotRows(b, metricRequests, series, 10, b.Requests) + + var sum int64 + for _, a := range got { + if a.rows < 1 { + t.Errorf("series %q allotted %d rows", a.label, a.rows) + } + sum += a.rows + } + if sum != 10 { + t.Errorf("allotted %d rows for a 10-row bar — series would fall off the chart", sum) + } + if len(got) != 3 { + t.Errorf("allotted %d bands, want all 3 plugins present", len(got)) + } +} + +// Every band drawn must have a legend entry. Marks come from the palette and +// repeat once it wraps, so a seventh series could draw with the first one's mark +// while the legend named neither — an undecodable band. +func TestRenderStacked_EveryDrawnBandIsInTheLegend(t *testing.T) { + labels := map[string]int64{} + for i := 0; i < 12; i++ { + labels[fmt.Sprintf("series-%c", 'a'+i)] = int64(100 - i*5) + } + lines := renderStackedBars(mkSeriesBuckets([]map[string]int64{labels}), metricRequests, usage.GroupMethod, 120) + + // Split chart rows from legend rows at the axis. + var axisAt int + for i, l := range lines { + if strings.ContainsRune(stripANSI(l), '┼') { + axisAt = i + break + } + } + chart := stripANSI(strings.Join(lines[:axisAt], "\n")) + legend := stripANSI(strings.Join(lines[axisAt:], "\n")) + + // Collect the distinct marks actually drawn. + drawn := map[rune]bool{} + for _, r := range chart { + if unicode.IsLetter(r) || r == '·' { + drawn[r] = true + } + } + if len(drawn) == 0 { + t.Fatal("no marks drawn") + } + for r := range drawn { + // The legend lists each mark followed by its name. + if !strings.ContainsRune(legend, r) { + t.Errorf("mark %q is drawn on the chart but absent from the legend:\n%s", string(r), legend) + } + } +} + +// Folding the tail must preserve the totals: bounding how many bands are drawn is +// the point, losing traffic is not. +func TestFoldTailSeries_PreservesTotals(t *testing.T) { + labels := map[string]int64{} + var want int64 + for i := 0; i < 10; i++ { + v := int64(100 - i*5) + labels[fmt.Sprintf("s%c", 'a'+i)] = v + want += v + } + buckets := mkSeriesBuckets([]map[string]int64{labels}) + series := collectSeries(buckets, metricRequests) + + kept, folded := foldTailSeries(buckets, metricRequests, series, 4) + if len(kept) != 5 { // 4 named + the fold + t.Errorf("kept %d series, want 4 named plus the fold", len(kept)) + } + var got int64 + for _, c := range folded[0].Series { + got += c.Requests + } + if got != want { + t.Errorf("folded buckets total %d, want %d — folding lost traffic", got, want) + } + // An existing "(other)" from the aggregator's own capping must merge, not + // collide: two bands both meaning "the rest" would be indefensible. + withOther := mkSeriesBuckets([]map[string]int64{{ + "a": 100, "b": 90, "c": 80, "d": 70, "e": 60, tailLabel: 50, + }}) + s2 := collectSeries(withOther, metricRequests) + kept2, _ := foldTailSeries(withOther, metricRequests, s2, 3) + seen := 0 + for _, k := range kept2 { + if k.label == tailLabel { + seen++ + } + } + if seen != 1 { + t.Errorf("%d %q bands after folding, want exactly 1", seen, tailLabel) + } +} + // Allotment must fill the bar exactly: overshooting pushes top segments outside // the frame, undershooting leaves a gap at the top. func TestAllotRows_SumsToBarHeight(t *testing.T) { From 6f347af8d4b217d5486b34b5ab1d2654115d8147 Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Sun, 6 Sep 2026 12:17:55 -0400 Subject: [PATCH 6/8] fix: Keep the folded and unlabelled bands, unify the remainder predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings, all reproduced before changing anything. renderLegend re-capped at maxNamedSeries, the same number foldTailSeries had already folded to. The fold emits maxNamedSeries named bands plus "(other)", so capping again cut that fold off: the largest unnamed band was drawn on the chart and the legend replaced it with "(+1 more)", which named nothing. Capping is foldTailSeries' job alone — renderLegend now names everything it is handed, and the elision plumbing is gone. A bar too short for every band kept the first barRows of them by position. present ends with the unlabelled remainder, so that dropped exactly the band keeping the bar honest: at three rows or fewer a bucket that was 94% unclaimed reattributed all of it to the named series, reinstating the misattribution the remainder exists to prevent. It now keeps the largest bands by value, in stacking order. Writing the test for that surfaced a further case: on a ONE-row bar the remainder was excluded entirely, because 940/1000 truncates to zero rows, and the single row went to a named series holding 3% of the bucket. The threshold now rounds. unlabelledTotal counted every shortfall while allotRows only drew one that filled a row, so the legend could key a band the chart never drew. Both now call one drawsRemainderBand predicate — they had already drifted once, truncating in one and rounding in the other, which is why it is a named function rather than a repeated expression. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/cmd/abctl/tui/usage_stacked.go | 110 ++++++++++++------ .../cmd/abctl/tui/usage_stacked_test.go | 100 ++++++++++++++-- 2 files changed, 167 insertions(+), 43 deletions(-) diff --git a/authbridge/cmd/abctl/tui/usage_stacked.go b/authbridge/cmd/abctl/tui/usage_stacked.go index 92da23256..17345e2ac 100644 --- a/authbridge/cmd/abctl/tui/usage_stacked.go +++ b/authbridge/cmd/abctl/tui/usage_stacked.go @@ -98,20 +98,61 @@ func foldTailSeries(buckets []usage.Bucket, m usageMetric, series []seriesKey, k // unlabelledTotal is how much of the window no series claims, summed across // buckets that actually show a remainder band. Returns 0 when the labels account // for everything, or over-account for it (per-plugin attribution does). -func unlabelledTotal(buckets []usage.Bucket, m usageMetric, series []seriesKey) int64 { +func unlabelledTotal(buckets []usage.Bucket, m usageMetric, series []seriesKey, peak int64) int64 { var out int64 for _, b := range buckets { var sum int64 for _, s := range series { sum += m.valueOf(b.Series[s.label]) } - if d := m.value(b) - sum; d > 0 { + d := m.value(b) - sum + if d <= 0 { + continue + } + // Count it only when this bucket would actually DRAW the band. allotRows + // requires the remainder to fill at least one row, so summing every + // shortfall here named "(unlabelled)" in the legend for a chart that never + // draws it — a key to a band that is not there. + if drawsRemainderBand(d, m.value(b), barRowsFor(m.value(b), peak)) { out += d } } return out } +// drawsRemainderBand reports whether an unclaimed share of a bucket is large +// enough to occupy a row. +// +// One predicate with two callers on purpose: allotRows decides whether to DRAW the +// band and unlabelledTotal decides whether to NAME it in the legend, and when +// those two disagreed the legend keyed a band the chart never drew. They had +// already drifted once — truncating in one and rounding in the other — which is +// the whole reason this is a named function. +// +// Rounds rather than truncates: on a one-row bar 940/1000 truncates to zero rows, +// so the remainder was excluded and the single row went to a named series holding +// 3% of the bucket. +func drawsRemainderBand(remainder, total, barRows int64) bool { + if remainder <= 0 || total <= 0 || barRows <= 0 { + return false + } + return 2*remainder*barRows >= total +} + +// barRowsFor is a bar's height in whole rows. Shared by the renderer and by +// unlabelledTotal so the legend's idea of which bands are drawn cannot drift from +// the chart's. +func barRowsFor(total, peak int64) int64 { + if total <= 0 || peak <= 0 { + return 0 + } + rows := total * int64(plotRows) / peak + if rows == 0 { + rows = 1 // non-zero traffic never renders as an empty column + } + return rows +} + // collectSeries totals each label across every bucket and returns them largest // first, with ties broken by label so the legend and the stack order are stable // between renders. An unstable order would make the chart appear to reshuffle on @@ -188,11 +229,18 @@ func renderStackedBars(buckets []usage.Bucket, m usageMetric, group usage.Group, // share one. series, buckets = foldTailSeries(buckets, m, series, maxNamedSeries) + var peak int64 + for _, b := range buckets { + if v := m.value(b); v > peak { + peak = v + } + } + // The unlabelled remainder is drawn as a band but is not in `series`, so // register it for marks, colour and the legend. Appended last so it ranks // behind every named series and cannot take a palette slot from one. legendSeries := series - if unlabelled := unlabelledTotal(buckets, m, series); unlabelled > 0 { + if unlabelled := unlabelledTotal(buckets, m, series, peak); unlabelled > 0 { legendSeries = append(append([]seriesKey(nil), series...), seriesKey{label: unlabelledLabel, total: unlabelled}) } @@ -208,13 +256,6 @@ func renderStackedBars(buckets []usage.Bucket, m usageMetric, group usage.Group, // same thing in every bucket and in the legend. letters := assignLetters(legendSeries) - var peak int64 - for _, b := range buckets { - if v := m.value(b); v > peak { - peak = v - } - } - out := make([]string, 0, plotRows+4) for row := plotRows; row >= 1; row-- { var sb strings.Builder @@ -250,10 +291,7 @@ func stackedCell(b usage.Bucket, m usageMetric, group usage.Group, } // Whole rows only — see the renderStackedBars comment on why segments cannot // use partial blocks. - barRows := total * int64(plotRows) / peak - if barRows == 0 { - barRows = 1 // non-zero traffic must never render as an empty column - } + barRows := barRowsFor(total, peak) if int64(row) > barRows { return strings.Repeat(" ", barWidth) } @@ -311,7 +349,7 @@ func allotRows(b usage.Bucket, m usageMetric, series []seriesKey, barRows, total // Only when the shortfall is large enough to occupy a row: rounding noise does // not deserve a band, and a one-row remainder on every bar would be more // misleading than omitting it. - if unlabelled := total - seriesSum; unlabelled > 0 && unlabelled*barRows/total > 0 { + if unlabelled := total - seriesSum; drawsRemainderBand(unlabelled, total, barRows) { present = append(present, rowAlloc{label: unlabelledLabel, value: unlabelled}) seriesSum += unlabelled } @@ -319,9 +357,23 @@ func allotRows(b usage.Bucket, m usageMetric, series []seriesKey, barRows, total // as many as fit, largest first (series is already sorted). The legend still // names the rest. if int64(len(present)) >= barRows { - out := present[:barRows] - for i := range out { - out[i].rows = 1 + // Keep the LARGEST bands, not the first barRows of them. present ends with + // the unlabelled remainder, so truncating positionally dropped exactly the + // band that keeps the bar honest: a 3-row bar that was 94% unclaimed + // reattributed all of it to the named series, which is the misattribution + // the remainder exists to prevent. + byValue := append([]rowAlloc(nil), present...) + sort.SliceStable(byValue, func(i, j int) bool { return byValue[i].value > byValue[j].value }) + keep := make(map[string]bool, barRows) + for _, a := range byValue[:barRows] { + keep[a.label] = true + } + out := make([]rowAlloc, 0, barRows) + for _, a := range present { // preserve stacking order + if keep[a.label] { + a.rows = 1 + out = append(out, a) + } } return out } @@ -405,12 +457,12 @@ func renderLegend(series []seriesKey, group usage.Group, const sep = " " const indent = " " + // No cap here. foldTailSeries has already reduced the series to at most + // maxNamedSeries named bands plus a "(other)" fold, and re-capping at the same + // number cut that fold off — the largest unnamed band was drawn on the chart + // and replaced in the legend by "(+1 more)", which named nothing. Whatever + // reaches this function is what the chart draws, so all of it gets an entry. named := series - elided := 0 - if len(named) > maxNamedSeries { - elided = len(named) - maxNamedSeries - named = named[:maxNamedSeries] - } var lines []string var parts []string @@ -452,18 +504,6 @@ func renderLegend(series []seriesKey, group usage.Group, } flush() - if elided > 0 { - bare := fmt.Sprintf("(+%d more)", elided) - // Measure what is actually appended — sep + bare — not the indented form. - // Counting the indent instead of the separator happened to agree at width - // 80 and was three columns short everywhere else. - suffix := len([]rune(sep)) + len([]rune(bare)) - if n := len(lines); n > 0 && len([]rune(stripANSIWidth(lines[n-1])))+suffix <= width { - lines[n-1] += sep + bare - } else { - lines = append(lines, indent+bare) - } - } if len(lines) == 0 { return nil } diff --git a/authbridge/cmd/abctl/tui/usage_stacked_test.go b/authbridge/cmd/abctl/tui/usage_stacked_test.go index 5aa6857c6..60fa20ff4 100644 --- a/authbridge/cmd/abctl/tui/usage_stacked_test.go +++ b/authbridge/cmd/abctl/tui/usage_stacked_test.go @@ -233,8 +233,13 @@ func TestRenderStacked_OverflowSeriesAreMarked(t *testing.T) { } lines := renderStackedBars(mkSeriesBuckets([]map[string]int64{labels}), metricRequests, usage.GroupMethod, 80) joined := stripANSI(strings.Join(lines, "\n")) - if !strings.Contains(joined, "more)") { - t.Errorf("legend does not report series past the palette: %q", joined) + // Series past the palette fold into one named band. A "(+N more)" count would + // be worse: the band is drawn, so it needs a key, not a tally. + if !strings.Contains(joined, tailLabel) { + t.Errorf("legend does not name the folded band %q: %q", tailLabel, joined) + } + if strings.Contains(joined, "more)") { + t.Errorf("legend reports a count instead of naming the drawn band: %q", joined) } } @@ -496,10 +501,10 @@ func TestRenderLegend_BoundsALoneOverWideEntry(t *testing.T) { } } -// The elision note is appended as sep+note, so the fit check must measure that. -// Counting the indent instead happened to agree at width 80 and was three columns -// short everywhere else. -func TestRenderLegend_ElisionNoteRespectsWidth(t *testing.T) { +// renderLegend names everything it is handed — capping is foldTailSeries' job, and +// doing it in both places cut the fold off. Whatever the chart draws must be +// named, at every width, wrapping as needed. +func TestRenderLegend_NamesEverySeriesAtEveryWidth(t *testing.T) { var series []seriesKey for i := 0; i < 9; i++ { series = append(series, seriesKey{fmt.Sprintf("series-%c-name", 'a'+i), int64(100 - i)}) @@ -513,8 +518,10 @@ func TestRenderLegend_ElisionNoteRespectsWidth(t *testing.T) { for width := 30; width <= 100; width++ { lines := renderLegend(series, usage.GroupMethod, letters, rank, width) joined := stripANSI(strings.Join(lines, "\n")) - if !strings.Contains(joined, "more)") { - t.Errorf("width %d: nine series past the palette but no elision note", width) + for _, s := range series { + if !strings.Contains(joined, s.label) { + t.Errorf("width %d: legend omits %q, whose band is drawn", width, s.label) + } } for _, l := range lines { if got := len([]rune(stripANSI(l))); got > width { @@ -524,6 +531,29 @@ func TestRenderLegend_ElisionNoteRespectsWidth(t *testing.T) { } } +// The fold must survive the legend. foldTailSeries emits maxNamedSeries named +// bands plus "(other)"; re-capping at maxNamedSeries cut that fold off, so the +// largest unnamed band was drawn and the legend said "(+1 more)" — naming nothing. +func TestRenderLegend_KeepsTheFoldedBand(t *testing.T) { + var series []seriesKey + for i := 0; i < 12; i++ { + series = append(series, seriesKey{fmt.Sprintf("series-%c", 'a'+i), int64(100 - i*5)}) + } + kept, _ := foldTailSeries(nil, metricRequests, series, maxNamedSeries) + if len(kept) != maxNamedSeries+1 { + t.Fatalf("foldTailSeries kept %d, want %d named plus the fold", len(kept), maxNamedSeries) + } + letters := assignLetters(kept) + rank := map[string]int{} + for i, s := range kept { + rank[s.label] = i + } + joined := stripANSI(strings.Join(renderLegend(kept, usage.GroupMethod, letters, rank, 120), "\n")) + if !strings.Contains(joined, tailLabel) { + t.Errorf("legend dropped the folded band:\n%s", joined) + } +} + func TestTruncateLegendText(t *testing.T) { const text = " claude-sonnet-5 (2.2M)" for _, max := range []int{0, 1, 2, 5, 10, 22, 23, 40} { @@ -536,3 +566,57 @@ func TestTruncateLegendText(t *testing.T) { t.Errorf("a text that already fits was altered: %q", got) } } + +// A short bar cannot show every band, so it keeps the LARGEST — not the first few +// by position. present ends with the unlabelled remainder, so positional +// truncation dropped exactly the band that keeps the bar honest: a 3-row bar that +// was 94% unclaimed reattributed all of it to the named series. +func TestAllotRows_ShortBarKeepsTheLargestBands(t *testing.T) { + b := mkSeriesBuckets([]map[string]int64{{"a": 30, "b": 20, "c": 10}})[0] + b.Counts.Requests = 1000 // 94% of the bucket claimed by no label + series := collectSeries([]usage.Bucket{b}, metricRequests) + + for _, barRows := range []int64{1, 2, 3, 4, 10} { + got := allotRows(b, metricRequests, series, barRows, b.Requests) + var sum int64 + found := false + for _, a := range got { + sum += a.rows + if a.label == unlabelledLabel { + found = true + } + } + if sum != barRows { + t.Errorf("barRows=%d: allotted %d rows", barRows, sum) + } + if !found { + t.Errorf("barRows=%d: dropped the unlabelled band, reattributing 94%% of the bucket", barRows) + } + } +} + +// The legend must not key a band the chart never draws. allotRows only emits the +// remainder when it fills a row, so the legend has to use the same predicate. +func TestUnlabelledTotal_OnlyCountsDrawnBands(t *testing.T) { + // A one-token shortfall against a large bucket cannot fill a row. + b := mkSeriesBuckets([]map[string]int64{{"a": 999}})[0] + b.Counts.Requests = 1000 + series := collectSeries([]usage.Bucket{b}, metricRequests) + + if got := unlabelledTotal([]usage.Bucket{b}, metricRequests, series, b.Requests); got != 0 { + t.Errorf("unlabelledTotal = %d for a sub-row shortfall, want 0", got) + } + // And the rendered legend agrees. + joined := stripANSI(strings.Join(renderStackedBars([]usage.Bucket{b}, metricRequests, usage.GroupMethod, 80), "\n")) + if strings.Contains(joined, unlabelledLabel) { + t.Errorf("legend names %q for a band that is never drawn:\n%s", unlabelledLabel, joined) + } + + // A large shortfall does draw, and is named. + b2 := mkSeriesBuckets([]map[string]int64{{"a": 100}})[0] + b2.Counts.Requests = 1000 + s2 := collectSeries([]usage.Bucket{b2}, metricRequests) + if got := unlabelledTotal([]usage.Bucket{b2}, metricRequests, s2, b2.Requests); got != 900 { + t.Errorf("unlabelledTotal = %d, want 900", got) + } +} From 8d258e822390f383390c19f24fd25f2a4617f0cd Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Mon, 7 Sep 2026 13:16:23 -0400 Subject: [PATCH 7/8] fix: Attribute request-only plugins in the by-plugin usage breakdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit context-guru showed a `modify` action in abctl's event timeline but was absent from the usage screen's by-plugin chart. The listener splits plugin invocations by phase — the request event carries InvocationPhaseRequest, the response event InvocationPhaseResponse — and Record counts only response and denied events, deliberately, since a request event has no status, duration or token usage and counting it would double every request. A plugin that acts only on the request therefore appeared in neither. context-guru and tool-prune are exactly that shape (WritesRequestBody with a stub OnResponse), so `by plugin` silently meant "plugins that ran on the response". Request-phase plugin names are now held by RequestID — the field whose doc comment says it exists for this pairing — and merged when the paired response arrives. Each request-only plugin gets that response's own tokens and latency, and one turn still counts as one request: the request event contributes labels only, never traffic. Pairing on the id rather than positionally matters because a client can have several requests in flight, and positional pairing would attribute one turn's plugins to another's tokens. A plugin that ran in both phases is deduped across the two halves, as are repeated invocations from one plugin on a single pass. The pending map is bounded and swept: a response that never arrives — client disconnect, upstream hang, a proxy started mid-turn — would otherwise leak an entry per abandoned turn on the synchronous append path. Past the cap the labels are dropped rather than the map grown, and a response whose request was never seen is still counted, just without request-phase plugins. Verified live against a proxy: the request-phase invocation is now paired on requestId and folded into the same bucket. Tests cover the attribution, out-of-order pairing across concurrent turns, that a request event adds no traffic and does not dilute the latency mean, orphans in both directions, and the pending bound and sweep. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/usage/requestpair_test.go | 229 +++++++++++++++++++ authbridge/authlib/usage/usage.go | 164 +++++++++++-- 2 files changed, 370 insertions(+), 23 deletions(-) create mode 100644 authbridge/authlib/usage/requestpair_test.go diff --git a/authbridge/authlib/usage/requestpair_test.go b/authbridge/authlib/usage/requestpair_test.go new file mode 100644 index 000000000..c029ceba9 --- /dev/null +++ b/authbridge/authlib/usage/requestpair_test.go @@ -0,0 +1,229 @@ +package usage + +import ( + "fmt" + "testing" + "time" + + "github.com/rossoctl/cortex/authbridge/authlib/pipeline" +) + +// reqEvent is the request half of a turn, carrying only request-phase +// invocations — which is what the listener actually appends. +func reqEvent(at time.Time, id string, plugins ...string) *pipeline.SessionEvent { + inv := &pipeline.Invocations{} + for _, p := range plugins { + inv.Outbound = append(inv.Outbound, pipeline.Invocation{ + Plugin: p, Phase: pipeline.InvocationPhaseRequest, + }) + } + return &pipeline.SessionEvent{ + At: at, Direction: pipeline.Outbound, Phase: pipeline.SessionRequest, + RequestID: id, Invocations: inv, + } +} + +// respEventWith is the response half, carrying only response-phase invocations. +func respEventWith(at time.Time, id string, tokens int, plugins ...string) *pipeline.SessionEvent { + inv := &pipeline.Invocations{} + for _, p := range plugins { + inv.Outbound = append(inv.Outbound, pipeline.Invocation{ + Plugin: p, Phase: pipeline.InvocationPhaseResponse, + }) + } + return &pipeline.SessionEvent{ + At: at, Direction: pipeline.Outbound, Phase: pipeline.SessionResponse, + RequestID: id, StatusCode: 200, Duration: time.Second, + Inference: &pipeline.InferenceExtension{Model: "claude-sonnet-5", TotalTokens: tokens}, + Invocations: inv, + } +} + +func pluginSeries(t *testing.T, a *Aggregator, session string) map[string]Counts { + t.Helper() + out := map[string]Counts{} + for _, b := range a.Snapshot(10*time.Minute, BucketWidth, session, GroupPlugin).Buckets { + for k, v := range b.Series { + cur := out[k] + cur.Requests += v.Requests + cur.Tokens += v.Tokens + out[k] = cur + } + } + return out +} + +// A plugin that only acts on the request must appear in the by-plugin breakdown. +// The listener splits invocations by phase, so context-guru and tool-prune +// (WritesRequestBody with a stub OnResponse) appear only on the request event — +// which Record skips for counting. Before pairing, `by plugin` silently meant +// "plugins that ran on the response". +func TestRecord_RequestOnlyPluginIsAttributed(t *testing.T) { + now := time.Date(2026, 9, 7, 12, 0, 30, 0, time.UTC) + a := New(WithClock(fixedClock(now))) + + a.Record("s1", reqEvent(now, "r1", "inference-parser", "context-guru")) + a.Record("s1", respEventWith(now, "r1", 100, "inference-parser")) + + series := pluginSeries(t, a, "") + if _, ok := series["context-guru"]; !ok { + t.Fatalf("request-only plugin missing from by-plugin; got %v", keys(series)) + } + // It inherits the response's tokens, the same whole-attribution the aggregator + // already documents for multi-plugin responses. + if got := series["context-guru"].Tokens; got != 100 { + t.Errorf("context-guru tokens = %d, want 100 (the paired response's)", got) + } + if got := series["context-guru"].Requests; got != 1 { + t.Errorf("context-guru requests = %d, want 1", got) + } +} + +// The request event must contribute labels only. Counting it as traffic would +// double every request and halve the latency mean. +func TestRecord_RequestEventAddsNoTraffic(t *testing.T) { + now := time.Date(2026, 9, 7, 12, 0, 30, 0, time.UTC) + a := New(WithClock(fixedClock(now))) + + a.Record("s1", reqEvent(now, "r1", "context-guru")) + // No response yet: nothing counted at all. + if got := a.Snapshot(time.Minute, BucketWidth, "", GroupNone).Totals; got.Requests != 0 { + t.Errorf("totals after a lone request event = %+v, want zero", got) + } + + a.Record("s1", respEventWith(now, "r1", 100, "inference-parser")) + totals := a.Snapshot(time.Minute, BucketWidth, "", GroupNone).Totals + if totals.Requests != 1 { + t.Errorf("requests = %d, want 1 — one turn is one request", totals.Requests) + } + if totals.Tokens != 100 { + t.Errorf("tokens = %d, want 100", totals.Tokens) + } + b := a.Snapshot(time.Minute, BucketWidth, "", GroupNone).Buckets[0] + if b.LatMeanMs != 1000 { + t.Errorf("latency mean = %v, want 1000 — the request event must not dilute it", b.LatMeanMs) + } +} + +// Pairing is by id, not by arrival order: a client can have several requests in +// flight, and positional pairing would attribute one turn's plugins to another's +// tokens. +func TestRecord_PairsByIDNotPosition(t *testing.T) { + now := time.Date(2026, 9, 7, 12, 0, 30, 0, time.UTC) + a := New(WithClock(fixedClock(now))) + + // Two turns open; responses arrive in the opposite order. + a.Record("s1", reqEvent(now, "r1", "context-guru")) + a.Record("s1", reqEvent(now, "r2", "tool-prune")) + a.Record("s1", respEventWith(now, "r2", 20, "inference-parser")) + a.Record("s1", respEventWith(now, "r1", 700, "inference-parser")) + + series := pluginSeries(t, a, "") + if got := series["context-guru"].Tokens; got != 700 { + t.Errorf("context-guru tokens = %d, want 700 (r1's response)", got) + } + if got := series["tool-prune"].Tokens; got != 20 { + t.Errorf("tool-prune tokens = %d, want 20 (r2's response)", got) + } +} + +// A plugin that ran in both phases is one plugin that touched one turn, so it must +// not be counted twice. +func TestRecord_PluginInBothPhasesCountsOnce(t *testing.T) { + now := time.Date(2026, 9, 7, 12, 0, 30, 0, time.UTC) + a := New(WithClock(fixedClock(now))) + + a.Record("s1", reqEvent(now, "r1", "inference-parser")) + a.Record("s1", respEventWith(now, "r1", 100, "inference-parser")) + + if got := pluginSeries(t, a, "")["inference-parser"].Requests; got != 1 { + t.Errorf("inference-parser requests = %d, want 1 — it ran in both phases", got) + } +} + +// Several invocations from one plugin on a single pass are still one plugin. +func TestInvocationPlugins_Dedupes(t *testing.T) { + inv := &pipeline.Invocations{ + Outbound: []pipeline.Invocation{{Plugin: "p"}, {Plugin: "p"}, {Plugin: "q"}}, + Inbound: []pipeline.Invocation{{Plugin: "q"}, {Plugin: ""}}, + } + got := invocationPlugins(inv) + if len(got) != 2 { + t.Errorf("invocationPlugins = %v, want two distinct names", got) + } + if invocationPlugins(nil) != nil { + t.Error("invocationPlugins(nil) should be nil") + } +} + +// A response whose request never arrived must still be counted — just without +// request-phase plugins. A proxy that starts mid-turn is the normal case for this. +func TestRecord_ResponseWithoutItsRequest(t *testing.T) { + now := time.Date(2026, 9, 7, 12, 0, 30, 0, time.UTC) + a := New(WithClock(fixedClock(now))) + + a.Record("s1", respEventWith(now, "orphan", 42, "inference-parser")) + totals := a.Snapshot(time.Minute, BucketWidth, "", GroupNone).Totals + if totals.Requests != 1 || totals.Tokens != 42 { + t.Errorf("orphan response not counted: %+v", totals) + } +} + +// An event with no RequestID cannot be paired, and must not be paired +// positionally — that misattributes as soon as two requests are in flight. +func TestRecord_NoRequestIDIsNotPaired(t *testing.T) { + now := time.Date(2026, 9, 7, 12, 0, 30, 0, time.UTC) + a := New(WithClock(fixedClock(now))) + + req := reqEvent(now, "", "context-guru") + a.Record("s1", req) + if len(a.pending) != 0 { + t.Error("held a request half with no id to pair it against") + } + a.Record("s1", respEventWith(now, "", 100, "inference-parser")) + if _, ok := pluginSeries(t, a, "")["context-guru"]; ok { + t.Error("attributed an unpairable request half to a response") + } +} + +// Requests whose responses never arrive must not accumulate forever: that is one +// leaked entry per abandoned turn, on the synchronous append path. +func TestRecord_PendingIsBounded(t *testing.T) { + base := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + now := base + a := New(WithClock(func() time.Time { return now })) + + for i := 0; i < maxPendingRequests*2; i++ { + now = base.Add(time.Duration(i) * time.Second) + a.Record("s1", reqEvent(now, fmt.Sprintf("req-%d", i), "context-guru")) + } + a.mu.RLock() + n := len(a.pending) + a.mu.RUnlock() + if n > maxPendingRequests { + t.Errorf("pending holds %d entries, cap is %d", n, maxPendingRequests) + } +} + +// A request half older than pendingTTL is swept: the listener has abandoned that +// turn too, so its plugins will never be paired. +func TestRecord_StalePendingIsExpired(t *testing.T) { + base := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) + now := base + a := New(WithClock(func() time.Time { return now })) + + a.Record("s1", reqEvent(now, "old", "context-guru")) + + // Fill to the cap far in the future, which triggers the sweep. + now = base.Add(pendingTTL + time.Minute) + for i := 0; i < maxPendingRequests; i++ { + a.Record("s1", reqEvent(now, fmt.Sprintf("new-%d", i), "tool-prune")) + } + + a.mu.RLock() + _, stillThere := a.pending["old"] + a.mu.RUnlock() + if stillThere { + t.Error("a request half older than pendingTTL was not swept") + } +} diff --git a/authbridge/authlib/usage/usage.go b/authbridge/authlib/usage/usage.go index b3e16f998..ab0e981b2 100644 --- a/authbridge/authlib/usage/usage.go +++ b/authbridge/authlib/usage/usage.go @@ -144,8 +144,33 @@ type Aggregator struct { maxSess int pricer Pricer now func() time.Time + + // pending holds request-phase plugin names awaiting their response event, + // keyed by RequestID. See Record. + pending map[string]*pendingRequest +} + +// pendingRequest is the request half of one turn: the plugins that ran before the +// response existed, held until the response arrives so they can be attributed to +// the same bucket with the same tokens and latency. +type pendingRequest struct { + plugins []string + at time.Time // for expiry when no response ever arrives } +// maxPendingRequests bounds the pending map. A request whose response never +// arrives — client disconnect, upstream hang, a proxy restart mid-turn — would +// otherwise leak an entry per turn forever. +// +// Generous relative to real in-flight concurrency for one sidecar, so eviction is +// a backstop rather than something the steady state relies on. +const maxPendingRequests = 4096 + +// pendingTTL bounds how long a request half waits for its response. Matched to the +// streaming read timeout in the forward proxy: a turn quiet for longer than that +// has been abandoned by the listener too, so its plugins will never be paired. +const pendingTTL = 5 * time.Minute + // sessionRing is one session's buckets plus the last time it was written. // // lastSeen exists because the store evicts and expires sessions without telling @@ -191,6 +216,7 @@ func New(opts ...Option) *Aggregator { a := &Aggregator{ all: make([]bucket, NumBuckets), sessions: make(map[string]*sessionRing), + pending: make(map[string]*pendingRequest), maxSess: defaultMaxSessions, now: time.Now, } @@ -202,33 +228,60 @@ func New(opts ...Option) *Aggregator { // Record folds one event into the aggregate. // -// Response events only: a request event carries no status, no duration and no -// usage, so counting it would double every request and pull the latency mean -// toward zero. Denials (phase "denied") are counted as errors — they are -// requests that happened and failed, and omitting them would make an -// authentication outage look like a traffic drop. +// Counts and timings come from the response event only: a request event carries no +// status, no duration and no usage, so counting it as traffic would double every +// request and pull the latency mean toward zero. Denials (phase "denied") are +// counted as errors — they are requests that happened and failed, and omitting +// them would make an authentication outage look like a traffic drop. +// +// A request event is not ignored, though. The listener splits plugin invocations +// by phase — the request event carries InvocationPhaseRequest, the response event +// InvocationPhaseResponse — so a plugin that only acts on the request appears in +// neither the response event nor, previously, the by-plugin breakdown. context-guru +// and tool-prune are exactly that shape (WritesRequestBody with a stub OnResponse), +// so `by plugin` silently meant "plugins that ran on the response". +// +// Request-phase plugin names are therefore held by RequestID — the field that +// exists for this pairing — and merged when the paired response arrives, so each +// gets the response's own tokens and latency and one turn counts once. Pairing on +// the id rather than positionally matters because a client can have several +// requests in flight at a time. func (a *Aggregator) Record(sessionID string, e *pipeline.SessionEvent) { if e == nil { return } - if e.Phase != pipeline.SessionResponse && e.Phase != pipeline.SessionDenied { - return - } at := e.At if at.IsZero() { at = a.now() } - t := at.Truncate(BucketWidth) a.mu.Lock() defer a.mu.Unlock() - a.foldInto(a.all, t, e) + if e.Phase == pipeline.SessionRequest { + a.holdRequestPluginsLocked(e, at) + return + } + if e.Phase != pipeline.SessionResponse && e.Phase != pipeline.SessionDenied { + return + } + + // Claim the request half, if it is still waiting. + var requestPlugins []string + if e.RequestID != "" { + if p, ok := a.pending[e.RequestID]; ok { + requestPlugins = p.plugins + delete(a.pending, e.RequestID) + } + } + + t := at.Truncate(BucketWidth) + a.foldInto(a.all, t, e, requestPlugins) if ring, ok := a.sessions[sessionID]; ok { ring.lastSeen = at - a.foldInto(ring.buckets, t, e) + a.foldInto(ring.buckets, t, e, requestPlugins) return } // maxSess == 0 means no per-session rings at all — see WithMaxSessions. The @@ -246,7 +299,70 @@ func (a *Aggregator) Record(sessionID string, e *pipeline.SessionEvent) { } ring := &sessionRing{buckets: make([]bucket, NumBuckets), lastSeen: at} a.sessions[sessionID] = ring - a.foldInto(ring.buckets, t, e) + a.foldInto(ring.buckets, t, e, requestPlugins) +} + +// holdRequestPluginsLocked stashes a request event's plugin names until its +// response arrives. Caller holds mu. +// +// Nothing is counted here — no requests, no tokens, no latency. The request event +// contributes only the LABELS its plugins need in order to be attributed later. +func (a *Aggregator) holdRequestPluginsLocked(e *pipeline.SessionEvent, at time.Time) { + if e.RequestID == "" || e.Invocations == nil { + // Without an id there is nothing to pair against, and pairing positionally + // misattributes as soon as two requests are in flight — the reason + // SessionEvent carries RequestID at all. + return + } + names := invocationPlugins(e.Invocations) + if len(names) == 0 { + return + } + // Sweep before inserting so a burst of abandoned turns cannot push the map + // past its bound between sweeps. + if len(a.pending) >= maxPendingRequests { + a.expirePendingLocked(at) + } + if len(a.pending) >= maxPendingRequests { + // Still full of live requests: drop this one's labels rather than grow + // without bound. The response will still be counted, just without its + // request-phase plugins — losing a label is preferable to unbounded memory + // on the synchronous append path. + return + } + a.pending[e.RequestID] = &pendingRequest{plugins: names, at: at} +} + +// expirePendingLocked drops request halves whose response never arrived. Caller +// holds mu. +func (a *Aggregator) expirePendingLocked(now time.Time) { + for id, p := range a.pending { + if now.Sub(p.at) > pendingTTL { + delete(a.pending, id) + } + } +} + +// invocationPlugins returns the distinct plugin names in an Invocations set. +// +// Deduped because one plugin can append several invocations to a single pass, and +// counting it twice would inflate its share of a stacked bar. +func invocationPlugins(inv *pipeline.Invocations) []string { + if inv == nil { + return nil + } + seen := make(map[string]bool, len(inv.Outbound)+len(inv.Inbound)) + var out []string + for _, list := range [][]pipeline.Invocation{inv.Outbound, inv.Inbound} { + for _, iv := range list { + if iv.Plugin == "" || seen[iv.Plugin] { + continue + } + seen[iv.Plugin] = true + out = append(out, iv.Plugin) + } + } + return out } // evictColdestLocked drops the ring with the oldest lastSeen. Caller holds mu. @@ -267,7 +383,7 @@ func (a *Aggregator) evictColdestLocked() { } } -func (a *Aggregator) foldInto(ring []bucket, t time.Time, e *pipeline.SessionEvent) { +func (a *Aggregator) foldInto(ring []bucket, t time.Time, e *pipeline.SessionEvent, requestPlugins []string) { b := &ring[slot(t)] if !b.start.Equal(t) { *b = bucket{start: t} // stale lap: reset rather than accumulate onto old data @@ -313,17 +429,19 @@ func (a *Aggregator) foldInto(ring []bucket, t time.Time, e *pipeline.SessionEve // Invocations is a POINTER and is nil whenever no plugin appended a record — // which is the common case for a plain proxied response. Dereferencing it // unguarded panics inside Store.Append, i.e. on the request hot path. - if e.Invocations != nil { - for _, inv := range e.Invocations.Outbound { - if inv.Plugin != "" { - addLabel(&b.byPlugin, inv.Plugin, one) - } - } - for _, inv := range e.Invocations.Inbound { - if inv.Plugin != "" { - addLabel(&b.byPlugin, inv.Plugin, one) - } + // Response-phase plugins from this event, plus the request-phase plugins held + // from its paired request event. Deduped across the two halves: a plugin that + // ran in both phases is still one plugin that touched one turn. + seen := make(map[string]bool, 4) + for _, name := range invocationPlugins(e.Invocations) { + seen[name] = true + addLabel(&b.byPlugin, name, one) + } + for _, name := range requestPlugins { + if seen[name] { + continue } + addLabel(&b.byPlugin, name, one) } } From 3f2a7e214086ef1747560e28e9cd1985da273c1f Mon Sep 17 00:00:00 2001 From: Ed Snible Date: Tue, 8 Sep 2026 09:07:50 -0400 Subject: [PATCH 8/8] fix: Draw the remainder for a fully unlabelled bucket, plus review nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses pdettori's review on #883. **Blocking:** allotRows returned early on seriesSum == 0, before the remainder band was considered. A bucket with traffic but no labelled series therefore got no allotment at all; stackedCell exhausted its empty loop and fell through to painting every row as series[0], while unlabelledTotal counted that same bucket and keyed an (unlabelled) band the chart never drew. That is the chart/legend divergence drawsRemainderBand was factored out to prevent, arrived at from the other side — and the same misattribution class this PR claims to fix, reached through the denominator being zero rather than through the proportion. Reachable in the by-plugin view by any bucket whose turns invoked no plugin. The remainder is now considered before the empty check, so a fully unclaimed bucket allots its rows to (unlabelled). Two tests cover it: the allotment itself, and that the rendered bar contains only the remainder mark and agrees with its legend. The existing test covered only the partially-labelled case (seriesSum > 0), which is why the suite passed straight through this. Non-blocking: - Record acquired a.mu before holdRequestPluginsLocked's two cheap guards, so a plain proxied request with no plugins — the common case, and the one this runs synchronously inside Store.Append for — took a global mutex only to return immediately. Both guards are now checked before the lock; neither touches aggregator state. They stay in the helper so it is correct on its own terms. - Dropped stripANSIWidth: no callers. renderLegend tracks display width with a running counter instead, which never builds a stripped copy. - Dropped foldTailSeries' unused usageMetric parameter. - Moved unlabelledMark above seriesLetter's doc comment — wedged between the comment and the function, it took the whole block in godoc and left seriesLetter undocumented. - Corrected the maxNamedSeries comment: the palette bounds NAMED series, but (unlabelled) and (other) are appended outside that bound and wrap onto the first two named colours. Harmless by design — the letter is the encoding — but the comment claimed more than it delivered. - README: the Keybindings table had no usage-pane rows at all, not even `u` to open it, and the Panes section described every pane but this one. Both now cover it, including why `b` is absent for latency. Assisted-By: Claude (Anthropic AI) Signed-off-by: Ed Snible --- authbridge/authlib/usage/usage.go | 13 ++++ authbridge/cmd/abctl/README.md | 29 +++++++ authbridge/cmd/abctl/tui/usage_glyphs.go | 10 +-- authbridge/cmd/abctl/tui/usage_stacked.go | 47 ++++++------ .../cmd/abctl/tui/usage_stacked_test.go | 75 ++++++++++++++++++- 5 files changed, 142 insertions(+), 32 deletions(-) diff --git a/authbridge/authlib/usage/usage.go b/authbridge/authlib/usage/usage.go index ab0e981b2..17fb95a06 100644 --- a/authbridge/authlib/usage/usage.go +++ b/authbridge/authlib/usage/usage.go @@ -251,6 +251,15 @@ func (a *Aggregator) Record(sessionID string, e *pipeline.SessionEvent) { return } + // A request event with nothing to hold is the common case — Invocations is nil + // on any plain proxied request — and this runs synchronously inside + // Store.Append on the request hot path. Checked before the lock so that path + // stays lock-free: neither guard touches aggregator state, so there is nothing + // to protect. + if e.Phase == pipeline.SessionRequest && (e.RequestID == "" || e.Invocations == nil) { + return + } + at := e.At if at.IsZero() { at = a.now() @@ -312,6 +321,10 @@ func (a *Aggregator) holdRequestPluginsLocked(e *pipeline.SessionEvent, at time. // Without an id there is nothing to pair against, and pairing positionally // misattributes as soon as two requests are in flight — the reason // SessionEvent carries RequestID at all. + // + // Record checks the same two conditions before taking the lock, so this is + // normally unreachable; kept so the helper is correct on its own terms + // rather than relying on its only caller. return } names := invocationPlugins(e.Invocations) diff --git a/authbridge/cmd/abctl/README.md b/authbridge/cmd/abctl/README.md index 2a9690c79..14f86a304 100644 --- a/authbridge/cmd/abctl/README.md +++ b/authbridge/cmd/abctl/README.md @@ -101,6 +101,29 @@ The UI has these top-level panes. `Enter` drills in; `Esc` backs out. - **Plugin detail**: drill-into-row for Pipeline or Catalog. Shows description, position, reads/writes, body access, plugin config, and per-dependency satisfaction status against the active chain. +- **Usage**: time-bucketed charts of volume, errors, latency and cost, + opened by `u` from Sessions (all sessions) or Events/Detail (the + selected session). Sourced from `/v1/usage`, which the proxy + aggregates server-side — so every operator watching a pod sees the + same history, including traffic from before they attached. Refetches + every 20s while in view. + + `m` cycles the metric. Counts (tokens/requests/errors) render as bars; + latency renders as mean-with-whiskers (`┼` mean, `┬`/`┴` ±1σ), because + a bar encodes magnitude from a zero baseline and mean latency has no + meaningful zero. `b` cycles the breakdown, which stacks each bar by + status, model or plugin — each series marked with a letter derived + from its name (`s` for claude-sonnet-5) on a coloured ground, so the + chart reads without colour too. Statuses ≥400 render red. `b` is not + offered for latency: the aggregator holds no per-label latency, so + there is no per-status mean to plot. + + An idle bucket shows `0` rather than an empty column, so a gap in + traffic is distinguishable from traffic too small to plot. A bucket + carrying traffic no label claims shows an `(unlabelled)` band, and + series past the palette fold into `(other)` — every band drawn has a + legend entry. + - **Catalog**: registered-plugin browser, opened by `P` from any session-view pane. Lists every plugin the running binary knows how to construct, including ones not in the active pipeline. Useful for @@ -148,6 +171,12 @@ Layered on top of all of them: | `p` | any | pause/resume stream | | `y` | detail | yank event JSON to `/tmp` | | `g` / `G` | lists | jump to top / bottom | +| `u` | sessions, events, detail | open the usage charts (sessions: all sessions; events/detail: the selected session) | +| `m` | usage | cycle metric: tokens / requests / errors / latency | +| `w` | usage | cycle window: 10m / 1h / 6h | +| `b` | usage | cycle breakdown: none / status / method / plugin (not offered for latency — there is no per-label latency) | +| `s` | usage | toggle between this session and all sessions | +| `Esc` | usage | back to the pane it was opened from | | `P` | any session-view pane (not the picker) | open the registered-plugin catalog | | `r` | catalog | refresh the catalog from `/v1/plugins` | | `e` | pipeline | edit pipeline subtree in `$EDITOR` | diff --git a/authbridge/cmd/abctl/tui/usage_glyphs.go b/authbridge/cmd/abctl/tui/usage_glyphs.go index 89e30ed63..a04d7e885 100644 --- a/authbridge/cmd/abctl/tui/usage_glyphs.go +++ b/authbridge/cmd/abctl/tui/usage_glyphs.go @@ -45,17 +45,17 @@ var vendorPrefixes = map[string]bool{ "mistral": true, "cohere": true, "ollama": true, "litellm": true, } +// unlabelledMark is the band for traffic no series claims. A middle dot rather +// than a letter: there is no name to abbreviate, and it should not read as one +// more model. +const unlabelledMark = '·' + // seriesLetter derives a one-character mark from a label. // // The rules, in order: skip a vendor prefix so sibling models differ; prefer a // letter over a digit so "claude-sonnet-5" is `s` and not `5`; fall back to the // first usable character; and use '?' when nothing qualifies. Callers dedupe the // result — see assignLetters. -// unlabelledMark is the band for traffic no series claims. A middle dot rather -// than a letter: there is no name to abbreviate, and it should not read as one -// more model. -const unlabelledMark = '·' - func seriesLetter(label string) rune { if label == unlabelledLabel { return unlabelledMark diff --git a/authbridge/cmd/abctl/tui/usage_stacked.go b/authbridge/cmd/abctl/tui/usage_stacked.go index 17345e2ac..6fea037c1 100644 --- a/authbridge/cmd/abctl/tui/usage_stacked.go +++ b/authbridge/cmd/abctl/tui/usage_stacked.go @@ -14,8 +14,15 @@ import ( const unlabelledLabel = "(unlabelled)" // maxNamedSeries is how many series get their own mark and legend entry before -// the rest fold together. Bounded by the palette so no two named series share a +// the rest fold together. Bounded by the palette so no two NAMED series share a // colour. +// +// The two synthetic bands are outside that bound: renderStackedBars appends +// "(unlabelled)" and foldTailSeries can add "(other)", so with the palette full +// their ranks wrap onto the first two named colours. Harmless by the design in +// usage_glyphs.go — the letter is the encoding, and `·` and `o` still tell them +// apart from any model — but the colour alone does not distinguish them, which is +// why this says "named" rather than "every". var maxNamedSeries = len(seriesPalette) // seriesKey is one label's total across the window, used to decide segment order @@ -39,7 +46,7 @@ const tailLabel = "(other)" // seventh series could draw with the same mark as the first while the legend // named neither. Returns the buckets unchanged when nothing needs folding, so the // common case allocates nothing. -func foldTailSeries(buckets []usage.Bucket, m usageMetric, series []seriesKey, keep int) ([]seriesKey, []usage.Bucket) { +func foldTailSeries(buckets []usage.Bucket, series []seriesKey, keep int) ([]seriesKey, []usage.Bucket) { if len(series) <= keep { return series, buckets } @@ -227,7 +234,7 @@ func renderStackedBars(buckets []usage.Bucket, m usageMetric, group usage.Group, // while the legend named only the first few left bands nothing could decode — // the marks repeat once the palette wraps, so two unrelated series could even // share one. - series, buckets = foldTailSeries(buckets, m, series, maxNamedSeries) + series, buckets = foldTailSeries(buckets, series, maxNamedSeries) var peak int64 for _, b := range buckets { @@ -337,15 +344,21 @@ func allotRows(b usage.Bucket, m usageMetric, series []seriesKey, barRows, total seriesSum += v } } - if len(present) == 0 || seriesSum == 0 { - return nil - } // Traffic no label claims gets its own band rather than being absorbed by the // named series. The bar's height comes from the bucket total, so silently // sharing the unlabelled remainder out drew a bucket that is 10% // claude-sonnet-5 as a solid `s` bar — the height said "lots of traffic" and // every row of it claimed to be sonnet. // + // Considered BEFORE the empty check below, not after. Returning early on + // seriesSum == 0 reached the same misattribution by another route: a bucket + // with traffic but no labelled series got no allotment at all, stackedCell + // exhausted its empty loop and fell through to painting every row as + // series[0], while unlabelledTotal counted that bucket and keyed an + // (unlabelled) band the chart never drew. That is the chart/legend divergence + // drawsRemainderBand exists to prevent, inverted. Reachable in the by-plugin + // view by any bucket whose turns invoked no plugin. + // // Only when the shortfall is large enough to occupy a row: rounding noise does // not deserve a band, and a one-row remainder on every bar would be more // misleading than omitting it. @@ -353,6 +366,10 @@ func allotRows(b usage.Bucket, m usageMetric, series []seriesKey, barRows, total present = append(present, rowAlloc{label: unlabelledLabel, value: unlabelled}) seriesSum += unlabelled } + // Nothing to draw: neither a labelled series nor a remainder worth a row. + if len(present) == 0 || seriesSum == 0 { + return nil + } // More series than rows: the bar cannot show them all, so give a row each to // as many as fit, largest first (series is already sorted). The legend still // names the rest. @@ -536,21 +553,3 @@ func truncateLegendText(text string, max int) string { } return string(r[:max-1]) + "…" } - -// stripANSIWidth returns text with escape sequences removed, for measuring how -// many columns a styled string occupies. -func stripANSIWidth(s string) string { - var b strings.Builder - for i := 0; i < len(s); { - if s[i] == 0x1b { - for i < len(s) && s[i] != 'm' { - i++ - } - i++ - continue - } - b.WriteByte(s[i]) - i++ - } - return b.String() -} diff --git a/authbridge/cmd/abctl/tui/usage_stacked_test.go b/authbridge/cmd/abctl/tui/usage_stacked_test.go index 60fa20ff4..a98649d37 100644 --- a/authbridge/cmd/abctl/tui/usage_stacked_test.go +++ b/authbridge/cmd/abctl/tui/usage_stacked_test.go @@ -384,7 +384,7 @@ func TestFoldTailSeries_PreservesTotals(t *testing.T) { buckets := mkSeriesBuckets([]map[string]int64{labels}) series := collectSeries(buckets, metricRequests) - kept, folded := foldTailSeries(buckets, metricRequests, series, 4) + kept, folded := foldTailSeries(buckets, series, 4) if len(kept) != 5 { // 4 named + the fold t.Errorf("kept %d series, want 4 named plus the fold", len(kept)) } @@ -401,7 +401,7 @@ func TestFoldTailSeries_PreservesTotals(t *testing.T) { "a": 100, "b": 90, "c": 80, "d": 70, "e": 60, tailLabel: 50, }}) s2 := collectSeries(withOther, metricRequests) - kept2, _ := foldTailSeries(withOther, metricRequests, s2, 3) + kept2, _ := foldTailSeries(withOther, s2, 3) seen := 0 for _, k := range kept2 { if k.label == tailLabel { @@ -539,7 +539,7 @@ func TestRenderLegend_KeepsTheFoldedBand(t *testing.T) { for i := 0; i < 12; i++ { series = append(series, seriesKey{fmt.Sprintf("series-%c", 'a'+i), int64(100 - i*5)}) } - kept, _ := foldTailSeries(nil, metricRequests, series, maxNamedSeries) + kept, _ := foldTailSeries(nil, series, maxNamedSeries) if len(kept) != maxNamedSeries+1 { t.Fatalf("foldTailSeries kept %d, want %d named plus the fold", len(kept), maxNamedSeries) } @@ -620,3 +620,72 @@ func TestUnlabelledTotal_OnlyCountsDrawnBands(t *testing.T) { t.Errorf("unlabelledTotal = %d, want 900", got) } } + +// A bucket with traffic but NO labelled series at all must draw the remainder, +// not fall through to painting every row as the largest series. +// +// The existing unlabelled test covers the partially-labelled case (seriesSum > 0), +// which is why the suite passed straight through this one: an early return on +// seriesSum == 0 gave the bucket no allotment, stackedCell exhausted its empty +// loop and hit the series[0] fallback, and unlabelledTotal counted the bucket +// anyway — so the legend keyed an (unlabelled) band the chart never drew. +// +// Reachable in the by-plugin view by any bucket whose turns invoked no plugin. +func TestAllotRows_FullyUnlabelledBucketDrawsTheRemainder(t *testing.T) { + b := usage.Bucket{Counts: usage.Counts{Requests: 900}} // no Series at all + labelled := mkSeriesBuckets([]map[string]int64{{"plugin-x": 800, "plugin-y": 200}})[0] + series := collectSeries([]usage.Bucket{labelled, b}, metricRequests) + + got := allotRows(b, metricRequests, series, 9, b.Requests) + if len(got) == 0 { + t.Fatal("no allotment for a bucket with traffic — every row would paint as series[0]") + } + var sum int64 + for _, a := range got { + if a.label != unlabelledLabel { + t.Errorf("allotted rows to %q, which claims none of this bucket", a.label) + } + sum += a.rows + } + if sum != 9 { + t.Errorf("allotted %d rows, bar is 9 tall", sum) + } +} + +// The chart and the legend must agree on a fully unlabelled bucket: it draws the +// remainder mark, and no named series appears in a bucket that has none. +func TestRenderStacked_FullyUnlabelledBucketMatchesItsLegend(t *testing.T) { + base := time.Date(2026, 9, 8, 12, 0, 0, 0, time.UTC) + labelled := usage.Bucket{At: base, Counts: usage.Counts{Requests: 1000}, + Series: map[string]usage.Counts{ + "plugin-x": {Requests: 800}, "plugin-y": {Requests: 200}, + }} + unlabelled := usage.Bucket{At: base.Add(time.Minute), Counts: usage.Counts{Requests: 900}} + + lines := renderStackedBars([]usage.Bucket{labelled, unlabelled}, metricRequests, usage.GroupPlugin, 80) + + // The second bar's column: axisLabel + 1*barStride. + col := axisLabel + barStride + var secondBar string + for _, l := range lines[:plotRows] { + r := []rune(stripANSI(l)) + if len(r) > col && r[col] != ' ' { + secondBar += string(r[col]) + } + } + if secondBar == "" { + t.Fatal("the fully unlabelled bucket drew nothing") + } + for _, r := range secondBar { + if r != unlabelledMark { + t.Errorf("unlabelled bucket drew %q, want only %q — a named series was misattributed", + secondBar, string(unlabelledMark)) + break + } + } + // And the legend keys exactly what was drawn. + joined := stripANSI(strings.Join(lines, "\n")) + if !strings.Contains(joined, unlabelledLabel) { + t.Error("legend does not name the remainder band that is drawn") + } +}