Conversation
🦋 Changeset detectedLatest commit: 72470f7 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-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Deep ReviewScope: PR #3114 — feat: PromQL Dashboards & Alerting (23 files, ~1,868 insertions; All 8 prior review findings (PromQL never evaluating, wrong ClickHouse target, single-series-only, ungrouped-stores-groups, query-failure telemetry, ✅ No critical (P0/P1) issues found. The union widening is backward-compatible, injection sinks are parameterized/encoded, tenant isolation holds, and the query-failure path records proper telemetry. The items below are recommendations. 🟡 P2 — recommended
🔵 P3 nitpicks (9)
Reviewers (12): security, adversarial, reliability, performance, api-contract, kieran-typescript, testing, maintainability, project-standards, previous-comments, agent-native, learnings. Testing gaps:
Coverage notes: The correctness reviewer (dispatched at Opus tier) did not return before synthesis was required; its dimension is partially covered by the orchestrator's own verification of the bucket math, threshold/state transitions, and auto-resolve loop, plus the adversarial reviewer (also Opus tier). Report-only mode ran no validation pass. Security found no exploitable issues but flagged a pre-existing, by-design SSRF surface (any team member can set a Connection No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
![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 Review
15 finding(s): 🔴 0 critical · 🟠 6 major · 🔵 9 minor 12 posted as inline comment(s) on the changed lines. 3 listed below. Findings outside the changed lines
2 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
- 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. |
Aryainguz
changed the title
- Merged main to fix conflicts with PR hyperdxio#3146 (multi-expression charts) - Resolved conflict in PromqlChartEditor.tsx preserving Alert UI with the new useFieldArray structure - Centralized duplicated backend routing helpers from prometheus.ts and checkAlerts into timeseriesEngine.ts - Updated evaluatePromqlAlert to evaluate against the last expression of a chart config, mirroring SQL builder charts
| }) | ||
| ) { | ||
| // ClickHouse 26.6+ with the prometheus_api_v1 handler configured. | ||
| const chUpstream = new URL(connection.host); |
There was a problem hiding this comment.
🔴 critical — ClickHouse HTTP-API path queries /api/v1/query_range instead of /prometheus/api/v1/query_range
When clickhouseServesPrometheusHttpApi returns true, queryPrometheusHttp joins /api/v1/query_range onto the bare ClickHouse host. ClickHouse serves this handler under CLICKHOUSE_PROMETHEUS_API_PREFIX (/prometheus/api/v1), so every PromQL alert on a 26.6+ server with the handler configured gets a 404 and records QUERY_ERROR on every run. Pass the path in and use ${CLICKHOUSE_PROMETHEUS_API_PREFIX}/query_range here, the same way routers/api/prometheus.ts queryRangeHandler does. No test covers this branch, so add one.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| : savedConfig; | ||
| const rawExpression = resolvedConfig.promqlExpression; | ||
| const promqlExpression = Array.isArray(rawExpression) | ||
| ? rawExpression[rawExpression.length - 1].expression |
There was a problem hiding this comment.
🟠 major — Alert evaluates a different expression than the tile renders for Number charts, and crashes or queries on blank rows
Number tiles keep every stored expression (formPromqlExpressions doesn't truncate), but getQueriedPromqlSeries renders only the first non-blank one, while the alert picks the raw last entry. The alert also ignores queryType/reducer. A tile switched from Line (2 expressions) to Number therefore alerts on a series the user can't see. The raw last entry can also be blank (a 400 on every run), and an empty array throws a TypeError. Use getQueriedPromqlSeries(resolvedConfig) from core/promql.ts and pick the series consistently with the display type.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| > | ||
| Table | ||
| </Tabs.Tab> | ||
| {tableSource?.kind !== SourceKind.Promql && ( |
There was a problem hiding this comment.
🟠 major — Table tab is hidden for PromQL sources even though PromQL tables are supported
isPromqlDisplayType includes DisplayType.Table, and DBTableChart renders PromQL configs. Hiding the Table tab whenever tableSource.kind === Promql stops users creating PromQL tables, and leaves existing PromQL table tiles with no active tab. Keep the Table tab and hide only Search/Heatmap/Patterns.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| } | ||
| }; | ||
|
|
||
| const isPromQL = |
There was a problem hiding this comment.
🟠 major — About 300-line PromQL branch copies the existing time-series state machine
The empty-bucket handling, per-group ALERT/PENDING/OK transitions, auto-resolve, notification loop and updateAlertState are copied almost verbatim from the builder path at ~L2026–2215. That contradicts the PR's 'without duplicating the evaluation loop' claim, and future fixes will have to be made twice. Instead, convert PrometheusMatrixResult[] into per-bucket {groupKey, value, attributes} rows and feed them into one shared evaluation loop. That also removes a subtle divergence: the copied merge condition at L1689 differs from the original's.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| }) | ||
| ) { | ||
| // ClickHouse 26.6+ with the prometheus_api_v1 handler configured. | ||
| const chUpstream = new URL(connection.host); |
There was a problem hiding this comment.
🔵 minor — HTTP-API upstream hand-builds settings and drops the max_result_rows limit
The code builds chUpstream by hand and sets only max_execution_time, so PromQL alert evaluations on ClickHouse run without the max_result_rows bound that the proxy enforces. Use the clickhousePrometheusUpstream(connection.host) helper this PR moved into timeseriesEngine.ts. Also use PROMETHEUS_CH_TIMEOUT_MS instead of the hardcoded 30_000 in queryPrometheusHttp.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| ); | ||
| }); | ||
|
|
||
| it('should have __name__ stripped from metric map', async () => { |
There was a problem hiding this comment.
🔵 minor — Test name says __name__ is stripped, but the assertion (and code) keep it
The test asserts metric: { __name__: 'up', host: 'A' }, and the comment claims processAlert strips __name__ from the group key, but processAlert deliberately keeps it (__name__:up, host:node-1). Rename the test and fix the comment so it pins the real contract.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| setValue('alert', undefined); | ||
| } | ||
| }, [configType, displayType, previousDisplayType, setValue]); | ||
| }, [configType, displayType, isPromqlInput, previousDisplayType, setValue]); |
There was a problem hiding this comment.
🔵 minor — isPromqlInput added to deps, but PromQL still uses the builder alert-support check
The effect body never reads isPromqlInput. PromQL display types go through displayTypeSupportsBuilderAlerts rather than the displayTypeSupportsPromQLAlerts that this PR enables, so the two lists can drift apart. Add a isPromqlInput ? displayTypeSupportsPromQLAlerts(displayType) branch.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| } | ||
| > | ||
| <IconBell size={14} className="me-2" /> | ||
| Add alert |
There was a problem hiding this comment.
🔵 minor — 'Add alert' button block copied from RawSqlChartEditor
This block is identical to RawSqlChartEditor.tsx:299-317 (setValue with DEFAULT_TILE_ALERT plus displayName, IS_LOCAL_MODE gate, bell icon). The repo's REQUIRED DRY rule says to extract a shared AddTileAlertButton component, e.g. next to TileAlertEditor, and use it in both editors.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| useEffect( | ||
| () => { | ||
| if (shouldBeImmediate) { | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
🔵 minor — Bare eslint-disable-next-line suppresses every rule on unrelated lines
The PR adds rule-less disables at L105 and L205 on code it doesn't otherwise change, which hides any future lint error on those lines. Name the specific rule (react-hooks/set-state-in-effect, as EditTimeChartForm.tsx:765 does) or drop these unrelated edits.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| return url.toString(); | ||
| } | ||
|
|
||
| function newClickhouseClient(connection: { |
There was a problem hiding this comment.
🔵 minor — Security and behaviour rationale comments deleted during the move
The move dropped the explanations for joinPrometheusUpstreamUrl (VictoriaMetrics prefix, #3046, opaque-URL guard), clickhousePrometheusUpstream (why limits are pinned on the host so callers can't loosen them), CALLER_SETTABLE_PARAM_KEYS (the extra_label tenant-scope bypass), and isClientDisconnect, even though the code is unchanged. Carry these doc comments over to timeseriesEngine.ts and restore the ones left in place.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| throw new Error('Expected builder chart config for saved search alert'); | ||
| } | ||
| try { | ||
| const withClauses = await computeAliasWithClauses( |
There was a problem hiding this comment.
🟠 major — Refactor dropped aliasWithClauses = withClauses, so saved-search sample lines lose their alias WITH clauses
When the SAVED_SEARCH block was moved below the PromQL branch, the assignment was lost. aliasWithClauses (line 1285) is now always undefined when sampleLinesFor → fetchSampleLines({ aliasWith }) runs. As a result, every saved-search notification whose WHERE uses a select alias (e.g. toString(Body) AS body) builds a failing or empty sample-row query. Put aliasWithClauses = withClauses; back inside the try block.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| upstreamHost: string, | ||
| headers?: Record<string, string>, | ||
| ): Promise<PrometheusMatrixResult[] | null> { | ||
| const url = joinPrometheusUpstreamUrl(upstreamHost, '/api/v1/query_range'); |
There was a problem hiding this comment.
🟠 major — ClickHouse prometheus_api_v1 path requests /api/v1/query_range instead of /prometheus/api/v1/query_range, and sends no database/table
The router calls ${CLICKHOUSE_PROMETHEUS_API_PREFIX}/query_range for ClickHouse (routers/api/prometheus.ts:425) and passes database/table so the handler can pick the TimeSeries table. queryPrometheusHttp always joins /api/v1/query_range and sends neither param. So on CH 26.6+ with the handler configured, every PromQL alert gets a 404 (or queries the wrong table) and records QUERY_ERROR. Pass the path in as a parameter (use the prefix for ClickHouse) and set database/table from source.from. Also build the host with clickhousePrometheusUpstream() instead of the hand-rolled chUpstream, which only pins max_execution_time and drops max_result_rows. None of the unit tests reach this branch because the mocked client makes the probe return false.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| windowSizeInMins: number; | ||
| variables?: ChartVariable[]; | ||
| }): Promise<PrometheusMatrixResult[] | null> { | ||
| const connection = await getConnectionById(teamId, connectionId, true); |
There was a problem hiding this comment.
🟠 major — PromQL alerts run against the source's connection and require a source, ignoring the config's required connection
In PromqlBaseChartConfigSchema, connection is required and source is optional. However, getTileDetails/getInlineAlertDetails in providers/default.ts only special-case raw SQL. PromQL configs therefore fall through to Source.findOne({ _id: config.source }), and a PromQL tile without a source (e.g. on a Prometheus-endpoint connection) is dropped with 'source not found'. When a source does exist, connectionId is source.connection, not savedConfig.connection. Add a PromQL branch in the provider, like the raw SQL one: load the connection from config.connection and the source as optional metadata. The int test builds details directly with createAlertDetails and never goes through the provider, so it cannot catch this.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| > | ||
| Table | ||
| </Tabs.Tab> | ||
| {tableSource?.kind !== SourceKind.Promql && ( |
There was a problem hiding this comment.
🟠 major — Table tab hidden for PromQL sources although Table is a supported PromQL display type
isPromqlDisplayType still includes DisplayType.Table, and DBTableChart renders PromQL configs. Hiding the tab means an existing PromQL Table tile opens with no active tab and can't be switched back to Table. Keep the Table tab visible, and hide only Search/Heatmap/Patterns.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| } | ||
| }; | ||
|
|
||
| const isPromQL = |
There was a problem hiding this comment.
🔵 minor — PromQL branch copies ~200 lines of the time-series evaluation loop instead of reusing it
The empty-bucket handling, per-group state machine, missing-group auto-resolve and notification loop are near-verbatim copies of lines ~2026-2217. The hand-rolled expectedBuckets loop also duplicates timeBucketByGranularity. The copies will drift apart. Convert the PromQL matrix into the same {bucketStart → rows} shape and feed it into the existing loop, so there is one evaluation path, which is what the PR description says it does.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const queryStartedAt = performance.now(); | ||
| try { | ||
| promqlResults = await evaluatePromqlAlert({ | ||
| savedConfig: savedConfig as PromqlSavedChartConfig, |
There was a problem hiding this comment.
🔵 minor — savedConfig as PromqlSavedChartConfig cast after re-deriving the config with duplicate ternaries
isPromQL and savedConfig repeat the same INLINE/TILE ternary, and the second then needs an as cast, which the TypeScript convention asks to avoid. Derive savedConfig once, then narrow it with isPromqlSavedChartConfig(savedConfig) so the type flows through without a cast.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| setValue('alert', undefined); | ||
| } | ||
| }, [configType, displayType, previousDisplayType, setValue]); | ||
| }, [configType, displayType, isPromqlInput, previousDisplayType, setValue]); |
There was a problem hiding this comment.
🔵 minor — isPromqlInput added to the deps but the effect still uses builder alert rules for PromQL
For configType === 'promql' the effect calls displayTypeSupportsBuilderAlerts, and isPromqlInput is never read. It works only because displayTypeSupportsPromQLAlerts currently lists the same three types, which makes it a third copy of that list. Branch on promql and call displayTypeSupportsPromQLAlerts, or make the PromQL helper delegate to the builder one.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| ); | ||
| }); | ||
|
|
||
| it('should have __name__ stripped from metric map', async () => { |
There was a problem hiding this comment.
🔵 minor — Test name says __name__ is stripped but the test asserts it is kept
The test asserts metric: { __name__: 'up', host: 'A' }, which contradicts its title. Also, processAlert doesn't strip __name__ from the group key (the int test expects __name__:up, host:node-1). Rename the test to describe the actual contract, and fix the comment that says processAlert strips it.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| // e.g. VictoriaMetrics's `extra_label`, which a Connection host may pin as a | ||
| // tenant-isolation scope -- and un-pin or override it, even though no | ||
| // legitimate caller ever sends that key. | ||
| // Only real Prometheus API params are ever caller-settable in proxyToPrometheus. |
There was a problem hiding this comment.
🔵 minor — Moving helpers deleted the security rationale for CALLER_SETTABLE_PARAM_KEYS and isClientDisconnect
The removed comments explained that the allowlist stops callers from overriding a host-pinned extra_label tenant scope, and why res.destroyed must not be used. The moved helpers also lost the #3046 link and the opaque-URL/double-slash notes. None of this changed behaviour, so restore the comments here and alongside the moved functions in timeseriesEngine.ts.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| useEffect( | ||
| () => { | ||
| if (shouldBeImmediate) { | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
🔵 minor — Bare eslint-disable-next-line added to unrelated hooks disables every rule
The two unscoped disables in useDebounce/useLocalStorage have nothing to do with PromQL, and they suppress all lint rules on those lines. Name the specific rule (e.g. react-hooks/set-state-in-effect), or drop the change from this PR.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| }) | ||
| ) { | ||
| // ClickHouse 26.6+ with the prometheus_api_v1 handler configured. | ||
| const chUpstream = new URL(connection.host); |
There was a problem hiding this comment.
🔴 critical — ClickHouse 26.6+ PromQL alerts never send database/table, so the handler can't find the TimeSeries table
In the prometheus_api_v1 branch, set database/table from source.from on the query_range URL, the same way the proxy forwards them (CALLER_SETTABLE_PARAM_KEYS in routers/api/prometheus.ts notes the handler selects the table from them). As written, the handler evaluates against its default table or errors. Move the source/tableName checks above this branch so both ClickHouse paths use them.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| // ClickHouse 26.6+ with the prometheus_api_v1 handler configured. | ||
| const chUpstream = new URL(connection.host); | ||
| chUpstream.searchParams.set( | ||
| 'max_execution_time', |
There was a problem hiding this comment.
🟠 major — Alert path re-implements query limits and drops max_result_rows
Use clickhousePrometheusUpstream(connection.host) from controllers/timeseriesEngine.ts, which this PR exports for exactly this purpose. The hand-rolled URL here only pins max_execution_time, so an alert query has no result-row cap, unlike the proxy.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| } | ||
| }; | ||
|
|
||
| const isPromQL = |
There was a problem hiding this comment.
🟠 major — PromQL branch copies about 300 lines of the time-series state machine
The empty-bucket handling, per-group threshold evaluation, missing-group auto-resolve, notification loop and updateAlertState are copied from the standard path (index.ts:2026-2217). Normalise the PromQL results into the same per-bucket rows that loop consumes and share one evaluator. The PR description claims the evaluation loop is not duplicated, but it is, and future fixes will only land in one copy. The hand-written bucket loop also duplicates timeBucketByGranularity.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| > | ||
| Table | ||
| </Tabs.Tab> | ||
| {tableSource?.kind !== SourceKind.Promql && ( |
There was a problem hiding this comment.
🟠 major — Table tab is hidden for PromQL sources although PromQL table charts are supported
Remove the tableSource?.kind !== SourceKind.Promql guard on the Table tab. isPromqlDisplayType still includes Table and DBTableChart renders PromQL configs (DBTableChart.tsx:90,118). With the guard, existing PromQL table tiles open with no tab selected and users can't create new ones.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| ? additionalAlertWarnings.join(' ') | ||
| : undefined | ||
| } | ||
| tooltip="The threshold will be evaluated against the last value returned by the last PromQL expression" |
There was a problem hiding this comment.
🔵 minor — Tooltip describes evaluation the backend doesn't perform
Reword the tooltip or align the backend. The worker evaluates each window bucket via query_range at step = alert interval, and ignores the expression's queryType: 'instant', its reducer and the tile's step. A Number tile with an avg/max reducer therefore alerts on a different value than the one it displays, and not on 'the last value'.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| continue; | ||
| } | ||
|
|
||
| const bucketEvaluations = new Map< |
There was a problem hiding this comment.
🔵 minor — bucketEvaluations dedup can never trigger
seriesForBucket is already a Map keyed by groupKey, so bucketEvaluations.get(groupKey) is always empty on insert. The earlier 'keep the highest value' reduction is also wrong for BELOW thresholds, since it keeps the non-breaching value. Drop both and evaluate seriesForBucket directly.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| setValue('alert', undefined); | ||
| } | ||
| }, [configType, displayType, previousDisplayType, setValue]); | ||
| }, [configType, displayType, isPromqlInput, previousDisplayType, setValue]); |
There was a problem hiding this comment.
🔵 minor — isPromqlInput added to deps but unused; PromQL alert gating uses the builder predicate
In this effect, use displayTypeSupportsPromQLAlerts(displayType) when configType === 'promql', and drop the unused isPromqlInput dependency.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| expect(alertHistories[1].state).toBe('OK'); | ||
| }); | ||
|
|
||
| describe('PromQL Alerts', () => { |
There was a problem hiding this comment.
🔵 minor — PromQL integration test bypasses the provider and the real query paths
The test builds details by hand from a log source, so it misses that getTileDetails rejects this sourceless tile config in production. It also mocks queryRangeViaTableFunction, so the 26.6+ HTTP path is never exercised. Add a provider-level test with a PromQL tile, plus a unit test asserting the database/table params on the ClickHouse HTTP URL. Also rename the unit test 'should have name stripped', which asserts that __name__ is kept.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| // tenant-isolation scope -- and un-pin or override it, even though no | ||
| // legitimate caller ever sends that key. | ||
| // Only real Prometheus API params are ever caller-settable in proxyToPrometheus. | ||
| const CALLER_SETTABLE_PARAM_KEYS = new Set([ |
There was a problem hiding this comment.
🔵 minor — Security-relevant rationale comments deleted during the move
Restore the removed docs: why CALLER_SETTABLE_PARAM_KEYS exists (stopping callers from overriding a host-pinned tenant scope such as extra_label), the isClientDisconnect error-code rationale, and the #3046 context on joinPrometheusUpstreamUrl in timeseriesEngine.ts. Otherwise the next editor loses the reason for a security boundary.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| useEffect( | ||
| () => { | ||
| if (shouldBeImmediate) { | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
🔵 minor — Blanket eslint-disable-next-line added in code unrelated to this feature
Name the specific rule being suppressed, or drop these edits from this PR. A bare disable silences every rule on that line.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
…use http endpoint)
| ? isPromqlSavedChartConfig(details.tile.config) | ||
| : false; | ||
|
|
||
| if (isPromQL) { |
There was a problem hiding this comment.
🟠 major — PromQL branch copies ~300 lines of the time-series state machine
The empty-bucket handling, per-group ALERT/PENDING/OK transitions, missing-group auto-resolve, notification loop and history write are near-verbatim copies of the builder path at index.ts ~2043-2234. That leaves two state machines to keep in sync by hand, and fixes to one (e.g. the NULL-skip in parseAlertData) won't reach the other. Normalise the PromQL results into the same checkDataByBucket shape (bucketStart → rows with group key and value), then share one evaluation loop after it.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| > | ||
| Table | ||
| </Tabs.Tab> | ||
| {tableSource?.kind !== SourceKind.Promql && ( |
There was a problem hiding this comment.
🟠 major — Table tab is hidden for PromQL sources, although Table is a supported PromQL display type
isPromqlDisplayType still includes DisplayType.Table, and DBTableChart renders PromQL configs. Hiding the Table tab when tableSource.kind === Promql means a user can no longer pick Table for a PromQL chart, and an existing PromQL table tile opens with no matching tab. Keep the Table tab and hide only Search/Heatmap/Patterns.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| const sourceChanged = sourceId !== prevSourceIdRef.current; | ||
| prevSourceIdRef.current = sourceId; | ||
|
|
||
| if (sourceChanged && tableSource?.kind === SourceKind.Promql) { |
There was a problem hiding this comment.
🔵 minor — Auto-switch to PromQL on source change usually doesn't fire, because tableSource loads asynchronously
prevSourceIdRef is updated on the render where sourceId changes, but useSource hasn't resolved tableSource yet (it's undefined, or still the previous source). By the time the Promql source loads, sourceChanged is false, so configType/displayType are never switched. Key the switch on tableSource?.id changing, or compare tableSource.id === sourceId before acting.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
|
|
||
| // ClickHouse path — create a client for both the HTTP API probe and the | ||
| // table function fallback. | ||
| const client = new ClickhouseClient({ |
There was a problem hiding this comment.
🔵 minor — ClickhouseClient isn't closed on the HTTP-API path or when the probe throws
client.close() sits in a finally that only wraps the table-function fallback. When clickhouseServesPrometheusHttpApi returns true, the function returns early and the client is never closed, and that happens on every evaluation tick of every PromQL alert on a 26.6+ server. Wrap everything after the client is constructed in try { … } finally { await client.close(); }.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| url.searchParams.set('step', String(stepSec)); | ||
| const resp = await fetch(url.toString(), { | ||
| headers, | ||
| signal: AbortSignal.timeout(30_000), |
There was a problem hiding this comment.
🔵 minor — PromQL fetch timeouts are reported as QUERY_ERROR, and the 30s limit is hardcoded
AbortSignal.timeout rejects with a TimeoutError ("The operation was aborted due to timeout"). isTimeoutErrorShallow (checkAlerts/errors.ts:89) doesn't recognise it, so makeQueryAlertError records QUERY_ERROR instead of QUERY_TIMEOUT. Recognise e.name === 'TimeoutError' in errors.ts, and use the imported PROMETHEUS_CH_TIMEOUT_MS instead of the literal 30_000.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| metric?: Record<string, string>; | ||
| values?: [number, string][]; | ||
| }[]; | ||
| }; |
There was a problem hiding this comment.
🔵 minor — Backend dispatch (Prometheus / ClickHouse HTTP API / table function) is duplicated from the proxy router
evaluatePromqlAlert re-implements the three-way selection that routers/api/prometheus.ts query_range handler does at lines 385-444. The PR moved only the leaf helpers into timeseriesEngine, not the selection itself. Extract a single queryRange({connection, database, table, expr, start, end, step}) into controllers/timeseriesEngine.ts and call it from both places.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| continue; | ||
| } | ||
|
|
||
| const bucketEvaluations = new Map< |
There was a problem hiding this comment.
🔵 minor — bucketEvaluations dedupe can never trigger
seriesForBucket is already a Map keyed by groupKey, so iterating it into bucketEvaluations always finds existing undefined and the worst-case merge is dead code. The same goes for the 'keep the highest value' branch above it: step equals the window, so no two samples of one series land in the same bucket. Iterate seriesForBucket directly.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| }); | ||
|
|
||
| describe('PromQL Alerts', () => { | ||
| it('should process a PromQL alert, respecting grouping, state transitions, and backfilling', async () => { |
There was a problem hiding this comment.
🔵 minor — Int test claims to cover state transitions and backfilling but runs one tick over one bucket
The test calls processAlertAtTime once, and the mock returns one point per series. No ALERT→OK resolve, no multi-bucket backfill, no missing-group auto-resolve, and no numConsecutiveWindows path is exercised. It also builds details directly, bypassing the provider that loads PromQL tiles in production. Add a second tick where node-1 drops below the threshold or disappears (assert the resolve notification), plus a delayed-now case that returns several step points.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| await userEvent.click(screen.getByTestId('chart-save-alert-button')); | ||
|
|
||
| expect(await screen.findByText('Alert name is required')).toBeVisible(); | ||
| const elements = await screen.findAllByText('Alert name is required'); |
There was a problem hiding this comment.
🔵 minor — Test loosened to findAllByText to hide a duplicated 'Alert name is required' message
The builder inline-alert form now renders the validation error more than once, and the test was changed to accept that rather than find the cause. Find which component renders the second copy and restore the findByText single-match assertion.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| useEffect( | ||
| () => { | ||
| if (shouldBeImmediate) { | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
🔵 minor — Bare eslint-disable-next-line added in unrelated hooks
Two rule-less disables were added to useDebounce and useLocalStorage, which this PR doesn't otherwise touch. A rule-less disable suppresses every rule on that line. Name the specific rule (e.g. react-hooks/set-state-in-effect) or drop the change from this PR.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
…t and add capability memoization
| > | ||
| Table | ||
| </Tabs.Tab> | ||
| {tableSource?.kind !== SourceKind.Promql && ( |
There was a problem hiding this comment.
🟠 major — Hiding the Table tab for PromQL sources breaks PromQL table charts
Remove the tableSource?.kind !== SourceKind.Promql guard around the Table tab. isPromqlDisplayType still includes DisplayType.Table, and PromQL+table is a supported config (ChartEditor/__tests__/utils.test.ts:132). With the tab hidden, users can't create PromQL table charts, and existing PromQL table tiles open with no active tab selected. This change is also outside the scope of alerting.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| } | ||
| }; | ||
|
|
||
| const isPromQL = |
There was a problem hiding this comment.
🟠 major — PromQL path copies about 250 lines of the bucket, threshold, auto-resolve and notification state machine
The PromQL branch re-implements the builder time-series loop almost line for line (empty-bucket handling, consecutive-window firing, auto-resolve, notification dispatch, updateAlertState). Instead, convert PrometheusMatrixResult[] into the per-bucket {groupKey, value, attributes} rows the existing loop consumes and reuse that loop, so future fixes to alert state handling don't have to be made in two places.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
|
|
||
| // Resolve the expression to evaluate. The config stores either a bare string | ||
| // (tiles saved before multi-expression support) or an array of PromqlSeries. | ||
| // Alerts always target the LAST expression, mirroring how SQL builder alerts |
There was a problem hiding this comment.
🔵 minor — Expression selection re-implements getQueriedPromqlSeries and disagrees with the notification template
Replace the hand-rolled blank filter and first/last display-type switch with getQueriedPromqlSeries(resolvedConfig).at(-1)?.expression from common-utils/src/core/promql.ts. That helper already trims blank rows and keeps only the first series for non-time-series displays. Also make describeChartConfigQuery in checkAlerts/template.ts:146 use the same helper. It currently reports the unfiltered getPromqlSeries(...).at(-1), which can name a different expression than the one that was evaluated.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| continue; | ||
| } | ||
|
|
||
| const bucketEvaluations = new Map< |
There was a problem hiding this comment.
🔵 minor — The bucketEvaluations worst-case merge can never run in the PromQL path
seriesForBucket is already a Map keyed by groupKey, so bucketEvaluations.get(groupKey) is always undefined and the !existing.exceeds && exceeds merge never triggers. Iterate seriesForBucket directly and compute exceeds inline.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| ? additionalAlertWarnings.join(' ') | ||
| : undefined | ||
| } | ||
| tooltip="The threshold will be evaluated against the last value returned by the last PromQL expression" |
There was a problem hiding this comment.
🔵 minor — Alert tooltip describes evaluation the backend doesn't do
Change the tooltip to say the threshold is checked on each alert window's value, using the last expression for time-series charts and the first for Number charts. evaluatePromqlAlert uses validExpressions[0] for Number displays and evaluates every window bucket; it does not apply the chart's reducer to the 'last value'.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| setValue('alert', undefined); | ||
| } | ||
| }, [configType, displayType, previousDisplayType, setValue]); | ||
| }, [configType, displayType, isPromqlInput, previousDisplayType, setValue]); |
There was a problem hiding this comment.
🔵 minor — isPromqlInput was added to the effect deps, but PromQL charts are still checked with the builder alert rule
Inside the effect, branch on PromQL and call displayTypeSupportsPromQLAlerts(displayType), or drop the unused dependency. Today PromQL charts go through displayTypeSupportsBuilderAlerts, which only works because the two display-type sets happen to match.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| useEffect( | ||
| () => { | ||
| if (shouldBeImmediate) { | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
🔵 minor — Blanket eslint-disable comments added to unrelated hooks
Remove the two rule-less // eslint-disable-next-line lines in useDebounce and useLocalStorage. If a suppression really is needed, name the specific rule. As written they silence every rule on those lines and have nothing to do with this feature.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| details.source, | ||
| metadata, | ||
| ); | ||
| if (withClauses) { |
There was a problem hiding this comment.
🟠 major — Saved-search notifications no longer get alias WITH clauses because aliasWithClauses is never assigned
Moving this block dropped the old aliasWithClauses = withClauses; line. The let aliasWithClauses at line 1310 stays undefined, so fetchSampleLines({ aliasWith: aliasWithClauses }) (line 1347) runs without the saved search's aliases, and the sample-row query fails or comes back empty for WHERE clauses that use aliases. Put the aliasWithClauses = withClauses; assignment back inside the try block.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| > | ||
| Table | ||
| </Tabs.Tab> | ||
| {tableSource?.kind !== SourceKind.Promql && ( |
There was a problem hiding this comment.
🟠 major — Table tab is hidden for PromQL sources, although PromQL tables are supported
isPromqlDisplayType still includes DisplayType.Table, and DBTableChart renders PromQL configs (isPromqlChartConfig, DBTableChart.tsx:90). Hiding the tab removes the only way to create a PromQL table, and existing PromQL table tiles open with no matching active tab. Keep the Table tab and hide only Search, Heatmap and Patterns.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| } | ||
| }; | ||
|
|
||
| const isPromQL = |
There was a problem hiding this comment.
🟠 major — PromQL path copies the whole bucket/threshold/resolve state machine instead of reusing it
Lines 1589–1813 are a near line-for-line copy of the builder evaluation loop at 2026 onward: empty buckets, the worst-case map per bucket, auto-resolve and the notification loop. Every future fix to alert state will need to be made twice, and the PR description itself says the loop is not duplicated. Convert promqlResults into rows keyed by bucket (a timestamp plus group fields) and send them through the existing loop.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| ); | ||
| if (validExpressions.length > 0) { | ||
| if ( | ||
| resolvedConfig.displayType === 'number' || |
There was a problem hiding this comment.
🔵 minor — Expression selection re-implements getQueriedPromqlSeries and disagrees with the template and the editor tooltip
Use getQueriedPromqlSeries(resolvedConfig).at(-1) from common-utils/src/core/promql.ts:34 here. As written, non-time-series types take the first expression, while template.ts:146 shows getPromqlSeries(config).at(-1) in notifications. The tooltip at PromqlChartEditor.tsx:222 also says the last value of the last expression is used, but every window is actually evaluated. Keep one rule and use it in all three places.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| url.searchParams.set('step', String(stepSec)); | ||
| const resp = await fetch(url.toString(), { | ||
| headers, | ||
| signal: AbortSignal.timeout(30_000), |
There was a problem hiding this comment.
🔵 minor — PromQL fetch timeouts are recorded as QUERY_ERROR, not QUERY_TIMEOUT
AbortSignal.timeout(30_000) rejects with a TimeoutError whose message matches none of the checks in isQueryTimeoutError (tasks/checkAlerts/errors.ts:85–135). Passing PROMETHEUS_CH_TIMEOUT_MS to makeQueryAlertError therefore has no effect. Detect e.name === 'TimeoutError' (or wrap the error) so it is classified as a timeout, and use PROMETHEUS_CH_TIMEOUT_MS instead of the literal 30_000.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| if (!bucketSeriesValues.has(nearestBucketMs)) { | ||
| bucketSeriesValues.set(nearestBucketMs, new Map()); | ||
| } | ||
| // Keep the highest value if the same series appears twice in a bucket |
There was a problem hiding this comment.
🔵 minor — The same-series dedupe cannot trigger, and if it did it would keep the wrong value for BELOW thresholds
Prometheus returns one point per step for each unique label set, so two values never map to the same bucket and group. If they did, keeping the highest value would hide a breach of a BELOW threshold. Remove this dedupe and the following bucketEvaluations pass, or pick the value using doesExceedThreshold.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| try { | ||
| promqlResults = await evaluatePromqlAlert({ | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion | ||
| savedConfig: savedConfig as PromqlSavedChartConfig, |
There was a problem hiding this comment.
🔵 minor — savedConfig as PromqlSavedChartConfig cast, and an undefined branch that can never run
isPromQL already narrows the config, so the ternary's undefined arm is never reached. Narrow once, e.g. const savedConfig = details.taskType === INLINE ? details.chartConfig : details.taskType === TILE ? details.tile.config : undefined; if (savedConfig && isPromqlSavedChartConfig(savedConfig)). This removes the cast, which the repository conventions say to avoid.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| expect(alertHistories[1].state).toBe('OK'); | ||
| }); | ||
|
|
||
| describe('PromQL Alerts', () => { |
There was a problem hiding this comment.
🔵 minor — PromQL integration tests bypass the provider, where PromQL loading breaks
Both tests build tiles without source and pass source directly to createAlertDetails, so the real getTileAlertDetails / getInlineAlertDetails path is never exercised. The second test checks only the fetch URL, not the resulting alert state. Add a test that loads a PromQL tile alert through DefaultAlertProvider.getAlertTasks, and assert the resulting history state in the HTTP API test.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| ); | ||
| }); | ||
|
|
||
| it('should have __name__ stripped from metric map', async () => { |
There was a problem hiding this comment.
🔵 minor — Test is named 'should have name stripped' but asserts __name__ is kept, and its comment contradicts processAlert
Rename the test to describe what it checks. Fix the comment that says processAlert strips __name__ from the group key: index.ts deliberately keeps it. Also remove the getConnectionById mocks here and in the Prometheus test, since evaluatePromqlAlert never calls it.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| useEffect( | ||
| () => { | ||
| if (shouldBeImmediate) { | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
🔵 minor — Blanket // eslint-disable-next-line added to unrelated hooks
These bare directives silence every rule on those lines, in code this feature does not touch. Remove them, or name the specific rule being suppressed.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const tsMs = Math.round(tsSec * 1000) - windowMs; | ||
|
|
||
| if (expectedBuckets.length === 0) continue; | ||
| const startMs = expectedBuckets[0].getTime(); |
There was a problem hiding this comment.
🟠 major — PromQL branch copies ~250 lines of the time-series alert state machine
Empty-bucket handling, per-group ALERT/PENDING/OK transitions, missing-group auto-resolve, notification dispatch and history persistence are all copied from the time-series path below (lines ~2050-2242). The two copies have already drifted (e.g. shouldFireBasedOnConsecutiveWindows('') vs (), a hand-rolled bucket loop vs timeBucketByGranularity). Fix: turn the PromQL matrix into the same per-bucket {groupKey, value, attributes} rows and run the existing evaluator on them. Move that evaluator into a helper so a future fix only has to be made once.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| dateRange, | ||
| windowSizeInMins, | ||
| ); | ||
|
|
There was a problem hiding this comment.
🔵 minor — Per-bucket dedup and isPromQL || are dead code
seriesForBucket is already a Map keyed by groupKey, so the bucketEvaluations loop's existing is always undefined. It just copies the map; iterate seriesForBucket directly. Likewise hasGroupBy = isPromQL || alertHasGroupBy(details) is redundant: this PR makes alertConfigHasGroupBy return true for PromQL configs (utils/alerts.ts:82). Drop isPromQL || and the previousMap && null check too, since previousMap is always set.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| setValue('alert', undefined); | ||
| } | ||
| }, [configType, displayType, previousDisplayType, setValue]); | ||
| }, [configType, displayType, isPromqlInput, previousDisplayType, setValue]); |
There was a problem hiding this comment.
🔵 minor — isPromqlInput added to deps but PromQL still uses the builder alert check
The effect body never reads isPromqlInput. PromQL configs still go through displayTypeSupportsBuilderAlerts, so the change does nothing. Branch on configType === 'promql' to use displayTypeSupportsPromQLAlerts, or revert the dependency change.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| await userEvent.click(screen.getByTestId('chart-save-alert-button')); | ||
|
|
||
| expect(await screen.findByText('Alert name is required')).toBeVisible(); | ||
| const elements = await screen.findAllByText('Alert name is required'); |
There was a problem hiding this comment.
🔵 minor — Test loosened to accept duplicate 'Alert name is required' messages
findByText became findAllByText(...)[0], which hides the fact that this builder-mode form now shows the validation error more than once. Find out why the error renders twice and fix it, then restore the single-match assertion.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| // same aliases against the same source. | ||
| // Populated below for SAVED_SEARCH alerts; referenced by the shared | ||
| // sampleLinesFor closure that may be called after the non-PromQL query path. | ||
| let aliasWithClauses: BuilderChartConfigWithOptDateRange['with']; |
There was a problem hiding this comment.
🟠 major — aliasWithClauses is never assigned any more, so saved-search sample lines lose their alias WITH clauses
When the SAVED_SEARCH block moved below the PromQL branch (~line 1881), the aliasWithClauses = withClauses; line was dropped. sampleLinesFor → fetchSampleLines({ aliasWith: aliasWithClauses }) now always gets undefined. If a saved search's WHERE references a select alias (e.g. toString(Body) AS body), the sample-row query fails and the notification goes out with no sample lines. Fix: put the assignment back inside the computeAliasWithClauses try block.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| h.lastValues.push({ count: 0, startTime: lastExpectedBucket }); | ||
| h.state = AlertState.OK; | ||
| h.counts = 0; | ||
| latestAlertContext.delete(groupKey); |
There was a problem hiding this comment.
🟠 major — New missing-group resolution silently drops a breach that happened earlier in the same backfilled run
Take a group whose previous state was OK. It breaches in bucket N, which sets latestAlertContext, and then has no rows in the last bucket. wasAlertingOrPending is true (from history.state), so latestAlertContext.delete(groupKey) runs. The ALERT notification is suppressed, and because groupPrevious is still OK, no resolve is sent either. The same data with a below-threshold value in the last bucket sends both notifications. Fix: keep the context (only set state OK and push the 0 value) so the breach-then-resolve pair still goes out. The copy at line 1790 needs the same fix.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| > | ||
| Table | ||
| </Tabs.Tab> | ||
| {tableSource?.kind !== SourceKind.Promql && ( |
There was a problem hiding this comment.
🟠 major — Table tab is hidden for PromQL sources, removing a supported PromQL display type
isPromqlDisplayType still includes Table and DBTableChart renders PromQL configs, but the tab is now hidden whenever the source kind is Promql. Users can no longer create PromQL table tiles, and editing an existing one shows no active tab. Fix: drop the conditional around the Table tab. Only Search, Heatmap and Patterns need hiding.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| } | ||
| } | ||
|
|
||
| // Auto-resolve: groups that were alerting/pending but absent from the newest bucket. |
There was a problem hiding this comment.
🔵 minor — PromQL branch copies the query-error handling, missing-group resolution and notification loop of the SQL path
About 150 lines in the PromQL branch (the error/metrics block, the auto-resolve loop at 1745 vs 2197, the notification loop) duplicate the generic path verbatim, so a fix to one (like the context-drop bug above) has to be made twice. Fix: extract resolveMissingGroups/sendTransitionNotifications/recordQueryFailure helpers and call them from both paths. The dashboard-variables mapping at 1531 also duplicates getChartConfigFromAlert:829.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| @@ -1550,9 +1550,11 @@ export const isFormulaSourceKind = ( | |||
| export function displayTypeSupportsPromQLAlerts( | |||
There was a problem hiding this comment.
🔵 minor — displayTypeSupportsPromQLAlerts restates displayTypeSupportsBuilderAlerts line for line
The body is identical to displayTypeSupportsBuilderAlerts (same file, line 1508). Delegate to it, or document why the two must be able to diverge. As it is, EditTimeChartForm's clear-alert effect uses the builder predicate for PromQL and only works because the two match.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| setValue('alert', undefined); | ||
| } | ||
| }, [configType, displayType, previousDisplayType, setValue]); | ||
| }, [configType, displayType, isPromqlInput, previousDisplayType, setValue]); |
There was a problem hiding this comment.
🔵 minor — isPromqlInput was added to the effect's deps but the effect never reads it
The effect still picks between the raw SQL and builder predicates. Either use displayTypeSupportsPromQLAlerts when configType === 'promql', or remove the unused dependency.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
|
|
||
| let servesHttpApi = false; | ||
| if (clickhouseCapabilityMemo) { | ||
| if (!clickhouseCapabilityMemo.has(connection.id)) { |
There was a problem hiding this comment.
🔵 minor — Capability memo is check-then-set around an await, so concurrent alerts on one connection all probe
processAlertTask queues every alert on a connection into task_queue concurrently. Each one sees has() as false before the first probe resolves, so all of them run the version query plus the HTTP probe, which the memo was meant to avoid. Fix: store the Promise<boolean> in the memo and await it.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| url.searchParams.set('step', String(stepSec)); | ||
| const resp = await fetch(url.toString(), { | ||
| headers, | ||
| signal: AbortSignal.timeout(30_000), |
There was a problem hiding this comment.
🔵 minor — Hardcoded 30s timeout duplicates PROMETHEUS_CH_TIMEOUT_MS, and the maxExecutionSec override restates the default
Use AbortSignal.timeout(PROMETHEUS_CH_TIMEOUT_MS). maxExecutionSec: Math.round(PROMETHEUS_CH_TIMEOUT_MS / 1000) at line 1113 equals the default PROMETHEUS_MAX_EXECUTION_SEC (30), so drop the argument. The variables.length > 0 guard at 1003 is also unneeded, since substitutePromqlChartConfigVariables already handles null and empty lists.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| @@ -5163,6 +5164,136 @@ describe('checkAlerts', () => { | |||
| expect(alertHistories[1].state).toBe('OK'); | |||
| }); | |||
There was a problem hiding this comment.
🔵 minor — PromQL integration tests never exercise backfill or disappearing-series resolution
The 'respecting grouping, state transitions, and backfilling' test only evaluates one bucket (it asserts backfilledBuckets is 0), and no test covers a previously alerting PromQL series that vanishes. The routing test only checks the fetch URL, not the resulting state. Add a multi-bucket run and a two-run disappearing-series case, and assert on histories and notifications.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| useEffect( | ||
| () => { | ||
| if (shouldBeImmediate) { | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
🔵 minor — Blanket // eslint-disable-next-line with no rule name, in code this PR doesn't otherwise touch
This disables every rule on that line (same again at line 205). Name the specific rule (e.g. react-hooks/set-state-in-effect, as EditTimeChartForm.tsx:765 does) or drop the unrelated change.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| metadata, | ||
| ); | ||
| if (withClauses) { | ||
| chartConfig.with = withClauses; |
There was a problem hiding this comment.
🟠 major — Saved-search notifications lost alias WITH clauses: aliasWithClauses is never assigned now
The refactor dropped aliasWithClauses = withClauses;. The variable is declared at line 1311 and read by sampleLinesFor → fetchSampleLines({ aliasWith }), but it is now always undefined. A saved-search alert whose WHERE uses a select alias (e.g. toString(Body) AS body) gets a failed or empty sample-rows query in its notification body. Put the assignment back next to chartConfig.with = withClauses.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| h.lastValues.push({ count: 0, startTime: lastExpectedBucket }); | ||
| h.state = AlertState.OK; | ||
| h.counts = 0; | ||
| latestAlertContext.delete(groupKey); |
There was a problem hiding this comment.
🟠 major — New disappearing-group resolve swallows the ALERT notification for a breach inside the same run
The rewritten auto-resolve now also resolves groups present in histories whose last bucket is missing, and it calls latestAlertContext.delete(groupKey). Take the changed int test (~line 8725): service-b breaches at 22:05, then disappears at 22:10. It ends OK, but no ALERT is sent because its context was deleted, and no resolve is sent because the previous state was OK. The breach is never notified. A group that breaches and then reports an OK value keeps its context and gets both ALERT and resolve. Don't delete the context here, so the notification loop sends ALERT and then resolve. The same block is copied at line 1790 in the PromQL branch.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| // target the last series and how the tile renders its primary value. | ||
| if (validExpressions.length > 0) { | ||
| promqlExpression = | ||
| validExpressions[validExpressions.length - 1].expression; |
There was a problem hiding this comment.
🟠 major — Number tiles: the alert evaluates a different PromQL expression than the tile shows
evaluatePromqlAlert always picks the last non-blank expression. For non-time-series display types, getQueriedPromqlSeries (common-utils/src/core/promql.ts) queries only the first expression. A Number tile with two stored expressions displays #1 while the alert fires on #2. Resolve the expression with getQueriedPromqlSeries(resolvedConfig).at(-1)?.expression instead of hand-rolling the string/array normalization that getPromqlSeries already does. Also update template.ts:142-146, which still says PromQL can't be alerted on and quotes getPromqlSeries(config).at(-1) without skipping blank expressions.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| > | ||
| Table | ||
| </Tabs.Tab> | ||
| {tableSource?.kind !== SourceKind.Promql && ( |
There was a problem hiding this comment.
🟠 major — Table tab hidden for PromQL sources removes the existing PromQL table display
isPromqlDisplayType still accepts DisplayType.Table, and PromQL+table configs are supported (see ChartEditor/tests/utils.test.ts: 'persists alternateRowBackground for a promql+table config'). With a Promql source selected, the Table tab no longer renders. Users can't create PromQL tables, and editing an existing one leaves the tab bar with no active tab. Only gate the Search/Heatmap/Patterns tabs.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| } | ||
| }; | ||
|
|
||
| if (isPromQL) { |
There was a problem hiding this comment.
🟠 major — PromQL branch copies about 250 lines of the time-series state machine, and the copies have already diverged
Empty-bucket handling, per-bucket evaluation, auto-resolve (1745-1792 vs 2197-2248), the notification loop and updateAlertState are all duplicated. Already, the per-bucket dedup condition differs from line 2156, and the 'keep the highest value' rule hides breaches for BELOW thresholds. Convert PrometheusMatrixResult[] into the same bucket→group map checkDataByBucket produces and reuse the shared evaluation. Then bucketEvaluations at 1696 goes away: seriesForBucket is already keyed by group, so existing is always undefined.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| setValue('alert', undefined); | ||
| } | ||
| }, [configType, displayType, previousDisplayType, setValue]); | ||
| }, [configType, displayType, isPromqlInput, previousDisplayType, setValue]); |
There was a problem hiding this comment.
🔵 minor — isPromqlInput added to the dependency list but never read by the effect
The effect still picks displayTypeSupportsRawSqlAlerts or displayTypeSupportsBuilderAlerts from configType alone. The new dependency changes nothing and suggests PromQL is handled when it isn't. Either use displayTypeSupportsPromQLAlerts when configType === 'promql', or drop the dependency.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| }); | ||
|
|
||
| describe('PromQL Alerts', () => { | ||
| it('should process a PromQL alert, respecting grouping, state transitions, and backfilling', async () => { |
There was a problem hiding this comment.
🔵 minor — PromQL int test only reaches the mocked table function because of leaked global.fetch state
CI runs ClickHouse 26.8 with the prometheus_api_v1 handler (docker-compose.ci.yml). This test doesn't stub clickhouseServesPrometheusHttpApi, so the real probe runs. It returns false only because earlier tests (lines 4120-4990) replace global.fetch and never restore it. Run it alone (-t PromQL) and it takes the HTTP path, so queryRangeViaTableFunction is never called. Stub the probe to false explicitly, like the routing test at 11576 does, and restore spies afterwards.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| ); | ||
| }); | ||
|
|
||
| it('should have __name__ stripped from metric map', async () => { |
There was a problem hiding this comment.
🔵 minor — Test name says __name__ is stripped, but it asserts __name__ is kept; getConnectionById mocks are dead
The assertion expects metric: { __name__: 'up', ... }, and processAlert keeps __name__ in the group key (int test expects __name__:up, host:node-1). The comment 'the caller strips name when building the group key' is also false. Rename the test and fix the comment. evaluatePromqlAlert never calls getConnectionById, so remove those beforeEach mocks from both new test files.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| await userEvent.click(screen.getByTestId('chart-save-alert-button')); | ||
|
|
||
| expect(await screen.findByText('Alert name is required')).toBeVisible(); | ||
| const elements = await screen.findAllByText('Alert name is required'); |
There was a problem hiding this comment.
🔵 minor — Test changed to tolerate the validation error rendering more than once
Switching findByText to findAllByText(...)[0] lets a regression that shows 'Alert name is required' twice pass unnoticed, and nothing explains why a second copy now appears for a builder Number config. Find the duplicate render and fix it, or assert the exact expected count.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| // e.g. VictoriaMetrics's `extra_label`, which a Connection host may pin as a | ||
| // tenant-isolation scope -- and un-pin or override it, even though no | ||
| // legitimate caller ever sends that key. | ||
| // Only real Prometheus API params are ever caller-settable in proxyToPrometheus. |
There was a problem hiding this comment.
🔵 minor — Moving the helpers deleted security and edge-case rationale from their doc comments
The move removed the explanations for CALLER_SETTABLE_PARAM_KEYS (stops a caller overriding a host-pinned VictoriaMetrics extra_label tenant scope), for the opaque-scheme guard and #3046 link in joinPrometheusUpstreamUrl, for pinning limits on the host in clickhousePrometheusUpstream, and for isClientDisconnect. Move the doc comments along with the code. Also, the re-export block exists only so tests can keep importing from the router; point those imports at @/controllers/timeseriesEngine and drop it.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
|
Hey @pulpdrew, thanks for the thorough review! Here’s a quick summary of the updates:
Let me know if everything looks good or if you’d like me to make any other changes. |
This branch has not been deployed
2 participants
Feature: End-to-End PromQL Dashboards & Alerting
Summary
This PR introduces comprehensive, end-to-end support for PromQL within HyperDX. Users can now natively write PromQL queries in the Chart Editor, save them to Dashboards, attach threshold alerts to them, and rely on the backend alerting engine to evaluate them and dispatch notifications.
This represents a major architectural extension to the time-series engine, seamlessly unifying PromQL with existing Builder and Raw SQL systems without duplicating the evaluation loop.
Description
1. Dashboards & Chart Editor (
packages/app)PromqlChartEditor.tsxto allow users to construct PromQL queries directly within the UI.DBDashboardPage.tsxto recognize and renderPromqlSavedChartConfigtiles. By dynamically injecting the active ClickHousesourceandconnection, PromQL charts now live side-by-side with Builder charts on Dashboards.TileAlertEditor(the bell icon) directly into the PromQL editor, empowering users to attach threshold alerts to their PromQL expressions effortlessly.2. Unified Alert Evaluation Engine (
packages/api)processAlert: Instead of creating a fragmented, secondary alerting loop for PromQL, the PromQL evaluation logic has been integrated into the core time-series loop insidepackages/api/src/tasks/checkAlerts/index.ts.INLINEalerts (carrying theirchartConfig). The backend evaluates the PromQL expression against the ClickHouse cluster and parses the response array (tagsandtime_series).3. Alert Details Page & Chart Previews (
packages/app)buildAlertChartConfiginAlertDetailChart.tsxbypassed PromQL charts, causing the Alert Details page to render an empty "This alert's chart can't be previewed here" state.promqlExpressioninto thePromqlConfigWithDateRangetype. The Alert Details page now seamlessly renders the interactive metric chart, the threshold reference lines, and the alert evaluation history for PromQL alerts.Tests
( Lint, Unit And Integration Test Tested Locally )
A rigorous integration testing suite was built to ensure the reliability of the new PromQL alerting pipeline:
checkAlerts.int.test.ts):{ tags: [string, string][], time_series: [string, number][] }[]).make dev-intsuite runs flawlessly with 177/177 Alert tests passing across the MongoDB and ClickHouse Docker containers.evaluatePromqlAlert.test.ts):UI Impact & Walkthrough ( From Dashboard To Alerts and Alert Details Page )
Screen.Recording.2026-09-14.at.12.55.29.AM.mov
Fixes #3113