From 7a3e3b1bc9a505410e6aa2d0b9e29e98deac8f3a Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 20:21:09 -0500 Subject: [PATCH 01/18] Gate metric-alert evaluation cadence by rule window --- .../src/lib/metric-alert-evaluator.spec.ts | 70 +++++++++++++++++++ .../src/lib/metric-alert-evaluator.ts | 28 ++++++++ .../src/tasks/evaluate-metric-alert-rules.ts | 24 +++++-- 3 files changed, 118 insertions(+), 4 deletions(-) diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index a7020b38db..85d51e80a7 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -3,7 +3,9 @@ import { printWithValues, type SqlValue } from '@hive/clickhouse'; import type { ClickHouseClient } from './clickhouse-client.js'; import { buildSavedFilterConditions, + evaluationIntervalMinutes, groupRulesByQuery, + isRuleDue, queryClickHouseWindows, type MetricAlertRuleRow, } from './metric-alert-evaluator.js'; @@ -39,6 +41,7 @@ function makeRule(overrides: Partial): MetricAlertRuleRow { severity: 'WARNING', state: 'NORMAL', stateChangedAt: null, + lastEvaluatedAt: null, confirmationMinutes: 0, savedFilterId: null, savedFilterFilters: null, @@ -68,6 +71,73 @@ describe('groupRulesByQuery', () => { }); }); +describe('evaluationIntervalMinutes', () => { + test('tiers by window size', () => { + expect(evaluationIntervalMinutes(60)).toBe(1); + expect(evaluationIntervalMinutes(61)).toBe(5); + expect(evaluationIntervalMinutes(360)).toBe(5); + expect(evaluationIntervalMinutes(361)).toBe(15); + expect(evaluationIntervalMinutes(1440)).toBe(15); + expect(evaluationIntervalMinutes(1441)).toBe(30); + expect(evaluationIntervalMinutes(10080)).toBe(30); + expect(evaluationIntervalMinutes(43200)).toBe(30); + }); + + test('interval never exceeds the window, so the dwell stays sampled', () => { + for (const window of [1, 60, 61, 360, 361, 1440, 1441, 10080, 43200]) { + expect(evaluationIntervalMinutes(window)).toBeLessThanOrEqual(window); + } + }); +}); + +describe('isRuleDue', () => { + const evalTime = new Date('2026-07-08T12:00:00.000Z'); + // ISO timestamp for a point `minutes` before evalTime (mirrors the `to_json` + // timestamp string fetchEnabledRules returns for last_evaluated_at). + const ago = (minutes: number) => new Date(evalTime.getTime() - minutes * 60_000).toISOString(); + + test('a never-evaluated rule is always due', () => { + expect( + isRuleDue(makeRule({ lastEvaluatedAt: null, timeWindowMinutes: 43200 }), evalTime), + ).toBe(true); + }); + + test('30-day rule: due once its 30-min interval has elapsed', () => { + const base = { timeWindowMinutes: 43200, state: 'NORMAL' as const }; + expect(isRuleDue(makeRule({ ...base, lastEvaluatedAt: ago(0) }), evalTime)).toBe(false); + expect(isRuleDue(makeRule({ ...base, lastEvaluatedAt: ago(29) }), evalTime)).toBe(false); + expect(isRuleDue(makeRule({ ...base, lastEvaluatedAt: ago(30) }), evalTime)).toBe(true); + expect(isRuleDue(makeRule({ ...base, lastEvaluatedAt: ago(31) }), evalTime)).toBe(true); + }); + + test('sub-second tolerance only: 5s short of the interval is still not due', () => { + const almost = new Date(evalTime.getTime() - (30 * 60_000 - 5_000)).toISOString(); + expect( + isRuleDue(makeRule({ timeWindowMinutes: 43200, state: 'NORMAL', lastEvaluatedAt: almost }), evalTime), + ).toBe(false); + }); + + test('a 1h-window rule stays on the every-minute cadence', () => { + const base = { timeWindowMinutes: 60, state: 'NORMAL' as const }; + expect(isRuleDue(makeRule({ ...base, lastEvaluatedAt: ago(0) }), evalTime)).toBe(false); + expect(isRuleDue(makeRule({ ...base, lastEvaluatedAt: ago(1) }), evalTime)).toBe(true); + }); + + test('PENDING/RECOVERING keep full 1-min resolution regardless of window', () => { + for (const state of ['PENDING', 'RECOVERING'] as const) { + expect( + isRuleDue(makeRule({ timeWindowMinutes: 43200, state, lastEvaluatedAt: ago(1) }), evalTime), + ).toBe(true); + } + // The same 30-day rule in a steady state, 1 min after eval, is NOT due. + for (const state of ['NORMAL', 'FIRING'] as const) { + expect( + isRuleDue(makeRule({ timeWindowMinutes: 43200, state, lastEvaluatedAt: ago(1) }), evalTime), + ).toBe(false); + } + }); +}); + describe('buildSavedFilterConditions', () => { test('null filters -> no conditions', () => { const { logger } = makeLogger(); diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.ts index 4abfaf886b..5f0215b5a2 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.ts @@ -31,6 +31,7 @@ const MetricAlertRuleRowSchema = z.object({ severity: z.enum(['INFO', 'WARNING', 'CRITICAL']), state: z.enum(['NORMAL', 'PENDING', 'FIRING', 'RECOVERING']), stateChangedAt: z.string().nullable(), + lastEvaluatedAt: z.string().nullable(), confirmationMinutes: z.number(), savedFilterId: z.string().nullable(), // The attached saved filter's `filters` jsonb (or null). Kept as `unknown` so a @@ -167,6 +168,7 @@ export async function fetchEnabledRules(pg: PostgresDatabasePool): Promise 24h (7d, 30d): every 30 min +} + +// Whether a rule is due on the tick at evaluationTime. PENDING/RECOVERING rules +// stay at 1-min resolution so the confirmationMinutes dwell is sampled every +// tick; a never-evaluated rule is always due. +export function isRuleDue( + rule: Pick, + evaluationTime: Date, +): boolean { + if (rule.state === 'PENDING' || rule.state === 'RECOVERING') return true; + if (!rule.lastEvaluatedAt) return true; + const intervalMs = evaluationIntervalMinutes(rule.timeWindowMinutes) * 60_000; + const lastMs = new Date(rule.lastEvaluatedAt).getTime(); + // Small tolerance so an exactly-interval-old rule isn't skipped on sub-second + // jitter. Fires up to ~1 tick early, never late. + return evaluationTime.getTime() - lastMs >= intervalMs - 1_000; +} + // Validated shape of the saved-filter `filters` jsonb that the evaluator applies. // `dateRange` is deliberately NOT read (an alert defines its own rolling window, // so the filter's saved date range is meaningless here). `.passthrough()` tolerates diff --git a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts index 4c44956e64..af3640e610 100644 --- a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts +++ b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts @@ -7,6 +7,7 @@ import { evaluateRule, fetchEnabledRules, groupRulesByQuery, + isRuleDue, queryClickHouseWindows, } from '../lib/metric-alert-evaluator.js'; @@ -64,8 +65,21 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { logger.info({ count: rules.length }, 'Evaluating metric alert rules'); + // Evaluate only groups with at least one due member. Members of a group share + // a window (so one cadence), and the batched UPDATE below keeps their + // last_evaluated_at aligned, so a group is due as a unit. const groups = groupRulesByQuery(rules); - const groupList = [...groups.values()]; + const dueGroupList = [...groups.values()].filter(group => + group.some(rule => isRuleDue(rule, evaluationTime)), + ); + + if (dueGroupList.length === 0) { + logger.debug( + { evaluationTime: evaluationTime.toISOString(), groups: groups.size, rules: rules.length }, + 'No metric alert rule groups are due this tick', + ); + return; + } async function processGroup(groupRules: (typeof rules)[number][]): Promise<{ failed: boolean; @@ -170,6 +184,8 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { attributes: { 'rules.count': rules.length, 'groups.count': groups.size, + 'groups.due': dueGroupList.length, + 'rules.due': dueGroupList.reduce((total, group) => total + group.length, 0), 'evaluation.time': evaluationTime.toISOString(), }, }, @@ -185,8 +201,8 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { // without exhausting the PG pool. let groupsFailed = 0; const evaluatedRuleIds: string[] = []; - for (let i = 0; i < groupList.length; i += GROUP_CONCURRENCY) { - const batch = groupList.slice(i, i + GROUP_CONCURRENCY); + for (let i = 0; i < dueGroupList.length; i += GROUP_CONCURRENCY) { + const batch = dueGroupList.slice(i, i + GROUP_CONCURRENCY); const results = await Promise.allSettled(batch.map(processGroup)); for (const r of results) { if (r.status === 'rejected') { @@ -226,7 +242,7 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { logger.info( { - groupsAttempted: groups.size, + groupsAttempted: dueGroupList.length, groupsFailed, rulesEvaluated: evaluatedRuleIds.length, }, From 381df290daa91ffa84abec8a576fc403b5e9fef0 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 20:25:00 -0500 Subject: [PATCH 02/18] Add operations_by_target_daily ClickHouse rollup migration --- .../019-metric-alert-target-daily-rollup.ts | 49 +++++++++++++++++++ packages/migrations/src/clickhouse.ts | 1 + 2 files changed, 50 insertions(+) create mode 100644 packages/migrations/src/clickhouse-actions/019-metric-alert-target-daily-rollup.ts diff --git a/packages/migrations/src/clickhouse-actions/019-metric-alert-target-daily-rollup.ts b/packages/migrations/src/clickhouse-actions/019-metric-alert-target-daily-rollup.ts new file mode 100644 index 0000000000..1311ab98f4 --- /dev/null +++ b/packages/migrations/src/clickhouse-actions/019-metric-alert-target-daily-rollup.ts @@ -0,0 +1,49 @@ +import type { Action } from '../clickhouse'; + +// Daily by-target rollup for the metric-alert evaluator. Mirrors the +// minutely/hourly rollups in 018 but bucketed per day, so long-window rules +// (>= 7 days) read ~60 daily buckets for a 30-day query instead of ~1,440 +// hourly ones. The 90-day TTL covers the 60-day (2x window) scan of a 30-day +// rule. +const tableColumns = ` + target LowCardinality(String) CODEC(ZSTD(1)), + timestamp DateTime('UTC') CODEC(DoubleDelta, LZ4), + total UInt32 CODEC(T64, ZSTD(1)), + total_ok UInt32 CODEC(T64, ZSTD(1)), + duration_avg AggregateFunction(avg, UInt64) CODEC(ZSTD(1)), + duration_quantiles AggregateFunction(quantilesTDigest(0.75, 0.9, 0.95, 0.99), UInt64) CODEC(ZSTD(1)) +`; + +export const action: Action = async exec => { + await exec(` + CREATE TABLE IF NOT EXISTS default.operations_by_target_daily + ( + ${tableColumns} + ) + ENGINE = SummingMergeTree + -- Monthly, not toYYYYMMDD like the hourly rollup: daily buckets would make + -- one partition per day. Keep monthly so a 90-day TTL has few parts. + PARTITION BY toYYYYMM(timestamp) + PRIMARY KEY (target, timestamp) + ORDER BY (target, timestamp) + TTL timestamp + INTERVAL 90 DAY + SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1 + `); + + await exec(` + CREATE MATERIALIZED VIEW IF NOT EXISTS default.operations_by_target_daily_mv TO default.operations_by_target_daily + AS ( + SELECT + target, + toStartOfDay(timestamp) AS timestamp, + CAST(count() AS UInt32) AS total, + CAST(sum(ok) AS UInt32) AS total_ok, + avgState(duration) AS duration_avg, + quantilesTDigestState(0.75, 0.9, 0.95, 0.99)(duration) AS duration_quantiles + FROM default.operations + GROUP BY + target, + timestamp + ) + `); +}; diff --git a/packages/migrations/src/clickhouse.ts b/packages/migrations/src/clickhouse.ts index 46b764e35a..2aa876004c 100644 --- a/packages/migrations/src/clickhouse.ts +++ b/packages/migrations/src/clickhouse.ts @@ -182,6 +182,7 @@ export async function migrateClickHouse( import('./clickhouse-actions/016-subgraph-otel-traces-cleanup'), import('./clickhouse-actions/017-affected-app-deployments-performance'), import('./clickhouse-actions/018-metric-alert-target-rollups'), + import('./clickhouse-actions/019-metric-alert-target-daily-rollup'), ]); async function actionRunner(action: Action, index: number) { From 62f2b95806b443bae3eceb73bebbc5a0d0151a09 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 20:31:51 -0500 Subject: [PATCH 03/18] Route metric-alert windows >= 7 days to daily rollups --- .../src/lib/metric-alert-evaluator.spec.ts | 34 +++++++++++++++++++ .../src/lib/metric-alert-evaluator.ts | 14 +++++++- 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index 85d51e80a7..b3a9075acf 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -252,4 +252,38 @@ describe('queryClickHouseWindows', () => { await queryClickHouseWindows(clickhouse, target, 720, [], evalTime); expect(calls[0].sql).toContain('FROM operations_by_target_hourly'); }); + + test('windows below 7 days stay on the hourly rollup', async () => { + const { clickhouse, calls } = captureClient(); + // 1 day, 3 days, and one minute under the 7-day cutoff all read hourly. + await queryClickHouseWindows(clickhouse, target, 1440, [], evalTime); + await queryClickHouseWindows(clickhouse, target, 4320, [], evalTime); + await queryClickHouseWindows(clickhouse, target, 10079, [], evalTime); + for (const call of calls) { + expect(call.sql).toContain('FROM operations_by_target_hourly'); + } + }); + + test('windows >= 7 days read the daily rollup (unfiltered -> by_target)', async () => { + const { clickhouse, calls } = captureClient(); + await queryClickHouseWindows(clickhouse, target, 10080, [], evalTime); + const { sql } = calls[0]; + expect(sql).toContain('FROM operations_by_target_daily'); + expect(sql).toContain('quantilesTDigestMerge('); + }); + + test('windows >= 7 days read the daily rollup (filtered -> legacy operations_daily)', async () => { + const { clickhouse, calls } = captureClient(); + const conds = buildSavedFilterConditions( + { clientFilters: [{ name: 'web', versions: null }] }, + makeLogger().logger, + ); + await queryClickHouseWindows(clickhouse, target, 43200, conds, evalTime); + const { sql } = calls[0]; + expect(sql).toContain('FROM operations_daily'); + expect(sql).not.toContain('_by_target'); + expect(sql).toContain('quantilesMerge('); + expect(sql).not.toContain('TDigest'); + expect(sql).toContain('client_name = {p2: String}'); + }); }); diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.ts index 5f0215b5a2..c662640ec0 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.ts @@ -68,6 +68,13 @@ function makeGroupKey(rule: MetricAlertRuleRow): GroupKey { // nanoseconds; latency rule thresholds and all display surfaces use ms. const NS_TO_MS = 1e6; +// Windows >= 7 days read the daily rollups (operations_by_target_daily / +// operations_daily) instead of hourly, so a 30-day query scans ~60 daily buckets +// instead of ~1,440 hourly. Below 7d stays on hourly/minutely for exact sub-day +// precision. The API keeps windows at/above this a whole number of days (see +// assertTimeWindowInRange) so daily buckets don't silently round the window. +const DAILY_THRESHOLD_MINUTES = 7 * 24 * 60; // 10080 + function extractMetricValue(row: ClickHouseWindowRow, rule: MetricAlertRuleRow): number { const total = Number(row.total); const totalOk = Number(row.total_ok); @@ -303,7 +310,12 @@ export async function queryClickHouseWindows( // (rather than a separate flag/param) makes the filtered-query-on-rollup // combination unrepresentable. const useTargetRollup = filterConditions.length === 0; - const resolution = timeWindowMinutes <= 360 ? 'minutely' : 'hourly'; + const resolution = + timeWindowMinutes <= 360 + ? 'minutely' + : timeWindowMinutes >= DAILY_THRESHOLD_MINUTES + ? 'daily' + : 'hourly'; const tableName = useTargetRollup ? `operations_by_target_${resolution}` : `operations_${resolution}`; From a9760ee1849c8eb69737f8b887749cf7f8765a3f Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 20:48:05 -0500 Subject: [PATCH 04/18] Reject metric-alert windows >= 7 days that are not whole days --- .../alerts/providers/metric-alert-rules-manager.ts | 12 ++++++++++++ .../services/api/src/modules/commerce/constants.ts | 6 ++++++ 2 files changed, 18 insertions(+) diff --git a/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts b/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts index 0ac51e82d7..e3cd93a43a 100644 --- a/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts +++ b/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts @@ -3,6 +3,7 @@ import type { MetricAlertRule } from '../../../shared/entities'; import { AccessError } from '../../../shared/errors'; import { Session } from '../../auth/lib/authz'; import { + METRIC_ALERT_RULE_DAILY_ROLLUP_THRESHOLD_MINUTES, METRIC_ALERT_RULE_TIME_WINDOW_MAX_MINUTES, METRIC_ALERT_RULE_TIME_WINDOW_MIN_MINUTES, METRIC_ALERT_RULES_PER_TARGET_LIMIT, @@ -338,6 +339,17 @@ export class MetricAlertRulesManager { `Time window must be a whole number of minutes between ${METRIC_ALERT_RULE_TIME_WINDOW_MIN_MINUTES} and ${METRIC_ALERT_RULE_TIME_WINDOW_MAX_MINUTES} (30 days).`, ); } + // Windows at/above the daily-rollup threshold read whole-day ClickHouse + // buckets, so they must be a whole number of days or the window is silently + // rounded. The UI presets already satisfy this; this guards direct API callers. + if ( + timeWindowMinutes >= METRIC_ALERT_RULE_DAILY_ROLLUP_THRESHOLD_MINUTES && + timeWindowMinutes % (24 * 60) !== 0 + ) { + throw new MetricAlertRuleValidationError( + 'Time windows of 7 days or more must be a whole number of days.', + ); + } } private async assertChannelsBelongToProject( diff --git a/packages/services/api/src/modules/commerce/constants.ts b/packages/services/api/src/modules/commerce/constants.ts index c185421f07..b365c820d5 100644 --- a/packages/services/api/src/modules/commerce/constants.ts +++ b/packages/services/api/src/modules/commerce/constants.ts @@ -53,3 +53,9 @@ export const METRIC_ALERT_RULES_PER_TARGET_LIMIT = 10; */ export const METRIC_ALERT_RULE_TIME_WINDOW_MIN_MINUTES = 1; export const METRIC_ALERT_RULE_TIME_WINDOW_MAX_MINUTES = 30 * 24 * 60; // 43200 + +// Windows at or above this read the daily ClickHouse rollup, whose buckets are +// whole days. A window this size must therefore be a whole number of days +// (multiple of 1440 min) or the daily aggregate would silently round it. Mirrors +// DAILY_THRESHOLD_MINUTES in the workflows evaluator; keep the two in sync. +export const METRIC_ALERT_RULE_DAILY_ROLLUP_THRESHOLD_MINUTES = 7 * 24 * 60; // 10080 From 2d0a0c1e1456237e04a41f441fe3ac560d4208c2 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 20:52:46 -0500 Subject: [PATCH 05/18] Make workflows service deploy after db migrations --- deployment/index.ts | 1 + deployment/services/workflows.ts | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/deployment/index.ts b/deployment/index.ts index c184dcdb0a..c35259c1ea 100644 --- a/deployment/index.ts +++ b/deployment/index.ts @@ -201,6 +201,7 @@ deployWorkflows({ schema, redis, clickhouse, + dbMigrations, }); const zendesk = configureZendesk({ environment }); diff --git a/deployment/services/workflows.ts b/deployment/services/workflows.ts index 29d3da0531..30807f19e2 100644 --- a/deployment/services/workflows.ts +++ b/deployment/services/workflows.ts @@ -3,6 +3,7 @@ import { serviceLocalEndpoint } from '../utils/local-endpoint'; import { ServiceSecret } from '../utils/secrets'; import { ServiceDeployment } from '../utils/service-deployment'; import { Clickhouse } from './clickhouse'; +import { DbMigrations } from './db-migrations'; import { Docker } from './docker'; import { Environment } from './environment'; import { Observability } from './observability'; @@ -29,6 +30,7 @@ export function deployWorkflows({ schema, redis, clickhouse, + dbMigrations, }: { postgres: Postgres; observability: Observability; @@ -41,6 +43,7 @@ export function deployWorkflows({ schema: Schema; redis: Redis; clickhouse: Clickhouse; + dbMigrations: DbMigrations; }) { const featureFlagsConfig = new pulumi.Config('featureFlags'); return ( @@ -75,7 +78,10 @@ export function deployWorkflows({ image, replicas: environment.podsConfig.general.replicas, }, - [redis.deployment, redis.service], + // Depend on dbMigrations so the ClickHouse migration (which creates + // operations_by_target_daily) runs before this service starts routing + // long-window queries to it. Mirrors deployGraphQL. + [dbMigrations, redis.deployment, redis.service], ) // PG .withSecret('POSTGRES_HOST', postgres.pgBouncerSecret, 'host') From 815eb93eb8ff9b348076e37e2b1b40e3f49e69df Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 21:02:07 -0500 Subject: [PATCH 06/18] Skip the previous window for absolute-only alert groups --- .../src/lib/metric-alert-evaluator.spec.ts | 27 ++++++++++++++++ .../src/lib/metric-alert-evaluator.ts | 32 +++++++++++++------ .../src/tasks/evaluate-metric-alert-rules.ts | 15 +++++++-- 3 files changed, 62 insertions(+), 12 deletions(-) diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index b3a9075acf..7b95603016 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -286,4 +286,31 @@ describe('queryClickHouseWindows', () => { expect(sql).not.toContain('TDigest'); expect(sql).toContain('client_name = {p2: String}'); }); + + test('absolute-only groups skip the previous window (1x scan, constant label)', async () => { + const { clickhouse, calls } = captureClient(); + const result = await queryClickHouseWindows(clickhouse, target, 60, [], evalTime, false); + const { sql } = calls[0]; + // Single-window query: constant 'current' label, no previous branch. + expect(sql).toContain("'current' as window"); + expect(sql).not.toContain("'previous'"); + // Scans from the current window start, not the previous window start. + const anchor = evalTime.getTime(); + const currentStart = anchor - 60_000 - 60 * 60_000; + const previousStart = anchor - 60_000 - 2 * 60 * 60_000; + expect(sql).toContain(String(currentStart)); + expect(sql).not.toContain(String(previousStart)); + // Previous reported null so the caller persists previousValue = null. + expect(result.previous).toBeNull(); + }); + + test('groups needing the previous window fetch both (default)', async () => { + const { clickhouse, calls } = captureClient(); + await queryClickHouseWindows(clickhouse, target, 60, [], evalTime, true); + const { sql } = calls[0]; + expect(sql).toContain("ELSE 'previous'"); + const anchor = evalTime.getTime(); + const previousStart = anchor - 60_000 - 2 * 60 * 60_000; + expect(sql).toContain(String(previousStart)); + }); }); diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.ts index c662640ec0..c1df001733 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.ts @@ -292,6 +292,10 @@ export async function queryClickHouseWindows( // this job up late, using wall-clock would shift the queried window // forward and could miss the spike that should have fired the alert. evaluationTime: Date, + // Absolute-threshold groups never compare against the prior window, so skip + // fetching it: scan 1x the window and return previous = null. Defaults true so + // percentage-change callers are unaffected. + needsPreviousWindow: boolean = true, ): Promise<{ current: ClickHouseWindowRow | null; previous: ClickHouseWindowRow | null }> { const anchorMs = evaluationTime.getTime(); const offsetMs = 60_000; @@ -334,19 +338,24 @@ export async function queryClickHouseWindows( const filterClause = filterConditions.length > 0 ? sql` AND ${sql.join(filterConditions, ' AND ')}` : sql``; + // Skip the previous window when no rule in the group needs it: scan from the + // current window start (1x window) and label every row 'current'. Otherwise + // scan both windows and tag each row current/previous. + const scanStart = needsPreviousWindow ? previousWindowStart : currentWindowStart; + const windowSelector = needsPreviousWindow + ? sql`CASE WHEN timestamp >= fromUnixTimestamp64Milli(${sql.raw(String(currentWindowStart.getTime()))}) THEN 'current' ELSE 'previous' END` + : sql`'current'`; + const statement = sql` SELECT - CASE - WHEN timestamp >= fromUnixTimestamp64Milli(${sql.raw(String(currentWindowStart.getTime()))}) THEN 'current' - ELSE 'previous' - END as window, + ${windowSelector} as window, sum(total) as total, sum(total_ok) as total_ok, avgMerge(duration_avg) as average, ${sql.raw(percentilesMerge)}(0.75, 0.90, 0.95, 0.99)(duration_quantiles) as percentiles FROM ${sql.raw(tableName)} WHERE target = ${targetId} - AND timestamp >= fromUnixTimestamp64Milli(${sql.raw(String(previousWindowStart.getTime()))}) + AND timestamp >= fromUnixTimestamp64Milli(${sql.raw(String(scanStart.getTime()))}) AND timestamp < fromUnixTimestamp64Milli(${sql.raw(String(currentWindowEnd.getTime()))}) ${filterClause} GROUP BY window @@ -374,14 +383,14 @@ export async function queryClickHouseWindows( return { current: rows.find(r => r.window === 'current') ?? null, - previous: rows.find(r => r.window === 'previous') ?? null, + previous: needsPreviousWindow ? (rows.find(r => r.window === 'previous') ?? null) : null, }; } export async function evaluateRule(args: { rule: MetricAlertRuleRow; current: ClickHouseWindowRow; - previous: ClickHouseWindowRow; + previous: ClickHouseWindowRow | null; pg: PostgresDatabasePool; logger: Logger; // Anchor for state_changed_at, last_triggered_at, expires_at, and @@ -394,8 +403,11 @@ export async function evaluateRule(args: { const now = evaluationTime; const currentValue = extractMetricValue(current, rule); - const previousValue = extractMetricValue(previous, rule); - const breached = isThresholdBreached(currentValue, previousValue, rule); + // A skipped previous window (absolute-only group) yields a null previousValue, + // persisted as-is. FIXED_VALUE never reads it; pass 0 to the breach check just + // for typing (percentage-change groups always fetch the previous window). + const previousValue = previous ? extractMetricValue(previous, rule) : null; + const breached = isThresholdBreached(currentValue, previousValue ?? 0, rule); // State-log retention is derived from the rule's organization plan, which // is included on the row by `fetchEnabledRules` (single JOIN) — no extra @@ -600,7 +612,7 @@ async function logTransition( fromState: string, toState: string, value: number, - previousValue: number, + previousValue: number | null, retentionDays: number, evaluationTime: Date, incidentId: string | null = null, diff --git a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts index af3640e610..06b9085b0f 100644 --- a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts +++ b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts @@ -93,6 +93,12 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { // isolating the failure to this group. const filterConditions = buildSavedFilterConditions(representative.savedFilterFilters, logger); + // Only PERCENTAGE_CHANGE rules compare against the prior window. If no rule + // in the group is one, skip fetching it (half the scan) and persist a null + // previousValue. Any percentage-change rule flips the whole group back to + // fetching both windows. + const needsPreviousWindow = groupRules.some(r => r.thresholdType === 'PERCENTAGE_CHANGE'); + // startActiveSpan makes this span the current OTel context for the // duration of the callback, so the slonik PG interceptor and the // fetch instrumentation parent their auto-spans under this one. That's @@ -118,6 +124,7 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { representative.timeWindowMinutes, filterConditions, evaluationTime, + needsPreviousWindow, ); } catch (error) { logger.error( @@ -143,9 +150,13 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { percentiles: [0, 0, 0, 0] as [number, number, number, number], }; const current = windows.current ?? { window: 'current' as const, ...ZERO_WINDOW }; - const previous = windows.previous ?? { window: 'previous' as const, ...ZERO_WINDOW }; + // A skipped previous window stays null (not synthesized to zeros): the + // group is absolute-only, so evaluateRule persists previousValue null. + const previous = needsPreviousWindow + ? (windows.previous ?? { window: 'previous' as const, ...ZERO_WINDOW }) + : null; - if (!windows.current || !windows.previous) { + if (!windows.current || (needsPreviousWindow && !windows.previous)) { logger.debug( { targetId: representative.targetId }, 'No traffic in window(s), evaluating against zeros', From 31fd97f2a131ca625ce5478df0c3ecb1837378c3 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 21:08:46 -0500 Subject: [PATCH 07/18] Handle null previous value in alert notifications --- .../workflows/src/lib/metric-alert-notifier.ts | 14 +++++++++++--- .../send-metric-alert-channel-notification.ts | 5 +++-- .../target/alerts/alert-notification-preview.tsx | 3 +-- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/packages/services/workflows/src/lib/metric-alert-notifier.ts b/packages/services/workflows/src/lib/metric-alert-notifier.ts index cad8c550d9..5782cb6938 100644 --- a/packages/services/workflows/src/lib/metric-alert-notifier.ts +++ b/packages/services/workflows/src/lib/metric-alert-notifier.ts @@ -27,7 +27,8 @@ export type NotificationEvent = { | 'direction' >; currentValue: number; - previousValue: number; + // Null for absolute-only groups that skip the previous window. + previousValue: number | null; organizationSlug: string; projectSlug: string; targetSlug: string; @@ -209,11 +210,16 @@ function formatChangeText(event: NotificationEvent): string { : 'Traffic'; if (event.state === 'firing') { + const thresholdText = `Threshold: ${rule.direction.toLowerCase()} ${rule.thresholdValue}${rule.thresholdType === 'PERCENTAGE_CHANGE' ? '%' : unit}`; + // Absolute-only rules have no previous window to compare against. + if (previousValue === null) { + return `${metricLabel}: **${currentValue.toFixed(2)}${unit}** — ${thresholdText}`; + } const changePercent = previousValue !== 0 ? (((currentValue - previousValue) / previousValue) * 100).toFixed(1) : 'N/A'; - return `${metricLabel}: **${currentValue.toFixed(2)}${unit}** (was ${previousValue.toFixed(2)}${unit}, ${changePercent}% change) — Threshold: ${rule.direction.toLowerCase()} ${rule.thresholdValue}${rule.thresholdType === 'PERCENTAGE_CHANGE' ? '%' : unit}`; + return `${metricLabel}: **${currentValue.toFixed(2)}${unit}** (was ${previousValue.toFixed(2)}${unit}, ${changePercent}% change) — ${thresholdText}`; } return `${metricLabel}: **${currentValue.toFixed(2)}${unit}** (threshold: ${rule.thresholdValue}${rule.thresholdType === 'PERCENTAGE_CHANGE' ? '%' : unit})`; @@ -222,7 +228,9 @@ function formatChangeText(event: NotificationEvent): string { function buildWebhookPayload(event: NotificationEvent) { const { rule, currentValue, previousValue } = event; const changePercent = - previousValue !== 0 ? ((currentValue - previousValue) / previousValue) * 100 : null; + previousValue !== null && previousValue !== 0 + ? ((currentValue - previousValue) / previousValue) * 100 + : null; return { type: 'metric_alert', diff --git a/packages/services/workflows/src/tasks/send-metric-alert-channel-notification.ts b/packages/services/workflows/src/tasks/send-metric-alert-channel-notification.ts index 2182e9caa2..88086567b7 100644 --- a/packages/services/workflows/src/tasks/send-metric-alert-channel-notification.ts +++ b/packages/services/workflows/src/tasks/send-metric-alert-channel-notification.ts @@ -25,7 +25,8 @@ const HydrationRowSchema = z.object({ fromState: z.enum(['NORMAL', 'PENDING', 'FIRING', 'RECOVERING']), toState: z.enum(['NORMAL', 'PENDING', 'FIRING', 'RECOVERING']), value: z.number(), - previousValue: z.number(), + // Null for absolute-only groups whose evaluation skips the previous window. + previousValue: z.number().nullable(), stateLogCreatedAt: z.string(), // rule (subset; the notifier only reads these fields off `event.rule`) ruleId: z.string(), @@ -163,7 +164,7 @@ export const task = implementTask(SendMetricAlertChannelNotificationTask, async direction: row.ruleDirection, }, currentValue: Number(row.value), - previousValue: Number(row.previousValue), + previousValue: row.previousValue === null ? null : Number(row.previousValue), organizationSlug: row.organizationSlug, projectSlug: row.projectSlug, targetSlug: row.targetSlug, diff --git a/packages/web/app/src/components/target/alerts/alert-notification-preview.tsx b/packages/web/app/src/components/target/alerts/alert-notification-preview.tsx index 2d22b39cc7..c1578e3e07 100644 --- a/packages/web/app/src/components/target/alerts/alert-notification-preview.tsx +++ b/packages/web/app/src/components/target/alerts/alert-notification-preview.tsx @@ -17,7 +17,6 @@ const WEBHOOK_JSON_SCHEMA = { 'state', 'alert', 'currentValue', - 'previousValue', 'threshold', 'target', 'project', @@ -37,7 +36,7 @@ const WEBHOOK_JSON_SCHEMA = { }, }, currentValue: { type: 'number' }, - previousValue: { type: 'number' }, + previousValue: { type: ['number', 'null'] }, changePercent: { type: ['number', 'null'] }, threshold: { type: 'object', From 6a44e1190a11241904233f766050a0f409848722 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 21:24:16 -0500 Subject: [PATCH 08/18] Skip unused duration columns for error/traffic alert groups --- .../src/lib/metric-alert-evaluator.spec.ts | 27 +++++++++++ .../src/lib/metric-alert-evaluator.ts | 45 +++++++++++-------- .../src/tasks/evaluate-metric-alert-rules.ts | 8 ++++ 3 files changed, 62 insertions(+), 18 deletions(-) diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index 7b95603016..9b94224ad4 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -313,4 +313,31 @@ describe('queryClickHouseWindows', () => { const previousStart = anchor - 60_000 - 2 * 60 * 60_000; expect(sql).toContain(String(previousStart)); }); + + test('groups needing neither duration column select NULL placeholders', async () => { + const { clickhouse, calls } = captureClient(); + await queryClickHouseWindows(clickhouse, target, 60, [], evalTime, true, false, false); + const { sql } = calls[0]; + expect(sql).toContain('NULL as average'); + expect(sql).toContain('NULL as percentiles'); + expect(sql).not.toContain('avgMerge(duration_avg)'); + expect(sql).not.toContain('duration_quantiles'); + }); + + test('percentile groups select the quantiles column; avg still skipped', async () => { + const { clickhouse, calls } = captureClient(); + await queryClickHouseWindows(clickhouse, target, 60, [], evalTime, true, false, true); + const { sql } = calls[0]; + expect(sql).toContain('duration_quantiles'); + expect(sql).toContain('NULL as average'); + }); + + test('avg groups select avgMerge; percentiles skipped', async () => { + const { clickhouse, calls } = captureClient(); + await queryClickHouseWindows(clickhouse, target, 60, [], evalTime, true, true, false); + const { sql } = calls[0]; + expect(sql).toContain('avgMerge(duration_avg) as average'); + expect(sql).toContain('NULL as percentiles'); + expect(sql).not.toContain('duration_quantiles'); + }); }); diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.ts index c1df001733..9a4d847623 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.ts @@ -52,8 +52,10 @@ const ClickHouseWindowRowSchema = z.object({ window: z.enum(['current', 'previous']), total: z.string(), total_ok: z.string(), - average: z.number(), - percentiles: z.tuple([z.number(), z.number(), z.number(), z.number()]), + // Null when a group's rules don't need them (no LATENCY rule of that metric), + // in which case the query selects `NULL as ...` and doesn't read the column. + average: z.number().nullable(), + percentiles: z.tuple([z.number(), z.number(), z.number(), z.number()]).nullable(), }); type ClickHouseWindowRow = z.infer; @@ -85,12 +87,14 @@ function extractMetricValue(row: ClickHouseWindowRow, rule: MetricAlertRuleRow): case 'ERROR_RATE': return total > 0 ? ((total - totalOk) / total) * 100 : 0; case 'LATENCY': { + // A LATENCY rule forces its column to be selected, so these are non-null + // when actually read; the `?? 0` is just for the nullable type. const metricMap: Record = { - AVG: row.average, - P75: row.percentiles[0], - P90: row.percentiles[1], - P95: row.percentiles[2], - P99: row.percentiles[3], + AVG: row.average ?? 0, + P75: row.percentiles?.[0] ?? 0, + P90: row.percentiles?.[1] ?? 0, + P95: row.percentiles?.[2] ?? 0, + P99: row.percentiles?.[3] ?? 0, }; // ClickHouse stores `duration` in nanoseconds, but rule thresholds (the // form input) and every display surface are in @@ -215,9 +219,8 @@ export function evaluationIntervalMinutes(timeWindowMinutes: number): number { return 30; // > 24h (7d, 30d): every 30 min } -// Whether a rule is due on the tick at evaluationTime. PENDING/RECOVERING rules -// stay at 1-min resolution so the confirmationMinutes dwell is sampled every -// tick; a never-evaluated rule is always due. +// PENDING/RECOVERING rules stay at 1-min resolution so the confirmationMinutes +// dwell is sampled every tick. A never-evaluated rule is always due. export function isRuleDue( rule: Pick, evaluationTime: Date, @@ -292,10 +295,12 @@ export async function queryClickHouseWindows( // this job up late, using wall-clock would shift the queried window // forward and could miss the spike that should have fired the alert. evaluationTime: Date, - // Absolute-threshold groups never compare against the prior window, so skip - // fetching it: scan 1x the window and return previous = null. Defaults true so - // percentage-change callers are unaffected. + // False = scan only the current window, return previous = null. Default true. needsPreviousWindow: boolean = true, + // False = select `NULL as ` so ClickHouse doesn't read that duration + // column. Default true. The caller decides all three per group (processGroup). + needsAverage: boolean = true, + needsPercentiles: boolean = true, ): Promise<{ current: ClickHouseWindowRow | null; previous: ClickHouseWindowRow | null }> { const anchorMs = evaluationTime.getTime(); const offsetMs = 60_000; @@ -338,21 +343,25 @@ export async function queryClickHouseWindows( const filterClause = filterConditions.length > 0 ? sql` AND ${sql.join(filterConditions, ' AND ')}` : sql``; - // Skip the previous window when no rule in the group needs it: scan from the - // current window start (1x window) and label every row 'current'. Otherwise - // scan both windows and tag each row current/previous. const scanStart = needsPreviousWindow ? previousWindowStart : currentWindowStart; const windowSelector = needsPreviousWindow ? sql`CASE WHEN timestamp >= fromUnixTimestamp64Milli(${sql.raw(String(currentWindowStart.getTime()))}) THEN 'current' ELSE 'previous' END` : sql`'current'`; + // A literal NULL isn't a column reference, so the (large) duration column isn't + // read when the group doesn't need it, while the row keeps a stable shape. + const averageCol = needsAverage ? sql`avgMerge(duration_avg) as average` : sql`NULL as average`; + const percentilesCol = needsPercentiles + ? sql`${sql.raw(percentilesMerge)}(0.75, 0.90, 0.95, 0.99)(duration_quantiles) as percentiles` + : sql`NULL as percentiles`; + const statement = sql` SELECT ${windowSelector} as window, sum(total) as total, sum(total_ok) as total_ok, - avgMerge(duration_avg) as average, - ${sql.raw(percentilesMerge)}(0.75, 0.90, 0.95, 0.99)(duration_quantiles) as percentiles + ${averageCol}, + ${percentilesCol} FROM ${sql.raw(tableName)} WHERE target = ${targetId} AND timestamp >= fromUnixTimestamp64Milli(${sql.raw(String(scanStart.getTime()))}) diff --git a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts index 06b9085b0f..19e25b388f 100644 --- a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts +++ b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts @@ -99,6 +99,12 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { // fetching both windows. const needsPreviousWindow = groupRules.some(r => r.thresholdType === 'PERCENTAGE_CHANGE'); + // Only LATENCY rules read the duration columns; skip the heavy percentile + // column unless a percentile-metric LATENCY rule is present, and avgMerge + // unless an AVG-metric one is. Pure error/traffic groups fetch neither. + const needsPercentiles = groupRules.some(r => r.type === 'LATENCY' && r.metric !== 'AVG'); + const needsAverage = groupRules.some(r => r.type === 'LATENCY' && r.metric === 'AVG'); + // startActiveSpan makes this span the current OTel context for the // duration of the callback, so the slonik PG interceptor and the // fetch instrumentation parent their auto-spans under this one. That's @@ -125,6 +131,8 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { filterConditions, evaluationTime, needsPreviousWindow, + needsAverage, + needsPercentiles, ); } catch (error) { logger.error( From fdb90227beba9a07e2181c576284e9a13340f9d2 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 21:28:58 -0500 Subject: [PATCH 09/18] Evaluate metric-alert groups with a concurrency pool --- packages/services/workflows/package.json | 1 + .../src/tasks/evaluate-metric-alert-rules.ts | 38 +++++++++---------- pnpm-lock.yaml | 3 ++ 3 files changed, 22 insertions(+), 20 deletions(-) diff --git a/packages/services/workflows/package.json b/packages/services/workflows/package.json index 19b847f8c2..7ed39cb86a 100644 --- a/packages/services/workflows/package.json +++ b/packages/services/workflows/package.json @@ -31,6 +31,7 @@ "graphql-yoga": "5.13.3", "mjml": "4.14.0", "nodemailer": "9.0.1", + "p-limit": "6.2.0", "sendmail": "1.6.1", "zod": "3.25.76" } diff --git a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts index 19e25b388f..5909d1ffff 100644 --- a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts +++ b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts @@ -1,3 +1,4 @@ +import pLimit from 'p-limit'; import { z } from 'zod'; import { psql } from '@hive/postgres'; import { SpanKind, SpanStatusCode, trace } from '@hive/service-common'; @@ -210,28 +211,25 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { }, async span => { try { - // Bounded parallelism over groups: each group = 1 CH query + per-rule - // state writes. allSettled (not Promise.all) so that one unexpected - // throw inside evaluateRule doesn't strand the rest of the batch's - // successful work; the throwing group's rules get re-evaluated on the - // next cron tick (60s) rather than via graphile-worker retries, which - // would otherwise re-run work that's already idempotently committed. - // With GROUP_CONCURRENCY=5 we cut wall-clock per tick by up to 5x - // without exhausting the PG pool. + // Fixed-capacity pool: keep GROUP_CONCURRENCY groups in flight at once so + // a slow group can't idle the other slots (a batch barrier would wait for + // the whole batch before starting the next). allSettled so one thrown + // group doesn't strand the rest; it re-evaluates on the next 60s tick, + // not via graphile-worker retries that would re-run already-committed work. + const limit = pLimit(GROUP_CONCURRENCY); let groupsFailed = 0; const evaluatedRuleIds: string[] = []; - for (let i = 0; i < dueGroupList.length; i += GROUP_CONCURRENCY) { - const batch = dueGroupList.slice(i, i + GROUP_CONCURRENCY); - const results = await Promise.allSettled(batch.map(processGroup)); - for (const r of results) { - if (r.status === 'rejected') { - groupsFailed++; - logger.error({ error: r.reason }, 'Group evaluation threw unexpectedly'); - } else if (r.value.failed) { - groupsFailed++; - } else { - evaluatedRuleIds.push(...r.value.evaluatedIds); - } + const results = await Promise.allSettled( + dueGroupList.map(group => limit(() => processGroup(group))), + ); + for (const r of results) { + if (r.status === 'rejected') { + groupsFailed++; + logger.error({ error: r.reason }, 'Group evaluation threw unexpectedly'); + } else if (r.value.failed) { + groupsFailed++; + } else { + evaluatedRuleIds.push(...r.value.evaluatedIds); } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index db72989934..747b65c24a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2065,6 +2065,9 @@ importers: nodemailer: specifier: 9.0.1 version: 9.0.1 + p-limit: + specifier: 6.2.0 + version: 6.2.0 sendmail: specifier: 1.6.1 version: 1.6.1 From d2fcb9370f525e224b2a12d03c119b39085f3c85 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 21:32:30 -0500 Subject: [PATCH 10/18] Add rule intent to metric-alert evaluate-group spans --- .../workflows/src/tasks/evaluate-metric-alert-rules.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts index 5909d1ffff..53399a2f9f 100644 --- a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts +++ b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts @@ -119,6 +119,13 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { 'target.id': representative.targetId, 'rules.in_group': groupRules.length, time_window_minutes: representative.timeWindowMinutes, + // Rule intent, so a slow trace is identifiable in TraceQL without + // reading db.query. `filter.applied` is the one we most needed. + 'metric.types': [...new Set(groupRules.map(r => r.type))], + 'threshold.types': [...new Set(groupRules.map(r => r.thresholdType))], + 'filter.applied': representative.savedFilterId != null, + 'window.needs_previous': needsPreviousWindow, + 'metric.needs_percentiles': needsPercentiles, }, }, async span => { From bb5fc080f3ef25f6d42c1d470fb81162219c929f Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Wed, 8 Jul 2026 22:18:48 -0500 Subject: [PATCH 11/18] Add filtered-vs-unfiltered rule gauges and dashboard panels --- .../grafana-dashboards/Metric-Alerts.json | 130 +++++++++++++++++- packages/services/workflows/src/metrics.ts | 14 ++ .../src/tasks/evaluate-metric-alert-rules.ts | 19 ++- 3 files changed, 155 insertions(+), 8 deletions(-) diff --git a/deployment/grafana-dashboards/Metric-Alerts.json b/deployment/grafana-dashboards/Metric-Alerts.json index 0825b5f254..c285a221b2 100644 --- a/deployment/grafana-dashboards/Metric-Alerts.json +++ b/deployment/grafana-dashboards/Metric-Alerts.json @@ -272,9 +272,125 @@ "title": "evaluateMetricAlertRules task duration (p99)", "type": "timeseries" }, + { + "datasource": { "type": "prometheus", "uid": "PROM_DATASOURCE_UID" }, + "description": "Enabled query groups (one ClickHouse query each) split by whether a saved filter is attached. Filtered groups read the heavier legacy tables (cost scales with the target's traffic cardinality), so watch this for growth in the more expensive with-filter population.", + "fieldConfig": { + "defaults": { + "color": { "mode": "palette-classic" }, + "custom": { + "axisCenteredZero": false, + "axisPlacement": "auto", + "axisSoftMin": 0, + "drawStyle": "line", + "fillOpacity": 10, + "lineInterpolation": "linear", + "lineWidth": 1, + "stacking": { "group": "A", "mode": "none" }, + "thresholdsStyle": { "mode": "off" } + }, + "mappings": [], + "min": 0, + "noValue": "0", + "thresholds": { "mode": "absolute", "steps": [{ "color": "green", "value": null }] }, + "unit": "short" + }, + "overrides": [ + { + "matcher": { "id": "byName", "options": "with filter" }, + "properties": [{ "id": "color", "value": { "fixedColor": "orange", "mode": "fixed" } }] + } + ] + }, + "gridPos": { "h": 8, "w": 12, "x": 0, "y": 16 }, + "id": 5, + "options": { + "legend": { "calcs": ["last", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true }, + "tooltip": { "mode": "multi", "sort": "desc" } + }, + "pluginVersion": "9.3.6", + "targets": [ + { + "datasource": { "type": "prometheus", "uid": "PROM_DATASOURCE_UID" }, + "editorMode": "code", + "expr": "hive_metric_alert_rule_groups{filtered=\"true\"}", + "legendFormat": "with filter", + "range": true, + "refId": "A" + }, + { + "datasource": { "type": "prometheus", "uid": "PROM_DATASOURCE_UID" }, + "editorMode": "code", + "expr": "hive_metric_alert_rule_groups{filtered=\"false\"}", + "legendFormat": "no filter", + "range": true, + "refId": "B" + } + ], + "title": "Alert query groups: filtered vs unfiltered", + "type": "timeseries" + }, + { + "datasource": { "type": "prometheus", "uid": "PROM_DATASOURCE_UID" }, + "description": "Enabled alert rules split by whether a saved filter is attached. Companion to the query-groups panel (a group can hold several rules that share one query).", + "fieldConfig": { + "defaults": { + "color": { "mode": "palette-classic" }, + "custom": { + "axisCenteredZero": false, + "axisPlacement": "auto", + "axisSoftMin": 0, + "drawStyle": "line", + "fillOpacity": 10, + "lineInterpolation": "linear", + "lineWidth": 1, + "stacking": { "group": "A", "mode": "none" }, + "thresholdsStyle": { "mode": "off" } + }, + "mappings": [], + "min": 0, + "noValue": "0", + "thresholds": { "mode": "absolute", "steps": [{ "color": "green", "value": null }] }, + "unit": "short" + }, + "overrides": [ + { + "matcher": { "id": "byName", "options": "with filter" }, + "properties": [{ "id": "color", "value": { "fixedColor": "orange", "mode": "fixed" } }] + } + ] + }, + "gridPos": { "h": 8, "w": 12, "x": 12, "y": 16 }, + "id": 6, + "options": { + "legend": { "calcs": ["last", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true }, + "tooltip": { "mode": "multi", "sort": "desc" } + }, + "pluginVersion": "9.3.6", + "targets": [ + { + "datasource": { "type": "prometheus", "uid": "PROM_DATASOURCE_UID" }, + "editorMode": "code", + "expr": "hive_metric_alert_enabled_rules{filtered=\"true\"}", + "legendFormat": "with filter", + "range": true, + "refId": "A" + }, + { + "datasource": { "type": "prometheus", "uid": "PROM_DATASOURCE_UID" }, + "editorMode": "code", + "expr": "hive_metric_alert_enabled_rules{filtered=\"false\"}", + "legendFormat": "no filter", + "range": true, + "refId": "B" + } + ], + "title": "Enabled rules: filtered vs unfiltered", + "type": "timeseries" + }, { "collapsed": false, - "gridPos": { "h": 1, "w": 24, "x": 0, "y": 16 }, + "gridPos": { "h": 1, "w": 24, "x": 0, "y": 24 }, "id": 100, "panels": [], "title": "Traces", @@ -296,7 +412,7 @@ }, "overrides": [] }, - "gridPos": { "h": 9, "w": 12, "x": 0, "y": 17 }, + "gridPos": { "h": 9, "w": 12, "x": 0, "y": 25 }, "id": 101, "options": { "cellHeight": "sm", @@ -334,7 +450,7 @@ }, "overrides": [] }, - "gridPos": { "h": 9, "w": 12, "x": 12, "y": 17 }, + "gridPos": { "h": 9, "w": 12, "x": 12, "y": 25 }, "id": 102, "options": { "cellHeight": "sm", @@ -389,7 +505,7 @@ } ] }, - "gridPos": { "h": 9, "w": 12, "x": 0, "y": 26 }, + "gridPos": { "h": 9, "w": 12, "x": 0, "y": 34 }, "id": 104, "options": { "legend": { @@ -443,7 +559,7 @@ }, "overrides": [] }, - "gridPos": { "h": 9, "w": 12, "x": 12, "y": 26 }, + "gridPos": { "h": 9, "w": 12, "x": 12, "y": 34 }, "id": 105, "options": { "legend": { @@ -497,7 +613,7 @@ }, "overrides": [] }, - "gridPos": { "h": 9, "w": 24, "x": 0, "y": 35 }, + "gridPos": { "h": 9, "w": 24, "x": 0, "y": 43 }, "id": 106, "options": { "legend": { @@ -541,7 +657,7 @@ }, "overrides": [] }, - "gridPos": { "h": 9, "w": 24, "x": 0, "y": 44 }, + "gridPos": { "h": 9, "w": 24, "x": 0, "y": 52 }, "id": 103, "options": { "cellHeight": "sm", diff --git a/packages/services/workflows/src/metrics.ts b/packages/services/workflows/src/metrics.ts index 27b1429a0a..d2ba965aae 100644 --- a/packages/services/workflows/src/metrics.ts +++ b/packages/services/workflows/src/metrics.ts @@ -54,3 +54,17 @@ export const metricAlertClickHouseQueryDuration = new metrics.Histogram({ buckets: [0.05, 0.1, 0.25, 0.5, 1, 2, 5, 10], labelNames: ['outcome'], }); + +// Enabled rules/query-groups split by whether a saved filter is attached. Set +// each tick from the full enabled inventory. Filtered rules read the heavier +// legacy tables, so this tracks how much of that expensive population exists. +export const metricAlertEnabledRules = new metrics.Gauge({ + name: 'hive_metric_alert_enabled_rules', + help: 'Enabled metric alert rules, by whether a saved filter is attached', + labelNames: ['filtered'], +}); +export const metricAlertRuleGroups = new metrics.Gauge({ + name: 'hive_metric_alert_rule_groups', + help: 'Metric alert query groups (one ClickHouse query each), by whether a saved filter is attached', + labelNames: ['filtered'], +}); diff --git a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts index 53399a2f9f..f598a580a7 100644 --- a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts +++ b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts @@ -11,6 +11,7 @@ import { isRuleDue, queryClickHouseWindows, } from '../lib/metric-alert-evaluator.js'; +import { metricAlertEnabledRules, metricAlertRuleGroups } from '../metrics.js'; // How many groups to evaluate in parallel. Each group = 1 ClickHouse round-trip // plus a per-rule state-machine evaluation that holds a Postgres connection for @@ -58,6 +59,23 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { // Evaluate every enabled rule. The org feature flag gates rule creation in // the API, not evaluation here; the per-rule `enabled` column is the gate. const rules = await fetchEnabledRules(context.pg); + const groups = groupRulesByQuery(rules); + + // Population gauges, refreshed every tick from the full enabled inventory so + // Grafana can watch the more expensive with-filter rules. Both label values + // are set (including 0) so neither retains a stale reading. + let filteredGroups = 0; + let filteredRules = 0; + for (const group of groups.values()) { + if (group[0].savedFilterId != null) { + filteredGroups += 1; + filteredRules += group.length; + } + } + metricAlertRuleGroups.set({ filtered: 'true' }, filteredGroups); + metricAlertRuleGroups.set({ filtered: 'false' }, groups.size - filteredGroups); + metricAlertEnabledRules.set({ filtered: 'true' }, filteredRules); + metricAlertEnabledRules.set({ filtered: 'false' }, rules.length - filteredRules); if (rules.length === 0) { logger.debug('No enabled metric alert rules found'); @@ -69,7 +87,6 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { // Evaluate only groups with at least one due member. Members of a group share // a window (so one cadence), and the batched UPDATE below keeps their // last_evaluated_at aligned, so a group is due as a unit. - const groups = groupRulesByQuery(rules); const dueGroupList = [...groups.values()].filter(group => group.some(rule => isRuleDue(rule, evaluationTime)), ); From a8c2e6c4d92bce605470cced6aaff608dcdb14b4 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Thu, 9 Jul 2026 09:39:52 -0500 Subject: [PATCH 12/18] prettier --- deployment/grafana-dashboards/Metric-Alerts.json | 14 ++++++++++++-- .../src/lib/metric-alert-evaluator.spec.ts | 11 +++++++---- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/deployment/grafana-dashboards/Metric-Alerts.json b/deployment/grafana-dashboards/Metric-Alerts.json index c285a221b2..d747294a6f 100644 --- a/deployment/grafana-dashboards/Metric-Alerts.json +++ b/deployment/grafana-dashboards/Metric-Alerts.json @@ -305,7 +305,12 @@ "gridPos": { "h": 8, "w": 12, "x": 0, "y": 16 }, "id": 5, "options": { - "legend": { "calcs": ["last", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true }, + "legend": { + "calcs": ["last", "max"], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, "tooltip": { "mode": "multi", "sort": "desc" } }, "pluginVersion": "9.3.6", @@ -363,7 +368,12 @@ "gridPos": { "h": 8, "w": 12, "x": 12, "y": 16 }, "id": 6, "options": { - "legend": { "calcs": ["last", "max"], "displayMode": "table", "placement": "bottom", "showLegend": true }, + "legend": { + "calcs": ["last", "max"], + "displayMode": "table", + "placement": "bottom", + "showLegend": true + }, "tooltip": { "mode": "multi", "sort": "desc" } }, "pluginVersion": "9.3.6", diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index 9b94224ad4..7d7c0a0b22 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -97,9 +97,9 @@ describe('isRuleDue', () => { const ago = (minutes: number) => new Date(evalTime.getTime() - minutes * 60_000).toISOString(); test('a never-evaluated rule is always due', () => { - expect( - isRuleDue(makeRule({ lastEvaluatedAt: null, timeWindowMinutes: 43200 }), evalTime), - ).toBe(true); + expect(isRuleDue(makeRule({ lastEvaluatedAt: null, timeWindowMinutes: 43200 }), evalTime)).toBe( + true, + ); }); test('30-day rule: due once its 30-min interval has elapsed', () => { @@ -113,7 +113,10 @@ describe('isRuleDue', () => { test('sub-second tolerance only: 5s short of the interval is still not due', () => { const almost = new Date(evalTime.getTime() - (30 * 60_000 - 5_000)).toISOString(); expect( - isRuleDue(makeRule({ timeWindowMinutes: 43200, state: 'NORMAL', lastEvaluatedAt: almost }), evalTime), + isRuleDue( + makeRule({ timeWindowMinutes: 43200, state: 'NORMAL', lastEvaluatedAt: almost }), + evalTime, + ), ).toBe(false); }); From 20e753f2524b0b002310b6b47b9fceaa4bb602c8 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Thu, 9 Jul 2026 10:51:27 -0500 Subject: [PATCH 13/18] Count actually-filtering rules in the metric-alert filter gauges --- .../src/tasks/evaluate-metric-alert-rules.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts index f598a580a7..ee0d83bf55 100644 --- a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts +++ b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts @@ -61,13 +61,16 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { const rules = await fetchEnabledRules(context.pg); const groups = groupRulesByQuery(rules); - // Population gauges, refreshed every tick from the full enabled inventory so - // Grafana can watch the more expensive with-filter rules. Both label values - // are set (including 0) so neither retains a stale reading. + // Population gauges, refreshed every tick so Grafana can watch the expensive + // with-filter rules. "Filtered" means the saved filter actually yields query + // conditions (the heavier legacy-table path), not merely that a saved_filter_id + // is set; a dangling or empty filter yields no conditions and evaluates + // unfiltered. Both label values are set each tick (including 0) so neither + // retains a stale reading. let filteredGroups = 0; let filteredRules = 0; for (const group of groups.values()) { - if (group[0].savedFilterId != null) { + if (buildSavedFilterConditions(group[0].savedFilterFilters, logger).length > 0) { filteredGroups += 1; filteredRules += group.length; } From fbef784eb5073ed8cbb09951bf8ceef9b502d78f Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Thu, 9 Jul 2026 12:06:35 -0500 Subject: [PATCH 14/18] Derive metric-alert group query needs in one helper --- .../src/lib/metric-alert-evaluator.spec.ts | 137 ++++++++++++++++-- .../src/lib/metric-alert-evaluator.ts | 39 ++++- .../src/tasks/evaluate-metric-alert-rules.ts | 31 +--- 3 files changed, 164 insertions(+), 43 deletions(-) diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index 7d7c0a0b22..0bd7c0fbb2 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -3,10 +3,12 @@ import { printWithValues, type SqlValue } from '@hive/clickhouse'; import type { ClickHouseClient } from './clickhouse-client.js'; import { buildSavedFilterConditions, + deriveGroupNeeds, evaluationIntervalMinutes, groupRulesByQuery, isRuleDue, queryClickHouseWindows, + type GroupNeeds, type MetricAlertRuleRow, } from './metric-alert-evaluator.js'; @@ -199,6 +201,63 @@ describe('buildSavedFilterConditions', () => { }); }); +describe('deriveGroupNeeds', () => { + const { logger } = makeLogger(); + + test('error/traffic-only group needs neither duration column nor previous window', () => { + const n = deriveGroupNeeds( + [makeRule({ type: 'ERROR_RATE' }), makeRule({ type: 'TRAFFIC' })], + logger, + ); + expect(n).toMatchObject({ + needsPreviousWindow: false, + needsAverage: false, + needsPercentiles: false, + }); + }); + + test('a single PERCENTAGE_CHANGE rule flips the whole group to fetch the previous window', () => { + const n = deriveGroupNeeds( + [ + makeRule({ thresholdType: 'FIXED_VALUE' }), + makeRule({ thresholdType: 'PERCENTAGE_CHANGE' }), + ], + logger, + ); + expect(n.needsPreviousWindow).toBe(true); + }); + + test('LATENCY metrics select their columns; AVG and percentiles are independent', () => { + expect(deriveGroupNeeds([makeRule({ type: 'LATENCY', metric: 'AVG' })], logger)).toMatchObject({ + needsAverage: true, + needsPercentiles: false, + }); + expect(deriveGroupNeeds([makeRule({ type: 'LATENCY', metric: 'P95' })], logger)).toMatchObject({ + needsAverage: false, + needsPercentiles: true, + }); + // A mixed AVG + percentile latency group needs both. + expect( + deriveGroupNeeds( + [ + makeRule({ type: 'LATENCY', metric: 'AVG' }), + makeRule({ type: 'LATENCY', metric: 'P99' }), + ], + logger, + ), + ).toMatchObject({ needsAverage: true, needsPercentiles: true }); + }); + + test('builds filter conditions from the representative saved filter', () => { + const filters = { clientFilters: [{ name: 'web', versions: null }] }; + const filtered = deriveGroupNeeds([makeRule({ savedFilterFilters: filters })], logger); + expect(filtered.filterConditions.length).toBeGreaterThan(0); + expect( + deriveGroupNeeds([makeRule({ savedFilterFilters: null })], logger).filterConditions, + ).toEqual([]); + }); +}); + describe('queryClickHouseWindows', () => { function captureClient() { const calls: Array<{ sql: string; queryId: string; params?: Record }> = []; @@ -214,9 +273,19 @@ describe('queryClickHouseWindows', () => { const evalTime = new Date('2024-06-01T12:00:00.000Z'); const target = '11111111-1111-1111-1111-111111111111'; + // Full query shape by default (both windows, both duration columns, no filter); + // override just the field a test exercises. + const needs = (overrides: Partial = {}): GroupNeeds => ({ + filterConditions: [], + needsPreviousWindow: true, + needsAverage: true, + needsPercentiles: true, + ...overrides, + }); + test('no filter -> target-keyed minutely rollup, no hash/client predicate, only target bound', async () => { const { clickhouse, calls } = captureClient(); - await queryClickHouseWindows(clickhouse, target, 60, [], evalTime); + await queryClickHouseWindows(clickhouse, target, 60, evalTime, needs()); const { sql, params } = calls[0]; // Unfiltered -> the target-keyed rollup. It dropped the hash/client // dimensions, so the query must emit no hash/client predicate. @@ -235,7 +304,13 @@ describe('queryClickHouseWindows', () => { { clientFilters: [{ name: 'web', versions: null }] }, makeLogger().logger, ); - await queryClickHouseWindows(clickhouse, target, 60, conds, evalTime); + await queryClickHouseWindows( + clickhouse, + target, + 60, + evalTime, + needs({ filterConditions: conds }), + ); const { sql, params } = calls[0]; // A filtered query MUST stay on the legacy table — the rollup has no // hash/client columns to predicate on. @@ -252,16 +327,16 @@ describe('queryClickHouseWindows', () => { test('window > 360 minutes reads the hourly rollup', async () => { const { clickhouse, calls } = captureClient(); - await queryClickHouseWindows(clickhouse, target, 720, [], evalTime); + await queryClickHouseWindows(clickhouse, target, 720, evalTime, needs()); expect(calls[0].sql).toContain('FROM operations_by_target_hourly'); }); test('windows below 7 days stay on the hourly rollup', async () => { const { clickhouse, calls } = captureClient(); // 1 day, 3 days, and one minute under the 7-day cutoff all read hourly. - await queryClickHouseWindows(clickhouse, target, 1440, [], evalTime); - await queryClickHouseWindows(clickhouse, target, 4320, [], evalTime); - await queryClickHouseWindows(clickhouse, target, 10079, [], evalTime); + await queryClickHouseWindows(clickhouse, target, 1440, evalTime, needs()); + await queryClickHouseWindows(clickhouse, target, 4320, evalTime, needs()); + await queryClickHouseWindows(clickhouse, target, 10079, evalTime, needs()); for (const call of calls) { expect(call.sql).toContain('FROM operations_by_target_hourly'); } @@ -269,7 +344,7 @@ describe('queryClickHouseWindows', () => { test('windows >= 7 days read the daily rollup (unfiltered -> by_target)', async () => { const { clickhouse, calls } = captureClient(); - await queryClickHouseWindows(clickhouse, target, 10080, [], evalTime); + await queryClickHouseWindows(clickhouse, target, 10080, evalTime, needs()); const { sql } = calls[0]; expect(sql).toContain('FROM operations_by_target_daily'); expect(sql).toContain('quantilesTDigestMerge('); @@ -281,7 +356,13 @@ describe('queryClickHouseWindows', () => { { clientFilters: [{ name: 'web', versions: null }] }, makeLogger().logger, ); - await queryClickHouseWindows(clickhouse, target, 43200, conds, evalTime); + await queryClickHouseWindows( + clickhouse, + target, + 43200, + evalTime, + needs({ filterConditions: conds }), + ); const { sql } = calls[0]; expect(sql).toContain('FROM operations_daily'); expect(sql).not.toContain('_by_target'); @@ -292,7 +373,13 @@ describe('queryClickHouseWindows', () => { test('absolute-only groups skip the previous window (1x scan, constant label)', async () => { const { clickhouse, calls } = captureClient(); - const result = await queryClickHouseWindows(clickhouse, target, 60, [], evalTime, false); + const result = await queryClickHouseWindows( + clickhouse, + target, + 60, + evalTime, + needs({ needsPreviousWindow: false }), + ); const { sql } = calls[0]; // Single-window query: constant 'current' label, no previous branch. expect(sql).toContain("'current' as window"); @@ -309,7 +396,13 @@ describe('queryClickHouseWindows', () => { test('groups needing the previous window fetch both (default)', async () => { const { clickhouse, calls } = captureClient(); - await queryClickHouseWindows(clickhouse, target, 60, [], evalTime, true); + await queryClickHouseWindows( + clickhouse, + target, + 60, + evalTime, + needs({ needsPreviousWindow: true }), + ); const { sql } = calls[0]; expect(sql).toContain("ELSE 'previous'"); const anchor = evalTime.getTime(); @@ -319,7 +412,13 @@ describe('queryClickHouseWindows', () => { test('groups needing neither duration column select NULL placeholders', async () => { const { clickhouse, calls } = captureClient(); - await queryClickHouseWindows(clickhouse, target, 60, [], evalTime, true, false, false); + await queryClickHouseWindows( + clickhouse, + target, + 60, + evalTime, + needs({ needsAverage: false, needsPercentiles: false }), + ); const { sql } = calls[0]; expect(sql).toContain('NULL as average'); expect(sql).toContain('NULL as percentiles'); @@ -329,7 +428,13 @@ describe('queryClickHouseWindows', () => { test('percentile groups select the quantiles column; avg still skipped', async () => { const { clickhouse, calls } = captureClient(); - await queryClickHouseWindows(clickhouse, target, 60, [], evalTime, true, false, true); + await queryClickHouseWindows( + clickhouse, + target, + 60, + evalTime, + needs({ needsAverage: false, needsPercentiles: true }), + ); const { sql } = calls[0]; expect(sql).toContain('duration_quantiles'); expect(sql).toContain('NULL as average'); @@ -337,7 +442,13 @@ describe('queryClickHouseWindows', () => { test('avg groups select avgMerge; percentiles skipped', async () => { const { clickhouse, calls } = captureClient(); - await queryClickHouseWindows(clickhouse, target, 60, [], evalTime, true, true, false); + await queryClickHouseWindows( + clickhouse, + target, + 60, + evalTime, + needs({ needsAverage: true, needsPercentiles: false }), + ); const { sql } = calls[0]; expect(sql).toContain('avgMerge(duration_avg) as average'); expect(sql).toContain('NULL as percentiles'); diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.ts index 9a4d847623..ab3432da0d 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.ts @@ -285,23 +285,48 @@ export function buildSavedFilterConditions(rawFilters: unknown, logger: Logger): }); } +// The window query's shape, derived once from a group's rules. Metric and +// threshold type aren't part of the group key, so a group can mix them; each +// flag is the union across the group (fetch/compute if ANY rule needs it). +export type GroupNeeds = { + // Saved-filter predicates. Empty = unfiltered (reads the cheaper by_target + // rollups); non-empty forces the legacy tables that keep hash/client dims. + filterConditions: SqlValue[]; + // Any PERCENTAGE_CHANGE rule needs the prior window to compare against; + // absolute-only groups skip it (half the scan) and persist previousValue null. + needsPreviousWindow: boolean; + // Which duration columns a LATENCY rule reads. A group with none skips both, + // so ClickHouse never touches the heavy duration_quantiles blob. + needsAverage: boolean; + needsPercentiles: boolean; +}; + +// All rules in a group share a saved filter (it's part of the group key), so the +// filter is built once from the representative. A malformed filter yields no +// conditions (evaluates unfiltered) and is logged, isolating the failure here. +export function deriveGroupNeeds(groupRules: MetricAlertRuleRow[], logger: Logger): GroupNeeds { + return { + filterConditions: buildSavedFilterConditions(groupRules[0].savedFilterFilters, logger), + needsPreviousWindow: groupRules.some(r => r.thresholdType === 'PERCENTAGE_CHANGE'), + needsAverage: groupRules.some(r => r.type === 'LATENCY' && r.metric === 'AVG'), + needsPercentiles: groupRules.some(r => r.type === 'LATENCY' && r.metric !== 'AVG'), + }; +} + export async function queryClickHouseWindows( clickhouse: ClickHouseClient, targetId: string, timeWindowMinutes: number, - filterConditions: SqlValue[], // Anchor windows to the cron's scheduled run time (graphile-worker's // job.run_at), not wall-clock now. If the worker is backed up and picks // this job up late, using wall-clock would shift the queried window // forward and could miss the spike that should have fired the alert. evaluationTime: Date, - // False = scan only the current window, return previous = null. Default true. - needsPreviousWindow: boolean = true, - // False = select `NULL as ` so ClickHouse doesn't read that duration - // column. Default true. The caller decides all three per group (processGroup). - needsAverage: boolean = true, - needsPercentiles: boolean = true, + // What the group's rules actually read, so the query scans the fewest + // windows/columns. Derived once per group by deriveGroupNeeds. + needs: GroupNeeds, ): Promise<{ current: ClickHouseWindowRow | null; previous: ClickHouseWindowRow | null }> { + const { filterConditions, needsPreviousWindow, needsAverage, needsPercentiles } = needs; const anchorMs = evaluationTime.getTime(); const offsetMs = 60_000; const windowMs = timeWindowMinutes * 60_000; diff --git a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts index ee0d83bf55..2395d3addb 100644 --- a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts +++ b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts @@ -4,7 +4,7 @@ import { psql } from '@hive/postgres'; import { SpanKind, SpanStatusCode, trace } from '@hive/service-common'; import { defineTask, implementTask } from '../kit.js'; import { - buildSavedFilterConditions, + deriveGroupNeeds, evaluateRule, fetchEnabledRules, groupRulesByQuery, @@ -70,7 +70,7 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { let filteredGroups = 0; let filteredRules = 0; for (const group of groups.values()) { - if (buildSavedFilterConditions(group[0].savedFilterFilters, logger).length > 0) { + if (deriveGroupNeeds(group, logger).filterConditions.length > 0) { filteredGroups += 1; filteredRules += group.length; } @@ -108,23 +108,11 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { }> { const representative = groupRules[0]; - // All rules in a group share the same saved filter (it's part of the group key), - // so build the ClickHouse conditions once from the representative. - // A malformed filter yields no conditions (evaluates unfiltered) and is logged, - // isolating the failure to this group. - const filterConditions = buildSavedFilterConditions(representative.savedFilterFilters, logger); - - // Only PERCENTAGE_CHANGE rules compare against the prior window. If no rule - // in the group is one, skip fetching it (half the scan) and persist a null - // previousValue. Any percentage-change rule flips the whole group back to - // fetching both windows. - const needsPreviousWindow = groupRules.some(r => r.thresholdType === 'PERCENTAGE_CHANGE'); - - // Only LATENCY rules read the duration columns; skip the heavy percentile - // column unless a percentile-metric LATENCY rule is present, and avgMerge - // unless an AVG-metric one is. Pure error/traffic groups fetch neither. - const needsPercentiles = groupRules.some(r => r.type === 'LATENCY' && r.metric !== 'AVG'); - const needsAverage = groupRules.some(r => r.type === 'LATENCY' && r.metric === 'AVG'); + // Derive the query shape once: the saved filter, whether to fetch the prior + // window, and which duration columns to read. Shared with the gauge loop + // above so "what a group needs" lives in one place. + const needs = deriveGroupNeeds(groupRules, logger); + const { needsPreviousWindow, needsPercentiles } = needs; // startActiveSpan makes this span the current OTel context for the // duration of the callback, so the slonik PG interceptor and the @@ -156,11 +144,8 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { clickhouse, representative.targetId, representative.timeWindowMinutes, - filterConditions, evaluationTime, - needsPreviousWindow, - needsAverage, - needsPercentiles, + needs, ); } catch (error) { logger.error( From bd19727a788ba678c08fc8df178f609102740377 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Thu, 9 Jul 2026 14:28:16 -0500 Subject: [PATCH 15/18] const MINUTES_PER_DAY and pin the daily-rollup threshold --- .../providers/metric-alert-rules-manager.ts | 3 ++- .../api/src/modules/commerce/constants.ts | 9 ++++--- .../src/lib/metric-alert-evaluator.spec.ts | 25 ++++++++++++++++--- .../src/lib/metric-alert-evaluator.ts | 7 +++++- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts b/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts index e3cd93a43a..cb73d7cf8c 100644 --- a/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts +++ b/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts @@ -7,6 +7,7 @@ import { METRIC_ALERT_RULE_TIME_WINDOW_MAX_MINUTES, METRIC_ALERT_RULE_TIME_WINDOW_MIN_MINUTES, METRIC_ALERT_RULES_PER_TARGET_LIMIT, + MINUTES_PER_DAY, } from '../../commerce/constants'; import { OrganizationManager } from '../../organization/providers/organization-manager'; import { Logger } from '../../shared/providers/logger'; @@ -344,7 +345,7 @@ export class MetricAlertRulesManager { // rounded. The UI presets already satisfy this; this guards direct API callers. if ( timeWindowMinutes >= METRIC_ALERT_RULE_DAILY_ROLLUP_THRESHOLD_MINUTES && - timeWindowMinutes % (24 * 60) !== 0 + timeWindowMinutes % MINUTES_PER_DAY !== 0 ) { throw new MetricAlertRuleValidationError( 'Time windows of 7 days or more must be a whole number of days.', diff --git a/packages/services/api/src/modules/commerce/constants.ts b/packages/services/api/src/modules/commerce/constants.ts index b365c820d5..dc7a6f2185 100644 --- a/packages/services/api/src/modules/commerce/constants.ts +++ b/packages/services/api/src/modules/commerce/constants.ts @@ -51,11 +51,12 @@ export const METRIC_ALERT_RULES_PER_TARGET_LIMIT = 10; * reasons; these constants are the absolute bounds the API enforces against * any caller (form, seed scripts, customer integrations). */ +export const MINUTES_PER_DAY = 24 * 60; // 1440 export const METRIC_ALERT_RULE_TIME_WINDOW_MIN_MINUTES = 1; -export const METRIC_ALERT_RULE_TIME_WINDOW_MAX_MINUTES = 30 * 24 * 60; // 43200 +export const METRIC_ALERT_RULE_TIME_WINDOW_MAX_MINUTES = 30 * MINUTES_PER_DAY; // 43200 // Windows at or above this read the daily ClickHouse rollup, whose buckets are // whole days. A window this size must therefore be a whole number of days -// (multiple of 1440 min) or the daily aggregate would silently round it. Mirrors -// DAILY_THRESHOLD_MINUTES in the workflows evaluator; keep the two in sync. -export const METRIC_ALERT_RULE_DAILY_ROLLUP_THRESHOLD_MINUTES = 7 * 24 * 60; // 10080 +// (a multiple of MINUTES_PER_DAY) or the daily aggregate would silently round it. +// Mirrors DAILY_THRESHOLD_MINUTES in the workflows evaluator; keep the two in sync. +export const METRIC_ALERT_RULE_DAILY_ROLLUP_THRESHOLD_MINUTES = 7 * MINUTES_PER_DAY; // 10080 diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index 0bd7c0fbb2..de65cbd8b4 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -3,6 +3,7 @@ import { printWithValues, type SqlValue } from '@hive/clickhouse'; import type { ClickHouseClient } from './clickhouse-client.js'; import { buildSavedFilterConditions, + DAILY_THRESHOLD_MINUTES, deriveGroupNeeds, evaluationIntervalMinutes, groupRulesByQuery, @@ -92,6 +93,18 @@ describe('evaluationIntervalMinutes', () => { }); }); +describe('DAILY_THRESHOLD_MINUTES', () => { + test('is exactly 7 whole days, matching the API validation cutoff', () => { + // Pins the routing cutoff. It must equal + // METRIC_ALERT_RULE_DAILY_ROLLUP_THRESHOLD_MINUTES in the api commerce + // constants — the two live in separate packages (workflows can't import + // api), so a change here without the API side would let a non-whole-day + // window route to the daily rollup and silently round. + expect(DAILY_THRESHOLD_MINUTES).toBe(7 * 24 * 60); + expect(DAILY_THRESHOLD_MINUTES).toBe(10080); + }); +}); + describe('isRuleDue', () => { const evalTime = new Date('2026-07-08T12:00:00.000Z'); // ISO timestamp for a point `minutes` before evalTime (mirrors the `to_json` @@ -333,10 +346,16 @@ describe('queryClickHouseWindows', () => { test('windows below 7 days stay on the hourly rollup', async () => { const { clickhouse, calls } = captureClient(); - // 1 day, 3 days, and one minute under the 7-day cutoff all read hourly. + // 1 day, 3 days, and one minute under the cutoff all read hourly. await queryClickHouseWindows(clickhouse, target, 1440, evalTime, needs()); await queryClickHouseWindows(clickhouse, target, 4320, evalTime, needs()); - await queryClickHouseWindows(clickhouse, target, 10079, evalTime, needs()); + await queryClickHouseWindows( + clickhouse, + target, + DAILY_THRESHOLD_MINUTES - 1, + evalTime, + needs(), + ); for (const call of calls) { expect(call.sql).toContain('FROM operations_by_target_hourly'); } @@ -344,7 +363,7 @@ describe('queryClickHouseWindows', () => { test('windows >= 7 days read the daily rollup (unfiltered -> by_target)', async () => { const { clickhouse, calls } = captureClient(); - await queryClickHouseWindows(clickhouse, target, 10080, evalTime, needs()); + await queryClickHouseWindows(clickhouse, target, DAILY_THRESHOLD_MINUTES, evalTime, needs()); const { sql } = calls[0]; expect(sql).toContain('FROM operations_by_target_daily'); expect(sql).toContain('quantilesTDigestMerge('); diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.ts index ab3432da0d..c49b80578a 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.ts @@ -70,12 +70,17 @@ function makeGroupKey(rule: MetricAlertRuleRow): GroupKey { // nanoseconds; latency rule thresholds and all display surfaces use ms. const NS_TO_MS = 1e6; +const MINUTES_PER_DAY = 24 * 60; // 1440 + // Windows >= 7 days read the daily rollups (operations_by_target_daily / // operations_daily) instead of hourly, so a 30-day query scans ~60 daily buckets // instead of ~1,440 hourly. Below 7d stays on hourly/minutely for exact sub-day // precision. The API keeps windows at/above this a whole number of days (see // assertTimeWindowInRange) so daily buckets don't silently round the window. -const DAILY_THRESHOLD_MINUTES = 7 * 24 * 60; // 10080 +// Exported so the routing tests pin their boundary to it. Must equal the API's +// METRIC_ALERT_RULE_DAILY_ROLLUP_THRESHOLD_MINUTES (separate package, can't +// import); keep the two in sync. +export const DAILY_THRESHOLD_MINUTES = 7 * MINUTES_PER_DAY; // 10080 function extractMetricValue(row: ClickHouseWindowRow, rule: MetricAlertRuleRow): number { const total = Number(row.total); From 816fdbcc89b28ba598769cccfd47113aa4da6649 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Thu, 9 Jul 2026 14:34:49 -0500 Subject: [PATCH 16/18] Fail loud when a latency alert reads an unselected column --- .../src/lib/metric-alert-evaluator.spec.ts | 46 ++++++++++++++++ .../src/lib/metric-alert-evaluator.ts | 55 +++++++++++++------ 2 files changed, 84 insertions(+), 17 deletions(-) diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index de65cbd8b4..eccba57311 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -6,6 +6,7 @@ import { DAILY_THRESHOLD_MINUTES, deriveGroupNeeds, evaluationIntervalMinutes, + extractMetricValue, groupRulesByQuery, isRuleDue, queryClickHouseWindows, @@ -214,6 +215,51 @@ describe('buildSavedFilterConditions', () => { }); }); +describe('extractMetricValue', () => { + const row = (over: Partial[0]> = {}) => ({ + window: 'current' as const, + total: '1000', + total_ok: '900', + average: null, + percentiles: null, + ...over, + }); + + test('TRAFFIC returns the total count', () => { + expect(extractMetricValue(row(), makeRule({ type: 'TRAFFIC' }))).toBe(1000); + }); + + test('ERROR_RATE returns the error percentage (0 when no traffic)', () => { + expect(extractMetricValue(row(), makeRule({ type: 'ERROR_RATE' }))).toBeCloseTo(10); + expect( + extractMetricValue(row({ total: '0', total_ok: '0' }), makeRule({ type: 'ERROR_RATE' })), + ).toBe(0); + }); + + test('LATENCY converts the selected column from nanoseconds to ms', () => { + const avg = extractMetricValue( + row({ average: 1.2e9 }), + makeRule({ type: 'LATENCY', metric: 'AVG' }), + ); + expect(avg).toBe(1200); + // percentiles tuple is [P75, P90, P95, P99]; P95 is index 2. + const p95 = extractMetricValue( + row({ percentiles: [1e9, 2e9, 3e9, 4e9] }), + makeRule({ type: 'LATENCY', metric: 'P95' }), + ); + expect(p95).toBe(3000); + }); + + test('a LATENCY rule whose duration column was not selected throws (no silent 0)', () => { + expect(() => + extractMetricValue(row({ average: null }), makeRule({ type: 'LATENCY', metric: 'AVG' })), + ).toThrow(/duration_avg/); + expect(() => + extractMetricValue(row({ percentiles: null }), makeRule({ type: 'LATENCY', metric: 'P95' })), + ).toThrow(/duration_quantiles/); + }); +}); + describe('deriveGroupNeeds', () => { const { logger } = makeLogger(); diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.ts index c49b80578a..7b681b339c 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.ts @@ -82,7 +82,30 @@ const MINUTES_PER_DAY = 24 * 60; // 1440 // import); keep the two in sync. export const DAILY_THRESHOLD_MINUTES = 7 * MINUTES_PER_DAY; // 10080 -function extractMetricValue(row: ClickHouseWindowRow, rule: MetricAlertRuleRow): number { +// A LATENCY rule forces its duration column into the SELECT (see deriveGroupNeeds), +// so a null column here means the query shape and the rule disagree: a routing or +// column-selection bug, never real data (an empty window synthesizes zeros, not +// null). Throw so it's caught per-group, logged, and retried next tick, instead of +// reading 0, which renders as 0ms and would silently keep a latency alert from firing. +function requireColumn(value: T | null, column: string, rule: MetricAlertRuleRow): T { + if (value === null) { + throw new Error( + `Metric alert rule ${rule.id} needs column "${column}" but its window query did not select it`, + ); + } + return value; +} + +const PERCENTILE_INDEX = { P75: 0, P90: 1, P95: 2, P99: 3 } as const; + +function percentileIndex(metric: MetricAlertRuleRow['metric']): number { + if (metric && metric in PERCENTILE_INDEX) { + return PERCENTILE_INDEX[metric as keyof typeof PERCENTILE_INDEX]; + } + throw new Error(`Expected a percentile metric (P75-P99), got ${metric}`); +} + +export function extractMetricValue(row: ClickHouseWindowRow, rule: MetricAlertRuleRow): number { const total = Number(row.total); const totalOk = Number(row.total_ok); @@ -92,22 +115,20 @@ function extractMetricValue(row: ClickHouseWindowRow, rule: MetricAlertRuleRow): case 'ERROR_RATE': return total > 0 ? ((total - totalOk) / total) * 100 : 0; case 'LATENCY': { - // A LATENCY rule forces its column to be selected, so these are non-null - // when actually read; the `?? 0` is just for the nullable type. - const metricMap: Record = { - AVG: row.average ?? 0, - P75: row.percentiles?.[0] ?? 0, - P90: row.percentiles?.[1] ?? 0, - P95: row.percentiles?.[2] ?? 0, - P99: row.percentiles?.[3] ?? 0, - }; - // ClickHouse stores `duration` in nanoseconds, but rule thresholds (the - // form input) and every display surface are in - // milliseconds. Convert here so the threshold comparison and the persisted - // value are in milliseconds and consistent with all of them. Without this, - // a ns value (~1.2e9) is compared against a ms threshold (e.g. 4000), - // so any non-trivial latency trips the rule. - return (rule.metric ? metricMap[rule.metric] : 0) / NS_TO_MS; + // Read only the column this metric needs (the other may be a NULL placeholder), + // and require it to be present (see requireColumn). + const ns = + rule.metric === 'AVG' + ? requireColumn(row.average, 'duration_avg', rule) + : requireColumn(row.percentiles, 'duration_quantiles', rule)[ + percentileIndex(rule.metric) + ]; + // ClickHouse stores `duration` in nanoseconds, but rule thresholds (the form + // input) and every display surface are in milliseconds. Convert here so the + // threshold comparison and the persisted value are in ms. Without this, a ns + // value (~1.2e9) is compared against a ms threshold (e.g. 4000), so any + // non-trivial latency trips the rule. + return ns / NS_TO_MS; } } } From 868cbf6c3140fda96dc2c71a81cd495547283d92 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Thu, 9 Jul 2026 14:50:59 -0500 Subject: [PATCH 17/18] Keep traffic on hourly and split alert groups by rollup tier --- .../src/lib/metric-alert-evaluator.spec.ts | 70 +++++++++++++++++++ .../src/lib/metric-alert-evaluator.ts | 41 ++++++++--- 2 files changed, 103 insertions(+), 8 deletions(-) diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index eccba57311..9331cf0b4d 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -10,6 +10,7 @@ import { groupRulesByQuery, isRuleDue, queryClickHouseWindows, + resolutionFor, type GroupNeeds, type MetricAlertRuleRow, } from './metric-alert-evaluator.js'; @@ -73,6 +74,44 @@ describe('groupRulesByQuery', () => { ]); expect(groups.size).toBe(2); }); + + test('at >= 7d a TRAFFIC rule and a latency rule split into hourly + daily groups', () => { + const groups = groupRulesByQuery([ + makeRule({ id: 'a', type: 'TRAFFIC', timeWindowMinutes: 43200 }), + makeRule({ id: 'b', type: 'LATENCY', metric: 'P95', timeWindowMinutes: 43200 }), + ]); + expect(groups.size).toBe(2); + // Not just that they split, but onto the two intended tiers: TRAFFIC -> hourly + // (exact counts), latency -> daily (cheap). + const tiers = [...groups.values()] + .map(g => resolutionFor(g[0].timeWindowMinutes, g[0].type !== 'TRAFFIC')) + .sort(); + expect(tiers).toEqual(['daily', 'hourly']); + }); + + test('below 7d a TRAFFIC rule and a latency rule share a group (same tier)', () => { + const groups = groupRulesByQuery([ + makeRule({ id: 'a', type: 'TRAFFIC', timeWindowMinutes: 720 }), + makeRule({ id: 'b', type: 'LATENCY', metric: 'P95', timeWindowMinutes: 720 }), + ]); + expect(groups.size).toBe(1); + }); +}); + +describe('resolutionFor', () => { + test('tiers by window, with TRAFFIC pinned off the daily rollup', () => { + expect(resolutionFor(60, true)).toBe('minutely'); + expect(resolutionFor(360, true)).toBe('minutely'); + expect(resolutionFor(720, true)).toBe('hourly'); + expect(resolutionFor(DAILY_THRESHOLD_MINUTES - 1, true)).toBe('hourly'); + expect(resolutionFor(DAILY_THRESHOLD_MINUTES, true)).toBe('daily'); + // allowDailyRollup=false (a TRAFFIC group) never reaches daily. + expect(resolutionFor(DAILY_THRESHOLD_MINUTES, false)).toBe('hourly'); + expect(resolutionFor(43200, false)).toBe('hourly'); + // Below the daily tier the flag is irrelevant. + expect(resolutionFor(720, false)).toBe('hourly'); + expect(resolutionFor(60, false)).toBe('minutely'); + }); }); describe('evaluationIntervalMinutes', () => { @@ -307,6 +346,23 @@ describe('deriveGroupNeeds', () => { ).toMatchObject({ needsAverage: true, needsPercentiles: true }); }); + test('any TRAFFIC rule blocks the daily rollup; latency/error-rate allow it', () => { + expect(deriveGroupNeeds([makeRule({ type: 'TRAFFIC' })], logger).allowDailyRollup).toBe(false); + // A TRAFFIC rule sharing a group with a latency rule still blocks it (union). + expect( + deriveGroupNeeds( + [makeRule({ type: 'LATENCY', metric: 'P95' }), makeRule({ type: 'TRAFFIC' })], + logger, + ).allowDailyRollup, + ).toBe(false); + expect( + deriveGroupNeeds( + [makeRule({ type: 'LATENCY', metric: 'AVG' }), makeRule({ type: 'ERROR_RATE' })], + logger, + ).allowDailyRollup, + ).toBe(true); + }); + test('builds filter conditions from the representative saved filter', () => { const filters = { clientFilters: [{ name: 'web', versions: null }] }; const filtered = deriveGroupNeeds([makeRule({ savedFilterFilters: filters })], logger); @@ -339,6 +395,7 @@ describe('queryClickHouseWindows', () => { needsPreviousWindow: true, needsAverage: true, needsPercentiles: true, + allowDailyRollup: true, ...overrides, }); @@ -415,6 +472,19 @@ describe('queryClickHouseWindows', () => { expect(sql).toContain('quantilesTDigestMerge('); }); + test('a group with a TRAFFIC rule stays on hourly at >= 7 days (exact counts, no daily rounding)', async () => { + const { clickhouse, calls } = captureClient(); + await queryClickHouseWindows( + clickhouse, + target, + DAILY_THRESHOLD_MINUTES, + evalTime, + needs({ allowDailyRollup: false }), + ); + expect(calls[0].sql).toContain('FROM operations_by_target_hourly'); + expect(calls[0].sql).not.toContain('_daily'); + }); + test('windows >= 7 days read the daily rollup (filtered -> legacy operations_daily)', async () => { const { clickhouse, calls } = captureClient(); const conds = buildSavedFilterConditions( diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.ts index 7b681b339c..1192f40cb3 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.ts @@ -63,7 +63,13 @@ type ClickHouseWindowRow = z.infer; type GroupKey = string; function makeGroupKey(rule: MetricAlertRuleRow): GroupKey { - return `${rule.targetId}:${rule.timeWindowMinutes}:${rule.savedFilterId ?? ''}`; + // Include the resolved rollup tier so rules that must read different tables + // never share one query: a >= 7d TRAFFIC rule (hourly, for exact counts) and a + // >= 7d latency rule (daily, cheap) on the same target/window/filter split into + // separate groups, instead of the latency rule being dragged onto the slow + // hourly scan. Tiers only differ at >= 7d, so shorter windows still share. + const resolution = resolutionFor(rule.timeWindowMinutes, rule.type !== 'TRAFFIC'); + return `${rule.targetId}:${rule.timeWindowMinutes}:${rule.savedFilterId ?? ''}:${resolution}`; } // Nanoseconds per millisecond. ClickHouse stores operation durations in @@ -82,6 +88,18 @@ const MINUTES_PER_DAY = 24 * 60; // 1440 // import); keep the two in sync. export const DAILY_THRESHOLD_MINUTES = 7 * MINUTES_PER_DAY; // 10080 +export type Resolution = 'minutely' | 'hourly' | 'daily'; + +// The ClickHouse rollup tier a window reads. Single source of truth for both the +// group key and the query, so they can't disagree about which table a rule hits. +// Windows <= 6h use minutely; >= 7d use daily unless a TRAFFIC count needs exact +// bucket boundaries (allowDailyRollup false pins it to hourly); the rest use hourly. +export function resolutionFor(timeWindowMinutes: number, allowDailyRollup: boolean): Resolution { + if (timeWindowMinutes <= 360) return 'minutely'; + if (allowDailyRollup && timeWindowMinutes >= DAILY_THRESHOLD_MINUTES) return 'daily'; + return 'hourly'; +} + // A LATENCY rule forces its duration column into the SELECT (see deriveGroupNeeds), // so a null column here means the query shape and the rule disagree: a routing or // column-selection bug, never real data (an empty window synthesizes zeros, not @@ -325,6 +343,11 @@ export type GroupNeeds = { // so ClickHouse never touches the heavy duration_quantiles blob. needsAverage: boolean; needsPercentiles: boolean; + // Whether a >= 7d window may read the daily rollup. TRAFFIC compares absolute + // counts, which daily buckets would skew by up to a full edge day (buckets snap + // to day boundaries, the rolling window doesn't), so a group with any TRAFFIC + // rule stays on hourly. Latency/error-rate tolerate the rounding. + allowDailyRollup: boolean; }; // All rules in a group share a saved filter (it's part of the group key), so the @@ -336,6 +359,7 @@ export function deriveGroupNeeds(groupRules: MetricAlertRuleRow[], logger: Logge needsPreviousWindow: groupRules.some(r => r.thresholdType === 'PERCENTAGE_CHANGE'), needsAverage: groupRules.some(r => r.type === 'LATENCY' && r.metric === 'AVG'), needsPercentiles: groupRules.some(r => r.type === 'LATENCY' && r.metric !== 'AVG'), + allowDailyRollup: !groupRules.some(r => r.type === 'TRAFFIC'), }; } @@ -352,7 +376,13 @@ export async function queryClickHouseWindows( // windows/columns. Derived once per group by deriveGroupNeeds. needs: GroupNeeds, ): Promise<{ current: ClickHouseWindowRow | null; previous: ClickHouseWindowRow | null }> { - const { filterConditions, needsPreviousWindow, needsAverage, needsPercentiles } = needs; + const { + filterConditions, + needsPreviousWindow, + needsAverage, + needsPercentiles, + allowDailyRollup, + } = needs; const anchorMs = evaluationTime.getTime(); const offsetMs = 60_000; const windowMs = timeWindowMinutes * 60_000; @@ -370,12 +400,7 @@ export async function queryClickHouseWindows( // (rather than a separate flag/param) makes the filtered-query-on-rollup // combination unrepresentable. const useTargetRollup = filterConditions.length === 0; - const resolution = - timeWindowMinutes <= 360 - ? 'minutely' - : timeWindowMinutes >= DAILY_THRESHOLD_MINUTES - ? 'daily' - : 'hourly'; + const resolution = resolutionFor(timeWindowMinutes, allowDailyRollup); const tableName = useTargetRollup ? `operations_by_target_${resolution}` : `operations_${resolution}`; From 176003f653ea8def3b113a3641dc3ed73fca63a1 Mon Sep 17 00:00:00 2001 From: Jonathan Brennan Date: Thu, 9 Jul 2026 14:55:27 -0500 Subject: [PATCH 18/18] Persist null previous value for fixed-threshold alerts --- .../src/lib/metric-alert-evaluator.spec.ts | 33 +++++++++++++++++++ .../src/lib/metric-alert-evaluator.ts | 22 ++++++++++--- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index 9331cf0b4d..7835554d0f 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -9,6 +9,7 @@ import { extractMetricValue, groupRulesByQuery, isRuleDue, + previousValueForRule, queryClickHouseWindows, resolutionFor, type GroupNeeds, @@ -299,6 +300,38 @@ describe('extractMetricValue', () => { }); }); +describe('previousValueForRule', () => { + const prev = { + window: 'previous' as const, + total: '500', + total_ok: '450', + average: null, + percentiles: null, + }; + + test('FIXED_VALUE persists null even when the previous window was fetched', () => { + // The window may have been fetched for a PERCENTAGE_CHANGE group-mate; a + // FIXED_VALUE rule still persists null so its history is grouping-independent. + expect( + previousValueForRule(makeRule({ thresholdType: 'FIXED_VALUE', type: 'TRAFFIC' }), prev), + ).toBeNull(); + }); + + test('FIXED_VALUE with no previous window is also null', () => { + expect(previousValueForRule(makeRule({ thresholdType: 'FIXED_VALUE' }), null)).toBeNull(); + }); + + test('PERCENTAGE_CHANGE persists the real previous value', () => { + expect( + previousValueForRule(makeRule({ thresholdType: 'PERCENTAGE_CHANGE', type: 'TRAFFIC' }), prev), + ).toBe(500); + }); + + test('PERCENTAGE_CHANGE with no previous window falls back to null', () => { + expect(previousValueForRule(makeRule({ thresholdType: 'PERCENTAGE_CHANGE' }), null)).toBeNull(); + }); +}); + describe('deriveGroupNeeds', () => { const { logger } = makeLogger(); diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.ts index 1192f40cb3..f0442b4286 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.ts @@ -151,6 +151,20 @@ export function extractMetricValue(row: ClickHouseWindowRow, rule: MetricAlertRu } } +// The previousValue a rule persists to its incident/state log. Only PERCENTAGE_CHANGE +// rules compare against the previous window, so a FIXED_VALUE rule persists null even +// when the window happens to have been fetched for a PERCENTAGE_CHANGE group-mate. That +// keeps a rule's persisted value (and its "was X" history display) independent of who +// shares its group. The breach check ignores it for FIXED_VALUE either way. +export function previousValueForRule( + rule: MetricAlertRuleRow, + previous: ClickHouseWindowRow | null, +): number | null { + return rule.thresholdType === 'PERCENTAGE_CHANGE' && previous + ? extractMetricValue(previous, rule) + : null; +} + function isThresholdBreached( currentValue: number, previousValue: number, @@ -488,10 +502,10 @@ export async function evaluateRule(args: { const now = evaluationTime; const currentValue = extractMetricValue(current, rule); - // A skipped previous window (absolute-only group) yields a null previousValue, - // persisted as-is. FIXED_VALUE never reads it; pass 0 to the breach check just - // for typing (percentage-change groups always fetch the previous window). - const previousValue = previous ? extractMetricValue(previous, rule) : null; + // See previousValueForRule: null for FIXED_VALUE (even when fetched), real value + // for PERCENTAGE_CHANGE. The `?? 0` feeds the breach check for typing only; a + // FIXED_VALUE rule ignores it, a PERCENTAGE_CHANGE rule always has a real value. + const previousValue = previousValueForRule(rule, previous); const breached = isThresholdBreached(currentValue, previousValue ?? 0, rule); // State-log retention is derived from the rule's organization plan, which