Implement ticket 057: fix rate() line charts for sparse/bursty occurrences
A rate() series over a metric whose occurrences are confined to a short burst within a much wider queried range (e.g. sum by (code) (rate(KeyPressed[5m])) against a ~10h log where all key presses happen in the first 10s) rendered as broken: the line simply stopped at the last real occurrence, and the chart's x-axis auto-scaled to the tiny span of actual data points rather than the queried range, silently narrowing a 10-hour view down to ~10 seconds with no indication anything was off. Backend (src/engine.rs): windowed_rate_series now appends a synthetic boundary point at start_ms/end_ms (computed via the existing rate::rate_at against the original occurrence points, so a boundary inside a still-warm window gets its true partial value rather than a hardcoded 0) whenever the real series doesn't already reach that edge. Wired into both the bare rate() and sum()/avg(rate()) evaluate_promql branches, which already had start_ms/end_ms in scope. Frontend (PanelChart.tsx/Dashboards.tsx): the line chart's XAxis now uses an explicit numeric domain pinned to the panel's queried range (passed down from Dashboards.tsx's PanelPreview) instead of recharts' default auto-scale-to-data-extent, so sparse data can no longer make the axis look narrower than what was actually queried. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XFUNZwCvz3kcod3352ZR2o
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
5493358db8
commit
79d252c1df
+105
-6
@@ -1615,13 +1615,41 @@ fn project_labels(
|
||||
/// just converts at the boundary and delegates — the actual window-scan logic lives only in
|
||||
/// `crate::rate::windowed_rate_series`, also used (via `crate::rate::RateWindow`) by alert
|
||||
/// `Threshold`/`Ratio` conditions in `crate::alerts`, so both agree on what "windowed rate" means.
|
||||
fn windowed_rate_series(points: &[(i64, f64)], range_ms: i64) -> Vec<(i64, f64)> {
|
||||
let points: Vec<(Timestamp, f64)> =
|
||||
///
|
||||
/// Ticket 057: `rate::windowed_rate_series` only evaluates *at each occurrence's own timestamp*,
|
||||
/// so a metric whose occurrences stop (or start) well inside a much wider queried range produces a
|
||||
/// series that just ends (or begins) at the last (or first) real occurrence — nothing says "the
|
||||
/// rate is 0 here" for the rest of the range. When `start_ms`/`end_ms` are given (a dashboard panel
|
||||
/// always supplies both), this appends a synthetic boundary point at each edge — computed via
|
||||
/// `rate::rate_at` against the original occurrence points, so a boundary that falls inside a
|
||||
/// still-warm window reads its true partial value rather than being forced to 0 — whenever the
|
||||
/// real series doesn't already reach that edge.
|
||||
fn windowed_rate_series(
|
||||
points: &[(i64, f64)],
|
||||
range_ms: i64,
|
||||
start_ms: Option<i64>,
|
||||
end_ms: Option<i64>,
|
||||
) -> Vec<(i64, f64)> {
|
||||
let ts_points: Vec<(Timestamp, f64)> =
|
||||
points.iter().map(|(ms, value)| (Timestamp::from_millis(*ms), *value)).collect();
|
||||
rate::windowed_rate_series(&points, range_ms)
|
||||
let mut series: Vec<(i64, f64)> = rate::windowed_rate_series(&ts_points, range_ms)
|
||||
.into_iter()
|
||||
.map(|(ts, value)| (ts.as_millis(), value))
|
||||
.collect()
|
||||
.collect();
|
||||
|
||||
if let Some(end) = end_ms {
|
||||
if series.last().is_none_or(|(ts, _)| *ts < end) {
|
||||
let value = rate::rate_at(&ts_points, Timestamp::from_millis(end), range_ms);
|
||||
series.push((end, value));
|
||||
}
|
||||
}
|
||||
if let Some(start) = start_ms {
|
||||
if series.first().is_none_or(|(ts, _)| *ts > start) {
|
||||
let value = rate::rate_at(&ts_points, Timestamp::from_millis(start), range_ms);
|
||||
series.insert(0, (start, value));
|
||||
}
|
||||
}
|
||||
series
|
||||
}
|
||||
|
||||
/// Fixed window width (ms) used by `merge_series_windowed` to combine multiple raw-occurrence
|
||||
@@ -1857,7 +1885,7 @@ fn evaluate_promql(
|
||||
let mut groups: BTreeMap<BTreeMap<String, String>, Vec<Vec<(i64, f64)>>> = BTreeMap::new();
|
||||
for (labels, points) in series_points {
|
||||
let points = match rate_range_ms {
|
||||
Some(range_ms) => windowed_rate_series(&points, range_ms),
|
||||
Some(range_ms) => windowed_rate_series(&points, range_ms, start_ms, end_ms),
|
||||
None => points,
|
||||
};
|
||||
let group_key = project_labels(&labels, &parsed.by_labels);
|
||||
@@ -1883,7 +1911,7 @@ fn evaluate_promql(
|
||||
.into_iter()
|
||||
.map(|(labels, points)| PromqlSeriesDto {
|
||||
labels,
|
||||
points: windowed_rate_series(&points, range_ms),
|
||||
points: windowed_rate_series(&points, range_ms, start_ms, end_ms),
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
@@ -3052,6 +3080,77 @@ mod tests {
|
||||
assert_eq!(series[1].points, vec![(0, 0.5)]);
|
||||
}
|
||||
|
||||
/// Ticket 057: reproduces the reported `sum by (code) (rate(KeyPressed[5m]))` chart — a burst
|
||||
/// of occurrences confined to the very start of a much wider queried range. Without a
|
||||
/// synthetic trailing point, the series would just stop at the last occurrence's timestamp
|
||||
/// (`1`), giving no indication the rate has decayed to 0 for the rest of `end_ms`.
|
||||
#[test]
|
||||
fn rate_series_gets_a_trailing_zero_point_once_occurrences_stop_early_in_a_wider_range() {
|
||||
let mut index = BTreeMap::new();
|
||||
declare_requests_metric(&mut index);
|
||||
ingest_log(&mut index, 0, "request path=/a");
|
||||
ingest_log(&mut index, 1, "request path=/a");
|
||||
|
||||
let parsed = promql::parse_query("rate(requests[2s])").unwrap();
|
||||
let series = evaluate_promql(&index, &parsed, None, Some(100_000)).unwrap();
|
||||
|
||||
assert_eq!(series.len(), 1);
|
||||
assert_eq!(
|
||||
series[0].points,
|
||||
vec![(0, 0.5), (1, 1.0), (100_000, 0.0)],
|
||||
"the window (2s) is long since aged out by end_ms, so the appended boundary point must read 0"
|
||||
);
|
||||
}
|
||||
|
||||
/// Mirror of the trailing case: occurrences starting well after `start_ms` shouldn't make the
|
||||
/// line appear to begin mid-range with no indication the rate was 0 beforehand.
|
||||
#[test]
|
||||
fn rate_series_gets_a_leading_zero_point_when_occurrences_start_late_in_a_wider_range() {
|
||||
let mut index = BTreeMap::new();
|
||||
declare_requests_metric(&mut index);
|
||||
ingest_log(&mut index, 50_000, "request path=/a");
|
||||
|
||||
let parsed = promql::parse_query("rate(requests[2s])").unwrap();
|
||||
let series = evaluate_promql(&index, &parsed, Some(0), None).unwrap();
|
||||
|
||||
assert_eq!(series[0].points, vec![(0, 0.0), (50_000, 0.5)]);
|
||||
}
|
||||
|
||||
/// Same shape as the two tests above, but through `sum(rate(...)) by (label)` — the exact
|
||||
/// query form from the ticket's report — so each label's line independently reflects
|
||||
/// rate-zero for the remainder of the range instead of just its own group stopping early.
|
||||
#[test]
|
||||
fn sum_by_rate_gets_boundary_points_per_group_across_a_wider_range() {
|
||||
let mut index = BTreeMap::new();
|
||||
declare_requests_metric(&mut index);
|
||||
ingest_log(&mut index, 0, "request path=/a");
|
||||
ingest_log(&mut index, 1, "request path=/a");
|
||||
ingest_log(&mut index, 0, "request path=/b");
|
||||
|
||||
let parsed = promql::parse_query("sum(rate(requests[2s])) by (path)").unwrap();
|
||||
let mut series = evaluate_promql(&index, &parsed, None, Some(100_000)).unwrap();
|
||||
series.sort_by(|a, b| a.labels.get("path").cmp(&b.labels.get("path")));
|
||||
|
||||
assert_eq!(series.len(), 2);
|
||||
assert_eq!(series[0].points, vec![(0, 0.5), (1, 1.0), (100_000, 0.0)]);
|
||||
assert_eq!(series[1].points, vec![(0, 0.5), (100_000, 0.0)]);
|
||||
}
|
||||
|
||||
/// A boundary point is only appended when the real series doesn't already reach that edge —
|
||||
/// continuously-occurring metrics (the common case) must render exactly as before.
|
||||
#[test]
|
||||
fn rate_series_skips_boundary_points_already_covered_by_real_data() {
|
||||
let mut index = BTreeMap::new();
|
||||
declare_requests_metric(&mut index);
|
||||
ingest_log(&mut index, 0, "request path=/a");
|
||||
ingest_log(&mut index, 1000, "request path=/a");
|
||||
|
||||
let parsed = promql::parse_query("rate(requests[2s])").unwrap();
|
||||
let series = evaluate_promql(&index, &parsed, Some(0), Some(1000)).unwrap();
|
||||
|
||||
assert_eq!(series[0].points, vec![(0, 0.5), (1000, 1.0)]);
|
||||
}
|
||||
|
||||
/// Ticket 040 acceptance criterion: a dashboard `rate()` chart and an alert `Threshold`
|
||||
/// condition over the same metric/window agree on the computed rate for the same historical
|
||||
/// data, since both now go through the single shared implementation in `crate::rate` rather
|
||||
|
||||
@@ -1,97 +0,0 @@
|
||||
# 057 — Line chart for a rate() over sparse/bursty occurrences collapses its x-axis to the data's own tiny time span
|
||||
|
||||
## Summary
|
||||
|
||||
Reported symptom: the dashboard line chart for `sum by (code) (rate(KeyPressed[5m]))`
|
||||
looks bad/unreadable against a real capture
|
||||
(`ottyl --file .../sim-world.log`, a ~10-hour log where `KeyPressed` is a
|
||||
user-declared metric matching `msg="key_event"` lines, labeled by
|
||||
`code`/`kind`/`msg` — see the profile's metric declaration in
|
||||
`~/Library/Application Support/ottyl/settings.json`). In the sample log
|
||||
all 20 `key_event` lines (16 `Left`, 3 `Up`, 1 `Down`, all `kind="Press"`)
|
||||
occur within the first ~10 seconds of ingestion; there are zero further
|
||||
key presses across the remaining ~9h55m of the log.
|
||||
|
||||
Two compounding causes, both reproducible from this shape of data:
|
||||
|
||||
1. **`rate()` never emits a trailing zero once occurrences stop.**
|
||||
`crate::rate::windowed_rate_series` (`src/rate.rs:40-45`) evaluates
|
||||
`rate_at` only *at each existing point's own timestamp* — there is no
|
||||
synthetic point emitted at the query's `end_ms` (or at "now") once the
|
||||
trailing window has aged out. `evaluate_promql`'s `Sum`/`Avg` branch
|
||||
(`src/engine.rs:1621-1648`) calls this per group then merges with
|
||||
`merge_series_locf` (`src/engine.rs:1508-1529`), which sweeps only the
|
||||
*union of each sub-series' own timestamps* — again nothing beyond the
|
||||
last real occurrence. So a series for `code="Left"` literally ends at
|
||||
the timestamp of the last `Left` press; there is no data point saying
|
||||
"rate is now 0" for the following ~9h55m.
|
||||
2. **The frontend's line chart x-axis auto-scales to whatever timestamps
|
||||
happen to be in the returned series, not to the queried time range.**
|
||||
`PanelChart.tsx`'s `toLineData` (`webapp/src/components/PanelChart.tsx:60-77`)
|
||||
builds `rows` only from timestamps present in `series[].points`; the
|
||||
`<XAxis dataKey="timestamp_ms" .../>` (`PanelChart.tsx:105-110`) has no
|
||||
explicit `domain`, so Recharts defaults to `['auto','auto']` computed
|
||||
from `rows` alone. Combined with (1), a panel queried over "last 6
|
||||
hours" (or any range spanning the whole log) that only has data in the
|
||||
first 10 seconds silently renders an x-axis spanning just those 10
|
||||
seconds — not the selected range — with no visual indication the axis
|
||||
has been narrowed or that activity stopped. The result is a cramped,
|
||||
jagged little chart (peak rate ≈ 16 presses / 300s ≈ 0.053/s, so also
|
||||
visually flat/near-zero) that looks broken rather than showing "a
|
||||
burst, then nothing."
|
||||
|
||||
The backend range filter (`start_ms`/`end_ms` in `evaluate_promql`,
|
||||
`src/engine.rs:1565-1584`) is applied correctly to which raw occurrences
|
||||
are considered — this is not a filtering bug, it's a rendering/shape
|
||||
problem in what gets returned and how it's charted.
|
||||
|
||||
## Scope
|
||||
|
||||
- Decide and implement a way for the charted line to reflect that the
|
||||
rate has decayed to (and stayed at) zero, rather than simply stopping.
|
||||
Options to weigh (pick one, document the choice in the PR):
|
||||
- Emit a synthetic trailing point at `end_ms` (or "now" for a live
|
||||
range) per group whenever the last real point's window has fully
|
||||
aged out and its rate would read 0 — cheapest fix, keeps all
|
||||
resampling logic where it already lives in `evaluate_promql`.
|
||||
- Resample `rate()`/`sum(rate())`/`avg(rate())` series onto a regular
|
||||
step grid across `[start_ms, end_ms]` (à la Prometheus's `step`)
|
||||
instead of only at occurrence timestamps — bigger change, but fixes
|
||||
the same problem for gauges/counters with irregular sampling more
|
||||
generally; note `merge_series_windowed` (`src/engine.rs:1484-1503`)
|
||||
already buckets on a fixed `MERGE_WINDOW_MS` grid for the
|
||||
non-rate `sum()`/`avg()` path, which is closer to this shape already.
|
||||
- Whichever approach is chosen, make `PanelChart.tsx`'s line chart honor
|
||||
the panel's actual queried range for the x-axis domain (pass
|
||||
`range.startMs`/`range.endMs` down from `Dashboards.tsx`'s
|
||||
`PanelPreview` into `PanelChart` and set `<XAxis domain={[startMs,
|
||||
endMs]} type="number" .../>`, or equivalent) so a chart with sparse
|
||||
data doesn't silently narrow the visible window — this is worth doing
|
||||
regardless of which backend option above is chosen, since it's the
|
||||
more directly misleading half of the bug.
|
||||
- Add a fixture reproducing this shape (a metric with occurrences
|
||||
clustered in a short burst near the start of a much longer ingested
|
||||
window) and a test on `evaluate_promql`/`windowed_rate_series` (or
|
||||
wherever the chosen fix lives) asserting the returned series covers
|
||||
the full requested range, not just the burst.
|
||||
- Out of scope: changing how `KeyPressed`-style occurrence metrics are
|
||||
declared, or adding a dedicated "gauge decay" concept — this is about
|
||||
the query range/x-axis mismatch specifically.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- Querying `sum by (code) (rate(KeyPressed[5m]))` (or an equivalent
|
||||
fixture) over a time range much wider than the span of actual
|
||||
occurrences returns a series (or is charted) that visibly reflects
|
||||
rate-zero for the remainder of the range, rather than a line that
|
||||
simply stops.
|
||||
- `PanelChart`'s line chart x-axis reflects the panel's selected/queried
|
||||
time range, not just the min/max timestamp actually present in the
|
||||
returned series — verified by a case where returned data covers only
|
||||
a fraction of the selected range.
|
||||
- Existing `evaluate_promql`/`rate.rs`/`PanelChart` tests for
|
||||
regularly-sampled or continuously-occurring metrics are unaffected.
|
||||
|
||||
## Depends on
|
||||
|
||||
None.
|
||||
@@ -96,7 +96,7 @@ function PanelPreview({ panel, range }: { panel: Panel; range: TimeRange | null
|
||||
if (loading && series.length === 0) {
|
||||
return <div className="flex h-48 items-center justify-center text-xs text-text-faint">Loading…</div>
|
||||
}
|
||||
return <PanelChart series={series} chartType={panel.chart_type} />
|
||||
return <PanelChart series={series} chartType={panel.chart_type} range={range} />
|
||||
}
|
||||
|
||||
function AutosaveStatus({ status, error }: { status: SaveStatus; error: string | null }) {
|
||||
|
||||
@@ -14,6 +14,7 @@ import {
|
||||
YAxis,
|
||||
} from 'recharts'
|
||||
import type { ChartType, PromqlSeries } from '../lib/api'
|
||||
import type { TimeRange } from './LogTimeline'
|
||||
|
||||
/// Renders a label combination as a series key/legend string, e.g. `{"path":"/a"}` -> `path=/a`; no labels renders as `value`.
|
||||
export function seriesLabel(labels: Record<string, string>): string {
|
||||
@@ -91,19 +92,36 @@ function sumValue(series: PromqlSeries): number {
|
||||
return series.points.reduce((total, [, value]) => total + value, 0)
|
||||
}
|
||||
|
||||
export function PanelChart({ series, chartType }: { series: PromqlSeries[]; chartType: ChartType }) {
|
||||
export function PanelChart({
|
||||
series,
|
||||
chartType,
|
||||
range,
|
||||
}: {
|
||||
series: PromqlSeries[]
|
||||
chartType: ChartType
|
||||
range?: TimeRange | null
|
||||
}) {
|
||||
if (series.length === 0) {
|
||||
return <div className="flex h-48 items-center justify-center text-xs text-text-faint">No data in range.</div>
|
||||
}
|
||||
|
||||
if (chartType === 'line') {
|
||||
const { rows, keys } = toLineData(series)
|
||||
// Ticket 057: without an explicit numeric domain, recharts auto-scales the x-axis to the
|
||||
// min/max timestamp actually present in `rows` — so a metric whose occurrences only cover a
|
||||
// sliver of the queried range (a burst followed by silence) silently renders as if that
|
||||
// sliver *were* the full range, with no visual sign the axis got narrowed. Pinning the domain
|
||||
// to the panel's actual queried range keeps the axis honest; `type="number"` is required for
|
||||
// a `domain` tuple to be interpreted as a value range rather than a category filter.
|
||||
return (
|
||||
<ResponsiveContainer width="100%" height={220}>
|
||||
<LineChart data={rows} margin={{ top: 8, right: 16, left: 0, bottom: 0 }}>
|
||||
<CartesianGrid strokeDasharray="3 3" stroke="var(--c-border)" />
|
||||
<XAxis
|
||||
dataKey="timestamp_ms"
|
||||
type="number"
|
||||
domain={range ? [range.startMs, range.endMs] : ['dataMin', 'dataMax']}
|
||||
allowDataOverflow
|
||||
tickFormatter={formatTimestamp}
|
||||
stroke="var(--c-text-subtle)"
|
||||
tick={{ fontSize: 11, fill: 'var(--c-text-muted)' }}
|
||||
|
||||
Reference in New Issue
Block a user