refactor(alerting): unify alert compilation and evaluation - #278
Conversation
Alert evaluation had four near-identical implementations of "lower a plan to observations": computeEvaluateBuckets and makeQueryEngineEvaluate held byte-equivalent traces/logs/metrics blocks, makeQueryEngineEvaluateSeries was a third wrapper, and makeQueryEngineEvaluateRawSql a fourth copy of the byGroup/reduce tail. Any per-source fix had to be made twice. Collapse them into computeAlertBuckets (the single lowering, four arms: traces/logs/metrics/raw_sql) plus reduceAlertBuckets (the single reduce tail, including the no-data fallback). evaluate, evaluateSeries, evaluateRawSql and the bucket-cache callback are now thin derivations. Net -20 lines while adding a whole new raw-SQL bucket path. Raw SQL stops being a second-class source: - New $__timeGroup(col) macro expands to toStartOfInterval(col, INTERVAL <granularity> SECOND), so a raw alert can return one row per evaluation window. Without it the range collapses into one synthetic bucket, which reduces to exactly the same scalar as before — scheduler behavior is unchanged either way. This is what makes raw-SQL preview possible. - The raw-SQL guards (group count, key length, sample validity) moved into the shared arm, so they now cover the preview path too. - Reject NUL in raw group keys: encodeEvalPoints uses NUL-delimited prefixes and relies on keys never containing NUL, which holds for CH-derived keys but not for arbitrary user SQL. Two deliberate deltas, both on rows that were already skipped: a raw row with value: null now reports sampleCount 0 (was 1) so `hasData === sampleCount > 0` holds uniformly — the invariant the bucket codec assumes; only the skip reason string changes. A row with a non-null value and an explicit samples: 0 now reads as no-data. Verified against the existing parity harness unchanged (QueryEngineEvaluateCache.test.ts), plus 5 new tests asserting raw SQL and a spec query produce byte-identical output for the same shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Your Pullfrog Router balance is exhausted. You have a payment method on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer. Top up balance → · Enable auto-reload →
|
🍁 Maple PR previewNote Preview resources were removed when this pull request closed. Final commit |
Stage B1/B2 of the alerting simplification. Builds on the unified bucket producer: now the callers stop branching too. **One evaluate entry point.** `QueryEngineService.evaluateRawSql` is gone. `evaluate` and `evaluateSeries` take an `AlertEvaluateRequest` carrying an `AlertBucketSource` discriminator, so a trace built-in, a query-builder draft and user SQL are the same request. `AlertsService.evaluateRule` loses its `plan.kind === "raw_sql"` branch entirely — `planEvaluateSource` is now the only place a plan's kind is inspected on the evaluation path. Span annotation, validation and bucket sizing moved into a shared `prepareAlertEvaluation` so the two entry points can't drift on which queries they accept. **One group-key vocabulary.** The engine emits a generic "all"; storage uses "__total__", which is public (v2 wire, alert_checks.GroupKey, the Electric-synced web collection) and can't be renamed. `evaluateRule` is now the single translator and returns storage vocabulary only, so the scheduler no longer re-derives the key and every remaining literal is the new `UNGROUPED_GROUP_KEY` constant from @maple/domain. That fixes a live bug: `previewRule` emitted "all" for ungrouped rules while the scheduler stored "__total__", so the preview chart and the tracking chart keyed the same series differently. **Raw SQL previews now work.** They were rejected server-side even though the frontend offers the chart. Two things had to be right: - The synthetic bucket for an unbucketed raw query is `range.startTime`, a Tinybird datetime. Consumers key buckets with `Date.parse`, which reads that space-separated form as LOCAL time — so every point missed its bucket and the whole series came back no-data. It now goes through `normalizeBucket` like every other source. - That rejection was also, incidentally, the only thing stopping an `alerts:read` API key from executing arbitrary ClickHouse. Preview is an `alerts:read` endpoint; creating or testing a raw rule requires `alerts:write` + org-admin. Removing the rejection without a gate would have been a privilege escalation, so raw previews are now gated twice: by scope in the v2 route (API keys carry root roles, so scope is what separates a read-only key) and by org-admin role in `previewRule` (which covers the session path, where non-admin users exist). Tests: the raw-preview rejection test inverts into a positive preview test; the v2 test that asserted "cannot be previewed" becomes a real scope test (403 for alerts:read, no warehouse call) plus a positive alerts:write case. Verified: 139 alerting tests, @maple/query-engine 952, @maple/domain 364, typecheck clean across api/query-engine/domain/web. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Your Pullfrog Router balance is exhausted. You have a payment method on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer. Top up balance → · Enable auto-reload →
|
Makisuo
changed the title
`AlertRuleDocument.environments` became a required field in 2a18697 ("fix: some alert creation bugs"), but these four fixtures were never updated, so `decodeRuleDoc` failed with `SchemaError: Missing key at ["environments"]` and took 24 tests with it. Pre-existing on main and unrelated to this branch — the same TypeScript job fails on main's own CI run. Fixed here because it blocks this PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Your Pullfrog Router balance is exhausted. You have a payment method on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer. Top up balance → · Enable auto-reload →
|
`QueryEngineService.test.ts` drives `makeQueryEngineEvaluate` and the
now-removed `makeQueryEngineEvaluateRawSql` directly, and I missed it when
folding raw SQL into `evaluate` — CI caught it. The requests now carry an
`AlertBucketSource` (`{kind: "spec", query}` / `{kind: "raw_sql", sql,
windowMinutes}`) and the raw describe exercises the same `evaluate` as every
spec source, which is the whole point of the fold.
Typecheck did not catch this because these request literals are contextually
typed at the call site, so a stale `query:` key reads as an excess property
on a structurally-satisfied object rather than a missing `source`.
Also drops a stale `evaluateRawSql` stub from the telemetry route test's
QueryEngineServiceShape, and refreshes the CompiledAlertQueryPlan doc
comment that still pointed at the deleted method.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Your Pullfrog Router balance is exhausted. You have a payment method on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer. Top up balance → · Enable auto-reload →
|
Both jobs are red on main; neither is caused by the alerting refactor. Fixed here so the branch can go green. **Service Map Perf** — the failing assertion is `cursor.longTasks === 0`, not the render-work ratio. Across three retries the baseline (recharts) mode swung 2 → 10 → 12 long tasks while the cursor mode's blocking time stayed at or below it (15/26/124ms vs 23/63/133ms) and the React render reduction held at ~62%. The long-task COUNT is environmental on GitHub's GPU-less runners, so zero is unachievable there; the render-work ratio is what actually detects a sync-storm regression. Adopts the split logs.perf.spec.ts already uses for the same reason: strict zero-long-task gate locally, a blocking-time ceiling on CI that only rejects order-of-magnitude regressions (1s, ~8x the observed worst case). Applied to service-detail.perf.spec.ts too — identical assertion, identical latent flake. **Local archive native** — the probe read the part layout immediately after two INSERTs and reported `0 parts`, i.e. an empty table. Zero rather than one rules out a background merge and points at insert visibility: the endpoint returns once the write is accepted, which is not when the part becomes visible to SELECT. Confirmed flaky rather than commit-caused — it passed at 6c1e9ee and first failed at 6a059d8, which only touches apps/slack-agent. Polls until all 8 rows are queryable, then asserts the layout, so the check tests parts rather than timing. The `== 2` assertion stays strict: a real merge collapsing the two parts defeats the scenario this probe exists to cover and must still fail loudly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Your Pullfrog Router balance is exhausted. You have a payment method on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer. Top up balance → · Enable auto-reload →
|
The probe hardcoded RANGE_DATE=2026-06-29 and inserted its 8 spans at that timestamp. `traces` carries `TTL toDate(Timestamp) + INTERVAL 30 DAY` (local-schema.sql), so once real time reached 2026-07-29 the fixture was exactly 30 days old and every row expired the moment it was written: the INSERT returned 200, the table stayed empty, and the part count read 0. Not a flake — a time bomb. `Local archive native` passed every main run on 2026-07-28 (fixture 29 days old) and has failed every run since. My first attempt at this misread the empty table as an insert-visibility race and added a poll; that theory was wrong (the count stayed 0 for the full 10s window), so the poll is reverted. Derives the range date from today instead (yesterday — a completed day, well inside the window), handling both GNU and BSD `date`. Keeps a cheap row-count assertion before the part check so a future retention problem names itself instead of surfacing as a confusing part count. Note: apps/cli/test/native-archive-gc-probe.sh has the same hardcoded date and is therefore archiving an empty table today. It still passes because it never asserts its rows exist, so it is silently degraded rather than failing — worth fixing separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Your Pullfrog Router balance is exhausted. You have a payment method on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer. Top up balance → · Enable auto-reload →
|
Same 30-day TTL time bomb as the merge probe: RANGE_DATE was pinned to 2026-06-29, so since 2026-07-29 every fixture row expired on write and each generation archived an empty table. This was masked — the merge probe failed first and aborted the job before the GC step ran. With the merge probe fixed the job reaches this step, which fails at `paths_csv: unbound variable`: with no shards on disk the collection loop never runs, and `local paths_csv` left it undeclared for the `[ -n ... ]` under `set -u`. I flagged this as "silently degraded, fix separately" one commit ago; it is not separate, it is the same bug one step later. Derives the date from today like the merge probe, and initialises paths_csv so an empty archive reports a real assertion instead of crashing the shell. No hardcoded fixture dates remain in apps/cli/test/*.sh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Your Pullfrog Router balance is exhausted. You have a payment method on file but auto-reload is disabled, so runs paused once your balance went past the overdraft buffer. Top up balance → · Enable auto-reload →
|
1 participant
Stages A, B1 and B2 of the alerting simplification plan. No DB migration; one v1 service-signature change and one new (tightened) authorization check on raw-SQL preview.
Why
Alerting has three superficially different rule kinds — built-in trace signals, the query builder, raw ClickHouse SQL — that almost already share a pipeline. The real problem is that the compiled plan was incomplete, so everything it didn't carry got re-implemented per call site.
What changed
1. One bucket producer (Stage A)
Four near-identical implementations of "lower a plan to observations" collapsed into one:
computeEvaluateBucketscomputeAlertBuckets— the single lowering, four arms (traces/logs/metrics/raw_sql)makeQueryEngineEvaluate(same three blocks copy-pasted)computeAlertBuckets→reduceAlertBucketsmakeQueryEngineEvaluateSeriescomputeAlertBucketsmakeQueryEngineEvaluateRawSqlThe traces blocks in the first two were byte-equivalent modulo
request.queryvsquery; same for logs and metrics. Any per-source fix had to be made twice.2. One evaluate entry point (Stage B2)
QueryEngineService.evaluateRawSqlis gone.evaluate/evaluateSeriestake anAlertEvaluateRequestcarrying anAlertBucketSourcediscriminator, so all three rule kinds are the same request.AlertsService.evaluateRuleloses itsplan.kind === "raw_sql"branch —planEvaluateSourceis now the only place a plan's kind is inspected on the evaluation path. Annotation, validation and bucket sizing moved into a sharedprepareAlertEvaluationso the two entry points can't drift on what they accept.3. One group-key vocabulary (Stage B2)
The engine emits a generic
"all"; storage uses"__total__", which is public (v2 wire,alert_checks.GroupKey, the Electric-synced web collection) and cannot be renamed.evaluateRuleis now the single translator and returns storage vocabulary only; every remaining literal is the newUNGROUPED_GROUP_KEYconstant.This fixes a live bug:
previewRuleemitted"all"for ungrouped rules while the scheduler stored"__total__", so the preview chart and the tracking chart keyed the same series differently.4. Raw SQL stops being second-class
$__timeGroup(col)macro →toStartOfInterval(col, INTERVAL <granularity> SECOND), so a raw alert can return one row per evaluation window. Without it the range collapses into one synthetic bucket that reduces to the same scalar — scheduler behavior is unchanged either way.encodeEvalPointsuses NUL-delimited prefixes and relies on keys never containing NUL — true for CH-derived keys, not for arbitrary user SQL.Reviewer notes — two things worth your attention
① The raw-preview rejection was load-bearing for authorization. Preview is an
alerts:readendpoint; creating or testing a raw rule requiresalerts:write+ org-admin. So"Raw SQL alerts cannot be previewed"was, incidentally, the only thing stopping a read-only API key from executing arbitrary ClickHouse. Removing it without a gate would have been a privilege escalation.Raw previews are now gated twice, because roles and scopes protect different callers:
rootroles by default (apiKeyDefaultRoles), so scope is what actually separates a read-only key.previewRule— covers the session/app path, where non-admin users exist.The v2 test that asserted
"cannot be previewed"is now a real scope test (403 foralerts:read, zero warehouse calls) plus a positivealerts:writecase.② A timezone bug that would have silently broken every unbucketed raw preview. The synthetic bucket was
range.startTime, a Tinybird datetime (2026-01-01 00:00:00). Consumers key buckets withDate.parse, which reads that space-separated form as local time — so every point missed its bucket and the series came back entirely no-data. It now goes throughnormalizeBucketlike every other source. Caught by the inverted preview test.Two deliberate behavior deltas, both on rows that were already skipped:
value: nullreportssampleCount: 0(was 1), sohasData === sampleCount > 0holds uniformly — the invariant the bucket codec assumes. Status staysskipped; only the reason string changes.samples: 0now reads as no-data, consistent with the spec sources.Watch the metrics arm:
composeMetricsGroupKeywas called in both copies and must be called exactly once in the merged branch — agroupName || "all"slip would silently re-key every metrics-alert incident and orphan-resolve them. Group keys are asserted explicitly in the tests.Signature change:
AlertsService.previewRuletakesrolesas its second argument.Verification
AlertsService,QueryEngineEvaluateCache,AlertDeliveryDispatch,AiTriageService.alert, v2alerts.http,alerts-error-map,alert-templating/renderer,run-raw-sql,AlertDestinationHydration)@maple/query-engine952 passed ·@maple/domain364 passedtypecheckclean acrossapi,query-engine,domain,webQueryEngineEvaluateCache.test.tspassing unchanged was the gate for the Stage A refactor; 5 new tests assert raw SQL and a spec query produce byte-identical output for the same row shape.CI notes — three pre-existing failures, all fixed
All three were red on main before this branch (main's run); none were caused by the alerting refactor. All checks now pass.
TypeScript.
AlertRuleDocument.environmentsbecame required in2a1869722but four web fixtures were never updated, sodecodeRuleDocthrewSchemaError: Missing key at ["environments"], taking 24 tests with it. (Separately,QueryEngineService.test.tsneeded updating for the unified request — that one was mine, see the test commit.)Service Map Perf. The failing assertion was
cursor.longTasks === 0, not the render-work ratio. The long-task count is environmental on GitHub's GPU-less runners — the synchronized baseline itself swings 2 → 12 between attempts — while the cursor mode's blocking time stays at or below it (15/26/124ms vs 23/63/133ms) and the render reduction holds at ~62%. Adopts the splitlogs.perf.spec.tsalready uses for exactly this reason: strict zero-long-task gate locally, a blocking ceiling on CI that only rejects order-of-magnitude regressions. The render-work ratio — what actually detects a sync-storm regression — is untouched.Local archive native — a time bomb, not a flake. Both archive probes pinned
RANGE_DATE="2026-06-29"for their fixtures whiletracescarriesTTL toDate(Timestamp) + INTERVAL 30 DAY. On 2026-07-29 the fixture turned exactly 30 days old, so every row expired the moment it was written: the INSERT returned 200 and the table stayed empty.The dates confirm it: the job passed every main run on 07-28 (fixture 29 days old) and failed every run from 07-29. That is also why bisecting was misleading — it pointed at
6a059d8e, a Slack-only commit, because the trigger was a calendar date rather than a change.native-archive-merge-probe.shfailed outright (expected 2 parts, got 0).native-archive-gc-probe.shwas silently archiving an empty table — it never asserted its rows existed, so it "passed" while testing nothing, and only surfaced once the merge probe stopped aborting the job first, aspaths_csv: unbound variable.Both now derive the date from
date(yesterday — a completed day, well inside the window; GNU and BSD forms handled). The GC probe also initialisespaths_csvso an empty archive reports a real assertion instead of crashing the shell. No hardcoded fixture dates remain inapps/cli/test/*.sh, so neither can age out again.Deliberately not in this PR
From the plan, in order: explicit
noDataBehavior(stop inferring it from signal type), pushing multi-service +excludeServiceNamesinto the compiled spec (needs anexpectedGroupKeysleft-join or multi-service down-detectors stop paging — the plan's highest-risk item), and deleting themetricsignal type (a three-deploy data conversion).🤖 Generated with Claude Code