mirror of
https://github.com/openobserve/openobserve.git
synced 2026-11-04 08:09:27 +00:00
Prabhat Sharmaandgithub-actions[bot]
360b989272
Supersedes #13738, whose commit is included here as the first of four. Adds PromQL support to count and time-sliced SLOs, lets metrics streams choose SQL or PromQL, and fixes three shipped bugs found along the way. ## Shipped bug: every SQL time-slice SLO recorded wrong history `fetch_rows`' `SliQueryPlan::Single` arm normalized every hit with `with_good_column: true`, but that arm serves **two** shapes and only one projects `zo_slo_good`: | shape | projects | result | |---|---|---| | count single-query | `zo_slo_good` + `zo_slo_value` | correct | | time slice | `zo_slo_value` only | `good` defaulted to `0.0` | So every SQL time-slice slice was classified from `0.0` instead of its aggregate. With a positive threshold, `<`/`<=` (e.g. `p95 < 300ms`) recorded every slice **good** — the SLO read ~100%, the budget never burned, and no alert on it could fire. `>`/`>=` recorded every slice **bad** and fired continuously. Shipped in v0.92.0 (`4772df1723`); `to_row` had no time-slice test, which is how it got through. Fixed by deriving the flag from the SLI type in a new `single_query_rows`. Deliberately **not** fixed inside `to_row` by falling back to `total` — that would read a count bucket whose `SUM` came back NULL as *fully good*, inventing uptime. A test forbids it. **Stored history is not rewritten.** It ages out within one window; an operator can force a rebuild sooner by making a real edit (a no-op re-save does nothing — `update` bumps only on `definition_changed`). No auto-migration: bumping every time-slice SLO at boot would freeze every attached alert org-wide until backfill cleared the coverage floor, and double each org's row budget for 97 days — a self-inflicted monitoring gap to repair a reporting error. ## PromQL support **Time slice** carried a `query_language` field the planner ignored, so a PromQL time-slice SLO saved cleanly and measured nothing, forever. - `SliQueryPlan::PromQlValue` — one expression, not a good/total pair, planned at slice ends from the same helper the count arm uses. - The expression is passed through **verbatim**. Grouping comes from series labels; wrapping it in `sum by (...)` the way the SQL arm injects GROUP BY would destroy the aggregate (the sum of four pods' p95 is not a p95). - `promql_value_rows` **rejects** two series colliding on one `(slice_start, group_key)` via `RejectReason::AmbiguousSeries` instead of summing. Summing is right for counters (what `promql_rows` does) and meaningless here. Detection is unconditional, so the ungrouped case — an unaggregated `histogram_quantile` returning one series per instance — is covered. Rejects are plumbed through `fetch_rows` into `PassResult.rejected` on both the live and backfill paths. **Count** already had a working `CountSource::PromQl` backend that no UI could reach. ## Metrics streams choose SQL or PromQL `language_suits_stream` refused SQL whenever `stream_type == "metrics"`, and the form derived the language from the stream type — so metrics had no SQL option. A metrics stream is an ordinary stream with a `value` column, and `SELECT histogram(_timestamp, '5 minute'), avg(value) FROM cpu_usage` is a perfectly good SLI. SQL is now valid for any stream type; PromQL stays metrics-only. The form asks via a toggle (defaulting to PromQL) instead of deriving. ## Validation - `scope` refused for PromQL — a SQL `WHERE` fragment with nowhere to go in a PromQL plan. - Empty `query` refused for **both** languages (the SQL aggregate was previously never validated — same silent-freeze shape). - Expressions parsed with `promql-parser` at save, in `core` (`config` has no parser). - **Parse-only by design.** A grouping check requiring the root to be an aggregation whose `by(...)` matches `group_by` wrongly rejects `histogram_quantile(0.95, sum by (le, region)(...))` (function-call root; `le` is a necessary label superset) and binary-op roots like `sum by (region)(increase(a[5m])) - sum by (region)(increase(b[5m]))`. A "warn instead" variant needs a warnings channel the save API doesn't have. ## Also fixed - **Time-slice comparator 422.** `SliConfig::TimeSlice.comparator` has no serde default, so a dropdown the user never opened was omitted and the save died with a bare 422 — blocking creation of *any* time-slice SLO from the UI. Seeded to `<` only when blank, so a stored comparator survives an edit. - **SLO/alert/service-stream regressions** (the commit from #13738): fractional-threshold serde round-trip, `SloAlertCondition` UI, alerts table, service streams. ## Verification - 487 config · 206 core · 45 infra · 418 frontend — all passing; clippy `-D warnings`, `cargo fmt`, prettier, eslint, `vue-tsc` all clean. - Driven through a real browser against a running server (isolated instance, own data dir). All four SLI/language combinations create and store correctly: count·PromQL, count·**SQL over metrics**, time-slice·PromQL, time-slice·**SQL over metrics**. Also verified: the toggle swaps field shapes both ways, scope shows for SQL and hides for PromQL, PromQL-only hints appear only in PromQL mode, and editing a stored SQL-over-metrics SLO reopens as SQL with its predicate intact. - Validation rejections confirmed live with actionable messages: PromQL-with-scope, unparseable PromQL, and an empty SQL aggregate. Known limitation, pre-existing: `threshold` likewise has no serde default and is not seeded, so leaving it blank still 422s. That is genuine user error rather than an untouched-control trap; adding form validation was out of scope. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>