diff --git a/.changeset/alert-webhook-source-query-threshold-max.md b/.changeset/alert-webhook-source-query-threshold-max.md new file mode 100644 index 0000000000..16a8513095 --- /dev/null +++ b/.changeset/alert-webhook-source-query-threshold-max.md @@ -0,0 +1,19 @@ +--- +'@hyperdx/api': patch +--- + +Fix `{{sourceQuery}}` returning empty for inline-query and dashboard-tile +alerts. It read only the saved search's filter, so alerts backed by a chart +config — where the query lives on the alert or the tile — advertised a variable +that never rendered. It now resolves the query from whichever config backs the +alert: the builder `where` or the raw `sqlTemplate`. + +Add `{{thresholdMax}}`, the upper bound of a `between` / `outside` condition. +Receivers previously saw only the lower bound and could not reconstruct the +range that fired. It renders empty for every other comparator. + +Test Webhook now sends a sample value for every template variable. It carried +only the original seven, so a body using an enriched variable rendered it empty +— and because `threshold`, `thresholdMax` and `value` are emitted unquoted, a +body like `{"value": {{value}}}` was sent as `{"value": }` and rejected, +failing the test for a template that works on a real firing. diff --git a/.changeset/webhook-form-template-variables.md b/.changeset/webhook-form-template-variables.md new file mode 100644 index 0000000000..9de3b10504 --- /dev/null +++ b/.changeset/webhook-form-template-variables.md @@ -0,0 +1,11 @@ +--- +'@hyperdx/app': patch +--- + +List every supported template variable in the webhook form, including the +enriched set added to Generic and incident.io bodies (`{{alertId}}`, +`{{status}}`, `{{alertType}}`, `{{comparator}}`, `{{threshold}}`, +`{{thresholdMax}}`, `{{value}}`, `{{groupKey}}`, `{{sourceQuery}}`, +`{{teamId}}`, `{{note}}` and ISO-8601 `{{startTimeISO}}` / `{{endTimeISO}}`). +Each variable now carries a one-line description, so a webhook body can be +written without leaving the form. diff --git a/docs/alert-webhook-template-variables.md b/docs/alert-webhook-template-variables.md index f9fcbece9b..194ce0ed08 100644 --- a/docs/alert-webhook-template-variables.md +++ b/docs/alert-webhook-template-variables.md @@ -18,17 +18,21 @@ a HyperDX alert without parsing the human-readable message body. | `{{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. | | `{{value}}` | number | The value that triggered or resolved the alert. | | `{{groupKey}}` | string | The breaching group, for a grouped alert. | -| `{{sourceQuery}}` | string | The search expression or SQL behind the alert. | +| `{{sourceQuery}}` | string | The query behind the alert: the saved search's filter, the chart's `where`, or the raw SQL. | | `{{teamId}}` | string | Team the alert belongs to. | | `{{note}}` | string | The alert's freeform note — commonly a runbook link. | Strings are JSON-escaped, so they are safe to drop into a quoted slot. -Numbers (`startTime`, `endTime`, `threshold`, `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 dashboard-tile alert has an empty `{{sourceQuery}}`. +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 variable in an unquoted numeric slot produces invalid JSON, so guard +the optional numbers: `{{#if thresholdMax}}"max": {{thresholdMax}},{{/if}}`. ## Example 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 c94cb76d4c..70c449c043 100644 --- a/packages/api/src/routers/api/__tests__/webhooks.int.test.ts +++ b/packages/api/src/routers/api/__tests__/webhooks.int.test.ts @@ -4,6 +4,8 @@ import { getLoggedInAgent, getServer } from '@/fixtures'; import Alert from '@/models/alert'; import Webhook, { WebhookService } from '@/models/webhook'; import * as transports from '@/tasks/checkAlerts/transports'; +import { buildWebhookTemplateVariables } from '@/tasks/checkAlerts/transports/generic'; +import type { Message } from '@/tasks/checkAlerts/transports/types'; const MOCK_WEBHOOK = { name: 'Test Webhook', @@ -1296,6 +1298,35 @@ describe('webhooks router', () => { }); }); + // The webhook form documents every one of these variables directly above + // the Test Webhook button, so a test send has to exercise the same set — + // an unset raw number renders `"value": ` and the receiver rejects a + // template that works on a real firing. + it('sends a sample value for every template variable', async () => { + const { agent, team } = await getLoggedInAgent(server); + + await agent + .post('/webhooks/test') + .send({ + service: WebhookService.Generic, + url: 'https://example.com/webhook', + body: '{"text": "test"}', + }) + .expect(200); + + const sent: Message = genericSpy.mock.calls[0][1]; + const rendered = buildWebhookTemplateVariables(sent); + + for (const [name, value] of Object.entries(rendered)) { + expect(`${name}=${value}`).not.toMatch(/=(undefined|null)?$/); + } + // Emitted unquoted, so these are what break a body when left unset. + for (const name of ['threshold', 'thresholdMax', 'value'] as const) { + expect(typeof rendered[name]).toBe('number'); + } + expect(sent.teamId).toBe(team._id.toString()); + }); + it('returns 404 when webhookId does not exist', async () => { const { agent } = await getLoggedInAgent(server); diff --git a/packages/api/src/routers/api/webhooks.ts b/packages/api/src/routers/api/webhooks.ts index aeab7c793f..99b40ec226 100644 --- a/packages/api/src/routers/api/webhooks.ts +++ b/packages/api/src/routers/api/webhooks.ts @@ -8,6 +8,7 @@ import type { import express from 'express'; import { ObjectId } from 'mongodb'; import mongoose from 'mongoose'; +import ms from 'ms'; import { z } from 'zod'; import { validateRequest } from 'zod-express-middleware'; @@ -18,6 +19,7 @@ import { handleSendGenericWebhook, handleSendSlackWebhook, } from '@/tasks/checkAlerts/transports'; +import type { Message } from '@/tasks/checkAlerts/transports/types'; import { isDuplicateKeyError } from '@/utils/errors'; import { validateWebhookUrl, @@ -459,15 +461,33 @@ router.post( body, }); - // Send test message - const testMessage = { + // Every field a real firing sends, so a body written against the + // documented variables renders here exactly as it will in production. + // The enriched variables especially: `threshold`, `thresholdMax` and + // `value` are emitted raw, so leaving them unset renders `"value": ` + // and the receiver rejects a template that would have worked. + // A range comparator is the useful sample — it is the one case where + // `thresholdMax` is populated. + const now = Date.now(); + const testMessage: Message = { hdxLink: 'https://hyperdx.io', title: 'Test Webhook from HyperDX', body: 'This is a test message to verify your webhook configuration is working correctly.', - startTime: Date.now(), - endTime: Date.now(), - state: AlertState.INSUFFICIENT_DATA, + startTime: now - ms('5m'), + endTime: now, + state: AlertState.ALERT, eventId: 'test-event-id', + alertId: 'test-alert-id', + status: 'firing', + alertType: 'search', + comparator: 'between', + threshold: 5, + thresholdMax: 10, + value: 7, + groupKey: 'test-group', + sourceQuery: 'SeverityText: "error"', + teamId: teamId.toString(), + note: 'Test webhook — no runbook', }; const testChannel = { type: 'webhook' as const, channel: testWebhook }; 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 d19ec269c9..19d716fc14 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/renderAlertTemplate.int.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/renderAlertTemplate.int.test.ts @@ -1,7 +1,10 @@ import { + AlertChartConfig, AlertState, AlertThresholdType, + DisplayType, SourceKind, + Tile, } from '@hyperdx/common-utils/dist/types'; import mongoose from 'mongoose'; @@ -142,6 +145,7 @@ const makeTileView = ( thresholdMax?: number; value?: number; group?: string; + tile?: Tile; } = {}, ): AlertMessageTemplateDefaultView => ({ alert: { @@ -151,13 +155,13 @@ const makeTileView = ( source: AlertSource.TILE, channel: { type: null }, interval: '1m', - tileId: 'test-tile-id', + tileId: (overrides.tile ?? testTile).id, }, dashboard: { _id: new mongoose.Types.ObjectId(), id: 'id-123', name: 'My Dashboard', - tiles: [testTile], + tiles: [overrides.tile ?? testTile], team: 'team-123' as any, tags: ['test'], createdAt: new Date(), @@ -172,6 +176,54 @@ 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 }, +): AlertMessageTemplateDefaultView => ({ + alert: { + thresholdType: AlertThresholdType.ABOVE, + threshold: 5, + source: AlertSource.INLINE, + channel: { type: null }, + interval: '1m', + chartConfig: makeInlineChartConfig(query), + }, + attributes: {}, + granularity: '5 minute', + isGroupedAlert: false, + startTime, + endTime, + value: 10, +}); + const render = async ( view: AlertMessageTemplateDefaultView, state: AlertState, @@ -433,9 +485,9 @@ 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 renderWithWebhook = async ( + const renderView = async ( state: AlertState, - viewOverrides: Parameters[0] = {}, + base: AlertMessageTemplateDefaultView, ) => { const webhook = castWebhook({ _id: new mongoose.Types.ObjectId(), @@ -445,7 +497,6 @@ describe('enriched message fields', () => { url: 'https://hooks.slack.com/services/x', }); const { dispatcher, dispatched } = makeRecordingDispatcher(); - const base = makeSearchView(viewOverrides); const result = await renderAlertTemplate({ alertProvider, @@ -469,6 +520,11 @@ 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', @@ -491,6 +547,70 @@ describe('enriched message fields', () => { expect(dispatched[0].message).toMatchObject({ status: 'resolved' }); }); + + it('carries both bounds of a range condition', async () => { + const { dispatched } = await renderWithWebhook(AlertState.ALERT, { + thresholdType: AlertThresholdType.BETWEEN, + threshold: 5, + thresholdMax: 7, + value: 6, + }); + + expect(dispatched[0].message).toMatchObject({ + comparator: 'between', + threshold: 5, + thresholdMax: 7, + }); + }); + + it('omits the upper bound when the condition is not a range', 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, + }); + + 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( + AlertState.ALERT, + makeTileView({ tile }), + ); + + expect(dispatched[0].message).toMatchObject({ + alertType: 'dashboard_chart', + sourceQuery: 'ServiceName: "checkout"', + }); + }); + + it('reads sourceQuery from an inline builder alert', async () => { + const { dispatched } = await renderView( + AlertState.ALERT, + makeInlineView({ where: 'SeverityText: "error"' }), + ); + + expect(dispatched[0].message).toMatchObject({ + alertType: 'inline_query', + sourceQuery: 'SeverityText: "error"', + }); + }); + + it('reads sourceQuery from an inline raw SQL alert', async () => { + const { dispatched } = await renderView( + AlertState.ALERT, + makeInlineView({ sqlTemplate: 'SELECT count() FROM otel_logs' }), + ); + + expect(dispatched[0].message).toMatchObject({ + sourceQuery: 'SELECT count() FROM otel_logs', + }); + }); }); describe('buildAlertMessageTemplateTitle', () => { diff --git a/packages/api/src/tasks/checkAlerts/template.ts b/packages/api/src/tasks/checkAlerts/template.ts index 598c45791f..1d4778ac65 100644 --- a/packages/api/src/tasks/checkAlerts/template.ts +++ b/packages/api/src/tasks/checkAlerts/template.ts @@ -136,6 +136,39 @@ 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; const NOTIFY_FN_NAME = '__hdx_notify_channel__'; const IS_MATCH_FN_NAME = 'is_match'; @@ -556,9 +589,15 @@ 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. + thresholdMax: isRangeThresholdType(alert.thresholdType) + ? alert.thresholdMax + : undefined, value, groupKey: group ?? '', - sourceQuery: savedSearch?.where ?? '', + sourceQuery: getAlertSourceQuery(view), 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 54c75f932f..b2dec2d2f6 100644 --- a/packages/api/src/tasks/checkAlerts/transports/__tests__/generic.test.ts +++ b/packages/api/src/tasks/checkAlerts/transports/__tests__/generic.test.ts @@ -96,8 +96,9 @@ describe('buildWebhookTemplateVariables', () => { alertId: 'alert-1', status: 'firing', alertType: 'search', - comparator: '>=', + comparator: 'between', threshold: 5, + thresholdMax: 10, value: 42, groupKey: 'checkout', sourceQuery: 'Body: "error"', @@ -111,8 +112,9 @@ describe('buildWebhookTemplateVariables', () => { alertId: 'alert-1', status: 'firing', alertType: 'search', - comparator: '>=', + comparator: 'between', threshold: 5, + thresholdMax: 10, value: 42, groupKey: 'checkout', teamId: 'team-1', @@ -129,5 +131,7 @@ describe('buildWebhookTemplateVariables', () => { expect(vars.status).toBe(''); expect(vars.note).toBe(''); expect(vars.startTimeISO).toBe(new Date(0).toISOString()); + // A raw number renders as an empty slot when absent, not "undefined". + expect(vars.thresholdMax).toBeUndefined(); }); }); diff --git a/packages/api/src/tasks/checkAlerts/transports/generic.ts b/packages/api/src/tasks/checkAlerts/transports/generic.ts index 1d82ce7713..75763eb467 100644 --- a/packages/api/src/tasks/checkAlerts/transports/generic.ts +++ b/packages/api/src/tasks/checkAlerts/transports/generic.ts @@ -100,6 +100,7 @@ export const buildWebhookTemplateVariables = (message: Message) => ({ status: escapeJsonString(message.status ?? ''), teamId: escapeJsonString(message.teamId ?? ''), threshold: message.threshold, + thresholdMax: message.thresholdMax, value: message.value, }); diff --git a/packages/api/src/tasks/checkAlerts/transports/types.ts b/packages/api/src/tasks/checkAlerts/transports/types.ts index 4fc161ef4c..b2612b9eff 100644 --- a/packages/api/src/tasks/checkAlerts/transports/types.ts +++ b/packages/api/src/tasks/checkAlerts/transports/types.ts @@ -19,6 +19,7 @@ export interface Message { alertType?: string; // search | dashboard_chart comparator?: string; // >=, >, <=, <, =, !=, between, outside threshold?: number; + thresholdMax?: number; // upper bound; only set when comparator is between/outside 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 4bc4d3886c..940134679b 100644 --- a/packages/app/src/components/TeamSettings/WebhookForm.tsx +++ b/packages/app/src/components/TeamSettings/WebhookForm.tsx @@ -13,8 +13,10 @@ import { isValidSlackUrl } from '@hyperdx/common-utils/dist/validation'; import { Alert, Button, + Code, Group, Radio, + SimpleGrid, Stack, Text, TextInput, @@ -42,6 +44,55 @@ const DEFAULT_GENERIC_WEBHOOK_BODY = [ const DEFAULT_GENERIC_WEBHOOK_BODY_TEMPLATE = DEFAULT_GENERIC_WEBHOOK_BODY.join(' | '); +// 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" }, +]; + const jsonLinterWithEmptyCheck = () => (editorView: EditorView) => { const text = editorView.state.doc.toString().trim(); if (text === '') return []; @@ -277,6 +328,7 @@ export function WebhookForm({ }; const service = useWatch({ control: form.control, name: 'service' }); + const templateVariables = getWebhookTemplateVariables(brandName); const headersText = useWatch({ control: form.control, name: 'headers' }); const hasMaskedHeaders = isEditing && !!headersText?.includes('****'); @@ -401,19 +453,30 @@ export function WebhookForm({ className="mb-4" color="gray" > - - Currently the body supports the following message template - variables: - -
- - {DEFAULT_GENERIC_WEBHOOK_BODY.map((body, index) => ( - - {body} - {index < DEFAULT_GENERIC_WEBHOOK_BODY.length - 1 && ', '} - + + The body supports the following template variables: + + + {templateVariables.map(({ name, description }) => ( + + {name} + + {description} + + ))} - + + + Strings are JSON-escaped, so they are safe inside quotes. Numbers + are emitted raw for unquoted slots. A variable the alert + doesn't carry renders as an empty string. + , ]} diff --git a/packages/app/src/components/TeamSettings/__tests__/WebhookForm.test.tsx b/packages/app/src/components/TeamSettings/__tests__/WebhookForm.test.tsx new file mode 100644 index 0000000000..dedd97335d --- /dev/null +++ b/packages/app/src/components/TeamSettings/__tests__/WebhookForm.test.tsx @@ -0,0 +1,73 @@ +import { MantineProvider } from '@mantine/core'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { + getWebhookTemplateVariables, + WebhookForm, +} from '@/components/TeamSettings/WebhookForm'; + +jest.mock('@/api', () => ({ + __esModule: true, + default: { + useSaveWebhook: () => ({ mutateAsync: jest.fn(), isPending: false }), + useUpdateWebhook: () => ({ mutateAsync: jest.fn(), isPending: false }), + useTestWebhook: () => ({ mutateAsync: jest.fn(), isPending: false }), + }, +})); + +// CodeMirror needs layout APIs jsdom doesn't provide. +jest.mock('@uiw/react-codemirror', () => ({ + __esModule: true, + default: ({ + value, + onChange, + }: { + value?: string; + onChange?: (value: string) => void; + }) => ( +