diff --git a/.changeset/webhook-template-range-and-inline-query.md b/.changeset/webhook-template-range-and-inline-query.md new file mode 100644 index 0000000000..4de5524d77 --- /dev/null +++ b/.changeset/webhook-template-range-and-inline-query.md @@ -0,0 +1,29 @@ +--- +'@hyperdx/api': minor +'@hyperdx/app': minor +'@hyperdx/common-utils': minor +--- + +Report the whole alert condition in the `{{sourceQuery}}` webhook template +variable. It read only a chart's top-level `where`, so an alert defined by a +per-series `aggCondition` — a common shape — still rendered empty. The variable +now reports every part of the condition the alert query actually applies: a +chart's `where` plus the `aggCondition` of the series the alert reads, and a +saved search's `where` plus its pinned filters. A chart's pinned filters are +deliberately excluded, since a tile or inline alert does not apply them. The +value is truncated at 2000 characters. + +Editing an alert off a `between` or `outside` comparator now clears the stored +`thresholdMax` instead of leaving the old bound on the document, where it was +also served by the alerts APIs and would advertise a range that no longer +fires. Webhook templates already guarded against this on read. + +The webhook form's variable list and the API's fallback body template both +derive from one list in common-utils, which `buildWebhookTemplateVariables` is +typed against, so a variable cannot be added without appearing in both places. +The "Send test" payload carries a sample value for every variable, so a body +template can be checked before an alert fires. + +The documented guard for an optional number is now +`{{#unless (eq thresholdMax undefined)}}` rather than `{{#if thresholdMax}}`, +which treats a legitimate bound of `0` as absent. diff --git a/docs/alert-webhook-template-variables.md b/docs/alert-webhook-template-variables.md index 194ce0ed08..3297af7291 100644 --- a/docs/alert-webhook-template-variables.md +++ b/docs/alert-webhook-template-variables.md @@ -17,11 +17,11 @@ a HyperDX alert without parsing the human-readable message body. | `{{status}}` | string | `firing`, `resolved`, `no_data`, `pending` or `error`. | | `{{alertType}}` | string | `search`, `dashboard_chart` or `inline_query`. | | `{{comparator}}` | string | `>=`, `>`, `<`, `<=`, `=`, `!=`, `between`, `outside`. | -| `{{threshold}}` | number | The configured threshold. | -| `{{thresholdMax}}` | number | Upper bound of a `between` / `outside` range; empty for every other comparator. | +| `{{threshold}}` | number | The configured threshold. For `between`/`outside`, the lower bound. | +| `{{thresholdMax}}` | number | Upper bound of a `between`/`outside` condition. Always unset for every other comparator, even if the alert was once a range, so guard it (see below). | | `{{value}}` | number | The value that triggered or resolved the alert. | | `{{groupKey}}` | string | The breaching group, for a grouped alert. | -| `{{sourceQuery}}` | string | The query behind the alert: the saved search's filter, the chart's `where`, or the raw SQL. | +| `{{sourceQuery}}` | string | The condition behind the alert — search expression or SQL (see below). | | `{{teamId}}` | string | Team the alert belongs to. | | `{{note}}` | string | The alert's freeform note — commonly a runbook link. | @@ -29,10 +29,49 @@ Strings are JSON-escaped, so they are safe to drop into a quoted slot. Numbers (`startTime`, `endTime`, `threshold`, `thresholdMax`, `value`) are emitted raw for unquoted slots. Every enriched variable is optional and renders as an empty string when the alert doesn't carry it — an alert with no group has -an empty `{{groupKey}}`, and a non-range alert has an empty `{{thresholdMax}}`. +an empty `{{groupKey}}`, for instance. -An empty variable in an unquoted numeric slot produces invalid JSON, so guard -the optional numbers: `{{#if thresholdMax}}"max": {{thresholdMax}},{{/if}}`. +`{{sourceQuery}}` reads whichever fields the alert's condition lives in: a +saved-search alert reports the search's, a dashboard-tile alert its tile's, and +an inline alert its own. For a raw SQL chart that is the whole SQL template. +Otherwise the condition is spread over several inputs and they are joined with +`AND`, each bracketed when there is more than one — for example +`(ServiceName: "checkout") AND (SeverityText: "error")`. An alert with no +condition at all reports an empty string. + +It reports only what the alert query actually applies, so it will not always +match everything the chart shows. A builder chart contributes its chart-level +`where` and the `aggCondition` of the series the alert reads — the last one, +which is the series that produces the value. A chart's pinned `filters` and +`having` are **not** included, because a tile or inline alert does not apply +them. A saved search contributes its `where` and its pinned `filters`, which it +does apply. + +Treat the result as opaque text rather than something to parse or re-run: +nothing in the variable set says which dialect you got, and the parts of a +builder condition can each be Lucene or SQL, so the joined string is not +necessarily valid in either. It is truncated at 2000 characters. PromQL charts +do not support alerts, so no webhook carries a PromQL expression. + +An empty string is not valid JSON in an unquoted numeric slot, so guard any +number that may be absent. Compare against `undefined` rather than using +`{{#if}}`, which treats a legitimate bound of `0` as absent: + +``` +{ + "threshold": {{threshold}}{{#unless (eq thresholdMax undefined)}}, + "threshold_max": {{thresholdMax}}{{/unless}} +} +``` + +Keep a newline or space between `{{/unless}}` and a closing `}` — Handlebars +reads `}}}` as a triple-stache and fails to compile the template. + +The **Send test** button on the webhook form fills every variable with a sample +value, so a template that uses them can be checked before an alert fires. The +sample is a `between` alert, so `{{thresholdMax}}` is populated there — a test +send will not catch a template that breaks when an optional number is absent, +which is what the guard above is for. ## Example diff --git a/packages/api/src/controllers/alerts.ts b/packages/api/src/controllers/alerts.ts index 727aef7590..59cbc183a8 100644 --- a/packages/api/src/controllers/alerts.ts +++ b/packages/api/src/controllers/alerts.ts @@ -5,6 +5,7 @@ import { validateRawSqlForAlert, } from '@hyperdx/common-utils/dist/core/utils'; import { isRawSqlSavedChartConfig } from '@hyperdx/common-utils/dist/guards'; +import { isRangeThresholdType } from '@hyperdx/common-utils/dist/types'; import { groupBy } from 'lodash'; import { Types } from 'mongoose'; import { z } from 'zod'; @@ -281,7 +282,13 @@ export const makeAlert = ( }), source: alert.source, threshold: alert.threshold, - thresholdMax: alert.thresholdMax, + // Omitted rather than set to undefined for a non-range comparator, so + // updateAlert can $unset the path. Writing null instead would surface on + // the create/update responses, which return the document directly, and a + // client round-tripping one back into an update would fail validation. + ...(isRangeThresholdType(alert.thresholdType) && { + thresholdMax: alert.thresholdMax, + }), thresholdType: alert.thresholdType, ...(userId && { createdBy: userId }), @@ -314,6 +321,22 @@ export const makeAlert = ( }; }; +// makeAlert omits thresholdMax for a non-range comparator, and Mongoose drops +// an omitted key from an update rather than clearing the path, so the bound has +// to be unset explicitly or an alert edited off `between` keeps a stale one and +// reports a range condition it no longer has. Every update path needs this; +// $unset is ignored on an upsert insert, so it is safe there too. +const makeAlertUpdate = ( + alertInput: AlertInput, + userId?: ObjectId, + refs: AlertRefs = {}, +) => ({ + $set: makeAlert(alertInput, userId, refs), + ...(!isRangeThresholdType(alertInput.thresholdType) && { + $unset: { thresholdMax: 1 }, + }), +}); + export const createAlert = async ( teamId: ObjectId, alertInput: z.infer, @@ -339,7 +362,7 @@ export const updateAlert = async ( _id: id, team: teamId, }, - makeAlert(alertInput, undefined, refs), + makeAlertUpdate(alertInput, undefined, refs), { returnDocument: 'after', }, @@ -404,12 +427,12 @@ export const createOrUpdateDashboardAlerts = async ( tileId, }; const oldAlert = await Alert.findOne(filter); - const alertValues = + const alertUpdate = oldAlert && oldAlert.createdBy - ? makeAlert(alertInput, undefined, { dashboard }) - : makeAlert(alertInput, userId, { dashboard }); + ? makeAlertUpdate(alertInput, undefined, { dashboard }) + : makeAlertUpdate(alertInput, userId, { dashboard }); - return await Alert.findOneAndUpdate(filter, alertValues, { + return await Alert.findOneAndUpdate(filter, alertUpdate, { new: true, upsert: true, }); diff --git a/packages/api/src/fixtures.ts b/packages/api/src/fixtures.ts index 845f8e16fa..407a1733cc 100644 --- a/packages/api/src/fixtures.ts +++ b/packages/api/src/fixtures.ts @@ -4,6 +4,7 @@ import { AlertThresholdType, BuilderSavedChartConfig, DisplayType, + Filter, RawSqlSavedChartConfig, SavedChartConfig, Tile, @@ -699,6 +700,7 @@ export const makeTile = (opts?: { id?: string; alert?: BuilderSavedChartConfig['alert']; sourceId?: string; + where?: string; }): Tile => ({ id: opts?.id ?? randomMongoId(), x: 1, @@ -712,6 +714,7 @@ export const makeChartConfig = (opts?: { id?: string; alert?: BuilderSavedChartConfig['alert']; sourceId?: string; + where?: string; }): SavedChartConfig => ({ name: 'Test Chart', source: opts?.sourceId ?? 'test-source', @@ -724,7 +727,7 @@ export const makeChartConfig = (opts?: { valueExpression: '', }, ], - where: '', + where: opts?.where ?? '', whereLanguage: 'lucene', granularity: 'auto', implicitColumnExpression: 'Body', @@ -804,6 +807,19 @@ export const RAW_SQL_ALERT_TEMPLATE = [ ' GROUP BY ts ORDER BY ts', ].join(''); +/** Raw SQL counterpart to {@link makeAlertChartConfig}. */ +export const makeRawSqlAlertChartConfig = (opts?: { + name?: string; + sqlTemplate?: string; + connectionId?: string; +}): AlertChartConfig => ({ + name: opts?.name ?? 'Raw SQL Alert Query', + configType: 'sql', + displayType: DisplayType.Line, + sqlTemplate: opts?.sqlTemplate ?? RAW_SQL_ALERT_TEMPLATE, + connection: opts?.connectionId ?? 'test-connection', +}); + export const makeRawSqlAlertTile = (opts?: { id?: string; connectionId?: string; @@ -814,12 +830,7 @@ export const makeRawSqlAlertTile = (opts?: { y: 1, w: 1, h: 1, - config: { - configType: 'sql', - displayType: DisplayType.Line, - sqlTemplate: opts?.sqlTemplate ?? RAW_SQL_ALERT_TEMPLATE, - connection: opts?.connectionId ?? 'test-connection', - } satisfies RawSqlSavedChartConfig, + config: makeRawSqlAlertChartConfig(opts), }); export const RAW_SQL_NUMBER_ALERT_TEMPLATE = [ @@ -912,6 +923,8 @@ export const makeAlertChartConfig = (opts: { displayType?: DisplayType; aggCondition?: string; groupBy?: string; + where?: string; + filters?: Filter[]; }): AlertChartConfig => ({ name: opts.name ?? 'Chart Alert Query', source: opts.sourceId, @@ -924,9 +937,10 @@ export const makeAlertChartConfig = (opts: { valueExpression: '', }, ], - where: '', + where: opts.where ?? '', whereLanguage: 'lucene', ...(opts.groupBy != null && { groupBy: opts.groupBy }), + ...(opts.filters != null && { filters: opts.filters }), }); export const makeInlineAlertInput = ({ diff --git a/packages/api/src/routers/api/__tests__/alerts.int.test.ts b/packages/api/src/routers/api/__tests__/alerts.int.test.ts index 781f97b1d5..a8da5fa1de 100644 --- a/packages/api/src/routers/api/__tests__/alerts.int.test.ts +++ b/packages/api/src/routers/api/__tests__/alerts.int.test.ts @@ -135,6 +135,42 @@ describe('alerts router', () => { expect(allAlerts.body.data[0].threshold).toBe(10); }); + it('clears thresholdMax when an alert is moved off a range comparator', async () => { + const dashboard = await agent + .post('/dashboards') + .send(MOCK_DASHBOARD) + .expect(200); + const alert = await agent + .post('/alerts') + .send({ + ...makeAlertInput({ + dashboardId: dashboard.body.id, + tileId: MOCK_TILES[0].id, + webhookId: webhook._id.toString(), + }), + thresholdType: AlertThresholdType.BETWEEN, + threshold: 5, + thresholdMax: 20, + }) + .expect(200); + expect(alert.body.data.thresholdMax).toBe(20); + + await agent + .put(`/alerts/${alert.body.data._id}`) + .send({ + ...alert.body.data, + dashboardId: dashboard.body.id, + thresholdType: AlertThresholdType.ABOVE, + thresholdMax: undefined, + }) + .expect(200); + + const updated = await agent + .get(`/alerts/${alert.body.data._id}`) + .expect(200); + expect(updated.body.data.thresholdMax).toBeUndefined(); + }); + it('returns channel.webhookId, name, and message in GET list and GET single', async () => { const dashboard = await agent .post('/dashboards') diff --git a/packages/api/src/routers/api/__tests__/dashboard.int.test.ts b/packages/api/src/routers/api/__tests__/dashboard.int.test.ts index 12c9647a75..5c3d44697e 100644 --- a/packages/api/src/routers/api/__tests__/dashboard.int.test.ts +++ b/packages/api/src/routers/api/__tests__/dashboard.int.test.ts @@ -365,6 +365,51 @@ describe('dashboard router', () => { }); }); + // The dashboard editor only ever sets thresholdMax, so switching a tile alert + // off a range comparator has to clear the stored bound on this path too. + it('clears thresholdMax when a tile alert is moved off a range comparator', async () => { + const mockAlert = makeMockAlert(webhook._id.toString()); + const tile = makeTile({ + alert: { + ...mockAlert, + thresholdType: AlertThresholdType.BETWEEN, + threshold: 5, + thresholdMax: 20, + }, + }); + const dashboard = await agent + .post('/dashboards') + .send({ name: 'Test Dashboard', tiles: [tile], tags: [] }) + .expect(200); + + const created = await agent.get(`/alerts`).expect(200); + expect(created.body.data[0].thresholdMax).toBe(20); + + await agent + .patch(`/dashboards/${dashboard.body.id}`) + .send({ + ...dashboard.body, + tiles: [ + { + ...dashboard.body.tiles[0], + config: { + ...dashboard.body.tiles[0].config, + alert: { + ...mockAlert, + thresholdType: AlertThresholdType.ABOVE, + threshold: 5, + }, + }, + }, + ], + }) + .expect(200); + + const updated = await agent.get(`/alerts`).expect(200); + expect(updated.body.data[0].thresholdType).toBe(AlertThresholdType.ABOVE); + expect(updated.body.data[0].thresholdMax).toBeUndefined(); + }); + it('rejects a tile alert with no notification channel', async () => { await agent .post('/dashboards') diff --git a/packages/api/src/routers/api/__tests__/webhooks.int.test.ts b/packages/api/src/routers/api/__tests__/webhooks.int.test.ts index 70c449c043..96a239fb0c 100644 --- a/packages/api/src/routers/api/__tests__/webhooks.int.test.ts +++ b/packages/api/src/routers/api/__tests__/webhooks.int.test.ts @@ -1235,6 +1235,39 @@ describe('webhooks router', () => { slackSpy.mockRestore(); }); + // The point of a test send is that a body written against the documented + // variables renders here exactly as it will on a real firing, so every + // enriched field has to be populated. + it('populates the enriched template variables on a test send', async () => { + const { agent } = await getLoggedInAgent(server); + + await agent + .post('/webhooks/test') + .send({ + service: WebhookService.Generic, + url: 'https://example.com/webhook', + body: '{"text": "test"}', + }) + .expect(200); + + expect(genericSpy).toHaveBeenCalledTimes(1); + // Pinned to the literals in the route, not expect.any: a regression that + // blanked sourceQuery back to '' is exactly what this guards. + expect(genericSpy.mock.calls[0][1]).toMatchObject({ + alertId: 'test-alert-id', + status: 'firing', + alertType: 'search', + comparator: 'between', + threshold: 5, + thresholdMax: 10, + value: 7, + groupKey: 'test-group', + sourceQuery: 'SeverityText: "error"', + note: 'Test webhook — no runbook', + teamId: expect.any(String), + }); + }); + it('resolves masked URL and headers when webhookId is provided', async () => { const { agent, team } = await getLoggedInAgent(server); diff --git a/packages/api/src/routers/api/webhooks.ts b/packages/api/src/routers/api/webhooks.ts index 99b40ec226..390f9b06a9 100644 --- a/packages/api/src/routers/api/webhooks.ts +++ b/packages/api/src/routers/api/webhooks.ts @@ -5,6 +5,7 @@ import type { WebhookTestApiResponse, WebhookUpdateApiResponse, } from '@hyperdx/common-utils/dist/types'; +import { AlertThresholdType } from '@hyperdx/common-utils/dist/types'; import express from 'express'; import { ObjectId } from 'mongodb'; import mongoose from 'mongoose'; @@ -13,8 +14,13 @@ import { z } from 'zod'; import { validateRequest } from 'zod-express-middleware'; import { createWebhook, deleteWebhook } from '@/controllers/webhook'; -import { AlertState } from '@/models/alert'; +import { AlertSource, AlertState } from '@/models/alert'; import Webhook, { WebhookService } from '@/models/webhook'; +import { + ALERT_STATUS_BY_STATE, + ALERT_TYPE_BY_SOURCE, + COMPARATOR_BY_THRESHOLD_TYPE, +} from '@/tasks/checkAlerts/template'; import { handleSendGenericWebhook, handleSendSlackWebhook, @@ -478,9 +484,9 @@ router.post( state: AlertState.ALERT, eventId: 'test-event-id', alertId: 'test-alert-id', - status: 'firing', - alertType: 'search', - comparator: 'between', + status: ALERT_STATUS_BY_STATE[AlertState.ALERT], + alertType: ALERT_TYPE_BY_SOURCE[AlertSource.SAVED_SEARCH], + comparator: COMPARATOR_BY_THRESHOLD_TYPE[AlertThresholdType.BETWEEN], threshold: 5, thresholdMax: 10, value: 7, diff --git a/packages/api/src/tasks/checkAlerts/__tests__/renderAlertTemplate.int.test.ts b/packages/api/src/tasks/checkAlerts/__tests__/renderAlertTemplate.int.test.ts index 70d42f144a..d5e05842ad 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/renderAlertTemplate.int.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/renderAlertTemplate.int.test.ts @@ -1,14 +1,19 @@ import { - AlertChartConfig, AlertState, AlertThresholdType, - DisplayType, + Filter, SourceKind, Tile, } from '@hyperdx/common-utils/dist/types'; import mongoose from 'mongoose'; -import { makeTile } from '@/fixtures'; +import { + makeAlertChartConfig, + makeRawSqlAlertChartConfig, + makeRawSqlAlertTile, + makeTile, + RAW_SQL_ALERT_TEMPLATE, +} from '@/fixtures'; import { AlertSource } from '@/models/alert'; import type { IWebhook } from '@/models/webhook'; import { @@ -87,14 +92,24 @@ const mockClickhouseClient = { const startTime = new Date('2023-03-17T22:10:00.000Z'); const endTime = new Date('2023-03-17T22:15:00.000Z'); +// Shared by the three view builders below, so a knob added for one is +// accepted by renderWithWebhook's `makeView` parameter for all of them. +type ViewOverrides = Partial & { + thresholdType?: AlertThresholdType; + threshold?: number; + thresholdMax?: number; + value?: number; + group?: string; + where?: string; + chartName?: string; + aggCondition?: string; + filters?: Filter[]; + rawSql?: boolean; + tile?: Tile; +}; + const makeSearchView = ( - overrides: Partial & { - thresholdType?: AlertThresholdType; - threshold?: number; - thresholdMax?: number; - value?: number; - group?: string; - } = {}, + overrides: ViewOverrides = {}, ): AlertMessageTemplateDefaultView => ({ alert: { thresholdType: overrides.thresholdType ?? AlertThresholdType.ABOVE, @@ -120,8 +135,9 @@ const makeSearchView = ( id: 'fake-saved-search-id', name: 'My Search', select: 'Body', - where: 'Body: "error"', + where: overrides.where ?? 'Body: "error"', whereLanguage: 'lucene', + ...(overrides.filters != null && { filters: overrides.filters }), orderBy: 'timestamp', source: 'fake-source-id' as any, tags: ['test'], @@ -139,14 +155,7 @@ const makeSearchView = ( const testTile = makeTile({ id: 'test-tile-id' }); const makeTileView = ( - overrides: Partial & { - thresholdType?: AlertThresholdType; - threshold?: number; - thresholdMax?: number; - value?: number; - group?: string; - tile?: Tile; - } = {}, + overrides: ViewOverrides = {}, ): AlertMessageTemplateDefaultView => ({ alert: { thresholdType: overrides.thresholdType ?? AlertThresholdType.ABOVE, @@ -176,52 +185,33 @@ const makeTileView = ( value: overrides.value ?? 10, }); -// An inline alert carries its own chart config, either builder (`where`) or -// raw SQL (`sqlTemplate`) — the two places its query can live. -const makeInlineChartConfig = ( - query: { where: string } | { sqlTemplate: string }, -): AlertChartConfig => - 'sqlTemplate' in query - ? { - name: 'Inline SQL', - configType: 'sql', - connection: 'connection-123', - displayType: DisplayType.Line, - sqlTemplate: query.sqlTemplate, - } - : { - name: 'Inline Chart', - source: 'fake-source-id', - displayType: DisplayType.Line, - select: [ - { - aggFn: 'count', - aggCondition: '', - aggConditionLanguage: 'lucene', - valueExpression: '', - }, - ], - where: query.where, - whereLanguage: 'lucene', - }; - const makeInlineView = ( - query: { where: string } | { sqlTemplate: string }, + overrides: ViewOverrides = {}, ): AlertMessageTemplateDefaultView => ({ alert: { - thresholdType: AlertThresholdType.ABOVE, - threshold: 5, + thresholdType: overrides.thresholdType ?? AlertThresholdType.ABOVE, + threshold: overrides.threshold ?? 5, + thresholdMax: overrides.thresholdMax, source: AlertSource.INLINE, channel: { type: null }, interval: '1m', - chartConfig: makeInlineChartConfig(query), + chartConfig: overrides.rawSql + ? makeRawSqlAlertChartConfig() + : makeAlertChartConfig({ + sourceId: 'fake-source-id', + name: overrides.chartName, + where: overrides.where ?? 'ServiceName: "checkout"', + aggCondition: overrides.aggCondition, + filters: overrides.filters, + }), }, attributes: {}, - granularity: '5 minute', + granularity: '1m', + group: overrides.group, isGroupedAlert: false, startTime, endTime, - value: 10, + value: overrides.value ?? 10, }); const render = async ( @@ -485,9 +475,12 @@ describe('renderAlertTemplate', () => { // The enriched fields are what a receiver routes and dedupes on, so they have // to survive the render, not just the variable builder in isolation. describe('enriched message fields', () => { - const renderView = async ( + const renderWithWebhook = async ( state: AlertState, - base: AlertMessageTemplateDefaultView, + viewOverrides: ViewOverrides = {}, + makeView: ( + overrides: ViewOverrides, + ) => AlertMessageTemplateDefaultView = makeSearchView, ) => { const webhook = castWebhook({ _id: new mongoose.Types.ObjectId(), @@ -497,6 +490,7 @@ describe('enriched message fields', () => { url: 'https://hooks.slack.com/services/x', }); const { dispatcher, dispatched } = makeRecordingDispatcher(); + const base = makeView(viewOverrides); const result = await renderAlertTemplate({ alertProvider, @@ -520,11 +514,6 @@ describe('enriched message fields', () => { return { dispatched, result }; }; - const renderWithWebhook = async ( - state: AlertState, - viewOverrides: Parameters[0] = {}, - ) => renderView(state, makeSearchView(viewOverrides)); - it('carries the alert identity and condition onto the dispatched job', async () => { const { dispatched, result } = await renderWithWebhook(AlertState.ALERT, { group: 'http', @@ -552,65 +541,187 @@ describe('enriched message fields', () => { const { dispatched } = await renderWithWebhook(AlertState.ALERT, { thresholdType: AlertThresholdType.BETWEEN, threshold: 5, - thresholdMax: 7, - value: 6, + thresholdMax: 20, + value: 12, }); expect(dispatched[0].message).toMatchObject({ comparator: 'between', threshold: 5, - thresholdMax: 7, + thresholdMax: 20, }); }); - it('omits the upper bound when the condition is not a range', async () => { + // Switching an alert off a range comparator leaves the old bound on the + // document, and reporting it would advertise a range that no longer fires. + it('drops a stale thresholdMax for a single-bound condition', async () => { const { dispatched } = await renderWithWebhook(AlertState.ALERT, { thresholdType: AlertThresholdType.ABOVE, threshold: 5, - // Set but irrelevant to this comparator — it must not reach the receiver. - thresholdMax: 7, + thresholdMax: 20, }); + expect(dispatched[0].message).toMatchObject({ comparator: '>=' }); expect(dispatched[0].message.thresholdMax).toBeUndefined(); }); - it('reads sourceQuery from the tile a dashboard alert points at', async () => { - const tile = makeTile({ id: 'queried-tile' }); - tile.config = makeInlineChartConfig({ where: 'ServiceName: "checkout"' }); - - const { dispatched } = await renderView( + it("reports an inline alert's query from its own chart config", async () => { + const { dispatched } = await renderWithWebhook( AlertState.ALERT, - makeTileView({ tile }), + {}, + makeInlineView, ); expect(dispatched[0].message).toMatchObject({ - alertType: 'dashboard_chart', + alertType: 'inline_query', sourceQuery: 'ServiceName: "checkout"', }); }); - it('reads sourceQuery from an inline builder alert', async () => { - const { dispatched } = await renderView( + it('reports the SQL of a raw SQL inline alert', async () => { + const { dispatched } = await renderWithWebhook( AlertState.ALERT, - makeInlineView({ where: 'SeverityText: "error"' }), + { rawSql: true }, + makeInlineView, ); expect(dispatched[0].message).toMatchObject({ alertType: 'inline_query', + sourceQuery: RAW_SQL_ALERT_TEMPLATE, + }); + }); + + it("reports a tile alert's query from its tile", async () => { + const { dispatched } = await renderWithWebhook( + AlertState.ALERT, + { tile: makeTile({ id: 'tile-1', where: 'SeverityText: "error"' }) }, + makeTileView, + ); + + expect(dispatched[0].message).toMatchObject({ + alertType: 'dashboard_chart', sourceQuery: 'SeverityText: "error"', }); }); - it('reads sourceQuery from an inline raw SQL alert', async () => { - const { dispatched } = await renderView( + it('reports the SQL of a raw SQL tile alert', async () => { + const { dispatched } = await renderWithWebhook( + AlertState.ALERT, + { tile: makeRawSqlAlertTile({ id: 'tile-1' }) }, + makeTileView, + ); + + expect(dispatched[0].message).toMatchObject({ + alertType: 'dashboard_chart', + sourceQuery: RAW_SQL_ALERT_TEMPLATE, + }); + }); + + // A chart alert is commonly defined purely by the series condition, with the + // chart-level Where left blank. Reporting `where` alone called that + // unconditional. + it("reports a series aggCondition when the chart's where is blank", async () => { + const { dispatched } = await renderWithWebhook( AlertState.ALERT, - makeInlineView({ sqlTemplate: 'SELECT count() FROM otel_logs' }), + { where: '', aggCondition: 'SeverityText: "error"' }, + makeInlineView, ); expect(dispatched[0].message).toMatchObject({ - sourceQuery: 'SELECT count() FROM otel_logs', + sourceQuery: 'SeverityText: "error"', }); }); + + it('brackets the where and the series condition when it joins them', async () => { + const { dispatched } = await renderWithWebhook( + AlertState.ALERT, + { + where: 'ServiceName: "checkout"', + aggCondition: 'SeverityText: "error"', + }, + makeInlineView, + ); + + expect(dispatched[0].message).toMatchObject({ + sourceQuery: '(ServiceName: "checkout") AND (SeverityText: "error")', + }); + }); + + // buildAlertChartConfigFromSavedConfig does not pass a chart's pinned + // filters into the alert query, so naming them would report a condition the + // alert never applied. + it("omits a chart's pinned filters, which the alert query does not apply", async () => { + const { dispatched } = await renderWithWebhook( + AlertState.ALERT, + { + where: 'ServiceName: "checkout"', + filters: [{ type: 'sql', condition: "Env = 'prod'" }], + }, + makeInlineView, + ); + + expect(dispatched[0].message).toMatchObject({ + sourceQuery: 'ServiceName: "checkout"', + }); + }); + + // Only the last series drives the value, so an earlier series' condition is + // not the one that fired. + it('reports only the alerting series condition on a multi-series chart', async () => { + const view = makeInlineView({ where: '' }); + const config = view.alert.chartConfig as { select: unknown[] }; + config.select = [ + { + aggFn: 'count', + aggCondition: 'SeverityText: "warn"', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + { + aggFn: 'count', + aggCondition: 'SeverityText: "error"', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ]; + + const { dispatched } = await renderWithWebhook( + AlertState.ALERT, + {}, + () => view, + ); + + expect(dispatched[0].message).toMatchObject({ + sourceQuery: 'SeverityText: "error"', + }); + }); + + // A saved search's pinned filters DO reach its alert query, unlike a chart's. + it("reports a saved search's pinned filters alongside its where", async () => { + const { dispatched } = await renderWithWebhook(AlertState.ALERT, { + where: 'Body: "error"', + filters: [ + { type: 'sql', condition: "ServiceName = 'checkout'" }, + { type: 'sql_ast', operator: '=', left: 'Region', right: "'us'" }, + ] satisfies Filter[], + }); + + expect(dispatched[0].message).toMatchObject({ + alertType: 'search', + sourceQuery: + "(Body: \"error\") AND (ServiceName = 'checkout') AND (Region = 'us')", + }); + }); + + it('reports an empty sourceQuery when the chart carries no condition', async () => { + const { dispatched } = await renderWithWebhook( + AlertState.ALERT, + { where: '' }, + makeInlineView, + ); + + expect(dispatched[0].message.sourceQuery).toBe(''); + }); }); describe('buildAlertMessageTemplateTitle', () => { @@ -785,7 +896,10 @@ describe('buildAlertMessageTemplateTitle', () => { describe('derived displayName', () => { it('names an inline alert from its chart config', () => { const result = buildAlertMessageTemplateTitle({ - view: makeInlineView({ where: 'level:error' }), + view: makeInlineView({ + where: 'level:error', + chartName: 'Inline Chart', + }), state: AlertState.ALERT, }); diff --git a/packages/api/src/tasks/checkAlerts/template.ts b/packages/api/src/tasks/checkAlerts/template.ts index e4dc30a481..9c9bf7dad6 100644 --- a/packages/api/src/tasks/checkAlerts/template.ts +++ b/packages/api/src/tasks/checkAlerts/template.ts @@ -2,13 +2,19 @@ import { ClickhouseClient } from '@hyperdx/common-utils/dist/clickhouse/node'; import { Metadata } from '@hyperdx/common-utils/dist/core/metadata'; import { renderChartConfig } from '@hyperdx/common-utils/dist/core/renderChartConfig'; import { formatDate, objectHash } from '@hyperdx/common-utils/dist/core/utils'; +import { + isPromqlSavedChartConfig, + isRawSqlSavedChartConfig, +} from '@hyperdx/common-utils/dist/guards'; import { AlertChannelType, AlertThresholdType, ChartConfigWithOptDateRange, DisplayType, + Filter, isRangeThresholdType, pickSampleWeightExpressionProps, + SavedChartConfig, SourceKind, zAlertChannelType, } from '@hyperdx/common-utils/dist/types'; @@ -106,10 +112,75 @@ const describeThreshold = (alert: AlertInput): string => { : `${alert.threshold}`; }; +const describeFilter = (filter: Filter): string => + filter.type === 'sql_ast' + ? `${filter.left} ${filter.operator} ${filter.right}` + : filter.condition; + +const joinConditions = (...parts: (string | undefined)[]): string => { + const present = parts + .map(part => part?.trim()) + .filter((part): part is string => !!part); + // A lone condition needs no brackets; two or more do, so that a part + // carrying its own top-level OR keeps its precedence under the AND join. + // renderChartConfig brackets each group the same way before combining them. + return present.length > 1 + ? present.map(part => `(${part})`).join(' AND ') + : (present[0] ?? ''); +}; + +// Reports only the fields the alert query is actually built from. +// buildAlertChartConfigFromSavedConfig assembles a chart alert field by field +// and passes neither `filters` nor `having`, so a tile or inline alert never +// narrows on those; naming them here would advertise a condition that never +// fired, the same way a stale thresholdMax would. +const describeChartConfigQuery = (config: SavedChartConfig): string => { + if (isRawSqlSavedChartConfig(config)) { + return config.sqlTemplate; + } + // Anticipatory: a PromQL chart cannot be alerted on today. The branch stays + // because a tile's config is the full union, and this guard is what narrows + // the builder case below. + if (isPromqlSavedChartConfig(config)) { + return config.promqlExpression; + } + const select = typeof config.select === 'string' ? [] : (config.select ?? []); + // Only the last series drives the value -- parseAlertData keeps the last + // value column it sees -- so an earlier series' condition is not the one + // that fired. + return joinConditions(config.where, select.at(-1)?.aggCondition); +}; + +// Only a saved-search alert states its query on the saved search. A tile +// alert's lives on the dashboard tile and an inline alert's on the alert +// itself, so both were reported as empty before. +const describeSourceQuery = ( + alert: AlertInput, + savedSearch?: ISavedSearch | null, + dashboard?: IDashboard | null, +): string => { + if (alert.source === AlertSource.INLINE) { + return alert.chartConfig ? describeChartConfigQuery(alert.chartConfig) : ''; + } + if (alert.source === AlertSource.TILE) { + const tile = dashboard?.tiles.find(t => t.id === alert.tileId); + return tile ? describeChartConfigQuery(tile.config) : ''; + } + // A saved-search alert counts rows rather than aggregating a series, so it + // has no aggCondition -- but the query does apply its pinned filters. + return savedSearch + ? joinConditions( + savedSearch.where, + ...(savedSearch.filters ?? []).map(describeFilter), + ) + : ''; +}; + // Mappings for the enriched webhook template variables. These turn internal // enums into stable, consumer-friendly strings a receiver can branch on -// without knowing HyperDX's internals. -const ALERT_STATUS_BY_STATE: Record = { +// without knowing HyperDX's internals. Exported so the "Send test" sample +// payload indexes them rather than restating the strings. +export const ALERT_STATUS_BY_STATE: Record = { [AlertState.ALERT]: 'firing', [AlertState.OK]: 'resolved', [AlertState.INSUFFICIENT_DATA]: 'no_data', @@ -118,18 +189,19 @@ const ALERT_STATUS_BY_STATE: Record = { [AlertState.ERROR]: 'error', }; -const COMPARATOR_BY_THRESHOLD_TYPE: Record = { - [AlertThresholdType.ABOVE]: '>=', - [AlertThresholdType.ABOVE_EXCLUSIVE]: '>', - [AlertThresholdType.BELOW]: '<', - [AlertThresholdType.BELOW_OR_EQUAL]: '<=', - [AlertThresholdType.EQUAL]: '=', - [AlertThresholdType.NOT_EQUAL]: '!=', - [AlertThresholdType.BETWEEN]: 'between', - [AlertThresholdType.NOT_BETWEEN]: 'outside', -}; +export const COMPARATOR_BY_THRESHOLD_TYPE: Record = + { + [AlertThresholdType.ABOVE]: '>=', + [AlertThresholdType.ABOVE_EXCLUSIVE]: '>', + [AlertThresholdType.BELOW]: '<', + [AlertThresholdType.BELOW_OR_EQUAL]: '<=', + [AlertThresholdType.EQUAL]: '=', + [AlertThresholdType.NOT_EQUAL]: '!=', + [AlertThresholdType.BETWEEN]: 'between', + [AlertThresholdType.NOT_BETWEEN]: 'outside', + }; -const ALERT_TYPE_BY_SOURCE: Record = { +export const ALERT_TYPE_BY_SOURCE: Record = { [AlertSource.SAVED_SEARCH]: 'search', [AlertSource.TILE]: 'dashboard_chart', // Detached alert: the chart config lives on the alert itself, so there is @@ -137,40 +209,11 @@ const ALERT_TYPE_BY_SOURCE: Record = { [AlertSource.INLINE]: 'inline_query', }; -/** - * The persisted query behind an alert, as a receiver would route on it. Each - * alert source keeps it somewhere different: a saved search on the search - * itself, an inline alert on the alert, a tile on its dashboard. - */ -const getAlertSourceQuery = ({ - alert, - dashboard, - savedSearch, -}: AlertMessageTemplateDefaultView): string => { - if (alert.source === AlertSource.SAVED_SEARCH) { - return savedSearch?.where ?? ''; - } - - const chartConfig = - alert.source === AlertSource.INLINE - ? alert.chartConfig - : dashboard?.tiles.find(t => t.id === alert.tileId)?.config; - if (chartConfig == null) { - return ''; - } - // Narrowed on `configType` rather than through isRawSqlSavedChartConfig / - // isPromqlSavedChartConfig: those predicate on SavedChartConfig, and an - // inline alert's AlertChartConfig is built from the without-alert variants, - // so the guards can't subtract a member from that union. - if ('configType' in chartConfig) { - // Raw SQL keeps the whole query in sqlTemplate. A PromQL chart can't be - // alerted on, but a tile's config is the full union, so it lands here. - return chartConfig.configType === 'sql' ? chartConfig.sqlTemplate : ''; - } - return chartConfig.where ?? ''; -}; - const MAX_MESSAGE_LENGTH = 500; +// A raw SQL template or a long filter set is unbounded in the schema, and +// {{sourceQuery}} goes straight into a webhook body, so cap it the way the +// sample-log block is capped. +const MAX_SOURCE_QUERY_LENGTH = 2000; const NOTIFY_FN_NAME = '__hdx_notify_channel__'; const IS_MATCH_FN_NAME = 'is_match'; @@ -594,15 +637,18 @@ export const renderAlertTemplate = async ({ alertType: alert.source ? ALERT_TYPE_BY_SOURCE[alert.source] : '', comparator: COMPARATOR_BY_THRESHOLD_TYPE[alert.thresholdType], threshold: alert.threshold, - // Only a range comparator has an upper bound; leaving it undefined - // elsewhere renders the variable empty rather than as a bound that - // isn't part of the condition. + // Gated on the comparator rather than trusting the stored field: + // makeAlertUpdate clears it going forward, but an alert written before + // that shipped still carries a bound from a range it no longer has. thresholdMax: isRangeThresholdType(alert.thresholdType) ? alert.thresholdMax : undefined, value, groupKey: group ?? '', - sourceQuery: getAlertSourceQuery(view), + sourceQuery: truncateString( + describeSourceQuery(alert, savedSearch, dashboard), + MAX_SOURCE_QUERY_LENGTH, + ), teamId, note: alert.note ?? '', }, diff --git a/packages/api/src/tasks/checkAlerts/transports/__tests__/generic.test.ts b/packages/api/src/tasks/checkAlerts/transports/__tests__/generic.test.ts index b2dec2d2f6..bee2e3656b 100644 --- a/packages/api/src/tasks/checkAlerts/transports/__tests__/generic.test.ts +++ b/packages/api/src/tasks/checkAlerts/transports/__tests__/generic.test.ts @@ -4,6 +4,7 @@ import { ObjectId } from 'mongodb'; import { AlertState } from '@/models/alert'; import { buildWebhookTemplateVariables, + createHandlebarsWithHelpers, getWebhookFetchTimeoutMs, handleSendGenericWebhook, } from '@/tasks/checkAlerts/transports/generic'; @@ -135,3 +136,27 @@ describe('buildWebhookTemplateVariables', () => { expect(vars.thresholdMax).toBeUndefined(); }); }); + +// The guard published in docs/alert-webhook-template-variables.md is the only +// way a receiver can put an optional number in an unquoted JSON slot, so a +// change to the helper set that breaks it would break every template using it. +describe('the documented guard for an optional numeric variable', () => { + const render = (thresholdMax?: number) => + createHandlebarsWithHelpers().compile( + '{"threshold": {{threshold}}{{#unless (eq thresholdMax undefined)}}, "threshold_max": {{thresholdMax}}{{/unless}}\n}', + // Same options as sendGenericWebhook: noEscape is load-bearing, because + // escapeJsonString already emits \" and HTML-escaping would mangle it. + { noEscape: true }, + )( + buildWebhookTemplateVariables({ ...message, threshold: 5, thresholdMax }), + ); + + it.each([ + [undefined, { threshold: 5 }], + [20, { threshold: 5, threshold_max: 20 }], + // A range bounded at zero is real, and `{{#if}}` would drop it. + [0, { threshold: 5, threshold_max: 0 }], + ])('renders valid JSON for thresholdMax=%s', (thresholdMax, expected) => { + expect(JSON.parse(render(thresholdMax))).toEqual(expected); + }); +}); diff --git a/packages/api/src/tasks/checkAlerts/transports/generic.ts b/packages/api/src/tasks/checkAlerts/transports/generic.ts index 75763eb467..27a184724c 100644 --- a/packages/api/src/tasks/checkAlerts/transports/generic.ts +++ b/packages/api/src/tasks/checkAlerts/transports/generic.ts @@ -1,5 +1,9 @@ import { objectHash } from '@hyperdx/common-utils/dist/core/utils'; -import { WebhookService } from '@hyperdx/common-utils/dist/types'; +import { + DEFAULT_GENERIC_WEBHOOK_BODY, + WebhookService, + WebhookTemplateVariable, +} from '@hyperdx/common-utils/dist/types'; import Handlebars from 'handlebars'; import { performance } from 'perf_hooks'; import { serializeError } from 'serialize-error'; @@ -59,13 +63,6 @@ export const logBlockedWebhookDelivery = ( } }; -// Fallback body for a generic/incidentio webhook persisted without one. Mirrors -// the default template the UI form applies (WebhookForm.tsx) so a webhook -// created via the API/MCP (where body is optional) still fires with a sensible -// payload instead of crashing Handlebars.compile on an undefined template. -const DEFAULT_GENERIC_WEBHOOK_BODY_TEMPLATE = - '{"text": "{{title}} | {{body}} | {{link}} | {{state}} | {{startTime}} | {{endTime}} | {{eventId}}"}'; - // Renders a Unix-ms timestamp as an ISO-8601 string, or '' if it isn't a valid // time (so a template slot never becomes the literal "Invalid Date"). const toIsoTimestamp = (t: number): string => { @@ -81,28 +78,32 @@ const toIsoTimestamp = (t: number): string => { * comparator, sourceQuery, …) enable routing/dedup in receivers without * parsing the message body. */ -export const buildWebhookTemplateVariables = (message: Message) => ({ - body: escapeJsonString(message.body), - endTime: message.endTime, - endTimeISO: escapeJsonString(toIsoTimestamp(message.endTime)), - eventId: message.eventId, - link: escapeJsonString(message.hdxLink), - startTime: message.startTime, - startTimeISO: escapeJsonString(toIsoTimestamp(message.startTime)), - state: message.state, - title: escapeJsonString(message.title), - alertId: escapeJsonString(message.alertId ?? ''), - alertType: escapeJsonString(message.alertType ?? ''), - comparator: escapeJsonString(message.comparator ?? ''), - groupKey: escapeJsonString(message.groupKey ?? ''), - note: escapeJsonString(message.note ?? ''), - sourceQuery: escapeJsonString(message.sourceQuery ?? ''), - status: escapeJsonString(message.status ?? ''), - teamId: escapeJsonString(message.teamId ?? ''), - threshold: message.threshold, - thresholdMax: message.thresholdMax, - value: message.value, -}); +// `satisfies` rather than a return type, so callers keep the precise value +// types while the published variable list stays enforced in both directions: a +// name added there must be built here, and one built here must be published. +export const buildWebhookTemplateVariables = (message: Message) => + ({ + body: escapeJsonString(message.body), + endTime: message.endTime, + endTimeISO: escapeJsonString(toIsoTimestamp(message.endTime)), + eventId: message.eventId, + link: escapeJsonString(message.hdxLink), + startTime: message.startTime, + startTimeISO: escapeJsonString(toIsoTimestamp(message.startTime)), + state: message.state, + title: escapeJsonString(message.title), + alertId: escapeJsonString(message.alertId ?? ''), + alertType: escapeJsonString(message.alertType ?? ''), + comparator: escapeJsonString(message.comparator ?? ''), + groupKey: escapeJsonString(message.groupKey ?? ''), + note: escapeJsonString(message.note ?? ''), + sourceQuery: escapeJsonString(message.sourceQuery ?? ''), + status: escapeJsonString(message.status ?? ''), + teamId: escapeJsonString(message.teamId ?? ''), + threshold: message.threshold, + thresholdMax: message.thresholdMax, + value: message.value, + }) satisfies Record; /** * Creates a Handlebars instance with common helpers registered. @@ -213,11 +214,10 @@ const sendGenericWebhook = async ( const handlebars = createHandlebarsWithHelpers(); // Handlebars.compile throws on undefined; the API/MCP create paths allow an - // absent body (the UI form applies the default). An explicit "" is honored. + // absent body (the UI form applies the same default). An explicit "" is + // honored. const bodyTemplate = - webhook.body == null - ? DEFAULT_GENERIC_WEBHOOK_BODY_TEMPLATE - : webhook.body; + webhook.body == null ? DEFAULT_GENERIC_WEBHOOK_BODY : webhook.body; body = handlebars.compile(bodyTemplate, { noEscape: true, diff --git a/packages/api/src/tasks/checkAlerts/transports/types.ts b/packages/api/src/tasks/checkAlerts/transports/types.ts index b2612b9eff..5c12bd7739 100644 --- a/packages/api/src/tasks/checkAlerts/transports/types.ts +++ b/packages/api/src/tasks/checkAlerts/transports/types.ts @@ -16,10 +16,11 @@ export interface Message { // Optional so existing callers and non-enriched templates are unaffected. alertId?: string; status?: string; // firing | resolved | no_data | pending - alertType?: string; // search | dashboard_chart + alertType?: string; // search | dashboard_chart | inline_query comparator?: string; // >=, >, <=, <, =, !=, between, outside threshold?: number; - thresholdMax?: number; // upper bound; only set when comparator is between/outside + // Upper bound of a `between`/`outside` condition; unset for the others. + thresholdMax?: number; value?: number; // the value that triggered/resolved the alert groupKey?: string; sourceQuery?: string; // the search expr / SQL that defines the alert diff --git a/packages/app/src/components/TeamSettings/WebhookForm.tsx b/packages/app/src/components/TeamSettings/WebhookForm.tsx index 940134679b..0c479fcd99 100644 --- a/packages/app/src/components/TeamSettings/WebhookForm.tsx +++ b/packages/app/src/components/TeamSettings/WebhookForm.tsx @@ -6,8 +6,11 @@ import { json, jsonParseLinter } from '@codemirror/lang-json'; import { linter } from '@codemirror/lint'; import { AlertState, + DEFAULT_GENERIC_WEBHOOK_BODY, + WEBHOOK_TEMPLATE_VARIABLES, WebhookApiData, WebhookService, + WebhookTemplateVariable, } from '@hyperdx/common-utils/dist/types'; import { isValidSlackUrl } from '@hyperdx/common-utils/dist/validation'; import { @@ -32,66 +35,45 @@ import api from '@/api'; import { useBrandDisplayName } from '@/theme/ThemeProvider'; import { isValidUrl } from '@/utils'; -const DEFAULT_GENERIC_WEBHOOK_BODY = [ - '{{title}}', - '{{body}}', - '{{link}}', - '{{state}}', - '{{startTime}}', - '{{endTime}}', - '{{eventId}}', -]; -const DEFAULT_GENERIC_WEBHOOK_BODY_TEMPLATE = - DEFAULT_GENERIC_WEBHOOK_BODY.join(' | '); +// The default body and the variable list both come from common-utils, so this +// form and the API's fallback cannot drift. The docs page restates them by +// hand; a unit test in common-utils keeps that table honest. +// Keyed by WebhookTemplateVariable so a variable added in common-utils cannot +// reach the transports without a description here. +const describeTemplateVariable = ( + brandName: string, +): Record => ({ + title: 'Alert title', + body: 'Rendered message body (markdown)', + link: `Deep link back into ${brandName}`, + state: 'Raw internal alert state', + status: 'firing, resolved, no_data, pending or error', + eventId: 'Unique id for this firing', + alertId: 'Stable id of the alert — the key to dedupe on', + alertType: 'search, dashboard_chart or inline_query', + comparator: '>=, >, <, <=, =, !=, between or outside', + threshold: 'The configured threshold (number)', + thresholdMax: 'Upper bound of a between/outside range (number)', + value: 'Value that triggered or resolved the alert (number)', + groupKey: 'The breaching group, if grouped', + sourceQuery: 'Search expression or SQL behind the alert', + startTime: 'Window start, Unix ms (number)', + endTime: 'Window end, Unix ms (number)', + startTimeISO: 'Window start, ISO-8601', + endTimeISO: 'Window end, ISO-8601', + teamId: 'Team the alert belongs to', + note: "Alert's note, commonly a runbook link", +}); -// Mirrors buildWebhookTemplateVariables in -// packages/api/src/tasks/checkAlerts/transports/generic.ts — keep in sync when -// variables are added or removed there. export const getWebhookTemplateVariables = ( brandName: string, -): { name: string; description: string }[] => [ - { name: '{{title}}', description: 'Alert title' }, - { name: '{{body}}', description: 'Rendered message body (markdown)' }, - { name: '{{link}}', description: `Deep link back into ${brandName}` }, - { name: '{{state}}', description: 'Raw internal alert state' }, - { - name: '{{status}}', - description: 'firing, resolved, no_data, pending or error', - }, - { name: '{{eventId}}', description: 'Unique id for this firing' }, - { - name: '{{alertId}}', - description: 'Stable id of the alert — the key to dedupe on', - }, - { - name: '{{alertType}}', - description: 'search, dashboard_chart or inline_query', - }, - { - name: '{{comparator}}', - description: '>=, >, <, <=, =, !=, between or outside', - }, - { name: '{{threshold}}', description: 'The configured threshold (number)' }, - { - name: '{{thresholdMax}}', - description: 'Upper bound of a between/outside range (number)', - }, - { - name: '{{value}}', - description: 'Value that triggered or resolved the alert (number)', - }, - { name: '{{groupKey}}', description: 'The breaching group, if grouped' }, - { - name: '{{sourceQuery}}', - description: 'Search expression or SQL behind the alert', - }, - { name: '{{startTime}}', description: 'Window start, Unix ms (number)' }, - { name: '{{endTime}}', description: 'Window end, Unix ms (number)' }, - { name: '{{startTimeISO}}', description: 'Window start, ISO-8601' }, - { name: '{{endTimeISO}}', description: 'Window end, ISO-8601' }, - { name: '{{teamId}}', description: 'Team the alert belongs to' }, - { name: '{{note}}', description: "Alert's note, commonly a runbook link" }, -]; +): { name: string; description: string }[] => { + const descriptions = describeTemplateVariable(brandName); + return WEBHOOK_TEMPLATE_VARIABLES.map(name => ({ + name: `{{${name}}}`, + description: descriptions[name], + })); +}; const jsonLinterWithEmptyCheck = () => (editorView: EditorView) => { const text = editorView.state.doc.toString().trim(); @@ -177,7 +159,7 @@ export function WebhookForm({ let defaultBody = body; if (!body) { if (service === WebhookService.Generic) { - defaultBody = `{"text": "${DEFAULT_GENERIC_WEBHOOK_BODY_TEMPLATE}"}`; + defaultBody = DEFAULT_GENERIC_WEBHOOK_BODY; } else if (service === WebhookService.IncidentIO) { defaultBody = `{ "title": "{{title}}", @@ -251,7 +233,7 @@ export function WebhookForm({ let defaultBody = body; if (!body) { if (service === WebhookService.Generic) { - defaultBody = `{"text": "${DEFAULT_GENERIC_WEBHOOK_BODY_TEMPLATE}"}`; + defaultBody = DEFAULT_GENERIC_WEBHOOK_BODY; } else if (service === WebhookService.IncidentIO) { defaultBody = `{ "title": "{{title}}", @@ -436,9 +418,7 @@ export function WebhookForm({ extensions={[ json(), linter(jsonLinterWithEmptyCheck()), - placeholder( - `{\n\t"text": "${DEFAULT_GENERIC_WEBHOOK_BODY_TEMPLATE}"\n}`, - ), + placeholder(DEFAULT_GENERIC_WEBHOOK_BODY), ]} theme="dark" value={field.value} diff --git a/packages/common-utils/src/__tests__/webhookTemplateVariables.test.ts b/packages/common-utils/src/__tests__/webhookTemplateVariables.test.ts new file mode 100644 index 0000000000..e3a90763e8 --- /dev/null +++ b/packages/common-utils/src/__tests__/webhookTemplateVariables.test.ts @@ -0,0 +1,30 @@ +import fs from 'fs'; +import path from 'path'; + +import { WEBHOOK_TEMPLATE_VARIABLES } from '@/types'; + +// The form callout and the API's fallback body both derive from +// WEBHOOK_TEMPLATE_VARIABLES, so they cannot fall behind it. The docs table is +// hand-written and has no such guarantee, which is what this pins. +describe('docs/alert-webhook-template-variables.md', () => { + const docPath = path.join( + __dirname, + '../../../../docs/alert-webhook-template-variables.md', + ); + + it('documents every published template variable', () => { + const doc = fs.readFileSync(docPath, 'utf8'); + // Table rows only: the prose and examples also contain {{#if}}/{{else}}, + // and a row may pair two variables (`{{startTime}}` / `{{endTime}}`). + const documented = new Set( + doc + .split('\n') + .filter(line => line.startsWith('|')) + .flatMap(line => [...line.matchAll(/\{\{(\w+)\}\}/g)].map(m => m[1])), + ); + + expect([...WEBHOOK_TEMPLATE_VARIABLES].sort()).toEqual( + [...documented].sort(), + ); + }); +}); diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index 37f95e6330..17cd34f54c 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -568,6 +568,55 @@ export enum WebhookService { IncidentIO = 'incidentio', } +/** + * Every variable a Generic/incident.io webhook body template can reference. + * The one published list: `buildWebhookTemplateVariables` (packages/api) is + * typed against it so a variable cannot be built without appearing here, and + * the webhook form renders it as the in-product list. Documented in + * docs/alert-webhook-template-variables.md. + * + * The first seven are also the default body template the form applies. + */ +export const DEFAULT_WEBHOOK_TEMPLATE_VARIABLES = [ + 'title', + 'body', + 'link', + 'state', + 'startTime', + 'endTime', + 'eventId', +] as const; + +export const WEBHOOK_TEMPLATE_VARIABLES = [ + ...DEFAULT_WEBHOOK_TEMPLATE_VARIABLES, + 'startTimeISO', + 'endTimeISO', + 'alertId', + 'status', + 'alertType', + 'comparator', + 'threshold', + 'thresholdMax', + 'value', + 'groupKey', + 'sourceQuery', + 'teamId', + 'note', +] as const; + +export type WebhookTemplateVariable = + (typeof WEBHOOK_TEMPLATE_VARIABLES)[number]; + +/** + * The body a Generic or incident.io webhook gets when it is saved without one. + * Published here so the form's default, the form's editor placeholder and the + * API's fallback are one string — the payload shape used to be respelled at + * each of those, and a change to it had to be repeated in all of them. + */ +export const DEFAULT_GENERIC_WEBHOOK_BODY = `{"text": "${DEFAULT_WEBHOOK_TEMPLATE_VARIABLES.map( + name => `{{${name}}}`, +).join(' | ')}"}`; + /** * Base webhook schema (matches backend IWebhook but with JSON-serialized types). * When making changes here, consider if they need to be made to the external