diff --git a/src/engine.rs b/src/engine.rs index 02c1a83..003b7f1 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -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, + end_ms: Option, +) -> 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, Vec>> = 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 diff --git a/tickets/057-rate-line-chart-x-axis-collapses-to-sparse-occurrence-window.md b/tickets/057-rate-line-chart-x-axis-collapses-to-sparse-occurrence-window.md deleted file mode 100644 index 323f458..0000000 --- a/tickets/057-rate-line-chart-x-axis-collapses-to-sparse-occurrence-window.md +++ /dev/null @@ -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 - `` (`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 ``, 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. diff --git a/webapp/src/components/Dashboards.tsx b/webapp/src/components/Dashboards.tsx index 194afb0..bd7cb64 100644 --- a/webapp/src/components/Dashboards.tsx +++ b/webapp/src/components/Dashboards.tsx @@ -96,7 +96,7 @@ function PanelPreview({ panel, range }: { panel: Panel; range: TimeRange | null if (loading && series.length === 0) { return
Loading…
} - return + return } function AutosaveStatus({ status, error }: { status: SaveStatus; error: string | null }) { diff --git a/webapp/src/components/PanelChart.tsx b/webapp/src/components/PanelChart.tsx index 4a12f25..070e6db 100644 --- a/webapp/src/components/PanelChart.tsx +++ b/webapp/src/components/PanelChart.tsx @@ -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 { @@ -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
No data in range.
} 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 (