feat(alerts): alert when a rule's window has no data - #1208
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (28)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Maple review🟢 Confidence 4/5 · likely safe to merge Adds an opt-in
What was checked
|
2504a7a to
2ecfd03
Compare
|
Note A newer push replaced |
2ecfd03 to
042af39
Compare
|
Note A newer push replaced |
A window with no data was always skipped (bar low-throughput rules, which read it as zero), so a rule whose query stopped matching, typically a raw SQL rule, went quiet with nothing to say it was blind. Rules take an opt-in alertOnNoData (alert_on_no_data on the v2 API and the MCP create/update tools, a switch under Evaluation timing in the app). It compiles to noDataBehavior "alert": an empty window evaluates as a breach with no value, so it opens and resolves an incident through the usual breach and healthy counts. Notifications read "No data" where the value would be. The stored no_data_behavior column already held the compiled value, so no migration is needed. get_alert_rule shows the behavior, the v2 rule exposes alert_on_no_data, and the IaC AlertRule resource accepts it. The alert rule docs now cover the samples column, the new switch, and how skipped checks report why.
…rules - alertOnNoData now wins over the low-throughput zero read. Read as zero, an empty window was skipped under a minimum sample count instead of breaching, and the flag read back as off on every rebuild (edits, the v2 rule, IaC drift). - Grouped rules reject it: an empty result breached under the engine's "all" key, never a real group, and a group that stops reporting is already held by its incident's telemetry check. The app disables the switch on grouped rules. - The preview treats a group's missing windows as skips, as the scheduler never evaluates them, and charts the empty-result series where nothing reported. - Changing the no-data behavior resolves open incidents, the testRule fallback goes through applyEvaluationLogic, the summary line reads "has no data" in place of "n/a", alert_on_no_data is in the audit diff, and the Electric alert rows read an unknown behavior as skip.
042af39 to
cd80cc0
Compare
Maple review🟡 Confidence 3/5 · needs attention Adds an opt-in alert-on-no-data mode that treats an empty window as a breach, wired through the scheduler, preview, v2 API, MCP tools and the rule form, with grouped rules rejected. The evaluation logic and tests hold up; the new
Findings🟠 Warning · F1 ·
|
| Change | Kind | Observable | Evidence |
|---|---|---|---|
no-data breach in runSchedulerTick |
background | yes | runs inside the existing scheduler span and incident/delivery metrics; no new call, log or entrypoint added |
cd80cc0 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
| * What an empty window evaluates to: `skip` the check, read it as `zero`, or | ||
| * `alert`, which counts it as a breach so a rule that goes blind opens an incident. | ||
| */ | ||
| export const QueryEngineNoDataBehavior = Schema.Literals(["skip", "zero", "alert"]).annotate({ |
There was a problem hiding this comment.
Warning
no_data_behavior: "alert" on the v2 wire breaks older generated clients
F1 · Warning · correctness
no_data_behavior is serialized from this literal, so the first rule saved with alert_on_no_data returns "alert" — a value the checked-in spec has no enum case for in clients generated before this PR. HomeModel.swift:201 and IncidentsListView.swift:31 decode the rule page with try? await rules.items, so one such rule fails the whole page and every incident card loses its rule name until the app is rebuilt from the new spec. Keep the wire value in {skip, zero} for this field (map alert to skip in toV2Rule at apps/api/src/routes/v2/alert-rules.http.ts:89) and let alert_on_no_data carry the new state, or expose the new mode behind a versioned field.
Decouple the wire contract from the stored mode: emit `skip`/`zero` in `no_data_behavior` and report alert-on-no-data through `alert_on_no_data` only, so shipped clients keep decoding rules.
🤖 Prompt to fix with an AI agent
In `packages/domain/src/query-engine.ts:711`: `no_data_behavior: "alert"` on the v2 wire breaks older generated clients.
`no_data_behavior` is serialized from this literal, so the first rule saved with `alert_on_no_data` returns `"alert"` — a value the checked-in spec has no enum case for in clients generated before this PR. `HomeModel.swift:201` and `IncidentsListView.swift:31` decode the rule page with `try? await rules.items`, so one such rule fails the whole page and every incident card loses its rule name until the app is rebuilt from the new spec. Keep the wire value in `{skip, zero}` for this field (map `alert` to `skip` in `toV2Rule` at `apps/api/src/routes/v2/alert-rules.http.ts:89`) and let `alert_on_no_data` carry the new state, or expose the new mode behind a versioned field.
Suggested fix: Decouple the wire contract from the stored mode: emit `skip`/`zero` in `no_data_behavior` and report alert-on-no-data through `alert_on_no_data` only, so shipped clients keep decoding rules.
Verify the problem exists at that location before changing it, and keep the fix to those lines.
1 participant
Stack 4/5. An empty window was always skipped (except throughput
</<=, which reads it as zero), so a rule whose query stopped matching went silent.Rules take an opt-in
alertOnNoData(alert_on_no_dataon v2 and the MCP create/update tools, a switch under Evaluation timing in the app; default off). It compiles tonoDataBehavior: "alert": an empty window evaluates as a breach with no value and opens/resolves an incident through the usual counters.It wins over the low-throughput zero read, which a minimum sample count would otherwise skip.
Grouped rules reject it (the app disables the switch): an empty result has no real group to breach under, and a group that stops reporting is already held by its incident's telemetry check. On a raw SQL rule with a group column it fires only when the query returns no rows at all, and the preview models exactly that.
Changing the behavior resolves open incidents. Notifications read "has no data over the last 5m" where the value would be.
no_data_behaviorwas already a stored compiled column, so no DB migration. v2 rules exposealert_on_no_data(so the IaCAlertRuleresource can declare it without drift) and it is in the audit diff.Alert rule docs cover the
samplescolumn, the switch, and skip reasons.Heads-up:
no_data_behaviorgains the enum valuealert. iOS builds generated from the previous spec will fail to decode a rule using it. The app reads rules throughtry?, so it degrades to missing rule names, and the spec and fixture are regenerated here.Test: alerting-core,
AlertsService.test.ts(empty warehouse opens an incident; throughput precedence; grouped rejection; grouped raw preview), summary line, v2 contract, alert-tools, web alerts libs and collection, alchemy contract.Summary by CodeRabbit
samplescolumn to report sample counts. Without one, each row counts as one sample.