fix: surface silent alert, parity, and preflight migration gaps - #367
fix: surface silent alert, parity, and preflight migration gaps#367shmsr wants to merge 1 commit into
Conversation
Consultant review of the Instruqt/training demos found engine issues that looked like success: empty Kibana alert actions, unbounded SHAPE_PASS, silent Grafana stat-grid collapse, unresolved datasource template variables, duplicated alert rules on re-run, and a generic LogQL alert reason. Make each of those operator-visible or bounded, and document the remaining policy gaps.
There was a problem hiding this comment.
Pull request overview
Surfaces previously silent migration gaps across alerting, parity verification, Grafana status grids, and datasource preflight.
Changes:
- Adds alert action-gap reporting and creation idempotency.
- Bounds
SHAPE_PASSand preserves stat/gauge series grouping. - Resolves datasource variables and updates operator documentation and skills.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_grafana_preflight.py |
Tests datasource-variable auditing. |
tests/test_grafana_multi_target_summary.py |
Tests status-grid grouping and warnings. |
tests/test_alert_offline_gate.py |
Tests empty-action classification. |
tests/test_alert_migration.py |
Tests alert losses, LogQL reasoning, and idempotency. |
tests/core/test_parity_oracle.py |
Tests the parity error ceiling. |
scripts/parity_promql_esql_oracle.py |
Bounds standalone parity verdicts. |
parity-rig/README.md |
Documents bounded parity verdicts. |
parity-rig/harness/parity.py |
Bounds harness SHAPE_PASS. |
observability_migration/targets/kibana/alerting.py |
Adds rule deduplication and warnings. |
observability_migration/core/verification/parity_oracle.py |
Enforces the parity ceiling. |
observability_migration/core/verification/alert_offline_gate.py |
Classifies empty actions as a configuration gap. |
observability_migration/core/mapping.py |
Tags and records action gaps and LogQL policy. |
observability_migration/adapters/source/grafana/translate.py |
Warns about summary-series collapse. |
observability_migration/adapters/source/grafana/promql.py |
Adds equivalent PromQL warnings. |
observability_migration/adapters/source/grafana/preflight.py |
Audits resolved and unresolved datasource variables. |
observability_migration/adapters/source/grafana/panels.py |
Preserves status-grid legend grouping. |
observability_migration/adapters/source/grafana/manifest.py |
Inventories datasource variables. |
observability_migration/adapters/source/grafana/cli.py |
Reports unresolved datasource variables. |
docs/testing.md |
Documents empty-action gate behavior. |
docs/sources/grafana.md |
Documents Grafana migration gaps. |
docs/command-contract.md |
Updates alert and parity command contracts. |
.cursor/skills/validate-side-by-side/SKILL.md |
Updates parity guidance. |
.cursor/skills/scan-o11y-environment/SKILL.md |
Adds datasource-variable guidance. |
.cursor/skills/review-and-enable-migrated-alerts/SKILL.md |
Strengthens connector review guidance. |
.cursor/skills/explain-migration-gaps/SKILL.md |
Adds status-grid guidance. |
.cursor/skills/assess-migration-readiness/SKILL.md |
Adds unresolved-variable blockers. |
.claude/skills/validate-side-by-side/SKILL.md |
Mirrors parity guidance. |
.claude/skills/scan-o11y-environment/SKILL.md |
Mirrors datasource guidance. |
.claude/skills/review-and-enable-migrated-alerts/SKILL.md |
Mirrors alert guidance. |
.claude/skills/explain-migration-gaps/SKILL.md |
Mirrors status-grid guidance. |
.claude/skills/assess-migration-readiness/SKILL.md |
Mirrors readiness guidance. |
.agents/skills/validate-side-by-side/SKILL.md |
Mirrors parity guidance. |
.agents/skills/scan-o11y-environment/SKILL.md |
Mirrors datasource guidance. |
.agents/skills/review-and-enable-migrated-alerts/SKILL.md |
Mirrors alert guidance. |
.agents/skills/explain-migration-gaps/SKILL.md |
Mirrors status-grid guidance. |
.agents/skills/assess-migration-readiness/SKILL.md |
Mirrors readiness guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if resolved_type: | ||
| ds_type = resolved_type | ||
| key = f"{ds_type}:{ds_name}" | ||
| ds_counter[key] += 1 | ||
| ds_panels[key] = ds_panels.get(key, 0) + 1 |
| for rule in collect_migrated_rules( | ||
| _list_all_rules( | ||
| kibana_url, | ||
| api_key=api_key, | ||
| space_id=space_id, | ||
| timeout=timeout, | ||
| verify=verify, | ||
| list_rules_fn=list_rules_fn, | ||
| ) | ||
| ): | ||
| name = str(rule.get("name") or "") | ||
| if name and name not in existing_by_name: | ||
| existing_by_name[name] = rule |
| f"WARNING: {empty_actions_created} created rule(s) have empty actions; " | ||
| "Grafana notification policies are not mapped onto Kibana connectors. " | ||
| "Enabling them evaluates and notifies nobody.", |
| - **`0`** — otherwise (including runs where every row is `STRUCTURAL`, `SOURCE_DRIFT`, `SKIP`, or non-`FAIL` numeric verdicts). | ||
|
|
||
| Besides **`FAIL`** / **`SOURCE_FAIL`** (which set exit `1`), verdicts **`ERROR`**, **`SKIP`**, **`SHAPE_PASS`**, and **`SOURCE_DRIFT`** do not fail the run but still warrant a look — route them to **`explain-migration-gaps`** or re-check `--window-minutes` / `--step-seconds` / target telemetry before trusting an all-green exit code. | ||
| Besides **`FAIL`** / **`SOURCE_FAIL`** (which set exit `1`), verdicts **`ERROR`**, **`SKIP`**, **`SHAPE_PASS`**, and **`SOURCE_DRIFT`** do not fail the run but still warrant a look — route them to **`explain-migration-gaps`** or re-check `--window-minutes` / `--step-seconds` / target telemetry before trusting an all-green exit code. **`SHAPE_PASS` is bounded at 25% relative error; above that the row is `FAIL`.** Still read `max_relative_error` on every `SHAPE_PASS` row — a 20% miss is a pass-shaped name, not numeric proof. |
| - **`0`** — otherwise (including runs where every row is `STRUCTURAL`, `SOURCE_DRIFT`, `SKIP`, or non-`FAIL` numeric verdicts). | ||
|
|
||
| Besides **`FAIL`** / **`SOURCE_FAIL`** (which set exit `1`), verdicts **`ERROR`**, **`SKIP`**, **`SHAPE_PASS`**, and **`SOURCE_DRIFT`** do not fail the run but still warrant a look — route them to **`explain-migration-gaps`** or re-check `--window-minutes` / `--step-seconds` / target telemetry before trusting an all-green exit code. | ||
| Besides **`FAIL`** / **`SOURCE_FAIL`** (which set exit `1`), verdicts **`ERROR`**, **`SKIP`**, **`SHAPE_PASS`**, and **`SOURCE_DRIFT`** do not fail the run but still warrant a look — route them to **`explain-migration-gaps`** or re-check `--window-minutes` / `--step-seconds` / target telemetry before trusting an all-green exit code. **`SHAPE_PASS` is bounded at 25% relative error; above that the row is `FAIL`.** Still read `max_relative_error` on every `SHAPE_PASS` row — a 20% miss is a pass-shaped name, not numeric proof. |
| _list_all_rules( | ||
| kibana_url, | ||
| api_key=api_key, | ||
| space_id=space_id, | ||
| timeout=timeout, | ||
| verify=verify, | ||
| list_rules_fn=list_rules_fn, | ||
| ) |
| - **`0`** — otherwise (including runs where every row is `STRUCTURAL`, `SOURCE_DRIFT`, `SKIP`, or non-`FAIL` numeric verdicts). | ||
|
|
||
| Besides **`FAIL`** / **`SOURCE_FAIL`** (which set exit `1`), verdicts **`ERROR`**, **`SKIP`**, **`SHAPE_PASS`**, and **`SOURCE_DRIFT`** do not fail the run but still warrant a look — route them to **`explain-migration-gaps`** or re-check `--window-minutes` / `--step-seconds` / target telemetry before trusting an all-green exit code. | ||
| Besides **`FAIL`** / **`SOURCE_FAIL`** (which set exit `1`), verdicts **`ERROR`**, **`SKIP`**, **`SHAPE_PASS`**, and **`SOURCE_DRIFT`** do not fail the run but still warrant a look — route them to **`explain-migration-gaps`** or re-check `--window-minutes` / `--step-seconds` / target telemetry before trusting an all-green exit code. **`SHAPE_PASS` is bounded at 25% relative error; above that the row is `FAIL`.** Still read `max_relative_error` on every `SHAPE_PASS` row — a 20% miss is a pass-shaped name, not numeric proof. |
giorgi-imerlishvili-elastic
left a comment
giorgi-imerlishvili-elastic
left a comment
There was a problem hiding this comment.
Thanks for covering the silent-gap cases and adding focused tests. I cannot approve this yet because several parts of the stated contract still fail on the PR head:
-
Unresolved datasource variables do not always become blockers.
build_datasource_audit()lowercasesdatasource_typebefore storing the unresolved token, then recounts against the original case-sensitive fields. Withdatasource_type="${DS_INFLUX}", the report containsunresolved_datasource_variables, butunresolved_datasource_panelsis0;_gate_panels()therefore adds no blocker. Count unresolved panels in the first pass using normalized variable names (or gate directly on unresolved entries), and add this representation to the tests. -
Resolved datasource panels are counted twice. A single
${datasource}panel resolving to InfluxDB currently reports bothunknown: 1andinfluxdb: 1, so datasource totals no longer match panel counts. Please resolve before incrementing the effective datasource counters and assert exact totals, as noted in this thread. -
Alert creation is not safely idempotent. The lookup uses
collect_migrated_rules(), whose name-prefix-or-tag predicate skips an unrelated, untagged user rule named[migrated] CPU high, contrary to the documented same-name-plus-tag contract (thread). Conversely, a listing error is converted to an empty list and creation proceeds, so a transient/auth failure can duplicate every migrated rule (thread). Use a creation-specific exact-name + marker-tag predicate and fail closed when the existing-rule inventory cannot be completed; cover both cases. -
The status-grid grouping can change source query semantics and still collapse series. On this head, a stat target
sum(rate(http_requests_total[5m]))withlegendFormat: {{job}}becomes a grouped query... BY service.name, widening a scalar source expression into multiple values. BareupbecomesAVG(LAST_OVER_TIME(up)) BY service.name, which merges multiple instances sharing a job even though Grafana renders one tile per original series. Legend text alone is not a complete series identity. Preserve the source expression's grouping/cardinality (or surface an honest unsupported/manual result), and add tests for outer aggregations and multiple instances with the same legend label. -
The 25% parity boundary is not stable. A mathematically exact 25% case (
0.3vs0.4) computes as0.25000000000000006and returnsFAIL, although the description and docs say only values above 25% fail. Use a tolerant/ULP-aware threshold consistently in core, harness, and script, with exact-boundary and just-over-boundary tests. -
Operator guidance still contradicts runtime behavior. All three
validate-side-by-sideskill mirrors sayERRORdoes not fail, while_run_comparereturns exit 1 forERROR; the existing mirror comments should be fixed together (cursor thread). Also make the shared empty-actions warning source-neutral because Datadog uses this path too (thread).
I ran the five focused changed-area test files at commit 61ba4fc: 489 passed, 130 subtests passed. Those tests are green, but they do not cover the cases above.
Summary
Consultant feedback from the Instruqt lab vs internal training review found several engine issues that looked like success. This PR makes each of those operator-visible or bounded. It does not invent Grafana notification-policy → Kibana connector mapping, and it does not auto-emit LogQL as Kibana rules.
--create-alert-rulesstill creates disabled rules (correct), but Grafana notification policies are not mapped onto Kibana connectors. Emitted rules are now taggedobs-migration-no-actions, the mapping records the loss, create prints a stderr warning, and the offline gate classifies emptyactionsasconfig_gaprather than success.SHAPE_PASSceiling: overlapping series with relative error above 25% is nowFAIL. Readmax_relative_error; do not treat the verdict name as numeric proof. Exit0still includes boundedSHAPE_PASS.stat/gaugetiles are one-per-series.legendFormatplaceholders such as{{job}}are kept as grouping so Target-health panels do not silently collapse into oneMAX()/LAST()scalar. Ungroupedupmigrates with a warning.$datasource/${DS_*}are resolved fromtemplating.list. InfluxDB (and other non-migratable types) behind a template variable land innon_migratable. Unresolved variables are recorded inunresolved_datasource_variablesand treated as blockers even whennon_migratableis empty.--create-alert-rulesskips existing[migrated]names taggedobs-migration(already_exists) instead of duplicating disabled rules.manual_requiredby policy (higher confidence bar than dashboard panels). The mapping reason now says so explicitly instead of a generic "no source-faithful query" message.Operator docs and the
.claude/.cursor/.agentsskill mirrors are updated in lockstep.Validation
.venv/bin/python -m pytest tests/— 5905 passed, 3 skipped (used--override-ini="addopts="so-xin pyproject did not stop the full sweep)Checklist