diff --git a/deployment/grafana-dashboards/Metric-Alerts.json b/deployment/grafana-dashboards/Metric-Alerts.json index 0825b5f254b..d747294a6fd 100644 --- a/deployment/grafana-dashboards/Metric-Alerts.json +++ b/deployment/grafana-dashboards/Metric-Alerts.json @@ -272,9 +272,135 @@ "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 +422,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 +460,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 +515,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 +569,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 +623,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 +667,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/deployment/index.ts b/deployment/index.ts index c184dcdb0ac..c35259c1eaa 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 29d3da0531a..30807f19e2f 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') 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 00000000000..1311ab98f47 --- /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 46b764e35a4..2aa876004cd 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) { 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 0ac51e82d76..cb73d7cf8c5 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,9 +3,11 @@ 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, + MINUTES_PER_DAY, } from '../../commerce/constants'; import { OrganizationManager } from '../../organization/providers/organization-manager'; import { Logger } from '../../shared/providers/logger'; @@ -338,6 +340,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 % MINUTES_PER_DAY !== 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 c185421f07b..dc7a6f2185d 100644 --- a/packages/services/api/src/modules/commerce/constants.ts +++ b/packages/services/api/src/modules/commerce/constants.ts @@ -51,5 +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 +// (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/package.json b/packages/services/workflows/package.json index 19b847f8c29..7ed39cb86a5 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/lib/metric-alert-evaluator.spec.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts index a7020b38dbb..7835554d0ff 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -3,8 +3,16 @@ import { printWithValues, type SqlValue } from '@hive/clickhouse'; import type { ClickHouseClient } from './clickhouse-client.js'; import { buildSavedFilterConditions, + DAILY_THRESHOLD_MINUTES, + deriveGroupNeeds, + evaluationIntervalMinutes, + extractMetricValue, groupRulesByQuery, + isRuleDue, + previousValueForRule, queryClickHouseWindows, + resolutionFor, + type GroupNeeds, type MetricAlertRuleRow, } from './metric-alert-evaluator.js'; @@ -39,6 +47,7 @@ function makeRule(overrides: Partial): MetricAlertRuleRow { severity: 'WARNING', state: 'NORMAL', stateChangedAt: null, + lastEvaluatedAt: null, confirmationMinutes: 0, savedFilterId: null, savedFilterFilters: null, @@ -66,6 +75,126 @@ 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', () => { + 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('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` + // 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', () => { @@ -126,6 +255,157 @@ 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('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(); + + 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('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); + 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 }> = []; @@ -141,9 +421,20 @@ 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, + allowDailyRollup: 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. @@ -162,7 +453,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. @@ -179,7 +476,150 @@ 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 cutoff all read hourly. + await queryClickHouseWindows(clickhouse, target, 1440, evalTime, needs()); + await queryClickHouseWindows(clickhouse, target, 4320, 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'); + } + }); + + test('windows >= 7 days read the daily rollup (unfiltered -> by_target)', async () => { + const { clickhouse, calls } = captureClient(); + 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('); + }); + + 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( + { clientFilters: [{ name: 'web', versions: null }] }, + makeLogger().logger, + ); + 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'); + expect(sql).toContain('quantilesMerge('); + 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, + needs({ needsPreviousWindow: 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, + needs({ needsPreviousWindow: 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)); + }); + + test('groups needing neither duration column select NULL placeholders', async () => { + const { clickhouse, calls } = captureClient(); + 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'); + 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, + needs({ needsAverage: false, needsPercentiles: 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, + needs({ needsAverage: true, needsPercentiles: 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 4abfaf886b0..f0442b42861 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 @@ -51,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; @@ -60,14 +63,67 @@ 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 // nanoseconds; latency rule thresholds and all display surfaces use ms. const NS_TO_MS = 1e6; -function extractMetricValue(row: ClickHouseWindowRow, rule: MetricAlertRuleRow): number { +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. +// 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 + +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 +// 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); @@ -77,24 +133,38 @@ function extractMetricValue(row: ClickHouseWindowRow, rule: MetricAlertRuleRow): case 'ERROR_RATE': return total > 0 ? ((total - totalOk) / total) * 100 : 0; case 'LATENCY': { - const metricMap: Record = { - AVG: row.average, - P75: row.percentiles[0], - P90: row.percentiles[1], - P95: row.percentiles[2], - P99: row.percentiles[3], - }; - // 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; } } } +// 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, @@ -167,6 +237,7 @@ export async function fetchEnabledRules(pg: PostgresDatabasePool): Promise 24h (7d, 30d): every 30 min +} + +// 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 @@ -247,17 +343,60 @@ 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; + // 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 +// 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'), + allowDailyRollup: !groupRules.some(r => r.type === 'TRAFFIC'), + }; +} + 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, + // 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, + allowDailyRollup, + } = needs; const anchorMs = evaluationTime.getTime(); const offsetMs = 60_000; const windowMs = timeWindowMinutes * 60_000; @@ -275,7 +414,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' : 'hourly'; + const resolution = resolutionFor(timeWindowMinutes, allowDailyRollup); const tableName = useTargetRollup ? `operations_by_target_${resolution}` : `operations_${resolution}`; @@ -294,19 +433,28 @@ export async function queryClickHouseWindows( const filterClause = filterConditions.length > 0 ? sql` AND ${sql.join(filterConditions, ' AND ')}` : sql``; + 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 - 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 + ${averageCol}, + ${percentilesCol} 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 @@ -334,14 +482,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 @@ -354,8 +502,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); + // 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 // is included on the row by `fetchEnabledRules` (single JOIN) — no extra @@ -560,7 +711,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/lib/metric-alert-notifier.ts b/packages/services/workflows/src/lib/metric-alert-notifier.ts index cad8c550d9d..5782cb6938b 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/metrics.ts b/packages/services/workflows/src/metrics.ts index 27b1429a0a7..d2ba965aae6 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 4c44956e64f..2395d3addbf 100644 --- a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts +++ b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts @@ -1,14 +1,17 @@ +import pLimit from 'p-limit'; import { z } from 'zod'; 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, + 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 @@ -56,6 +59,26 @@ 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 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 (deriveGroupNeeds(group, logger).filterConditions.length > 0) { + 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'); @@ -64,8 +87,20 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { logger.info({ count: rules.length }, 'Evaluating metric alert rules'); - const groups = groupRulesByQuery(rules); - const groupList = [...groups.values()]; + // 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 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; @@ -73,11 +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); + // 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 @@ -92,6 +127,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 => { @@ -102,8 +144,8 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { clickhouse, representative.targetId, representative.timeWindowMinutes, - filterConditions, evaluationTime, + needs, ); } catch (error) { logger.error( @@ -129,9 +171,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', @@ -170,33 +216,32 @@ 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(), }, }, 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 < groupList.length; i += GROUP_CONCURRENCY) { - const batch = groupList.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); } } @@ -226,7 +271,7 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { logger.info( { - groupsAttempted: groups.size, + groupsAttempted: dueGroupList.length, groupsFailed, rulesEvaluated: evaluatedRuleIds.length, }, 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 2182e9caa2c..88086567b75 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 2d22b39cc79..c1578e3e074 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', diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ae11dbc2db5..c490dec7e8c 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2071,6 +2071,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