Skip to content

feat: Substitute variables in PromQL charts - #2994

Open
pulpdrew wants to merge 3 commits into
mainfrom
drew/promql-variable-substitution
Open

feat: Substitute variables in PromQL charts#2994
pulpdrew wants to merge 3 commits into
mainfrom
drew/promql-variable-substitution

Conversation

@pulpdrew

@pulpdrew pulpdrew commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR extends variable substitution to PromQL charts. E2E has been updated to seed a Timeseries engine table for validating the variable substitution.

Note that this requires enabling the following feature toggles:

(In the UI)

  • NEXT_PUBLIC_ENABLE_PROMQL=true
  • NEXT_PUBLIC_ENABLE_DASHBOARD_VARIABLES=true

(In the API)

  • ENABLE_PROMQL=true

Screenshots or video

Screenshot 2026-08-25 at 11 40 24 AM Screenshot 2026-08-25 at 11 35 30 AM Screenshot 2026-08-25 at 11 35 21 AM Screenshot 2026-08-25 at 11 35 17 AM

How to test

  • Create a PromQL source, the default.metrics_ts timeseries table should exist after enabling promql
  • Create a dashboard and add a variable that queries instance IDs ResourceAttributes['service.instance.id'] from the metrics gauge table
  • Create a promql tile that references a variable, eg. http_client_duration_milliseconds_count{instance=~"$Instance"}

References

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

@changeset-bot

changeset-bot Bot commented Aug 25, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: dbfd2d6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@hyperdx/common-utils Patch
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

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

@vercel

vercel Bot commented Aug 25, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 26, 2026 5:51pm
hyperdx-storybook Ready Ready Preview Aug 26, 2026 5:51pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR extends dashboard-variable substitution to PromQL chart previews and rendered dashboard tiles.

  • Adds language-aware variable discovery, formatting, escaping, and substitution for PromQL.
  • Passes referenced dashboard variables into PromQL query configurations.
  • Adds PromQL-enabled E2E fixtures and coverage for variable-driven charts.
  • Restricts PromQL metadata lookup to PromQL sources.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/common-utils/src/variables.ts Adds PromQL-aware variable parsing, formatting, escaping, and chart-config substitution.
packages/app/src/DBDashboardPage.tsx Includes referenced dashboard variables in queried PromQL tile configurations.
packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx Resolves referenced variables for PromQL chart previews as well as existing chart types.
packages/app/src/components/ChartEditor/PromqlChartEditor.tsx Limits PromQL metric-name metadata requests to sources explicitly identified as PromQL.
packages/app/tests/e2e/features/dashboard-filter-variables.spec.ts Adds end-to-end coverage for creating and rendering a PromQL chart that references a dashboard variable.
packages/app/tests/e2e/fixtures/e2e-fixtures.json Adds the E2E PromQL source to the shared source fixture.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Dashboard[Dashboard variable selections] --> Filter[Find variables referenced by PromQL]
  Filter --> Config[Attach referenced variables to chart config]
  Config --> Substitute[Apply PromQL-aware substitution and escaping]
  Substitute --> Query[Execute PromQL query]
  Query --> Chart[Render chart]
Loading

Reviews (4): Last reviewed commit: "test: Create promql source in e2e fixtur..." | Re-trigger Greptile

Comment thread packages/common-utils/src/variables.ts
Comment thread packages/app/tests/e2e/features/dashboard-filter-variables.spec.ts Outdated
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 315 passed • 1 skipped • 1151s

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

Tests ran across 4 shards in parallel.

View full report →

Comment thread packages/app/tests/e2e/features/dashboard-filter-variables.spec.ts Outdated
@pulpdrew
pulpdrew marked this pull request as ready for review August 25, 2026 18:04
@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Aug 25, 2026
@github-actions

github-actions Bot commented Aug 25, 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)

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

Stats
  • Production files changed: 12
  • Production lines changed: 199 (+ 745 in test files, excluded from tier calculation)
  • Branch: drew/promql-variable-substitution
  • 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 25, 2026

Copy link
Copy Markdown
Contributor

Deep Review

No critical issues found. This PR cleanly extends dashboard-variable substitution to PromQL charts. The LANGUAGE_SETTINGS refactor consolidates two substitution entry points into one substituteVariables, the PromQL escaping is sound (regex-escape then double-quote-string-escape, verified against combined backslash/quote/metacharacter values), variable changes propagate into the React Query queryKey via the config object so tiles refetch on selection change, and unit + E2E coverage for the new behavior is thorough. The prior-review concerns were either explicitly accepted by the author (SQL regex-in-literal escaping) or withdrawn by the reviewer (E2E fixture "suite-wide exposure" — no cross-source generic field discovery exists).

No injection risk was found: dashboard values are regex-escaped and then have \ and " escaped for the double-quoted PromQL label matcher, so a crafted value cannot break out of the string literal to inject additional selectors. A raw newline in a value would yield an invalid (rejected) query rather than an exploit.

🔵 P3 nitpicks (2)
  • packages/common-utils/src/variables.ts:45escapePromqlStringValue is byte-identical to escapeLuceneValue, so the intent (two separately-evolving escapers vs. accidental duplication) is not obvious to a future maintainer.
    • Fix: Add a one-line comment stating the duplication is deliberate because the two languages' string-literal rules may diverge, or have one delegate to the other.
  • packages/common-utils/src/variables.ts:596 — The PromQL default regex format regex-escapes each value, so a variable used under the exact-match operator ({label="$var"} rather than =~"$var") silently produces a non-matching literal like label="a\.b" for value a.b.
    • Fix: Document that PromQL variable references assume the =~ regex operator (as the E2E test uses), so users know to pick =~ or the :csv format for exact matches.

Review lenses applied (7): correctness, security, adversarial, maintainability, testing, typescript, previous-comments.

Testing gaps: none substantive — new escaping, empty-selection (.*), multi-value alternation, macros-disabled, substitutePromqlChartConfigVariables, and filterReferencedVariables PromQL paths all have unit coverage, plus an end-to-end series-narrowing test.

Comment thread packages/app/tests/e2e/fixtures/e2e-fixtures.json
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