Skip to content

refactor(status): carry series dim/node structurally instead of the dim|node string#1484

Merged
kylebernhardy merged 2 commits into
stagefrom
kyle/series-keys
Jul 13, 2026
Merged

refactor(status): carry series dim/node structurally instead of the dim|node string#1484
kylebernhardy merged 2 commits into
stagefrom
kyle/series-keys

Conversation

@kylebernhardy

Copy link
Copy Markdown
Member

Closes #1450 — Use structured series keys instead of the dim|node string protocol.

The corruption class this closes

The pipeline encoded per-node series identity as a `${dim}|${node}` string, and six consumers re-derived dim/node from it — four with hand-rolled first-| splits, two via forked extractNode copies. Any dimension value containing | (URL paths can) truncated the dim and mangled the node — e.g. dim /api|v2/x on node n1.example.com produced a chip labeled /api and a series labeled v2/x|n1. connection.tsx layered a second string protocol on top, splitting its composite chip value on ' · ' to recover {path, method} for threshold scoping — same corruption class.

Now Series carries structured dim and node fields, populated by runPipeline on every path (groupBy per-node, groupBy cluster, groupBy-'node', Other bucket, field mode per-node and cluster). The composite key still exists — as a React key / recharts series name, built by the exported makeSeriesKey(dim, node) — but it is documented as write-only and no consumer parses it back. Hand-rolled indexOf('|')/slice parsing is gone from connection.tsx, memory.tsx, main-thread-utilization.tsx, DimensionSelectorRenderer, SmallMultiples, and LineChartWithNodeLegend; connection.tsx's ' · ' split is replaced by a composite-value → {path, method} map built during preprocessing.

extractNode unification

The two forked parsers now share one vocabulary — the structured fields — with the divergence made explicit instead of accidental:

  • SmallMultiples legends per-node series only: it reads Series.node and cluster-aggregate series (no node) stay out of the legend, exactly as before.
  • LineChartWithNodeLegend keeps its deliberate fallback via a documented legendGroup(s) = s.node ?? s.key: generic 'line' dispatch also feeds it cluster series keyed by dimension value (database-size with perNode: false) and derived series keyed by node id, both of which want one legend chip per key. Previously this fallback was an undocumented divergence in a copy-pasted extractNode.

groupBy-'node' specs (utilization, tls-reused, storage-volume) now get node set by the pipeline itself, so the fallback is only exercised by the two key-per-legend-chip cases above.

Shared per-node scaffolding

The ~15-line filter/relabel/color block (and the 5× copy-pasted shortenNodeLabel) collapse into:

  • hooks/useNodeFilteredSeries(data, nodes) — relabels per-node series with the short node name + stable node color, filters by the NodeLegend active-set, and returns isActive/handleLegendClick for the legend. Adopted by connection, memory, main-thread-utilization, and DimensionSelectorRenderer.
  • lib/nodeLabels.ts shortenNodeLabel — single definition, also adopted by PerPathRateRenderer (which had its own copy).

The surrounding chip/legend JSX was left in place — each renderer's layout differs enough (quantile radios, combobox switch, threshold filtering) that a shared layout component would have needed as many props as it saved lines.

Tests

  • pipeline-per-node.test.ts: new assertions that structured dim/node are populated on the groupBy per-node, field-mode per-node, cluster, and groupBy-'node' paths, plus the regression the issue was filed for — a dim of /api|v2/x round-trips uncorrupted.
  • New dimension-selector-pipe-dim.test.tsx: renderer-layer regression — with records whose path contains |, the chip row shows the full value as one chip, and selecting it yields one cleanly-labeled line per node (asserted via the chart's aria-label, which the old first-| split rendered as v2/x|n1).
  • Full suite: 201 files / 1336 passed, 11 skipped. tsc -b, oxlint, dprint clean.

Open concerns

  • makeSeriesKey output is still ambiguous as a string (dim a|b + node c collides with dim a + node b|c). It is only a React/recharts id now, so a collision would need two series in the same panel with that exact overlap — accepted as out of scope; making the key collision-proof (e.g. length-prefixing) would churn the keys pinned in tests for no user-visible gain.
  • Derived metrics (request-rate, error-rate, etc.) still emit Series without dim/node; consumers use the ?? s.key fallback for them. Migrating derived producers is follow-up material.
  • connection.tsx's composite chip value (path · method) can theoretically collide if a path contains ' · ' — but that collision already merges the records in the pipeline's groupBy dimension itself, so it predates this PR and isn't made worse by the new dimParts map (Gemini review nit, acknowledged).

Cross-model review

  • Codex (review --uncommitted): no actionable correctness issues.
  • Gemini 3.1 Pro: one accepted finding — thresholdMatches checked !selectedDims before !t.scope, so scope-less (global) thresholds would vanish when the selected dim had no dimParts entry (e.g. a future Other bucket); fixed by testing !t.scope first. Its remaining notes verified the legend-semantics preservation in both primitives and confirmed no lingering | parsers anywhere in the analytics tree.

Generated by kAIle (Claude Fable 5).

…im|node string

Series now carries structured dim/node fields populated by runPipeline on
every path (groupBy per-node/cluster/'node'/Other, field per-node/cluster).
The composite key remains only as a React/recharts id via the exported
makeSeriesKey and is never parsed back, so dimension values containing '|'
(URL paths can) no longer corrupt per-node splitting. connection.tsx's
secondary ' · ' split is replaced by a composite→{path,method} map, with
scope-less thresholds kept global regardless of map hits (Gemini review).

The forked extractNode semantics are unified on the structured fields:
SmallMultiples legends per-node series only; LineChartWithNodeLegend keeps
a documented node-or-whole-key fallback for cluster/derived series. The
copy-pasted per-node filter/relabel/color scaffolding and 5x duplicated
shortenNodeLabel collapse into useNodeFilteredSeries + lib/nodeLabels.

Fixes #1450

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UZEaYdKFXuQw2Hf6XpPdEV
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 48.1% 4532 / 9421
🔵 Statements 48.46% 4824 / 9954
🔵 Functions 39.85% 1077 / 2702
🔵 Branches 40.52% 2954 / 7290
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/features/instance/status/analytics/hooks/useNodeFilteredSeries.ts 100% 75% 100% 100%
src/features/instance/status/analytics/lib/nodeLabels.ts 100% 100% 100% 100%
src/features/instance/status/analytics/pipeline/connection.tsx 70% 43.9% 90% 71.42% 59-60, 79-84, 110, 123, 125-129, 133
src/features/instance/status/analytics/pipeline/main-thread-utilization.tsx 80% 62.5% 75% 86.66% 115-116
src/features/instance/status/analytics/pipeline/memory.tsx 100% 83.33% 100% 100%
src/features/instance/status/analytics/pipeline/pipeline.ts 93.71% 83.33% 82.35% 96.35% 124-127, 225-226, 229, 253, 307, 326, 340, 418
src/features/instance/status/analytics/primitives/DimensionSelectorRenderer.tsx 92.1% 84.09% 91.66% 96.66% 67, 88, 132
src/features/instance/status/analytics/primitives/LineChartWithNodeLegend.tsx 44.44% 8.33% 50% 46.66% 31, 38-39, 50-56
src/features/instance/status/analytics/primitives/PerPathRateRenderer.tsx 69.23% 53.12% 50% 76.19% 57-58, 61, 85-88
src/features/instance/status/analytics/primitives/SmallMultiples.tsx 93.75% 50% 100% 100% 37
Generated in workflow #1457 for commit f4fb01c by the Vitest Coverage Report Action

gemini-code-assist[bot]

This comment was marked as resolved.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kylebernhardy
kylebernhardy marked this pull request as ready for review July 13, 2026 14:28
@kylebernhardy
kylebernhardy requested a review from a team as a code owner July 13, 2026 14:28

@kriszyp kriszyp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean structured-field refactor — nicely done. I verified the restructured data flow preserves per-series/per-node grouping end to end: every pipeline emit path sets dim/node, all six consumers are migrated off the dim|node string protocol (zero |-parsers left in the analytics tree), and the #1477 per-instance threshold scoping is intact — actually strengthened by the scope-order and numeric-cast fixes in connection.tsx.

One minor, non-blocking suggestion: the group === '' branch in LineChartWithNodeLegend.legendGroup looks unreachable (legendGroup returns s.node or s.key, neither ever '') — harmless, could be dropped whenever you're back in that file.

Approving. Thanks for the thorough tests and the documented extractNode divergence.

— Claude (Sonnet 5), reviewed via review-queue

@kylebernhardy
kylebernhardy added this pull request to the merge queue Jul 13, 2026
Merged via the queue into stage with commit 8544c10 Jul 13, 2026
3 checks passed
@kylebernhardy
kylebernhardy deleted the kyle/series-keys branch July 13, 2026 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use structured series keys instead of the dim|node string protocol

2 participants