Skip to content

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

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

feat: Expand variables as empty in alert queries#2910
pulpdrew wants to merge 2 commits into
mainfrom
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: 988024b

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 18, 2026 1:33pm
hyperdx-storybook Ignored Ignored Preview Aug 18, 2026 1:33pm

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 variable references using empty selections and aligns alert previews and editor messaging with that behavior.

  • Centralizes extraction of dashboard variable declarations.
  • Supplies empty variable values to scheduled alert queries and alert-detail previews.
  • Adds variable-reference warnings to builder and raw-SQL alert editors.
  • Adds unit, integration, and end-to-end coverage for alert variable expansion.

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 reconstructed raw-SQL and builder alert chart configurations.
packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx Computes variable-reference warnings for alert-enabled chart forms and forwards them to both editor variants.
packages/app/src/components/alerts/AlertDetailChart.tsx Configures tile alert-detail previews with the same empty variable state used during scheduled execution.
packages/app/src/hooks/useDashboardFilters.tsx Reuses the centralized declaration helper while retaining selected dashboard variable values.
packages/common-utils/src/filters.ts Introduces a shared helper that derives unique variable declarations from dashboard filters.
packages/common-utils/src/variables.ts Adds warning generation for chart configurations that reference in-scope dashboard variables.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Filters[Dashboard variable declarations] --> Empty[Attach empty selections]
  Empty --> Editor[Chart editor alert preview]
  Empty --> Detail[Alert detail preview]
  Empty --> Task[Scheduled alert task]
  Editor --> Query[Rendered ClickHouse query]
  Detail --> Query
  Task --> Query
Loading

Reviews (5): 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 • 300 passed • 1 skipped • 989s

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

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented Aug 14, 2026

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: 141 (+ 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

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. The refactor extracting getDashboardVariableDeclarations preserves the prior useDashboardFilters behavior exactly, the alert-execution details.dashboard access is safe (the TILE provider returns early when the dashboard is missing, and AlertDetails.dashboard is non-optional), and substitution failures are caught and recorded as query errors rather than escaping the handler. The findings below are recommended follow-ups, not merge blockers.

🟡 P2 -- recommended

  • packages/api/src/tasks/checkAlerts/index.ts:630 -- A bare $var reference in a SQL where/having (e.g. ServiceName = $svc) substitutes an empty selection to the literal NULL, producing an always-false predicate, so the alert matches zero rows every window and silently never fires; unlike $__filter/$__conditionalAll, which correctly become match-all no-ops.
    • Fix: Give bare variable references an empty-state no-op consistent with the macros when running for an alert, or block/hard-warn alert creation on tiles that reference variables via bare non-macro SQL syntax.
    • reliability, adversarial
  • packages/app/src/components/alerts/AlertDetailChart.tsx:140 -- The new preview substitution (empty-state variables plus NumberLine display coercion) has no test coverage at any level, so a regression in what the alert preview renders would go undetected.
    • Fix: Add a unit test rendering TileAlertChart with variable-enabled filters and a Number-display tile, asserting the config carries variables with empty values and the display type is coerced to Line.
    • testing
🔵 P3 nitpicks (2)
  • packages/api/src/tasks/checkAlerts/index.ts:630 -- The getDashboardVariableDeclarations(filters).map(d => ({ ...d, values: [] })) empty-variable transform is duplicated verbatim (with near-identical comments) in checkAlerts/index.ts and AlertDetailChart.tsx:142, so the "alerts empty every variable" invariant lives in two independently maintained copies.
    • Fix: Extract a shared helper (e.g. getEmptyDashboardVariables(filters)) in packages/common-utils/src/filters.ts and call it from both sites.
  • packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx:399 -- The whole-form useWatch({ control }) as ChartEditorFormState cast adds a new eslint-disable (ratchet baseline bumped 143→144) and relies on RHF keeping the deep-partial object structurally complete, which the type system does not enforce.
    • Fix: Build the fields convertFormStateToSavedChartConfig needs via named useWatch/getValues calls instead of casting the deep-partial object, matching the file's existing per-field watch pattern.

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

Testing gaps:

  • No integration test covers a bare non-macro SQL variable reference (ServiceName = $svc) on an alerting tile — the exact pattern that degrades to = NULL — only the Lucene, $__filter, and $__conditionalAll no-op cases are exercised.
  • No coverage for the materialized-view optimization path interacting with variable-bearing alert queries, nor for the 300ms debounce window on the additionalAlertWarnings memo (only eventual outcome is asserted).

@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

@jordan-simonovski jordan-simonovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pulpdrew
pulpdrew force-pushed the drew/variable-alerting branch from fae0abd to 988024b Compare August 18, 2026 13:33
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.

2 participants