From 930a2f2536467a4eff6c79b3a9bbfbb24ffe97de Mon Sep 17 00:00:00 2001 From: Hugh Kaznowski Date: Sat, 5 Sep 2026 22:20:03 +0100 Subject: [PATCH] Add tickets to improve metric abstractions --- tickets/037-unify-param-declarations.md | 54 +++++++++++++++++++ tickets/038-duration-as-derived-param-kind.md | 44 +++++++++++++++ ...ic-param-query-resolves-declared-params.md | 32 +++++++++++ .../040-unify-rate-window-implementation.md | 49 +++++++++++++++++ tickets/041-fix-string-metric-coercion.md | 32 +++++++++++ tickets/042-fix-sum-avg-timestamp-merge.md | 38 +++++++++++++ tickets/043-histogram-by-labels-support.md | 32 +++++++++++ 7 files changed, 281 insertions(+) create mode 100644 tickets/037-unify-param-declarations.md create mode 100644 tickets/038-duration-as-derived-param-kind.md create mode 100644 tickets/039-metric-param-query-resolves-declared-params.md create mode 100644 tickets/040-unify-rate-window-implementation.md create mode 100644 tickets/041-fix-string-metric-coercion.md create mode 100644 tickets/042-fix-sum-avg-timestamp-merge.md create mode 100644 tickets/043-histogram-by-labels-support.md diff --git a/tickets/037-unify-param-declarations.md b/tickets/037-unify-param-declarations.md new file mode 100644 index 0000000..2d4b997 --- /dev/null +++ b/tickets/037-unify-param-declarations.md @@ -0,0 +1,54 @@ +# 037 — Unify param declarations: one `Param` type, referenced by name from metrics + +## Summary + +"Parameter" currently names two structurally near-identical but separate +types: `ParamDeclaration` (`src/data_value.rs`, driven by the Params panel, +used only to add a display column to the logs table) and +`RegexToMetricPrimitiveMatcher` (`src/dto.rs`), embedded inline inside a +metric declaration. A metric cannot reference an already-declared param by +name — `src/engine.rs` says so explicitly ("referencing an already-declared +`ParamDeclaration` by name isn't supported yet") — so the same +regex/capture-group/kind gets redefined from scratch per metric, and nothing +keeps a param declared in the Params panel consistent with a same-named +matcher inside a metric. + +## Scope + +- Collapse `ParamDeclaration` and `RegexToMetricPrimitiveMatcher` into one + `ParamDeclaration` type: `{ name, extractor (regex+capture_group | + json_field_path), kind }`. +- Change `MetricDeclaration` to reference params by name instead of embedding + its own extractor: `label_params: Vec`, `value_param: + Option`. Role (label vs. value) becomes a property of how a metric + uses a param, not a property of the param itself. +- Update `apply_metric_to_log` (`src/engine.rs`) to resolve `label_params`/ + `value_param` against the shared param declaration store instead of reading + an inline matcher. +- Update declare/validate paths (`validate_metric_request`, `engine.rs`) to + reject a metric referencing an undeclared param name. +- Update frontend types (`webapp/src/lib/api.ts`) and `Metrics.tsx`'s + declaration form to pick existing declared params (e.g. a select populated + from the Params panel) rather than re-entering a regex/capture-group/kind + per metric. +- Migration: existing stored `MetricDeclaration` records with inline matchers + need their matchers turned into standalone `ParamDeclaration` entries + (created implicitly, named e.g. `.` if no matching + declared param exists) so existing profiles keep working after upgrade. + +## Acceptance criteria + +- A param declared once in the Params panel can be selected as a label or + value param by multiple metric declarations without re-entering its + extractor. +- Declaring a metric that references a nonexistent param name is rejected at + declare-time with a clear error. +- Existing profiles with metrics declared under the old inline-matcher shape + load and behave identically after migration. +- `Metrics.tsx` no longer has a separate "regex + capture group + kind" entry + form duplicated from the Params panel's form. + +## Depends on + +None. [038](038-duration-as-derived-param-kind.md) and +[039](039-metric-param-query-resolves-declared-params.md) build on this. diff --git a/tickets/038-duration-as-derived-param-kind.md b/tickets/038-duration-as-derived-param-kind.md new file mode 100644 index 0000000..5fdcdc8 --- /dev/null +++ b/tickets/038-duration-as-derived-param-kind.md @@ -0,0 +1,44 @@ +# 038 — Make Duration an explicit derived-param kind, not a hidden case of Value + +## Summary + +Today a `Value`-role param with `kind: Duration` silently means something +different from every other kind: instead of parsing extracted text, it means +"elapsed time since the previous same-labeled event" +(`apply_metric_to_log`, `src/engine.rs`, backed by a `LastEventTimestamp` +index entry). Same enum variant (`MetricPrimitiveKind::Duration`), two +unrelated meanings depending on role — one is "parse this text as a +duration," which doesn't apply here at all; the other is "compute elapsed +time from event sequencing." This is a silent special case a reader has to +already know about to understand the code. + +## Scope + +- Add an explicit extractor variant, e.g. `Extractor::ElapsedSinceLast { + grouped_by: Vec }`, distinct from `Regex`/`JsonPath`. +- A param declared with this extractor is computed (elapsed time since the + previous event sharing the given label set), never text-extracted. + `MetricPrimitiveKind` no longer needs a role-dependent special case for + `Duration`. +- Update `apply_metric_to_log` to branch on extractor kind instead of on + `(role, kind)` combination. +- Update the Params panel / metric declaration UI (`Params.tsx`, + `Metrics.tsx`) to offer "elapsed since previous event" as its own + extractor option rather than a regex+kind combination that happens to mean + something else. +- Migration: existing declared/embedded params using `Value` role + + `Duration` kind convert to the new extractor variant. + +## Acceptance criteria + +- A "duration" param's behavior (elapsed time since last matching event) is + selectable directly in the UI without going through a regex field that is + never actually used. +- `apply_metric_to_log` has no branch that changes meaning based on role for + a given kind. +- Existing profiles using the old Value+Duration combination behave + identically after migration. + +## Depends on + +[037](037-unify-param-declarations.md). diff --git a/tickets/039-metric-param-query-resolves-declared-params.md b/tickets/039-metric-param-query-resolves-declared-params.md new file mode 100644 index 0000000..4c69dbe --- /dev/null +++ b/tickets/039-metric-param-query-resolves-declared-params.md @@ -0,0 +1,32 @@ +# 039 — `metric[param]` query syntax should resolve against declared params + +## Summary + +`compile_metric_comparison` (`src/engine.rs`) resolves the `param` in +`metric[param] value` by reading whatever key happens to be present in a +bucket's `labels` map at query time. There's no link back to a metric's +actual declared label params, so a typo or a param that was never a label on +that metric fails silently (empty match) instead of being rejected up front. + +## Scope + +- Once [037](037-unify-param-declarations.md) lands, validate `metric[param]` + filter expressions against the target metric's declared `label_params` at + parse/compile time. +- Return a clear filter-validation error when `param` is not one of the + metric's label params, instead of silently matching nothing. +- Confirm `src/filter_parser.rs` and `compile_metric_comparison` share the + same notion of "param" as the Params panel and metric declarations after + this change (no separate resolution path). + +## Acceptance criteria + +- `metric[nonexistent_param] == "x"` is rejected with a validation error + referencing the metric's actual label params, rather than silently + returning no matches. +- Existing valid filter queries using real label params continue to work + unchanged. + +## Depends on + +[037](037-unify-param-declarations.md). diff --git a/tickets/040-unify-rate-window-implementation.md b/tickets/040-unify-rate-window-implementation.md new file mode 100644 index 0000000..35cdab2 --- /dev/null +++ b/tickets/040-unify-rate-window-implementation.md @@ -0,0 +1,49 @@ +# 040 — One rate/window implementation, shared by PromQL and alerts + +## Summary + +"Rate over a window" is implemented twice with different semantics and no +shared code: + +- PromQL's `rate()` (`windowed_rate_series`, `src/engine.rs`) scans the + trailing window at every historical point over indexed bucket data + (occurrences-per-second, backfilled). +- Alerts' `Threshold`/`Ratio` conditions (`src/alerts.rs`) maintain an + independent live sliding `VecDeque` window over raw log-line matches + (current count only, resets on process restart, no backfill), deliberately + bypassing the bucket/PromQL pipeline for cost reasons (module doc: a + `metric[param]` comparison would require a full bucket rescan per line). + +These can and will disagree for equivalent-looking rate/ratio rules, since +they read different data and compute the window differently. + +## Scope + +- Extract a single windowed-rate function, e.g. in a new `src/rate.rs`: + `fn rate_at(points: &[(Timestamp, f64)], now: Timestamp, window_ms: i64) -> f64`. +- Have PromQL's `rate()` evaluation call this shared function (behavior + should be unchanged for existing dashboard queries). +- Have alerts call the same function against the bucket store instead of + maintaining a separate `VecDeque` over raw matches. If alerts' proportional + per-line cost guarantee can't be met by reading buckets directly, solve + that inside the shared module (e.g. an incrementally maintained window + index) rather than keeping two implementations. +- As a consequence, alert rate/ratio conditions gain backfilled history + (they no longer reset to empty on process restart) — call this out in + release notes since it changes alert-firing behavior on restart. + +## Acceptance criteria + +- Only one windowed-rate implementation exists in the codebase. +- A dashboard `rate()` chart and an alert `Threshold`/`Ratio` condition over + the same metric/window agree on the computed rate for the same historical + data, verified with a seeded fixture. +- Alert conditions no longer silently reset their window on process restart + (or this is an explicitly flagged, deliberate behavior change). +- Existing dashboard `rate()` queries produce unchanged output for existing + test fixtures. + +## Depends on + +None — independent of the param unification work +([037](037-unify-param-declarations.md)–[039](039-metric-param-query-resolves-declared-params.md)). diff --git a/tickets/041-fix-string-metric-coercion.md b/tickets/041-fix-string-metric-coercion.md new file mode 100644 index 0000000..c13d07f --- /dev/null +++ b/tickets/041-fix-string-metric-coercion.md @@ -0,0 +1,32 @@ +# 041 — Stop silently coercing string metric values to 1.0 in sum()/avg() + +## Summary + +`coerce_to_f64` (`src/engine.rs`) maps every `MetricPrimitive::String` value +to a flat `1.0`. This means `sum()`/`avg()` on a string-valued metric +silently becomes an occurrence count instead of erroring or behaving +predictably, with no signal to the user that their aggregate doesn't mean +what it looks like it means. + +## Scope + +- At PromQL query-evaluation time, detect when the target metric's declared + value kind is `String` and the requested aggregation is `sum`/`avg`. +- Either reject the query with a clear error ("cannot sum/avg a string-typed + metric; use count() or a numeric-typed metric"), or introduce an explicit + `count()` aggregation so occurrence-counting is something a user opts into + by name rather than something `sum()` does silently for the wrong metric + kind. +- Audit existing dashboards/panels for `sum()`/`avg()` over string-kind + metrics that would now be rejected or need updating to `count()`. + +## Acceptance criteria + +- Querying `sum()`/`avg()` over a string-kind metric either fails with a + clear, actionable error or is redirected to an explicit `count()` semantic + — it no longer silently returns a count while looking like a sum. +- Numeric-kind metrics are unaffected. + +## Depends on + +None. diff --git a/tickets/042-fix-sum-avg-timestamp-merge.md b/tickets/042-fix-sum-avg-timestamp-merge.md new file mode 100644 index 0000000..5dae257 --- /dev/null +++ b/tickets/042-fix-sum-avg-timestamp-merge.md @@ -0,0 +1,38 @@ +# 042 — Verify/fix exact-timestamp merging for non-rate sum()/avg() + +## Summary + +Non-rate `sum()`/`avg()` queries merge per-label series via +`merge_series_exact` (`src/engine.rs`), which only combines values whose +timestamps match *exactly*. In practice, raw occurrence timestamps across +different label groups essentially never land on the same millisecond, so +`sum(requests) by (path)` over raw occurrence counts likely produces sparse, +near-empty merged series rather than a meaningful "total requests over +time," even though that's the obvious expectation for such a query. The +existing code comment justifying exact-match merging ("nothing to carry +forward") only holds if the query isn't meant to sum sparse counts over +time — which is exactly the common case for occurrence-count dashboards. + +## Scope + +- Build a seeded fixture with multiple label groups whose events arrive at + different (non-coincident) timestamps and confirm whether + `sum(...) by (...)` currently produces the expected combined total or the + suspected sparse/near-empty result. +- If confirmed broken: bucket points into fixed time windows (the same way + `rate()` already windows) before summing/averaging across label groups, + instead of requiring exact timestamp coincidence. +- Add a regression test covering multi-label-group `sum()`/`avg()` over + time. + +## Acceptance criteria + +- `sum(metric) by (label)` over multiple label groups with staggered event + timestamps produces a sensible combined time series (not sparse/empty), + verified by a new test fixture. +- `avg()` behaves analogously. +- Existing single-label-group queries are unaffected. + +## Depends on + +None. diff --git a/tickets/043-histogram-by-labels-support.md b/tickets/043-histogram-by-labels-support.md new file mode 100644 index 0000000..de25270 --- /dev/null +++ b/tickets/043-histogram-by-labels-support.md @@ -0,0 +1,32 @@ +# 043 — Histogram queries ignore by() grouping, inconsistent with sum/avg/rate + +## Summary + +PromQL evaluation (`evaluate_promql`, `src/engine.rs`) has inconsistent +grouping semantics across aggregation ops: `sum`/`avg`/`rate` respect the +query's `by_labels` and group series accordingly, but `Histogram` collapses +all label combinations into a single histogram regardless of `by()`. The +same query language should not silently vary in what `by()` means depending +on the aggregation used. + +## Scope + +- Decide the intended behavior: either (a) support `by()` grouping for + histograms, producing one histogram per label group, or (b) explicitly + reject/ignore `by()` on histogram queries with a clear validation error + rather than silently accepting and discarding it. +- Implement the chosen behavior in `evaluate_promql`'s histogram branch. +- Update any dashboard panel UI (`Dashboards.tsx`) that lets a user combine + histogram with `by()` to reflect the supported behavior (either offering + grouped histograms or disabling `by()` for histogram panels). + +## Acceptance criteria + +- `histogram(metric) by (label)` either produces one histogram per label + group (verified by a seeded fixture) or is rejected/flagged clearly at + query time — it no longer silently accepts and ignores `by()`. +- `sum`/`avg`/`rate` behavior with `by()` is unchanged. + +## Depends on + +None.