chore: address CR

Author: Tofel

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.

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.

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 {

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) {