Skip to content

feat(alerts): record why a check was skipped - #1206

Merged
Makisuo merged 2 commits into
fix/alert-raw-sql-samples-docsfrom
fix/alert-check-skip-reasons
Oct 2, 2026
Merged

Makisuo merged 2 commits into
fix/alert-raw-sql-samples-docsfrom
fix/alert-check-skip-reasons

Conversation

@Makisuo

@Makisuo Makisuo commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Stack 2/5. A skipped check looked the same whether the query returned no rows, fell below the minimum sample count, or produced no scalar, and it sat next to healthy checks.

  • alerting-core sets skipReason (no_data, below_min_samples, no_value) on every skip, replacing the skippedForNoData flag.
  • The scheduler writes it to a new alert_checks.SkipReason column: ClickHouse migration 0036, local schema v27, Tinybird manifest regenerated.
  • Exposed as skipReason on check documents, skip_reason on the v2 checks API, and in list_alert_checks, whose summary reads 5 skipped [4 no data, 1 below min samples] and whose value column says why.
  • The rule diagnosis and the chart rail tell the two apart.

Rows recorded before the deploy have no reason (null).

Test: alerting-core, AlertsService.test.ts (new skip-reason case), AlertReadModelsService.test.ts, migrations index, local-store migrations (bun), alert-tools, v2 alerts.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Summary by CodeRabbit

  • New Features
    • Alert checks now report why they were skipped, distinguishing missing data, insufficient samples, and unavailable values.
    • Alert history, charts, diagnostic summaries, and check-list results display skipped-check reasons and relevant counts.
    • API responses include the skip reason for each alert check.
  • Bug Fixes
    • Alert evaluations with no data are now distinguished from checks skipped for other reasons, improving diagnostic guidance.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 24bca2f3-4893-4f8a-b5f8-04aa1960810a

📥 Commits

Reviewing files that changed from the base of the PR and between ff972f2 and 400d5a9.

⛔ Files ignored due to path filters (2)
  • packages/domain/src/generated/clickhouse-schema.ts is excluded by !**/generated/**
  • packages/domain/src/generated/tinybird-project-manifest.ts is excluded by !**/generated/**
📒 Files selected for processing (35)
  • apps/ai/src/mcp/tools/__tests__/alert-tools.test.ts
  • apps/ai/src/mcp/tools/list-alert-checks.ts
  • apps/api/src/routes/v2/alert-rules.http.ts
  • apps/api/src/routes/v2/alerts.http.test.ts
  • apps/cli/src/server/local-schema-history.ts
  • apps/cli/src/server/local-schema-version.ts
  • apps/cli/src/server/local-store-migrations/steps.ts
  • apps/cli/src/server/schema-identity.ts
  • apps/cli/src/server/schema/local-inserts.json
  • apps/cli/src/server/schema/local-schema-v27.sql
  • apps/cli/src/server/schema/local-schema.sql
  • apps/cli/test/local-store-migrations.test.ts
  • apps/cli/test/native-local-store-migration.sh
  • apps/ingest/src/clickhouse_insert_mappings.rs
  • apps/ios/Packages/MapleAPI/Sources/MapleAPI/openapi.json
  • apps/web/src/components/alerts/alert-rule-chart.browser.test.tsx
  • apps/web/src/components/alerts/alert-rule-chart.tsx
  • apps/web/src/lib/alerts/diagnosis.ts
  • apps/web/src/lib/alerts/form-utils.test.ts
  • apps/web/src/lib/alerts/form-utils.ts
  • apps/web/src/routes/alerts/$ruleId.tsx
  • packages/alerting-core/src/index.test.ts
  • packages/alerting-core/src/index.ts
  • packages/backend/src/services/alerts/AlertReadModelsService.ts
  • packages/backend/src/services/alerts/AlertsService.test.ts
  • packages/backend/src/services/alerts/AlertsService.ts
  • packages/domain/src/clickhouse/migrations/0036_alert_checks_skip_reason.ts
  • packages/domain/src/clickhouse/migrations/index.test.ts
  • packages/domain/src/clickhouse/migrations/index.ts
  • packages/domain/src/http/alerts.ts
  • packages/domain/src/http/v2/alert-rules.ts
  • packages/domain/src/mcp-outputs/alerts.ts
  • packages/domain/src/tinybird/datasources.ts
  • packages/query-engine/src/ch/queries/alert-checks.ts
  • packages/query-engine/src/ch/tables.ts
 _________________________________________
< Schrodinbug: works until I stare at it. >
 -----------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@maple-review-bot

maple-review-bot Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Maple review

🟡 Confidence 3/5 · needs attention
quality 100/100 · no findings · tests covered · risk medium

Warning

This review ended early; what follows is what it established.

A skipped alert check now records why it skipped (no_data, below_min_samples, no_value) in a new alert_checks.SkipReason column, plumbed through the query engine, v2 API, MCP tool and web UI. The change is additive and safe to merge. Two files (the v2 alerts HTTP test and the generated 2136-line local-schema-v27.sql) were not read before the pass ended.

  • evaluateAlertObservation now sets skipReason, replacing the skippedForNoData flag
  • ClickHouse migration 0036 and local schema v27 add alert_checks.SkipReason
  • list_alert_checks splits the skipped count by reason and labels the value column
  • v2 checks API and AlertCheckDocument expose skip_reason/skipReason
What was checked
  • Every previous skippedForNoData read is updated, not lost: planAlertLifecycle and AlertsService.ts:2032 test skipReason === "no_data", preserving the old gate semantics
  • Legacy rows degrade to null: migration 0036 uses DEFAULT '' and AlertReadModelsService.ts:333 maps ''/absent to null
  • All skip branches of evaluateAlertObservation and the synthetic no-data evaluations set a reason matching the AlertSkipReason literals
Files not reviewed (2)

The review ended before it read these diffs, so nothing above vouches for them.

  • apps/api/src/routes/v2/alerts.http.test.ts
  • apps/cli/src/server/schema/local-schema-v27.sql

e7894d9 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.

A skipped check read the same whether the query returned no rows, fell
below the minimum sample count, or produced no scalar, and next to healthy
checks it was easy to read a blind rule as a quiet one.

alerting-core now sets a skipReason (no_data, below_min_samples, no_value)
on every skip, replacing the skippedForNoData flag. The scheduler writes it
to a new alert_checks.SkipReason column (ClickHouse migration 0036, local
schema v27), and it is exposed as skipReason on check documents, skip_reason
on the v2 API, and in list_alert_checks, whose summary and value column now
say "no data" or "below min samples". The rule diagnosis and chart rail tell
the two apart too.
…none

Place alert_checks.SkipReason after the existing columns, where migration
0036's ADD COLUMN puts it, so the Tinybird datasource and every ClickHouse
copy agree on column order (local schema v27 regenerated). A skip reason
this build does not recognise now reads as null instead of failing the
whole checks page.
@Makisuo
Makisuo force-pushed the fix/alert-check-skip-reasons branch from e7894d9 to 6a90ec1 Compare October 2, 2026 17:48
@maple-review-bot

maple-review-bot Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Note

A newer push replaced 6a90ec1 before its review finished. The latest commit is reviewed in a new comment.

@Makisuo
Makisuo added this pull request to stack #1210 October 2, 2026 17:50
@maple-review-bot

maple-review-bot Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Note

A newer push replaced 400d5a9 before its review finished. The latest commit is reviewed in a new comment.

@Makisuo
Makisuo force-pushed the fix/alert-check-skip-reasons branch from 400d5a9 to 6a90ec1 Compare October 2, 2026 17:56
@Makisuo
Makisuo merged commit 27272b1 into main Oct 2, 2026
57 of 87 checks passed
@Makisuo
Makisuo deleted the fix/alert-check-skip-reasons branch October 2, 2026 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant