From 5aa7c1bae9f2fed32fc8d7b7a6a359996bb82329 Mon Sep 17 00:00:00 2001 From: Hugh Kaznowski Date: Sat, 5 Sep 2026 20:17:50 +0100 Subject: [PATCH] Fix regex help icon click-toggle and add PromQL by()/sum(rate()) grouping RegexHelpIcon previously used a single `open` boolean driven by both hover and click, so clicking to pin the popover open was undone by the next mouseleave. Split into separate `pinned` (click) and `hovering` (mouse-over) states so a click-toggle actually persists until clicked again. The seeded `log_category` dashboard's line panel plotted the bare metric name, since `sum()`/`rate()` always collapsed all label combinations into one series and PromQL syntax had no way to preserve a label dimension through an aggregation. Added `by (label, ...)` grouping and nested `sum(rate(metric[range]))`/`avg(rate(...))` parsing, and made `rate()` compute a real windowed time series instead of a single trailing point, so a query like `sum(rate(log_category[1m])) by (category)` now renders one line per category. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01XFUNZwCvz3kcod3352ZR2o --- src/engine.rs | 192 +++++++++++++++++++++--- src/profile.rs | 10 +- src/promql.rs | 141 ++++++++++++++++- webapp/src/components/RegexHelpIcon.tsx | 21 ++- 4 files changed, 330 insertions(+), 34 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index 30b2d86..71f8144 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1147,7 +1147,82 @@ fn matches_label_matchers( }) } -/// Evaluates a parsed PromQL query against `index`, producing chart-ready series: baseline is one series per label combination; `sum()`/`avg()` merge series by timestamp into one label-less series; `rate()` is simplified to a single point over `(last_ts - range, last_ts]` (an occurrence-count-per-second reading, not PromQL's true sliding-window-per-step) since bucket values are occurrence markers, not cumulative counters. +/// Projects a full label map down to just the names in `by_labels`; an empty `by_labels` collapses +/// everything to the empty map, i.e. "one group total" (the historical `sum()`/`avg()` behavior). +fn project_labels( + labels: &BTreeMap, + by_labels: &[String], +) -> BTreeMap { + by_labels + .iter() + .filter_map(|name| labels.get(name).map(|value| (name.clone(), value.clone()))) + .collect() +} + +/// Turns one series' occurrence points into a windowed rate: at each of the series' own timestamps, +/// counts how many points fall in the trailing `(ts - range_ms, ts]` window and divides by the window +/// width in seconds. Points are occurrence markers rather than cumulative counters, so this reads as +/// "occurrences per second over the trailing window", evaluated at every point instead of just the last. +fn windowed_rate_series(points: &[(i64, f64)], range_ms: i64) -> Vec<(i64, f64)> { + let range_seconds = range_ms as f64 / 1000.0; + points + .iter() + .map(|(ts, _)| { + let window_start = ts - range_ms; + let count_in_window = + points.iter().filter(|(t, _)| *t > window_start && *t <= *ts).count(); + (*ts, count_in_window as f64 / range_seconds) + }) + .collect() +} + +/// Merges several series by exact timestamp match: values sharing a timestamp are summed (or averaged). +/// This is the plain `sum()`/`avg()` behavior — appropriate when the underlying points are occurrence +/// markers rather than a continuous reading, so there's nothing to carry forward between timestamps. +fn merge_series_exact(sub_series: &[Vec<(i64, f64)>], is_avg: bool) -> Vec<(i64, f64)> { + let mut grouped: BTreeMap> = BTreeMap::new(); + for points in sub_series { + for (timestamp_ms, value) in points { + grouped.entry(*timestamp_ms).or_default().push(*value); + } + } + grouped + .into_iter() + .map(|(timestamp_ms, values)| { + let sum: f64 = values.iter().sum(); + let value = if is_avg { sum / values.len() as f64 } else { sum }; + (timestamp_ms, value) + }) + .collect() +} + +/// Merges several already-windowed rate series by sweeping the union of their timestamps, carrying each +/// sub-series' last-known value forward (0.0 before its first point) so every step reflects every +/// sub-series' most recent reading — unlike `merge_series_exact`, which only combines exact-timestamp matches. +fn merge_series_locf(sub_series: &[Vec<(i64, f64)>], is_avg: bool) -> Vec<(i64, f64)> { + let mut all_timestamps: std::collections::BTreeSet = std::collections::BTreeSet::new(); + for points in sub_series { + all_timestamps.extend(points.iter().map(|(ts, _)| *ts)); + } + let mut cursors = vec![0usize; sub_series.len()]; + let mut last_values = vec![0.0f64; sub_series.len()]; + all_timestamps + .into_iter() + .map(|ts| { + for (i, points) in sub_series.iter().enumerate() { + while cursors[i] < points.len() && points[cursors[i]].0 <= ts { + last_values[i] = points[cursors[i]].1; + cursors[i] += 1; + } + } + let sum: f64 = last_values.iter().sum(); + let value = if is_avg { sum / last_values.len() as f64 } else { sum }; + (ts, value) + }) + .collect() +} + +/// Evaluates a parsed PromQL query against `index`, producing chart-ready series: baseline is one series per label combination; `sum()`/`avg()` merge series into one series per `by_labels` group (the whole result collapses to a single label-less series when `by_labels` is empty); `rate()` — bare or wrapped in `sum()`/`avg()` — turns each series into a windowed occurrences-per-second reading via `windowed_rate_series`, so it plots as a real line rather than one trailing point. fn evaluate_promql( index: &BTreeMap, parsed: &PromqlQuery, @@ -1179,36 +1254,42 @@ fn evaluate_promql( .map(|(labels, points)| PromqlSeriesDto { labels, points }) .collect(), Some(AggOp::Sum) | Some(AggOp::Avg) => { - let mut grouped: BTreeMap> = BTreeMap::new(); - for points in series_points.values() { - for (timestamp_ms, value) in points { - grouped.entry(*timestamp_ms).or_default().push(*value); - } - } let is_avg = parsed.agg == Some(AggOp::Avg); - let points = grouped + // Validated by `promql::parse_query`: an inner rate() always carries a range. + let rate_range_ms = parsed + .inner_rate + .then(|| parsed.range.expect("inner rate() always carries a range").as_millis() as i64); + + 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), + None => points, + }; + let group_key = project_labels(&labels, &parsed.by_labels); + groups.entry(group_key).or_default().push(points); + } + + groups .into_iter() - .map(|(timestamp_ms, values)| { - let sum: f64 = values.iter().sum(); - let value = if is_avg { sum / values.len() as f64 } else { sum }; - (timestamp_ms, value) + .map(|(group_labels, sub_series)| { + let points = if rate_range_ms.is_some() { + merge_series_locf(&sub_series, is_avg) + } else { + merge_series_exact(&sub_series, is_avg) + }; + PromqlSeriesDto { labels: group_labels, points } }) - .collect(); - vec![PromqlSeriesDto { labels: BTreeMap::new(), points }] + .collect() } Some(AggOp::Rate) => { // Validated by `promql::parse_query`: `rate()` never parses without one. let range_ms = parsed.range.expect("rate() always carries a range").as_millis() as i64; - let range_seconds = range_ms as f64 / 1000.0; series_points .into_iter() - .filter_map(|(labels, points)| { - let last_ts = points.last()?.0; - let window_start = last_ts - range_ms; - let count_in_window = - points.iter().filter(|(ts, _)| *ts > window_start && *ts <= last_ts).count(); - let rate = count_in_window as f64 / range_seconds; - Some(PromqlSeriesDto { labels, points: vec![(last_ts, rate)] }) + .map(|(labels, points)| PromqlSeriesDto { + labels, + points: windowed_rate_series(&points, range_ms), }) .collect() } @@ -1925,6 +2006,73 @@ mod tests { assert_eq!(series[0].points, vec![(0, 2.0)]); } + #[test] + fn promql_sum_by_preserves_the_named_label_dimension() { + let mut index = BTreeMap::new(); + declare_requests_metric(&mut index); + ingest_log(&mut index, 0, "request path=/a"); + ingest_log(&mut index, 0, "request path=/b"); + ingest_log(&mut index, 1, "request path=/a"); + + let parsed = promql::parse_query("sum(requests) by (path)").unwrap(); + let mut series = evaluate_promql(&index, &parsed, None, None); + series.sort_by(|a, b| a.labels.get("path").cmp(&b.labels.get("path"))); + + assert_eq!(series.len(), 2); + assert_eq!(series[0].labels.get("path").map(String::as_str), Some("/a")); + assert_eq!(series[0].points, vec![(0, 1.0), (1, 1.0)]); + assert_eq!(series[1].labels.get("path").map(String::as_str), Some("/b")); + assert_eq!(series[1].points, vec![(0, 1.0)]); + } + + #[test] + fn bare_rate_produces_a_windowed_point_per_timestamp_not_a_single_point() { + 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, None, None); + + assert_eq!(series.len(), 1); + assert_eq!(series[0].points, vec![(0, 0.5), (1000, 1.0)]); + } + + #[test] + fn sum_of_rate_by_label_gives_each_label_value_its_own_line() { + 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"); + 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, None); + series.sort_by(|a, b| a.labels.get("path").cmp(&b.labels.get("path"))); + + assert_eq!(series.len(), 2); + assert_eq!(series[0].labels.get("path").map(String::as_str), Some("/a")); + assert_eq!(series[0].points, vec![(0, 0.5), (1000, 1.0)]); + assert_eq!(series[1].labels.get("path").map(String::as_str), Some("/b")); + assert_eq!(series[1].points, vec![(0, 0.5)]); + } + + #[test] + fn sum_of_rate_without_by_clause_still_collapses_to_one_series() { + let mut index = BTreeMap::new(); + declare_requests_metric(&mut index); + ingest_log(&mut index, 0, "request path=/a"); + ingest_log(&mut index, 0, "request path=/b"); + + let parsed = promql::parse_query("sum(rate(requests[2s]))").unwrap(); + let series = evaluate_promql(&index, &parsed, None, None); + + assert_eq!(series.len(), 1); + assert!(series[0].labels.is_empty()); + assert_eq!(series[0].points, vec![(0, 1.0)]); + } + #[test] fn promql_range_bounds_filter_points_via_start_end_ms() { let mut index = BTreeMap::new(); diff --git a/src/profile.rs b/src/profile.rs index f681f66..4ef2c81 100644 --- a/src/profile.rs +++ b/src/profile.rs @@ -236,7 +236,7 @@ fn seeded_default_dashboard() -> Dashboard { Panel { id: generate_id("error-volume-line"), title: "Error vs. non-error volume".to_string(), - promql: SEEDED_METRIC_NAME.to_string(), + promql: format!("sum(rate({SEEDED_METRIC_NAME}[1m])) by ({SEEDED_METRIC_LABEL})"), chart_type: ChartType::Line, }, Panel { @@ -793,9 +793,11 @@ mod tests { assert_eq!(default_profile.dashboards.len(), 1); let dashboard = &default_profile.dashboards[0]; assert_eq!(dashboard.panels.len(), 2); - assert!(dashboard.panels.iter().all(|p| p.promql == SEEDED_METRIC_NAME)); - assert!(dashboard.panels.iter().any(|p| p.chart_type == ChartType::Line)); - assert!(dashboard.panels.iter().any(|p| p.chart_type == ChartType::Pie)); + assert!(dashboard + .panels + .iter() + .any(|p| p.chart_type == ChartType::Line && p.promql.contains(SEEDED_METRIC_NAME))); + assert!(dashboard.panels.iter().any(|p| p.chart_type == ChartType::Pie && p.promql == SEEDED_METRIC_NAME)); // Reload (simulating a restart) and ensure_default again: must not re-seed or duplicate. let mut store = ProfileStore::load(&store_path); diff --git a/src/promql.rs b/src/promql.rs index 0e3bfc9..3a2ea0c 100644 --- a/src/promql.rs +++ b/src/promql.rs @@ -1,5 +1,6 @@ //! Hand-rolled parser for a small PromQL subset: metric name + label matchers, an optional `[5m]` range, and a top-level `sum()`/`avg()`/`rate()`/`histogram()`/`gauge()` wrapper. //! Only exact-match (`=`) label matchers are supported; `rate()` requires a duration range selector, while `histogram(metric[width])` reuses that `[...]` slot for a bare bucket width instead. +//! `sum()`/`avg()` may additionally wrap a `rate(...)` call directly (e.g. `sum(rate(requests[5m]))`), and either may be followed by a `by (label, ...)` clause to preserve one series per distinct value of the named labels instead of collapsing to a single series. use std::time::Duration; @@ -29,6 +30,12 @@ pub struct PromqlQuery { pub range: Option, /// Bare-number range selector, e.g. `[10]`; only meaningful for `histogram()`, where it's the bucket width. pub bucket_width: Option, + /// Set when `agg` is `Sum`/`Avg` and it wraps an inner `rate(metric[range])` rather than a bare vector + /// selector, e.g. `sum(rate(requests[5m]))`. `range` above carries the inner `rate()`'s duration in that case. + pub inner_rate: bool, + /// Labels named in a trailing `by (...)` clause; empty means "collapse to a single series" (the historical + /// behavior of `sum()`/`avg()`). When non-empty, one series is kept per distinct combination of these labels. + pub by_labels: Vec, } /// What a `[...]` range selector turned out to hold: a duration, or a bare width. @@ -217,7 +224,51 @@ impl<'a> Parser<'a> { Some(RangeSelector::Duration(d)) => (Some(d), None), Some(RangeSelector::BareWidth(w)) => (None, Some(w)), }; - Ok(PromqlQuery { agg: None, metric_name, matchers, range, bucket_width }) + Ok(PromqlQuery { + agg: None, + metric_name, + matchers, + range, + bucket_width, + inner_rate: false, + by_labels: Vec::new(), + }) + } + + /// Parses a trailing `by (label, ...)` clause, if present; returns an empty vec otherwise. + fn parse_by_clause(&mut self) -> Result, String> { + self.skip_whitespace(); + let checkpoint = self.pos; + let Ok(ident) = self.parse_identifier() else { + self.pos = checkpoint; + return Ok(Vec::new()); + }; + if ident != "by" { + self.pos = checkpoint; + return Ok(Vec::new()); + } + self.skip_whitespace(); + self.expect_char('(')?; + let mut labels = Vec::new(); + self.skip_whitespace(); + if self.peek() == Some(')') { + self.expect_char(')')?; + return Err("by () clause must name at least one label".to_string()); + } + loop { + labels.push(self.parse_identifier()?); + self.skip_whitespace(); + match self.peek() { + Some(',') => { + self.pos += 1; + } + Some(')') => break, + Some(c) => return Err(format!("expected ',' or ')', found '{c}'")), + None => return Err("unterminated by clause (missing ')')".to_string()), + } + } + self.expect_char(')')?; + Ok(labels) } fn parse_query(&mut self) -> Result { @@ -235,9 +286,39 @@ impl<'a> Parser<'a> { self.skip_whitespace(); if agg.is_some() && self.peek() == Some('(') { self.expect_char('(')?; - let mut inner = self.parse_vector_selector()?; + self.skip_whitespace(); + // `sum()`/`avg()` may wrap `rate(metric[range])` directly instead of a bare vector selector. + let mut inner = if matches!(agg, Some(AggOp::Sum) | Some(AggOp::Avg)) { + let inner_checkpoint = self.pos; + let took_inner_rate = self.parse_identifier().is_ok_and(|id| id == "rate"); + if took_inner_rate { + self.skip_whitespace(); + if self.peek() == Some('(') { + self.expect_char('(')?; + let mut vector = self.parse_vector_selector()?; + self.expect_char(')')?; + if vector.range.is_none() { + return Err( + "rate() requires a range selector, e.g. rate(my_metric[5m])" + .to_string(), + ); + } + vector.inner_rate = true; + vector + } else { + self.pos = inner_checkpoint; + self.parse_vector_selector()? + } + } else { + self.pos = inner_checkpoint; + self.parse_vector_selector()? + } + } else { + self.parse_vector_selector()? + }; self.expect_char(')')?; inner.agg = agg; + inner.by_labels = self.parse_by_clause()?; Ok(inner) } else { // Rewind and parse as a plain vector selector, covering metric names that collide with an agg_op keyword. @@ -390,4 +471,60 @@ mod tests { assert_eq!(query.range, Some(Duration::from_secs(300))); assert_eq!(query.bucket_width, None); } + + #[test] + fn parses_sum_of_rate() { + let query = parse_query("sum(rate(requests[1m]))").unwrap(); + assert_eq!(query.agg, Some(AggOp::Sum)); + assert!(query.inner_rate); + assert_eq!(query.metric_name, "requests"); + assert_eq!(query.range, Some(Duration::from_secs(60))); + assert!(query.by_labels.is_empty()); + } + + #[test] + fn parses_avg_of_rate_with_label_matchers() { + let query = parse_query(r#"avg(rate(requests{path="/a"}[5m]))"#).unwrap(); + assert_eq!(query.agg, Some(AggOp::Avg)); + assert!(query.inner_rate); + assert_eq!(query.matchers, vec![LabelMatcher { name: "path".to_string(), value: "/a".to_string() }]); + } + + #[test] + fn rejects_sum_of_rate_without_range() { + let err = parse_query("sum(rate(requests))").unwrap_err(); + assert!(err.contains("range selector")); + } + + #[test] + fn parses_sum_with_by_clause() { + let query = parse_query("sum(requests) by (path)").unwrap(); + assert_eq!(query.agg, Some(AggOp::Sum)); + assert!(!query.inner_rate); + assert_eq!(query.by_labels, vec!["path".to_string()]); + } + + #[test] + fn parses_sum_of_rate_with_by_clause_multiple_labels() { + let query = parse_query("sum(rate(requests[1m])) by (path, method)").unwrap(); + assert!(query.inner_rate); + assert_eq!(query.by_labels, vec!["path".to_string(), "method".to_string()]); + } + + #[test] + fn sum_without_by_clause_has_empty_by_labels() { + let query = parse_query("sum(requests)").unwrap(); + assert!(query.by_labels.is_empty()); + } + + #[test] + fn rejects_empty_by_clause() { + let err = parse_query("sum(requests) by ()").unwrap_err(); + assert!(err.contains("at least one label")); + } + + #[test] + fn rejects_unterminated_by_clause() { + assert!(parse_query("sum(requests) by (path").is_err()); + } } diff --git a/webapp/src/components/RegexHelpIcon.tsx b/webapp/src/components/RegexHelpIcon.tsx index 9130e42..f16ba15 100644 --- a/webapp/src/components/RegexHelpIcon.tsx +++ b/webapp/src/components/RegexHelpIcon.tsx @@ -23,23 +23,32 @@ const CHEAT_SHEET: { tokens: string; meaning: string }[] = [ // "@" icon next to regex inputs; shows a compact cheat sheet for the regex flavor the server actually // uses (Rust `regex` crate — no lookaround/backreferences), modeled on HelpIcon's button styling. export function RegexHelpIcon({ label }: { label?: string }) { - const [open, setOpen] = useState(false) + // Click pins the popover open until clicked again; hovering shows it only while the mouse + // stays over the icon/box, independent of the pinned state. + const [pinned, setPinned] = useState(false) + const [hovering, setHovering] = useState(false) + const open = pinned || hovering const ref = useRef(null) useEffect(() => { - if (!open) return + if (!pinned) return function onClickOutside(e: MouseEvent) { - if (ref.current && !ref.current.contains(e.target as Node)) setOpen(false) + if (ref.current && !ref.current.contains(e.target as Node)) setPinned(false) } document.addEventListener('mousedown', onClickOutside) return () => document.removeEventListener('mousedown', onClickOutside) - }, [open]) + }, [pinned]) return ( -
setOpen(true)} onMouseLeave={() => setOpen(false)}> +
setHovering(true)} + onMouseLeave={() => setHovering(false)} + >