feat: Support Alerting for PromQL Charts - #3114
Conversation
🦋 Changeset detectedLatest commit: 9fae4d8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@Aryainguz is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR adds PromQL alert configuration, validation, scheduled evaluation, per-series state tracking, and Prometheus/ClickHouse query support. Since the previous review, it:
Confidence Score: 5/5The PR appears safe to merge; no new actionable issue remains after the latest changes, and all previous findings are resolved. The latest changes correctly connect PromQL evaluation to the shared bounded query path, preserve multi-series handling, resolve source tables through loaded source metadata, and record query failures without advancing successful history.
|
| Filename | Overview |
|---|---|
| packages/api/src/tasks/checkAlerts/index.ts | Adds PromQL evaluation, per-series state transitions, variable substitution, error recording, and notification integration. |
| packages/api/src/controllers/timeseriesEngine.ts | Extracts the bounded ClickHouse PromQL range query into a reusable controller helper. |
| packages/api/src/routers/api/prometheus.ts | Reuses the extracted ClickHouse range-query helper without changing the response path. |
| packages/common-utils/src/types.ts | Adds PromQL chart configurations to the shared inline-alert schema. |
| packages/app/src/components/ChartEditor/utils.ts | Preserves PromQL alert configuration when converting editor state into a saved chart. |
| packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts | Classifies the PromQL legend as evaluation-inert while retaining query fields as lossy for external round trips. |
| packages/api/src/tasks/checkAlerts/tests/evaluatePromqlAlert.test.ts | Covers Prometheus and ClickHouse evaluation, multiple series, errors, tag conversion, and empty results. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[PromQL tile or inline alert] --> B[Scheduled alert worker]
B --> C{Connection type}
C -->|Prometheus endpoint| D[Prometheus query_range]
C -->|ClickHouse| E[prometheusQueryRange table function]
D --> F[Extract latest value per label set]
E --> F
F --> G[Compare each series with threshold]
G --> H[Update grouped history and state]
G --> I[Send firing or recovery notification]
C -->|Query failure| J[Record retryable alert error]
Reviews (4): Last reviewed commit: "fix(promql-alerts): address PR review fe..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Deep ReviewScope: PromQL chart alerting — All twelve prior review findings (evaluator never called, wrong ClickHouse target, single-series evaluation, missing type import, schema drift-guard, ✅ No critical issues found. 🟡 P2 -- recommended
🔵 P3 nitpicks (6)
Reviewers (5): security, testing, maintainability, reliability, previous-comments. Correctness, adversarial, API-contract, and TypeScript dimensions were verified directly against the code by the orchestrator: schema widening ( Testing gaps:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const startMs = Math.floor(startSec * 1000); | ||
| const endMs = Math.floor(endSec * 1000); | ||
|
|
||
| const resp = await client.query({ |
There was a problem hiding this comment.
🔵 minor — The prometheusQueryRange query, its client, and its timeouts are copied from the Prometheus router
The SQL string and query_params shape at lines 1045-1061 are a verbatim copy of queryRangeHandler in packages/api/src/routers/api/prometheus.ts:502-518, and 30_000/30 restate the exported PROMETHEUS_CH_TIMEOUT_MS / PROMETHEUS_MAX_EXECUTION_SEC (prometheus.ts:163, controllers/timeseriesEngine.ts:14). It also drops the router's max_result_rows cap. Extract one queryPrometheusRangeFromClickHouse helper (alongside formatMatrixResponse in prometheus.ts, or in controllers/timeseriesEngine.ts next to queryLabelValues) and call it from both. Related: the await import('@hyperdx/common-utils/dist/clickhouse/node') at line 1031 duplicates the static import already at line 10, and bypasses the API's ClickhouseClient wrapper in @/clickhouse that wires the pino logger — which is what prometheus.ts:491 uses.
| }), | ||
| }); | ||
|
|
||
| const result = await evaluatePromqlAlert({ |
There was a problem hiding this comment.
🔵 minor — The ClickHouse test asserts around the two parameters that are actually wrong
expect.objectContaining({ expr, startMs, endMs, stepSec }) deliberately omits db and table — the only two params the function derives rather than passes through, and the two that are incorrect (a source ObjectId and a regex guess). The test passes while the query targets a nonexistent table. Assert the full query_params object, including db and table resolved from the alert's source.
PR Review11 finding(s): 🔴 1 critical · 🟠 4 major · 🔵 6 minor 8 posted as inline comment(s) on the changed lines. 3 listed below. Findings outside the changed lines
1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. |
- Remove unused displayTypeSupportsPromQLAlerts workaround and inline the boolean flag with explicit HDX-4636 tracker in UI - Fix alertHasGroupBy comments to clarify PromQL multi-series behavior - Remove invalid type leak (variables) in schedule offset normalizer - Fix ISource type cast in provider fetching to safely use .from.databaseName - Clean up unused imports and stale comments around PromQL in inline alerts - Fix test fixture using 'any' by properly typing PromqlSavedChartConfig
| 'alignDateRangeToGranularity', | ||
| 'alternateRowBackground', | ||
| // 'alert', // TODO: Support alerts on PromQL (HDX-4636) | ||
| 'alert', |
There was a problem hiding this comment.
🟠 major — PromQL configs now persist alert, but the PromQL editor renders no alert UI — the alert becomes invisible and uneditable
PromqlChartEditor (packages/app/src/components/ChartEditor/PromqlChartEditor.tsx) renders no "Add Alert" button and no alert form (only ChartEditorControls/RawSqlChartEditor do), and the display-type cleanup effect in EditTimeChartForm.tsx:294-303 only clears alert on a displayType change, not on a configType change. So configuring an alert on a builder/SQL chart and then switching the segmented control to PromQL now silently persists and evaluates that alert against the PromQL expression with no way to see or remove it. Either render the alert editor in PromqlChartEditor (the stated goal of this PR) or clear alert when configType becomes promql until that UI exists.
| ? displayTypeSupportsRawSqlAlerts(chart.config.displayType) | ||
| : isPromQL | ||
| ? displayTypeSupportsPromQLAlerts(chart.config.displayType) | ||
| ? false // PromQL alert UI not yet implemented (HDX-4636) |
There was a problem hiding this comment.
🟠 major — Dashboard tile alert UI is hardcoded to false, so the new TILE PromQL evaluation path is unreachable
Both call sites replace displayTypeSupportsPromQLAlerts(...) with false // PromQL alert UI not yet implemented, which contradicts the changeset ("configuring and evaluating PromQL-based alerts directly from the chart explorer and dashboards") and leaves the whole AlertTaskType.TILE PromQL branch in checkAlerts/index.ts:1438 — including the dashboard-variable substitution wiring — dead. Either enable the tile alert affordance for PromQL display types or drop the TILE half of the backend branch from this PR.
| const value = res.value; | ||
|
|
||
| const history = getOrCreateHistory(groupKey); | ||
| history.lastValues.push({ count: value, startTime: dateRange[1] }); |
There was a problem hiding this comment.
🟠 major — Only the last point of the window range is evaluated, so backfilled windows are silently skipped
getAlertEvaluationDateRange + calcAlertDateRange (packages/api/src/tasks/util.ts:47) return a range covering up to 50 missed windows, and the builder path iterates expectedBuckets to evaluate each one (index.ts:1751-1892). evaluatePromqlAlert takes only series.values[values.length - 1] and the branch then reports backfilledBuckets = 0, so after a worker outage every intermediate window (and any breach in it) is dropped, and shouldFireBasedOnConsecutiveWindows sees one history row for many windows. Iterate the query_range/time_series points bucket-by-bucket like the time-series path instead of keeping only the last.
| await sendNotificationIfResolved(previous, history, groupKey); | ||
| } | ||
| } | ||
| } catch (e) { |
There was a problem hiding this comment.
🟠 major — PromQL evaluation failures are swallowed: no alert error is recorded and no query metrics are emitted
The catch logs and returns without calling alertProvider.recordAlertErrors, unlike the ClickHouse path (index.ts:1662-1685), so a broken expression, unreachable Prometheus, or missing connection leaves the alert with no ERROR history row and nothing visible in the UI — the alert just appears to never evaluate. It also skips recordOperationOutcome({operation:'alerts.query'}), alertQueryFailuresCounter and evaluationAnalytics.queryDurationMs, so PromQL alerts are invisible to the alert-query SLI. Reuse makeQueryAlertError + recordAlertErrors and record the same query metrics around the PromQL call.
P0: Fix tags tuple iteration in ClickHouse path - evaluatePromqlAlert now iterates series.tags as [key, value] tuples matching formatMatrixResponse, instead of Object.entries (which produced numeric indices and broke group key building entirely) P2: Throw on degraded Prometheus status instead of returning null - status !== 'success' now throws, preventing false auto-resolves from masking genuine Prometheus outages as empty results P2: Route PromQL failures through standard error recording - PromQL catch block now calls makeQueryAlertError, alertQueryFailuresCounter, recordOperationOutcome, and alertProvider.recordAlertErrors — matching the SQL path so failed windows get typed error history rows, operator metrics, and retry P3: Extract queryPrometheusRangeFromClickHouse helper - Moved to @/controllers/timeseriesEngine (with PROMETHEUS_MAX_RESULT_ROWS cap that was missing from the alert path); both prometheus router and evaluatePromqlAlert now use the same helper, eliminating duplication P3: Use API ClickhouseClient in evaluatePromqlAlert - Swapped common-utils direct ClickhouseClient for @/clickhouse wrapper so telemetry/logger wiring is included in the alert evaluation path P3: Reuse PROMETHEUS_CH_TIMEOUT_MS constant - Exported from prometheus router; evaluatePromqlAlert now imports and reuses it instead of hardcoding 30_000 / max_execution_time: 30 P3: Move legendTemplate to EVALUATION_INERT_CONFIG_KEYS - Was erroneously classified as KNOWN_LOSSY; it is render-only so a blind external GET -> PUT should not fail on it P3: Fix drift-guard test to label PromQL variant correctly - i === 0 ? 'builder' : 'raw SQL' now handles i === 2 as 'promql' Tests: Update evaluatePromqlAlert.test.ts - Mock queryPrometheusRangeFromClickHouse (new helper) instead of raw ClickhouseClient; fix tags fixtures to use tuple arrays; add test asserting status !== success throws
| throw new Error(`Connection ${connectionId} not found for PromQL alert`); | ||
| } | ||
|
|
||
| const endSec = dateRange[1].getTime() / 1000; |
There was a problem hiding this comment.
🟠 major — PromQL evaluation reads only the last point, so backfilled windows are silently skipped
dateRange spans from the previous history's createdAt to now and can cover many windows (getAlertEvaluationDateRange at line 686 plus calcAlertDateRange, which only truncates at MAX_NUM_WINDOWS). The builder path iterates every expected bucket (line 1818) and reports backfilledBuckets; evaluatePromqlAlert takes series.values[series.values.length - 1] and the branch hardcodes evaluationAnalytics.backfilledBuckets = 0. After a worker delay or a run of failed evaluations, every intermediate window is dropped without ever being evaluated (and numConsecutiveWindows never accumulates), because the next run starts from this run's createdAt. Return all points per series and loop over timeBucketByGranularity(dateRange[0], dateRange[1], ${windowSizeInMins} minute) the way the time-series path does.
| client, | ||
| // ISource always has `from` (it is on BaseSourceSchema); the nullable | ||
| // source param covers the case where no source is wired to the alert. | ||
| databaseName: source?.from.databaseName ?? 'default', |
There was a problem hiding this comment.
🟠 major — ClickHouse PromQL path guesses default.otel_metrics_gauge when the alert has no source
source?.from.databaseName ?? 'default' / source?.from.tableName ?? 'otel_metrics_gauge' invents a table for prometheusQueryRange, which requires a TimeSeries-engine table — otel_metrics_gauge is the OTel gauge MergeTree table, so the query fails (or, worse, silently reads the wrong table if one by that name exists). The equivalent HTTP route refuses rather than guessing: packages/api/src/routers/api/prometheus.ts:476 returns 400 when table is missing. Throw a descriptive error when a non-Prometheus connection has no source from, instead of defaulting.
| 'alignDateRangeToGranularity', | ||
| 'alternateRowBackground', | ||
| // 'alert', // TODO: Support alerts on PromQL (HDX-4636) | ||
| 'alert', |
There was a problem hiding this comment.
🟠 major — No UI exists to add or edit a PromQL alert; the only reachable path is a stale carry-over
PromqlChartEditor.tsx contains no alert affordance (no TileAlertEditor, no "Add alert" button — unlike RawSqlChartEditor.tsx:299), and DBDashboardPage.tsx:846,1057 now hardcode false for PromQL tiles, so nothing ever sets form.alert on a PromQL config deliberately. What persisting 'alert' here does enable is the leftover case: a user configures an alert in Builder/SQL mode and flips the configType SegmentedControl to PromQL (the clearing effect at EditTimeChartForm.tsx:294 only fires on displayType change), leaving "Save alert" live in ChartActionBar. With the isPromqlSavedChartConfig guard removed from buildInlineAlertPayload, that click now POSTs a PromQL payload that 400s (see the alerts.ts finding) instead of being a no-op. Either ship the alert editor for PromQL, or clear alert on configType change and keep the guard until the UI lands.
| : undefined; | ||
|
|
||
| try { | ||
| const promqlResults = await evaluatePromqlAlert({ |
There was a problem hiding this comment.
🔵 minor — The PromQL branch records only failures for the alerts.query SLI
The success path never calls recordOperationOutcome({ operation: 'alerts.query', outcome: 'success', ... }) and never sets evaluationAnalytics.queryDurationMs, while the catch at line 1509 does record errors — so the availability SLI for PromQL alerts reads as 0% success. Also, the error path measures from evalStartedAt (the start of the whole evaluation) rather than from a query-start timestamp, unlike queryStartedAt at line 1660. Capture a queryStartedAt around the evaluatePromqlAlert call and record both outcomes from it.
| const parsed = parseFloat(lastPoint[1]); | ||
| if (Number.isFinite(parsed)) { | ||
| // Format Prometheus metric object as a HyperDX group key: key1:"val1", key2:"val2" | ||
| const group = series.metric |
There was a problem hiding this comment.
🔵 minor — Series labels are discarded: no notification attributes, and the group key format differs from every other alert type
The builder path builds group keys as k:v (line 1888) and passes the parsed labels through as attributes so templates can render {{attributes.*}} (line 1908). evaluatePromqlAlert formats k:"v" and returns only the joined string, so PromQL alert notifications render an inconsistent group label and always have empty attributes. Return the label map alongside group, format the key as k:v, and pass the map as attributes to trySendNotification.
| const json = await resp.json<any>(); | ||
| if (!Array.isArray(json?.data)) return null; | ||
|
|
||
| const results: Array<{ group: string; value: number }> = []; |
There was a problem hiding this comment.
🔵 minor — The ClickHouse result parsing duplicates formatMatrixResponse, and the two backend branches duplicate each other
packages/api/src/routers/api/prometheus.ts:122 (formatMatrixResponse) already converts { tags, time_series } rows into { metric, values }. Calling it on json.data would let both branches share a single "take the last point, build the group key" loop instead of maintaining two copies that already differ (the Prometheus branch parseFloats the value and skips non-finite points, the ClickHouse branch does neither).
| @@ -0,0 +1,283 @@ | |||
| import mongoose from 'mongoose'; | |||
There was a problem hiding this comment.
🔵 minor — The ~150-line PromQL branch in processAlert has no test coverage
The new tests only exercise the query helper with both backends mocked; nothing covers threshold firing, PENDING/consecutive-window handling, auto-resolve, the missing-series recovery loop, or the variables substitution argument. checkAlerts.int.test.ts is where the equivalent builder/raw-SQL state machine is pinned — add a PromQL case there so a regression in the state transitions is caught.
| import { ISavedSearch } from '@/models/savedSearch'; | ||
| import { ISource } from '@/models/source'; | ||
| import { IWebhook } from '@/models/webhook'; | ||
| import { |
There was a problem hiding this comment.
🔵 minor — The check-alerts worker now imports an Express router for a constant and a URL helper
Importing @/routers/api/prometheus pulls express.Router(), express.urlencoded, and @/middleware/auth into the worker process just to reach PROMETHEUS_CH_TIMEOUT_MS and joinPrometheusUpstreamUrl (which is why the constant had to be exported). Move both to @/controllers/timeseriesEngine, where this PR already moved queryPrometheusRangeFromClickHouse, and have the router import them from there.
|
@Aryainguz please let me know when this is ready to review. Don't feel like you have to address every AI comment, some will push you to expand the scope of the PR unnecessarily and fix tangential bugs. We'd rather see a well-scoped, easy to review PR. We're still working on dialing in the agent reviewer. And if you find the scope increasing too much, feel free to add a brief high level plan in the corresponding issue, we can discuss, and then tackle this in a few well-scoped PRs. |
|
Hi @pulpdrew, I'm working on end to end implementation for same from dashboard to alerts for this, sharing recording of same on my local testing, should I include frontend changes for this in this PR only or created a well scoped subsequent PRs after this ? Screen.Recording.2026-09-13.at.8.47.48.PM.mov |
|
If the UI changes are small, feel free to include them here. If they're several hundred lines or more, it would be best to split them out. When taking a look at the alerts UI, please be sure to consider the new alert details page as well, in case there are any changes that need to be made there. |
2 participants
Description
This PR introduces the ability to create and evaluate alerts based on PromQL charts. Previously, alerting was restricted to standard ClickHouse builder configurations, but this adds full end-to-end support for triggering alerts off of Prometheus queries.
Changes Made
AlertChartConfigSchemaand thedisplayTypeSupportsPromQLAlertshelper to permit PromQL chart configs in alert payloads.checkAlerts/index.tsto cleanly separate the PromQL evaluation path from the standard ClickHouse builder path.evaluatePromqlAlertwhich proxies the alert query directly to the Prometheus endpoint (if configured) or theprometheusQueryRangeClickHouse table function.evaluatePromqlAlertcovering both the standard Prometheus API and ClickHouse proxy fallback scenarios.Verification
yarn ci:unit src/tasks/checkAlerts/__tests__/evaluatePromqlAlert.test.ts).Issue
Fixes #3113