[DX-5122] grafana alertcheck - #2808
Closed
Tofel wants to merge 43 commits into
Closed
Conversation
Strict parsers for the state and ruler endpoints (H1), a Prometheus-style duration parser, and fixtures sliced from real Grafana 13.1.0 payloads covering every required/optional-field and must-error case, including the "Normal (NoData)"/"Normal (Error)" composite reason states found live in the current fleet capture (not in the original plan's vocabulary).
Fix retry-error conflation, measure full poll latency, and harden Source test doubles for concurrency.
Add Resolve() for alert name resolution (uid:/Title/Folder/Title/ Folder/Group/Title forms, UID collapse, no-match suggestions) and the grafana-alertcheck CLI's list subcommand, the first runnable piece of the gate. Incorporates review fixes: reject empty path segments in classifyForm, guard uid: against an empty suffix, scope the no-match rule count and suggestions to supported rule kinds only, and exit 0 on -h/--help.
Add per-rule poll timings, scheduler, and budget check (P4). - schedule.go: DeriveTimings, Scheduler, CheckBudget (§5) - Address review: add Folder/Title resolve test, rename CheckBudget's minPollEvery to tightestUID
Add the JSONL evidence log (P5). - log.go: Header/Poll records, reduction, H2 transition markers, §3.2 verification, append-only Writer with flock, ReadLog - flock_unix.go: non-blocking exclusive lock, unix only - schedule.go: DeriveTimingsFromLog — log-mode cadence comes from the header, never from the definitions
Invariant defended: H2. The one question: can watch return success over a window that nothing is recording? Watch() records the first observation of each non-skipped rule, then detaches a child that polls at the cadence in the header. The parent returns only after the child reports ready on an inherited pipe, and writes the pidfile after that. A clean stop writes the sentinel; a hard error does not.
Invariant defended: H3. The one question: can a rule be called alive because it looked alive one poll ago? proveCoverage (grafana-alertcheck/internal/gate/coverage.go) is the pure coverage function: nine checks over one rule's polls — sentinel, from-bounds, heartbeat continuity, health error/nodata, liveness, in-window pause, rule absence, KeepLast. Liveness is absolute, never a delta. Cross-domain comparisons translate by each poll's own skew and widen boundary segments by its skew bound, fail-closed.
Invariant defended: H6/H7. The one question: can a violation ever outrank an unobservable rule, or can a pass happen without Violations empty and err nil? Adds classify.go: the pure per-instance classifier (outcome table, preexisting policy, BadFor) and decide(), the seam combining proveCoverage with those timelines under one Policy. Consolidates rule-poll filtering and skew translation onto pollsForRule/runnerTime, shared with coverage.go.
Invariant defended: H5/H7. The one question: can check report a pass over a window it did not prove? Check() is the I/O shell around the pure decide(). Single-step synthesizes the header and its own sentinel, so no mode flag reaches the pure layer. Log mode stops the recorder before the one full read. The header, not a definition re-resolved after the window closed, is the authority for what was paused when the window opened — it decides `skipped`, the drain set, and the transitionGrace max. The flock, not the pidfile, is the authority for whether a writer still exists.
renderer and H6/H7 exit-code mapping. Extend Result with per-rule/global thresholds and a real skew bound; export SkewHardLimit; reject --states normal.
Add coverage.go's declared-KeepLast check (no_data_state/exec_err_state, not just an observed reason) and close the remaining §22 gaps: newly_bad's no-early-exit clock assertion, a recorder-mode gap right after the deploy, a rule's own coverage gap overriding its own recovery, a genuinely skew-discriminating staleness test, exit-2 consequences on two Reason-only coverage tests, and end-to-end checks for log-name collapse, a truncated log, and the real watch-written state histogram.
|
👋 Tofel, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a few correctness/documentation issues (lock contention detection, response size limit handling, and naming/key docs mismatches) that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new grafana-alertcheck Go module to the repo: a release quality gate that records Grafana alert states during a window (watch) and then proves coverage + classifies outcomes (check) using strict parsing, deterministic rule resolution, and a fail-closed contract.
Changes:
- Introduces
internal/gatecore logic (HTTP source, strict parsers, coverage proof, rule-name resolution, process/lock handling) with extensive unit + integration-style tests and fixtures. - Adds CLI surface (
cmd/) forlist,watch, andcheck, plus human table output and Docusaurus docs. - Wires module into CI (workflow path added) and adds repo ignore entries/tool version pins for development.
File summaries
| File | Description |
|---|---|
| grafana-alertcheck/README.md | Module README and quickstart |
| grafana-alertcheck/internal/gate/watch_process.go | Detached watch child spawn/re-exec logic |
| grafana-alertcheck/internal/gate/watch_daemon_test.go | Integration test exercising real fork/exec + setsid recorder |
| grafana-alertcheck/internal/gate/testdata/state_zerotime_unpaused.json | State-endpoint fixture: invalid zero-time when unpaused |
| grafana-alertcheck/internal/gate/testdata/state_unknown_state.json | State-endpoint fixture: unknown instance state |
| grafana-alertcheck/internal/gate/testdata/state_reason_composite.json | State-endpoint fixture: composite reason parsing |
| grafana-alertcheck/internal/gate/testdata/state_paused.json | State-endpoint fixture: paused rule shape |
| grafana-alertcheck/internal/gate/testdata/state_only_active_instances.json | State-endpoint fixture: totals/instances mismatch |
| grafana-alertcheck/internal/gate/testdata/state_one_instance.json | State-endpoint fixture: canonical single instance |
| grafana-alertcheck/internal/gate/testdata/state_missing_state.json | State-endpoint fixture: missing required rule state |
| grafana-alertcheck/internal/gate/testdata/state_missing_optional.json | State-endpoint fixture: optional fields absent |
| grafana-alertcheck/internal/gate/testdata/state_missing_name.json | State-endpoint fixture: missing group name |
| grafana-alertcheck/internal/gate/testdata/state_missing_lasteval.json | State-endpoint fixture: missing lastEvaluation |
| grafana-alertcheck/internal/gate/testdata/state_missing_interval.json | State-endpoint fixture: missing group interval |
| grafana-alertcheck/internal/gate/testdata/state_missing_health.json | State-endpoint fixture: missing health |
| grafana-alertcheck/internal/gate/testdata/state_missing_file.json | State-endpoint fixture: missing group file/folder |
| grafana-alertcheck/internal/gate/testdata/state_health_nodata.json | State-endpoint fixture: nodata health case |
| grafana-alertcheck/internal/gate/testdata/state_health_error.json | State-endpoint fixture: error health case |
| grafana-alertcheck/internal/gate/testdata/ruler_rules.json | Ruler-endpoint fixture: grafana-managed rules and collisions |
| grafana-alertcheck/internal/gate/testdata/ruler_recording.json | Ruler-endpoint fixture: recording-rule shape |
| grafana-alertcheck/internal/gate/testdata/ruler_datasource_managed.json | Ruler-endpoint fixture: datasource-managed rule shape |
| grafana-alertcheck/internal/gate/testdata/README.md | Fixture provenance and expectations |
| grafana-alertcheck/internal/gate/source.go | Production HTTP source, retries/backoff, skew and body limits |
| grafana-alertcheck/internal/gate/source_fake_test.go | Fake clock/source test doubles |
| grafana-alertcheck/internal/gate/resolve.go | Operator name → rule definition resolution rules |
| grafana-alertcheck/internal/gate/resolve_test.go | Resolve behavior and edge-case tests |
| grafana-alertcheck/internal/gate/parse_state.go | Strict parsing of state endpoint + instance normalization |
| grafana-alertcheck/internal/gate/parse_state_test.go | Parser regression suite + high-cardinality synth test |
| grafana-alertcheck/internal/gate/parse_ruler.go | Strict parsing of ruler endpoint + kind classification |
| grafana-alertcheck/internal/gate/parse_ruler_test.go | Ruler parser tests covering kinds and fixtures |
| grafana-alertcheck/internal/gate/jsonreq.go | Required/optional JSON field decoding helpers |
| grafana-alertcheck/internal/gate/jsonreq_test.go | jsonreq helper tests |
| grafana-alertcheck/internal/gate/gate.go | Package doc stub |
| grafana-alertcheck/internal/gate/flock.go | File-lock helpers for writer presence/probing |
| grafana-alertcheck/internal/gate/flock_test.go | Lock contention classification test |
| grafana-alertcheck/internal/gate/duration.go | Prometheus/Grafana duration parser (d,w,y) |
| grafana-alertcheck/internal/gate/duration_test.go | Duration parser tests |
| grafana-alertcheck/internal/gate/coverage.go | Pure coverage proof logic and unobservable reasons |
| grafana-alertcheck/internal/gate/check_process.go | Recorder signalling helper |
| grafana-alertcheck/go.sum | Module dependency sums |
| grafana-alertcheck/go.mod | New module definition and dependencies |
| grafana-alertcheck/docs/reference/log-format.md | JSONL log schema documentation |
| grafana-alertcheck/docs/reference/cli.md | CLI flag and behavior reference |
| grafana-alertcheck/docs/reference/category.yaml | Docs category metadata (reference) |
| grafana-alertcheck/docs/index.md | Docs overview page |
| grafana-alertcheck/docs/how-alerts-are-evaluated.md | Evaluation/verdict model docs |
| grafana-alertcheck/docs/architecture.md | Invariants + design-seam documentation |
| grafana-alertcheck/docs/advanced.md | Scheduling/budget rationale docs |
| grafana-alertcheck/docs/category.yaml | Docs category metadata (top level) |
| grafana-alertcheck/cmd/watch.go | watch CLI implementation + daemon-child dispatch |
| grafana-alertcheck/cmd/watch_test.go | watch CLI flag-validation tests |
| grafana-alertcheck/cmd/table.go | Human table renderer |
| grafana-alertcheck/cmd/table_test.go | Golden-ish table rendering test |
| grafana-alertcheck/cmd/style.go | ANSI styling and note stream formatting |
| grafana-alertcheck/cmd/main.go | CLI entry point and subcommand dispatch |
| grafana-alertcheck/cmd/main_test.go | Main dispatch tests |
| grafana-alertcheck/cmd/list.go | list CLI implementation |
| grafana-alertcheck/cmd/list_test.go | list CLI tests using httptest server |
| grafana-alertcheck/cmd/env.go | Env-only Grafana connection config |
| grafana-alertcheck/cmd/common.go | Shared flag parsing + --states / --preexisting parsing |
| grafana-alertcheck/cmd/check.go | check CLI implementation and exit code mapping |
| grafana-alertcheck/cmd/check_test.go | check CLI validation + exit code tests |
| grafana-alertcheck/.tool-versions | Tool version pins for module dev/CI |
| .gitignore | Ignore rule adjustments (incl. tmp) |
| .github/workflows/test.yaml | Adds grafana-alertcheck to workflow test matrix |
Review details
- Files reviewed: 75/77 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+37
to
+44
| switch err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); { | ||
| case err == nil: | ||
| return true, nil | ||
| case errors.Is(err, syscall.EWOULDBLOCK): | ||
| return false, nil | ||
| default: | ||
| return false, fmt.Errorf("flock %s: %w", f.Name(), err) | ||
| } |
Comment on lines
+279
to
+290
| b, readErr := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) | ||
| tBodyRead := s.clock.Now() | ||
| if readErr != nil { | ||
| return requestResult{}, &TransportError{Err: fmt.Errorf("read response body (status %d): %w", resp.StatusCode, readErr)} | ||
| } | ||
| if len(b) >= maxResponseBytes { | ||
| // Hard error, never retried: a response this large is a stable | ||
| // property of the server's reply, not a transient network hiccup, so | ||
| // retrying would just reallocate the same bounded-but-pointless body. | ||
| return requestResult{}, fmt.Errorf("response body exceeded %d bytes", maxResponseBytes) | ||
| } | ||
| latency := tBodyRead.Sub(tSend) |
| | ---- | ------- | | ||
| | `HighErrorRate` | Title only, scoped by `--folder` | | ||
| | `Platform/HighErrorRate` | Folder + title | | ||
| | `Platform/api/HighErrorRate` | Folder + group + title (always unique) | |
| - `abnormal` holds only instances whose **canonical** state is not `normal`. | ||
| - `cleared`/`vanished` are instance keys that left the bad set, resolved against the same response: `cleared` = a real recovery; `vanished` = a discontinuity, never a recovery. | ||
|
|
||
| Instance keys are a sorted `k=v\n` join of labels, so they correlate across polls without hashing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes the stacked PRs: #2778