diff --git a/.changeset/quick-groups-seek.md b/.changeset/quick-groups-seek.md new file mode 100644 index 0000000000..cc8dd1380a --- /dev/null +++ b/.changeset/quick-groups-seek.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/api': patch +--- + +fix: Grouped alerts with many groups no longer slow down every alert check: the previous state of each group is read in one query with one index seek per group, instead of every history row of the last 7 days. The API builds a new AlertHistory index `{alert, group, createdAt, state}` at startup and then drops the `{alert, group, createdAt}` index it replaces. On a large AlertHistory collection the index build takes a while; checks keep using the old index until it is done. diff --git a/packages/api/src/__tests__/migrations.int.test.ts b/packages/api/src/__tests__/migrations.int.test.ts index 52a22dbfb3..95ff955909 100644 --- a/packages/api/src/__tests__/migrations.int.test.ts +++ b/packages/api/src/__tests__/migrations.int.test.ts @@ -2,8 +2,12 @@ import mongoose from 'mongoose'; import { createTeam } from '@/controllers/team'; import { clearDBCollections, closeDB, connectDB } from '@/fixtures'; -import { backfillAlertDisplayFields } from '@/migrations'; +import { + backfillAlertDisplayFields, + dropReplacedAlertHistoryIndex, +} from '@/migrations'; import Alert, { AlertSource, AlertThresholdType } from '@/models/alert'; +import AlertHistory from '@/models/alertHistory'; import Dashboard from '@/models/dashboard'; import { SavedSearch } from '@/models/savedSearch'; @@ -225,3 +229,48 @@ describe('backfillAlertDisplayFields', () => { ); }); }); + +describe('dropReplacedAlertHistoryIndex', () => { + const OLD = 'alert_1_group_1_createdAt_-1'; + const NEW = 'alert_1_group_1_createdAt_-1_state_1'; + const indexNames = async () => + (await AlertHistory.collection.indexes()).map(i => i.name); + + beforeAll(async () => { + await connectDB(); + await AlertHistory.init(); + }); + + afterAll(async () => { + await AlertHistory.createIndexes(); + await closeDB(); + }); + + it('drops the old index once the new one exists, and is idempotent', async () => { + await AlertHistory.createIndexes(); + await AlertHistory.collection.createIndex( + { alert: 1, group: 1, createdAt: -1 }, + { name: OLD }, + ); + expect(await indexNames()).toEqual(expect.arrayContaining([OLD, NEW])); + + await dropReplacedAlertHistoryIndex(); + await dropReplacedAlertHistoryIndex(); + + const names = await indexNames(); + expect(names).toContain(NEW); + expect(names).not.toContain(OLD); + }); + + it('keeps the old index while the new one is missing', async () => { + await AlertHistory.collection.createIndex( + { alert: 1, group: 1, createdAt: -1 }, + { name: OLD }, + ); + await AlertHistory.collection.dropIndex(NEW); + + await dropReplacedAlertHistoryIndex(); + + expect(await indexNames()).toContain(OLD); + }); +}); diff --git a/packages/api/src/migrations.ts b/packages/api/src/migrations.ts index 85fd2804c3..40a1092f2a 100644 --- a/packages/api/src/migrations.ts +++ b/packages/api/src/migrations.ts @@ -1,6 +1,7 @@ import mongoose from 'mongoose'; import Alert, { IAlert } from '@/models/alert'; +import AlertHistory from '@/models/alertHistory'; import Dashboard from '@/models/dashboard'; import { SavedSearch } from '@/models/savedSearch'; import { deriveAlertDisplayFields } from '@/utils/alerts'; @@ -129,10 +130,47 @@ export async function backfillAlertDisplayFields() { ); } +// {alert, group, createdAt, state} replaced this index. Mongoose creates +// indexes but never drops them, so drop it once the new one is built. +const REPLACED_ALERT_HISTORY_INDEX = 'alert_1_group_1_createdAt_-1'; +const ALERT_HISTORY_GROUP_INDEX = 'alert_1_group_1_createdAt_-1_state_1'; + +export async function dropReplacedAlertHistoryIndex() { + // Resolves when Mongoose's index builds finish (autoIndex). + await AlertHistory.init(); + const names = (await AlertHistory.collection.indexes()).map(i => i.name); + if ( + !names.includes(ALERT_HISTORY_GROUP_INDEX) || + !names.includes(REPLACED_ALERT_HISTORY_INDEX) + ) { + return; + } + try { + await AlertHistory.collection.dropIndex(REPLACED_ALERT_HISTORY_INDEX); + logger.info( + { index: REPLACED_ALERT_HISTORY_INDEX }, + 'Dropped replaced AlertHistory index', + ); + } catch (e) { + // Another API replica dropped it first. + if ( + !(e instanceof mongoose.mongo.MongoServerError) || + e.codeName !== 'IndexNotFound' + ) { + throw e; + } + } +} + export async function runStartupMigrations() { try { await backfillAlertDisplayFields(); } catch (e) { logger.error({ err: e }, 'Error backfilling alert display names and tags'); } + try { + await dropReplacedAlertHistoryIndex(); + } catch (e) { + logger.error({ err: e }, 'Error dropping replaced AlertHistory index'); + } } diff --git a/packages/api/src/models/alertHistory.ts b/packages/api/src/models/alertHistory.ts index a0b9fbb3b3..1d1656d232 100644 --- a/packages/api/src/models/alertHistory.ts +++ b/packages/api/src/models/alertHistory.ts @@ -135,8 +135,11 @@ AlertHistorySchema.index( // Used by getRecentAlertHistories (matches on alert, sorts by createdAt) AlertHistorySchema.index({ alert: 1, createdAt: -1 }); -// Used by getPreviousAlertHistories (groups by {alert, group}, sorts by createdAt) -AlertHistorySchema.index({ alert: 1, group: 1, createdAt: -1 }); +// Used by getPreviousAlertHistories (newest non-ERROR row per {alert, group}). +// `state` comes last so the ERROR exclusion is an index bound and the query +// keeps a DISTINCT_SCAN. Replaces {alert: 1, group: 1, createdAt: -1}, which +// dropReplacedAlertHistoryIndex (src/migrations.ts) drops at API startup. +AlertHistorySchema.index({ alert: 1, group: 1, createdAt: -1, state: 1 }); export default mongoose.model( 'AlertHistory', diff --git a/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts b/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts index 8ef7412f5c..6887360e4c 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts @@ -12006,6 +12006,112 @@ describe('checkAlerts', () => { new Date('2025-01-01T00:15:00Z'), ); }); + + it('returns the newest non-ERROR row per group, including groups that stopped reporting and rows without a group', async () => { + const alertId = new mongoose.Types.ObjectId(); + // Recent dates, so the 30-day TTL index can't remove rows mid-test. + const base = Date.now() - 60 * 60_000; + const at = (minute: number) => new Date(base + minute * 60_000); + const rows = [ + // group a: reports every minute, newest row OK + ...[1, 2, 3].map(m => ({ + group: 'a', + createdAt: at(m), + state: AlertState.ALERT, + })), + { group: 'a', createdAt: at(4), state: AlertState.OK }, + // group b: newest row is ERROR, so the row before it counts + { group: 'b', createdAt: at(3), state: AlertState.ALERT, fired: true }, + { group: 'b', createdAt: at(4), state: AlertState.ERROR }, + // group c: stopped reporting while in ALERT + { group: 'c', createdAt: at(1), state: AlertState.ALERT, fired: true }, + // no group (as non-grouped alerts write them): OK, then a newer ERROR + { createdAt: at(2), state: AlertState.OK }, + { createdAt: at(4), state: AlertState.ERROR }, + // only ERROR rows: no previous history for this group + { group: 'd', createdAt: at(4), state: AlertState.ERROR }, + ]; + await AlertHistory.insertMany(rows.map(r => ({ alert: alertId, ...r }))); + + const result = await getPreviousAlertHistories( + [alertId.toString()], + at(5), + ); + + const id = alertId.toString(); + expect(new Set(result.keys())).toEqual( + new Set([`${id}||a`, `${id}||b`, `${id}||c`, id]), + ); + expect(result.get(`${id}||a`)).toMatchObject({ + createdAt: at(4), + state: AlertState.OK, + group: 'a', + }); + expect(result.get(`${id}||b`)).toMatchObject({ + createdAt: at(3), + state: AlertState.ALERT, + fired: true, + group: 'b', + }); + expect(result.get(`${id}||c`)).toMatchObject({ + createdAt: at(1), + state: AlertState.ALERT, + fired: true, + }); + expect(result.get(id)).toMatchObject({ + createdAt: at(2), + state: AlertState.OK, + }); + for (const history of result.values()) { + expect(history._id.toString()).toBe(id); + } + }); + + it('reads one row per group, not every row in the lookback window', async () => { + await AlertHistory.createIndexes(); + const alertId = new mongoose.Types.ObjectId(); + // Recent dates, so the 30-day TTL index can't remove rows mid-test. + const now = new Date(); + const groups = 20; + const evaluations = 200; + await AlertHistory.insertMany( + Array.from({ length: groups * evaluations }, (_, i) => ({ + alert: alertId, + group: `g${i % groups}`, + createdAt: new Date( + now.getTime() - (Math.floor(i / groups) + 1) * 60_000, + ), + state: AlertState.OK, + })), + ); + // A failed evaluation every minute too, without a group (as the worker + // writes them): excluding them must not cost a document read each. + await AlertHistory.insertMany( + Array.from({ length: evaluations }, (_, i) => ({ + alert: alertId, + createdAt: new Date(now.getTime() - (i + 1) * 60_000), + state: AlertState.ERROR, + })), + ); + + const aggregateSpy = jest.spyOn(AlertHistory, 'aggregate'); + const result = await getPreviousAlertHistories([alertId.toString()], now); + expect(result.size).toBe(groups); + expect(aggregateSpy).toHaveBeenCalledTimes(1); + + const pipeline = aggregateSpy.mock.calls[0][0] as any[]; + aggregateSpy.mockRestore(); + const explain: any = await AlertHistory.collection + .aggregate(pipeline) + .explain('executionStats'); + const cursor = explain.stages?.[0]?.$cursor ?? explain; + // MongoDB 5.0-8.0 answer this with a DISTINCT_SCAN that skips the ERROR + // rows inside the index; the property that matters is one row read per + // group, not every row in the window. + expect(cursor.executionStats.totalDocsExamined).toBeLessThanOrEqual( + groups + 1, + ); + }); }); describe('getConsecutiveWindowHistories', () => { diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index 17a6b66c35..0f098a1d0b 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -1828,10 +1828,12 @@ export interface AggregatedAlertHistory { * Fetch the most recent AlertHistory value for each of the given alert IDs. * For group-by alerts, returns the latest history for each group within each alert. * - * Uses per-alert queries instead of batched $in to leverage the compound index - * {alert: 1, group: 1, createdAt: -1} for index-backed sorting. With a single - * alert value, the index delivers results already sorted by {group, createdAt desc}, - * so the $sort is a no-op and $group + $first can short-circuit per group. + * Uses per-alert queries so each one can use the compound index + * {alert: 1, group: 1, createdAt: -1, state: 1} as a DISTINCT_SCAN: one index + * seek per group instead of reading every row in the lookback window, which + * for a high-cardinality group-by alert is millions of rows (#3508). That plan + * needs a single-field $group key and only index fields in $match; ERROR rows + * are skipped inside the index (keys examined, documents not fetched). * * @param alertIds The list of alert IDs to query the latest history for. * @param now The current date and time. AlertHistory documents that have a createdAt > now are ignored. @@ -1862,24 +1864,23 @@ export const getPreviousAlertHistories = async ( createdAt: { $lte: now, $gte: lookbackDate }, // ERROR rows record failed evaluations; they must not count as // "window evaluated" or the failed window would never be - // retried/backfilled. + // retried/backfilled. `state` is the last index field, so this + // is an index bound, not a filter on fetched documents. state: { $ne: AlertState.ERROR }, }, }, - // With a single alert value, the compound index {alert: 1, group: 1, createdAt: -1} - // delivers results already in this sort order — this is an index-backed no-op sort. + // With a single alert value, the index delivers results already in + // this sort order — this is an index-backed no-op sort. { $sort: { alert: 1, group: 1, createdAt: -1 }, }, - // Group by {alert, group}, taking the first (latest) document's fields. - // Using $first on individual fields instead of $first: '$$ROOT' allows - // DocumentDB to avoid fetching full documents when not needed. + // One group key (not {alert, group}) so the planner can use a + // DISTINCT_SCAN. $first on individual fields instead of + // $first: '$$ROOT' lets DocumentDB avoid fetching full documents. { $group: { - _id: { - alert: '$alert', - group: '$group', - }, + _id: '$group', + alert: { $first: '$alert' }, createdAt: { $first: '$createdAt' }, state: { $first: '$state' }, fired: { $first: '$fired' }, @@ -1887,10 +1888,10 @@ export const getPreviousAlertHistories = async ( }, { $project: { - _id: '$_id.alert', + _id: '$alert', createdAt: 1, state: 1, - group: '$_id.group', + group: '$_id', fired: 1, }, },