From 9b1da3b3dee2fe724496945a6f64abbaa2ce28a5 Mon Sep 17 00:00:00 2001 From: Bartek Tofel Date: Mon, 5 Oct 2026 19:51:07 +0200 Subject: [PATCH 1/8] feat: support for non-Grafana managed alerts --- grafana-alertcheck/.changeset/v0.1.10.md | 6 + grafana-alertcheck/cmd/list.go | 26 ++- grafana-alertcheck/cmd/list_test.go | 37 ++++ grafana-alertcheck/cmd/table.go | 25 ++- grafana-alertcheck/docs/advanced.md | 8 + grafana-alertcheck/docs/architecture.md | 6 + .../docs/how-alerts-are-evaluated.md | 9 + grafana-alertcheck/docs/index.md | 2 +- grafana-alertcheck/docs/reference/cli.md | 19 +- .../docs/reference/log-format.md | 21 +- grafana-alertcheck/internal/gate/check.go | 135 +++++++++---- .../internal/gate/check_ds_test.go | 53 +++++ .../internal/gate/check_test.go | 52 ++++- grafana-alertcheck/internal/gate/classify.go | 38 ++-- grafana-alertcheck/internal/gate/coverage.go | 35 ++-- .../gate/datasource_semantics_test.go | 108 ++++++++++ grafana-alertcheck/internal/gate/handoff.go | 9 +- grafana-alertcheck/internal/gate/identity.go | 62 ++++++ .../internal/gate/identity_test.go | 40 ++++ grafana-alertcheck/internal/gate/labels.go | 6 +- grafana-alertcheck/internal/gate/load.go | 74 +++++++ grafana-alertcheck/internal/gate/log.go | 53 +++-- .../internal/gate/log_ds_test.go | 77 +++++++ .../internal/gate/parse_datasource.go | 190 ++++++++++++++++++ .../internal/gate/parse_datasource_test.go | 76 +++++++ .../internal/gate/parse_ruler.go | 26 ++- .../internal/gate/parse_state.go | 24 ++- grafana-alertcheck/internal/gate/resolve.go | 183 ++++++++++------- .../internal/gate/resolve_test.go | 38 +++- grafana-alertcheck/internal/gate/schedule.go | 30 +-- grafana-alertcheck/internal/gate/source.go | 143 ++++++++++++- .../internal/gate/source_ds_test.go | 139 +++++++++++++ .../internal/gate/source_fake_test.go | 65 +++++- .../internal/gate/source_test.go | 26 +-- grafana-alertcheck/internal/gate/terminal.go | 10 +- .../internal/gate/testdata/ds_rules.json | 50 +++++ grafana-alertcheck/internal/gate/watch.go | 137 ++++++++----- .../internal/gate/watch_daemon_test.go | 2 + .../internal/gate/watch_test.go | 35 +++- 39 files changed, 1767 insertions(+), 308 deletions(-) create mode 100644 grafana-alertcheck/.changeset/v0.1.10.md create mode 100644 grafana-alertcheck/internal/gate/check_ds_test.go create mode 100644 grafana-alertcheck/internal/gate/datasource_semantics_test.go create mode 100644 grafana-alertcheck/internal/gate/identity.go create mode 100644 grafana-alertcheck/internal/gate/identity_test.go create mode 100644 grafana-alertcheck/internal/gate/load.go create mode 100644 grafana-alertcheck/internal/gate/log_ds_test.go create mode 100644 grafana-alertcheck/internal/gate/parse_datasource.go create mode 100644 grafana-alertcheck/internal/gate/parse_datasource_test.go create mode 100644 grafana-alertcheck/internal/gate/source_ds_test.go create mode 100644 grafana-alertcheck/internal/gate/testdata/ds_rules.json diff --git a/grafana-alertcheck/.changeset/v0.1.10.md b/grafana-alertcheck/.changeset/v0.1.10.md new file mode 100644 index 000000000..2d9e0a169 --- /dev/null +++ b/grafana-alertcheck/.changeset/v0.1.10.md @@ -0,0 +1,6 @@ +- `list`, `watch` and `check` can now observe **datasource-managed** (Prometheus-flavored) alerting rules, auto-discovered from `/api/datasources` (strict `type == "prometheus"` and `jsonData.manageAlerts == true`, then probed) and read through Grafana's per-datasource Prometheus API. No new selection flag: the watch set is still `--alerts`/labels. Loki is deferred. +- `list` gains `DATASOURCE` and `KEY` columns. Datasource rules accept `Group/Title`, `DatasourceName/Group/Title`, and exact `key:` names; `--folder` stays Grafana-only, and ambiguity errors name the datasource. +- Datasource-managed identity is a separate rule key; `uid` stays empty for these rules. The log gains additive `key`, `source_kind`, `datasource_uid`, `datasource_name`, `file` and `rule_key` fields, with `schema_version` still `1`. An old v1 log without `rule_key` remains readable; a new log read by an old binary fails closed on the unresolved key. +- Datasource recovery semantics: because the Prometheus API returns only active instances, an instance leaving the active set is treated as a real recovery (`cleared`). Documented weaker guarantee: a vanished series is indistinguishable from a resolution. +- Pause is not observable for datasource-managed rules (no `isPaused` signal), so check 7 is skipped with an explicit note. `health: err` normalizes to `error` and still triggers check 4. +- The token now needs `datasources:read` plus datasource query permission even for Grafana-only runs; discovery failures name the missing permission. diff --git a/grafana-alertcheck/cmd/list.go b/grafana-alertcheck/cmd/list.go index c1d0e8532..a6355b213 100644 --- a/grafana-alertcheck/cmd/list.go +++ b/grafana-alertcheck/cmd/list.go @@ -45,7 +45,8 @@ func runList(args []string, stdout, stderr io.Writer) int { return 2 } - defs, err := src.Definitions(context.Background()) + fmt.Fprintln(stderr, "discovering rule sources and reading definitions (this can take seconds per source)...") + defs, err := gate.ListAllDefinitions(context.Background(), src) if err != nil { fmt.Fprintf(stderr, "reading rule definitions: %v\n", err) return 2 @@ -62,9 +63,11 @@ func runList(args []string, stdout, stderr io.Writer) int { }) tw := tabwriter.NewWriter(stdout, 0, 4, 2, ' ', 0) - fmt.Fprintln(tw, "KIND\tFOLDER\tGROUP\tTITLE\tUID") + fmt.Fprintln(tw, "KIND\tDATASOURCE\tFOLDER\tGROUP\tTITLE\tKEY\tUID") for _, d := range defs { - fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\n", kindLabel(d.Kind), d.Folder, d.Group, d.Title, uidOrDash(d.UID)) + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\n", + kindLabel(d.Kind), datasourceOrDash(d.DatasourceName), d.Folder, d.Group, d.Title, + keyOrDash(d.Key, d.UID), uidOrDash(d.UID)) } if err := tw.Flush(); err != nil { fmt.Fprintf(stderr, "writing output: %v\n", err) @@ -90,3 +93,20 @@ func uidOrDash(uid string) string { } return uid } + +func datasourceOrDash(name string) string { + if name == "" { + return "-" + } + return name +} + +func keyOrDash(key, uid string) string { + if key == "" { + key = uid + } + if key == "" { + return "-" + } + return key +} diff --git a/grafana-alertcheck/cmd/list_test.go b/grafana-alertcheck/cmd/list_test.go index 62aa200d2..6494f93c4 100644 --- a/grafana-alertcheck/cmd/list_test.go +++ b/grafana-alertcheck/cmd/list_test.go @@ -45,6 +45,8 @@ func grafanaTestServer(t *testing.T, version string) *httptest.Server { _, _ = w.Write([]byte(healthBody(version))) case "/api/ruler/grafana/api/v1/rules": _, _ = w.Write([]byte(rulerBody)) + case "/api/datasources": + _, _ = w.Write([]byte(`[]`)) default: t.Errorf("unexpected path %q", r.URL.Path) w.WriteHeader(http.StatusNotFound) @@ -68,6 +70,41 @@ func TestRunList_HappyPath(t *testing.T) { require.Contains(t, out, "grafana-managed") } +func TestRunList_IncludesDatasourceRules(t *testing.T) { + const dsBody = `{"status":"success","data":{"groups":[{"name":"ExampleMetrics","file":"/etc/vm/rules/example.yml","interval":60,"rules":[ + {"name":"ExampleTargetDown","type":"alerting","health":"ok","state":"firing","query":"up == 0","duration":300, + "labels":{"severity":"warning"},"alerts":[]}]}]}}` + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/health": + _, _ = w.Write([]byte(healthBody("13.1.0"))) + case "/api/ruler/grafana/api/v1/rules": + _, _ = w.Write([]byte(rulerBody)) + case "/api/datasources": + _, _ = w.Write([]byte(`[{"uid":"vm","name":"VM Prod","type":"prometheus","jsonData":{"manageAlerts":true}}]`)) + case "/api/prometheus/vm/api/v1/rules": + _, _ = w.Write([]byte(dsBody)) + default: + t.Errorf("unexpected path %q", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(srv.Close) + t.Setenv("GRAFANA_URL", srv.URL) + t.Setenv("GRAFANA_TOKEN", "test-token") + + var stdout, stderr bytes.Buffer + code := run([]string{"list"}, &stdout, &stderr) + require.Equal(t, 0, code) + out := stdout.String() + require.Contains(t, out, "DATASOURCE") + require.Contains(t, out, "KEY") + require.Contains(t, out, "VM Prod") + require.Contains(t, out, "ExampleTargetDown") + require.Contains(t, out, "ds:") +} + func TestRunList_UnsupportedVersion(t *testing.T) { srv := grafanaTestServer(t, "12.5.0") t.Setenv("GRAFANA_URL", srv.URL) diff --git a/grafana-alertcheck/cmd/table.go b/grafana-alertcheck/cmd/table.go index d756d1575..83306392a 100644 --- a/grafana-alertcheck/cmd/table.go +++ b/grafana-alertcheck/cmd/table.go @@ -38,7 +38,7 @@ no evaluation for — how long Grafana may go without evaluating the alert befor func renderTable(w io.Writer, res gate.Result) error { alertOf := make(map[string]string, len(res.Verdicts)) for _, v := range res.Verdicts { - alertOf[v.RuleUID] = v.Alert + alertOf[verdictKey(v)] = v.Alert } enabled := colorEnabled(w) @@ -64,7 +64,7 @@ func renderTable(w io.Writer, res gate.Result) error { for _, v := range sortedVerdicts(res.Verdicts) { fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\n", v.Alert, v.Outcome, v.BadFor.Round(time.Second), v.PollEvery.Round(time.Second), - provedLabel(res.Coverage[v.RuleUID]), v.Note) + provedLabel(res.Coverage[verdictKey(v)]), v.Note) } if err := tw.Flush(); err != nil { return fmt.Errorf("render table: %w", err) @@ -160,12 +160,29 @@ func alertLabel(v gate.Violation, alertOf map[string]string) string { if v.Alert != "" { return v.Alert } - if a, ok := alertOf[v.RuleUID]; ok { + if a, ok := alertOf[violationKey(v)]; ok { return a } return "-" } +// verdictKey is a RuleVerdict's identity: RuleKey when set, else RuleUID (a +// Result built directly in a test may carry only RuleUID). +func verdictKey(v gate.RuleVerdict) string { + if v.RuleKey != "" { + return v.RuleKey + } + return v.RuleUID +} + +// violationKey mirrors verdictKey for a Violation. +func violationKey(v gate.Violation) string { + if v.RuleKey != "" { + return v.RuleKey + } + return v.RuleUID +} + func alertOr(uid string, alertOf map[string]string) string { if a, ok := alertOf[uid]; ok { return a @@ -201,7 +218,7 @@ func groupedViolations(in []gate.Violation) []violationGroup { } func violationSignature(v gate.Violation) string { - return v.Alert + "\x00" + v.RuleUID + "\x00" + string(v.Outcome) + "\x00" + string(v.State) + "\x00" + v.Health + "\x00" + v.Note + return v.Alert + "\x00" + violationKey(v) + "\x00" + string(v.Outcome) + "\x00" + string(v.State) + "\x00" + v.Health + "\x00" + v.Note } func sameRendered(a, b gate.Violation) bool { diff --git a/grafana-alertcheck/docs/advanced.md b/grafana-alertcheck/docs/advanced.md index fa6f0be23..1b8fb87ca 100644 --- a/grafana-alertcheck/docs/advanced.md +++ b/grafana-alertcheck/docs/advanced.md @@ -35,6 +35,14 @@ The detached recorder then continues the schedule the first observations were on Single-step `check` runs the same pass itself. It cannot watch before it started, so a `from` inside the pass is a declared blind interval: the run warns, classifies from the pass completion, and the live poller continues the pass's schedule. It never classifies a window that opens before every rule has been observed. +## Datasource-managed rules: discovery and cost + +Datasource-managed rules are auto-discovered — there is no selection flag. The candidate filter is strict: `type == "prometheus"` **and** `jsonData.manageAlerts == true`. The strict `true` matters: the `AlertStateHistoryBackend` datasource shares VictoriaMetrics' backend and also reports `manageAlerts` truthy, so a `!= false` filter would make every rule name ambiguous. Loki is deferred: its ruler API is broken/disabled in our Grafana, so only Prometheus-flavored rules are in scope. + +Each candidate is probed with a `rule_name[]=__probe__` request; a `manageAlerts=true` source whose probe fails is a hard error naming the source, never a silently dropped source. + +Cost differs by mode. `list` and label selection take the **bulk** response — one request per datasource, several MB and several seconds for a large ruler. Name selection takes a **filtered** request, ~1 KB and ~1 s. The filter uses vmalert's `[]`-suffixed parameters (`rule_name[]`, `rule_group[]`, `file[]`): vmalert reads only those and ignores plain `rule_name=`, an upstream quirk pinned by tests. `limit_alerts` is a Grafana parameter that vmalert ignores and is therefore omitted. + ## Why the gate never queries state history Querying Grafana's alert state history after the fact fails closed *in the wrong direction* — it returns "pass" when the truth is unknown: diff --git a/grafana-alertcheck/docs/architecture.md b/grafana-alertcheck/docs/architecture.md index e8d62f442..0c3766937 100644 --- a/grafana-alertcheck/docs/architecture.md +++ b/grafana-alertcheck/docs/architecture.md @@ -32,6 +32,12 @@ HTTP ──> Source ──> []StateRule ──> reduce ──> []Poll ──> pr JSONL log ──> ReadLog ──┘ ``` +The `Source` interface is the only HTTP boundary and covers both rule kinds: `GrafanaDefinitions` reads the ruler, `DiscoverRuleSources` + `DatasourceDefinitions` read per-datasource Prometheus rules, and `RuleState` takes a `RuleRef` describing exactly how to find one rule again. + +### Key vs uid + +A rule's map key is its **key**, not its uid. For a Grafana-managed rule the key *is* the uid; a datasource-managed rule has no uid, so its key is a JSON-encoded `(datasource, group, name)` tuple. The key, not the uid, is what `rt`, the scheduler, the `Reducer`, `pausedAtStart`, exclusions, `Coverage`, `Thresholds` and `Verdicts` are indexed by. The log records both: `key` and, for a Grafana rule, `uid`; the poll reader uses `rule_key` when present and falls back to `rule_uid`, so an old v1 log stays readable. Identity is not weakened — for Grafana-managed rules behavior is byte-for-byte unchanged, because key == uid there. + - `proveCoverage` (the nine coverage checks) and `decide` (the instance timelines and outcomes) are pure; tests drive them with `[]Poll` literals and a fake `Clock`, with no sleeping or fixture server. - `Check`/`Watch` are I/O shells: HTTP, signals, the pidfile, file reads, the countdown print. The only test doubles needed are the `Source` and `Clock` interfaces. - `Policy` is the narrowed view of `Config` that reaches the pure layer — classification knobs and the window, no URL and no token. The token must never cross that line, which is the cheapest guarantee it never lands in an error string or a result. diff --git a/grafana-alertcheck/docs/how-alerts-are-evaluated.md b/grafana-alertcheck/docs/how-alerts-are-evaluated.md index 60c2c28f9..7a89e172b 100644 --- a/grafana-alertcheck/docs/how-alerts-are-evaluated.md +++ b/grafana-alertcheck/docs/how-alerts-are-evaluated.md @@ -61,6 +61,15 @@ When an instance leaves the bad set, the gate looks it up **in the same response A vanished instance that was bad stays `still_failing`. A metric that stops being emitted is not evidence of health — this is deliberate and can surprise users whose fix is to remove a metric rather than drive it to a good value. +### Datasource-managed rules + +Datasource-managed (Prometheus-flavored) rules are read through Grafana's per-datasource Prometheus API, not the ruler. Their response carries only **active** instances, so an instance leaving the active set is treated as a real recovery (`cleared`). The weaker guarantee is documented deliberately: a vanished series is indistinguishable from a resolution, so a fix that stops emitting a metric passes for a datasource-managed rule where it would fail for a Grafana-managed one. + +Two coverage checks differ for these rules: + +- **Pause is not observable** — the datasource API has no `isPaused` signal, so check 7 is skipped with an explicit note. A pause is not treated as a pass; it simply cannot be seen. +- **Health** — the datasource vocabulary reports `err`, which is normalized to `error`, so a sustained failing evaluation still triggers check 4. There are no `totals`, reasons or normal instances, so checks 5 and 9 never fire. + ## Coverage proof Before classifying, `check` must **prove** continuous coverage of `[from, to]` for each alert. Nine checks run; any failure makes the rule `not_verified`: diff --git a/grafana-alertcheck/docs/index.md b/grafana-alertcheck/docs/index.md index ef97473e7..052cf0b18 100644 --- a/grafana-alertcheck/docs/index.md +++ b/grafana-alertcheck/docs/index.md @@ -35,7 +35,7 @@ export GRAFANA_TOKEN=… Requires Grafana >= 13.0.0 and < 14.0.0. Outside that range the gate exits `2`. -Grafana API token needs to have `fixed:alerting:reader` permissions. Ask the o11y team for your token. +The Grafana API token needs `fixed:alerting:reader`, plus `datasources:read` and datasource query permission so datasource-managed rules can be discovered and read. Ask the o11y team for your token. ## Quickstart — recorder mode diff --git a/grafana-alertcheck/docs/reference/cli.md b/grafana-alertcheck/docs/reference/cli.md index 91ee6910f..251e24dd3 100644 --- a/grafana-alertcheck/docs/reference/cli.md +++ b/grafana-alertcheck/docs/reference/cli.md @@ -16,7 +16,7 @@ Connection details are always from the environment: `GRAFANA_URL` and `GRAFANA_T ## `list` -Lists every rule from the ruler endpoint — kind, folder, group, title, uid. Useful to check auth and to find `uid:` names. +Lists every rule — kind, datasource, folder, group, title, key, uid. Grafana-managed rules come from the ruler endpoint; datasource-managed rules are auto-discovered per datasource through the Prometheus API. Useful to check auth and to find `uid:`/`key:` names. The bulk datasource fetch can take seconds per source. ```bash grafana-alertcheck list @@ -98,16 +98,21 @@ By default `check` **exits early** on a failure that cannot become a pass: a pos ## Naming alerts -Alert names take one of four forms: +Alert names take one of these forms. Grafana-managed rules use folder/group; datasource-managed rules use datasource/group, and are auto-discovered — there is no selection flag. | Form | Meaning | | ---- | ------- | -| `HighErrorRate` | Title only, scoped by `--folder` | -| `Platform/HighErrorRate` | Folder + title | -| `Platform/api/HighErrorRate` | Folder + group + title (may still be ambiguous; use `uid:` for guaranteed uniqueness) | -| `uid:abc123` | Exact uid (present on both endpoints) | +| `HighErrorRate` | Title only; scoped by `--folder` for Grafana rules | +| `Platform/HighErrorRate` | Grafana folder + title | +| `Platform/api/HighErrorRate` | Grafana folder + group + title | +| `ExampleMetrics/HighErrorRate` | Datasource group + title | +| `VM Prod/ExampleMetrics/HighErrorRate` | Datasource name + group + title | +| `uid:abc123` | Exact Grafana uid | +| `key:ds:[…]` | Exact rule key across both kinds (copyable from `list`) | -Recording rules are refused with a specific error. Datasource-managed rules never reach resolution at all: the Grafana-managed ruler endpoint this tool queries does not return them. A no-match errors with case-insensitive substring suggestions and points at `list`, and states that datasource-managed rules cannot be observed and are not supported. A name matching multiple rules errors listing every candidate with the copyable `Folder/Group/Title` and its `uid:` form. Duplicate names that resolve to the same uid collapse to one (a note, not an error). +`--folder` scopes a bare Grafana title only; it does not apply to datasource-managed rules. A recording rule is refused with a specific error, as is a datasource-managed rule whose datasource could not be identified. A no-match errors with case-insensitive substring suggestions and points at `list`. A name matching multiple rules errors listing every candidate with its copyable full name, its source and its `uid:`/`key:` form. Duplicate names that resolve to the same rule collapse to one (a note, not an error). + +Auto-discovery reads `/api/datasources` and keeps only `type == "prometheus"` with `jsonData.manageAlerts == true`, then probes each. The token needs `datasources:read` plus datasource query permission; a failure names the permission. ## Selecting alerts by labels diff --git a/grafana-alertcheck/docs/reference/log-format.md b/grafana-alertcheck/docs/reference/log-format.md index 74694c8d7..239da9845 100644 --- a/grafana-alertcheck/docs/reference/log-format.md +++ b/grafana-alertcheck/docs/reference/log-format.md @@ -34,22 +34,39 @@ The header must be line 1, appear once, and carry `schema_version` `1` (any othe "ready_at": "2026-09-07T10:00:27Z", "rules": [ { + "key": "rule0000001", "uid": "rule0000001", "title": "HighErrorRate", "folder": "Platform", "group": "api", + "source_kind": "grafana", "for_seconds": 300, "interval_seconds": 60, "is_paused": false, "no_data_state": "OK", "exec_err_state": "OK", "poll_every_seconds": 30 + }, + { + "key": "ds:[\"vm\",\"ExampleMetrics\",\"ExampleTargetDown\"]", + "uid": "", + "title": "ExampleTargetDown", + "group": "ExampleMetrics", + "source_kind": "datasource", + "datasource_uid": "vm", + "datasource_name": "VM Prod", + "file": "/etc/vm/rules/example.yml", + "for_seconds": 300, + "interval_seconds": 60, + "is_paused": false, + "poll_every_seconds": 30 } ] } ``` -- `url` and `rules` are the log's identity — `check` validates them against the current environment and a fresh ruler read. +- `url` and `rules` are the log's identity — `check` validates them against the current environment and a fresh read. +- `key` is the rule's identity across both source kinds; `uid` is the API-given uid and is empty for a datasource-managed rule. `source_kind`, `datasource_uid`, `datasource_name` and `file` are additive (schema stays `1`) and let `check` re-resolve a datasource rule without discovery. A v1 log written before these fields existed still reads. - `started_at` is when the recording opened; `ready_at` is when the first-observation pass completed and every watched, non-paused rule had been observed once. The pass is sequential, so `check` refuses a `from` before `ready_at` (a window opening inside the pass would rest on observations that do not exist). `ready_at` is absent on logs written before the field existed; `check` then falls back to `started_at`. - `is_paused` records the pause state at record start (the moment `paused` means). - `poll_every_seconds` is the cadence the recording **actually used** (after any `--poll-interval` override). `check` derives `maxGap` from it, never from `interval_seconds`. @@ -60,6 +77,7 @@ The header must be line 1, appear once, and carry `schema_version` `1` (any othe ```json { "type": "poll", + "rule_key": "rule0000001", "rule_uid": "rule0000001", "grafana_now": "2026-09-07T10:00:30Z", "skew_ms": 20, @@ -80,6 +98,7 @@ The header must be line 1, appear once, and carry `schema_version` `1` (any othe Field notes: +- `rule_key` is the identity across both kinds; `rule_uid` is kept for compatibility and is empty for a datasource-managed rule. A reader uses `rule_key` when present, else `rule_uid`, so an old v1 log stays readable. - `grafana_now` is the response's `Date` header — never the runner clock. - `skew_ms`/`skew_bound_ms` are the per-poll clock-skew estimate and its uncertainty (RTT/2), in milliseconds for compactness only. - `found: false` is an authoritative `2xx` in which this rule was absent — a transport failure is retried and never becomes a poll. diff --git a/grafana-alertcheck/internal/gate/check.go b/grafana-alertcheck/internal/gate/check.go index ea07e3a88..6f36c7b47 100644 --- a/grafana-alertcheck/internal/gate/check.go +++ b/grafana-alertcheck/internal/gate/check.go @@ -206,11 +206,13 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { } cfg.To = roundWindowUp(from, cfg.To) - // ---- Resolve the definitions from the ruler API. ---------------------- - // Unconditional, in BOTH modes. A log's header supplies the alert set as - // UIDs and the recording facts, never the rule facts: `for`, - // intervalSeconds and Kind always come from a fresh ruler read, which is - // why LoggedRule.ForSeconds is never converted back into a Definition. + // ---- Resolve the definitions. ----------------------------------------- + // In both modes a fresh read supplies the rule facts: a log's header + // supplies the alert set (as keys and datasource UIDs) and the recording + // facts, never `for`, intervalSeconds or Kind, which is why + // LoggedRule.ForSeconds is never converted back into a Definition. Single- + // step reads the ruler plus every discovered datasource; log mode resolves + // the header's own datasource UIDs and skips discovery entirely. version, err := src.Version(ctx) if err != nil { return Result{}, fmt.Errorf("read grafana version: %w", err) @@ -218,10 +220,6 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { if err := CheckGrafanaVersion(version); err != nil { return Result{}, err } - allDefs, err := src.Definitions(ctx) - if err != nil { - return Result{}, fmt.Errorf("read rule definitions: %w", err) - } // ---- With a log, validate its identity. ------------------------------- // The header is read early — line 1 only, the one line a writer can never @@ -243,7 +241,7 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { return Result{}, fmt.Errorf("log identity: %w", err) } logHasHdr = true - resolved, notes, err = resolveFromLog(allDefs, earlyHdr, cfg) + resolved, notes, err = resolveFromLog(ctx, src, earlyHdr, cfg) if err == nil { if from.Truncate(time.Second).Before(earlyHdr.StartedAt.Truncate(time.Second)) { return Result{}, fmt.Errorf("check: `from` %s is before recording started at %s", @@ -259,6 +257,11 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { } } } else { + wantAll := len(cfg.IncludeLabels) > 0 || len(cfg.ExcludeLabels) > 0 || len(cfg.ExcludeAlerts) > 0 + allDefs, derr := loadDefinitions(ctx, src, cfg.namedAlerts(), wantAll) + if derr != nil { + return Result{}, fmt.Errorf("read rule definitions: %w", derr) + } resolved, notes, err = resolveAlertSet(allDefs, cfg.namedAlerts(), cfg.IncludeLabels, cfg.ExcludeLabels, cfg.ExcludeAlerts, cfg.Folder) } if err != nil { @@ -455,7 +458,7 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { return Result{}, err } // The authoritative header wins: the advisory read was only a fail-fast. - resolved, _, err = resolveFromLog(allDefs, header, cfg) + resolved, _, err = resolveFromLog(ctx, src, header, cfg) if err != nil { return Result{}, err } @@ -517,20 +520,75 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { // // Only the header-to-defs direction can fail: resolved is BUILT from the // header, so no resolved definition can be absent from it. -func resolveFromLog(allDefs []Definition, h Header, cfg Config) ([]Definition, []string, error) { +func resolveFromLog(ctx context.Context, src Source, h Header, cfg Config) ([]Definition, []string, error) { if h.URL != cfg.URL { return nil, nil, fmt.Errorf("log identity: %s recorded url %q but this run is configured for %q", cfg.Log, h.URL, cfg.URL) } - names := make([]string, 0, len(h.Rules)) + + // Group the header's rules by source: Grafana-managed rules resolve against + // a fresh ruler read, each datasource against one filtered rules request. + // The header names its sources, so discovery is skipped entirely. + var grafana []LoggedRule + dsRules := map[string][]LoggedRule{} + dsNames := map[string]string{} for _, lr := range h.Rules { - names = append(names, "uid:"+lr.UID) + if lr.DatasourceUID == "" { + grafana = append(grafana, lr) + continue + } + dsRules[lr.DatasourceUID] = append(dsRules[lr.DatasourceUID], lr) + dsNames[lr.DatasourceUID] = lr.DatasourceName } - resolved, notes, err := Resolve(allDefs, names, "") - if err != nil { - return nil, nil, fmt.Errorf("log identity: %s names a rule that no longer resolves: %w", cfg.Log, err) + + var resolved []Definition + if len(grafana) > 0 { + defs, err := src.GrafanaDefinitions(ctx) + if err != nil { + return nil, nil, fmt.Errorf("log identity: %w", err) + } + byKey := make(map[string]Definition, len(defs)) + for _, d := range defs { + byKey[defKey(d)] = d + } + for _, lr := range grafana { + d, ok := byKey[loggedKey(lr)] + if !ok { + return nil, nil, fmt.Errorf("log identity: %s names rule %s (%q), which no current definition matches", + cfg.Log, loggedKey(lr), lr.Title) + } + resolved = append(resolved, d) + } + } + + dsUIDs := make([]string, 0, len(dsRules)) + for uid := range dsRules { + dsUIDs = append(dsUIDs, uid) + } + sort.Strings(dsUIDs) + for _, uid := range dsUIDs { + names := make([]string, 0, len(dsRules[uid])) + for _, lr := range dsRules[uid] { + names = append(names, lr.Title) + } + defs, err := src.DatasourceDefinitions(ctx, RuleSource{UID: uid, Name: dsNames[uid]}, names) + if err != nil { + return nil, nil, fmt.Errorf("log identity: datasource %q: %w", dsNames[uid], err) + } + byKey := make(map[string]Definition, len(defs)) + for _, d := range defs { + byKey[defKey(d)] = d + } + for _, lr := range dsRules[uid] { + d, ok := byKey[loggedKey(lr)] + if !ok { + return nil, nil, fmt.Errorf("log identity: %s names rule %s (%q), which no current definition matches", + cfg.Log, loggedKey(lr), lr.Title) + } + resolved = append(resolved, d) + } } - return resolved, notes, nil + return resolved, nil, nil } // activeRules drops the rules whose DEFINITION says paused. They are skipped: @@ -554,7 +612,7 @@ func activeRules(defs []Definition) []Definition { func activeTimingsOf(active []Definition, rt map[string]RuleTimings) map[string]RuleTimings { out := make(map[string]RuleTimings, len(active)) for _, d := range active { - out[d.UID] = rt[d.UID] + out[defKey(d)] = rt[defKey(d)] } return out } @@ -567,7 +625,7 @@ type livePoller struct { src Source reducer *Reducer sched *Scheduler - titles map[string]string // uid -> title: poll by title, select by UID + refs map[string]RuleRef // key -> ref: how to find the rule again concurrency int } @@ -577,11 +635,11 @@ type livePoller struct { func newLivePoller(src Source, reducer *Reducer, active []Definition, rt map[string]RuleTimings, concurrency int, now time.Time, seed []Poll) *livePoller { - titles := make(map[string]string, len(active)) + refs := make(map[string]RuleRef, len(active)) cadence := make(map[string]time.Duration, len(active)) for _, d := range active { - titles[d.UID] = d.Title - cadence[d.UID] = rt[d.UID].pollEvery + refs[defKey(d)] = ruleRefOf(d) + cadence[defKey(d)] = rt[defKey(d)].pollEvery } sched := NewScheduler(cadence, now) if len(seed) > 0 { @@ -591,7 +649,7 @@ func newLivePoller(src Source, reducer *Reducer, active []Definition, rt map[str src: src, reducer: reducer, sched: sched, - titles: titles, + refs: refs, concurrency: concurrency, } } @@ -606,15 +664,15 @@ func newLivePoller(src Source, reducer *Reducer, active []Definition, rt map[str // collection is exit 2 and check discards the whole collection, so these come // back only to let the error say how far the run got before it stopped — // which is the one part of it an operator can act on. -func (p *livePoller) poll(ctx context.Context, uids []string) ([]Poll, error) { - observed, obsErr := observeAll(ctx, p.src, p.titles, uids, p.concurrency) - out := make([]Poll, 0, len(uids)) - for _, uid := range uids { - obs, ok := observed[uid] +func (p *livePoller) poll(ctx context.Context, keys []string) ([]Poll, error) { + observed, obsErr := observeAll(ctx, p.src, p.refs, keys, p.concurrency) + out := make([]Poll, 0, len(keys)) + for _, key := range keys { + obs, ok := observed[key] if !ok { continue } - out = append(out, p.reducer.Reduce(uid, obs)) + out = append(out, p.reducer.Reduce(key, obs)) } return out, obsErr } @@ -742,12 +800,15 @@ type drainVerdict struct { func drainWait(ctx context.Context, cfg Config, src Source, defs []Definition, pausedAtStart map[string]bool, rt map[string]RuleTimings, polls []Poll, windowEnd time.Time, timeout time.Duration) (map[string]drainVerdict, error) { - pending := make(map[string]string) // uid -> title, the shape observeAll wants + pending := make(map[string]string) // key -> title + refs := make(map[string]RuleRef, len(defs)) for _, d := range defs { - if pausedAtStart[d.UID] { + key := defKey(d) + refs[key] = ruleRefOf(d) + if pausedAtStart[key] { continue } - rulePolls := pollsForRule(polls, d.UID) + rulePolls := pollsForRule(polls, key) if n := len(rulePolls); n > 0 && !rulePolls[n-1].Found { continue } @@ -755,7 +816,7 @@ func drainWait(ctx context.Context, cfg Config, src Source, defs []Definition, p // recorded evaluations already reach past the end of the window has // answered the question, and polling it again asks nothing new. if !anyPollEvaluatedThrough(rulePolls, windowEnd) { - pending[d.UID] = d.Title + pending[key] = d.Title } } if len(pending) == 0 { @@ -774,7 +835,7 @@ func drainWait(ctx context.Context, cfg Config, src Source, defs []Definition, p } sort.Strings(uids) // deterministic request order and message order - observed, err := observeAll(ctx, src, pending, uids, cfg.Concurrency) + observed, err := observeAll(ctx, src, refs, uids, cfg.Concurrency) if err != nil { return nil, fmt.Errorf("drain wait: %w", err) } @@ -783,7 +844,7 @@ func drainWait(ctx context.Context, cfg Config, src Source, defs []Definition, p if !ok { continue } - rule := stateRuleByUID(obs.Rules, uid) + rule := stateRuleByKey(obs.Rules, uid) if rule == nil { // A 2xx that parsed and carries no matching rule is an // authoritative "the rule is gone" — the transport retried @@ -892,7 +953,7 @@ func mergeDrainTimeouts(res Result, drained map[string]drainVerdict) (Result, er var names []string for i := range res.Verdicts { - uid := res.Verdicts[i].RuleUID + uid := verdictKey(res.Verdicts[i]) verdict, ok := drained[uid] if !ok { continue diff --git a/grafana-alertcheck/internal/gate/check_ds_test.go b/grafana-alertcheck/internal/gate/check_ds_test.go new file mode 100644 index 000000000..636c65c49 --- /dev/null +++ b/grafana-alertcheck/internal/gate/check_ds_test.go @@ -0,0 +1,53 @@ +package gate + +import ( + "context" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// End-to-end through check: a datasource-managed rule that is bad at `from` and +// leaves the active set mid-window classifies as recovered, exit 0. +func TestCheck_DatasourceFireAndResolveIsRecovered(t *testing.T) { + clock := newVirtualClock(testNow) + from := testNow + to := testNow.Add(5 * time.Minute) + def := dsDef("A") + key := defKey(def) + + src := newCheckSource(nil) + src.defs = nil + src.ruleSources = []RuleSource{{UID: "vm", Name: "VM"}} + src.dsDefs = map[string][]Definition{"vm": {def}} + src.dsRespond = func(_ string, _ int) (Observation, error) { + now := clock.Now() + rule := StateRule{ + Key: key, DatasourceUID: "vm", Title: "A", Group: "G", Type: "alerting", + Health: "ok", LastEvaluation: now, + } + if now.Before(from.Add(2 * time.Minute)) { + rule.Instances = []Instance{{ + Labels: map[string]string{"x": "y"}, State: StateFiring, ActiveAt: from.Add(-time.Hour), + }} + } + return Observation{Rules: []StateRule{rule}, GrafanaNow: now, Latency: 100 * time.Millisecond}, nil + } + + cfg := Config{ + URL: "https://grafana.example.com", + Alerts: []string{"A"}, + From: from, + To: to, + Clock: clock, + Notes: &strings.Builder{}, + }.withDefaults() + + res, err := check(context.Background(), cfg, src) + require.NoError(t, err) + require.Empty(t, res.Violations) + require.Equal(t, OutcomeRecovered, res.Verdicts[0].Outcome) + require.Contains(t, res.Verdicts[0].Note, "check 7 skipped") +} diff --git a/grafana-alertcheck/internal/gate/check_test.go b/grafana-alertcheck/internal/gate/check_test.go index b53b12c3a..58c31e4fa 100644 --- a/grafana-alertcheck/internal/gate/check_test.go +++ b/grafana-alertcheck/internal/gate/check_test.go @@ -59,8 +59,12 @@ type checkSource struct { defs []Definition defsErr error - calls map[string]int - respond func(title string, call int) (Observation, error) + ruleSources []RuleSource + dsDefs map[string][]Definition + + calls map[string]int + respond func(title string, call int) (Observation, error) + dsRespond func(key string, call int) (Observation, error) } func newCheckSource(respond func(title string, call int) (Observation, error)) *checkSource { @@ -74,21 +78,51 @@ func newCheckSource(respond func(title string, call int) (Observation, error)) * func (s *checkSource) Version(context.Context) (string, error) { return s.version, s.versionErr } -func (s *checkSource) Definitions(context.Context) ([]Definition, error) { return s.defs, s.defsErr } +func (s *checkSource) GrafanaDefinitions(context.Context) ([]Definition, error) { + return s.defs, s.defsErr +} + +func (s *checkSource) DiscoverRuleSources(context.Context) ([]RuleSource, error) { + return s.ruleSources, nil +} + +func (s *checkSource) DatasourceDefinitions(_ context.Context, src RuleSource, names []string) ([]Definition, error) { + defs := s.dsDefs[src.UID] + if len(names) == 0 { + return defs, nil + } + want := make(map[string]bool, len(names)) + for _, n := range names { + want[n] = true + } + var out []Definition + for _, d := range defs { + if want[d.Title] { + out = append(out, d) + } + } + return out, nil +} // RuleState answers from the responder. A nil responder means the test // expects no state read at all — it fails with a message rather than a nil // dereference, because "this path must not poll" is an assertion several tests // here make on purpose. -func (s *checkSource) RuleState(_ context.Context, title string) (Observation, error) { +func (s *checkSource) RuleState(_ context.Context, ref RuleRef) (Observation, error) { + key := ref.Title + respond := s.respond + if ref.Kind == KindDatasourceManaged { + key = ref.Key + respond = s.dsRespond + } s.mu.Lock() - s.calls[title]++ - call := s.calls[title] + s.calls[key]++ + call := s.calls[key] s.mu.Unlock() - if s.respond == nil { - return Observation{}, fmt.Errorf("checkSource: this test expects no state read, but %q was polled", title) + if respond == nil { + return Observation{}, fmt.Errorf("checkSource: this test expects no state read, but %q was polled", key) } - return s.respond(title, call) + return respond(key, call) } func (s *checkSource) callCount(title string) int { diff --git a/grafana-alertcheck/internal/gate/classify.go b/grafana-alertcheck/internal/gate/classify.go index ab31edb3b..aa17e4d45 100644 --- a/grafana-alertcheck/internal/gate/classify.go +++ b/grafana-alertcheck/internal/gate/classify.go @@ -58,10 +58,13 @@ const ( // after the preexisting policy has been applied (isViolation below). type Violation struct { Alert, RuleUID string - Outcome Outcome - State State - Health string // raw, reporting-only, like Poll.Health - LastError string + // RuleKey is the identity across both source kinds; RuleUID is empty for a + // datasource-managed rule. + RuleKey string `json:"rule_key,omitempty"` + Outcome Outcome + State State + Health string // raw, reporting-only, like Poll.Health + LastError string // FirstSeen is the episode's onset in the runner domain (translated by the // poll's own skew), or `from` when preexisting — never a raw Grafana time. FirstSeen time.Time @@ -78,6 +81,7 @@ type Violation struct { // (passes included) so the table shows every alert asked for. type RuleVerdict struct { Alert, RuleUID string + RuleKey string `json:"rule_key,omitempty"` Outcome Outcome BadFor time.Duration // total wall-clock time any instance was bad inside the window, overlaps merged PollEvery time.Duration @@ -184,7 +188,7 @@ func runnerTime(p Poll, grafanaDomain time.Time) time.Time { // BadFor, and the Violations the preexisting policy charges against the run. // PURE: no I/O, no clock reads; polls need not be pre-filtered to this rule. func classifyRule(def Definition, polls []Poll, from, windowEnd time.Time, badStates map[State]bool, pol PreexistingPolicy) (Outcome, time.Duration, []Violation) { - rulePolls := pollsForRule(polls, def.UID) + rulePolls := pollsForRule(polls, defKey(def)) inWindow := inWindowPolls(rulePolls, from, windowEnd) timelines := make(map[string]*instanceTimeline) @@ -341,6 +345,7 @@ func classifyRule(def Definition, polls []Poll, from, windowEnd time.Time, badSt } viols = append(viols, Violation{ Alert: def.Title, + RuleKey: defKey(def), RuleUID: def.UID, Outcome: instOutcome, State: tl.lastState, @@ -434,10 +439,10 @@ func mergeDurations(eps []episode) time.Duration { // in a pure function. This is the single filter+sort implementation for the // package: proveCoverage calls it too, rather than keeping its own copy that // could silently drift from this one's membership test. -func pollsForRule(polls []Poll, uid string) []Poll { +func pollsForRule(polls []Poll, key string) []Poll { var out []Poll for _, p := range polls { - if p.RuleUID == uid { + if pollKey(p) == key { out = append(out, p) } } @@ -469,7 +474,7 @@ func applyNodataPolicy(def Definition, polls []Poll, cov *CoverageResult, t Rule if cov.Unobservable { return } - inWindow := inWindowPolls(pollsForRule(polls, def.UID), from, windowEnd) + inWindow := inWindowPolls(pollsForRule(polls, defKey(def)), from, windowEnd) runLen, sawAny := longestHealthRun(inWindow, "nodata") if !sawAny || runLen <= t.healthGrace { return @@ -549,25 +554,26 @@ func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition, pausedAtStart := h.pausedAtStart() for _, def := range defs { - if pausedAtStart[def.UID] { + key := defKey(def) + if pausedAtStart[key] { pausedRules = append(pausedRules, def) result.Verdicts = append(result.Verdicts, RuleVerdict{ - Alert: def.Title, RuleUID: def.UID, Outcome: OutcomePaused, - PollEvery: rt[def.UID].pollEvery, + Alert: def.Title, RuleKey: key, RuleUID: def.UID, Outcome: OutcomePaused, + PollEvery: rt[key].pollEvery, Note: "paused before the window opened", }) continue } watchedCount++ - t := rt[def.UID] + t := rt[key] cov := proveCoverage(h, polls, sentinel, t, def, pol.From, pol.To, gt.transitionGrace) if pol.NodataIsUnobservable { applyNodataPolicy(def, polls, &cov, t, pol.From, windowEnd) } - result.Coverage[def.UID] = cov - result.Thresholds[def.UID] = RuleThresholds{ + result.Coverage[key] = cov + result.Thresholds[key] = RuleThresholds{ MaxGap: t.maxGap, HealthGrace: t.healthGrace, EvalStaleAfter: t.evalStaleAfter, @@ -581,7 +587,7 @@ func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition, } result.Violations = append(result.Violations, viols...) result.Verdicts = append(result.Verdicts, RuleVerdict{ - Alert: def.Title, RuleUID: def.UID, Outcome: outcome, BadFor: badFor, + Alert: def.Title, RuleKey: key, RuleUID: def.UID, Outcome: outcome, BadFor: badFor, PollEvery: t.pollEvery, Note: strings.Join(cov.Notes, "; "), }) } @@ -607,7 +613,7 @@ func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition, // prints Note verbatim rather than re-deriving the hint, so the // exact wording here is what an operator reads. result.Violations = append(result.Violations, Violation{ - Alert: def.Title, RuleUID: def.UID, Outcome: OutcomePaused, + Alert: def.Title, RuleKey: defKey(def), RuleUID: def.UID, Outcome: OutcomePaused, Note: "paused before the window opened; counts against --min-observed unless --allow-paused is set", }) attributed++ diff --git a/grafana-alertcheck/internal/gate/coverage.go b/grafana-alertcheck/internal/gate/coverage.go index 977c45c9e..e9b0e083e 100644 --- a/grafana-alertcheck/internal/gate/coverage.go +++ b/grafana-alertcheck/internal/gate/coverage.go @@ -63,7 +63,7 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t RuleTimings, d // pollsForRule (classify.go) is the single filter+sort implementation; this // and classifyRule must not carry two independent copies. - rulePolls := pollsForRule(polls, def.UID) + rulePolls := pollsForRule(polls, defKey(def)) var res CoverageResult fail := func(reason UnobservableReason, note string) { @@ -173,19 +173,28 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t RuleTimings, d // Check 7 — isPaused in-window. The PRIMARY pause detector: liveness // (check 6) is only the backup for what IsPaused cannot show (a deleted // rule, a stopped scheduler, a blocked evaluation). This is what catches - // pause-then-unpause, which the drain wait alone passes. - var pausedCount int - var pausedAt time.Time - for _, p := range inWindow { - if p.IsPaused { - pausedCount++ - if pausedAt.IsZero() { - pausedAt = p.GrafanaNow + // pause-then-unpause, which the drain wait alone passes. A + // datasource-managed rule has no pause signal at all, so the check is + // skipped with an explicit note rather than passed silently. + if def.Kind == KindDatasourceManaged && !def.PauseObservable { + res.Notes = append(res.Notes, fmt.Sprintf( + "rule %q: pause is not observable for a datasource-managed rule; check 7 skipped", def.Title)) + res.Notes = append(res.Notes, fmt.Sprintf( + "rule %q: a datasource-managed instance leaving the active set is treated as a recovery (a vanished series is indistinguishable from a resolution)", def.Title)) + } else { + var pausedCount int + var pausedAt time.Time + for _, p := range inWindow { + if p.IsPaused { + pausedCount++ + if pausedAt.IsZero() { + pausedAt = p.GrafanaNow + } } } - } - if pausedCount > 0 { - fail(ReasonPausedInWindow, fmt.Sprintf("observed paused on %d poll(s), first at %s", pausedCount, pausedAt.Format(time.RFC3339))) + if pausedCount > 0 { + fail(ReasonPausedInWindow, fmt.Sprintf("observed paused on %d poll(s), first at %s", pausedCount, pausedAt.Format(time.RFC3339))) + } } // Check 8 — rule absent. Found==false is authoritative (the transport @@ -213,7 +222,7 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t RuleTimings, d // comma-joined, so membership via reasonsContain, never a literal index). nds, ees := def.NoDataState, def.ExecErrState for _, lr := range h.Rules { - if lr.UID == def.UID { + if loggedKey(lr) == defKey(def) { nds, ees = lr.NoDataState, lr.ExecErrState break } diff --git a/grafana-alertcheck/internal/gate/datasource_semantics_test.go b/grafana-alertcheck/internal/gate/datasource_semantics_test.go new file mode 100644 index 000000000..6eab70851 --- /dev/null +++ b/grafana-alertcheck/internal/gate/datasource_semantics_test.go @@ -0,0 +1,108 @@ +package gate + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func dsDef(name string) Definition { + return Definition{ + Key: ruleKey("vm", "G", name, ""), Title: name, Group: "G", + Kind: KindDatasourceManaged, DatasourceUID: "vm", DatasourceName: "VM", + IntervalSeconds: 60, + } +} + +// dsPoll is a datasource poll with the fields the pure layer reads. +func dsPoll(key string, at time.Time, health string) Poll { + return Poll{RuleKey: key, GrafanaNow: at, Found: true, Health: health, LastEvaluation: at} +} + +// A datasource instance that is bad at `from` and then leaves the active set is +// a recovery: Reduce turns the departure into Cleared, so classifyRule sees a +// real clear and the run passes. +func TestDecide_DatasourceDepartureIsRecovered(t *testing.T) { + from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + to := from.Add(10 * time.Minute) + def := dsDef("A") + key := defKey(def) + rt := map[string]RuleTimings{key: newRuleTimings(30*time.Second, 60)} + pol := Policy{From: from, To: to} + + var polls []Poll + for ts := from; !ts.After(to); ts = ts.Add(30 * time.Second) { + switch { + case ts.Equal(from): + polls = append(polls, Poll{ + RuleKey: key, GrafanaNow: ts, Found: true, Health: "ok", LastEvaluation: ts, + Abnormal: []Instance{{Labels: map[string]string{"x": "y"}, State: StateFiring, ActiveAt: from.Add(-time.Hour)}}, + }) + case ts.Equal(from.Add(5 * time.Minute)): + p := dsPoll(key, ts, "ok") + p.Cleared = []string{instanceKey(map[string]string{"x": "y"})} + polls = append(polls, p) + default: + polls = append(polls, dsPoll(key, ts, "ok")) + } + } + sentinel := to + res, err := decide(Header{StartedAt: from.Add(-time.Hour)}, polls, &sentinel, []Definition{def}, rt, GlobalTimings{}, pol) + require.NoError(t, err) + require.Empty(t, res.Violations) + require.Equal(t, OutcomeRecovered, res.Verdicts[0].Outcome) + require.Contains(t, res.Verdicts[0].Note, "treated as a recovery") +} + +// The same shape for a Grafana rule, but a VANISH rather than a clear, stays +// still_failing: a disappearing series must not read as a recovery. +func TestDecide_GrafanaVanishStaysFailing(t *testing.T) { + from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + to := from.Add(10 * time.Minute) + def := Definition{Key: "r1", UID: "r1", Title: "R1"} + rt := map[string]RuleTimings{"r1": newRuleTimings(30*time.Second, 60)} + pol := Policy{From: from, To: to} + + var polls []Poll + for ts := from; !ts.After(to); ts = ts.Add(30 * time.Second) { + if ts.Equal(from) { + polls = append(polls, Poll{ + RuleUID: "r1", RuleKey: "r1", GrafanaNow: ts, Found: true, Health: "ok", LastEvaluation: ts, + Abnormal: []Instance{{Labels: map[string]string{"x": "y"}, State: StateFiring, ActiveAt: from.Add(-time.Hour)}}, + }) + continue + } + p := quietPoll("r1", ts) + if ts.Equal(from.Add(5 * time.Minute)) { + p.Vanished = []string{instanceKey(map[string]string{"x": "y"})} + } + polls = append(polls, p) + } + sentinel := to + res, err := decide(Header{StartedAt: from.Add(-time.Hour)}, polls, &sentinel, []Definition{def}, rt, GlobalTimings{}, pol) + require.NoError(t, err) + require.NotEmpty(t, res.Violations, "a vanish must stay a failure") + require.Equal(t, OutcomeStillFailing, res.Verdicts[0].Outcome) +} + +// A datasource health=err normalizes to "error" and, sustained past +// healthGrace, makes the rule unobservable through the ordinary check 4. +func TestDecide_DatasourceHealthErrIsUnobservable(t *testing.T) { + from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + to := from.Add(10 * time.Minute) + def := dsDef("A") + key := defKey(def) + rt := map[string]RuleTimings{key: newRuleTimings(30*time.Second, 60)} + pol := Policy{From: from, To: to} + + var polls []Poll + for ts := from; !ts.After(to); ts = ts.Add(30 * time.Second) { + polls = append(polls, dsPoll(key, ts, "error")) + } + sentinel := to + res, err := decide(Header{StartedAt: from.Add(-time.Hour)}, polls, &sentinel, []Definition{def}, rt, GlobalTimings{}, pol) + require.Error(t, err) + require.Equal(t, OutcomeNotVerified, res.Verdicts[0].Outcome) + require.Equal(t, ReasonHealthError, res.Coverage[key].Reason) +} diff --git a/grafana-alertcheck/internal/gate/handoff.go b/grafana-alertcheck/internal/gate/handoff.go index d5a0ff0db..716bf06b3 100644 --- a/grafana-alertcheck/internal/gate/handoff.go +++ b/grafana-alertcheck/internal/gate/handoff.go @@ -59,16 +59,17 @@ func handoffProblems(t map[string]RuleTimings, measured map[string]time.Duration } jobs := make([]job, 0, len(t)) for _, p := range first { - rt, ok := t[p.RuleUID] + key := pollKey(p) + rt, ok := t[key] if !ok { continue } - m, ok := measured[p.RuleUID] + m, ok := measured[key] if !ok { - return nil, fmt.Errorf("startup handoff: rule %s was never measured", ruleLabel(rt.title, p.RuleUID)) + return nil, fmt.Errorf("startup handoff: rule %s was never measured", ruleLabel(rt.title, key)) } jobs = append(jobs, job{ - uid: p.RuleUID, + uid: key, title: rt.title, due: runnerTime(p, p.GrafanaNow).Add(rt.pollEvery), latency: m, diff --git a/grafana-alertcheck/internal/gate/identity.go b/grafana-alertcheck/internal/gate/identity.go new file mode 100644 index 000000000..361243530 --- /dev/null +++ b/grafana-alertcheck/internal/gate/identity.go @@ -0,0 +1,62 @@ +package gate + +import "encoding/json" + +// dsKeyPrefix marks a datasource-managed key. The key, not the prefix, is the +// identity; the prefix only lets a caller that has lost the Definition (an +// absent-rule poll, a log read) still tell the two source kinds apart. +const dsKeyPrefix = "ds:" + +// ruleKey is the one map key for a rule across both source kinds. Grafana-managed +// rules keep their uid. Datasource-managed rules have no uid, so they get a +// JSON-encoded tuple; JSON keeps group/name separators from colliding. This is a +// key, not an identity the API gave us — Definition.UID stays empty for ds rules. +func ruleKey(dsUID, group, name, uid string) string { + if uid != "" { + return uid + } + b, _ := json.Marshal([3]string{dsUID, group, name}) + return dsKeyPrefix + string(b) +} + +// defKey is a Definition's map key: Key when set, UID otherwise (a Definition +// built directly by a test may carry only UID). +func defKey(d Definition) string { + if d.Key != "" { + return d.Key + } + return d.UID +} + +// loggedKey is a LoggedRule's map key, mirroring defKey. +func loggedKey(lr LoggedRule) string { + if lr.Key != "" { + return lr.Key + } + return lr.UID +} + +// pollKey is a Poll's map key: rule_key when written, rule_uid otherwise (a v1 +// log written before rule_key existed). +func pollKey(p Poll) string { + if p.RuleKey != "" { + return p.RuleKey + } + return p.RuleUID +} + +// stateRuleKey is a StateRule's map key, mirroring defKey. +func stateRuleKey(r StateRule) string { + if r.Key != "" { + return r.Key + } + return r.UID +} + +// verdictKey is a RuleVerdict's map key, mirroring defKey. +func verdictKey(v RuleVerdict) string { + if v.RuleKey != "" { + return v.RuleKey + } + return v.RuleUID +} diff --git a/grafana-alertcheck/internal/gate/identity_test.go b/grafana-alertcheck/internal/gate/identity_test.go new file mode 100644 index 000000000..8099e6f91 --- /dev/null +++ b/grafana-alertcheck/internal/gate/identity_test.go @@ -0,0 +1,40 @@ +package gate + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestRuleKey_GrafanaKeepsUID(t *testing.T) { + require.Equal(t, "rule1", ruleKey("", "", "title", "rule1")) +} + +// A ds key must not collide with a Grafana uid and must not let group/name +// separators collide: a name containing ":" or "/" is still a distinct tuple. +func TestRuleKey_DatasourceInjectivity(t *testing.T) { + keys := []string{ + ruleKey("dsA", "g", "n", ""), + ruleKey("dsA", "g/n", "", ""), + ruleKey("dsA", "g", "/n", ""), + ruleKey("dsB", "g", "n", ""), + ruleKey("", "g", "n", ""), + } + seen := map[string]bool{} + for _, k := range keys { + require.True(t, len(k) > len(dsKeyPrefix) && k[:len(dsKeyPrefix)] == dsKeyPrefix) + require.False(t, seen[k], "key %q collided", k) + seen[k] = true + } + require.Equal(t, "u1", ruleKey("dsA", "g", "n", "u1"), "a uid wins over the ds tuple") +} + +func TestDefKey_FallsBackToUID(t *testing.T) { + require.Equal(t, "u1", defKey(Definition{UID: "u1"})) + require.Equal(t, "k1", defKey(Definition{Key: "k1", UID: "u1"})) +} + +func TestPollKey_FallsBackToUID(t *testing.T) { + require.Equal(t, "u1", pollKey(Poll{RuleUID: "u1"})) + require.Equal(t, "k1", pollKey(Poll{RuleKey: "k1", RuleUID: "u1"})) +} diff --git a/grafana-alertcheck/internal/gate/labels.go b/grafana-alertcheck/internal/gate/labels.go index ce8e60810..b9c018d4f 100644 --- a/grafana-alertcheck/internal/gate/labels.go +++ b/grafana-alertcheck/internal/gate/labels.go @@ -25,7 +25,7 @@ func SelectByLabels(defs []Definition, include, exclude []LabelMatcher) ([]Defin continue } matchedInclude++ - if d.Kind != KindGrafanaManaged { + if d.Kind == KindRecording || (d.Kind == KindDatasourceManaged && d.DatasourceUID == "") { return nil, fmt.Errorf("label selection matches %q, a %s, which is not supported", d.Title, kindName(d.Kind)) } if matchesAny(d.Labels, exclude) { @@ -81,11 +81,11 @@ func subtractExcluded(all, selected []Definition, excludeAlerts []string, folder } drop := make(map[string]bool, len(excluded)) for _, d := range excluded { - drop[d.UID] = true + drop[defKey(d)] = true } out := make([]Definition, 0, len(selected)) for _, d := range selected { - if !drop[d.UID] { + if !drop[defKey(d)] { out = append(out, d) } } diff --git a/grafana-alertcheck/internal/gate/load.go b/grafana-alertcheck/internal/gate/load.go new file mode 100644 index 000000000..cc15c9af7 --- /dev/null +++ b/grafana-alertcheck/internal/gate/load.go @@ -0,0 +1,74 @@ +package gate + +import ( + "context" + "fmt" + "strings" +) + +// ListAllDefinitions is the CLI's entry point for `list`: every Grafana-managed +// and datasource-managed definition, discovered from scratch. +func ListAllDefinitions(ctx context.Context, src Source) ([]Definition, error) { + return loadDefinitions(ctx, src, nil, true) +} + +// loadDefinitions reads Grafana-managed definitions from the ruler and +// datasource-managed definitions from every discovered rule source. wantAll +// fetches every ds rule (label selection, list); otherwise only the named rules +// are requested, one request per source. Ruler-returned datasource-managed rules +// are dropped: discovery is the authority for them, since only it knows the +// datasource UID. +func loadDefinitions(ctx context.Context, src Source, names []string, wantAll bool) ([]Definition, error) { + grafana, err := src.GrafanaDefinitions(ctx) + if err != nil { + return nil, err + } + sources, err := src.DiscoverRuleSources(ctx) + if err != nil { + return nil, err + } + + defs := make([]Definition, 0, len(grafana)) + for _, d := range grafana { + if d.Kind == KindDatasourceManaged { + continue + } + defs = append(defs, d) + } + + filter, fetchAll := dsFilterNames(names) + if wantAll { + fetchAll = true + } + for _, rs := range sources { + var dsDefs []Definition + if fetchAll { + dsDefs, err = src.DatasourceDefinitions(ctx, rs, nil) + } else { + dsDefs, err = src.DatasourceDefinitions(ctx, rs, filter) + } + if err != nil { + return nil, fmt.Errorf("datasource %q: %w", rs.Name, err) + } + defs = append(defs, dsDefs...) + } + return defs, nil +} + +// dsFilterNames extracts the server-side rule_name[] filters from the raw alert +// names. A key: or uid: form names no title, so the whole source must be +// fetched; otherwise the last /-separated segment is the title. +func dsFilterNames(names []string) (titles []string, fetchAll bool) { + for _, raw := range names { + n := strings.TrimSpace(raw) + if n == "" { + continue + } + if strings.HasPrefix(n, "key:") || strings.HasPrefix(n, "uid:") { + return nil, true + } + parts := strings.Split(n, "/") + titles = append(titles, parts[len(parts)-1]) + } + return titles, false +} diff --git a/grafana-alertcheck/internal/gate/log.go b/grafana-alertcheck/internal/gate/log.go index 68854e359..6187dfae4 100644 --- a/grafana-alertcheck/internal/gate/log.go +++ b/grafana-alertcheck/internal/gate/log.go @@ -36,10 +36,19 @@ const missingSeriesReason = "MissingSeries" // the header URL it IS the log's identity, which check validates, and it // supplies the alert set in check mode. type LoggedRule struct { - UID string `json:"uid"` - Title string `json:"title"` - Folder string `json:"folder"` - Group string `json:"group"` + // Key is the rule's identity across both source kinds; uid stays empty for + // datasource-managed rules. SourceKind, DatasourceUID/Name and File are + // additive (schema 1) and let a later check re-resolve a ds rule without + // discovery. + Key string `json:"key,omitempty"` + UID string `json:"uid"` + Title string `json:"title"` + Folder string `json:"folder"` + Group string `json:"group"` + SourceKind string `json:"source_kind,omitempty"` + DatasourceUID string `json:"datasource_uid,omitempty"` + DatasourceName string `json:"datasource_name,omitempty"` + File string `json:"file,omitempty"` // ForSeconds, IntervalSeconds, NoDataState and ExecErrState are purely // forensic: a resolve-time snapshot that makes the uploaded artifact // self-describing to a human reading it after the runner is gone. check @@ -104,7 +113,7 @@ func (h Header) readyAt() time.Time { func (h Header) pausedAtStart() map[string]bool { paused := make(map[string]bool, len(h.Rules)) for _, lr := range h.Rules { - paused[lr.UID] = lr.IsPaused + paused[loggedKey(lr)] = lr.IsPaused } return paused } @@ -112,7 +121,12 @@ func (h Header) pausedAtStart() map[string]bool { // Poll is one reduced observation of one rule — the log's heartbeat and the // only input the pure coverage and classification layers ever see. type Poll struct { - RuleUID string `json:"rule_uid"` + // RuleKey is the map key across both source kinds; RuleUID is kept for + // compatibility and is empty for datasource-managed rules. pollKey reads + // RuleKey when present, else RuleUID, so a v1 log written before rule_key + // stays readable. + RuleKey string `json:"rule_key,omitempty"` + RuleUID string `json:"rule_uid,omitempty"` GrafanaNow time.Time `json:"grafana_now"` // the response's Date header // SkewMS, SkewBoundMS and LatencyMS are milliseconds for JSONL // compactness ONLY. The pure layer never touches raw ms: it reads @@ -196,23 +210,28 @@ func (r *Reducer) Reduce(uid string, obs Observation) Poll { defer r.mu.Unlock() p := Poll{ - RuleUID: uid, + RuleKey: uid, GrafanaNow: obs.GrafanaNow, SkewMS: obs.Skew.Milliseconds(), SkewBoundMS: obs.SkewBound.Milliseconds(), LatencyMS: obs.Latency.Milliseconds(), } - rule := stateRuleByUID(obs.Rules, uid) + rule := stateRuleByKey(obs.Rules, uid) if rule == nil { // An authoritative "the rule is absent". No markers are computed and // the previous abnormal set is kept untouched: if the rule comes back // with an instance missing, the next poll still reports that instance - // as vanished rather than losing the transition entirely. + // as vanished rather than losing the transition entirely. The uid is + // kept for a Grafana rule (uid == key); a datasource key is not a uid. + if !strings.HasPrefix(uid, dsKeyPrefix) { + p.RuleUID = uid + } return p } p.Found = true + p.RuleUID = rule.UID p.State = rule.State p.Health = rule.Health p.LastError = rule.LastError @@ -246,6 +265,12 @@ func (r *Reducer) Reduce(uid string, obs Observation) Poll { } inst, found := present[key] switch { + case !found && rule.DatasourceUID != "": + // A datasource-managed response carries only active instances, so + // an instance leaving it IS the resolution. Documented weaker + // guarantee: a vanished series is indistinguishable from a + // recovery, and is treated as one. + p.Cleared = append(p.Cleared, key) case !found: // Fully absent from the response: a discontinuity, not a recovery. p.Vanished = append(p.Vanished, key) @@ -291,17 +316,17 @@ func (r *Reducer) seedFrom(polls []Poll) { for _, inst := range p.Abnormal { keys[instanceKey(inst.Labels)] = struct{}{} } - r.prevAbnormal[p.RuleUID] = keys + r.prevAbnormal[pollKey(p)] = keys } } -// stateRuleByUID picks one rule out of a state response BY UID (nil = the +// stateRuleByKey picks one rule out of a state response BY KEY (nil = the // authoritative "rule absent"). Never by title: the ?rule_name= filter is a // title filter and can return several rules sharing a title. The single // selection for the package — Reduce and the drain wait both use it. -func stateRuleByUID(rules []StateRule, uid string) *StateRule { +func stateRuleByKey(rules []StateRule, key string) *StateRule { for i := range rules { - if rules[i].UID == uid { + if stateRuleKey(rules[i]) == key { return &rules[i] } } @@ -440,7 +465,7 @@ func (w *Writer) WritePoll(p Poll) error { return fmt.Errorf("log writer already stopped") } if err := w.enc.Encode(pollRecord{Type: RecordPoll, Poll: p}); err != nil { - return fmt.Errorf("write poll for rule %s: %w", p.RuleUID, err) + return fmt.Errorf("write poll for rule %s: %w", pollKey(p), err) } return nil } diff --git a/grafana-alertcheck/internal/gate/log_ds_test.go b/grafana-alertcheck/internal/gate/log_ds_test.go new file mode 100644 index 000000000..16dc9f0bb --- /dev/null +++ b/grafana-alertcheck/internal/gate/log_ds_test.go @@ -0,0 +1,77 @@ +package gate + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func dsRule(uid, name string, insts ...Instance) StateRule { + return StateRule{ + Key: ruleKey("vm", "G", name, ""), DatasourceUID: "vm", + Title: name, Group: "G", Type: "alerting", Health: "ok", + LastEvaluation: testNow, Instances: insts, + } +} + +// A datasource instance leaving the active set IS a resolution: vmalert only +// returns active instances, so a departure is Cleared, never Vanished. +func TestReduce_DatasourceDepartureIsCleared(t *testing.T) { + firing := Instance{Labels: map[string]string{"x": "y"}, State: StateFiring, ActiveAt: testNow} + r := NewReducer() + key := ruleKey("vm", "G", "A", "") + r.Reduce(key, observation(testNow, dsRule("", "A", firing))) + p := r.Reduce(key, observation(testNow.Add(time.Minute), dsRule("", "A"))) + require.Equal(t, []string{instanceKey(firing.Labels)}, p.Cleared) + require.Empty(t, p.Vanished) +} + +// The same shape for a Grafana rule stays Vanished: a disappearing series must +// never read as a recovery. +func TestReduce_GrafanaDepartureIsVanished(t *testing.T) { + firing := Instance{Labels: map[string]string{"x": "y"}, State: StateFiring, ActiveAt: testNow} + r := NewReducer() + r.Reduce("r1", observation(testNow, StateRule{UID: "r1", Title: "A", Health: "ok", LastEvaluation: testNow, Instances: []Instance{firing}})) + p := r.Reduce("r1", observation(testNow.Add(time.Minute), StateRule{UID: "r1", Title: "A", Health: "ok", LastEvaluation: testNow.Add(time.Minute)})) + require.Empty(t, p.Cleared) + require.Equal(t, []string{instanceKey(firing.Labels)}, p.Vanished) +} + +// A datasource poll records the key but no uid. +func TestReduce_DatasourcePollCarriesKeyNotUID(t *testing.T) { + r := NewReducer() + key := ruleKey("vm", "G", "A", "") + p := r.Reduce(key, observation(testNow, dsRule("", "A"))) + require.Equal(t, key, p.RuleKey) + require.Empty(t, p.RuleUID) +} + +// A v1 log written before rule_key existed still reads: pollKey falls back to +// rule_uid. +func TestReadLog_OldPollWithoutRuleKey(t *testing.T) { + p := Poll{RuleUID: "rule1", GrafanaNow: testNow, Found: true} + require.Equal(t, "rule1", pollKey(p)) +} + +// The recorder's child rebuilds a datasource poll ref from the header alone. +func TestChildSchedule_DatasourceRef(t *testing.T) { + def := dsDef("A") + rt := map[string]RuleTimings{defKey(def): newRuleTimings(30*time.Second, 60)} + h := Header{StartedAt: testNow, Rules: loggedRules([]Definition{def}, rt)} + require.Equal(t, "datasource", h.Rules[0].SourceKind) + require.Equal(t, "vm", h.Rules[0].DatasourceUID) + + refs, cadence, err := childSchedule(h) + require.NoError(t, err) + ref := refs[defKey(def)] + require.Equal(t, KindDatasourceManaged, ref.Kind) + require.Equal(t, "vm", ref.DatasourceUID) + require.Equal(t, "G", ref.Group) + require.Equal(t, "A", ref.Name) + require.Equal(t, 30*time.Second, cadence[defKey(def)]) + + if _, _, err := DeriveTimingsFromLog(h, []Definition{def}); err != nil { + t.Fatalf("DeriveTimingsFromLog with a datasource rule: %v", err) + } +} diff --git a/grafana-alertcheck/internal/gate/parse_datasource.go b/grafana-alertcheck/internal/gate/parse_datasource.go new file mode 100644 index 000000000..02d1ed978 --- /dev/null +++ b/grafana-alertcheck/internal/gate/parse_datasource.go @@ -0,0 +1,190 @@ +package gate + +import ( + "encoding/json" + "fmt" + "strings" + "time" +) + +// ParseDatasourceRules parses a Prometheus/vmalert rules response +// (/api/prometheus/{uid}/api/v1/rules) into StateRules. It shares parseInstance +// with the Grafana parser but uses the datasource vocabulary: lowercase instance +// states, health "err" (not "error"), and a zero lastEvaluation is allowed +// (liveness treats zero as maximally stale). A missing or unparseable required +// field is an error, never a zero value. +func ParseDatasourceRules(body []byte, dsUID, dsName string) ([]StateRule, error) { + var top map[string]json.RawMessage + if err := json.Unmarshal(body, &top); err != nil { + return nil, fmt.Errorf("datasource rules response: %w", err) + } + var dataRaw json.RawMessage + if err := req(top, "data", &dataRaw); err != nil { + return nil, fmt.Errorf("datasource rules response: %w", err) + } + var data map[string]json.RawMessage + if err := json.Unmarshal(dataRaw, &data); err != nil { + return nil, fmt.Errorf("datasource rules response: data: %w", err) + } + var groupsRaw []json.RawMessage + if err := req(data, "groups", &groupsRaw); err != nil { + return nil, fmt.Errorf("datasource rules response: %w", err) + } + + var rules []StateRule + for gi, groupRaw := range groupsRaw { + var group map[string]json.RawMessage + if err := json.Unmarshal(groupRaw, &group); err != nil { + return nil, fmt.Errorf("datasource rules response: group %d: %w", gi, err) + } + var groupName, file string + if err := req(group, "name", &groupName); err != nil { + return nil, fmt.Errorf("datasource rules response: group %d: %w", gi, err) + } + if err := opt(group, "file", &file); err != nil { + return nil, fmt.Errorf("datasource rules response: group %q: %w", groupName, err) + } + var intervalSeconds float64 + if err := opt(group, "interval", &intervalSeconds); err != nil { + return nil, fmt.Errorf("datasource rules response: group %q: %w", groupName, err) + } + interval := time.Duration(intervalSeconds * float64(time.Second)) + + var rulesRaw []json.RawMessage + if err := req(group, "rules", &rulesRaw); err != nil { + return nil, fmt.Errorf("datasource rules response: group %q: %w", groupName, err) + } + for ri, ruleRaw := range rulesRaw { + rule, err := parseDatasourceRule(ruleRaw, dsUID, groupName, file, interval) + if err != nil { + return nil, fmt.Errorf("datasource rules response: group %q: rule %d: %w", groupName, ri, err) + } + rules = append(rules, rule) + } + } + return rules, nil +} + +func parseDatasourceRule(raw json.RawMessage, dsUID, group, file string, interval time.Duration) (StateRule, error) { + var m map[string]json.RawMessage + if err := json.Unmarshal(raw, &m); err != nil { + return StateRule{}, err + } + var name, ruleType string + if err := req(m, "name", &name); err != nil { + return StateRule{}, err + } + if err := req(m, "type", &ruleType); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } + + r := StateRule{ + Key: ruleKey(dsUID, group, name, ""), + Title: name, + Group: group, + File: file, + Interval: interval, + DatasourceUID: dsUID, + Type: ruleType, + } + if err := opt(m, "query", &r.Query); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } + if err := opt(m, "labels", &r.Labels); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } + var durationSeconds float64 + if err := opt(m, "duration", &durationSeconds); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } + r.For = time.Duration(durationSeconds * float64(time.Second)) + + // Recording rules carry no state, health or alerts; DefinitionsFromDatasource + // drops them, so parsing only the shared fields keeps a recording rule from + // failing on fields it was never going to have. + if ruleType == "recording" { + return r, nil + } + + if err := req(m, "health", &r.Health); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } + if r.Health == "err" { + r.Health = "error" + } + var lastEvalStr string + if err := opt(m, "lastEvaluation", &lastEvalStr); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } + if lastEvalStr != "" { + lastEval, err := time.Parse(time.RFC3339, lastEvalStr) + if err != nil { + return StateRule{}, fmt.Errorf("rule %q: lastEvaluation: %w", name, err) + } + r.LastEvaluation = lastEval + } + + if err := opt(m, "state", &r.State); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } + var alertsRaw []json.RawMessage + if err := opt(m, "alerts", &alertsRaw); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } + instances := make([]Instance, 0, len(alertsRaw)) + for ii, ar := range alertsRaw { + inst, err := parseInstanceWith(ar, normalizeDatasourceInstanceState) + if err != nil { + return StateRule{}, fmt.Errorf("rule %q: instance %d: %w", name, ii, err) + } + instances = append(instances, inst) + } + r.Instances = instances + return r, nil +} + +// datasourceInstanceStates is the strict datasource instance-state vocabulary. +// Only the two active states exist — a resolved instance is absent from the +// response, not reported as normal. +var datasourceInstanceStates = map[string]State{ + "firing": StateFiring, + "pending": StatePending, +} + +func normalizeDatasourceInstanceState(s string) (State, string, error) { + base, reason := s, "" + if i := strings.Index(s, " ("); i != -1 && strings.HasSuffix(s, ")") { + base, reason = s[:i], s[i+2:len(s)-1] + } + state, ok := datasourceInstanceStates[base] + if !ok { + return "", "", fmt.Errorf("unrecognized datasource instance state %q", s) + } + return state, reason, nil +} + +// DefinitionsFromDatasource converts datasource rule states into Definitions, +// keeping only alerting rules. A datasource-managed rule has no pause signal and +// no uid, so PauseObservable is false and UID stays empty. +func DefinitionsFromDatasource(rules []StateRule, dsUID, dsName string) []Definition { + var defs []Definition + for _, r := range rules { + if r.Type != "alerting" { + continue + } + defs = append(defs, Definition{ + Key: r.Key, + Title: r.Title, + Group: r.Group, + File: r.File, + For: r.For, + Labels: r.Labels, + Kind: KindDatasourceManaged, + DatasourceUID: dsUID, + DatasourceName: dsName, + PauseObservable: false, + IntervalSeconds: int(r.Interval / time.Second), + }) + } + return defs +} diff --git a/grafana-alertcheck/internal/gate/parse_datasource_test.go b/grafana-alertcheck/internal/gate/parse_datasource_test.go new file mode 100644 index 000000000..6ee448999 --- /dev/null +++ b/grafana-alertcheck/internal/gate/parse_datasource_test.go @@ -0,0 +1,76 @@ +package gate + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestParseDatasourceRules_Fixture(t *testing.T) { + rules, err := ParseDatasourceRules(readFixture(t, "ds_rules.json"), "ds-uid", "ExampleMetrics") + require.NoError(t, err) + require.Len(t, rules, 2, "recording rules are parsed but filtered later") + + alert := rules[0] + require.Equal(t, "ExampleTargetDown", alert.Title) + require.Equal(t, "ExampleMetrics", alert.Group) + require.Equal(t, "/etc/vm/rules/example.yml", alert.File) + require.Equal(t, "ds-uid", alert.DatasourceUID) + require.Equal(t, "alerting", alert.Type) + require.Equal(t, "up == 0", alert.Query) + require.Equal(t, 5*time.Minute, alert.For) + require.Equal(t, "firing", alert.State) + require.Equal(t, "ok", alert.Health) + require.Empty(t, alert.UID, "a datasource rule has no uid") + require.Equal(t, ruleKey("ds-uid", "ExampleMetrics", "ExampleTargetDown", ""), alert.Key) + require.Len(t, alert.Instances, 1) + require.Equal(t, StateFiring, alert.Instances[0].State) + require.Nil(t, alert.Totals) +} + +func TestDefinitionsFromDatasource_FiltersRecording(t *testing.T) { + rules, err := ParseDatasourceRules(readFixture(t, "ds_rules.json"), "ds-uid", "ExampleMetrics") + require.NoError(t, err) + + defs := DefinitionsFromDatasource(rules, "ds-uid", "ExampleMetrics") + require.Len(t, defs, 1, "only the alerting rule becomes a Definition") + require.Equal(t, KindDatasourceManaged, defs[0].Kind) + require.Equal(t, "ExampleMetrics", defs[0].DatasourceName) + require.False(t, defs[0].PauseObservable) + require.Equal(t, 60, defs[0].IntervalSeconds) +} + +func TestParseDatasourceRules_HealthErrNormalizes(t *testing.T) { + body := []byte(`{"status":"success","data":{"groups":[{"name":"g","file":"f","interval":60,"rules":[ + {"name":"A","type":"alerting","health":"err","lastEvaluation":"2026-08-01T00:00:00Z","state":"firing"}]}]}}`) + rules, err := ParseDatasourceRules(body, "d", "n") + require.NoError(t, err) + require.Equal(t, "error", rules[0].Health) +} + +func TestParseDatasourceRules_ZeroLastEvaluationAllowed(t *testing.T) { + body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ + {"name":"A","type":"alerting","health":"ok","state":"pending"}]}]}}`) + rules, err := ParseDatasourceRules(body, "d", "n") + require.NoError(t, err) + require.True(t, rules[0].LastEvaluation.IsZero()) +} + +func TestParseDatasourceRules_UnknownInstanceStateIsError(t *testing.T) { + body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ + {"name":"A","type":"alerting","health":"ok","state":"firing","alerts":[ + {"labels":{},"state":"inactive","activeAt":"2026-08-01T00:00:00Z"}]}]}]}}`) + _, err := ParseDatasourceRules(body, "d", "n") + require.Error(t, err) + require.Contains(t, err.Error(), "unrecognized datasource instance state") +} + +func TestParseDatasourceRules_PendingInstance(t *testing.T) { + body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ + {"name":"A","type":"alerting","health":"ok","state":"pending","alerts":[ + {"labels":{"x":"y"},"state":"pending","activeAt":"2026-08-01T00:00:00Z","value":"1"}]}]}]}}`) + rules, err := ParseDatasourceRules(body, "d", "n") + require.NoError(t, err) + require.Equal(t, StatePending, rules[0].Instances[0].State) +} diff --git a/grafana-alertcheck/internal/gate/parse_ruler.go b/grafana-alertcheck/internal/gate/parse_ruler.go index 4a6b1bbbd..340c2fe08 100644 --- a/grafana-alertcheck/internal/gate/parse_ruler.go +++ b/grafana-alertcheck/internal/gate/parse_ruler.go @@ -27,14 +27,23 @@ const ( // relativeTimeRange and keep_firing_for are deliberately not parsed: nothing in // the gate reads them. type Definition struct { + // Key is the map key across both source kinds (identity.go). UID stays the + // API-given uid and is empty for datasource-managed rules. + Key string UID, Title, Folder, FolderUID, Group string - For time.Duration - IntervalSeconds int - NoDataState string - ExecErrState string - IsPaused bool - Kind RuleKind - Labels map[string]string + // DatasourceUID, DatasourceName and File are populated for + // KindDatasourceManaged only; File is the Prometheus rule group's file. + DatasourceUID, DatasourceName, File string + For time.Duration + IntervalSeconds int + NoDataState string + ExecErrState string + IsPaused bool + Kind RuleKind + // PauseObservable is true only for Grafana-managed rules, whose state + // endpoint reports isPaused. A datasource-managed rule has no pause signal. + PauseObservable bool + Labels map[string]string } // ParseDefinitions strictly parses a ruler-endpoint response body @@ -140,7 +149,7 @@ func parseDefinition(raw json.RawMessage, folder, group string) (Definition, err if err := req(ga, "uid", &uid); err != nil { return Definition{}, fmt.Errorf("grafana_alert: %w", err) } - def := Definition{Folder: folder, Group: group, For: forDur, UID: uid, Labels: labels} + def := Definition{Key: uid, Folder: folder, Group: group, For: forDur, UID: uid, Labels: labels} // Classify by the presence of "record" before requiring anything else. // no_data_state/exec_err_state/is_paused/intervalSeconds are alerting-only @@ -172,6 +181,7 @@ func parseDefinition(raw json.RawMessage, folder, group string) (Definition, err } def.Kind = KindGrafanaManaged + def.PauseObservable = true if err := req(ga, "title", &def.Title); err != nil { return Definition{}, fmt.Errorf("rule %q: grafana_alert: %w", uid, err) } diff --git a/grafana-alertcheck/internal/gate/parse_state.go b/grafana-alertcheck/internal/gate/parse_state.go index 7c35c7e7d..3f8448bca 100644 --- a/grafana-alertcheck/internal/gate/parse_state.go +++ b/grafana-alertcheck/internal/gate/parse_state.go @@ -39,8 +39,19 @@ type Instance struct { // StateRule is one rule from the state endpoint // (/api/prometheus/grafana/api/v1/rules), fully and strictly parsed. type StateRule struct { + // Key is the map key across both source kinds (identity.go); UID is empty + // for datasource-managed rules. + Key string UID, Title, Folder, Group string - Interval time.Duration + // DatasourceUID and File are set for datasource-managed rules only; Type is + // the Prometheus rule type ("alerting"/"recording"); Query and For carry the + // Prometheus definition. + DatasourceUID string + File string + Type, Query string + For time.Duration + Labels map[string]string + Interval time.Duration // State and Health are raw, lowercase, and reporting-only — never // classified. State in particular is never normalized. State, Health string @@ -125,7 +136,7 @@ func parseStateRule(raw json.RawMessage, folder, group string, interval time.Dur return StateRule{}, fmt.Errorf("rule %q: %w", uid, err) } - r := StateRule{UID: uid, Title: name, Folder: folder, Group: group, Interval: interval} + r := StateRule{Key: uid, UID: uid, Title: name, Folder: folder, Group: group, Interval: interval, Type: "alerting"} if err := req(m, "state", &r.State); err != nil { return StateRule{}, fmt.Errorf("rule %q: %w", uid, err) @@ -183,6 +194,13 @@ func parseStateRule(raw json.RawMessage, folder, group string, interval time.Dur } func parseInstance(raw json.RawMessage) (Instance, error) { + return parseInstanceWith(raw, normalizeInstanceState) +} + +// parseInstanceWith is parseInstance with the state normalizer injected, so the +// datasource parser reuses the same strict field decoding with its own +// vocabulary. +func parseInstanceWith(raw json.RawMessage, normalize func(string) (State, string, error)) (Instance, error) { var m map[string]json.RawMessage if err := json.Unmarshal(raw, &m); err != nil { return Instance{}, fmt.Errorf("%w", err) @@ -192,7 +210,7 @@ func parseInstance(raw json.RawMessage) (Instance, error) { if err := req(m, "state", &rawState); err != nil { return Instance{}, err } - state, reason, err := normalizeInstanceState(rawState) + state, reason, err := normalize(rawState) if err != nil { return Instance{}, err } diff --git a/grafana-alertcheck/internal/gate/resolve.go b/grafana-alertcheck/internal/gate/resolve.go index 165f25e3c..030357864 100644 --- a/grafana-alertcheck/internal/gate/resolve.go +++ b/grafana-alertcheck/internal/gate/resolve.go @@ -12,8 +12,8 @@ import ( // // 1. Trim each name. // 2. Discard empty lines. -// 3. Resolve each name to a UID (this is what resolveOne does). -// 4. Collapse the result by UID — two names hitting the same rule is a note, +// 3. Resolve each name to a rule (this is what resolveOne does). +// 4. Collapse the result by key — two names hitting the same rule is a note, // never an error (almost always a copy mistake, and a message costs the // user less than a failure). // @@ -22,7 +22,7 @@ import ( // len(names) — using the input line count would make one rule named twice turn // an achievable default into an unsatisfiable one. func Resolve(defs []Definition, names []string, folder string) (resolved []Definition, notes []string, err error) { - seenUID := map[string]string{} // uid -> the first input name that resolved to it + seenKey := map[string]string{} // key -> the first input name that resolved to it for _, raw := range names { name := strings.TrimSpace(raw) if name == "" { @@ -34,12 +34,13 @@ func Resolve(defs []Definition, names []string, folder string) (resolved []Defin return nil, nil, rerr } - if firstName, ok := seenUID[def.UID]; ok { + key := defKey(def) + if firstName, ok := seenKey[key]; ok { notes = append(notes, fmt.Sprintf( - "%q and %q both resolve to %s (uid:%s); counted once", firstName, name, def.Title, def.UID)) + "%q and %q both resolve to %s (%s); counted once", firstName, name, def.Title, ruleRefLabel(def))) continue } - seenUID[def.UID] = name + seenKey[key] = name resolved = append(resolved, def) } return resolved, notes, nil @@ -47,22 +48,27 @@ func Resolve(defs []Definition, names []string, folder string) (resolved []Defin // resolveOne resolves a single trimmed, non-empty name against defs: one match // wins outright, zero is an error with suggestions, two or more is an error -// listing every candidate. folder scopes a bare title (no "/" in the name) to -// one folder; it is ignored for the "Folder/Title" and "Folder/Group/Title" -// forms, which already name their own folder. +// listing every candidate. folder scopes a bare Grafana title to one folder; it +// is ignored for the /-separated forms and for datasource-managed rules, which +// name their own group and datasource. // -// Unsupported kinds (datasource-managed, recording) are refused, and how that -// interacts with the no-match/ambiguous surfaces is decided here: a name can -// still match an unsupported rule (so -// naming one by title still gets the specific, named refusal, not a bare "no -// match"), but only *supported* candidates count for ambiguity — an -// unsupported rule sharing a title with a supported one is resolved silently -// in the supported rule's favor rather than reported as ambiguous — and the -// "%d rules available" count and substring suggestions in a genuine no-match -// are scoped to supported rules only, so an unsupported rule never inflates -// or pollutes either. uid: is always exact regardless of kind (typically -// copy-pasted from `list`, which already shows Kind). +// A name matches both kinds: Grafana forms are Title | Folder/Title | +// Folder/Group/Title, datasource forms are Title | Group/Title | +// DatasourceName/Group/Title, and an exact key: is unambiguous across both. +// Unsupported kinds (recording, and a datasource-managed rule with no +// datasource) are refused, and only supported candidates count for ambiguity. func resolveOne(defs []Definition, name, folder string) (Definition, error) { + if key, ok := strings.CutPrefix(name, "key:"); ok { + if key != "" { + for _, d := range defs { + if defKey(d) == key { + return refuseUnsupportedKind(name, d) + } + } + } + return Definition{}, fmt.Errorf("no rule matched %q: no rule has this key (run 'grafana-alertcheck list' to see keys)", name) + } + if uid, ok := strings.CutPrefix(name, "uid:"); ok { if uid != "" { for _, d := range defs { @@ -83,26 +89,20 @@ func resolveOne(defs []Definition, name, folder string) (Definition, error) { return Definition{}, fmt.Errorf("no rule matched %q: no rule has this uid (run 'grafana-alertcheck list' to see uids)", name) } - wantFolder, wantGroup, wantTitle, err := classifyForm(name, folder) + parts, err := parseNameForm(name) if err != nil { return Definition{}, err } var supportedCandidates, unsupportedCandidates []Definition for _, d := range defs { - if wantFolder != "" && d.Folder != wantFolder { - continue - } - if wantGroup != "" && d.Group != wantGroup { + if !matchesName(d, parts, folder) { continue } - if d.Title != wantTitle { - continue - } - if d.Kind == KindGrafanaManaged { - supportedCandidates = append(supportedCandidates, d) - } else { + if d.Kind == KindRecording || (d.Kind == KindDatasourceManaged && d.DatasourceUID == "") { unsupportedCandidates = append(unsupportedCandidates, d) + } else { + supportedCandidates = append(supportedCandidates, d) } } @@ -114,97 +114,126 @@ func resolveOne(defs []Definition, name, folder string) (Definition, error) { case len(unsupportedCandidates) > 0: return refuseUnsupportedKind(name, unsupportedCandidates[0]) default: - return Definition{}, noMatchError(supportedDefs(defs), name, wantTitle) + return Definition{}, noMatchError(supportedDefs(defs), name, parts[len(parts)-1]) + } +} + +// matchesName reports whether d matches the /-separated name form. A bare +// Grafana title is scoped by folder; datasource forms never use folder. +func matchesName(d Definition, parts []string, folder string) bool { + switch len(parts) { + case 1: + if d.Kind == KindGrafanaManaged { + return (folder == "" || d.Folder == folder) && d.Title == parts[0] + } + return d.Title == parts[0] + case 2: + if d.Kind == KindGrafanaManaged { + return d.Folder == parts[0] && d.Title == parts[1] + } + return d.Group == parts[0] && d.Title == parts[1] + case 3: + if d.Kind == KindGrafanaManaged { + return d.Folder == parts[0] && d.Group == parts[1] && d.Title == parts[2] + } + return d.DatasourceName == parts[0] && d.Group == parts[1] && d.Title == parts[2] } + return false } -// supportedDefs filters out the two refused kinds. Only these participate in -// name-based matching, the no-match rule count, and substring suggestions (see -// the policy note on resolveOne). +// supportedDefs filters out the refused kinds. Only these participate in +// name-based matching, the no-match rule count, and substring suggestions. func supportedDefs(defs []Definition) []Definition { out := make([]Definition, 0, len(defs)) for _, d := range defs { - if d.Kind == KindGrafanaManaged { - out = append(out, d) + if d.Kind == KindRecording || (d.Kind == KindDatasourceManaged && d.DatasourceUID == "") { + continue } + out = append(out, d) } return out } -// classifyForm splits name into the Title | Folder/Title | Folder/Group/Title -// forms. A bare title is scoped by folder when the caller supplied one; -// the two- and three-segment forms already carry their own folder and ignore -// it. -// -// Every segment must be non-empty. Without this, "/Title" would parse as an -// empty wantFolder — silently dropping the folder filter and matching -// unscoped, a fail-open — and "Folder/" would parse as an empty wantTitle, -// which would then feed noMatchError's substring search an empty needle that -// matches every title. -func classifyForm(name, folder string) (wantFolder, wantGroup, wantTitle string, err error) { +// parseNameForm splits name into 1..3 /-separated segments. Every segment must +// be non-empty: without this, "/Title" would parse as an empty first segment — +// silently dropping the filter and matching unscoped, a fail-open — and +// "Folder/" would parse as an empty title, feeding noMatchError's substring +// search an empty needle that matches every title. +func parseNameForm(name string) ([]string, error) { parts := strings.Split(name, "/") if slices.Contains(parts, "") { - return "", "", "", fmt.Errorf("no rule matched %q: empty /-separated segment (want Title, Folder/Title, or Folder/Group/Title)", name) + return nil, fmt.Errorf("no rule matched %q: empty /-separated segment (want Title, Group/Title, or Datasource/Group/Title)", name) } - switch len(parts) { - case 1: - return folder, "", parts[0], nil - case 2: - return parts[0], "", parts[1], nil - case 3: - return parts[0], parts[1], parts[2], nil - default: - return "", "", "", fmt.Errorf("no rule matched %q: too many /-separated segments (want Title, Folder/Title, or Folder/Group/Title)", name) + if len(parts) > 3 { + return nil, fmt.Errorf("no rule matched %q: too many /-separated segments (want Title, Group/Title, or Datasource/Group/Title)", name) } + return parts, nil } -// refuseUnsupportedKind rejects the two unsupported kinds with a clear, -// specific error — distinct from "no match" and from "ambiguous" — so -// an operator who names a recording or datasource-managed rule learns why, -// not just that nothing matched. +// refuseUnsupportedKind rejects the unsupported kinds with a clear, specific +// error — distinct from "no match" and from "ambiguous". func refuseUnsupportedKind(name string, d Definition) (Definition, error) { - switch d.Kind { - case KindDatasourceManaged: - return Definition{}, fmt.Errorf("%q resolves to %s, a datasource-managed rule, which is not supported", name, d.Title) - case KindRecording: + switch { + case d.Kind == KindRecording: return Definition{}, fmt.Errorf("%q resolves to %s, a recording rule, which is not supported", name, d.Title) + case d.Kind == KindDatasourceManaged && d.DatasourceUID == "": + return Definition{}, fmt.Errorf("%q resolves to %s, a datasource-managed rule whose datasource is unknown, which is not supported", name, d.Title) default: return d, nil } } -// noMatchError reports a no-match with the count of grafana-managed rules and -// case-insensitive substring suggestions. The trailing disclaimer covers rules -// the ruler response omits entirely, namely datasource-managed ones. +// ruleRefLabel names a rule for a note: a uid for Grafana, a copyable key for a +// datasource-managed rule. +func ruleRefLabel(d Definition) string { + if d.UID != "" { + return "uid:" + d.UID + } + return "key:" + defKey(d) +} + +// noMatchError reports a no-match with the count of supported rules and +// case-insensitive substring suggestions. func noMatchError(defs []Definition, name, wantTitle string) error { - msg := fmt.Sprintf("no rule matched %q (%d grafana-managed rules available; run 'grafana-alertcheck list' to see titles)", + msg := fmt.Sprintf("no rule matched %q (%d rules available; run 'grafana-alertcheck list' to see titles)", name, len(defs)) needle := strings.ToLower(wantTitle) var subs []string for _, d := range defs { if strings.Contains(strings.ToLower(d.Title), needle) { - subs = append(subs, fmt.Sprintf("%s/%s/%s", d.Folder, d.Group, d.Title)) + subs = append(subs, suggestionLabel(d)) } } if len(subs) > 0 { sort.Strings(subs) msg += fmt.Sprintf("; did you mean: %s", strings.Join(subs, ", ")) } - msg += "; datasource-managed alert rules cannot be observed and are not supported" return fmt.Errorf("%s", msg) } -// ambiguousError lists every candidate with its folder, its group, and the -// full copyable Folder/Group/Title — including the uid: form, which resolves -// unambiguously on the next attempt. +// suggestionLabel is the copyable name form for a supported rule. +func suggestionLabel(d Definition) string { + if d.Kind == KindDatasourceManaged { + return fmt.Sprintf("%s/%s/%s", d.DatasourceName, d.Group, d.Title) + } + return fmt.Sprintf("%s/%s/%s", d.Folder, d.Group, d.Title) +} + +// ambiguousError lists every candidate with its source, its group, and the full +// copyable name — including the key:/uid: form, which resolves unambiguously on +// the next attempt. func ambiguousError(name string, candidates []Definition) error { sorted := append([]Definition(nil), candidates...) - sort.Slice(sorted, func(i, j int) bool { return sorted[i].UID < sorted[j].UID }) + sort.Slice(sorted, func(i, j int) bool { return defKey(sorted[i]) < defKey(sorted[j]) }) var b strings.Builder - fmt.Fprintf(&b, "%q matches %d rules; use uid: or the full Folder/Group/Title:", name, len(sorted)) + fmt.Fprintf(&b, "%q matches %d rules; use key: or the full name:", name, len(sorted)) for _, d := range sorted { + if d.Kind == KindDatasourceManaged { + fmt.Fprintf(&b, "\n %s/%s/%s (datasource %s, key:%s)", d.DatasourceName, d.Group, d.Title, d.DatasourceName, defKey(d)) + continue + } fmt.Fprintf(&b, "\n %s/%s/%s (uid:%s)", d.Folder, d.Group, d.Title, d.UID) } return fmt.Errorf("%s", b.String()) diff --git a/grafana-alertcheck/internal/gate/resolve_test.go b/grafana-alertcheck/internal/gate/resolve_test.go index bb5088a06..0be60f6ea 100644 --- a/grafana-alertcheck/internal/gate/resolve_test.go +++ b/grafana-alertcheck/internal/gate/resolve_test.go @@ -61,7 +61,6 @@ func TestResolve_NoMatch(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "no rule matched") require.Contains(t, err.Error(), "list") - require.Contains(t, err.Error(), "datasource-managed alert rules cannot be observed") } func TestResolve_NoMatchSubstringSuggestion(t *testing.T) { @@ -124,7 +123,7 @@ func TestResolve_UnsupportedKindsExcludedFromNoMatchSurfaces(t *testing.T) { _, _, err = Resolve(combined, []string{"Example"}, "") require.Error(t, err, "want a no-match error for a name matching no title exactly") - wantCount := fmt.Sprintf("(%d grafana-managed rules available", len(supported)) + wantCount := fmt.Sprintf("(%d rules available", len(supported)) require.Contains(t, err.Error(), wantCount) require.NotContains(t, err.Error(), "ExampleTargetDown") require.NotContains(t, err.Error(), "example:recorded_metric:rate5m") @@ -197,6 +196,41 @@ func TestResolve_EmptyAndBlankLinesDiscarded(t *testing.T) { require.Equal(t, "rule0000007", resolved[0].UID) } +func dsResolveDef(ds, dsName, group, title string) Definition { + return Definition{ + Key: ruleKey(ds, group, title, ""), Title: title, Group: group, + Kind: KindDatasourceManaged, DatasourceUID: ds, DatasourceName: dsName, + } +} + +func TestResolve_DatasourceFormsAndKey(t *testing.T) { + defs := []Definition{dsResolveDef("vm", "VictoriaMetrics - Prod", "ExampleMetrics", "ExampleTargetDown")} + + for _, name := range []string{ + "ExampleTargetDown", + "ExampleMetrics/ExampleTargetDown", + "VictoriaMetrics - Prod/ExampleMetrics/ExampleTargetDown", + "key:" + ruleKey("vm", "ExampleMetrics", "ExampleTargetDown", ""), + } { + resolved, _, err := Resolve(defs, []string{name}, "") + require.NoErrorf(t, err, "name %q", name) + require.Len(t, resolved, 1) + require.Equal(t, "vm", resolved[0].DatasourceUID) + } +} + +func TestResolve_DatasourceAmbiguityAcrossSources(t *testing.T) { + defs := []Definition{ + dsResolveDef("vm-a", "A", "G", "Same"), + dsResolveDef("vm-b", "B", "G", "Same"), + } + _, _, err := Resolve(defs, []string{"Same"}, "") + require.Error(t, err) + require.Contains(t, err.Error(), "matches 2 rules") + require.Contains(t, err.Error(), "A") + require.Contains(t, err.Error(), "B") +} + func TestResolve_FolderScopesBareTitle(t *testing.T) { defs := rulerDefs(t) // Bare title, scoped to the wrong folder — must not match. diff --git a/grafana-alertcheck/internal/gate/schedule.go b/grafana-alertcheck/internal/gate/schedule.go index 65bb7d419..66cf0814b 100644 --- a/grafana-alertcheck/internal/gate/schedule.go +++ b/grafana-alertcheck/internal/gate/schedule.go @@ -87,7 +87,7 @@ func DeriveTimings(defs []Definition, override time.Duration) (rules map[string] } rt := newRuleTimings(pollEvery, d.IntervalSeconds) rt.title = d.Title - rules[d.UID] = rt + rules[defKey(d)] = rt } // In this mode defs ARE the start-of-step snapshot, so they answer what was // paused at the window open; only the log-mode counterpart uses the header. @@ -100,7 +100,7 @@ func DeriveTimings(defs []Definition, override time.Duration) (rules map[string] func pausedSet(defs []Definition) map[string]bool { paused := make(map[string]bool, len(defs)) for _, d := range defs { - paused[d.UID] = d.IsPaused + paused[defKey(d)] = d.IsPaused } return paused } @@ -119,31 +119,32 @@ func pausedSet(defs []Definition) map[string]bool { // It checks only the header-to-defs direction. A definition absent from the // header is Check's log-identity validation to judge, not this function's. func DeriveTimingsFromLog(h Header, defs []Definition) (rules map[string]RuleTimings, global GlobalTimings, err error) { - byUID := make(map[string]Definition, len(defs)) + byKey := make(map[string]Definition, len(defs)) for _, d := range defs { - byUID[d.UID] = d + byKey[defKey(d)] = d } rules = make(map[string]RuleTimings, len(h.Rules)) for _, lr := range h.Rules { - def, ok := byUID[lr.UID] + key := loggedKey(lr) + def, ok := byKey[key] if !ok { return nil, GlobalTimings{}, fmt.Errorf( - "log header names rule %s (%q), which no current definition matches", lr.UID, lr.Title) + "log header names rule %s (%q), which no current definition matches", key, lr.Title) } - if _, duplicate := rules[lr.UID]; duplicate { + if _, duplicate := rules[key]; duplicate { return nil, GlobalTimings{}, fmt.Errorf( - "log header names rule %s (%q) twice; its recorded cadence is ambiguous", lr.UID, lr.Title) + "log header names rule %s (%q) twice; its recorded cadence is ambiguous", key, lr.Title) } if lr.PollEverySeconds <= 0 { return nil, GlobalTimings{}, fmt.Errorf( "log header records poll_every_seconds=%v for rule %s (%q); the recorded cadence is required to derive maxGap", - lr.PollEverySeconds, lr.UID, lr.Title) + lr.PollEverySeconds, key, lr.Title) } pollEvery := time.Duration(lr.PollEverySeconds * float64(time.Second)) rt := newRuleTimings(pollEvery, def.IntervalSeconds) rt.title = def.Title // the current title: the header's may predate a rename - rules[lr.UID] = rt + rules[key] = rt } // The header, not defs, decides which rules are excluded from the grace: // defs were resolved after the window closed. See deriveGlobalTimings. @@ -167,7 +168,7 @@ func deriveGlobalTimings(defs []Definition, pausedAtStart map[string]bool) Globa if interval > maxInterval { maxInterval = interval } - if pausedAtStart[d.UID] { + if pausedAtStart[defKey(d)] { continue } if candidate := d.For + interval; candidate > g.transitionGrace { @@ -230,12 +231,13 @@ func NewSchedulerFromPolls(every map[string]time.Duration, polls []Poll, now tim } last := make(map[string]time.Time, len(every)) for _, p := range polls { - if _, owned := every[p.RuleUID]; !owned || p.GrafanaNow.IsZero() { + key := pollKey(p) + if _, owned := every[key]; !owned || p.GrafanaNow.IsZero() { continue } at := runnerTime(p, p.GrafanaNow) - if cur, ok := last[p.RuleUID]; !ok || at.After(cur) { - last[p.RuleUID] = at + if cur, ok := last[key]; !ok || at.After(cur) { + last[key] = at } } for uid, pollEvery := range every { diff --git a/grafana-alertcheck/internal/gate/source.go b/grafana-alertcheck/internal/gate/source.go index 18051160c..345efb6d2 100644 --- a/grafana-alertcheck/internal/gate/source.go +++ b/grafana-alertcheck/internal/gate/source.go @@ -75,13 +75,44 @@ func (e *RetryExhaustedError) Error() string { return fmt.Sprintf("gave up after %d sequential failures: %v", e.Failures, e.Cause) } +// RuleSource is one discovered datasource that can serve alerting rules. +type RuleSource struct{ UID, Name string } + +// RuleRef is everything a poll needs to find one rule again: the identity key, +// the source kind, and the exact filters each source API expects. +type RuleRef struct { + Key string + Kind RuleKind + DatasourceUID string // "" = Grafana-managed + UID string // Grafana-managed only + Group, Name, File, Title string +} + +// ruleRefOf narrows a Definition to the poll identity. +func ruleRefOf(d Definition) RuleRef { + return RuleRef{ + Key: defKey(d), + Kind: d.Kind, + DatasourceUID: d.DatasourceUID, + UID: d.UID, + Group: d.Group, + Name: d.Title, + File: d.File, + Title: d.Title, + } +} + // Source is everything the gate reads from Grafana. httpSource is the one // production implementation; the tests use a scripted fake // (source_fake_test.go) instead of real HTTP. type Source interface { Version(ctx context.Context) (string, error) - Definitions(ctx context.Context) ([]Definition, error) - RuleState(ctx context.Context, title string) (Observation, error) + GrafanaDefinitions(ctx context.Context) ([]Definition, error) + DiscoverRuleSources(ctx context.Context) ([]RuleSource, error) + // DatasourceDefinitions names empty = fetch every rule; names non-empty = + // one request with repeated rule_name[] params. + DatasourceDefinitions(ctx context.Context, src RuleSource, names []string) ([]Definition, error) + RuleState(ctx context.Context, ref RuleRef) (Observation, error) } // grafanaVersion is a parsed major.minor.patch triple. @@ -196,7 +227,7 @@ func (s *httpSource) Version(ctx context.Context) (string, error) { }) } -func (s *httpSource) Definitions(ctx context.Context) ([]Definition, error) { +func (s *httpSource) GrafanaDefinitions(ctx context.Context) ([]Definition, error) { return retryTransport(ctx, s.clock, s.maxSequentialFailures, s.backoffBase, s.backoffCap, func() ([]Definition, error) { r, err := s.doRequest(ctx, "/api/ruler/grafana/api/v1/rules") if err != nil { @@ -210,14 +241,100 @@ func (s *httpSource) Definitions(ctx context.Context) ([]Definition, error) { }) } -func (s *httpSource) RuleState(ctx context.Context, title string) (Observation, error) { - path := "/api/prometheus/grafana/api/v1/rules?rule_name=" + url.QueryEscape(title) +// DiscoverRuleSources lists every datasource that can serve Prometheus-flavored +// alerting rules. The filter is strict — type=="prometheus" AND +// jsonData.manageAlerts==true — because the AlertStateHistoryBackend datasource +// shares VictoriaMetrics' backend and would otherwise make every rule name +// ambiguous. Each candidate is probed; a probe failure is a hard error naming +// the datasource, since a silently dropped source is a fail-open. +func (s *httpSource) DiscoverRuleSources(ctx context.Context) ([]RuleSource, error) { + return retryTransport(ctx, s.clock, s.maxSequentialFailures, s.backoffBase, s.backoffCap, func() ([]RuleSource, error) { + r, err := s.doRequest(ctx, "/api/datasources") + if err != nil { + return nil, datasourceReadError(err) + } + var listed []struct { + UID string `json:"uid"` + Name string `json:"name"` + Type string `json:"type"` + JSONData struct { + ManageAlerts bool `json:"manageAlerts"` + } `json:"jsonData"` + } + if err := json.Unmarshal(r.Body, &listed); err != nil { + return nil, &TransportError{Err: fmt.Errorf("parse /api/datasources: %w", err)} + } + var out []RuleSource + for _, d := range listed { + if d.Type != "prometheus" || !d.JSONData.ManageAlerts { + continue + } + if err := s.probeRuleSource(ctx, d.UID); err != nil { + return nil, fmt.Errorf("datasource %q (%s) has manageAlerts=true but its rule API is unusable: %w", d.Name, d.UID, err) + } + out = append(out, RuleSource{UID: d.UID, Name: d.Name}) + } + return out, nil + }) +} + +// datasourceReadError names the missing permission on a 403/400, since that is +// the one operator action the error can suggest. +func datasourceReadError(err error) error { + return fmt.Errorf("list datasources (requires datasources:read plus datasource query permission): %w", err) +} + +// probeRuleSource confirms a candidate serves rules: a 200 with no groups for a +// probe name means a working, permitted rule API. A non-2xx is returned as-is. +func (s *httpSource) probeRuleSource(ctx context.Context, uid string) error { + path := "/api/prometheus/" + url.PathEscape(uid) + "/api/v1/rules" + datasourceQuery([]string{"__probe__"}, "", "") + _, err := s.doRequest(ctx, path) + return err +} + +func (s *httpSource) DatasourceDefinitions(ctx context.Context, src RuleSource, names []string) ([]Definition, error) { + path := "/api/prometheus/" + url.PathEscape(src.UID) + "/api/v1/rules" + datasourceQuery(names, "", "") + return retryTransport(ctx, s.clock, s.maxSequentialFailures, s.backoffBase, s.backoffCap, func() ([]Definition, error) { + r, err := s.doRequest(ctx, path) + if err != nil { + return nil, err + } + rules, parseErr := ParseDatasourceRules(r.Body, src.UID, src.Name) + if parseErr != nil { + return nil, &TransportError{Err: fmt.Errorf("parse datasource rule definitions: %w", parseErr)} + } + return DefinitionsFromDatasource(rules, src.UID, src.Name), nil + }) +} + +// datasourceQuery builds the vmalert filter query. The keys are literally +// rule_name[], rule_group[] and file[] — vmalert reads only the []-suffixed +// forms and ignores plain rule_name= (pinned by a unit test). +func datasourceQuery(names []string, group, file string) string { + if len(names) == 0 && group == "" && file == "" { + return "" + } + v := url.Values{} + for _, n := range names { + v.Add("rule_name[]", n) + } + if group != "" { + v.Add("rule_group[]", group) + } + if file != "" { + v.Add("file[]", file) + } + return "?" + v.Encode() +} + +func (s *httpSource) RuleState(ctx context.Context, ref RuleRef) (Observation, error) { + path, parse := s.ruleStateRequest(ref) return retryTransport(ctx, s.clock, s.maxSequentialFailures, s.backoffBase, s.backoffCap, func() (Observation, error) { r, err := s.doRequest(ctx, path) if err != nil { return Observation{}, err } - rules, parseErr := ParseState(r.Body) + rules, parseErr := parse(r.Body) if parseErr != nil { // Treated as transient, not a schema break: an unparseable 2xx // is far more likely a mid-stream hiccup than a permanent shape @@ -235,6 +352,20 @@ func (s *httpSource) RuleState(ctx context.Context, title string) (Observation, }) } +// ruleStateRequest picks the endpoint and parser for one ref. Grafana selects by +// title (the ?rule_name= filter can return several rules sharing a title, so the +// caller selects by key); a datasource rule is filtered by name, group and file +// so the response carries exactly that rule. +func (s *httpSource) ruleStateRequest(ref RuleRef) (string, func([]byte) ([]StateRule, error)) { + if ref.Kind == KindDatasourceManaged { + path := "/api/prometheus/" + url.PathEscape(ref.DatasourceUID) + "/api/v1/rules" + + datasourceQuery([]string{ref.Name}, ref.Group, ref.File) + return path, func(b []byte) ([]StateRule, error) { return ParseDatasourceRules(b, ref.DatasourceUID, "") } + } + path := "/api/prometheus/grafana/api/v1/rules?rule_name=" + url.QueryEscape(ref.Title) + return path, ParseState +} + // requestResult is the outcome of one successful HTTP attempt in doRequest: // the raw body plus everything derived from timing the round trip against // the response's own clock. diff --git a/grafana-alertcheck/internal/gate/source_ds_test.go b/grafana-alertcheck/internal/gate/source_ds_test.go new file mode 100644 index 000000000..77f29b43a --- /dev/null +++ b/grafana-alertcheck/internal/gate/source_ds_test.go @@ -0,0 +1,139 @@ +package gate + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// The []-suffixed keys are an upstream vmalert quirk: plain rule_name= is +// ignored, so the raw query must carry the bracketed forms. +func TestDatasourceQuery_BracketedKeys(t *testing.T) { + require.Empty(t, datasourceQuery(nil, "", ""), "no filters means the bulk request") + got := datasourceQuery([]string{"A"}, "G", "F") + require.Equal(t, "?file%5B%5D=F&rule_group%5B%5D=G&rule_name%5B%5D=A", got) +} + +func TestDiscoverRuleSources_StrictFilterAndProbe(t *testing.T) { + datasources := `[ + {"uid":"vm","name":"VictoriaMetrics - Prod","type":"prometheus","jsonData":{"manageAlerts":true}}, + {"uid":"ash","name":"AlertStateHistoryBackend","type":"prometheus","jsonData":{"manageAlerts":false}}, + {"uid":"loki","name":"Loki","type":"loki","jsonData":{"manageAlerts":true}} + ]` + var probed []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/datasources": + _, _ = w.Write([]byte(datasources)) + case "/api/prometheus/vm/api/v1/rules": + probed = append(probed, r.URL.RawQuery) + _, _ = w.Write([]byte(`{"status":"success","data":{"groups":[]}}`)) + default: + t.Errorf("unexpected path %q", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer srv.Close() + + src := NewHTTPSource(srv.URL, "", newFakeClock(time.Now())) + got, err := src.DiscoverRuleSources(context.Background()) + require.NoError(t, err) + require.Equal(t, []RuleSource{{UID: "vm", Name: "VictoriaMetrics - Prod"}}, got) + require.Equal(t, []string{"rule_name%5B%5D=__probe__"}, probed) +} + +func TestDiscoverRuleSources_ProbeFailureIsHardError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/datasources": + _, _ = w.Write([]byte(`[{"uid":"vm","name":"VictoriaMetrics - Prod","type":"prometheus","jsonData":{"manageAlerts":true}}]`)) + default: + w.WriteHeader(http.StatusForbidden) + } + })) + defer srv.Close() + + src := NewHTTPSource(srv.URL, "", newFakeClock(time.Now())) + _, err := src.DiscoverRuleSources(context.Background()) + require.Error(t, err) + require.Contains(t, err.Error(), "VictoriaMetrics - Prod") + require.Contains(t, err.Error(), "vm") +} + +func TestDatasourceDefinitions_FilteredQuery(t *testing.T) { + var gotQuery string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotQuery = r.URL.RawQuery + require.Equal(t, "/api/prometheus/vm/api/v1/rules", r.URL.Path) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(readFixture(t, "ds_rules.json")) + })) + defer srv.Close() + + src := NewHTTPSource(srv.URL, "", newFakeClock(time.Now())) + defs, err := src.DatasourceDefinitions(context.Background(), RuleSource{UID: "vm", Name: "VM"}, []string{"ExampleTargetDown"}) + require.NoError(t, err) + require.Equal(t, "rule_name%5B%5D=ExampleTargetDown", gotQuery) + require.Len(t, defs, 1) + require.Equal(t, KindDatasourceManaged, defs[0].Kind) + require.Equal(t, "VM", defs[0].DatasourceName) +} + +func TestFakeSource_DatasourceScriptedByKey(t *testing.T) { + f := newFakeSource() + key := ruleKey("vm", "G", "A", "") + f.scriptKey(key, Observation{Rules: []StateRule{{Key: key, DatasourceUID: "vm", Title: "A"}}}, nil) + obs, err := f.RuleState(context.Background(), RuleRef{Key: key, Kind: KindDatasourceManaged, DatasourceUID: "vm"}) + require.NoError(t, err) + require.Len(t, obs.Rules, 1) +} + +func TestLoadDefinitions_DiscoversAndDropsRulerDatasourceRules(t *testing.T) { + f := newFakeSource() + f.defs = []Definition{ + {Key: "g1", UID: "g1", Title: "Grafana Rule", Kind: KindGrafanaManaged}, + {Title: "RulerDsRule", Kind: KindDatasourceManaged}, // no datasource UID: dropped + } + f.ruleSources = []RuleSource{{UID: "vm", Name: "VM"}} + f.dsDefs = map[string][]Definition{"vm": {dsDef("A"), dsDef("B")}} + + all, err := loadDefinitions(context.Background(), f, nil, true) + require.NoError(t, err) + require.Len(t, all, 3, "Grafana + two ds, ruler ds rule dropped") + + filtered, err := loadDefinitions(context.Background(), f, []string{"A"}, false) + require.NoError(t, err) + require.Len(t, filtered, 2) + require.Equal(t, "A", filtered[1].Title) +} + +func TestRuleState_DatasourceAssertsAllFilters(t *testing.T) { + var gotPath, gotQuery string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath, gotQuery = r.URL.Path, r.URL.RawQuery + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(readFixture(t, "ds_rules.json")) + })) + defer srv.Close() + + src := NewHTTPSource(srv.URL, "", newFakeClock(time.Now())) + ref := RuleRef{ + Key: ruleKey("vm", "ExampleMetrics", "ExampleTargetDown", ""), + Kind: KindDatasourceManaged, DatasourceUID: "vm", + Group: "ExampleMetrics", Name: "ExampleTargetDown", File: "/etc/vm/rules/example.yml", + } + obs, err := src.RuleState(context.Background(), ref) + require.NoError(t, err) + require.Equal(t, "/api/prometheus/vm/api/v1/rules", gotPath) + require.Equal(t, + "file%5B%5D=%2Fetc%2Fvm%2Frules%2Fexample.yml&rule_group%5B%5D=ExampleMetrics&rule_name%5B%5D=ExampleTargetDown", + gotQuery) + require.Len(t, obs.Rules, 2) + require.Equal(t, ref.Key, obs.Rules[0].Key) +} diff --git a/grafana-alertcheck/internal/gate/source_fake_test.go b/grafana-alertcheck/internal/gate/source_fake_test.go index fe5e1e470..17ddf0a66 100644 --- a/grafana-alertcheck/internal/gate/source_fake_test.go +++ b/grafana-alertcheck/internal/gate/source_fake_test.go @@ -134,11 +134,17 @@ type fakeSource struct { defs []Definition defsErr error - // states maps a rule title to a queue of scripted results, popped one - // per call to RuleState. Once the queue is down to its last entry, that - // entry repeats — so a test can script the interesting transitions and - // let a long collection loop settle into steady state without scripting - // every single poll. + // ruleSources is what DiscoverRuleSources returns; dsDefs maps a datasource + // UID to the definitions DatasourceDefinitions returns. + ruleSources []RuleSource + sourcesErr error + dsDefs map[string][]Definition + + // states maps a rule title (Grafana) or key (datasource) to a queue of + // scripted results, popped one per call to RuleState. Once the queue is down + // to its last entry, that entry repeats — so a test can script the + // interesting transitions and let a long collection loop settle into steady + // state without scripting every single poll. states map[string][]scriptedObservation } @@ -152,7 +158,7 @@ func (f *fakeSource) Version(_ context.Context) (string, error) { return f.version, f.versionErr } -func (f *fakeSource) Definitions(_ context.Context) ([]Definition, error) { +func (f *fakeSource) GrafanaDefinitions(_ context.Context) ([]Definition, error) { f.mu.Lock() defer f.mu.Unlock() // Defensive copy: Definition is a value type, so Clone copies the @@ -161,16 +167,46 @@ func (f *fakeSource) Definitions(_ context.Context) ([]Definition, error) { return slices.Clone(f.defs), f.defsErr } -func (f *fakeSource) RuleState(_ context.Context, title string) (Observation, error) { +func (f *fakeSource) DiscoverRuleSources(_ context.Context) ([]RuleSource, error) { f.mu.Lock() defer f.mu.Unlock() - q := f.states[title] + return slices.Clone(f.ruleSources), f.sourcesErr +} + +func (f *fakeSource) DatasourceDefinitions(_ context.Context, src RuleSource, names []string) ([]Definition, error) { + f.mu.Lock() + defer f.mu.Unlock() + defs := slices.Clone(f.dsDefs[src.UID]) + if len(names) == 0 { + return defs, nil + } + want := make(map[string]bool, len(names)) + for _, n := range names { + want[n] = true + } + var out []Definition + for _, d := range defs { + if want[d.Title] { + out = append(out, d) + } + } + return out, nil +} + +func (f *fakeSource) RuleState(_ context.Context, ref RuleRef) (Observation, error) { + key := ref.Title + if ref.Kind == KindDatasourceManaged { + key = ref.Key + } + f.mu.Lock() + defer f.mu.Unlock() + q := f.states[key] if len(q) == 0 { - return Observation{}, fmt.Errorf("fakeSource: no scripted response for %q", title) + return Observation{}, fmt.Errorf("fakeSource: no scripted response for %q", key) } next := q[0] if len(q) > 1 { - f.states[title] = q[1:] + f.states[key] = q[1:] } // Defensive copy of the shared Rules slice so a caller mutating the // returned Observation can't corrupt the scripted state other calls read. @@ -179,11 +215,18 @@ func (f *fakeSource) RuleState(_ context.Context, title string) (Observation, er } // script appends one scripted (Observation, error) pair to be returned, in -// order, by RuleState(ctx, title). +// order, by RuleState(ctx, ref) for the Grafana rule with this title. func (f *fakeSource) script(title string, obs Observation, err error) { f.mu.Lock() defer f.mu.Unlock() f.states[title] = append(f.states[title], scriptedObservation{obs: obs, err: err}) } +// scriptKey is script's datasource counterpart, keyed by the rule key. +func (f *fakeSource) scriptKey(key string, obs Observation, err error) { + f.mu.Lock() + defer f.mu.Unlock() + f.states[key] = append(f.states[key], scriptedObservation{obs: obs, err: err}) +} + var _ Source = (*fakeSource)(nil) diff --git a/grafana-alertcheck/internal/gate/source_test.go b/grafana-alertcheck/internal/gate/source_test.go index b6ab70c7c..f403fba65 100644 --- a/grafana-alertcheck/internal/gate/source_test.go +++ b/grafana-alertcheck/internal/gate/source_test.go @@ -146,7 +146,7 @@ func TestHTTPSource_RuleState_EmptyIsNotAnError(t *testing.T) { clock := newFakeClock(time.Now()) src := NewHTTPSource(srv.URL, "", clock) - obs, err := src.RuleState(context.Background(), "Anything") + obs, err := src.RuleState(context.Background(), RuleRef{Title: "Anything"}) require.NoError(t, err) require.Empty(t, obs.Rules, "an authoritative 2xx is not a transport error") require.False(t, obs.GrafanaNow.IsZero(), "want the response's Date header value") @@ -165,7 +165,7 @@ func TestHTTPSource_RuleState_EscapesRuleName(t *testing.T) { clock := newFakeClock(time.Now()) src := NewHTTPSource(srv.URL, "", clock) title := "[JD] No Job Proposals & More" - _, err := src.RuleState(context.Background(), title) + _, err := src.RuleState(context.Background(), RuleRef{Title: title}) require.NoError(t, err) require.Equal(t, "rule_name="+url.QueryEscape(title), gotQuery) } @@ -182,7 +182,7 @@ func TestHTTPSource_Definitions_HappyPath(t *testing.T) { clock := newFakeClock(time.Now()) src := NewHTTPSource(srv.URL, "", clock) - defs, err := src.Definitions(context.Background()) + defs, err := src.GrafanaDefinitions(context.Background()) require.NoError(t, err) require.NotEmpty(t, defs) } @@ -287,7 +287,7 @@ func TestHTTPSource_ObservationTiming(t *testing.T) { }, emptyStateBody()) }) src := NewHTTPSource(srv.URL, "", clock) - obs, err := src.RuleState(context.Background(), "Anything") + obs, err := src.RuleState(context.Background(), RuleRef{Title: "Anything"}) require.NoError(t, err) require.Equal(t, c.drift, obs.Skew) require.Equal(t, time.Second, obs.SkewBound, "RTT/2 with a 2s round trip to headers") @@ -324,7 +324,7 @@ func TestHTTPSourceStalenessNeverFalsePositiveUnderSkew(t *testing.T) { }) src := NewHTTPSource(srv.URL, "", clock) - obs, err := src.RuleState(context.Background(), def.Title) + obs, err := src.RuleState(context.Background(), RuleRef{Title: def.Title}) require.NoError(t, err) require.True(t, obs.GrafanaNow.Equal(serverDate), "want the Date header, never the runner's clock") @@ -409,7 +409,7 @@ func TestHTTPSource_RuleState_GarbageBodyRetries(t *testing.T) { clock := newFakeClock(time.Now()) src := NewHTTPSource(srv.URL, "", clock) - obs, err := src.RuleState(context.Background(), "Anything") + obs, err := src.RuleState(context.Background(), RuleRef{Title: "Anything"}) require.NoError(t, err) require.Empty(t, obs.Rules) mu.Lock() @@ -429,7 +429,7 @@ func TestHTTPSource_Definitions_GarbageBodyGivesUp(t *testing.T) { clock := newFakeClock(time.Now()) src := NewHTTPSource(srv.URL, "", clock) - _, err := src.Definitions(context.Background()) + _, err := src.GrafanaDefinitions(context.Background()) require.Error(t, err) require.Equal(t, int32(6), calls.Load(), "a persistently unparseable 2xx body retries like any other transport failure") @@ -514,7 +514,7 @@ func TestFakeSource(t *testing.T) { v, err := f.Version(ctx) require.NoError(t, err) require.Equal(t, "13.1.0", v) - defs, err := f.Definitions(ctx) + defs, err := f.GrafanaDefinitions(ctx) require.NoError(t, err) require.Len(t, defs, 1) @@ -522,18 +522,18 @@ func TestFakeSource(t *testing.T) { f.script("Rule One", Observation{}, fmt.Errorf("boom")) f.script("Rule One", Observation{Rules: nil}, nil) - obs, err := f.RuleState(ctx, "Rule One") + obs, err := f.RuleState(ctx, RuleRef{Title: "Rule One"}) require.NoError(t, err) require.Len(t, obs.Rules, 1) - _, err = f.RuleState(ctx, "Rule One") + _, err = f.RuleState(ctx, RuleRef{Title: "Rule One"}) require.Error(t, err, "RuleState() call 2: want the scripted error, got nil") - obs, err = f.RuleState(ctx, "Rule One") + obs, err = f.RuleState(ctx, RuleRef{Title: "Rule One"}) require.NoError(t, err) require.Nil(t, obs.Rules, "last script entry, then repeats") - obs, err = f.RuleState(ctx, "Rule One") + obs, err = f.RuleState(ctx, RuleRef{Title: "Rule One"}) require.NoError(t, err) require.Nil(t, obs.Rules) - _, err = f.RuleState(ctx, "Unscripted Rule") + _, err = f.RuleState(ctx, RuleRef{Title: "Unscripted Rule"}) require.Error(t, err) } diff --git a/grafana-alertcheck/internal/gate/terminal.go b/grafana-alertcheck/internal/gate/terminal.go index c8b4197bb..ed1b47455 100644 --- a/grafana-alertcheck/internal/gate/terminal.go +++ b/grafana-alertcheck/internal/gate/terminal.go @@ -20,6 +20,7 @@ const ( type Termination struct { Kind TerminationKind `json:"kind"` Alert string `json:"alert,omitempty"` + RuleKey string `json:"rule_key,omitempty"` RuleUID string `json:"rule_uid,omitempty"` Outcome Outcome `json:"outcome,omitempty"` Reason UnobservableReason `json:"reason,omitempty"` @@ -42,21 +43,23 @@ func terminalVerdict(h Header, polls []Poll, defs []Definition, rt map[string]Ru var violation *Termination for _, def := range defs { - if pausedAtStart[def.UID] { + key := defKey(def) + if pausedAtStart[key] { continue } // The synthetic sentinel at `at` satisfies check 1, leaving only the // checks decidable from the polls so far. The policy-specific nodata // escalation is applied here too, or a configured terminal inability // would never fail fast. - cov := proveCoverage(h, polls, &at, rt[def.UID], def, from, at, 0) + cov := proveCoverage(h, polls, &at, rt[key], def, from, at, 0) if pol.NodataIsUnobservable { - applyNodataPolicy(def, polls, &cov, rt[def.UID], from, at) + applyNodataPolicy(def, polls, &cov, rt[key], from, at) } if cov.Unobservable { return Termination{ Kind: TerminationNotVerified, Alert: def.Title, + RuleKey: key, RuleUID: def.UID, Outcome: OutcomeNotVerified, Reason: cov.Reason, @@ -70,6 +73,7 @@ func terminalVerdict(h Header, polls []Poll, defs []Definition, rt map[string]Ru v := Termination{ Kind: TerminationViolation, Alert: def.Title, + RuleKey: key, RuleUID: def.UID, Outcome: outcome, At: at, diff --git a/grafana-alertcheck/internal/gate/testdata/ds_rules.json b/grafana-alertcheck/internal/gate/testdata/ds_rules.json new file mode 100644 index 000000000..fd24fe7d2 --- /dev/null +++ b/grafana-alertcheck/internal/gate/testdata/ds_rules.json @@ -0,0 +1,50 @@ +{ + "status": "success", + "data": { + "groups": [ + { + "name": "ExampleMetrics", + "file": "/etc/vm/rules/example.yml", + "interval": 60, + "rules": [ + { + "state": "firing", + "name": "ExampleTargetDown", + "query": "up == 0", + "duration": 300, + "labels": { + "severity": "warning", + "team": "example-team" + }, + "annotations": { + "summary": "target down" + }, + "alerts": [ + { + "labels": { + "instance": "example-host-1" + }, + "annotations": { + "summary": "target down" + }, + "state": "firing", + "activeAt": "2026-08-01T00:00:00Z", + "value": "0" + } + ], + "health": "ok", + "lastEvaluation": "2026-08-01T00:01:00Z", + "type": "alerting" + }, + { + "name": "example:recorded_metric:rate5m", + "query": "rate(example_metric_total[5m])", + "health": "ok", + "lastEvaluation": "2026-08-01T00:01:00Z", + "type": "recording" + } + ] + } + ] + } +} diff --git a/grafana-alertcheck/internal/gate/watch.go b/grafana-alertcheck/internal/gate/watch.go index 98e29145f..a44d59a06 100644 --- a/grafana-alertcheck/internal/gate/watch.go +++ b/grafana-alertcheck/internal/gate/watch.go @@ -291,7 +291,8 @@ func prepareWatch(ctx context.Context, cfg WatchConfig, src Source) (*preparedWa return nil, err } - defs, err := src.Definitions(ctx) + wantAll := len(cfg.IncludeLabels) > 0 || len(cfg.ExcludeLabels) > 0 || len(cfg.ExcludeAlerts) > 0 + defs, err := loadDefinitions(ctx, src, cfg.Alerts, wantAll) if err != nil { return nil, fmt.Errorf("read rule definitions: %w", err) } @@ -314,9 +315,9 @@ func prepareWatch(ctx context.Context, cfg WatchConfig, src Source) (*preparedWa // A cadence of zero would make the child spin: every rule is due the // instant it was marked. It also cannot be written into the header, // where check requires a positive value to derive maxGap from. - if rt[d.UID].pollEvery <= 0 { + if rt[defKey(d)].pollEvery <= 0 { return nil, fmt.Errorf("rule %q (%s) reports intervalSeconds=%d: there is no poll cadence to record at", - d.Title, d.UID, d.IntervalSeconds) + d.Title, defKey(d), d.IntervalSeconds) } } @@ -359,12 +360,12 @@ func openRecording(ctx context.Context, cfg WatchConfig, src Source, writer *Wri activeTimings := make(map[string]RuleTimings, len(resolved)) for _, d := range resolved { if d.IsPaused { - fmt.Fprintf(cfg.Notes, "note: rule %q (%s) is paused: recorded as skipped, not waited for\n", d.Title, d.UID) + fmt.Fprintf(cfg.Notes, "note: rule %q (%s) is paused: recorded as skipped, not waited for\n", d.Title, defKey(d)) continue } active = append(active, d) - activeTimings[d.UID] = rt[d.UID] - fmt.Fprintf(cfg.Notes, "recording %q (%s) every %s (maxGap %s)\n", d.Title, d.UID, rt[d.UID].pollEvery, rt[d.UID].maxGap) + activeTimings[defKey(d)] = rt[defKey(d)] + fmt.Fprintf(cfg.Notes, "recording %q (%s) every %s (maxGap %s)\n", d.Title, defKey(d), rt[defKey(d)].pollEvery, rt[defKey(d)].maxGap) } polls, measured, err := firstObservations(ctx, src, active, NewReducer(), cfg.Concurrency, cfg.Notes) @@ -427,21 +428,34 @@ func loggedRules(defs []Definition, rt map[string]RuleTimings) []LoggedRule { out := make([]LoggedRule, 0, len(defs)) for _, d := range defs { out = append(out, LoggedRule{ + Key: defKey(d), UID: d.UID, Title: d.Title, Folder: d.Folder, Group: d.Group, + SourceKind: sourceKind(d.Kind), + DatasourceUID: d.DatasourceUID, + DatasourceName: d.DatasourceName, + File: d.File, ForSeconds: d.For.Seconds(), IntervalSeconds: d.IntervalSeconds, IsPaused: d.IsPaused, NoDataState: d.NoDataState, ExecErrState: d.ExecErrState, - PollEverySeconds: rt[d.UID].pollEvery.Seconds(), + PollEverySeconds: rt[defKey(d)].pollEvery.Seconds(), }) } return out } +// sourceKind is the header's source_kind value for a rule kind. +func sourceKind(k RuleKind) string { + if k == KindDatasourceManaged { + return "datasource" + } + return "grafana" +} + // firstObservations takes one observation of every active rule, verifies normal // instances are visible in those very responses, and reduces each into the // window's first heartbeat, plus measured latency (the only honest budget @@ -450,14 +464,14 @@ func loggedRules(defs []Definition, rt map[string]RuleTimings) []LoggedRule { func firstObservations(ctx context.Context, src Source, active []Definition, reducer *Reducer, concurrency int, notes io.Writer) ([]Poll, map[string]time.Duration, error) { - titles := make(map[string]string, len(active)) - uids := make([]string, 0, len(active)) + refs := make(map[string]RuleRef, len(active)) + keys := make([]string, 0, len(active)) for _, d := range active { - titles[d.UID] = d.Title - uids = append(uids, d.UID) + refs[defKey(d)] = ruleRefOf(d) + keys = append(keys, defKey(d)) } - observed, err := observeAll(ctx, src, titles, uids, concurrency) + observed, err := observeAll(ctx, src, refs, keys, concurrency) if err != nil { return nil, nil, err } @@ -465,9 +479,14 @@ func firstObservations(ctx context.Context, src Source, active []Definition, red // Verify this before anything downstream relies on it: if the state // endpoint ever stops returning normal instances, the reduction's "keep // the non-normal ones" silently becomes "keep everything it happened to - // send" and the transition markers lose their ground truth. + // send" and the transition markers lose their ground truth. A + // datasource-managed response has no normal instances by construction, so + // the check applies to Grafana-managed rules only. for _, d := range active { - if err := VerifyNormalInstancesVisible(observed[d.UID].Rules); err != nil { + if d.Kind != KindGrafanaManaged { + continue + } + if err := VerifyNormalInstancesVisible(observed[defKey(d)].Rules); err != nil { return nil, nil, err } } @@ -475,9 +494,10 @@ func firstObservations(ctx context.Context, src Source, active []Definition, red polls := make([]Poll, 0, len(active)) measured := make(map[string]time.Duration, len(active)) for _, d := range active { - obs := observed[d.UID] - measured[d.UID] = obs.Latency - poll := reducer.Reduce(d.UID, obs) + key := defKey(d) + obs := observed[key] + measured[key] = obs.Latency + poll := reducer.Reduce(key, obs) if !poll.Found { // Authoritative, not transient (the transport already retried // every transient failure): the rule resolved in the ruler API but @@ -485,7 +505,7 @@ func firstObservations(ctx context.Context, src Source, active []Definition, red // which the coverage proof turns into unobservable — a note rather // than an error here, because the state endpoint can lag a freshly // created rule and the coverage proof fails closed either way. - fmt.Fprintf(notes, "warning: rule %q (%s) is absent from the state endpoint; recorded as not found\n", d.Title, d.UID) + fmt.Fprintf(notes, "warning: rule %q (%s) is absent from the state endpoint; recorded as not found\n", d.Title, key) } polls = append(polls, poll) } @@ -496,18 +516,18 @@ func firstObservations(ctx context.Context, src Source, active []Definition, red // flight, dispatching in uids order (a worker takes the next uid as it frees) // so the startup-handoff simulation matches. Polls by TITLE, selects by UID, // and returns the first error in UID order alongside the successes. -func observeAll(ctx context.Context, src Source, titles map[string]string, uids []string, concurrency int) (map[string]Observation, error) { +func observeAll(ctx context.Context, src Source, refs map[string]RuleRef, keys []string, concurrency int) (map[string]Observation, error) { if concurrency < 1 { concurrency = 1 } - if concurrency > len(uids) { - concurrency = len(uids) + if concurrency > len(keys) { + concurrency = len(keys) } var ( mu sync.Mutex - out = make(map[string]Observation, len(uids)) + out = make(map[string]Observation, len(keys)) firstErr error - firstErrUID string + firstErrKey string next int ) var wg sync.WaitGroup @@ -515,23 +535,23 @@ func observeAll(ctx context.Context, src Source, titles map[string]string, uids wg.Go(func() { for { mu.Lock() - if next == len(uids) { + if next == len(keys) { mu.Unlock() return } - uid := uids[next] + key := keys[next] next++ mu.Unlock() - obs, err := src.RuleState(ctx, titles[uid]) + obs, err := src.RuleState(ctx, refs[key]) mu.Lock() if err != nil { - if firstErr == nil || uid < firstErrUID { - firstErr, firstErrUID = err, uid + if firstErr == nil || key < firstErrKey { + firstErr, firstErrKey = err, key } } else { - out[uid] = obs + out[key] = obs } mu.Unlock() } @@ -540,7 +560,7 @@ func observeAll(ctx context.Context, src Source, titles map[string]string, uids wg.Wait() if firstErr != nil { - return out, fmt.Errorf("poll rule %q (%s): %w", titles[firstErrUID], firstErrUID, firstErr) + return out, fmt.Errorf("poll rule %q (%s): %w", refs[firstErrKey].Title, firstErrKey, firstErr) } return out, nil } @@ -596,7 +616,7 @@ func RunDaemonChild(ctx context.Context, cfg DaemonChildConfig) error { return fmt.Errorf("log %s records url %q but this recorder is configured for %q", cfg.Out, header.URL, cfg.URL) } - titles, cadence, err := childSchedule(header) + refs, cadence, err := childSchedule(header) if err != nil { return err } @@ -627,7 +647,7 @@ func RunDaemonChild(ctx context.Context, cfg DaemonChildConfig) error { Src: NewHTTPSource(cfg.URL, cfg.Token, cfg.Clock), Writer: writer, Reducer: reducer, - Titles: titles, + Refs: refs, Cadence: cadence, Seed: polls, Until: cfg.Until, @@ -657,26 +677,45 @@ func reportReady(fd int) error { // childSchedule derives what the child polls, and how often, from the header // alone. Cadence comes from PollEverySeconds (the cadence actually used), never // re-derived from the evaluation interval; paused rules are excluded. It -// returns cadences only — the recorder must not carry coverage thresholds it -// has no business applying. -func childSchedule(h Header) (titles map[string]string, cadence map[string]time.Duration, err error) { - titles = make(map[string]string, len(h.Rules)) +// returns refs and cadences only — the recorder must not carry coverage +// thresholds it has no business applying. +func childSchedule(h Header) (refs map[string]RuleRef, cadence map[string]time.Duration, err error) { + refs = make(map[string]RuleRef, len(h.Rules)) cadence = make(map[string]time.Duration, len(h.Rules)) for _, lr := range h.Rules { if lr.IsPaused { continue } + key := loggedKey(lr) if lr.PollEverySeconds <= 0 { return nil, nil, fmt.Errorf("log header records poll_every_seconds=%v for rule %s (%q): there is no cadence to record at", - lr.PollEverySeconds, lr.UID, lr.Title) + lr.PollEverySeconds, key, lr.Title) } - if _, duplicate := titles[lr.UID]; duplicate { - return nil, nil, fmt.Errorf("log header names rule %s (%q) twice; its recorded cadence is ambiguous", lr.UID, lr.Title) + if _, duplicate := refs[key]; duplicate { + return nil, nil, fmt.Errorf("log header names rule %s (%q) twice; its recorded cadence is ambiguous", key, lr.Title) } - titles[lr.UID] = lr.Title - cadence[lr.UID] = time.Duration(lr.PollEverySeconds * float64(time.Second)) + refs[key] = RuleRef{ + Key: key, + Kind: kindOfLogged(lr), + DatasourceUID: lr.DatasourceUID, + UID: lr.UID, + Group: lr.Group, + Name: lr.Title, + File: lr.File, + Title: lr.Title, + } + cadence[key] = time.Duration(lr.PollEverySeconds * float64(time.Second)) + } + return refs, cadence, nil +} + +// kindOfLogged recovers a LoggedRule's source kind. A datasource-managed rule +// always carries a datasource UID; a Grafana-managed rule never does. +func kindOfLogged(lr LoggedRule) RuleKind { + if lr.DatasourceUID != "" { + return KindDatasourceManaged } - return titles, cadence, nil + return KindGrafanaManaged } // watchLoopConfig is the child's working state: what to poll, how often, and @@ -686,8 +725,8 @@ type watchLoopConfig struct { Src Source Writer *Writer Reducer *Reducer - Titles map[string]string // uid -> title: poll by title, select by UID - Cadence map[string]time.Duration // uid -> pollEvery, as recorded in the header + Refs map[string]RuleRef // key -> ref: how to find the rule again + Cadence map[string]time.Duration // key -> pollEvery, as recorded in the header // Seed is the polls already in the log when this loop starts: it continues // their schedule instead of re-staggering. nil means a fresh schedule. Seed []Poll @@ -765,14 +804,14 @@ func watchLoop(ctx context.Context, cfg watchLoopConfig) error { // successes first is deliberate: a heartbeat that was genuinely observed is // evidence, and dropping it because a different rule failed would turn one // rule's transport failure into a coverage gap for the others. -func (cfg watchLoopConfig) pollBatch(ctx context.Context, uids []string) error { - observed, obsErr := observeAll(ctx, cfg.Src, cfg.Titles, uids, cfg.Concurrency) - for _, uid := range uids { - obs, ok := observed[uid] +func (cfg watchLoopConfig) pollBatch(ctx context.Context, keys []string) error { + observed, obsErr := observeAll(ctx, cfg.Src, cfg.Refs, keys, cfg.Concurrency) + for _, key := range keys { + obs, ok := observed[key] if !ok { continue } - if err := cfg.Writer.WritePoll(cfg.Reducer.Reduce(uid, obs)); err != nil { + if err := cfg.Writer.WritePoll(cfg.Reducer.Reduce(key, obs)); err != nil { return err } } diff --git a/grafana-alertcheck/internal/gate/watch_daemon_test.go b/grafana-alertcheck/internal/gate/watch_daemon_test.go index 04c1276e9..99f0b44ae 100644 --- a/grafana-alertcheck/internal/gate/watch_daemon_test.go +++ b/grafana-alertcheck/internal/gate/watch_daemon_test.go @@ -147,6 +147,8 @@ func grafanaTestServer(t *testing.T) *httptest.Server { fmt.Fprint(w, healthBody("13.1.0")) case strings.HasPrefix(r.URL.Path, "/api/ruler/"): _, _ = w.Write(ruler) + case r.URL.Path == "/api/datasources": + _, _ = w.Write([]byte(`[]`)) case strings.HasPrefix(r.URL.Path, "/api/prometheus/"): if r.URL.Query().Get("rule_name") == "" { // The gate must never read the state endpoint unfiltered. diff --git a/grafana-alertcheck/internal/gate/watch_test.go b/grafana-alertcheck/internal/gate/watch_test.go index 576745da2..890cf4f64 100644 --- a/grafana-alertcheck/internal/gate/watch_test.go +++ b/grafana-alertcheck/internal/gate/watch_test.go @@ -43,11 +43,20 @@ func (s *loopSource) Version(context.Context) (string, error) { return "", errors.New("loopSource: the recorder loop must not read the version") } -func (s *loopSource) Definitions(context.Context) ([]Definition, error) { +func (s *loopSource) GrafanaDefinitions(context.Context) ([]Definition, error) { return nil, errors.New("loopSource: the recorder loop must not read the definitions") } -func (s *loopSource) RuleState(_ context.Context, title string) (Observation, error) { +func (s *loopSource) DiscoverRuleSources(context.Context) ([]RuleSource, error) { + return nil, errors.New("loopSource: the recorder loop must not discover sources") +} + +func (s *loopSource) DatasourceDefinitions(context.Context, RuleSource, []string) ([]Definition, error) { + return nil, errors.New("loopSource: the recorder loop must not read datasource definitions") +} + +func (s *loopSource) RuleState(_ context.Context, ref RuleRef) (Observation, error) { + title := ref.Title s.mu.Lock() s.calls[title]++ call := s.calls[title] @@ -110,7 +119,10 @@ func TestWatchLoopPollsEachRuleAtItsOwnCadence(t *testing.T) { Src: src, Writer: w, Reducer: NewReducer(), - Titles: map[string]string{tightUID: "Tight Rule", slackUID: "Slack Rule"}, + Refs: map[string]RuleRef{ + tightUID: {Key: tightUID, Title: "Tight Rule"}, + slackUID: {Key: slackUID, Title: "Slack Rule"}, + }, Cadence: map[string]time.Duration{ tightUID: 5 * time.Second, slackUID: 150 * time.Second, @@ -152,7 +164,7 @@ func TestWatchLoopContinuesTheSeededSchedule(t *testing.T) { Src: src, Writer: w, Reducer: NewReducer(), - Titles: map[string]string{"r1": "Example"}, + Refs: map[string]RuleRef{"r1": {Key: "r1", Title: "Example"}}, Cadence: map[string]time.Duration{"r1": 30 * time.Second}, Seed: []Poll{{RuleUID: "r1", GrafanaNow: testNow.Add(-90 * time.Second), Found: true}}, Until: testNow.Add(time.Minute), @@ -178,9 +190,12 @@ func TestObserveAllDispatchesInOrder(t *testing.T) { return observation(testNow, testStateRule(title, title, time.Minute, testNow)), nil }) uids := []string{"r1", "r2", "r3", "r4"} - titles := map[string]string{"r1": "One", "r2": "Two", "r3": "Three", "r4": "Four"} + refs := map[string]RuleRef{ + "r1": {Key: "r1", Title: "One"}, "r2": {Key: "r2", Title: "Two"}, + "r3": {Key: "r3", Title: "Three"}, "r4": {Key: "r4", Title: "Four"}, + } - out, err := observeAll(context.Background(), src, titles, uids, 1) + out, err := observeAll(context.Background(), src, refs, uids, 1) require.NoError(t, err) require.Len(t, out, 4) require.Equal(t, []string{"One", "Two", "Three", "Four"}, order) @@ -206,7 +221,7 @@ func TestWatchLoopHardErrorLeavesNoSentinel(t *testing.T) { Src: src, Writer: w, Reducer: NewReducer(), - Titles: map[string]string{"r1": "Example"}, + Refs: map[string]RuleRef{"r1": {Key: "r1", Title: "Example"}}, Cadence: map[string]time.Duration{"r1": 30 * time.Second}, Until: testNow.Add(time.Hour), Concurrency: 1, @@ -245,7 +260,7 @@ func TestWatchLoopSignalDuringPollIsACleanStop(t *testing.T) { Src: src, Writer: w, Reducer: NewReducer(), - Titles: map[string]string{"r1": "Example"}, + Refs: map[string]RuleRef{"r1": {Key: "r1", Title: "Example"}}, Cadence: map[string]time.Duration{"r1": 30 * time.Second}, Concurrency: 1, Clock: clock, @@ -272,7 +287,7 @@ func TestWatchLoopWithNothingToPollStillFinishesTheLog(t *testing.T) { Src: src, Writer: w, Reducer: NewReducer(), - Titles: map[string]string{}, + Refs: map[string]RuleRef{}, Cadence: map[string]time.Duration{}, Until: testNow.Add(time.Minute), Concurrency: 1, @@ -306,7 +321,7 @@ func TestWatchLoopPollBatchKeepsTheHeartbeatsItGot(t *testing.T) { Src: src, Writer: w, Reducer: NewReducer(), - Titles: map[string]string{"ok": "Healthy", "bad": "Broken"}, + Refs: map[string]RuleRef{"ok": {Key: "ok", Title: "Healthy"}, "bad": {Key: "bad", Title: "Broken"}}, Cadence: map[string]time.Duration{"ok": 30 * time.Second, "bad": 30 * time.Second}, Concurrency: 2, Clock: clock, From f2da6affe516393786d6a03aa34149e1febdf290 Mon Sep 17 00:00:00 2001 From: Bartek Tofel Date: Tue, 6 Oct 2026 11:18:39 +0200 Subject: [PATCH 2/8] chore: code review fixes --- grafana-alertcheck/.changeset/v0.1.10.md | 2 + grafana-alertcheck/cmd/table.go | 17 ++- grafana-alertcheck/cmd/table_test.go | 9 ++ grafana-alertcheck/docs/architecture.md | 2 +- grafana-alertcheck/docs/reference/cli.md | 12 +- .../docs/reference/log-format.md | 2 +- grafana-alertcheck/internal/gate/check.go | 5 + .../internal/gate/check_ds_test.go | 4 +- grafana-alertcheck/internal/gate/classify.go | 22 ++- grafana-alertcheck/internal/gate/coverage.go | 42 +++++- .../gate/datasource_semantics_test.go | 21 ++- grafana-alertcheck/internal/gate/identity.go | 16 +-- .../internal/gate/identity_test.go | 21 +-- grafana-alertcheck/internal/gate/labels.go | 2 +- grafana-alertcheck/internal/gate/load.go | 32 +++-- .../internal/gate/log_ds_test.go | 8 +- .../internal/gate/parse_datasource.go | 4 +- .../internal/gate/parse_datasource_test.go | 14 +- grafana-alertcheck/internal/gate/resolve.go | 126 ++++++++++++------ .../internal/gate/resolve_test.go | 34 ++++- grafana-alertcheck/internal/gate/source.go | 13 +- .../internal/gate/source_ds_test.go | 32 ++++- 22 files changed, 315 insertions(+), 125 deletions(-) diff --git a/grafana-alertcheck/.changeset/v0.1.10.md b/grafana-alertcheck/.changeset/v0.1.10.md index 2d9e0a169..159cd2a1c 100644 --- a/grafana-alertcheck/.changeset/v0.1.10.md +++ b/grafana-alertcheck/.changeset/v0.1.10.md @@ -4,3 +4,5 @@ - Datasource recovery semantics: because the Prometheus API returns only active instances, an instance leaving the active set is treated as a real recovery (`cleared`). Documented weaker guarantee: a vanished series is indistinguishable from a resolution. - Pause is not observable for datasource-managed rules (no `isPaused` signal), so check 7 is skipped with an explicit note. `health: err` normalizes to `error` and still triggers check 4. - The token now needs `datasources:read` plus datasource query permission even for Grafana-only runs; discovery failures name the missing permission. +- A datasource-managed rule's name can itself contain `/` (e.g. `devex-cicd/prod/griddle-github: ContainersNotReady`). The full name is now matched exactly and used as the `rule_name[]` fetch filter, instead of splitting it into `Group/Title` segments and filtering by the last segment. +- The `RESULTS` table gains a `SOURCE` column (`grafana`/`datasource`). Datasource caveats are printed once before the table, and `DETAILS` carries only rule-specific notes without repeating the alert name. `--output json` adds `source_kind` and a run-level `caveats`. diff --git a/grafana-alertcheck/cmd/table.go b/grafana-alertcheck/cmd/table.go index 83306392a..492196d1a 100644 --- a/grafana-alertcheck/cmd/table.go +++ b/grafana-alertcheck/cmd/table.go @@ -5,6 +5,7 @@ import ( "io" "sort" "strconv" + "strings" "text/tabwriter" "time" @@ -60,11 +61,11 @@ func renderTable(w io.Writer, res gate.Result) error { fmt.Fprintln(w, "RESULTS") tw := tabwriter.NewWriter(w, 0, 4, 2, ' ', 0) - fmt.Fprintln(tw, "ALERT\tVERDICT\tBROKEN FOR\tCHECKED EVERY\tWINDOW COVERED\tDETAILS") + fmt.Fprintln(tw, "ALERT\tVERDICT\tBROKEN FOR\tCHECKED EVERY\tWINDOW COVERED\tSOURCE\tDETAILS") for _, v := range sortedVerdicts(res.Verdicts) { - fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\n", + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\t%s\t%s\t%s\n", v.Alert, v.Outcome, v.BadFor.Round(time.Second), v.PollEvery.Round(time.Second), - provedLabel(res.Coverage[verdictKey(v)]), v.Note) + provedLabel(res.Coverage[verdictKey(v)]), v.SourceKind, details(v.Alert, v.Note)) } if err := tw.Flush(); err != nil { return fmt.Errorf("render table: %w", err) @@ -133,6 +134,16 @@ func violationsLabel(n int, enabled bool) string { return mark + " " + s } +// details strips the redundant `rule "": ` prefix every coverage note +// carries for the JSON consumer — the ALERT column already names the rule, and +// keeping it would repeat a long name in every DETAILS cell. +func details(title, note string) string { + if note == "" { + return "" + } + return strings.ReplaceAll(note, fmt.Sprintf("rule %q: ", title), "") +} + // provedLabel is the table's WINDOW COVERED column: "yes" for a fully // observed window, "no" with the reason and largest gap for a not-verified // rule, and "-" for a rule decide never asked proveCoverage about at all diff --git a/grafana-alertcheck/cmd/table_test.go b/grafana-alertcheck/cmd/table_test.go index db5c68929..a2b58d6d0 100644 --- a/grafana-alertcheck/cmd/table_test.go +++ b/grafana-alertcheck/cmd/table_test.go @@ -60,6 +60,7 @@ func TestRenderTable(t *testing.T) { require.Contains(t, out, "Zebra Alert") require.Contains(t, out, "healthy") require.Contains(t, out, "WINDOW COVERED") + require.Contains(t, out, "SOURCE") // The violations section must show up even without --output json, and must // carry the --allow-paused hint text verbatim. INSTANCES is a single word @@ -92,6 +93,14 @@ func TestRenderTable(t *testing.T) { require.Contains(t, out, "❌ violations: 2") } +// The DETAILS cell drops the `rule "<title>": ` prefix the JSON notes carry, so +// the ALERT column is not repeated in every cell. +func TestDetails_StripsRulePrefix(t *testing.T) { + require.Equal(t, "", details("A", "")) + require.Equal(t, "gap of 5m0s", details("A", `rule "A": gap of 5m0s`)) + require.Equal(t, "gap; health=error", details("A", `rule "A": gap; rule "A": health=error`)) +} + // The footer verdict line picks its emoji by whether there are violations. func TestViolationsLabel(t *testing.T) { require.Equal(t, "✅ violations: 0", violationsLabel(0, false)) diff --git a/grafana-alertcheck/docs/architecture.md b/grafana-alertcheck/docs/architecture.md index 0c3766937..8bb06816e 100644 --- a/grafana-alertcheck/docs/architecture.md +++ b/grafana-alertcheck/docs/architecture.md @@ -36,7 +36,7 @@ The `Source` interface is the only HTTP boundary and covers both rule kinds: `Gr ### Key vs uid -A rule's map key is its **key**, not its uid. For a Grafana-managed rule the key *is* the uid; a datasource-managed rule has no uid, so its key is a JSON-encoded `(datasource, group, name)` tuple. The key, not the uid, is what `rt`, the scheduler, the `Reducer`, `pausedAtStart`, exclusions, `Coverage`, `Thresholds` and `Verdicts` are indexed by. The log records both: `key` and, for a Grafana rule, `uid`; the poll reader uses `rule_key` when present and falls back to `rule_uid`, so an old v1 log stays readable. Identity is not weakened — for Grafana-managed rules behavior is byte-for-byte unchanged, because key == uid there. +A rule's map key is its **key**, not its uid. For a Grafana-managed rule the key *is* the uid; a datasource-managed rule has none, so its key is a JSON `(datasource, group, name, file)` tuple — file included because a Prometheus group name is only unique within a file. Every internal map is indexed by key. The log records `key` and, for a Grafana rule, `uid`; the reader uses `rule_key` when present and falls back to `rule_uid`, so old v1 logs stay readable. Grafana behavior is unchanged because key == uid there. - `proveCoverage` (the nine coverage checks) and `decide` (the instance timelines and outcomes) are pure; tests drive them with `[]Poll` literals and a fake `Clock`, with no sleeping or fixture server. - `Check`/`Watch` are I/O shells: HTTP, signals, the pidfile, file reads, the countdown print. The only test doubles needed are the `Source` and `Clock` interfaces. diff --git a/grafana-alertcheck/docs/reference/cli.md b/grafana-alertcheck/docs/reference/cli.md index 251e24dd3..1c2fe1844 100644 --- a/grafana-alertcheck/docs/reference/cli.md +++ b/grafana-alertcheck/docs/reference/cli.md @@ -98,7 +98,7 @@ By default `check` **exits early** on a failure that cannot become a pass: a pos ## Naming alerts -Alert names take one of these forms. Grafana-managed rules use folder/group; datasource-managed rules use datasource/group, and are auto-discovered — there is no selection flag. +Alert names take one of these forms. Grafana-managed rules use folder/group; datasource-managed rules are auto-discovered (no selection flag) and use datasource/group. | Form | Meaning | | ---- | ------- | @@ -110,9 +110,11 @@ Alert names take one of these forms. Grafana-managed rules use folder/group; dat | `uid:abc123` | Exact Grafana uid | | `key:ds:[…]` | Exact rule key across both kinds (copyable from `list`) | -`--folder` scopes a bare Grafana title only; it does not apply to datasource-managed rules. A recording rule is refused with a specific error, as is a datasource-managed rule whose datasource could not be identified. A no-match errors with case-insensitive substring suggestions and points at `list`. A name matching multiple rules errors listing every candidate with its copyable full name, its source and its `uid:`/`key:` form. Duplicate names that resolve to the same rule collapse to one (a note, not an error). +A datasource rule's **name can itself contain `/`** (e.g. `devex-cicd/prod/griddle-github: ContainersNotReady`). The exact name is tried first, so the `TITLE` from `list` always resolves, and `key:` is the unambiguous fallback. -Auto-discovery reads `/api/datasources` and keeps only `type == "prometheus"` with `jsonData.manageAlerts == true`, then probes each. The token needs `datasources:read` plus datasource query permission; a failure names the permission. +`--folder` scopes a bare Grafana title only. A recording rule, or a datasource rule with no identifiable datasource, is refused with a specific error; a no-match suggests substrings and points at `list`; an ambiguous name lists every candidate with its full name, source and `uid:`/`key:`. Duplicate names collapse to one (a note, not an error). + +Auto-discovery keeps `/api/datasources` entries with `type == "prometheus"` and `jsonData.manageAlerts == true`, then probes each. The token needs `datasources:read` plus datasource query permission; a failure names the permission. ## Selecting alerts by labels @@ -131,7 +133,9 @@ The label flags cannot be combined with `--alerts` or `--folder`, and they are r ## Output and exit codes -The human table goes to **stderr**: `RESULTS` (one row per rule, with the verdict, time broken, check cadence and whether the window was observed), `VIOLATIONS` (one per distinct rule/verdict/state/health/note signature, with an `INSTANCES` count of the instances it stands for — instance identity is only in the JSON), and `LIMITS USED` (each rule's observation limits in plain words, explained by a legend under the table, plus the extra observation time, the evaluation wait, the largest measured clock difference and the detected Grafana version; the closing violations count is marked ✅/❌). The JSON outcome values are `healthy`, `new_failure`, `still_failing`, `recovered`, `unstable`, `paused`, `not_verified` and the synthetic `not_counted`. `--output json` writes the result to stdout. +The human table goes to **stderr**: `RESULTS` (one row per rule: verdict, time broken, check cadence, whether the window was observed, and `SOURCE` — `grafana` or `datasource`), `VIOLATIONS` (one per distinct rule/verdict/state/health/note signature, with an `INSTANCES` count — instance identity is only in the JSON), and `LIMITS USED` (each rule's observation limits in plain words, explained by a legend, plus the extra observation time, the evaluation wait, the largest measured clock difference and the Grafana version; the closing violations count is marked ✅/❌). + +`DETAILS` carries only rule-specific notes; kind-level caveats (datasource rules have no pause signal and treat a departure as a recovery) are printed once, before the table. The JSON outcome values are `healthy`, `new_failure`, `still_failing`, `recovered`, `unstable`, `paused`, `not_verified` and the synthetic `not_counted`; `--output json` adds each rule's `source_kind` and the run-level `caveats`, and writes the result to stdout. | Code | Meaning | | ---- | ------- | diff --git a/grafana-alertcheck/docs/reference/log-format.md b/grafana-alertcheck/docs/reference/log-format.md index 239da9845..d9e52f454 100644 --- a/grafana-alertcheck/docs/reference/log-format.md +++ b/grafana-alertcheck/docs/reference/log-format.md @@ -48,7 +48,7 @@ The header must be line 1, appear once, and carry `schema_version` `1` (any othe "poll_every_seconds": 30 }, { - "key": "ds:[\"vm\",\"ExampleMetrics\",\"ExampleTargetDown\"]", + "key": "ds:[\"vm\",\"ExampleMetrics\",\"ExampleTargetDown\",\"/etc/vm/rules/example.yml\"]", "uid": "", "title": "ExampleTargetDown", "group": "ExampleMetrics", diff --git a/grafana-alertcheck/internal/gate/check.go b/grafana-alertcheck/internal/gate/check.go index 6f36c7b47..a217f6f84 100644 --- a/grafana-alertcheck/internal/gate/check.go +++ b/grafana-alertcheck/internal/gate/check.go @@ -276,6 +276,11 @@ func check(ctx context.Context, cfg Config, src Source) (Result, error) { for _, n := range notes { fmt.Fprintf(cfg.Notes, "note: %s\n", n) } + // Kind-level caveats are printed once, here, rather than repeated in every + // datasource rule's per-row details. + for _, c := range datasourceCaveats(resolved) { + fmt.Fprintf(cfg.Notes, "note: %s\n", c) + } if len(cfg.IncludeLabels) > 0 { printLabelSelection(cfg.Notes, resolved, cfg.IncludeLabels, cfg.ExcludeLabels, len(cfg.ExcludeAlerts)) } diff --git a/grafana-alertcheck/internal/gate/check_ds_test.go b/grafana-alertcheck/internal/gate/check_ds_test.go index 636c65c49..42e7b2ee0 100644 --- a/grafana-alertcheck/internal/gate/check_ds_test.go +++ b/grafana-alertcheck/internal/gate/check_ds_test.go @@ -49,5 +49,7 @@ func TestCheck_DatasourceFireAndResolveIsRecovered(t *testing.T) { require.NoError(t, err) require.Empty(t, res.Violations) require.Equal(t, OutcomeRecovered, res.Verdicts[0].Outcome) - require.Contains(t, res.Verdicts[0].Note, "check 7 skipped") + require.NotContains(t, res.Verdicts[0].Note, "check 7 skipped") + require.Contains(t, notesOf(cfg), "pause is not observable") + require.Contains(t, notesOf(cfg), "treated as a recovery") } diff --git a/grafana-alertcheck/internal/gate/classify.go b/grafana-alertcheck/internal/gate/classify.go index aa17e4d45..079ab7a06 100644 --- a/grafana-alertcheck/internal/gate/classify.go +++ b/grafana-alertcheck/internal/gate/classify.go @@ -82,10 +82,13 @@ type Violation struct { type RuleVerdict struct { Alert, RuleUID string RuleKey string `json:"rule_key,omitempty"` - Outcome Outcome - BadFor time.Duration // total wall-clock time any instance was bad inside the window, overlaps merged - PollEvery time.Duration - Note string + // SourceKind is "grafana" or "datasource" (sourceKind), so a reader can see + // which classification semantics apply without prose. + SourceKind string `json:"source_kind,omitempty"` + Outcome Outcome + BadFor time.Duration // total wall-clock time any instance was bad inside the window, overlaps merged + PollEvery time.Duration + Note string } // Policy is decide's narrowed, pure-layer view of a Config: the classification @@ -143,6 +146,10 @@ type Result struct { Global GlobalThresholds Verdicts []RuleVerdict Violations []Violation + // Caveats are run-level, kind-level policy notes (e.g. datasource-managed + // pause is unobservable). They are not per-rule details; the CLI prints them + // once so they never bloat the table. + Caveats []string `json:"caveats,omitempty"` // TerminatedEarly is set only when fail-fast stopped before the window // closed; the coverage proof is then over [from, At]. To and Global below // still report the requested values. Published JSON output. @@ -516,6 +523,7 @@ func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition, GraceSource: graceSourceOrNone(gt.graceSource), DrainTimeout: gt.drainTimeout, }, + Caveats: datasourceCaveats(defs), } skewSeen := false for _, p := range polls { @@ -558,7 +566,8 @@ func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition, if pausedAtStart[key] { pausedRules = append(pausedRules, def) result.Verdicts = append(result.Verdicts, RuleVerdict{ - Alert: def.Title, RuleKey: key, RuleUID: def.UID, Outcome: OutcomePaused, + Alert: def.Title, RuleKey: key, RuleUID: def.UID, + SourceKind: sourceKind(def.Kind), Outcome: OutcomePaused, PollEvery: rt[key].pollEvery, Note: "paused before the window opened", }) @@ -587,7 +596,8 @@ func decide(h Header, polls []Poll, sentinel *time.Time, defs []Definition, } result.Violations = append(result.Violations, viols...) result.Verdicts = append(result.Verdicts, RuleVerdict{ - Alert: def.Title, RuleKey: key, RuleUID: def.UID, Outcome: outcome, BadFor: badFor, + Alert: def.Title, RuleKey: key, RuleUID: def.UID, + SourceKind: sourceKind(def.Kind), Outcome: outcome, BadFor: badFor, PollEvery: t.pollEvery, Note: strings.Join(cov.Notes, "; "), }) } diff --git a/grafana-alertcheck/internal/gate/coverage.go b/grafana-alertcheck/internal/gate/coverage.go index e9b0e083e..9f1d2358f 100644 --- a/grafana-alertcheck/internal/gate/coverage.go +++ b/grafana-alertcheck/internal/gate/coverage.go @@ -2,6 +2,8 @@ package gate import ( "fmt" + "sort" + "strings" "time" ) @@ -175,13 +177,8 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t RuleTimings, d // rule, a stopped scheduler, a blocked evaluation). This is what catches // pause-then-unpause, which the drain wait alone passes. A // datasource-managed rule has no pause signal at all, so the check is - // skipped with an explicit note rather than passed silently. - if def.Kind == KindDatasourceManaged && !def.PauseObservable { - res.Notes = append(res.Notes, fmt.Sprintf( - "rule %q: pause is not observable for a datasource-managed rule; check 7 skipped", def.Title)) - res.Notes = append(res.Notes, fmt.Sprintf( - "rule %q: a datasource-managed instance leaving the active set is treated as a recovery (a vanished series is indistinguishable from a resolution)", def.Title)) - } else { + // skipped; the run-level caveat is printed once by datasourceCaveats. + if def.Kind != KindDatasourceManaged || def.PauseObservable { var pausedCount int var pausedAt time.Time for _, p := range inWindow { @@ -243,6 +240,37 @@ func proveCoverage(h Header, polls []Poll, sentinel *time.Time, t RuleTimings, d return res } +// datasourceCaveats is the one-time, kind-level caveat set for +// datasource-managed rules. These are policy facts that apply to every such +// rule — the Prometheus API has no isPaused signal, and returns only active +// instances — so they are reported once for the run, never per rule. +func datasourceCaveats(defs []Definition) []string { + var names []string + seen := make(map[string]bool) + for _, d := range defs { + if d.Kind != KindDatasourceManaged { + continue + } + name := d.DatasourceName + if name == "" { + name = d.DatasourceUID + } + if name != "" && !seen[name] { + seen[name] = true + names = append(names, name) + } + } + if len(names) == 0 { + return nil + } + sort.Strings(names) + scope := "datasource-managed rules (" + strings.Join(names, ", ") + ")" + return []string{ + scope + ": pause is not observable, so check 7 is skipped", + scope + ": an instance leaving the active set is treated as a recovery (a vanished series is indistinguishable from a resolution)", + } +} + // inWindowPolls filters to polls inside [from, windowEnd] via the cross-domain // membership test: each GrafanaNow is translated to the runner domain by its // own skew, widened by its skew bound, so clock imprecision never excludes a diff --git a/grafana-alertcheck/internal/gate/datasource_semantics_test.go b/grafana-alertcheck/internal/gate/datasource_semantics_test.go index 6eab70851..56dd05e4b 100644 --- a/grafana-alertcheck/internal/gate/datasource_semantics_test.go +++ b/grafana-alertcheck/internal/gate/datasource_semantics_test.go @@ -9,7 +9,7 @@ import ( func dsDef(name string) Definition { return Definition{ - Key: ruleKey("vm", "G", name, ""), Title: name, Group: "G", + Key: ruleKey("vm", "G", name, "f", ""), Title: name, Group: "G", File: "f", Kind: KindDatasourceManaged, DatasourceUID: "vm", DatasourceName: "VM", IntervalSeconds: 60, } @@ -20,6 +20,19 @@ func dsPoll(key string, at time.Time, health string) Poll { return Poll{RuleKey: key, GrafanaNow: at, Found: true, Health: health, LastEvaluation: at} } +func TestDatasourceCaveats(t *testing.T) { + require.Empty(t, datasourceCaveats(nil)) + require.Empty(t, datasourceCaveats([]Definition{{Kind: KindGrafanaManaged}})) + + a, b, c := dsDef("A"), dsDef("B"), dsDef("C") + c.DatasourceName = "Mimir" + got := datasourceCaveats([]Definition{a, b, c}) + require.Len(t, got, 2) + require.Contains(t, got[0], "pause is not observable") + require.Contains(t, got[0], "Mimir, VM") + require.Contains(t, got[1], "treated as a recovery") +} + // A datasource instance that is bad at `from` and then leaves the active set is // a recovery: Reduce turns the departure into Cleared, so classifyRule sees a // real clear and the run passes. @@ -52,7 +65,11 @@ func TestDecide_DatasourceDepartureIsRecovered(t *testing.T) { require.NoError(t, err) require.Empty(t, res.Violations) require.Equal(t, OutcomeRecovered, res.Verdicts[0].Outcome) - require.Contains(t, res.Verdicts[0].Note, "treated as a recovery") + // The recovery caveat is run-level, not repeated in the per-rule note. + require.NotContains(t, res.Verdicts[0].Note, "treated as a recovery") + require.Equal(t, "datasource", res.Verdicts[0].SourceKind) + require.Len(t, res.Caveats, 2) + require.Contains(t, res.Caveats[1], "treated as a recovery") } // The same shape for a Grafana rule, but a VANISH rather than a clear, stays diff --git a/grafana-alertcheck/internal/gate/identity.go b/grafana-alertcheck/internal/gate/identity.go index 361243530..88f5f23af 100644 --- a/grafana-alertcheck/internal/gate/identity.go +++ b/grafana-alertcheck/internal/gate/identity.go @@ -2,20 +2,18 @@ package gate import "encoding/json" -// dsKeyPrefix marks a datasource-managed key. The key, not the prefix, is the -// identity; the prefix only lets a caller that has lost the Definition (an -// absent-rule poll, a log read) still tell the two source kinds apart. +// dsKeyPrefix marks a datasource-managed key, so a caller without the +// Definition can still tell the two source kinds apart. const dsKeyPrefix = "ds:" -// ruleKey is the one map key for a rule across both source kinds. Grafana-managed -// rules keep their uid. Datasource-managed rules have no uid, so they get a -// JSON-encoded tuple; JSON keeps group/name separators from colliding. This is a -// key, not an identity the API gave us — Definition.UID stays empty for ds rules. -func ruleKey(dsUID, group, name, uid string) string { +// ruleKey is the one map key across both source kinds: a Grafana rule keeps its +// uid; a datasource rule has none, so it gets a JSON tuple (file included — a +// Prometheus group name is only unique within a file). +func ruleKey(dsUID, group, name, file, uid string) string { if uid != "" { return uid } - b, _ := json.Marshal([3]string{dsUID, group, name}) + b, _ := json.Marshal([4]string{dsUID, group, name, file}) return dsKeyPrefix + string(b) } diff --git a/grafana-alertcheck/internal/gate/identity_test.go b/grafana-alertcheck/internal/gate/identity_test.go index 8099e6f91..961b68cb8 100644 --- a/grafana-alertcheck/internal/gate/identity_test.go +++ b/grafana-alertcheck/internal/gate/identity_test.go @@ -7,18 +7,21 @@ import ( ) func TestRuleKey_GrafanaKeepsUID(t *testing.T) { - require.Equal(t, "rule1", ruleKey("", "", "title", "rule1")) + require.Equal(t, "rule1", ruleKey("", "", "title", "", "rule1")) } -// A ds key must not collide with a Grafana uid and must not let group/name -// separators collide: a name containing ":" or "/" is still a distinct tuple. +// A ds key must not collide with a Grafana uid and must not let separators +// collide: a name containing ":" or "/" is still a distinct tuple, and the +// same group/name in two files is two rules. func TestRuleKey_DatasourceInjectivity(t *testing.T) { keys := []string{ - ruleKey("dsA", "g", "n", ""), - ruleKey("dsA", "g/n", "", ""), - ruleKey("dsA", "g", "/n", ""), - ruleKey("dsB", "g", "n", ""), - ruleKey("", "g", "n", ""), + ruleKey("dsA", "g", "n", "f", ""), + ruleKey("dsA", "g/n", "", "f", ""), + ruleKey("dsA", "g", "/n", "f", ""), + ruleKey("dsB", "g", "n", "f", ""), + ruleKey("", "g", "n", "f", ""), + ruleKey("dsA", "g", "n", "f1", ""), + ruleKey("dsA", "g", "n", "f2", ""), } seen := map[string]bool{} for _, k := range keys { @@ -26,7 +29,7 @@ func TestRuleKey_DatasourceInjectivity(t *testing.T) { require.False(t, seen[k], "key %q collided", k) seen[k] = true } - require.Equal(t, "u1", ruleKey("dsA", "g", "n", "u1"), "a uid wins over the ds tuple") + require.Equal(t, "u1", ruleKey("dsA", "g", "n", "f", "u1"), "a uid wins over the ds tuple") } func TestDefKey_FallsBackToUID(t *testing.T) { diff --git a/grafana-alertcheck/internal/gate/labels.go b/grafana-alertcheck/internal/gate/labels.go index b9c018d4f..2215020c6 100644 --- a/grafana-alertcheck/internal/gate/labels.go +++ b/grafana-alertcheck/internal/gate/labels.go @@ -25,7 +25,7 @@ func SelectByLabels(defs []Definition, include, exclude []LabelMatcher) ([]Defin continue } matchedInclude++ - if d.Kind == KindRecording || (d.Kind == KindDatasourceManaged && d.DatasourceUID == "") { + if !isSupported(d) { return nil, fmt.Errorf("label selection matches %q, a %s, which is not supported", d.Title, kindName(d.Kind)) } if matchesAny(d.Labels, exclude) { diff --git a/grafana-alertcheck/internal/gate/load.go b/grafana-alertcheck/internal/gate/load.go index cc15c9af7..f6b17d8a3 100644 --- a/grafana-alertcheck/internal/gate/load.go +++ b/grafana-alertcheck/internal/gate/load.go @@ -13,11 +13,9 @@ func ListAllDefinitions(ctx context.Context, src Source) ([]Definition, error) { } // loadDefinitions reads Grafana-managed definitions from the ruler and -// datasource-managed definitions from every discovered rule source. wantAll -// fetches every ds rule (label selection, list); otherwise only the named rules -// are requested, one request per source. Ruler-returned datasource-managed rules -// are dropped: discovery is the authority for them, since only it knows the -// datasource UID. +// datasource-managed definitions from every discovered source. wantAll fetches +// every ds rule; otherwise only the named rules are requested. Ruler-returned ds +// rules are dropped — only discovery knows their datasource UID. func loadDefinitions(ctx context.Context, src Source, names []string, wantAll bool) ([]Definition, error) { grafana, err := src.GrafanaDefinitions(ctx) if err != nil { @@ -55,10 +53,18 @@ func loadDefinitions(ctx context.Context, src Source, names []string, wantAll bo return defs, nil } -// dsFilterNames extracts the server-side rule_name[] filters from the raw alert -// names. A key: or uid: form names no title, so the whole source must be -// fetched; otherwise the last /-separated segment is the title. -func dsFilterNames(names []string) (titles []string, fetchAll bool) { +// dsFilterNames extracts the server-side rule_name[] filters. A key: or uid: +// form forces a bulk fetch; otherwise both the whole input and its last +// /-separated segment are sent (a ds rule's name can itself contain "/", while +// Group/Title names it by the trailing segment). +func dsFilterNames(names []string) (filters []string, fetchAll bool) { + seen := make(map[string]bool) + add := func(s string) { + if s != "" && !seen[s] { + seen[s] = true + filters = append(filters, s) + } + } for _, raw := range names { n := strings.TrimSpace(raw) if n == "" { @@ -67,8 +73,10 @@ func dsFilterNames(names []string) (titles []string, fetchAll bool) { if strings.HasPrefix(n, "key:") || strings.HasPrefix(n, "uid:") { return nil, true } - parts := strings.Split(n, "/") - titles = append(titles, parts[len(parts)-1]) + add(n) + if i := strings.LastIndex(n, "/"); i != -1 { + add(n[i+1:]) + } } - return titles, false + return filters, false } diff --git a/grafana-alertcheck/internal/gate/log_ds_test.go b/grafana-alertcheck/internal/gate/log_ds_test.go index 16dc9f0bb..ae388caa5 100644 --- a/grafana-alertcheck/internal/gate/log_ds_test.go +++ b/grafana-alertcheck/internal/gate/log_ds_test.go @@ -9,8 +9,8 @@ import ( func dsRule(uid, name string, insts ...Instance) StateRule { return StateRule{ - Key: ruleKey("vm", "G", name, ""), DatasourceUID: "vm", - Title: name, Group: "G", Type: "alerting", Health: "ok", + Key: ruleKey("vm", "G", name, "f", ""), DatasourceUID: "vm", + Title: name, Group: "G", File: "f", Type: "alerting", Health: "ok", LastEvaluation: testNow, Instances: insts, } } @@ -20,7 +20,7 @@ func dsRule(uid, name string, insts ...Instance) StateRule { func TestReduce_DatasourceDepartureIsCleared(t *testing.T) { firing := Instance{Labels: map[string]string{"x": "y"}, State: StateFiring, ActiveAt: testNow} r := NewReducer() - key := ruleKey("vm", "G", "A", "") + key := ruleKey("vm", "G", "A", "f", "") r.Reduce(key, observation(testNow, dsRule("", "A", firing))) p := r.Reduce(key, observation(testNow.Add(time.Minute), dsRule("", "A"))) require.Equal(t, []string{instanceKey(firing.Labels)}, p.Cleared) @@ -41,7 +41,7 @@ func TestReduce_GrafanaDepartureIsVanished(t *testing.T) { // A datasource poll records the key but no uid. func TestReduce_DatasourcePollCarriesKeyNotUID(t *testing.T) { r := NewReducer() - key := ruleKey("vm", "G", "A", "") + key := ruleKey("vm", "G", "A", "f", "") p := r.Reduce(key, observation(testNow, dsRule("", "A"))) require.Equal(t, key, p.RuleKey) require.Empty(t, p.RuleUID) diff --git a/grafana-alertcheck/internal/gate/parse_datasource.go b/grafana-alertcheck/internal/gate/parse_datasource.go index 02d1ed978..99251bf85 100644 --- a/grafana-alertcheck/internal/gate/parse_datasource.go +++ b/grafana-alertcheck/internal/gate/parse_datasource.go @@ -13,7 +13,7 @@ import ( // states, health "err" (not "error"), and a zero lastEvaluation is allowed // (liveness treats zero as maximally stale). A missing or unparseable required // field is an error, never a zero value. -func ParseDatasourceRules(body []byte, dsUID, dsName string) ([]StateRule, error) { +func ParseDatasourceRules(body []byte, dsUID string) ([]StateRule, error) { var top map[string]json.RawMessage if err := json.Unmarshal(body, &top); err != nil { return nil, fmt.Errorf("datasource rules response: %w", err) @@ -79,7 +79,7 @@ func parseDatasourceRule(raw json.RawMessage, dsUID, group, file string, interva } r := StateRule{ - Key: ruleKey(dsUID, group, name, ""), + Key: ruleKey(dsUID, group, name, file, ""), Title: name, Group: group, File: file, diff --git a/grafana-alertcheck/internal/gate/parse_datasource_test.go b/grafana-alertcheck/internal/gate/parse_datasource_test.go index 6ee448999..70935a488 100644 --- a/grafana-alertcheck/internal/gate/parse_datasource_test.go +++ b/grafana-alertcheck/internal/gate/parse_datasource_test.go @@ -8,7 +8,7 @@ import ( ) func TestParseDatasourceRules_Fixture(t *testing.T) { - rules, err := ParseDatasourceRules(readFixture(t, "ds_rules.json"), "ds-uid", "ExampleMetrics") + rules, err := ParseDatasourceRules(readFixture(t, "ds_rules.json"), "ds-uid") require.NoError(t, err) require.Len(t, rules, 2, "recording rules are parsed but filtered later") @@ -23,14 +23,14 @@ func TestParseDatasourceRules_Fixture(t *testing.T) { require.Equal(t, "firing", alert.State) require.Equal(t, "ok", alert.Health) require.Empty(t, alert.UID, "a datasource rule has no uid") - require.Equal(t, ruleKey("ds-uid", "ExampleMetrics", "ExampleTargetDown", ""), alert.Key) + require.Equal(t, ruleKey("ds-uid", "ExampleMetrics", "ExampleTargetDown", "/etc/vm/rules/example.yml", ""), alert.Key) require.Len(t, alert.Instances, 1) require.Equal(t, StateFiring, alert.Instances[0].State) require.Nil(t, alert.Totals) } func TestDefinitionsFromDatasource_FiltersRecording(t *testing.T) { - rules, err := ParseDatasourceRules(readFixture(t, "ds_rules.json"), "ds-uid", "ExampleMetrics") + rules, err := ParseDatasourceRules(readFixture(t, "ds_rules.json"), "ds-uid") require.NoError(t, err) defs := DefinitionsFromDatasource(rules, "ds-uid", "ExampleMetrics") @@ -44,7 +44,7 @@ func TestDefinitionsFromDatasource_FiltersRecording(t *testing.T) { func TestParseDatasourceRules_HealthErrNormalizes(t *testing.T) { body := []byte(`{"status":"success","data":{"groups":[{"name":"g","file":"f","interval":60,"rules":[ {"name":"A","type":"alerting","health":"err","lastEvaluation":"2026-08-01T00:00:00Z","state":"firing"}]}]}}`) - rules, err := ParseDatasourceRules(body, "d", "n") + rules, err := ParseDatasourceRules(body, "d") require.NoError(t, err) require.Equal(t, "error", rules[0].Health) } @@ -52,7 +52,7 @@ func TestParseDatasourceRules_HealthErrNormalizes(t *testing.T) { func TestParseDatasourceRules_ZeroLastEvaluationAllowed(t *testing.T) { body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ {"name":"A","type":"alerting","health":"ok","state":"pending"}]}]}}`) - rules, err := ParseDatasourceRules(body, "d", "n") + rules, err := ParseDatasourceRules(body, "d") require.NoError(t, err) require.True(t, rules[0].LastEvaluation.IsZero()) } @@ -61,7 +61,7 @@ func TestParseDatasourceRules_UnknownInstanceStateIsError(t *testing.T) { body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ {"name":"A","type":"alerting","health":"ok","state":"firing","alerts":[ {"labels":{},"state":"inactive","activeAt":"2026-08-01T00:00:00Z"}]}]}]}}`) - _, err := ParseDatasourceRules(body, "d", "n") + _, err := ParseDatasourceRules(body, "d") require.Error(t, err) require.Contains(t, err.Error(), "unrecognized datasource instance state") } @@ -70,7 +70,7 @@ func TestParseDatasourceRules_PendingInstance(t *testing.T) { body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ {"name":"A","type":"alerting","health":"ok","state":"pending","alerts":[ {"labels":{"x":"y"},"state":"pending","activeAt":"2026-08-01T00:00:00Z","value":"1"}]}]}]}}`) - rules, err := ParseDatasourceRules(body, "d", "n") + rules, err := ParseDatasourceRules(body, "d") require.NoError(t, err) require.Equal(t, StatePending, rules[0].Instances[0].State) } diff --git a/grafana-alertcheck/internal/gate/resolve.go b/grafana-alertcheck/internal/gate/resolve.go index 030357864..8907040d6 100644 --- a/grafana-alertcheck/internal/gate/resolve.go +++ b/grafana-alertcheck/internal/gate/resolve.go @@ -46,17 +46,15 @@ func Resolve(defs []Definition, names []string, folder string) (resolved []Defin return resolved, notes, nil } -// resolveOne resolves a single trimmed, non-empty name against defs: one match -// wins outright, zero is an error with suggestions, two or more is an error -// listing every candidate. folder scopes a bare Grafana title to one folder; it -// is ignored for the /-separated forms and for datasource-managed rules, which -// name their own group and datasource. +// resolveOne resolves one trimmed, non-empty name against defs: one match wins, +// zero is an error with suggestions, two or more is ambiguous. folder scopes a +// bare Grafana title; it is ignored for /-separated forms and for datasource +// rules. // -// A name matches both kinds: Grafana forms are Title | Folder/Title | -// Folder/Group/Title, datasource forms are Title | Group/Title | -// DatasourceName/Group/Title, and an exact key: is unambiguous across both. -// Unsupported kinds (recording, and a datasource-managed rule with no -// datasource) are refused, and only supported candidates count for ambiguity. +// Grafana forms: Title | Folder/Title | Folder/Group/Title. Datasource forms: +// Title | Group/Title | DatasourceName/Group/Title. key: is exact across both. +// Recording rules and datasource rules with no datasource are refused, and only +// supported candidates count for ambiguity. func resolveOne(defs []Definition, name, folder string) (Definition, error) { if key, ok := strings.CutPrefix(name, "key:"); ok { if key != "" { @@ -89,64 +87,109 @@ func resolveOne(defs []Definition, name, folder string) (Definition, error) { return Definition{}, fmt.Errorf("no rule matched %q: no rule has this uid (run 'grafana-alertcheck list' to see uids)", name) } + // A datasource rule's name can contain "/", so the full input may be a title, + // not a segmented form: try an exact title match before splitting. + if def, found, err := pickCandidate(defs, name, func(d Definition) bool { + return titleMatches(d, name, folder) + }); found { + return def, err + } + parts, err := parseNameForm(name) if err != nil { return Definition{}, err } + if def, found, err := pickCandidate(defs, name, func(d Definition) bool { + return matchesName(d, parts, folder) + }); found { + return def, err + } + return Definition{}, noMatchError(supportedDefs(defs), name, parts[len(parts)-1]) +} - var supportedCandidates, unsupportedCandidates []Definition +// pickCandidate applies the shared one-match/ambiguous/unsupported/no-match +// policy to a candidate predicate. found is false when nothing matched, so the +// caller can try the next interpretation. +func pickCandidate(defs []Definition, name string, match func(Definition) bool) (Definition, bool, error) { + var supported, unsupported []Definition for _, d := range defs { - if !matchesName(d, parts, folder) { + if !match(d) { continue } - if d.Kind == KindRecording || (d.Kind == KindDatasourceManaged && d.DatasourceUID == "") { - unsupportedCandidates = append(unsupportedCandidates, d) + if isSupported(d) { + supported = append(supported, d) } else { - supportedCandidates = append(supportedCandidates, d) + unsupported = append(unsupported, d) } } - switch { - case len(supportedCandidates) == 1: - return supportedCandidates[0], nil - case len(supportedCandidates) > 1: - return Definition{}, ambiguousError(name, supportedCandidates) - case len(unsupportedCandidates) > 0: - return refuseUnsupportedKind(name, unsupportedCandidates[0]) + case len(supported) == 1: + return supported[0], true, nil + case len(supported) > 1: + return Definition{}, true, ambiguousError(name, supported) + case len(unsupported) > 0: + def, err := refuseUnsupportedKind(name, unsupported[0]) + return def, true, err default: - return Definition{}, noMatchError(supportedDefs(defs), name, parts[len(parts)-1]) + return Definition{}, false, nil } } -// matchesName reports whether d matches the /-separated name form. A bare -// Grafana title is scoped by folder; datasource forms never use folder. +// titleMatches is the exact-title interpretation: a datasource rule matches by +// its full name, a Grafana rule by its title scoped to --folder. +func titleMatches(d Definition, name, folder string) bool { + if d.Kind == KindDatasourceManaged { + return d.Title == name + } + return (folder == "" || d.Folder == folder) && d.Title == name +} + +// matchesName reports whether d matches the /-separated name form. Only +// datasource-managed rules use the datasource forms; every other kind — +// Grafana-managed and recording alike — uses the folder/group forms, so a +// recording rule named by its real path is still matched and refused +// specifically rather than falling through to a generic no-match. func matchesName(d Definition, parts []string, folder string) bool { switch len(parts) { case 1: - if d.Kind == KindGrafanaManaged { - return (folder == "" || d.Folder == folder) && d.Title == parts[0] + if d.Kind == KindDatasourceManaged { + return d.Title == parts[0] } - return d.Title == parts[0] + return (folder == "" || d.Folder == folder) && d.Title == parts[0] case 2: - if d.Kind == KindGrafanaManaged { - return d.Folder == parts[0] && d.Title == parts[1] + if d.Kind == KindDatasourceManaged { + return d.Group == parts[0] && d.Title == parts[1] } - return d.Group == parts[0] && d.Title == parts[1] + return d.Folder == parts[0] && d.Title == parts[1] case 3: - if d.Kind == KindGrafanaManaged { - return d.Folder == parts[0] && d.Group == parts[1] && d.Title == parts[2] + if d.Kind == KindDatasourceManaged { + return d.DatasourceName == parts[0] && d.Group == parts[1] && d.Title == parts[2] } - return d.DatasourceName == parts[0] && d.Group == parts[1] && d.Title == parts[2] + return d.Folder == parts[0] && d.Group == parts[1] && d.Title == parts[2] } return false } +// isSupported reports whether a definition can be observed: not a recording +// rule, and not a datasource-managed rule with no datasource. The one predicate +// for resolve, label selection and the no-match surfaces, so they cannot drift. +func isSupported(d Definition) bool { + switch d.Kind { + case KindRecording: + return false + case KindDatasourceManaged: + return d.DatasourceUID != "" + default: + return true + } +} + // supportedDefs filters out the refused kinds. Only these participate in // name-based matching, the no-match rule count, and substring suggestions. func supportedDefs(defs []Definition) []Definition { out := make([]Definition, 0, len(defs)) for _, d := range defs { - if d.Kind == KindRecording || (d.Kind == KindDatasourceManaged && d.DatasourceUID == "") { + if !isSupported(d) { continue } out = append(out, d) @@ -173,14 +216,13 @@ func parseNameForm(name string) ([]string, error) { // refuseUnsupportedKind rejects the unsupported kinds with a clear, specific // error — distinct from "no match" and from "ambiguous". func refuseUnsupportedKind(name string, d Definition) (Definition, error) { - switch { - case d.Kind == KindRecording: - return Definition{}, fmt.Errorf("%q resolves to %s, a recording rule, which is not supported", name, d.Title) - case d.Kind == KindDatasourceManaged && d.DatasourceUID == "": - return Definition{}, fmt.Errorf("%q resolves to %s, a datasource-managed rule whose datasource is unknown, which is not supported", name, d.Title) - default: + if isSupported(d) { return d, nil } + if d.Kind == KindRecording { + return Definition{}, fmt.Errorf("%q resolves to %s, a recording rule, which is not supported", name, d.Title) + } + return Definition{}, fmt.Errorf("%q resolves to %s, a datasource-managed rule whose datasource is unknown, which is not supported", name, d.Title) } // ruleRefLabel names a rule for a note: a uid for Grafana, a copyable key for a @@ -231,7 +273,7 @@ func ambiguousError(name string, candidates []Definition) error { fmt.Fprintf(&b, "%q matches %d rules; use key: or the full name:", name, len(sorted)) for _, d := range sorted { if d.Kind == KindDatasourceManaged { - fmt.Fprintf(&b, "\n %s/%s/%s (datasource %s, key:%s)", d.DatasourceName, d.Group, d.Title, d.DatasourceName, defKey(d)) + fmt.Fprintf(&b, "\n %s/%s/%s (datasource_uid:%s, key:%s)", d.DatasourceName, d.Group, d.Title, d.DatasourceUID, defKey(d)) continue } fmt.Fprintf(&b, "\n %s/%s/%s (uid:%s)", d.Folder, d.Group, d.Title, d.UID) diff --git a/grafana-alertcheck/internal/gate/resolve_test.go b/grafana-alertcheck/internal/gate/resolve_test.go index 0be60f6ea..44c8dc94c 100644 --- a/grafana-alertcheck/internal/gate/resolve_test.go +++ b/grafana-alertcheck/internal/gate/resolve_test.go @@ -82,9 +82,18 @@ func TestResolve_RefusesDatasourceManaged(t *testing.T) { func TestResolve_RefusesRecording(t *testing.T) { defs, err := ParseDefinitions(readFixture(t, "ruler_recording.json")) require.NoError(t, err) - _, _, err = Resolve(defs, []string{"uid:rule0000011"}, "") - require.Error(t, err) - require.Contains(t, err.Error(), "recording rule") + // Both the uid form and the rule's real Folder/Group/Title path must match + // and be refused specifically — a recording rule must not fall through the + // datasource name forms to a generic no-match. + for _, name := range []string{ + "uid:rule0000011", + "ExampleMetrics/Recording Group/example:recorded_metric:rate5m", + "ExampleMetrics/example:recorded_metric:rate5m", + } { + _, _, err := Resolve(defs, []string{name}, "") + require.Errorf(t, err, "name %q", name) + require.Containsf(t, err.Error(), "recording rule", "name %q", name) + } } func TestResolve_RejectsEmptySegments(t *testing.T) { @@ -198,7 +207,7 @@ func TestResolve_EmptyAndBlankLinesDiscarded(t *testing.T) { func dsResolveDef(ds, dsName, group, title string) Definition { return Definition{ - Key: ruleKey(ds, group, title, ""), Title: title, Group: group, + Key: ruleKey(ds, group, title, "", ""), Title: title, Group: group, Kind: KindDatasourceManaged, DatasourceUID: ds, DatasourceName: dsName, } } @@ -210,7 +219,7 @@ func TestResolve_DatasourceFormsAndKey(t *testing.T) { "ExampleTargetDown", "ExampleMetrics/ExampleTargetDown", "VictoriaMetrics - Prod/ExampleMetrics/ExampleTargetDown", - "key:" + ruleKey("vm", "ExampleMetrics", "ExampleTargetDown", ""), + "key:" + ruleKey("vm", "ExampleMetrics", "ExampleTargetDown", "", ""), } { resolved, _, err := Resolve(defs, []string{name}, "") require.NoErrorf(t, err, "name %q", name) @@ -219,6 +228,21 @@ func TestResolve_DatasourceFormsAndKey(t *testing.T) { } } +// A datasource-managed rule's name can itself contain "/", so the whole input +// must be tried as an exact title before the segmented forms. +func TestResolve_DatasourceNameWithSlashes(t *testing.T) { + name := "devex-cicd/prod/griddle-github: ContainersNotReady" + defs := []Definition{{ + Key: ruleKey("ds", "DevexCICDGriddleGitHubServiceAlerts", name, "f", ""), + Title: name, Group: "DevexCICDGriddleGitHubServiceAlerts", + Kind: KindDatasourceManaged, DatasourceUID: "ds", DatasourceName: "VM", + }} + resolved, _, err := Resolve(defs, []string{name}, "") + require.NoError(t, err) + require.Len(t, resolved, 1) + require.Equal(t, name, resolved[0].Title) +} + func TestResolve_DatasourceAmbiguityAcrossSources(t *testing.T) { defs := []Definition{ dsResolveDef("vm-a", "A", "G", "Same"), diff --git a/grafana-alertcheck/internal/gate/source.go b/grafana-alertcheck/internal/gate/source.go index 345efb6d2..067325fc1 100644 --- a/grafana-alertcheck/internal/gate/source.go +++ b/grafana-alertcheck/internal/gate/source.go @@ -242,11 +242,10 @@ func (s *httpSource) GrafanaDefinitions(ctx context.Context) ([]Definition, erro } // DiscoverRuleSources lists every datasource that can serve Prometheus-flavored -// alerting rules. The filter is strict — type=="prometheus" AND -// jsonData.manageAlerts==true — because the AlertStateHistoryBackend datasource -// shares VictoriaMetrics' backend and would otherwise make every rule name -// ambiguous. Each candidate is probed; a probe failure is a hard error naming -// the datasource, since a silently dropped source is a fail-open. +// rules. The filter is strict (type=="prometheus" AND manageAlerts==true): +// AlertStateHistoryBackend shares VictoriaMetrics' backend, so a looser filter +// would make every rule name ambiguous. A probe failure is a hard error — a +// silently dropped source is a fail-open. func (s *httpSource) DiscoverRuleSources(ctx context.Context) ([]RuleSource, error) { return retryTransport(ctx, s.clock, s.maxSequentialFailures, s.backoffBase, s.backoffCap, func() ([]RuleSource, error) { r, err := s.doRequest(ctx, "/api/datasources") @@ -299,7 +298,7 @@ func (s *httpSource) DatasourceDefinitions(ctx context.Context, src RuleSource, if err != nil { return nil, err } - rules, parseErr := ParseDatasourceRules(r.Body, src.UID, src.Name) + rules, parseErr := ParseDatasourceRules(r.Body, src.UID) if parseErr != nil { return nil, &TransportError{Err: fmt.Errorf("parse datasource rule definitions: %w", parseErr)} } @@ -360,7 +359,7 @@ func (s *httpSource) ruleStateRequest(ref RuleRef) (string, func([]byte) ([]Stat if ref.Kind == KindDatasourceManaged { path := "/api/prometheus/" + url.PathEscape(ref.DatasourceUID) + "/api/v1/rules" + datasourceQuery([]string{ref.Name}, ref.Group, ref.File) - return path, func(b []byte) ([]StateRule, error) { return ParseDatasourceRules(b, ref.DatasourceUID, "") } + return path, func(b []byte) ([]StateRule, error) { return ParseDatasourceRules(b, ref.DatasourceUID) } } path := "/api/prometheus/grafana/api/v1/rules?rule_name=" + url.QueryEscape(ref.Title) return path, ParseState diff --git a/grafana-alertcheck/internal/gate/source_ds_test.go b/grafana-alertcheck/internal/gate/source_ds_test.go index 77f29b43a..65dba239c 100644 --- a/grafana-alertcheck/internal/gate/source_ds_test.go +++ b/grafana-alertcheck/internal/gate/source_ds_test.go @@ -85,9 +85,37 @@ func TestDatasourceDefinitions_FilteredQuery(t *testing.T) { require.Equal(t, "VM", defs[0].DatasourceName) } +func TestDSFilterNames(t *testing.T) { + filters, fetchAll := dsFilterNames([]string{"devex-cicd/prod/griddle-github: ContainersNotReady"}) + require.False(t, fetchAll) + require.ElementsMatch(t, []string{ + "devex-cicd/prod/griddle-github: ContainersNotReady", + "griddle-github: ContainersNotReady", + }, filters) + + _, fetchAll = dsFilterNames([]string{"key:ds:[\"a\"]"}) + require.True(t, fetchAll) +} + +// A datasource rule whose name contains "/" is fetched by its full name, so +// loadDefinitions must request the whole input, not just the last segment. +func TestLoadDefinitions_SlashyDatasourceName(t *testing.T) { + name := "devex-cicd/prod/griddle-github: ContainersNotReady" + f := newFakeSource() + f.ruleSources = []RuleSource{{UID: "vm", Name: "VM"}} + f.dsDefs = map[string][]Definition{"vm": {{ + Key: ruleKey("vm", "G", name, "f", ""), Title: name, Group: "G", + Kind: KindDatasourceManaged, DatasourceUID: "vm", DatasourceName: "VM", + }}} + defs, err := loadDefinitions(context.Background(), f, []string{name}, false) + require.NoError(t, err) + require.Len(t, defs, 1) + require.Equal(t, name, defs[0].Title) +} + func TestFakeSource_DatasourceScriptedByKey(t *testing.T) { f := newFakeSource() - key := ruleKey("vm", "G", "A", "") + key := ruleKey("vm", "G", "A", "f", "") f.scriptKey(key, Observation{Rules: []StateRule{{Key: key, DatasourceUID: "vm", Title: "A"}}}, nil) obs, err := f.RuleState(context.Background(), RuleRef{Key: key, Kind: KindDatasourceManaged, DatasourceUID: "vm"}) require.NoError(t, err) @@ -124,7 +152,7 @@ func TestRuleState_DatasourceAssertsAllFilters(t *testing.T) { src := NewHTTPSource(srv.URL, "", newFakeClock(time.Now())) ref := RuleRef{ - Key: ruleKey("vm", "ExampleMetrics", "ExampleTargetDown", ""), + Key: ruleKey("vm", "ExampleMetrics", "ExampleTargetDown", "/etc/vm/rules/example.yml", ""), Kind: KindDatasourceManaged, DatasourceUID: "vm", Group: "ExampleMetrics", Name: "ExampleTargetDown", File: "/etc/vm/rules/example.yml", } From 300505f861d4282ae2092aca4f0029e88306ed04 Mon Sep 17 00:00:00 2001 From: Bartek Tofel <tofel.b@gmail.com> Date: Tue, 6 Oct 2026 12:06:56 +0200 Subject: [PATCH 3/8] chore: address cr comments --- grafana-alertcheck/.changeset/v0.1.10.md | 1 + grafana-alertcheck/docs/advanced.md | 4 +- grafana-alertcheck/internal/gate/identity.go | 41 ++++++++++- grafana-alertcheck/internal/gate/labels.go | 9 ++- .../internal/gate/labels_test.go | 38 ++++++++++ grafana-alertcheck/internal/gate/load.go | 73 +++++++++++++------ .../internal/gate/parse_datasource.go | 5 ++ .../internal/gate/parse_datasource_test.go | 9 +++ grafana-alertcheck/internal/gate/resolve.go | 10 ++- .../internal/gate/resolve_test.go | 12 +++ .../internal/gate/source_ds_test.go | 42 +++++++++-- 11 files changed, 209 insertions(+), 35 deletions(-) diff --git a/grafana-alertcheck/.changeset/v0.1.10.md b/grafana-alertcheck/.changeset/v0.1.10.md index 159cd2a1c..d248f10ee 100644 --- a/grafana-alertcheck/.changeset/v0.1.10.md +++ b/grafana-alertcheck/.changeset/v0.1.10.md @@ -4,5 +4,6 @@ - Datasource recovery semantics: because the Prometheus API returns only active instances, an instance leaving the active set is treated as a real recovery (`cleared`). Documented weaker guarantee: a vanished series is indistinguishable from a resolution. - Pause is not observable for datasource-managed rules (no `isPaused` signal), so check 7 is skipped with an explicit note. `health: err` normalizes to `error` and still triggers check 4. - The token now needs `datasources:read` plus datasource query permission even for Grafana-only runs; discovery failures name the missing permission. +- A backend can serve two distinct datasource-managed rules under one identity (same datasource/group/name/file, differing by labels/query). Loading and `list` accept them; a selection that matches both fails closed with an actionable message, while selecting one by a distinguishing label works. `uid:` selectors no longer trigger a bulk datasource read, and a datasource rule's `lastError` is recorded. - A datasource-managed rule's name can itself contain `/` (e.g. `devex-cicd/prod/griddle-github: ContainersNotReady`). The full name is now matched exactly and used as the `rule_name[]` fetch filter, instead of splitting it into `Group/Title` segments and filtering by the last segment. - The `RESULTS` table gains a `SOURCE` column (`grafana`/`datasource`). Datasource caveats are printed once before the table, and `DETAILS` carries only rule-specific notes without repeating the alert name. `--output json` adds `source_kind` and a run-level `caveats`. diff --git a/grafana-alertcheck/docs/advanced.md b/grafana-alertcheck/docs/advanced.md index 1b8fb87ca..ec829a209 100644 --- a/grafana-alertcheck/docs/advanced.md +++ b/grafana-alertcheck/docs/advanced.md @@ -37,10 +37,12 @@ Single-step `check` runs the same pass itself. It cannot watch before it started ## Datasource-managed rules: discovery and cost -Datasource-managed rules are auto-discovered — there is no selection flag. The candidate filter is strict: `type == "prometheus"` **and** `jsonData.manageAlerts == true`. The strict `true` matters: the `AlertStateHistoryBackend` datasource shares VictoriaMetrics' backend and also reports `manageAlerts` truthy, so a `!= false` filter would make every rule name ambiguous. Loki is deferred: its ruler API is broken/disabled in our Grafana, so only Prometheus-flavored rules are in scope. +Datasource-managed rules are auto-discovered — there is no selection flag. The candidate filter is strict: `type == "prometheus"` **and** `jsonData.manageAlerts == true`. The strict `true` matters: the `AlertStateHistoryBackend` datasource points at the same VictoriaMetrics backend but does not set `manageAlerts: true`, so a `!= false` filter would include it and make every rule name ambiguous. Loki is deferred: its ruler API is broken/disabled in our Grafana, so only Prometheus-flavored rules are in scope. Each candidate is probed with a `rule_name[]=__probe__` request; a `manageAlerts=true` source whose probe fails is a hard error naming the source, never a silently dropped source. +vmalert allows two distinct alerting rules to share a name within one group. Such rules get the same identity, so a selection that matches both cannot observe them separately and fails closed with a message asking you to narrow the selection (a distinguishing label selects one); loading and `list` still show both. + Cost differs by mode. `list` and label selection take the **bulk** response — one request per datasource, several MB and several seconds for a large ruler. Name selection takes a **filtered** request, ~1 KB and ~1 s. The filter uses vmalert's `[]`-suffixed parameters (`rule_name[]`, `rule_group[]`, `file[]`): vmalert reads only those and ignores plain `rule_name=`, an upstream quirk pinned by tests. `limit_alerts` is a Grafana parameter that vmalert ignores and is therefore omitted. ## Why the gate never queries state history diff --git a/grafana-alertcheck/internal/gate/identity.go b/grafana-alertcheck/internal/gate/identity.go index 88f5f23af..20b1e2adf 100644 --- a/grafana-alertcheck/internal/gate/identity.go +++ b/grafana-alertcheck/internal/gate/identity.go @@ -1,6 +1,9 @@ package gate -import "encoding/json" +import ( + "encoding/json" + "fmt" +) // dsKeyPrefix marks a datasource-managed key, so a caller without the // Definition can still tell the two source kinds apart. @@ -58,3 +61,39 @@ func verdictKey(v RuleVerdict) string { } return v.RuleUID } + +// rejectDuplicateKeys fails closed on two definitions sharing a key. A Grafana +// uid is unique by construction; a backend may serve two distinct +// datasource-managed rules under one (datasource, group, name, file), and those +// cannot be told apart, so a selection that matches both must be narrowed +// rather than silently observing one of them. +func rejectDuplicateKeys(defs []Definition) error { + seen := make(map[string]Definition, len(defs)) + for _, d := range defs { + key := defKey(d) + prev, ok := seen[key] + if !ok { + seen[key] = d + continue + } + if d.Kind == KindDatasourceManaged { + return fmt.Errorf( + "two datasource-managed rules share the identity %s: %s and %s; narrow the selection (e.g. a distinguishing label) so only one matches", + key, describeRule(prev), describeRule(d)) + } + return fmt.Errorf("two rules share the key %s: %s and %s", key, describeRule(prev), describeRule(d)) + } + return nil +} + +// describeRule names a definition for a duplicate-key error. +func describeRule(d Definition) string { + if d.Kind == KindDatasourceManaged { + src := d.DatasourceName + if src == "" { + src = d.DatasourceUID + } + return fmt.Sprintf("datasource %q, group %q, file %q, name %q", src, d.Group, d.File, d.Title) + } + return fmt.Sprintf("uid %s, title %q", d.UID, d.Title) +} diff --git a/grafana-alertcheck/internal/gate/labels.go b/grafana-alertcheck/internal/gate/labels.go index 2215020c6..ea38a1036 100644 --- a/grafana-alertcheck/internal/gate/labels.go +++ b/grafana-alertcheck/internal/gate/labels.go @@ -65,6 +65,11 @@ func resolveAlertSet(defs []Definition, names []string, include, exclude []Label if err != nil { return nil, nil, err } + // Only the rules actually being watched must be distinct: a collision in + // the loaded inventory is harmless unless the selection matches both. + if err := rejectDuplicateKeys(selected); err != nil { + return nil, nil, err + } return selected, notes, nil } @@ -111,7 +116,9 @@ func labelSelectionHeader(include, exclude []LabelMatcher, excludeCount int) str func printLabelSelection(w io.Writer, resolved []Definition, include, exclude []LabelMatcher, excludeCount int) { fmt.Fprintf(w, "%s:\n", labelSelectionHeader(include, exclude, excludeCount)) for _, d := range resolved { - fmt.Fprintf(w, " - %s (%s)\n", d.Title, d.UID) + // defKey, not UID: a datasource-managed rule has no uid, and its key is + // the copyable identity (`key:<key>`). + fmt.Fprintf(w, " - %s (%s)\n", d.Title, defKey(d)) } } diff --git a/grafana-alertcheck/internal/gate/labels_test.go b/grafana-alertcheck/internal/gate/labels_test.go index f7dcb422a..9bae874a7 100644 --- a/grafana-alertcheck/internal/gate/labels_test.go +++ b/grafana-alertcheck/internal/gate/labels_test.go @@ -103,6 +103,30 @@ func TestSelectByLabels_EmptyIncludeIsNotAnError(t *testing.T) { require.Equal(t, []string{"rule0000002"}, selectedUIDs(selected)) } +// Two distinct datasource rules can share one identity (same datasource, +// group, name and file, differing only by labels/query). A selection that +// matches both cannot observe them distinctly and must fail closed; one that +// matches a single rule is fine. +func TestResolveAlertSet_DuplicateIdentityOnlyFailsWhenBothSelected(t *testing.T) { + a := Definition{ + Key: "ds:k", Title: "Same", Group: "G", Kind: KindDatasourceManaged, + DatasourceUID: "vm", DatasourceName: "VM", + Labels: map[string]string{"product": "ccip", "severity": "critical"}, + } + b := a + b.Labels = map[string]string{"product": "ccip", "severity": "warning"} + defs := []Definition{a, b} + + _, _, err := resolveAlertSet(defs, nil, []LabelMatcher{{Key: "product", Value: "ccip"}}, nil, nil, "") + require.Error(t, err) + require.Contains(t, err.Error(), "share the identity") + require.Contains(t, err.Error(), "narrow the selection") + + selected, _, err := resolveAlertSet(defs, nil, []LabelMatcher{{Key: "severity", Value: "critical"}}, nil, nil, "") + require.NoError(t, err) + require.Len(t, selected, 1) +} + // --exclude-alerts subtracts from an enumerated set: the names resolve like // --alerts, so uid: forms work, and the result keeps the input order. func TestResolveAlertSet_SubtractsExcludedNames(t *testing.T) { @@ -145,6 +169,20 @@ func TestResolveAlertSet_ExcludingEverythingFails(t *testing.T) { require.Contains(t, err.Error(), "drops every selected rule") } +// A datasource rule has no uid, so the listing must show its key instead of an +// empty pair of parentheses. +func TestPrintLabelSelection_DatasourceShowsKey(t *testing.T) { + defs := []Definition{{ + Key: ruleKey("vm", "G", "A", "f", ""), Title: "A", Group: "G", + Kind: KindDatasourceManaged, DatasourceUID: "vm", DatasourceName: "VM", + }} + var b strings.Builder + printLabelSelection(&b, defs, []LabelMatcher{{Key: "k", Value: "v"}}, nil, 0) + out := b.String() + require.Contains(t, out, " - A (ds:") + require.NotContains(t, out, "()") +} + func TestPrintLabelSelection(t *testing.T) { defs := rulerDefs(t) selected, err := SelectByLabels(defs, []LabelMatcher{{Key: "severity", Value: "warning"}}, nil) diff --git a/grafana-alertcheck/internal/gate/load.go b/grafana-alertcheck/internal/gate/load.go index f6b17d8a3..28996c479 100644 --- a/grafana-alertcheck/internal/gate/load.go +++ b/grafana-alertcheck/internal/gate/load.go @@ -34,35 +34,53 @@ func loadDefinitions(ctx context.Context, src Source, names []string, wantAll bo defs = append(defs, d) } - filter, fetchAll := dsFilterNames(names) - if wantAll { - fetchAll = true - } - for _, rs := range sources { - var dsDefs []Definition - if fetchAll { - dsDefs, err = src.DatasourceDefinitions(ctx, rs, nil) - } else { - dsDefs, err = src.DatasourceDefinitions(ctx, rs, filter) - } - if err != nil { - return nil, fmt.Errorf("datasource %q: %w", rs.Name, err) + fetch := planDatasourceFetch(names, wantAll) + if !fetch.Skip { + for _, rs := range sources { + var dsDefs []Definition + if fetch.All { + dsDefs, err = src.DatasourceDefinitions(ctx, rs, nil) + } else { + dsDefs, err = src.DatasourceDefinitions(ctx, rs, fetch.Names) + } + if err != nil { + return nil, fmt.Errorf("datasource %q: %w", rs.Name, err) + } + defs = append(defs, dsDefs...) } - defs = append(defs, dsDefs...) } + // Duplicate keys are deliberately NOT rejected here: loading is an + // inventory (`list`), and a collision only matters when both rules are + // actually selected. resolveAlertSet enforces that on the selected set. return defs, nil } -// dsFilterNames extracts the server-side rule_name[] filters. A key: or uid: -// form forces a bulk fetch; otherwise both the whole input and its last -// /-separated segment are sent (a ds rule's name can itself contain "/", while -// Group/Title names it by the trailing segment). -func dsFilterNames(names []string) (filters []string, fetchAll bool) { +// dsFetchPlan is how loadDefinitions should query the datasource rule sources. +type dsFetchPlan struct { + // Skip is set when no selector can name a datasource rule (every selector + // is uid:, which only a Grafana rule carries), so the sources need not be + // read at all. + Skip bool + // All fetches every rule of each source: a key: selector names a ds rule and + // the API has no key filter, and wantAll asks for everything. + All bool + // Names are the rule_name[] filters for a name selection. + Names []string +} + +// planDatasourceFetch decides what to ask the datasource sources for. A uid: +// selector can only name a Grafana rule, so it never triggers a ds read; a key: +// selector can name a ds rule and forces a bulk read. +func planDatasourceFetch(names []string, wantAll bool) dsFetchPlan { + if wantAll { + return dsFetchPlan{All: true} + } + var plan dsFetchPlan seen := make(map[string]bool) add := func(s string) { if s != "" && !seen[s] { seen[s] = true - filters = append(filters, s) + plan.Names = append(plan.Names, s) } } for _, raw := range names { @@ -70,13 +88,22 @@ func dsFilterNames(names []string) (filters []string, fetchAll bool) { if n == "" { continue } - if strings.HasPrefix(n, "key:") || strings.HasPrefix(n, "uid:") { - return nil, true + switch { + case strings.HasPrefix(n, "key:"): + return dsFetchPlan{All: true} + case strings.HasPrefix(n, "uid:"): + continue } + // Both the whole input and its last /-separated segment: a ds rule's + // name can itself contain "/", while Group/Title names it by the + // trailing segment. add(n) if i := strings.LastIndex(n, "/"); i != -1 { add(n[i+1:]) } } - return filters, false + if len(plan.Names) == 0 { + return dsFetchPlan{Skip: true} + } + return plan } diff --git a/grafana-alertcheck/internal/gate/parse_datasource.go b/grafana-alertcheck/internal/gate/parse_datasource.go index 99251bf85..daedfcacf 100644 --- a/grafana-alertcheck/internal/gate/parse_datasource.go +++ b/grafana-alertcheck/internal/gate/parse_datasource.go @@ -123,6 +123,11 @@ func parseDatasourceRule(raw json.RawMessage, dsUID, group, file string, interva } r.LastEvaluation = lastEval } + // The backend diagnostic for health=err; reporting-only, like the Grafana + // parser's lastError. + if err := opt(m, "lastError", &r.LastError); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } if err := opt(m, "state", &r.State); err != nil { return StateRule{}, fmt.Errorf("rule %q: %w", name, err) diff --git a/grafana-alertcheck/internal/gate/parse_datasource_test.go b/grafana-alertcheck/internal/gate/parse_datasource_test.go index 70935a488..9205a9be3 100644 --- a/grafana-alertcheck/internal/gate/parse_datasource_test.go +++ b/grafana-alertcheck/internal/gate/parse_datasource_test.go @@ -49,6 +49,15 @@ func TestParseDatasourceRules_HealthErrNormalizes(t *testing.T) { require.Equal(t, "error", rules[0].Health) } +func TestParseDatasourceRules_LastError(t *testing.T) { + body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ + {"name":"A","type":"alerting","health":"err","lastError":"query failed: bad","state":"firing"}]}]}}`) + rules, err := ParseDatasourceRules(body, "d") + require.NoError(t, err) + require.Equal(t, "error", rules[0].Health) + require.Equal(t, "query failed: bad", rules[0].LastError) +} + func TestParseDatasourceRules_ZeroLastEvaluationAllowed(t *testing.T) { body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ {"name":"A","type":"alerting","health":"ok","state":"pending"}]}]}}`) diff --git a/grafana-alertcheck/internal/gate/resolve.go b/grafana-alertcheck/internal/gate/resolve.go index 8907040d6..62534c5d9 100644 --- a/grafana-alertcheck/internal/gate/resolve.go +++ b/grafana-alertcheck/internal/gate/resolve.go @@ -58,11 +58,19 @@ func Resolve(defs []Definition, names []string, folder string) (resolved []Defin func resolveOne(defs []Definition, name, folder string) (Definition, error) { if key, ok := strings.CutPrefix(name, "key:"); ok { if key != "" { + var matches []Definition for _, d := range defs { if defKey(d) == key { - return refuseUnsupportedKind(name, d) + matches = append(matches, d) } } + // A key shared by two distinct rules cannot select one of them. + if err := rejectDuplicateKeys(matches); err != nil { + return Definition{}, err + } + if len(matches) == 1 { + return refuseUnsupportedKind(name, matches[0]) + } } return Definition{}, fmt.Errorf("no rule matched %q: no rule has this key (run 'grafana-alertcheck list' to see keys)", name) } diff --git a/grafana-alertcheck/internal/gate/resolve_test.go b/grafana-alertcheck/internal/gate/resolve_test.go index 44c8dc94c..2ac5baa59 100644 --- a/grafana-alertcheck/internal/gate/resolve_test.go +++ b/grafana-alertcheck/internal/gate/resolve_test.go @@ -243,6 +243,18 @@ func TestResolve_DatasourceNameWithSlashes(t *testing.T) { require.Equal(t, name, resolved[0].Title) } +// A key shared by two distinct rules cannot select one of them. +func TestResolve_KeySharedByDistinctRulesIsAmbiguous(t *testing.T) { + a := Definition{ + Key: "ds:k", Title: "Same", Group: "G", Kind: KindDatasourceManaged, + DatasourceUID: "vm", DatasourceName: "VM", + } + b := a + _, _, err := Resolve([]Definition{a, b}, []string{"key:ds:k"}, "") + require.Error(t, err) + require.Contains(t, err.Error(), "share the identity") +} + func TestResolve_DatasourceAmbiguityAcrossSources(t *testing.T) { defs := []Definition{ dsResolveDef("vm-a", "A", "G", "Same"), diff --git a/grafana-alertcheck/internal/gate/source_ds_test.go b/grafana-alertcheck/internal/gate/source_ds_test.go index 65dba239c..1e1609ce2 100644 --- a/grafana-alertcheck/internal/gate/source_ds_test.go +++ b/grafana-alertcheck/internal/gate/source_ds_test.go @@ -21,7 +21,7 @@ func TestDatasourceQuery_BracketedKeys(t *testing.T) { func TestDiscoverRuleSources_StrictFilterAndProbe(t *testing.T) { datasources := `[ {"uid":"vm","name":"VictoriaMetrics - Prod","type":"prometheus","jsonData":{"manageAlerts":true}}, - {"uid":"ash","name":"AlertStateHistoryBackend","type":"prometheus","jsonData":{"manageAlerts":false}}, + {"uid":"ash","name":"AlertStateHistoryBackend","type":"prometheus","jsonData":{}}, {"uid":"loki","name":"Loki","type":"loki","jsonData":{"manageAlerts":true}} ]` var probed []string @@ -85,16 +85,26 @@ func TestDatasourceDefinitions_FilteredQuery(t *testing.T) { require.Equal(t, "VM", defs[0].DatasourceName) } -func TestDSFilterNames(t *testing.T) { - filters, fetchAll := dsFilterNames([]string{"devex-cicd/prod/griddle-github: ContainersNotReady"}) - require.False(t, fetchAll) +func TestPlanDatasourceFetch(t *testing.T) { + p := planDatasourceFetch([]string{"devex-cicd/prod/griddle-github: ContainersNotReady"}, false) + require.False(t, p.Skip) + require.False(t, p.All) require.ElementsMatch(t, []string{ "devex-cicd/prod/griddle-github: ContainersNotReady", "griddle-github: ContainersNotReady", - }, filters) - - _, fetchAll = dsFilterNames([]string{"key:ds:[\"a\"]"}) - require.True(t, fetchAll) + }, p.Names) + + // A uid can only name a Grafana rule: no datasource read at all. + require.True(t, planDatasourceFetch([]string{"uid:abc"}, false).Skip) + // A key can name a datasource rule, and there is no key filter. + require.True(t, planDatasourceFetch([]string{"key:ds:[\"a\"]"}, false).All) + // Mixed: still filtered, by the name selector only. + m := planDatasourceFetch([]string{"uid:abc", "Foo"}, false) + require.False(t, m.Skip) + require.False(t, m.All) + require.Equal(t, []string{"Foo"}, m.Names) + // wantAll (list, labels) always bulk-reads. + require.True(t, planDatasourceFetch(nil, true).All) } // A datasource rule whose name contains "/" is fetched by its full name, so @@ -113,6 +123,22 @@ func TestLoadDefinitions_SlashyDatasourceName(t *testing.T) { require.Equal(t, name, defs[0].Title) } +// Loading is an inventory: a duplicate identity must not break `list`, because +// only a selection that matches both rules is a problem. +func TestLoadDefinitions_AllowsDuplicateKeys(t *testing.T) { + name := "A" + f := newFakeSource() + f.ruleSources = []RuleSource{{UID: "vm", Name: "VM"}} + dup := Definition{ + Key: ruleKey("vm", "G", name, "f", ""), Title: name, Group: "G", File: "f", + Kind: KindDatasourceManaged, DatasourceUID: "vm", DatasourceName: "VM", + } + f.dsDefs = map[string][]Definition{"vm": {dup, dup}} + defs, err := loadDefinitions(context.Background(), f, []string{name}, false) + require.NoError(t, err) + require.Len(t, defs, 2) +} + func TestFakeSource_DatasourceScriptedByKey(t *testing.T) { f := newFakeSource() key := ruleKey("vm", "G", "A", "f", "") From 7c3322949136bf1b76f2eca2dc69a204fec86d21 Mon Sep 17 00:00:00 2001 From: Bartek Tofel <tofel.b@gmail.com> Date: Tue, 6 Oct 2026 12:24:14 +0200 Subject: [PATCH 4/8] chore: one more time with CR --- grafana-alertcheck/.changeset/v0.1.10.md | 2 +- grafana-alertcheck/docs/advanced.md | 2 +- grafana-alertcheck/internal/gate/identity.go | 61 ++++++++++++++----- grafana-alertcheck/internal/gate/labels.go | 7 ++- .../internal/gate/labels_test.go | 28 ++++++--- 5 files changed, 72 insertions(+), 28 deletions(-) diff --git a/grafana-alertcheck/.changeset/v0.1.10.md b/grafana-alertcheck/.changeset/v0.1.10.md index d248f10ee..40103ccaf 100644 --- a/grafana-alertcheck/.changeset/v0.1.10.md +++ b/grafana-alertcheck/.changeset/v0.1.10.md @@ -4,6 +4,6 @@ - Datasource recovery semantics: because the Prometheus API returns only active instances, an instance leaving the active set is treated as a real recovery (`cleared`). Documented weaker guarantee: a vanished series is indistinguishable from a resolution. - Pause is not observable for datasource-managed rules (no `isPaused` signal), so check 7 is skipped with an explicit note. `health: err` normalizes to `error` and still triggers check 4. - The token now needs `datasources:read` plus datasource query permission even for Grafana-only runs; discovery failures name the missing permission. -- A backend can serve two distinct datasource-managed rules under one identity (same datasource/group/name/file, differing by labels/query). Loading and `list` accept them; a selection that matches both fails closed with an actionable message, while selecting one by a distinguishing label works. `uid:` selectors no longer trigger a bulk datasource read, and a datasource rule's `lastError` is recorded. +- A backend can serve two distinct datasource-managed rules under one identity (same datasource/group/name/file, differing by labels/query). Loading and `list` accept them, but a selection that includes such a rule fails closed: the state query cannot tell the siblings apart, so narrowing to one does not make it observable. `uid:` selectors no longer trigger a bulk datasource read, and a datasource rule's `lastError` is recorded. - A datasource-managed rule's name can itself contain `/` (e.g. `devex-cicd/prod/griddle-github: ContainersNotReady`). The full name is now matched exactly and used as the `rule_name[]` fetch filter, instead of splitting it into `Group/Title` segments and filtering by the last segment. - The `RESULTS` table gains a `SOURCE` column (`grafana`/`datasource`). Datasource caveats are printed once before the table, and `DETAILS` carries only rule-specific notes without repeating the alert name. `--output json` adds `source_kind` and a run-level `caveats`. diff --git a/grafana-alertcheck/docs/advanced.md b/grafana-alertcheck/docs/advanced.md index ec829a209..fd1f3bc2e 100644 --- a/grafana-alertcheck/docs/advanced.md +++ b/grafana-alertcheck/docs/advanced.md @@ -41,7 +41,7 @@ Datasource-managed rules are auto-discovered — there is no selection flag. The Each candidate is probed with a `rule_name[]=__probe__` request; a `manageAlerts=true` source whose probe fails is a hard error naming the source, never a silently dropped source. -vmalert allows two distinct alerting rules to share a name within one group. Such rules get the same identity, so a selection that matches both cannot observe them separately and fails closed with a message asking you to narrow the selection (a distinguishing label selects one); loading and `list` still show both. +vmalert allows two distinct alerting rules to share a name within one group. They get the same identity, and the state query is by datasource/group/name/file, so the tool cannot tell them apart: loading and `list` still show both, but a selection that includes either one fails closed — narrowing by a distinguishing label does not help, because the poll would still reduce whichever sibling the backend lists first. Cost differs by mode. `list` and label selection take the **bulk** response — one request per datasource, several MB and several seconds for a large ruler. Name selection takes a **filtered** request, ~1 KB and ~1 s. The filter uses vmalert's `[]`-suffixed parameters (`rule_name[]`, `rule_group[]`, `file[]`): vmalert reads only those and ignores plain `rule_name=`, an upstream quirk pinned by tests. `limit_alerts` is a Grafana parameter that vmalert ignores and is therefore omitted. diff --git a/grafana-alertcheck/internal/gate/identity.go b/grafana-alertcheck/internal/gate/identity.go index 20b1e2adf..f448d1aa7 100644 --- a/grafana-alertcheck/internal/gate/identity.go +++ b/grafana-alertcheck/internal/gate/identity.go @@ -3,6 +3,7 @@ package gate import ( "encoding/json" "fmt" + "strings" ) // dsKeyPrefix marks a datasource-managed key, so a caller without the @@ -62,30 +63,60 @@ func verdictKey(v RuleVerdict) string { return v.RuleUID } -// rejectDuplicateKeys fails closed on two definitions sharing a key. A Grafana -// uid is unique by construction; a backend may serve two distinct -// datasource-managed rules under one (datasource, group, name, file), and those -// cannot be told apart, so a selection that matches both must be narrowed -// rather than silently observing one of them. +// rejectDuplicateKeys fails closed on two definitions in one list sharing a +// key. It is the key: selector's guard: a key shared by two rules cannot pick +// one of them. func rejectDuplicateKeys(defs []Definition) error { - seen := make(map[string]Definition, len(defs)) + byKey := make(map[string][]Definition, len(defs)) for _, d := range defs { key := defKey(d) - prev, ok := seen[key] - if !ok { - seen[key] = d - continue + byKey[key] = append(byKey[key], d) + } + for key, group := range byKey { + if len(group) > 1 { + return duplicateKeyError(key, group) } - if d.Kind == KindDatasourceManaged { - return fmt.Errorf( - "two datasource-managed rules share the identity %s: %s and %s; narrow the selection (e.g. a distinguishing label) so only one matches", - key, describeRule(prev), describeRule(d)) + } + return nil +} + +// rejectSharedSelectedKeys fails closed when a SELECTED rule's identity is +// shared in the loaded inventory. The state query is by +// datasource/group/name/file, so both siblings come back and stateRuleByKey +// would reduce whichever the backend lists first — not necessarily the one the +// selection matched. Narrowing the selection therefore does not make the rule +// observable; it must be excluded. +func rejectSharedSelectedKeys(all, selected []Definition) error { + byKey := make(map[string][]Definition, len(all)) + for _, d := range all { + key := defKey(d) + byKey[key] = append(byKey[key], d) + } + for _, d := range selected { + key := defKey(d) + if group := byKey[key]; len(group) > 1 { + return duplicateKeyError(key, group) } - return fmt.Errorf("two rules share the key %s: %s and %s", key, describeRule(prev), describeRule(d)) } return nil } +// duplicateKeyError explains a key collision. A datasource collision is the +// interesting one: the rules are genuinely distinct (a backend may serve two +// same-name rules in one group/file) but cannot be told apart by the state API. +func duplicateKeyError(key string, defs []Definition) error { + descs := make([]string, len(defs)) + for i, d := range defs { + descs[i] = describeRule(d) + } + if defs[0].Kind == KindDatasourceManaged { + return fmt.Errorf( + "%d datasource-managed rules share the identity %s (%s) and cannot be told apart: the state query is by datasource/group/name/file, so this rule cannot be observed; exclude it from the selection", + len(defs), key, strings.Join(descs, "; ")) + } + return fmt.Errorf("%d rules share the key %s (%s)", len(defs), key, strings.Join(descs, "; ")) +} + // describeRule names a definition for a duplicate-key error. func describeRule(d Definition) string { if d.Kind == KindDatasourceManaged { diff --git a/grafana-alertcheck/internal/gate/labels.go b/grafana-alertcheck/internal/gate/labels.go index ea38a1036..768007fed 100644 --- a/grafana-alertcheck/internal/gate/labels.go +++ b/grafana-alertcheck/internal/gate/labels.go @@ -65,9 +65,10 @@ func resolveAlertSet(defs []Definition, names []string, include, exclude []Label if err != nil { return nil, nil, err } - // Only the rules actually being watched must be distinct: a collision in - // the loaded inventory is harmless unless the selection matches both. - if err := rejectDuplicateKeys(selected); err != nil { + // A collision in the inventory is harmless until a selected rule's identity + // is shared: the state query cannot tell the siblings apart, so such a rule + // is unobservable even when the selection narrows to it. + if err := rejectSharedSelectedKeys(defs, selected); err != nil { return nil, nil, err } return selected, notes, nil diff --git a/grafana-alertcheck/internal/gate/labels_test.go b/grafana-alertcheck/internal/gate/labels_test.go index 9bae874a7..457abfc08 100644 --- a/grafana-alertcheck/internal/gate/labels_test.go +++ b/grafana-alertcheck/internal/gate/labels_test.go @@ -104,10 +104,10 @@ func TestSelectByLabels_EmptyIncludeIsNotAnError(t *testing.T) { } // Two distinct datasource rules can share one identity (same datasource, -// group, name and file, differing only by labels/query). A selection that -// matches both cannot observe them distinctly and must fail closed; one that -// matches a single rule is fine. -func TestResolveAlertSet_DuplicateIdentityOnlyFailsWhenBothSelected(t *testing.T) { +// group, name and file, differing only by labels/query). The state query cannot +// tell them apart, so a selected rule with a shared identity is unobservable +// even when the selection narrows to it; an unrelated selection is fine. +func TestResolveAlertSet_SharedIdentityFailsEvenWhenOneSelected(t *testing.T) { a := Definition{ Key: "ds:k", Title: "Same", Group: "G", Kind: KindDatasourceManaged, DatasourceUID: "vm", DatasourceName: "VM", @@ -115,16 +115,28 @@ func TestResolveAlertSet_DuplicateIdentityOnlyFailsWhenBothSelected(t *testing.T } b := a b.Labels = map[string]string{"product": "ccip", "severity": "warning"} - defs := []Definition{a, b} + other := Definition{ + Key: "ds:other", Title: "Other", Group: "G", Kind: KindDatasourceManaged, + DatasourceUID: "vm", DatasourceName: "VM", + Labels: map[string]string{"product": "branch-out"}, + } + defs := []Definition{a, b, other} + // Both siblings selected. _, _, err := resolveAlertSet(defs, nil, []LabelMatcher{{Key: "product", Value: "ccip"}}, nil, nil, "") require.Error(t, err) - require.Contains(t, err.Error(), "share the identity") - require.Contains(t, err.Error(), "narrow the selection") + require.Contains(t, err.Error(), "cannot be told apart") + + // ONE sibling selected: still unobservable, so still an error. + _, _, err = resolveAlertSet(defs, nil, []LabelMatcher{{Key: "severity", Value: "critical"}}, nil, nil, "") + require.Error(t, err) + require.Contains(t, err.Error(), "cannot be told apart") - selected, _, err := resolveAlertSet(defs, nil, []LabelMatcher{{Key: "severity", Value: "critical"}}, nil, nil, "") + // An unrelated rule is fine even though the inventory has a collision. + selected, _, err := resolveAlertSet(defs, nil, []LabelMatcher{{Key: "product", Value: "branch-out"}}, nil, nil, "") require.NoError(t, err) require.Len(t, selected, 1) + require.Equal(t, "Other", selected[0].Title) } // --exclude-alerts subtracts from an enumerated set: the names resolve like From 8c8db367b42faeec54477eb90e63068357ebcb4e Mon Sep 17 00:00:00 2001 From: Bartek Tofel <tofel.b@gmail.com> Date: Tue, 6 Oct 2026 12:35:59 +0200 Subject: [PATCH 5/8] chore: third time's the charm --- grafana-alertcheck/internal/gate/resolve.go | 48 ++++++++++--------- .../internal/gate/resolve_test.go | 15 ++++++ 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/grafana-alertcheck/internal/gate/resolve.go b/grafana-alertcheck/internal/gate/resolve.go index 62534c5d9..9cf44c4b0 100644 --- a/grafana-alertcheck/internal/gate/resolve.go +++ b/grafana-alertcheck/internal/gate/resolve.go @@ -95,35 +95,39 @@ func resolveOne(defs []Definition, name, folder string) (Definition, error) { return Definition{}, fmt.Errorf("no rule matched %q: no rule has this uid (run 'grafana-alertcheck list' to see uids)", name) } - // A datasource rule's name can contain "/", so the full input may be a title, - // not a segmented form: try an exact title match before splitting. - if def, found, err := pickCandidate(defs, name, func(d Definition) bool { - return titleMatches(d, name, folder) - }); found { - return def, err - } - - parts, err := parseNameForm(name) - if err != nil { - return Definition{}, err + // Two interpretations are possible: the whole input as one rule's exact + // title (a datasource rule's name can itself contain "/"), and the + // /-separated forms. Collect candidates from BOTH — a selector that is + // ambiguous between them must be reported, never silently resolved to one. + parts, formErr := parseNameForm(name) + var candidates []Definition + seen := make(map[string]bool, len(defs)) + for _, d := range defs { + exact := titleMatches(d, name, folder) + segmented := formErr == nil && matchesName(d, parts, folder) + if !exact && !segmented { + continue + } + if key := defKey(d); !seen[key] { + seen[key] = true + candidates = append(candidates, d) + } } - if def, found, err := pickCandidate(defs, name, func(d Definition) bool { - return matchesName(d, parts, folder) - }); found { + if def, found, err := pickCandidate(candidates, name); found { return def, err } + if formErr != nil { + return Definition{}, formErr + } return Definition{}, noMatchError(supportedDefs(defs), name, parts[len(parts)-1]) } -// pickCandidate applies the shared one-match/ambiguous/unsupported/no-match -// policy to a candidate predicate. found is false when nothing matched, so the -// caller can try the next interpretation. -func pickCandidate(defs []Definition, name string, match func(Definition) bool) (Definition, bool, error) { +// pickCandidate applies the shared one-match/ambiguous/unsupported policy to the +// collected candidates. found is false when nothing matched, so the caller can +// fall through to the no-match surface. +func pickCandidate(candidates []Definition, name string) (Definition, bool, error) { var supported, unsupported []Definition - for _, d := range defs { - if !match(d) { - continue - } + for _, d := range candidates { if isSupported(d) { supported = append(supported, d) } else { diff --git a/grafana-alertcheck/internal/gate/resolve_test.go b/grafana-alertcheck/internal/gate/resolve_test.go index 2ac5baa59..ef46ad573 100644 --- a/grafana-alertcheck/internal/gate/resolve_test.go +++ b/grafana-alertcheck/internal/gate/resolve_test.go @@ -243,6 +243,21 @@ func TestResolve_DatasourceNameWithSlashes(t *testing.T) { require.Equal(t, name, resolved[0].Title) } +// The same string can be one datasource rule's exact title AND a Grafana +// Folder/Title selector. That must be reported as ambiguous, not silently +// resolved to whichever interpretation is tried first. +func TestResolve_ExactTitleVsSegmentedIsAmbiguous(t *testing.T) { + ds := Definition{ + Key: ruleKey("vm", "G", "Platform/HighErrorRate", "f", ""), + Title: "Platform/HighErrorRate", Group: "G", + Kind: KindDatasourceManaged, DatasourceUID: "vm", DatasourceName: "VM", + } + grafana := Definition{Key: "u1", UID: "u1", Title: "HighErrorRate", Folder: "Platform", Kind: KindGrafanaManaged} + _, _, err := Resolve([]Definition{grafana, ds}, []string{"Platform/HighErrorRate"}, "") + require.Error(t, err) + require.Contains(t, err.Error(), "matches 2 rules") +} + // A key shared by two distinct rules cannot select one of them. func TestResolve_KeySharedByDistinctRulesIsAmbiguous(t *testing.T) { a := Definition{ From d2531189b85fd59c814065bf05cbd58d02a8c1b0 Mon Sep 17 00:00:00 2001 From: Bartek Tofel <tofel.b@gmail.com> Date: Tue, 6 Oct 2026 13:30:27 +0200 Subject: [PATCH 6/8] chore: integrate recovery status --- grafana-alertcheck/.changeset/v0.1.10.md | 1 + grafana-alertcheck/docs/architecture.md | 2 +- grafana-alertcheck/docs/how-alerts-are-evaluated.md | 2 ++ grafana-alertcheck/docs/reference/log-format.md | 1 + grafana-alertcheck/internal/gate/handoff.go | 4 ++-- grafana-alertcheck/internal/gate/handoff_test.go | 4 ++-- grafana-alertcheck/internal/gate/log.go | 4 ++-- grafana-alertcheck/internal/gate/parse_datasource.go | 11 ++++++++++- .../internal/gate/parse_datasource_test.go | 10 ++++++++++ grafana-alertcheck/internal/gate/schedule.go | 7 ++++--- 10 files changed, 35 insertions(+), 11 deletions(-) diff --git a/grafana-alertcheck/.changeset/v0.1.10.md b/grafana-alertcheck/.changeset/v0.1.10.md index 40103ccaf..96924ea3a 100644 --- a/grafana-alertcheck/.changeset/v0.1.10.md +++ b/grafana-alertcheck/.changeset/v0.1.10.md @@ -3,6 +3,7 @@ - Datasource-managed identity is a separate rule key; `uid` stays empty for these rules. The log gains additive `key`, `source_kind`, `datasource_uid`, `datasource_name`, `file` and `rule_key` fields, with `schema_version` still `1`. An old v1 log without `rule_key` remains readable; a new log read by an old binary fails closed on the unresolved key. - Datasource recovery semantics: because the Prometheus API returns only active instances, an instance leaving the active set is treated as a real recovery (`cleared`). Documented weaker guarantee: a vanished series is indistinguishable from a resolution. - Pause is not observable for datasource-managed rules (no `isPaused` signal), so check 7 is skipped with an explicit note. `health: err` normalizes to `error` and still triggers check 4. +- Datasource-managed rules record the backend's `keep_firing_for` (`keep_firing_for_ms` in the log). The datasource API has no `recovering` state — it keeps an alert firing through its keep-firing-for and then drops it — so the recovery observation applies to Grafana-managed rules only. - The token now needs `datasources:read` plus datasource query permission even for Grafana-only runs; discovery failures name the missing permission. - A backend can serve two distinct datasource-managed rules under one identity (same datasource/group/name/file, differing by labels/query). Loading and `list` accept them, but a selection that includes such a rule fails closed: the state query cannot tell the siblings apart, so narrowing to one does not make it observable. `uid:` selectors no longer trigger a bulk datasource read, and a datasource rule's `lastError` is recorded. - A datasource-managed rule's name can itself contain `/` (e.g. `devex-cicd/prod/griddle-github: ContainersNotReady`). The full name is now matched exactly and used as the `rule_name[]` fetch filter, instead of splitting it into `Group/Title` segments and filtering by the last segment. diff --git a/grafana-alertcheck/docs/architecture.md b/grafana-alertcheck/docs/architecture.md index 8bb06816e..2d1eb8cd2 100644 --- a/grafana-alertcheck/docs/architecture.md +++ b/grafana-alertcheck/docs/architecture.md @@ -82,4 +82,4 @@ On a clean stop (SIGTERM/SIGINT/`--until`) the child finishes the in-flight writ `watch` records raw evidence, so nothing trusts a state that could become unreachable. Two consequences a maintainer must preserve: - The **header is authoritative for recording facts** (the cadence actually used, the URL, the alert set); the ruler API is authoritative for **rule facts** (`for`, `intervalSeconds`, kind). `check` always re-resolves definitions fresh and never reconstructs them from the header — the header duplicates `for`/`interval` only so the uploaded artifact is self-describing. -- The **cadence authority** is the header's `poll_every_seconds`, not the definitions. Re-deriving it would compare gaps recorded at an override cadence against default-cadence thresholds — fail-open in the faster-override direction. +- The **cadence authority** is the header's `poll_every_seconds`, not the definitions. Re-deriving it would compare recorded gaps against thresholds derived from a different cadence — fail-open if the two ever diverge. diff --git a/grafana-alertcheck/docs/how-alerts-are-evaluated.md b/grafana-alertcheck/docs/how-alerts-are-evaluated.md index cc7059ff3..dff1c78b0 100644 --- a/grafana-alertcheck/docs/how-alerts-are-evaluated.md +++ b/grafana-alertcheck/docs/how-alerts-are-evaluated.md @@ -71,6 +71,8 @@ Two coverage checks differ for these rules: - **Pause is not observable** — the datasource API has no `isPaused` signal, so check 7 is skipped with an explicit note. A pause is not treated as a pass; it simply cannot be seen. - **Health** — the datasource vocabulary reports `err`, which is normalized to `error`, so a sustained failing evaluation still triggers check 4. There are no `totals`, reasons or normal instances, so checks 5 and 9 never fire. +There is also no `recovering` state: the datasource API keeps an alert `firing` through its *keep firing for* and then drops it, so the recovery observation below does not apply — an instance that clears simply leaves the active set, which is the `cleared` recovery described above. + ## Coverage proof Before classifying, `check` must **prove** continuous coverage of `[from, to]` for each alert. Nine checks run; any failure makes the rule `not_verified`: diff --git a/grafana-alertcheck/docs/reference/log-format.md b/grafana-alertcheck/docs/reference/log-format.md index 6c8429adb..f96302b4b 100644 --- a/grafana-alertcheck/docs/reference/log-format.md +++ b/grafana-alertcheck/docs/reference/log-format.md @@ -102,6 +102,7 @@ Field notes: - `grafana_now` is the response's `Date` header — never the runner clock. - `skew_ms`/`skew_bound_ms` are the per-poll clock-skew estimate and its uncertainty (RTT/2), in milliseconds for compactness only. - `found: false` is an authoritative `2xx` in which this rule was absent — a transport failure is retried and never becomes a poll. +- `keep_firing_for_ms` is the rule's recovery period as reported by this response; `0`/absent means no instance can be `recovering`. A datasource rule reports it from the backend's `keep_firing_for`, but such a rule never reaches `recovering` (the backend keeps it firing, then drops it). - `state`, `health`, `last_error` are raw rule-level strings, reporting-only. - `histogram` is a verbatim copy of the response `totals`; written, never analysed. - `reasons` counts non-empty instance reasons (`NoData`, `Error`, `KeepLast`, …); composite states stay visible only here. diff --git a/grafana-alertcheck/internal/gate/handoff.go b/grafana-alertcheck/internal/gate/handoff.go index 716bf06b3..02286ebbe 100644 --- a/grafana-alertcheck/internal/gate/handoff.go +++ b/grafana-alertcheck/internal/gate/handoff.go @@ -38,10 +38,10 @@ func CheckStartupHandoff(t map[string]RuleTimings, measured map[string]time.Dura fmt.Fprintf(&b, " - %s\n", p) } if minC := minHandoffConcurrency(t, measured, first, readyAt, windowOpen, concurrency, len(t)); minC > concurrency { - fmt.Fprintf(&b, "fix by: raising --concurrency to at least %d (currently %d), raising poll-interval, or watching fewer alerts", + fmt.Fprintf(&b, "fix by: raising --concurrency to at least %d (currently %d), or watching fewer alerts", minC, concurrency) } else { - b.WriteString("fix by: raising poll-interval or watching fewer alerts") + b.WriteString("fix by: watching fewer alerts") } return fmt.Errorf("%s", b.String()) } diff --git a/grafana-alertcheck/internal/gate/handoff_test.go b/grafana-alertcheck/internal/gate/handoff_test.go index bdcf45f06..71043d3a6 100644 --- a/grafana-alertcheck/internal/gate/handoff_test.go +++ b/grafana-alertcheck/internal/gate/handoff_test.go @@ -138,6 +138,6 @@ func TestCheckStartupHandoff_NoConcurrencyCanFixIt(t *testing.T) { err := CheckStartupHandoff(timings, measured, first, base, base.Add(-time.Second), 1) require.Error(t, err) - require.Contains(t, err.Error(), "fix by: raising poll-interval") - require.NotContains(t, err.Error(), "raising concurrency") + require.Contains(t, err.Error(), "fix by: watching fewer alerts") + require.NotContains(t, err.Error(), "raising --concurrency") } diff --git a/grafana-alertcheck/internal/gate/log.go b/grafana-alertcheck/internal/gate/log.go index be79fc257..3a5d101f1 100644 --- a/grafana-alertcheck/internal/gate/log.go +++ b/grafana-alertcheck/internal/gate/log.go @@ -63,8 +63,8 @@ type LoggedRule struct { NoDataState string `json:"no_data_state"` ExecErrState string `json:"exec_err_state"` // PollEverySeconds is the cadence this recording ACTUALLY used. Load-bearing: - // check derives maxGap from it, never from the definitions — getting that - // wrong is fail-open in the faster-override direction. + // check derives maxGap from it, never from the definitions — the two must + // not be allowed to diverge. PollEverySeconds float64 `json:"poll_every_seconds"` } diff --git a/grafana-alertcheck/internal/gate/parse_datasource.go b/grafana-alertcheck/internal/gate/parse_datasource.go index daedfcacf..31a3b294d 100644 --- a/grafana-alertcheck/internal/gate/parse_datasource.go +++ b/grafana-alertcheck/internal/gate/parse_datasource.go @@ -128,6 +128,14 @@ func parseDatasourceRule(raw json.RawMessage, dsUID, group, file string, interva if err := opt(m, "lastError", &r.LastError); err != nil { return StateRule{}, fmt.Errorf("rule %q: %w", name, err) } + // vmalert reports the alerting rule's keep-firing-for in seconds under the + // snake_case key. Unlike Grafana it has no recovering state: the alert stays + // firing for this long and is then dropped, so this is informational. + var keepFiringForSeconds float64 + if err := opt(m, "keep_firing_for", &keepFiringForSeconds); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } + r.KeepFiringFor = time.Duration(keepFiringForSeconds * float64(time.Second)) if err := opt(m, "state", &r.State); err != nil { return StateRule{}, fmt.Errorf("rule %q: %w", name, err) @@ -150,7 +158,8 @@ func parseDatasourceRule(raw json.RawMessage, dsUID, group, file string, interva // datasourceInstanceStates is the strict datasource instance-state vocabulary. // Only the two active states exist — a resolved instance is absent from the -// response, not reported as normal. +// response, not reported as normal, and there is no recovering state (the +// backend keeps an alert firing through its keep-firing-for, then drops it). var datasourceInstanceStates = map[string]State{ "firing": StateFiring, "pending": StatePending, diff --git a/grafana-alertcheck/internal/gate/parse_datasource_test.go b/grafana-alertcheck/internal/gate/parse_datasource_test.go index 9205a9be3..3ad2b3c45 100644 --- a/grafana-alertcheck/internal/gate/parse_datasource_test.go +++ b/grafana-alertcheck/internal/gate/parse_datasource_test.go @@ -58,6 +58,16 @@ func TestParseDatasourceRules_LastError(t *testing.T) { require.Equal(t, "query failed: bad", rules[0].LastError) } +// vmalert's keep-firing-for is snake_case and in seconds; the alert stays +// firing for it, so it is recorded but never a recovering state. +func TestParseDatasourceRules_KeepFiringFor(t *testing.T) { + body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ + {"name":"A","type":"alerting","health":"ok","state":"firing","keep_firing_for":300}]}]}}`) + rules, err := ParseDatasourceRules(body, "d") + require.NoError(t, err) + require.Equal(t, 5*time.Minute, rules[0].KeepFiringFor) +} + func TestParseDatasourceRules_ZeroLastEvaluationAllowed(t *testing.T) { body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ {"name":"A","type":"alerting","health":"ok","state":"pending"}]}]}}`) diff --git a/grafana-alertcheck/internal/gate/schedule.go b/grafana-alertcheck/internal/gate/schedule.go index 8f94d5245..f38c114b2 100644 --- a/grafana-alertcheck/internal/gate/schedule.go +++ b/grafana-alertcheck/internal/gate/schedule.go @@ -98,9 +98,10 @@ func pausedSet(defs []Definition) map[string]bool { // DeriveTimingsFromLog is DeriveTimings' log-mode counterpart: pollEvery comes // from the header (the cadence actually used), not the definitions — re-deriving -// it here would compare recorded gaps against default-cadence thresholds, an -// exit 2 on a clean window (slower override) or a silently passing recorder gap -// (faster override). evalStaleAfter still comes from defs (2 × intervalSeconds). +// it here would compare recorded gaps against thresholds derived from a +// different cadence, an exit 2 on a clean window when the recording was slower +// or a silently passing recorder gap when it was faster. evalStaleAfter still +// comes from defs (2 × intervalSeconds). // // Three header shapes are hard errors rather than a best-effort derivation, // because each would silently widen a threshold: a rule with no matching From 870a86d851fddd543b48d25b40c0492430b25c6c Mon Sep 17 00:00:00 2001 From: Bartek Tofel <tofel.b@gmail.com> Date: Tue, 6 Oct 2026 13:49:05 +0200 Subject: [PATCH 7/8] chore: address CR --- grafana-alertcheck/.changeset/v0.1.10.md | 2 + grafana-alertcheck/docs/reference/cli.md | 2 +- grafana-alertcheck/internal/gate/check.go | 4 ++ .../internal/gate/check_test.go | 15 ++++++ .../internal/gate/parse_datasource.go | 26 +++++---- .../internal/gate/parse_datasource_test.go | 18 +++++-- grafana-alertcheck/internal/gate/resolve.go | 53 +++++-------------- .../internal/gate/resolve_test.go | 11 ---- .../internal/gate/source_ds_test.go | 2 +- 9 files changed, 66 insertions(+), 67 deletions(-) diff --git a/grafana-alertcheck/.changeset/v0.1.10.md b/grafana-alertcheck/.changeset/v0.1.10.md index 96924ea3a..a02d1e8f2 100644 --- a/grafana-alertcheck/.changeset/v0.1.10.md +++ b/grafana-alertcheck/.changeset/v0.1.10.md @@ -8,3 +8,5 @@ - A backend can serve two distinct datasource-managed rules under one identity (same datasource/group/name/file, differing by labels/query). Loading and `list` accept them, but a selection that includes such a rule fails closed: the state query cannot tell the siblings apart, so narrowing to one does not make it observable. `uid:` selectors no longer trigger a bulk datasource read, and a datasource rule's `lastError` is recorded. - A datasource-managed rule's name can itself contain `/` (e.g. `devex-cicd/prod/griddle-github: ContainersNotReady`). The full name is now matched exactly and used as the `rule_name[]` fetch filter, instead of splitting it into `Group/Title` segments and filtering by the last segment. - The `RESULTS` table gains a `SOURCE` column (`grafana`/`datasource`). Datasource caveats are printed once before the table, and `DETAILS` carries only rule-specific notes without repeating the alert name. `--output json` adds `source_kind` and a run-level `caveats`. +- Recording rules are never observed: a datasource rules response drops them at parse time, so one can no longer shadow a same-named alerting rule in state selection. +- A no-match error no longer lists substring suggestions; it points at `list`. diff --git a/grafana-alertcheck/docs/reference/cli.md b/grafana-alertcheck/docs/reference/cli.md index 408c2d146..64439b154 100644 --- a/grafana-alertcheck/docs/reference/cli.md +++ b/grafana-alertcheck/docs/reference/cli.md @@ -111,7 +111,7 @@ Alert names take one of these forms. Grafana-managed rules use folder/group; dat A datasource rule's **name can itself contain `/`** (e.g. `devex-cicd/prod/griddle-github: ContainersNotReady`). The exact name is tried first, so the `TITLE` from `list` always resolves, and `key:` is the unambiguous fallback. -`--folder` scopes a bare Grafana title only. A recording rule, or a datasource rule with no identifiable datasource, is refused with a specific error; a no-match suggests substrings and points at `list`; an ambiguous name lists every candidate with its full name, source and `uid:`/`key:`. Duplicate names collapse to one (a note, not an error). +`--folder` scopes a bare Grafana title only. A recording rule, or a datasource rule with no identifiable datasource, is refused with a specific error; a no-match points at `list`; an ambiguous name lists every candidate with its full name, source and `uid:`/`key:`. Duplicate names collapse to one (a note, not an error). Auto-discovery keeps `/api/datasources` entries with `type == "prometheus"` and `jsonData.manageAlerts == true`, then probes each. The token needs `datasources:read` plus datasource query permission; a failure names the permission. diff --git a/grafana-alertcheck/internal/gate/check.go b/grafana-alertcheck/internal/gate/check.go index a8cf9aca9..b754b866d 100644 --- a/grafana-alertcheck/internal/gate/check.go +++ b/grafana-alertcheck/internal/gate/check.go @@ -585,6 +585,10 @@ func resolveFromLog(ctx context.Context, src Source, h Header, cfg Config) ([]De return nil, nil, fmt.Errorf("log identity: %s names rule %s (%q), which no current definition matches", cfg.Log, loggedKey(lr), lr.Title) } + if d.Kind != KindGrafanaManaged { + return nil, nil, fmt.Errorf("log identity: %s names rule %s (%q), which is now a %s", + cfg.Log, loggedKey(lr), lr.Title, kindName(d.Kind)) + } resolved = append(resolved, d) } } diff --git a/grafana-alertcheck/internal/gate/check_test.go b/grafana-alertcheck/internal/gate/check_test.go index 9a2dce38a..ce71d2d92 100644 --- a/grafana-alertcheck/internal/gate/check_test.go +++ b/grafana-alertcheck/internal/gate/check_test.go @@ -1077,6 +1077,21 @@ func TestCheckFailClosedOnWrongLogIdentity(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "log identity") }) + + t.Run("rule is now a recording rule", func(t *testing.T) { + dir := t.TempDir() + windowEnd := testNow.Add(5*time.Minute + checkGrace) + logPath := recordedLog(t, dir, "https://grafana.example.com", + testNow.Add(-time.Minute), testNow.Add(-time.Minute), windowEnd, windowEnd.Add(30*time.Second), 0) + + cfg := recorderConfig(t, newVirtualClock(testNow), logPath) + src := newCheckSource(nil) + src.defs = []Definition{{UID: checkUID, Title: checkTitle, Kind: KindRecording, IntervalSeconds: 60}} + + _, err := check(context.Background(), cfg, src) + require.Error(t, err) + require.Contains(t, err.Error(), "recording rule") + }) } // `from` before the recording's StartedAt is statically knowable from the diff --git a/grafana-alertcheck/internal/gate/parse_datasource.go b/grafana-alertcheck/internal/gate/parse_datasource.go index 31a3b294d..eb6064eff 100644 --- a/grafana-alertcheck/internal/gate/parse_datasource.go +++ b/grafana-alertcheck/internal/gate/parse_datasource.go @@ -13,6 +13,11 @@ import ( // states, health "err" (not "error"), and a zero lastEvaluation is allowed // (liveness treats zero as maximally stale). A missing or unparseable required // field is an error, never a zero value. +// +// Recording rules are dropped here, at the one place a response is parsed: +// they have no instances or state to observe, and keeping them would let a +// recording rule that shares a datasource/group/name/file shadow the alerting +// rule in state selection. func ParseDatasourceRules(body []byte, dsUID string) ([]StateRule, error) { var top map[string]json.RawMessage if err := json.Unmarshal(body, &top); err != nil { @@ -59,6 +64,9 @@ func ParseDatasourceRules(body []byte, dsUID string) ([]StateRule, error) { if err != nil { return nil, fmt.Errorf("datasource rules response: group %q: rule %d: %w", groupName, ri, err) } + if rule.Type != "alerting" { + continue + } rules = append(rules, rule) } } @@ -99,9 +107,9 @@ func parseDatasourceRule(raw json.RawMessage, dsUID, group, file string, interva } r.For = time.Duration(durationSeconds * float64(time.Second)) - // Recording rules carry no state, health or alerts; DefinitionsFromDatasource - // drops them, so parsing only the shared fields keeps a recording rule from - // failing on fields it was never going to have. + // Recording rules carry no state, health or alerts, and ParseDatasourceRules + // drops them; parsing only the shared fields keeps one from failing on + // fields it was never going to have before the caller discards it. if ruleType == "recording" { return r, nil } @@ -177,15 +185,13 @@ func normalizeDatasourceInstanceState(s string) (State, string, error) { return state, reason, nil } -// DefinitionsFromDatasource converts datasource rule states into Definitions, -// keeping only alerting rules. A datasource-managed rule has no pause signal and -// no uid, so PauseObservable is false and UID stays empty. +// DefinitionsFromDatasource converts datasource rule states into Definitions. +// Its input is already alerting-only (ParseDatasourceRules drops recording +// rules). A datasource-managed rule has no pause signal and no uid, so +// PauseObservable is false and UID stays empty. func DefinitionsFromDatasource(rules []StateRule, dsUID, dsName string) []Definition { - var defs []Definition + defs := make([]Definition, 0, len(rules)) for _, r := range rules { - if r.Type != "alerting" { - continue - } defs = append(defs, Definition{ Key: r.Key, Title: r.Title, diff --git a/grafana-alertcheck/internal/gate/parse_datasource_test.go b/grafana-alertcheck/internal/gate/parse_datasource_test.go index 3ad2b3c45..78a1f80b3 100644 --- a/grafana-alertcheck/internal/gate/parse_datasource_test.go +++ b/grafana-alertcheck/internal/gate/parse_datasource_test.go @@ -10,7 +10,7 @@ import ( func TestParseDatasourceRules_Fixture(t *testing.T) { rules, err := ParseDatasourceRules(readFixture(t, "ds_rules.json"), "ds-uid") require.NoError(t, err) - require.Len(t, rules, 2, "recording rules are parsed but filtered later") + require.Len(t, rules, 1, "recording rules are dropped at parse time") alert := rules[0] require.Equal(t, "ExampleTargetDown", alert.Title) @@ -29,18 +29,30 @@ func TestParseDatasourceRules_Fixture(t *testing.T) { require.Nil(t, alert.Totals) } -func TestDefinitionsFromDatasource_FiltersRecording(t *testing.T) { +func TestDefinitionsFromDatasource(t *testing.T) { rules, err := ParseDatasourceRules(readFixture(t, "ds_rules.json"), "ds-uid") require.NoError(t, err) defs := DefinitionsFromDatasource(rules, "ds-uid", "ExampleMetrics") - require.Len(t, defs, 1, "only the alerting rule becomes a Definition") + require.Len(t, defs, 1) require.Equal(t, KindDatasourceManaged, defs[0].Kind) require.Equal(t, "ExampleMetrics", defs[0].DatasourceName) require.False(t, defs[0].PauseObservable) require.Equal(t, 60, defs[0].IntervalSeconds) } +// A recording rule that shares the alerting rule's datasource/group/name/file +// must never reach state selection, or it could shadow the alert. +func TestParseDatasourceRules_DropsRecordingShadow(t *testing.T) { + body := []byte(`{"status":"success","data":{"groups":[{"name":"g","file":"f","interval":60,"rules":[ + {"name":"A","type":"recording","query":"up"}, + {"name":"A","type":"alerting","health":"ok","state":"firing","lastEvaluation":"2026-08-01T00:00:00Z"}]}]}}`) + rules, err := ParseDatasourceRules(body, "d") + require.NoError(t, err) + require.Len(t, rules, 1) + require.Equal(t, "alerting", rules[0].Type) +} + func TestParseDatasourceRules_HealthErrNormalizes(t *testing.T) { body := []byte(`{"status":"success","data":{"groups":[{"name":"g","file":"f","interval":60,"rules":[ {"name":"A","type":"alerting","health":"err","lastEvaluation":"2026-08-01T00:00:00Z","state":"firing"}]}]}}`) diff --git a/grafana-alertcheck/internal/gate/resolve.go b/grafana-alertcheck/internal/gate/resolve.go index 9cf44c4b0..9d906e9ab 100644 --- a/grafana-alertcheck/internal/gate/resolve.go +++ b/grafana-alertcheck/internal/gate/resolve.go @@ -47,9 +47,8 @@ func Resolve(defs []Definition, names []string, folder string) (resolved []Defin } // resolveOne resolves one trimmed, non-empty name against defs: one match wins, -// zero is an error with suggestions, two or more is ambiguous. folder scopes a -// bare Grafana title; it is ignored for /-separated forms and for datasource -// rules. +// zero is a no-match error, two or more is ambiguous. folder scopes a bare +// Grafana title; it is ignored for /-separated forms and for datasource rules. // // Grafana forms: Title | Folder/Title | Folder/Group/Title. Datasource forms: // Title | Group/Title | DatasourceName/Group/Title. key: is exact across both. @@ -85,13 +84,9 @@ func resolveOne(defs []Definition, name, folder string) (Definition, error) { } // uid == "" falls through to the same message as "not found": several // Definition kinds legitimately carry UID == "" (datasource-managed - // rules have no uid at all), so matching on an empty suffix - // would silently hit one of those and report a misleading - // kind-specific refusal for what is really an empty/typo'd uid. This - // deliberately does not go through noMatchError: that function's - // substring suggestion would degenerate to an empty needle, which - // strings.Contains matches against every title — printing the whole - // fleet instead of a real suggestion. + // rules have no uid at all), so matching on an empty suffix would + // silently hit one of those and report a misleading kind-specific + // refusal for what is really an empty/typo'd uid. return Definition{}, fmt.Errorf("no rule matched %q: no rule has this uid (run 'grafana-alertcheck list' to see uids)", name) } @@ -119,7 +114,7 @@ func resolveOne(defs []Definition, name, folder string) (Definition, error) { if formErr != nil { return Definition{}, formErr } - return Definition{}, noMatchError(supportedDefs(defs), name, parts[len(parts)-1]) + return Definition{}, noMatchError(supportedDefs(defs), name) } // pickCandidate applies the shared one-match/ambiguous/unsupported policy to the @@ -197,7 +192,7 @@ func isSupported(d Definition) bool { } // supportedDefs filters out the refused kinds. Only these participate in -// name-based matching, the no-match rule count, and substring suggestions. +// name-based matching and the no-match rule count. func supportedDefs(defs []Definition) []Definition { out := make([]Definition, 0, len(defs)) for _, d := range defs { @@ -210,10 +205,8 @@ func supportedDefs(defs []Definition) []Definition { } // parseNameForm splits name into 1..3 /-separated segments. Every segment must -// be non-empty: without this, "/Title" would parse as an empty first segment — -// silently dropping the filter and matching unscoped, a fail-open — and -// "Folder/" would parse as an empty title, feeding noMatchError's substring -// search an empty needle that matches every title. +// be non-empty: without this, "/Title" would parse as an empty first segment, +// silently dropping the filter and matching unscoped — a fail-open. func parseNameForm(name string) ([]string, error) { parts := strings.Split(name, "/") if slices.Contains(parts, "") { @@ -246,32 +239,10 @@ func ruleRefLabel(d Definition) string { return "key:" + defKey(d) } -// noMatchError reports a no-match with the count of supported rules and -// case-insensitive substring suggestions. -func noMatchError(defs []Definition, name, wantTitle string) error { - msg := fmt.Sprintf("no rule matched %q (%d rules available; run 'grafana-alertcheck list' to see titles)", +// noMatchError reports a no-match with the count of supported rules. +func noMatchError(defs []Definition, name string) error { + return fmt.Errorf("no rule matched %q (%d rules available; run 'grafana-alertcheck list' to see titles)", name, len(defs)) - - needle := strings.ToLower(wantTitle) - var subs []string - for _, d := range defs { - if strings.Contains(strings.ToLower(d.Title), needle) { - subs = append(subs, suggestionLabel(d)) - } - } - if len(subs) > 0 { - sort.Strings(subs) - msg += fmt.Sprintf("; did you mean: %s", strings.Join(subs, ", ")) - } - return fmt.Errorf("%s", msg) -} - -// suggestionLabel is the copyable name form for a supported rule. -func suggestionLabel(d Definition) string { - if d.Kind == KindDatasourceManaged { - return fmt.Sprintf("%s/%s/%s", d.DatasourceName, d.Group, d.Title) - } - return fmt.Sprintf("%s/%s/%s", d.Folder, d.Group, d.Title) } // ambiguousError lists every candidate with its source, its group, and the full diff --git a/grafana-alertcheck/internal/gate/resolve_test.go b/grafana-alertcheck/internal/gate/resolve_test.go index ef46ad573..9be9f0fb8 100644 --- a/grafana-alertcheck/internal/gate/resolve_test.go +++ b/grafana-alertcheck/internal/gate/resolve_test.go @@ -63,14 +63,6 @@ func TestResolve_NoMatch(t *testing.T) { require.Contains(t, err.Error(), "list") } -func TestResolve_NoMatchSubstringSuggestion(t *testing.T) { - defs := rulerDefs(t) - _, _, err := Resolve(defs, []string{"paused rule"}, "") - require.Error(t, err) - require.Contains(t, err.Error(), "did you mean") - require.Contains(t, err.Error(), "Example Paused Rule") -} - func TestResolve_RefusesDatasourceManaged(t *testing.T) { defs, err := ParseDefinitions(readFixture(t, "ruler_datasource_managed.json")) require.NoError(t, err) @@ -134,9 +126,6 @@ func TestResolve_UnsupportedKindsExcludedFromNoMatchSurfaces(t *testing.T) { wantCount := fmt.Sprintf("(%d rules available", len(supported)) require.Contains(t, err.Error(), wantCount) - require.NotContains(t, err.Error(), "ExampleTargetDown") - require.NotContains(t, err.Error(), "example:recorded_metric:rate5m") - require.Contains(t, err.Error(), "Example Paused Rule") } func TestResolve_UnsupportedHomonymResolvesSupportedSilently(t *testing.T) { diff --git a/grafana-alertcheck/internal/gate/source_ds_test.go b/grafana-alertcheck/internal/gate/source_ds_test.go index 1e1609ce2..0de93ac6c 100644 --- a/grafana-alertcheck/internal/gate/source_ds_test.go +++ b/grafana-alertcheck/internal/gate/source_ds_test.go @@ -188,6 +188,6 @@ func TestRuleState_DatasourceAssertsAllFilters(t *testing.T) { require.Equal(t, "file%5B%5D=%2Fetc%2Fvm%2Frules%2Fexample.yml&rule_group%5B%5D=ExampleMetrics&rule_name%5B%5D=ExampleTargetDown", gotQuery) - require.Len(t, obs.Rules, 2) + require.Len(t, obs.Rules, 1, "the recording rule is dropped at parse time") require.Equal(t, ref.Key, obs.Rules[0].Key) } From 58142d3037bbb2d5690ead35228802e9f1663bb8 Mon Sep 17 00:00:00 2001 From: Bartek Tofel <tofel.b@gmail.com> Date: Tue, 6 Oct 2026 13:58:23 +0200 Subject: [PATCH 8/8] chore: address CR --- grafana-alertcheck/.changeset/v0.1.10.md | 2 +- .../docs/reference/log-format.md | 2 +- .../internal/gate/parse_datasource.go | 15 ++++++++-- .../internal/gate/parse_datasource_test.go | 29 +++++++++++++++---- 4 files changed, 37 insertions(+), 11 deletions(-) diff --git a/grafana-alertcheck/.changeset/v0.1.10.md b/grafana-alertcheck/.changeset/v0.1.10.md index a02d1e8f2..011c43b4d 100644 --- a/grafana-alertcheck/.changeset/v0.1.10.md +++ b/grafana-alertcheck/.changeset/v0.1.10.md @@ -3,7 +3,7 @@ - Datasource-managed identity is a separate rule key; `uid` stays empty for these rules. The log gains additive `key`, `source_kind`, `datasource_uid`, `datasource_name`, `file` and `rule_key` fields, with `schema_version` still `1`. An old v1 log without `rule_key` remains readable; a new log read by an old binary fails closed on the unresolved key. - Datasource recovery semantics: because the Prometheus API returns only active instances, an instance leaving the active set is treated as a real recovery (`cleared`). Documented weaker guarantee: a vanished series is indistinguishable from a resolution. - Pause is not observable for datasource-managed rules (no `isPaused` signal), so check 7 is skipped with an explicit note. `health: err` normalizes to `error` and still triggers check 4. -- Datasource-managed rules record the backend's `keep_firing_for` (`keep_firing_for_ms` in the log). The datasource API has no `recovering` state — it keeps an alert firing through its keep-firing-for and then drops it — so the recovery observation applies to Grafana-managed rules only. +- Datasource-managed rules record the backend's keep-firing-for (`keep_firing_for` on vmalert, `keepFiringFor` on Prometheus/Mimir; `keep_firing_for_ms` in the log). The datasource API has no `recovering` state — it keeps an alert firing through its keep-firing-for and then drops it — so the recovery observation applies to Grafana-managed rules only. A datasource rule with an unrecognized `type` is now a hard error rather than silently dropped. - The token now needs `datasources:read` plus datasource query permission even for Grafana-only runs; discovery failures name the missing permission. - A backend can serve two distinct datasource-managed rules under one identity (same datasource/group/name/file, differing by labels/query). Loading and `list` accept them, but a selection that includes such a rule fails closed: the state query cannot tell the siblings apart, so narrowing to one does not make it observable. `uid:` selectors no longer trigger a bulk datasource read, and a datasource rule's `lastError` is recorded. - A datasource-managed rule's name can itself contain `/` (e.g. `devex-cicd/prod/griddle-github: ContainersNotReady`). The full name is now matched exactly and used as the `rule_name[]` fetch filter, instead of splitting it into `Group/Title` segments and filtering by the last segment. diff --git a/grafana-alertcheck/docs/reference/log-format.md b/grafana-alertcheck/docs/reference/log-format.md index f96302b4b..c07418223 100644 --- a/grafana-alertcheck/docs/reference/log-format.md +++ b/grafana-alertcheck/docs/reference/log-format.md @@ -102,7 +102,7 @@ Field notes: - `grafana_now` is the response's `Date` header — never the runner clock. - `skew_ms`/`skew_bound_ms` are the per-poll clock-skew estimate and its uncertainty (RTT/2), in milliseconds for compactness only. - `found: false` is an authoritative `2xx` in which this rule was absent — a transport failure is retried and never becomes a poll. -- `keep_firing_for_ms` is the rule's recovery period as reported by this response; `0`/absent means no instance can be `recovering`. A datasource rule reports it from the backend's `keep_firing_for`, but such a rule never reaches `recovering` (the backend keeps it firing, then drops it). +- `keep_firing_for_ms` is the rule's recovery period as reported by this response; `0`/absent means no instance can be `recovering`. A datasource rule reports it from the backend's keep-firing-for (`keep_firing_for` on vmalert, `keepFiringFor` on Prometheus/Mimir), but such a rule never reaches `recovering` (the backend keeps it firing, then drops it). - `state`, `health`, `last_error` are raw rule-level strings, reporting-only. - `histogram` is a verbatim copy of the response `totals`; written, never analysed. - `reasons` counts non-empty instance reasons (`NoData`, `Error`, `KeepLast`, …); composite states stay visible only here. diff --git a/grafana-alertcheck/internal/gate/parse_datasource.go b/grafana-alertcheck/internal/gate/parse_datasource.go index eb6064eff..952c248e3 100644 --- a/grafana-alertcheck/internal/gate/parse_datasource.go +++ b/grafana-alertcheck/internal/gate/parse_datasource.go @@ -85,6 +85,11 @@ func parseDatasourceRule(raw json.RawMessage, dsUID, group, file string, interva if err := req(m, "type", &ruleType); err != nil { return StateRule{}, fmt.Errorf("rule %q: %w", name, err) } + // Reject an unrecognized type rather than dropping it: a schema change must + // not silently shrink the rule set the run proceeds over. + if ruleType != "alerting" && ruleType != "recording" { + return StateRule{}, fmt.Errorf("rule %q: unknown rule type %q (want alerting or recording)", name, ruleType) + } r := StateRule{ Key: ruleKey(dsUID, group, name, file, ""), @@ -136,13 +141,17 @@ func parseDatasourceRule(raw json.RawMessage, dsUID, group, file string, interva if err := opt(m, "lastError", &r.LastError); err != nil { return StateRule{}, fmt.Errorf("rule %q: %w", name, err) } - // vmalert reports the alerting rule's keep-firing-for in seconds under the - // snake_case key. Unlike Grafana it has no recovering state: the alert stays - // firing for this long and is then dropped, so this is informational. + // The keep-firing-for period, in seconds. vmalert spells it keep_firing_for, + // Prometheus and Mimir keepFiringFor. Unlike Grafana there is no recovering + // state: the alert stays firing for this long and is then dropped, so this + // is informational. var keepFiringForSeconds float64 if err := opt(m, "keep_firing_for", &keepFiringForSeconds); err != nil { return StateRule{}, fmt.Errorf("rule %q: %w", name, err) } + if err := opt(m, "keepFiringFor", &keepFiringForSeconds); err != nil { + return StateRule{}, fmt.Errorf("rule %q: %w", name, err) + } r.KeepFiringFor = time.Duration(keepFiringForSeconds * float64(time.Second)) if err := opt(m, "state", &r.State); err != nil { diff --git a/grafana-alertcheck/internal/gate/parse_datasource_test.go b/grafana-alertcheck/internal/gate/parse_datasource_test.go index 78a1f80b3..806d2defa 100644 --- a/grafana-alertcheck/internal/gate/parse_datasource_test.go +++ b/grafana-alertcheck/internal/gate/parse_datasource_test.go @@ -70,14 +70,31 @@ func TestParseDatasourceRules_LastError(t *testing.T) { require.Equal(t, "query failed: bad", rules[0].LastError) } -// vmalert's keep-firing-for is snake_case and in seconds; the alert stays -// firing for it, so it is recorded but never a recovering state. +// The keep-firing-for is in seconds and spelled keep_firing_for by vmalert and +// keepFiringFor by Prometheus/Mimir; the alert stays firing for it, so it is +// recorded but never a recovering state. func TestParseDatasourceRules_KeepFiringFor(t *testing.T) { + for name, body := range map[string][]byte{ + "vmalert": []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ + {"name":"A","type":"alerting","health":"ok","state":"firing","keep_firing_for":300}]}]}}`), + "prometheus": []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ + {"name":"A","type":"alerting","health":"ok","state":"firing","keepFiringFor":300}]}]}}`), + } { + t.Run(name, func(t *testing.T) { + rules, err := ParseDatasourceRules(body, "d") + require.NoError(t, err) + require.Equal(t, 5*time.Minute, rules[0].KeepFiringFor) + }) + } +} + +// An unknown rule type must fail closed, not be dropped from the inventory. +func TestParseDatasourceRules_UnknownTypeIsError(t *testing.T) { body := []byte(`{"status":"success","data":{"groups":[{"name":"g","rules":[ - {"name":"A","type":"alerting","health":"ok","state":"firing","keep_firing_for":300}]}]}}`) - rules, err := ParseDatasourceRules(body, "d") - require.NoError(t, err) - require.Equal(t, 5*time.Minute, rules[0].KeepFiringFor) + {"name":"A","type":"future","health":"ok","state":"firing"}]}]}}`) + _, err := ParseDatasourceRules(body, "d") + require.Error(t, err) + require.Contains(t, err.Error(), "unknown rule type") } func TestParseDatasourceRules_ZeroLastEvaluationAllowed(t *testing.T) {