Skip to content

fix(status): stop the connections chart diving to ~0 on single-node buckets - #1587

Merged
dawsontoth merged 12 commits into
stagefrom
claude/mqtt-status-chart-gaps-ace583
Jul 30, 2026
Merged

fix(status): stop the connections chart diving to ~0 on single-node buckets#1587
dawsontoth merged 12 commits into
stagefrom
claude/mqtt-status-chart-gaps-ace583

Conversation

@dawsontoth

@dawsontoth dawsontoth commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #1576.

Customer-reported (harper-pro 5.1.22): the Status → Traffic → Connections panel showed frequent one-sample dives to near zero, while Grafana over the same instances and window showed none. The customer was load-testing and watching connections closely; nothing actually dropped.

Harper never reports a zero. The tell is that each dive's depth equals the quiet node's own connection count at that instant, not 0 — one spike in the customer's screenshot bottoms at ~100 rather than 0, which is the Milan node sitting at ~100 connections at the time. A real connection loss can't do that.

Root cause

Three things compound:

  1. Core emits mqtt-connections only when the count is nonzero — if (numberOfConnections > 0) in harper's server/mqtt.ts (~L177). A node at 0 connections emits no row at all, so per-node coverage of the gauge is inherently ragged.
  2. Those rows carry period: 0, so resolvePeriod falls back to the spec's bucket.fallbackMs (60 s) and snapToBucketTime rounds every row onto a 60 s lattice — while the observed emission cadence is 90 s. 90 mod 60 = 30, so the two nodes' phases beat against the lattice and co-land in the same bucket only intermittently.
  3. crossNode: 'sum' then sums only the nodes present in that bucket. A bucket holding just the quiet node renders the cluster total as that node's 1–5 connections instead of ~289 — a one-sample vertical dive to ≈0.

What changed

Bucket alignment alone can't fix this: core's > 0 guard guarantees ragged coverage however well we bucket. So gauge-shaped specs now get per-node last-observation-carry-forward with a bounded staleness horizon before the cross-node sum.

New module pipeline/carryForward.ts holds the policy; pipeline.ts grows one aggregateOverTime helper that replaces the three places which previously looped aggregateTwoPass over sorted bucket times (groupBy series, the Other bucket, and field-mode series).

A node absent from a bucket contributes its most recent value, but only for ~2 of its own emission intervals — estimated as the median gap between that node's observation instants, floored at the spec's bucket size. The bound is the point: "no row" is genuinely ambiguous between "this node's sample landed in the adjacent bucket" (carry it) and "this node is idle at 0" (don't). Unbounded LOCF would overstate a node that has genuinely gone quiet. Median rather than mean so a node that keeps crossing zero is still credited with its active cadence instead of one inflated by the absences.

The cadence is measured from raw, pre-snap instants — the estimator has to see the true 90 s cadence, not the 60/120 alternation the lattice snap produces.

Scope

Keys on crossNode: 'sum' with a temporal aggregator of max/last, i.e. a cross-node sum of per-node level snapshots. Today that is exactly:

  • connections (max/sum) — the reported panel
  • database-size (last/sum) — the identical latent bug, called out in the issue

Additive counters (sum/summqtt-traffic-*, bytes-sent/bytes-received, fsWrite) are deliberately excluded: there a missing row legitimately means zero, and carrying the previous bucket's throughput forward would invent traffic that never happened. There's a test asserting this for bytes-sent.

Invariants preserved

  • Carry-forward never creates bucket times. A bucket where no node reported stays absent, so the series keeps its original x positions and a genuine cluster-wide gap still reads as a gap.
  • Carried values carry count: 0, so SeriesPoint.count — which drives confidence gating and the tooltip's sample count — keeps reflecting real observations only.
  • Per-node mode is untouched. The chip-selector and "Stack by: Node" views read per-node values directly; carry-forward belongs to the cross-node sum alone.
  • LOCF only looks backwards — buckets before a node's first sample are not back-filled.

Verification

The new tests fail without the fix. Gating isGaugeCrossNodeSum to false produces expected [300, 300, 100, 300] to deeply equal [300, 300, 300, 300] — the dive to one node's own value, reproduced exactly.

Replaying the reported row shape (960 + 228 rows over 24 h, period: 0, 90 s cadence, independent phases) reproduces the issue's evidence block and then clears it:

before after
minimum y in series 1 283
points below 200 (dives) 114 0
bucket census 114 both · 846 us-west-only · 114 milan-only unchanged

Live on a real 2-node stage cluster (anvils.acme-inc.stage). I temporarily instrumented aggregateOverTime and loaded the Storage tab: nodes=2 buckets=1440 incompleteBuckets=960 carried=959 expired=0, horizons ≈ 180 002 ms. Two things worth a reviewer's attention:

  • This bug was live on our own stage cluster too — 67% of database-size buckets held only one node, across all five database bands. It just reads as a plausible-looking wobble rather than an obvious dive, because the bands are large and slow-moving.
  • The cadence estimator independently landed on ~90 s there as well, so fallbackMs: 60000 is systematically wrong for these gauge metrics. That corroborates the issue's secondary finding that core stamping a real period would remove the guess entirely.

Probe removed afterwards; the panel renders a clean continuous 24 h stack at the full two-node total, all five bands, no error-boundary fallback.

Tests

The existing connections-pipeline.test.ts / database-size-pipeline.test.ts suites only exercise uniform period: 60000 with every node present at every timestamp, so this class of bug could not surface there. Added:

  • __tests__/pipeline/cross-node-gauge-gaps.test.ts — staggered 90 s cadence with lattice phase beating (including a guard asserting the setup really does produce single-node buckets, so nothing passes vacuously); horizon-expiry when a node goes genuinely idle; cluster-wide gaps staying gaps; no node resurrected across an outage; point-count and count invariants; database-size same-shape case; bytes-sent counter-untouched case.
  • __tests__/pipeline/carryForward.test.ts — unit coverage for the shape predicate, the median-gap estimator (order independence, duplicate instants, scattered absences), and horizon construction.
  • __tests__/fixtures/connections/node-drop.json — minimal hand-checkable node-drop fixture, mirroring the table-size/node-drop.json precedent the issue points at.

Full repo gate green: 1881 tests / 264 files, tsc -b, oxlint, dprint check.

Deliberately out of scope

  • Bucket alignment. The issue notes it's insufficient on its own, and changing snapToBucketTime would affect every spec for no gain here.
  • A spec-level opt-out knob. The gauge shape is derived from one source of truth (isGaugeCrossNodeSum); easy to add a knob later if reviewers prefer it explicit per spec.
  • StackedAreaChart's render-layer forward-fill (lines 98–113) is unbounded. That's why "Stack by: Node" mode never showed these dives — but it also means a genuinely idle node's band persists for the rest of the window, which is now inconsistent with the bounded horizon one layer up. It touches every stacked-area panel, so it wants its own change.
  • The issue's other secondary findings (harper-pro ignoring bucket_ms; analytics.aggregatePeriod: 60 vs rows landing 90 s apart) are core-side and want a companion core issue.

🤖 Generated with Claude Code

…uckets

The Status → Traffic → Connections panel showed frequent one-sample dives to
near zero while Grafana over the same window showed none. Harper never reports
a zero — each dive's depth equalled the *quiet* node's own connection count,
which a real disconnect can't produce.

Three things compound into the artifact:

- Core emits `mqtt-connections` only when the count is nonzero
  (`if (numberOfConnections > 0)` in server/mqtt.ts), so a node at 0 emits no
  row at all and per-node coverage is inherently ragged.
- Those gauge rows carry `period: 0`, so `resolvePeriod` falls back to the
  spec's `bucket.fallbackMs` (60 s) and every row snaps onto a 60 s lattice —
  while the observed emission cadence is 90 s. 90 mod 60 = 30, so the nodes'
  phases beat against the lattice and co-land only intermittently.
- `crossNode: 'sum'` then sums only the nodes *present* in a bucket, so a
  bucket holding one node renders the cluster total as that node's value.

Bucket alignment alone can't fix this — core's `> 0` guard guarantees ragged
coverage however well we bucket. Instead, gauge-shaped specs now get per-node
last-observation-carry-forward with a bounded staleness horizon before the
cross-node sum: a node absent from a bucket contributes its most recent value,
but only for ~2 of its own emission intervals (estimated from the median gap
between its observation instants, floored at the bucket size). The bound
matters because "no row" is ambiguous between "this node's sample landed in
the adjacent bucket" (carry it) and "this node is idle at 0" (don't) —
unbounded LOCF would overstate a node that has genuinely gone quiet.

This keys on `crossNode: 'sum'` with a temporal aggregator of `max`/`last`,
which covers `connections` (max/sum) and `database-size` (last/sum) — the
latter had the identical latent bug. Additive counters like `mqtt-traffic-*`
and `bytes-sent` (sum/sum) are deliberately excluded: there a missing row
legitimately means zero, and carrying the previous bucket's throughput forward
would invent traffic. Carry-forward never creates bucket times, so a genuine
cluster-wide gap still reads as a gap, and carried values are counted with
`count: 0` so confidence gating and tooltip sample counts stay honest.

Verified against a live 2-node stage cluster: 960 of 1440 `database-size`
buckets held only one node, and the cadence estimator independently landed on
the same ~90 s interval (horizon ≈ 180 s). Replaying the reported row shape
(960 + 228 rows over 24 h) takes the series minimum from 1 to 283 and the dive
count from 114 to 0.

Test coverage: the existing connections/database-size suites only exercised
uniform `period: 60000` with every node present at every timestamp, so this
class of bug could not surface. Adds staggered-cadence cases, a
`connections/node-drop.json` fixture mirroring the table-size precedent,
horizon-expiry and cluster-wide-gap cases, and a counter-untouched case.

Refs #1576

Co-Authored-By: Claude Opus 5 <[email protected]>
@dawsontoth
dawsontoth requested a review from a team as a code owner July 29, 2026 18:51
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 53.99% 6376 / 11808
🔵 Statements 54.64% 6871 / 12573
🔵 Functions 45.63% 1549 / 3394
🔵 Branches 47.56% 4369 / 9186
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/features/instance/status/analytics/__tests__/fixtures/connections/node-drop.json 0% 0% 0% 0%
src/features/instance/status/analytics/components/TimeRangePicker.tsx 90.9% 35.71% 85.71% 90.9% 90
src/features/instance/status/analytics/context/timePresets.ts 100% 100% 100% 100%
src/features/instance/status/analytics/lib/csvExport.ts 76.13% 64.93% 100% 76.92% 46, 230, 233-291
src/features/instance/status/analytics/pipeline/carryForward.ts 100% 100% 100% 100%
src/features/instance/status/analytics/pipeline/connection.tsx 93.75% 75.6% 100% 97.5% 58, 131, 137
src/features/instance/status/analytics/pipeline/downsample.ts 97.95% 95.12% 100% 100% 113
src/features/instance/status/analytics/pipeline/main-thread-utilization.tsx 80% 62.5% 75% 86.66% 119-120
src/features/instance/status/analytics/pipeline/memory.tsx 100% 83.33% 100% 100%
src/features/instance/status/analytics/pipeline/pipeline.ts 91.47% 83.07% 100% 93.16% 164-167, 292-293, 296, 367-375, 379, 411, 465-467, 584
src/features/instance/status/analytics/pipeline/derived/error-rate.ts 96.55% 95.45% 100% 100% 21
src/features/instance/status/analytics/pipeline/derived/mqtt-traffic.tsx 100% 83.33% 100% 100%
src/features/instance/status/analytics/pipeline/derived/request-rate.tsx 93.54% 76% 80% 98.21% 21, 39, 63, 95
src/features/instance/status/analytics/primitives/DimensionSelectorRenderer.tsx 95.12% 84.78% 100% 100% 75, 102
src/features/instance/status/analytics/primitives/MetricRenderer.tsx 66.66% 58.33% 88.88% 66.66% 24-37, 87-90, 169, 193-202, 204-214, 216-222, 233
src/features/instance/status/analytics/primitives/PerPathRateRenderer.tsx 100% 94.11% 100% 100%
src/features/instance/status/analytics/primitives/StackedAreaChart.tsx 95.83% 75.86% 100% 100% 37
src/features/instance/status/analytics/primitives/TrafficByTypeRenderer.tsx 96.22% 88.88% 100% 100% 83, 84
src/features/instance/status/analytics/primitives/mergeStackedRows.ts 100% 100% 100% 100%
src/features/instance/status/analytics/tabs/kpi/useKpiTileData.ts 95.23% 90% 100% 100% 70
Generated in workflow #1653 for commit 51d878b by the Vitest Coverage Report Action

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a bounded last-observation-carry-forward (LOCF) mechanism for cross-node gauge sums (such as connections and database size) in the analytics pipeline. This directly addresses an issue where a single node's absence from a time bucket caused the cluster total to dive incorrectly. The implementation estimates each node's emission cadence using median gaps to determine a staleness horizon. Reviewer feedback highlights two valuable improvement opportunities in pipeline.ts: first, using optional chaining instead of non-null assertions when retrieving and iterating over observationTimes to avoid potential type-checking issues; second, deleting stale nodes from the lastObserved map once they exceed their horizon to prevent memory leaks and redundant iterations over subsequent time buckets.

Comment thread src/features/instance/status/analytics/pipeline/pipeline.ts Outdated
Comment thread src/features/instance/status/analytics/pipeline/pipeline.ts
Two review notes on the #1576 carry-forward, both in pipeline.ts:

- Replace the `observationTimes!.get(key) ?? []` non-null assertion in the
  Other-bucket merge with an explicit lookup. The assertion was sound —
  `otherObservationTimes` is non-null exactly when `observationTimes` is — but
  the coupling was implicit, and hoisting the lookup makes it checkable.

- Evict a node from `lastObserved` once its observation ages past the staleness
  horizon. Buckets are walked in ascending order, so that observation can only
  get staler; keeping it meant re-testing it against every remaining bucket.
  Bounded by node count rather than bucket count, so not the unbounded leak it
  might look like — but it does accumulate every node id ever seen, which is
  real on a wide window over a cluster whose nodes churn (rolling replacement,
  autoscaling).

Behavior is unchanged. Adds a regression test for the eviction's one live
edge: a node that ages out and later returns must be re-registered rather than
staying permanently excluded. Writing it surfaced a property worth recording —
the horizon comes from the node's median gap across the *whole* window, so a
node with only two samples either side of a long silence has that silence as
its median gap and never ages out at all. The test gives each node a few
cadence-spaced samples first, and says why.

Co-Authored-By: Claude Opus 5 <[email protected]>
Comment thread src/features/instance/status/analytics/pipeline/pipeline.ts Outdated
dawsontoth and others added 2 commits July 30, 2026 08:22
#1576's fix bounded last-observation-carry-forward for cross-node gauge sums in
the pipeline. StackedAreaChart has a second, independent forward-fill at the
render layer, and it was unbounded: it merges every series onto the union of x
positions and carries each series' last-seen value across the positions it did
not report, with no staleness limit.

That second fill is why "Stack by: Node" never showed the reported dives — and,
by the same unboundedness, why an idle node keeps contributing to the stack.
The pipeline's bound cannot reach this case: the renderer remaps the spec's
dimension to 'node', so each series holds exactly one node and is present in
every bucket it has, making pipeline carry-forward a structural no-op. Nothing
but this merge bridges those bands, so the bound has to exist here too.

The fill stays — it is load-bearing. Harper's gauge rows carry `period: 0` and
snap onto the spec's 60 s fallback lattice, which a 90 s emission cadence beats
against, so per-node coverage of the lattice is ragged and a strict merge draws
a shredded stack. What changes is that a band now expires:
`STALENESS_INTERVALS × max(median reporting gap, lattice step)`, plus one
lattice step of slack the pipeline does not need — a primitive only sees times
that have already been snapped, and snapping compresses alternate gaps, so
without the slack a series could expire on the beat pattern the snap induced.
Cadence is derived from the series' own point spacing rather than threaded
through SeriesData: derived metrics and hand-built SeriesData reach this
primitive too, so a threaded field would be absent often enough that this
derivation would remain the live path anyway.

Past the horizon a band fills 0, not null. That mirrors the pipeline (dropping
an expired node from a `crossNode: 'sum'` *is* contributing 0), and it is what a
stacked chart means — bands add, so 0 is the neutral element while a null
punches a hole through the stack and drops every band above it. A cluster-wide
gap still reads as a gap: x positions come only from series that reported, so
when everything stops the rows stop rather than filling with zeros.

Checked every stacked-area panel for regressions. The gauges (connections,
database-size — crossNode 'sum' over 'max'/'last') are the motivating fix. The
additive counters (mqtt-traffic-*, bytes-sent/received — sum/sum) improve too,
for the reason the pipeline excluded them: a missing bucket for a rate series is
no traffic, so expiring to 0 is the honest value where carrying the last
throughput forward invented traffic indefinitely. Observed values are never
rewritten, including a genuine drop to 0, and nothing is ever back-filled before
a series' first point.

Extracted the merge to mergeStackedRows so it is unit-testable — the existing
stacked-area suite shows why: every assertion about rendered geometry there is
`it.skip`, because neither jsdom nor happy-dom lays Recharts out. Also memoized
it in the component, since the chart re-renders on every hover tick.

csvExport keeps exporting raw points with empty cells for gaps. It never shared
this fill, deliberately — the export is the data, not the drawing.

Refs #1576

Co-Authored-By: Claude Opus 5 <[email protected]>
…ings

Review on #1587 noted the carry-forward observation-recording and horizon
build are wasted work when the groupBy dimension is `node`: each dim's buckets
then hold exactly that one node (dimVal and the row's node are the same field),
so the crossNode sum is identity and there is never an absent node to fill.

The suggested `!perNode && isGaugeCrossNodeSum(...)` trims it, but only for the
`perNode && dimensionIsNode` combo — it leaves carry-forward enabled for
`!perNode && dimensionIsNode`, which is exactly the "Stack by: Node" view the
comment cites (that remap runs with perNode=false). Gating on `!dimensionIsNode`
instead skips the bookkeeping in both node-dimension combos, which is the whole
set where it's a no-op.

Output is unchanged — carry-forward over a single-node bucket is identity — so
the connections / database-size / cross-node-gauge-gaps / per-node suites all
stay green; this only stops collecting state the node-dimension path can't use.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@dawsontoth
dawsontoth requested a review from cb1kenobi July 30, 2026 13:22

@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.

Looks good, one comment
🤖 Reviewed with GPT 5.6

Comment thread src/features/instance/status/analytics/pipeline/carryForward.ts Outdated
dawsontoth and others added 8 commits July 30, 2026 13:24
Review on #1587: the median cadence estimate is only robust while short,
normal-cadence gaps are the majority, and two real shapes break that —

  - a node observed just twice, 12 h apart, reads as a 12 h "cadence", so
    2 x that is a 24 h horizon;
  - three samples with one normal gap and one outage average the two into a
    multi-hour median.

Either turns bounded staleness into carry-forever: after one brief nonzero
observation the node's stale count is summed into every bucket another node
supplies for the rest of the window, which hides exactly what this feature must
not hide — a genuine transition to zero, or a node dropping out. I had found
this edge while writing the earlier eviction test and only documented it;
kriszyp is right that documenting isn't enough.

Adds MAX_STALENESS_MS = 5 min (Prometheus' fixed lookback, which this module
already cites) as an absolute ceiling, applied to the *cadence estimate*:

    horizon = STALENESS_INTERVALS * max(min(cadence, MAX/INTERVALS), floorMs)

Capping the cadence rather than the horizon keeps the floor meaningful: on a
spec whose own bucket is coarser than 5 min, spanning two buckets still matters
more than the backstop, so the floor wins there. Net guarantee is now
horizon <= max(MAX_STALENESS_MS, 2 x floorMs). The #1576 case is unchanged
(90 s cadence -> 180 s horizon, well under the cap).

Regressions, both verified to fail without the cap:
  - the pipeline-level case kriszyp asked for — two widely separated
    observations followed by regular buckets from another node. Uncapped, 714
    tail buckets read 288 (BUSY+QUIET) instead of 285; capped, the sparse node
    is dropped ~5 min after each observation and the tail is clean.
  - unit coverage for the 12 h-apart and one-gap-one-outage shapes, the
    floor-beats-cap interaction, and an invariant that no observation shape can
    exceed max(MAX_STALENESS_MS, 2 x floor).

Also updates the existing 'scales each node by its own observed cadence' case,
whose 10-minute-cadence node asserted the old uncapped 20-minute horizon.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
The Status tab's time presets each declare the densest bucket Harper should
serve (1h→1m, 6h→1m, 24h→5m, 7d→15m, 30d→1h), but that number only ever
reached the server as the `bucket_ms` hint. Client-side, the pipeline snapped
every record onto `spec.bucket.fallbackMs` — 60 s regardless of the selected
window. Builds that ignore `bucket_ms` (harper-pro 5.1.22) return rows at raw
emission cadence, so the rendered density was:

    preset  intended   rendered   ratio
    1h            60         60      1x
    6h           360        360      1x
    24h          288      1,440      5x
    7d           672     10,080     15x
    30d          720     43,200     60x

At 30 d the Storage tab drew ~43k points x 5 database bands, roughly 216k SVG
path coordinates in a single chart.

Adds an opt-in `downsampleToWindow` pass that folds finished series onto the
lattice the requested window implies (`targetBucketMs`, read out of the same
preset table by duration so there is one source of truth). Measured on a live
2-node stage cluster at 7 d: each band's path `d` attribute drops from ~110 KB
to ~8 KB.

Why a post-aggregation fold rather than simply widening `snapToBucketTime`:
widening the *record* lattice changes what the temporal aggregator sees, and
for a `rate`-transformed field with `temporal: 'sum'` it then sums per-second
rates belonging to different periods. Measured directly — a steady 1000 B/s
became 3000 B/s on a 5 min lattice, and it would have been 60x at 30 d,
silently inflating bytes-sent/received, both mqtt-traffic-* panels and
fsWrite. Folding after every aggregation pass leaves temporal, cross-node and
the bounded carry-forward from #1576 operating on exactly the buckets they do
today. `render-density.test.ts` pins the counters at 1000 B/s across 24h/7d/30d
so this can't regress.

Per-aggregator fold rules (downsample.ts): rate fields collapse with `mean`,
never `sum`; percentiles collapse with `max`, since a p95-of-p95s is not a p95
and an operator would rather keep spikes than smooth them away at 30 d; gauges
keep `max`/`last`; count-weighted-mean stays count-weighted via
`SeriesPoint.count`. Counts are summed so confidence gating still sees the real
observation total, and an all-null coarse bucket stays an explicit null gap.

Opted in at the 18 chart-render and CSV-export call sites (so a download
matches the chart it came from). KPI tiles deliberately opt out: they collapse
to a headline number, so there is no density to save, and folding first would
redefine `latestValue` from "the newest bucket" to "an average over the newest
coarse bucket".

`error-rate`, `request-rate` and `transaction-log-growth` assemble series from
raw columns without going through runPipeline, so MetricRenderer and csvExport
fold their output too via `downsampleDerivedSeriesData` — otherwise those
panels would stay dense next to coarsened neighbours on the same tab. The fold
is idempotent, so applying it to mqtt-traffic's recompute (which opts in
internally) is a no-op rather than a double average. `DerivedMetricSpec` gains
an optional `downsampleAggregator`, defaulting to `mean`, which is correct for
all three shipped derived metrics.

One behavioral note: `runPipeline` previously ignored its `window` argument
entirely, so callers could pass anything. It is now load-bearing.
bytes-sent-tolerance's `0 → MAX_SAFE_INTEGER` sentinel read as a ~285-century
window and folded the series into one bucket; it now passes the fixture's own
span. `recomputeRequestRate` uses the same sentinel but never delegates to
runPipeline, so it is unaffected.

This does NOT reduce server load — we already send `bucket_ms` and the server
ignores it. At 7 d the analytics requests still return tens of thousands of
rows and were observed hitting the 60 s axios timeout, and getAnalytics'
`MAX_ROWS = 50_000` tail-keep can silently truncate the oldest part of a wide
window. Those need core to honor `bucket_ms`; worth a companion core issue.

Co-Authored-By: Claude Opus 5 <[email protected]>
Follow-up on the render-lattice change in this branch: the targets it shipped
were still too dense for a panel-sized chart. Recalibrates the preset table and
adds a second, finer bucket for the expand dialog.

                panel bucket   panel pts        expanded bucket   expanded pts
    1h          1m (unchanged)        60        1m                          60
    6h          1m -> 2m       360 -> 180       1m                         360
    24h         5m -> 10m      288 -> 144       5m                         288
    7d          15m -> 1h      672 -> 168       15m                        672
    30d         1h -> 4h       720 -> 180       1h                         720

Panel counts now land inside a documented 100-200 target: above ~200 the line
is denser than the panel has pixels, so the extra samples cost payload and
render time and buy nothing. 1h is the one deliberate undershoot at 60 — Harper
aggregates on a 60 s period, so there is no finer data to ask for. Encoded as
`PANEL_POINT_TARGET` with a test per preset rather than left as a comment, so
the next person retuning the table gets told when they leave the band.

The expanded column is what the panel column used to be. The expand dialog is a
~95vw canvas with roughly 4x the horizontal room, so it can carry 2-4x the
detail; `TimePreset.expandedBucketMs` states that per preset instead of
deriving it from a divisor, which keeps every lattice a round number (a
computed 10m/3 would put ticks on 200-second boundaries).

`fillParent` already flowed from ChartExpandButton down to every chart render
path, so plumbing this is just forwarding it as `expanded` into
`RunPipelineOptions` and adding it to the affected useMemo dependency arrays —
without that last part, expanding a chart would keep the panel-resolution memo.

CSV export deliberately stays at panel resolution even when triggered from
inside the dialog: an exported file should be a stable artifact of
(metric, window), not of transient dialog state.

Also loosens a too-strict assertion in render-density: 6h at 2m buckets is now
exactly 2x the 60 s fallback lattice, so `before > intended * 2` no longer
holds. Asserts the property that actually matters — any preset coarser than the
60 s fallback comes out strictly shorter — and keeps the 1h identity check.

Co-Authored-By: Claude Opus 5 <[email protected]>
`downsamplePoints` short-circuited on `byBucket.size === points.length`,
returning the input array on the assumption that a one-to-one point-to-bucket
mapping means the points already sit on the target lattice. It doesn't: points
can map one-to-one and still every one of them move.

Records carrying a real 90 s `period` snap onto a 90 s lattice, and folding that
onto the 1 h preset's 60 s target sends k*90_000 to round(1.5k)*60_000 —
0, 120_000, 180_000, 300_000, 360_000 … five distinct buckets from five points,
but only the even-k ones land where they started. The early return left the
series off the target grid, which breaks the StorageTab trend's
`syncMethod="value"` crosshair match against the metric panels (#1514).

Tracks whether any point actually moved and keeps the fast path only when none
did — which still covers the case it was written for, the 1 h preset whose
target equals the snap lattice, including its referential-identity guarantee.

Worth noting when this becomes reachable: today Harper stamps `period: 0` on
these rows, so the snap lattice is the spec's 60 s fallback and coincides with
the 1 h target. It bites as soon as core starts stamping a real period, which is
exactly what HarperFast/harper#1997 asks for — so the latent version of this
would have shipped and then broken on a server upgrade.

Regression test pins the 90 s -> 60 s case; it fails on the old guard with
`[0, 90000, 180000, 270000, 360000]` instead of the aligned lattice.

Co-Authored-By: Claude Opus 5 <[email protected]>
…hen folded

Review on #1588 flagged that the derived-metric downsample doesn't reach the
Requests-tab rate charts. Two distinct issues, and the wiring is slightly
different from the report:

- request-rate has a custom Renderer (PerPathRateRenderer), so it takes
  MetricRenderer's `if (derived.Renderer)` branch and never hit the derived
  fold — its chart stayed at one point per 60 s at 7 d/30 d next to panels
  capped at ~180. Fixed by folding inside PerPathRateRenderer's data memo
  (same shape TrafficByTypeRenderer uses), keyed off the window via
  targetBucketMs. req/s is a rate over equal-width buckets, so the default
  'mean' fold is correct; fillParent is now forwarded so the expand dialog
  folds at the finer expanded resolution too.

- error-rate, contrary to the review, has NO custom Renderer — it takes the
  `else` branch and WAS already folded on screen, but with the default 'mean'.
  Since it's a ratio (1 − Σtotal/Σcount), a plain mean of per-bucket ratios is
  the ratio-of-ratios bug its own docstring warns against, and it was hitting
  the chart, not just CSV. Setting `downsampleAggregator: 'count-weighted-mean'`
  on the spec fixes both the chart and the CSV at once (both read it): since
  each point carries count=sumCount and ratio·count = errors, count-weighting
  reconstructs Σerrors/Σcount.

Tests: error-rate-downsample locks the Σ-correct fold (two disparate-volume
buckets → ≈0.0188, not the 0.455 mean-of-ratios, with a guard showing 'mean'
would reproduce it) and pins the spec's aggregator. per-path-rate-renderer
renders PerPathRateRenderer with a mocked LineChart and asserts a dense series
is folded to the window resolution, and left raw when no window is supplied —
the MetricRenderer/PerPathRateRenderer coverage the review noted was missing.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Two clarity fixes to the Status toolbar, both about the picker being read as
something it isn't.

1. Auto-refresh interval + refresh-now are now one bordered control instead of
   a bare Select sitting beside the range picker at the same gap and weight.
   Standalone, "60s" read as a second *time* setting — a chart granularity —
   rather than "re-fetch every 60s". Wrapping it with the refresh icon (shared
   border/background on the group; children drop their own) is what says the
   interval belongs to the refresh action. Row gap goes 2 -> 3 so the grouping
   reads by spacing too. Adds role="group" + an "Auto-refresh interval" label
   on the previously-unlabeled interval Select.

2. Each range option (and the collapsed trigger) now shows the resolution it
   renders at as a second line — "Last 24 hours" / "by 10 minutes". After the
   #1588 recalibration the bucket is the actual rendered resolution (the server
   ignores our bucket_ms, so the client folds to targetBucketMs == the preset's
   bucketMs), so surfacing it tells the operator what they're actually looking
   at. New `formatBucketLabel` in timePresets renders it; a shared `RangeLabel`
   keeps trigger and options identical. Trigger goes h-auto/min-h-9 so the two
   lines aren't clipped, with explicit SelectValue children (not a bare
   <SelectValue/>, which would mirror the option subtree into the trigger).

Verified in light and dark by injecting the exact markup with the real
compiled CSS (the preview browser isn't signed in, so the live authenticated
toolbar wasn't reachable and I did not sign in): two-line trigger doesn't clip,
the refresh group reads as one joined control, options carry the "by X" line.

Stacked on #1588 (claude/status-bucket-density) — the resolution labels only
mean the recalibrated bucket sizes that PR introduces, so this must not merge
before it.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Mirror the range picker's two-line treatment on the auto-refresh interval:
the interval value on top ("Off" / "30s" / "60s" / "5m"), a muted "reload"
beneath. Standalone the value still read like another time setting; the
sub-label names what the number does, matching the "Last 24 hours / by 10
minutes" pattern beside it.

Only the collapsed trigger gets the second line — the dropdown options stay
single-line, because "reload" on every option is noise (unlike the range
picker, where each option's bucket differs and the second line is per-option
information).

Knock-on layout: the two-line interval makes the refresh group two lines tall,
so it now matches the range picker's height (both 41px) instead of being
shorter. The group switches to items-stretch and the refresh button to
h-auto/w-9 so it fills that height and the joined control stays flush; the
divider gains a small vertical margin so it doesn't touch the rounded corners.

Verified live (the app was signed in this time): interval shows "Off/reload",
range shows "Last 6 hours/by 2 minutes", the two controls are the same height,
the dropdown still opens with single-line options, and the console is clean.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Review on #1590 flagged that <SelectValue> ignores its children and reflects
the selected item's plain text, which would collapse the two-line trigger to
"Last 24 hoursby 10 minutes". That symptom doesn't reproduce on this repo's
Radix version — the live trigger's innerHTML is the full styled two-span stack,
and the user's screenshot shows it rendering correctly — but the reviewer's fix
is the better structure regardless: drive the collapsed value from the
`presetId` / `refreshMs` props by rendering RangeLabel (and the interval's
two-line span) directly inside <SelectTrigger>, instead of relying on how a
given Radix version treats <SelectValue> children. Removes the dependency on
version-specific behavior and the trigger's `*:data-[slot=select-value]:truncate`
interaction; drops the now-unused SelectValue import.

Also scopes the resolution test to the collapsed trigger (first combobox)
rather than a document-wide getByText, which would otherwise also match the
option in the dropdown portal and not prove the trigger itself shows it.

Verified live: both triggers still render two lines (no data-slot=select-value
now), and the dropdown still opens with all five two-line options.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@dawsontoth
dawsontoth enabled auto-merge July 30, 2026 21:21
@dawsontoth
dawsontoth disabled auto-merge July 30, 2026 21:22
@dawsontoth
dawsontoth added this pull request to the merge queue Jul 30, 2026
Merged via the queue into stage with commit a684ff9 Jul 30, 2026
2 checks passed
@dawsontoth
dawsontoth deleted the claude/mqtt-status-chart-gaps-ace583 branch July 30, 2026 21:29
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.

Connections chart dives to ~0 when only one node reports in a time bucket (cross-node sum treats absent node as zero)

3 participants