diff --git a/.changeset/large-pets-pay.md b/.changeset/large-pets-pay.md new file mode 100644 index 00000000000..d7c1a4667dd --- /dev/null +++ b/.changeset/large-pets-pay.md @@ -0,0 +1,5 @@ +--- +'hive': patch +--- + +Metric Alerts optimization: Creates a new daily ClickHouse rollup and routes long-windowed rules/groups diff --git a/.github/workflows/changeset-version.yaml b/.github/workflows/changeset-version.yaml index abab625b8b3..2ba727917ec 100644 --- a/.github/workflows/changeset-version.yaml +++ b/.github/workflows/changeset-version.yaml @@ -14,6 +14,14 @@ jobs: steps: - name: checkout uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 + with: + # `changeset version` walks git history to find the commit that added + # each changeset. On a shallow clone it deepens via `git fetch --deepen` + # in a loop, which intermittently stalls and hangs the job. Full history + # skips that path; blob:none keeps the download to the ~14 MiB commit/tree + # graph instead of ~394 MiB, since the file blobs are never needed here. + fetch-depth: 0 + filter: blob:none - name: setup environment uses: ./.github/actions/setup 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..e7ddf89dc9f 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,9 @@ export function deployWorkflows({ image, replicas: environment.podsConfig.general.replicas, }, - [redis.deployment, redis.service], + // Depend on dbMigrations so the ClickHouse migration (operations_by_target_daily) + // runs before this service routes long-window queries to it. + [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..83244086b28 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 buckets, so they + // must be a whole number of days or the window silently rounds. Guards direct + // API callers; the UI presets already comply. + 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..034631481f0 100644 --- a/packages/services/api/src/modules/commerce/constants.ts +++ b/packages/services/api/src/modules/commerce/constants.ts @@ -51,5 +51,11 @@ 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 >= this read the daily ClickHouse rollup (whole-day buckets), so they +// must be a whole number of days. 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 29f86747b32..bb0f596d75a 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.spec.ts @@ -3,12 +3,14 @@ import { printWithValues, type SqlValue } from '@hive/clickhouse'; import type { ClickHouseClient } from './clickhouse-client.js'; import { buildSavedFilterConditions, + DAILY_THRESHOLD_MINUTES, evaluationIntervalMinutes, extractMetricValue, groupRulesByQuery, isRuleDue, previousValueForRule, queryClickHouseWindows, + resolutionFor, type MetricAlertRuleRow, } from './metric-alert-evaluator.js'; @@ -71,6 +73,26 @@ 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); + 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('evaluationIntervalMinutes', () => { @@ -86,6 +108,27 @@ describe('evaluationIntervalMinutes', () => { }); }); +describe('DAILY_THRESHOLD_MINUTES', () => { + // Must match METRIC_ALERT_RULE_DAILY_ROLLUP_THRESHOLD_MINUTES in the api + // package (separate package, can't import); pin the value on both sides. + test('is exactly 7 whole days', () => { + expect(DAILY_THRESHOLD_MINUTES).toBe(10080); + }); +}); + +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'); + }); +}); + describe('isRuleDue', () => { const evalTime = new Date('2026-07-08T12:00:00.000Z'); // ISO timestamp for a point `minutes` before evalTime (mirrors the `to_json` @@ -252,6 +295,57 @@ describe('queryClickHouseWindows', () => { 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(); + await queryClickHouseWindows(clickhouse, target, 1440, [], evalTime); + await queryClickHouseWindows(clickhouse, target, 4320, [], evalTime); + await queryClickHouseWindows(clickhouse, target, DAILY_THRESHOLD_MINUTES - 1, [], 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, DAILY_THRESHOLD_MINUTES, [], evalTime); + const { sql } = calls[0]; + expect(sql).toContain('FROM operations_by_target_daily'); + expect(sql).toContain('quantilesTDigestMerge('); + }); + + test('a TRAFFIC group (allowDailyRollup=false) stays on hourly at >= 7 days', async () => { + const { clickhouse, calls } = captureClient(); + // trailing args: needsPreviousWindow, needsAverage, needsPercentiles, allowDailyRollup + await queryClickHouseWindows( + clickhouse, + target, + DAILY_THRESHOLD_MINUTES, + [], + evalTime, + true, + true, + true, + 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, 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}'); + }); + 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); diff --git a/packages/services/workflows/src/lib/metric-alert-evaluator.ts b/packages/services/workflows/src/lib/metric-alert-evaluator.ts index 03ad041ff00..48cf43de090 100644 --- a/packages/services/workflows/src/lib/metric-alert-evaluator.ts +++ b/packages/services/workflows/src/lib/metric-alert-evaluator.ts @@ -62,13 +62,34 @@ type ClickHouseWindowRow = z.infer; type GroupKey = string; function makeGroupKey(rule: MetricAlertRuleRow): GroupKey { - return `${rule.targetId}:${rule.timeWindowMinutes}:${rule.savedFilterId ?? ''}`; + // Include the resolved tier so a >= 7d TRAFFIC rule (hourly) and a >= 7d latency + // rule (daily) on the same target/window/filter don't share one query. + 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; +const MINUTES_PER_DAY = 24 * 60; // 1440 + +// Windows >= 7 days read the daily rollups (far fewer buckets than hourly). Must +// equal the API's METRIC_ALERT_RULE_DAILY_ROLLUP_THRESHOLD_MINUTES (separate +// package, can't import); keep 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 the group +// key and the query. TRAFFIC (allowDailyRollup false) stays on hourly at >= 7d so +// its absolute counts aren't skewed by daily buckets snapping to day boundaries. +export function resolutionFor(timeWindowMinutes: number, allowDailyRollup: boolean): Resolution { + if (timeWindowMinutes <= 360) return 'minutely'; + if (allowDailyRollup && timeWindowMinutes >= DAILY_THRESHOLD_MINUTES) return 'daily'; + return 'hourly'; +} + // A null column means it wasn't selected (a bug), not missing data (empty windows // give zeros). Throw rather than read a phantom 0 that would silently never fire. function requireColumn(value: T | null, column: string, rule: MetricAlertRuleRow): T { @@ -315,6 +336,8 @@ export async function queryClickHouseWindows( // False selects `NULL as ` so ClickHouse skips reading that duration column. needsAverage: boolean = true, needsPercentiles: boolean = true, + // False keeps a >= 7d window on hourly (TRAFFIC needs exact bucket boundaries). + allowDailyRollup: boolean = true, ): Promise<{ current: ClickHouseWindowRow | null; previous: ClickHouseWindowRow | null }> { const anchorMs = evaluationTime.getTime(); const offsetMs = 60_000; @@ -333,7 +356,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}`; 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 ad83820ca68..d2710e6ef3c 100644 --- a/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts +++ b/packages/services/workflows/src/tasks/evaluate-metric-alert-rules.ts @@ -125,6 +125,9 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { const needsPercentiles = groupRules.some(r => r.type === 'LATENCY' && r.metric !== 'AVG'); const needsAverage = groupRules.some(r => r.type === 'LATENCY' && r.metric === 'AVG'); + // Any TRAFFIC rule keeps the group on hourly at >= 7d (exact counts). + const allowDailyRollup = !groupRules.some(r => r.type === 'TRAFFIC'); + // 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 @@ -161,6 +164,7 @@ export const task = implementTask(EvaluateMetricAlertRulesTask, async args => { needsPreviousWindow, needsAverage, needsPercentiles, + allowDailyRollup, ); } catch (error) { logger.error(