fix(api): make {{sourceQuery}} match the condition the alert evaluates

.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.

docs/alert-webhook-template-variables.md

@@ -17,22 +17,61 @@ 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. |
 
 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
 

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<typeof internalAlertSchema>,
@@ -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,
       });

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 = ({

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')

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')

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);