Skip to content

feat: Expand variables as empty in alert queries - #2910

Open
pulpdrew wants to merge 2 commits into
drew/builder-variablesfrom
drew/variable-alerting
Open

feat: Expand variables as empty in alert queries#2910
pulpdrew wants to merge 2 commits into
drew/builder-variablesfrom
drew/variable-alerting

Conversation

@pulpdrew

Copy link
Copy Markdown
Contributor

Summary

This PR extends the dashboard variable functionality to support substitution in alerts.

  1. During alert execution, variables are expanded to empty states
  2. In the chart editor, a warning is shown when configuring an alert on a query that uses variables, indicating the empty state will be used.
  3. The alert preview now substitutes empty variable state

Screenshots or video

Empty variable state in alert previews

Screenshot 2026-08-14 at 7 36 30 AM Screenshot 2026-08-14 at 7 36 19 AM

Empty states substituted in execution

Screenshot 2026-08-14 at 7 39 54 AM

Warning

Screenshot 2026-08-14 at 7 35 57 AM

How to test locally

  1. Clone and restart to ensure NEXT_PUBLIC_ENABLE_DASHBOARD_VARIABLES=true
  2. Create a dashboard
  3. Add a filter and enable variable mode
  4. Add some tiles that reference variables or variable macros and add alerts to them

References

  • Linear Issue: Closes HDX-5054
  • Related PRs:

@changeset-bot

changeset-bot Bot commented Aug 14, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: a3250c4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel

vercel Bot commented Aug 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
hyperdx-oss Ignored Ignored Preview Aug 14, 2026 12:49pm
hyperdx-storybook Ignored Ignored Preview Aug 14, 2026 12:49pm

Request Review

@pulpdrew
pulpdrew changed the base branch from main to drew/builder-variables August 14, 2026 11:41
@pulpdrew pulpdrew changed the title Drew/variable alerting feat: Expand variables as empty in alert queries Aug 14, 2026
@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes dashboard-tile alerts evaluate variables using empty selections and aligns alert previews and editor messaging with that behavior.

  • Centralizes extraction of dashboard variable declarations.
  • Supplies empty variable values to backend alert execution and frontend alert previews.
  • Adds variable-aware warnings to builder and raw-SQL alert editors.
  • Adds unit, integration, and end-to-end coverage for substitution and warning behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/api/src/tasks/checkAlerts/index.ts Adds empty dashboard-variable declarations to raw-SQL and builder tile alert configurations.
packages/app/src/components/alerts/AlertDetailChart.tsx Aligns tile-alert previews with scheduled execution by supplying empty variable selections.
packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx Computes variable-reference warnings from the debounced editor configuration while an alert is configured.
packages/common-utils/src/filters.ts Centralizes ordered, deduplicated extraction of variable declarations from dashboard filters.
packages/common-utils/src/variables.ts Adds shared warning text identifying variables referenced by an alerting tile.
scripts/ci/ratchet-baseline.json Updates suppression-count baselines to match the repository state.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  DashboardFilters[Dashboard variable declarations] --> EmptyVariables[Attach empty selections]
  EmptyVariables --> AlertTask[Scheduled alert execution]
  EmptyVariables --> AlertPreview[Alert detail preview]
  EmptyVariables --> EditorPreview[Chart editor preview]
  AlertTask --> QueryRenderer[Shared variable substitution]
  AlertPreview --> QueryRenderer
  EditorPreview --> QueryRenderer
  QueryRenderer --> ClickHouse[(ClickHouse)]
  EditorPreview --> Warning[Empty-state warning]
Loading

Reviews (3): Last reviewed commit: "chore: Update rachet" | Re-trigger Greptile

@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 296 passed • 1 skipped • 1106s

Status Count
✅ Passed 296
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew
pulpdrew marked this pull request as ready for review August 14, 2026 13:41
@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Aug 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

Additional context: touches background tasks or the delivery pipeline lightly (11 lines, under the 30-line bar for Tier 4)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 9
  • Production lines changed: 147 (+ 647 in test files, excluded from tier calculation)
  • Branch: drew/variable-alerting
  • Author: pulpdrew

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

Scope: 15 files vs base 2e1a153c — dashboard-variable support extended to alerts (empty-state expansion at execution, editor warning, preview substitution). Mode: report-only.

No critical (P0/P1) issues found. The alert-execution path is well-contained: getChartConfigFromAlert runs inside processAlert's per-alert try/catch, empty guarded macros ($__filter/$__conditionalAll) and Lucene refs render to safe match-all no-ops, and query failures are recorded as ERROR history rows and retried rather than crashing the task. The useDashboardFilters refactor is behaviorally equivalent to the old inline loop, and variables survives both the raw-SQL pick() and the materialized-view optimizer clone. One reliability risk and a set of advisory items are worth addressing before merge.

🟡 P2 — recommended

  • packages/api/src/tasks/checkAlerts/index.ts:629 — Empty-state substitution renders an unguarded ServiceName = $var (sqlstring) predicate to ServiceName = NULL and IN (${var:csv}) to IN (), so an ABOVE-threshold alert on such a tile matches zero rows and silently never fires (or errors permanently for csv) with nothing recorded in executionErrors; pre-PR this reference was left literal and errored loudly, so the change turns a diagnosable failure into a silent one, and the only guardrail (getAlertVariableWarning) is editor-only and identical for match-all vs match-none forms.
    • Fix: At execution, detect unguarded sqlstring/csv references and surface a QUERY_ERROR/INVALID_ALERT execution error instead of rendering NULL/IN (), and make getAlertVariableWarning distinguish match-none/invalid reference forms from the safe macro/Lucene no-op forms.
    • adversarial, correctness
🔵 P3 nitpicks (8)
  • packages/api/src/tasks/checkAlerts/index.ts:629 — A tile alert whose $__filter(expr, svc) names a variable later removed/renamed/disabled on the dashboard throws MacroExpansionError at render, recorded as QUERY_ERROR, so the alert quietly stops evaluating with no linkage from the dashboard edit that broke it.
    • Fix: Treat a macro reference to an undeclared variable as an empty-selection no-op, or emit a dedicated variable no longer exists execution error, and validate tile alerts when a dashboard filter is removed.
    • adversarial
  • packages/api/src/tasks/checkAlerts/index.ts:629 — Because empty guarded selections expand to match-all (1=1), a variable-scoped tile evaluates across all series at alert time, so a BELOW/absence-style alert can no longer detect a single selection's outage; the generic warning does not convey this per-threshold consequence.
    • Fix: Steer BELOW/absence alerts away from variable-scoped tiles or warn specifically for thresholdType BELOW.
    • adversarial
  • packages/api/src/tasks/checkAlerts/index.ts:630 — Dropping the variable predicate broadens the alert query's scan to the full unfiltered dataset, raising ClickHouse load and QUERY_TIMEOUT risk on high-cardinality tables.
    • Fix: Bound the scan (rely on the alert window) and monitor query duration for variable-scoped tile alerts; document the intended broadening.
    • reliability
  • packages/app/src/components/alerts/AlertDetailChart.tsx:142 — The new empty-variable injection into all three config-assembly branches (raw-SQL no-source, raw-SQL with source, builder) has no unit or component test, so dropping variables from one branch would go uncaught.
    • Fix: Add a TileAlertChart test asserting the assembled config carries variables with values: [] across the branches.
    • testing
  • packages/api/src/tasks/checkAlerts/index.ts:629 and packages/app/src/components/alerts/AlertDetailChart.tsx:142 — The getDashboardVariableDeclarations(...).map(d => ({ ...d, values: [] })) empty-state construction is duplicated between the evaluator and the preview, which must stay behaviorally identical.
    • Fix: Extract a shared getEmptyDashboardVariables(filters) helper in common-utils and call it from both sites.
    • maintainability
  • packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx:396 — The additionalWarnings?: string[] prop is named for its shape rather than its intent (only ever alert-variable warnings) and each consumer re-implements the length ? join(' ') : undefined collapse.
    • Fix: Rename to alertWarnings and centralize the join formatting.
    • maintainability
  • packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx:400 — A second 300ms useDebouncedValue(watchedForm) pipeline runs parallel to RawSqlChartEditor's existing debouncedRawSqlConfig, so the two can transiently disagree and must be kept in sync.
    • Fix: Derive the variable warning from the existing debounced config in RawSqlChartEditor instead of adding a whole-form debounce.
    • kieran-typescript
  • packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx:399useWatch({ control }) as ChartEditorFormState asserts completeness RHF's DeepPartial type cannot guarantee; the downstream config ? ... : undefined guard makes it safe but the assertion disables checking for the whole form object.
    • Fix: Optionally pass the partial type through convertFormStateToSavedChartConfig and let it narrow, rather than asserting; the guarded escape hatch is otherwise acceptable.
    • kieran-typescript

Reviewers (6): correctness, adversarial, reliability, testing, maintainability, kieran-typescript.

Testing gaps:

  • No execution-level test for an unguarded raw-SQL/builder col = $var (→ NULL) or ${var:csv} (→ IN ()) at alert time — only the safe $__filter/$__conditionalAll/Lucene match-all forms are covered, so the silent-false-negative case is unverified.
  • No test for a tile alert whose macro references a variable the dashboard no longer declares (the requireVariable throw path).
  • AlertDetailChart variable injection and the MV-optimize-then-substitute ordering for builder alerts have no coverage.

@pulpdrew
pulpdrew requested review from a team, bot-hyperdx and jordan-simonovski and removed request for a team and bot-hyperdx August 14, 2026 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant