diff --git a/.changeset/alert-history-map-per-alert.md b/.changeset/alert-history-map-per-alert.md new file mode 100644 index 0000000000..e6f6e424df --- /dev/null +++ b/.changeset/alert-history-map-per-alert.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/api': patch +--- + +perf: check-alerts gives each alert only its own previous history, so a tick +no longer scans every alert's group history once per alert 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 d3d0bbdc3b..30badac6df 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts @@ -43,6 +43,7 @@ import { getPreviousAlertHistories, getScheduledWindowStart, parseAlertData, + partitionPreviousMapsByAlert, processAlert, } from '@/tasks/checkAlerts'; import { @@ -6933,6 +6934,132 @@ describe('checkAlerts', () => { expect(resolutionCall).toBeDefined(); }); + describe('with a previous-history map shared by every alert in the tick', () => { + const now = new Date('2023-11-16T22:12:00.000Z'); + + const setupGroupedAlertBesideFiringAlert = async () => { + const setup = await setupSavedSearchAlertTest(); + const { team, webhook, source, savedSearch } = setup; + const alertConfig = { + source: AlertSource.SAVED_SEARCH, + channel: { type: 'webhook', webhookId: webhook._id.toString() }, + interval: '5m', + thresholdType: AlertThresholdType.ABOVE, + threshold: 1, + savedSearchId: savedSearch.id, + } as const; + const savedSearchDetails = { + taskType: AlertTaskType.SAVED_SEARCH, + savedSearch, + } as const; + + const grouped = await createAlertDetails( + team, + source, + { ...alertConfig, groupBy: 'ServiceName' }, + savedSearchDetails, + ); + const firing = await createAlertDetails( + team, + source, + alertConfig, + savedSearchDetails, + ); + await AlertHistory.create({ + alert: firing.alert.id, + createdAt: new Date('2023-11-16T22:05:00.000Z'), + state: AlertState.ALERT, + fired: true, + counts: 1, + lastValues: [], + }); + + return { ...setup, grouped, firing }; + }; + + // Mirrors CheckAlertTask.execute(): load one shared map, then split it. + const evaluateGroupedAlert = async ({ + grouped, + firing, + connection, + clickhouseClient, + teamWebhooksById, + }: Awaited>) => { + const shared = await getPreviousAlertHistories( + [grouped.alert.id, firing.alert.id], + now, + ); + expect(shared.get(firing.alert.id)?.state).toBe(AlertState.ALERT); + + const [task] = partitionPreviousMapsByAlert([ + { + alerts: [ + { ...grouped, previousMap: shared }, + { ...firing, previousMap: shared }, + ], + conn: connection, + now, + }, + ]); + await processAlert( + now, + task.alerts[0], + clickhouseClient, + connection.id, + alertProvider, + teamWebhooksById, + ); + + return AlertHistory.find({ alert: grouped.alert.id }).sort({ + createdAt: 1, + }); + }; + + it("does not write an empty-group history from another alert's ALERT state", async () => { + const setup = await setupGroupedAlertBesideFiringAlert(); + await bulkInsertLogs( + [1, 2].map(() => ({ + ServiceName: 'service-a', + Timestamp: new Date('2023-11-16T22:05:00.000Z'), + SeverityText: 'error', + Body: 'Error from service-a', + })), + ); + + const histories = await evaluateGroupedAlert(setup); + + expect(histories.map(h => [h.group, h.state])).toEqual([ + ['ServiceName:service-a', AlertState.ALERT], + ]); + }); + + it('records every empty bucket when only another alert is firing', async () => { + const setup = await setupGroupedAlertBesideFiringAlert(); + await AlertHistory.create({ + alert: setup.grouped.alert.id, + createdAt: new Date('2023-11-16T21:55:00.000Z'), + state: AlertState.OK, + group: 'ServiceName:service-a', + counts: 0, + lastValues: [], + }); + + const histories = await evaluateGroupedAlert(setup); + + const latest = histories[histories.length - 1]; + expect(histories).toHaveLength(2); + expect(latest.group).toBeUndefined(); + expect(latest.state).toBe(AlertState.OK); + expect( + latest.lastValues.map(v => [v.startTime.toISOString(), v.count]), + ).toEqual([ + ['2023-11-16T21:55:00.000Z', 0], + ['2023-11-16T22:00:00.000Z', 0], + ['2023-11-16T22:05:00.000Z', 0], + ]); + }); + }); + // The sample rows quoted in the message body carry no group predicate, so // every group's notification used to re-run the identical query. it('fetches the message body sample rows once for all groups in a window', async () => { diff --git a/packages/api/src/tasks/checkAlerts/__tests__/checkAlertsTask.test.ts b/packages/api/src/tasks/checkAlerts/__tests__/checkAlertsTask.test.ts index 5b5d348898..06f00de963 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/checkAlertsTask.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/checkAlertsTask.test.ts @@ -3,11 +3,19 @@ import mongoose from 'mongoose'; import * as config from '@/config'; import { ObjectId } from '@/models'; -import { AlertSource, AlertThresholdType, IAlert } from '@/models/alert'; +import { + AlertSource, + AlertState, + AlertThresholdType, + IAlert, +} from '@/models/alert'; import { ISource } from '@/models/source'; import { IWebhook } from '@/models/webhook'; import * as checkAlerts from '@/tasks/checkAlerts'; -import CheckAlertTask from '@/tasks/checkAlerts'; +import CheckAlertTask, { + AggregatedAlertHistory, + partitionPreviousMapsByAlert, +} from '@/tasks/checkAlerts'; import { AlertDetails, AlertProvider, @@ -24,6 +32,130 @@ jest.mock('@/tasks/checkAlerts/providers', () => { }; }); +type TestHistory = Omit; + +const buildHistory = (group?: string): TestHistory => ({ + createdAt: new Date('2026-01-01T00:00:00Z'), + state: AlertState.OK, + group, +}); + +const buildPartitionTask = ( + alerts: { id: string; previousMap: Map }[], + teamId = new mongoose.Types.ObjectId(), +): AlertTask => + ({ + alerts: alerts.map(({ id, previousMap }) => ({ + alert: { + id, + team: { _id: teamId }, + source: AlertSource.SAVED_SEARCH, + threshold: 10, + thresholdType: AlertThresholdType.ABOVE, + interval: '1m', + channel: { type: 'webhook', webhookId: 'webhook-123' }, + }, + taskType: AlertTaskType.SAVED_SEARCH, + previousMap, + })), + conn: { + id: 'conn-123', + _id: new mongoose.Types.ObjectId(), + host: config.CLICKHOUSE_HOST, + username: config.CLICKHOUSE_USER, + password: config.CLICKHOUSE_PASSWORD, + name: '', + team: teamId, + }, + now: new Date(), + }) as unknown as AlertTask; + +describe('partitionPreviousMapsByAlert', () => { + it('gives each alert only its own entries from a shared map, keys unchanged', () => { + const groupedA = buildHistory('service:api'); + const groupedB = buildHistory('service:web'); + const ungrouped = buildHistory(); + const otherAlert = buildHistory(); + const shared = new Map([ + ['grouped||service:api', groupedA], + ['grouped||service:web', groupedB], + ['ungrouped', ungrouped], + ['not-in-tick', otherAlert], + ]); + + const [task] = partitionPreviousMapsByAlert([ + buildPartitionTask([ + { id: 'grouped', previousMap: shared }, + { id: 'ungrouped', previousMap: shared }, + { id: 'no-history', previousMap: shared }, + ]), + ]); + + expect(task.alerts[0].previousMap).toEqual( + new Map([ + ['grouped||service:api', groupedA], + ['grouped||service:web', groupedB], + ]), + ); + expect(task.alerts[1].previousMap).toEqual( + new Map([['ungrouped', ungrouped]]), + ); + expect(task.alerts[2].previousMap.size).toBe(0); + }); + + it('keeps a group name that contains the delimiter with its alert', () => { + const history = buildHistory('path:a||b'); + const shared = new Map([['alert-1||path:a||b', history]]); + + const [task] = partitionPreviousMapsByAlert([ + buildPartitionTask([{ id: 'alert-1', previousMap: shared }]), + ]); + + expect(task.alerts[0].previousMap).toEqual( + new Map([['alert-1||path:a||b', history]]), + ); + }); + + it('splits maps shared across tasks and keeps already separate maps', () => { + const shared = new Map([ + ['alert-1', buildHistory()], + ['alert-2||g:1', buildHistory('g:1')], + ]); + const own = new Map([['alert-3', buildHistory()]]); + + const tasks = partitionPreviousMapsByAlert([ + buildPartitionTask([{ id: 'alert-1', previousMap: shared }]), + buildPartitionTask([ + { id: 'alert-2', previousMap: shared }, + { id: 'alert-3', previousMap: own }, + ]), + ]); + + expect([...tasks[0].alerts[0].previousMap.keys()]).toEqual(['alert-1']); + expect([...tasks[1].alerts[0].previousMap.keys()]).toEqual([ + 'alert-2||g:1', + ]); + expect([...tasks[1].alerts[1].previousMap.keys()]).toEqual(['alert-3']); + }); + + it('does not mutate the input tasks or maps', () => { + const shared = new Map([ + ['alert-1', buildHistory()], + ['alert-2', buildHistory()], + ]); + const input = buildPartitionTask([ + { id: 'alert-1', previousMap: shared }, + { id: 'alert-2', previousMap: shared }, + ]); + + partitionPreviousMapsByAlert([input]); + + expect(shared.size).toBe(2); + expect(input.alerts[0].previousMap).toBe(shared); + expect(input.alerts[1].previousMap).toBe(shared); + }); +}); + describe('CheckAlertTask', () => { describe('execute', () => { let mockAlertProvider: jest.Mocked; @@ -331,6 +463,53 @@ describe('CheckAlertTask', () => { ); }); + it('should pass each alert only its own previous history', async () => { + const args: CheckAlertsTaskArgs = { taskName: TaskName.CHECK_ALERTS }; + const task = new CheckAlertTask(args); + + const groupA = buildHistory('g:a'); + const groupB = buildHistory('g:b'); + const ungrouped = buildHistory(); + const shared = new Map([ + ['alert-1||g:a', groupA], + ['alert-1||g:b', groupB], + ['alert-2', ungrouped], + ]); + + mockAlertProvider.getAlertTasks.mockResolvedValue([ + buildPartitionTask([ + { id: 'alert-1', previousMap: shared }, + { id: 'alert-2', previousMap: shared }, + ]), + ]); + mockAlertProvider.getWebhooks.mockResolvedValue(new Map()); + mockAlertProvider.getClickHouseClient.mockResolvedValue( + new ClickhouseClient({}), + ); + const receivedMaps = new Map>(); + mockProcessAlert.mockImplementation( + async (_now: Date, details: AlertDetails) => { + receivedMaps.set(details.alert.id, details.previousMap); + }, + ); + + await task.execute(); + + expect(receivedMaps).toEqual( + new Map([ + [ + 'alert-1', + new Map([ + ['alert-1||g:a', groupA], + ['alert-1||g:b', groupB], + ]), + ], + ['alert-2', new Map([['alert-2', ungrouped]])], + ]), + ); + expect(shared.size).toBe(3); + }); + describe('when a connection fails', () => { // The task queue runs batches fire-and-forget, and the worker's global // unhandledRejection handler exits the process, so a single failing diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index d34af3dd85..18ece93b57 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -628,6 +628,48 @@ const extractGroupKeyFromMapKey = (mapKey: string, alertId: string): string => { : ''; }; +/** + * Give each alert a history map holding only its own entries. Providers may + * hand every alert one map shared across the tick, and every reader scans the + * map, so a shared map makes tick CPU grow with alerts times total groups. + */ +export const partitionPreviousMapsByAlert = ( + alertTasks: AlertTask[], +): AlertTask[] => { + const bucketsBySharedMap = new Map< + Map, + Map> + >(); + const getBuckets = (previousMap: Map) => { + let buckets = bucketsBySharedMap.get(previousMap); + if (buckets == null) { + buckets = new Map(); + for (const [key, history] of previousMap) { + const delimiterIndex = key.indexOf(ALERT_GROUP_DELIMITER); + const alertId = + delimiterIndex === -1 ? key : key.substring(0, delimiterIndex); + let bucket = buckets.get(alertId); + if (bucket == null) { + bucket = new Map(); + buckets.set(alertId, bucket); + } + bucket.set(key, history); + } + bucketsBySharedMap.set(previousMap, buckets); + } + return buckets; + }; + + return alertTasks.map(task => ({ + ...task, + alerts: task.alerts.map(details => ({ + ...details, + previousMap: + getBuckets(details.previousMap).get(details.alert.id) ?? new Map(), + })), + })); +}; + /** Determine if we should skip the alert check based on how recently it was last evaluated. */ const shouldSkipAlertCheck = ( details: AlertDetails, @@ -2221,7 +2263,9 @@ export default class CheckAlertTask implements HdxTask { 'finished provider initialization', ); - const alertTasks = await this.provider.getAlertTasks(); + const alertTasks = partitionPreviousMapsByAlert( + await this.provider.getAlertTasks(), + ); const taskCount = alertTasks.length; logger.debug( {