Update wishlist and remove done ticets

This commit is contained in:
Hugh Kaznowski
2026-09-06 01:31:32 +01:00
parent 1a481d374b
commit ac1efd9e53
8 changed files with 10 additions and 281 deletions
-54
View File
@@ -1,54 +0,0 @@
# 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<String>`, `value_param:
Option<String>`. 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. `<metric_name>.<param_name>` 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.
@@ -1,44 +0,0 @@
# 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<String> }`, 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).
@@ -1,32 +0,0 @@
# 039 — `metric[param]` query syntax should resolve against declared params
## Summary
`compile_metric_comparison` (`src/engine.rs`) resolves the `param` in
`metric[param] <op> 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).
@@ -1,49 +0,0 @@
# 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)).
-32
View File
@@ -1,32 +0,0 @@
# 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.
@@ -1,38 +0,0 @@
# 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.
@@ -1,32 +0,0 @@
# 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.