Skip to content

feat: Broadcast adhoc dashboard filter conditions - #3366

Merged
pulpdrew merged 1 commit into
mainfrom
drew/adhoc-filter-broadcast-sql
Oct 16, 2026
Merged

pulpdrew merged 1 commit into
mainfrom
drew/adhoc-filter-broadcast-sql

Conversation

@pulpdrew

@pulpdrew pulpdrew commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR broadcasts adhoc filter conditions to tiles

  • Builder tiles: broadcasts through the existing filter config property
  • SQL Tiles: broadcasts through the existing $__filters macro
  • PromQL: Auto-applied to the promql expression by parsing the expression and injecting the condition(s) as matchers on each metric selector.

Screenshots or video

Without filters

Screenshot 2026-10-14 at 8 47 09 AM

With filters

Screenshot 2026-10-14 at 8 46 40 AM

Rendered conditions are shown in the SQL/PromQL preview:

Screenshot 2026-10-14 at 8 49 11 AM Screenshot 2026-10-14 at 8 48 08 AM

How to test on Vercel preview

  • Create a dashboard
  • Add some adhoc filters and some tiles (on the same sources)
  • Select some values for the filters and view the filtering in action

To test promql, test locally against a PromQL source on the metrics_ts table.

References

  • Linear Issue: Related to HDX-5658
  • Related PRs:

@pulpdrew
pulpdrew added this pull request to stack #3342 October 13, 2026 18:33
@vercel

vercel Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

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

Project Deployment Actions Updated
hyperdx-oss Ready Ready Preview Oct 16, 2026 5:15pm UTC
hyperdx-storybook Ready Ready Preview Oct 16, 2026 5:15pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 59cb26b

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

@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 431 passed • 1 skipped • 1222s

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

Tests ran across 4 shards in parallel.

View full report →

@github-actions

github-actions Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Duplicate code analysis

No new duplicate code introduced by this PR.


jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 125 in total (1.69%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

@greptile-apps

greptile-apps Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge, with the DateTime suggestion guard supplied by dependent PR #3380.

Summary

Broadcasts ad hoc conditions to builder queries, raw SQL through $__filters, and PromQL selectors. Editor previews use the same language-specific filters.

  • Dashboard ad hoc conditions now narrow matching SQL and PromQL tiles.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Dashboard ad hoc conditions] --> B[Select filters for tile source]
  B --> C[SQL conditions]
  B --> D[PromQL matchers]
  C --> E[Builder filters]
  C --> F[Raw SQL filters macro]
  D --> G[Expand variables and macros]
  G --> H[Add matchers to queried selectors]
  E --> I[Tile and editor preview]
  F --> I
  H --> I
Loading

Reviews (7) · Last reviewed commit: "feat: Broadcast adhoc dashboard filter c..." · Reviewed by Greptile

Comment thread packages/common-utils/src/macros.ts Outdated
Comment thread packages/common-utils/src/filters.ts
Comment thread packages/common-utils/src/core/promqlLabelInjection.ts Outdated
Comment thread packages/common-utils/src/filters.ts
Comment thread packages/common-utils/src/filters.ts
@pulpdrew
pulpdrew force-pushed the drew/adhoc-filter-broadcast-sql branch from 43e067a to 5138567 Compare October 15, 2026 17:56
@pulpdrew
pulpdrew marked this pull request as ready for review October 15, 2026 17:56
@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Oct 15, 2026
@github-actions

github-actions Bot commented Oct 15, 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:

  • Diff size: 535 production lines changed (Tier 2 max: < 250)
  • Cross-layer change: touches frontend (packages/app) + 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: 13
  • Production lines changed: 535 (+ 751 in test files, excluded from tier calculation)
  • Branch: drew/adhoc-filter-broadcast-sql
  • 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.

Comment thread packages/common-utils/src/filters.ts
Comment thread packages/common-utils/src/filters.ts
Comment thread packages/app/src/components/DBEditTimeChartForm/utils.ts
Comment thread packages/common-utils/src/filters.ts
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

  • 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
  • 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
  • Anything about surrounding code, or asking you to widen the change (hoist a helper,
    dedupe with another file, fix other call sites): never fix here, whatever the
    severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

1 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 1 minor

None could be anchored to changed lines; they are listed below.

Findings outside the changed lines

1 minor
  • 🔵 packages/app/src/components/DBEditTimeChartForm/utils.ts:516 — Choosing filters by query language now lives in three places → Keep one place that picks filters by language: let withBroadcastFilters do the split and have resolveTilePreviewFilters only decide whether filters apply. Right now resolveTilePreviewFilters picks PromQL vs SQL filters here, then EditTimeChartForm.tsx:547 runs the result through withBroadcastFilters, which does the same pick again. Tile in DBDashboardPage.tsx:657-664 does it a third time with its own sqlFilters/promqlFilters memos. If the rule changes (for example, Lucene becomes allowed on PromQL), all three must be edited by hand.

Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.

@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. This PR is well executed: all four prior review findings (unused-expression parse failures, SQL→PromQL LIKE throw, comment-aware separators, newline escaping) are confirmed resolved in the current head, and the SQL/PromQL BroadcastFilter partition was verified leak-proof at the tile boundary. The items below are recommendations and nits.

🟡 P2 — recommended

  • packages/common-utils/src/variables.ts:68 — escapePromqlStringValue now escapes \, " and \n but not carriage return or other control characters, so a filter value containing a raw \r renders a double-quoted matcher that Prometheus rejects and errors the tile.
    • Fix: Escape \r (and \t) alongside \n, or strip the control-character class, mirroring the newline handling just added.
    • correctness, adversarial, kieran-typescript
  • packages/common-utils/src/macros.ts:597 — substitutePromqlChartConfigTemplates calls injectPromqlLabelMatchers for every queried series in one .map, and that function throws on any parse error, so one expression the pinned lezer grammar rejects but Prometheus accepts takes down the entire multi-series tile the moment a PromQL filter is active (a tile that rendered before the filter was applied). The lezer/Prometheus divergence trigger is unverified in this diff; the whole-tile abort is confirmed from the control flow.
    • Fix: Catch per series and degrade only the failing expression (surfacing which one failed) rather than aborting the whole tile.
    • adversarial, correctness
🔵 P3 nitpicks (3)
  • packages/common-utils/src/macros.ts:580 — queriedSeries.has(series) compares PromqlSeries by object identity and works only because getQueriedPromqlSeries returns the same references getPromqlSeries produced; a future refactor that clones series would make .has() always false, silently skipping injection and showing unfiltered data as filtered — the exact failure the throw was added to prevent. No defect today.
    • Fix: Select queried series by index or by an inline predicate (isTimeSeriesDisplayType + non-blank expression) instead of reference-identity Set membership, and add a test asserting matchers reach queried but not blank rows.
    • maintainability, kieran-typescript
  • packages/common-utils/src/filters.ts:985 — adhocConditionsToFilters produces SQL ConditionFilters but is named generically while its sibling is explicitly adhocConditionsToPromqlFilters, blurring the SQL/PromQL split this change exists to make explicit.
    • Fix: Rename to adhocConditionsToSqlFilters so the pair reads symmetrically at the call sites.
  • packages/app/src/components/DBEditTimeChartForm/utils.ts:254 — the previewFilters nested ternary mixes two different && short-circuit shapes, making the PromQL-vs-SQL branch hard to parse.
    • Fix: Early-return the PromQL partition in an if (isPromqlChartConfig(config)) block, then handle the SQL/configConsumesBroadcastFilters gate separately.

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

Testing gaps:

  • No positive assertion that a broadcast-enabled SQL ad hoc filter's conditions reach broadcastedFilters after the new getSqlFilters wrapping.
  • No test for a multi-series PromQL tile where one queried series is valid and a sibling is unparseable (the whole-tile-abort behavior above).
  • injectPromqlLabelMatchers lacks coverage for Prometheus-valid-but-potentially-lezer-divergent inputs (keyword-like names, negative offset, @ start(), nested subqueries) and for selectors carrying offset/@/range modifiers.
  • No test exercises a filter value containing a carriage return or tab.

Residual risk (pre-existing, unverified): SQL ad hoc keys are interpolated verbatim into toString(${key}); map/JSON keys from toQuotedClickHouseKeyExpression emit Column['<property>'] without re-escaping the property, so a telemetry attribute name containing a single quote could break out of the string. This predates the PR but the broadcast path now feeds it — worth confirming map-key properties are escaped.

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

Base automatically changed from drew/adhoc-filters-inputs to main October 15, 2026 19:21
Comment thread packages/common-utils/src/types.ts
Comment thread packages/app/src/components/DBEditTimeChartForm/utils.ts
Comment thread packages/app/src/DBDashboardPage.tsx
Comment thread packages/common-utils/src/filters.ts
Comment thread packages/app/src/components/DBEditTimeChartForm/utils.ts
Comment thread packages/app/src/DBDashboardPage.tsx

@brandon-pereira brandon-pereira left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM - nice one!

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. The implementation is well-structured and well-tested: the SQL/PromQL BroadcastFilter split is enforced at both ends via getSqlFilters/getPromqlFilters, PromQL matcher injection is correctly scoped to queried series only, and all four earlier review findings (unused-expression parse failures, the SQL→PromQL LIKE throw, comment-aware separators, and newline escaping) are confirmed resolved in the current head. The SQL injection surface on adhoc keys is a pre-existing, by-design trust boundary (dashboards already carry viewer-authored SQL executed with the viewer's own credentials), not a new vulnerability. The items below are recommendations and nits.

🟡 P2 — recommended

  • packages/common-utils/src/filters.ts:963 — renderAdhocConditionSql broadcasts a hand-typed condition key verbatim as toString(${key}), so a free-typed key naming a non-identifier column (e.g. one containing a hyphen) renders as a subtraction expression that ClickHouse mis-evaluates, breaking or wrongly filtering every matching tile; dropdown-selected keys are already quoted via toQuotedClickHouseKeyExpression, so only the hand-typed path is exposed.
    • Fix: Quote non-identifier column keys at condition-save time in the editor submit path (or inside renderAdhocConditionSql) using the same conversion the value lookup applies, so typed and selected keys render identically.
    • previous-comments, security
  • packages/common-utils/src/macros.ts:596 — substitutePromqlChartConfigTemplates injects matchers for every queried series in one .map, and injectPromqlLabelMatchers throws on any lezer parse failure, so a single queried series the pinned lezer grammar rejects but Prometheus accepts aborts the entire multi-series tile the moment a PromQL filter is active; queryPromqlChartConfig has no surrounding try/catch, unlike the preview path. The whole-tile blast radius is confirmed from control flow; the lezer/Prometheus divergence trigger is not demonstrated in this diff.
    • Fix: Wrap injection per series and degrade only the failing expression (surfacing which one failed) instead of letting one throw discard the sibling series.
    • adversarial, correctness
🔵 P3 nitpicks (4)
  • packages/common-utils/src/filters.ts:1028 — Switching an adhoc filter from SQL to PromQL keeps its saved conditions, but isAdhocConditionSupported validates only the operator; a dropped LIKE/NOT LIKE condition and a SQL-shaped key like attributes['foo'] (which becomes a syntactically valid but never-matching PromQL label) both leave the tile silently showing more or less data than the filter implies, with no on-tile indicator.
    • Fix: Flag keys that are not plausible PromQL label names on the PromQL path and surface a per-tile "N conditions not applied" indicator mirroring the editor's invalid-pill marker.
  • packages/common-utils/src/variables.ts:68 — escapePromqlStringValue escapes \, ", and \n but not \r or \t; these pass through the Prometheus lexer as literal characters rather than breaking the query, so the worst case is a matcher that silently matches nothing on pasted values containing them.
    • Fix: Escape \r and \t alongside \n, or strip the control-character class, so such values render a matcher consistent with the intended text.
  • packages/common-utils/src/macros.ts:596 — queriedSeries.has(series) selects queried series by object identity, working only because getQueriedPromqlSeries returns the same references getPromqlSeries produced; a future refactor that clones or normalizes series would make .has() always false and silently skip injection, rendering unfiltered data as filtered. No defect today (a test covers it).
    • Fix: Select queried series by index or an explicit queried flag rather than reference-identity Set membership.
  • packages/app/src/components/DBEditTimeChartForm/utils.ts:254 — The previewFilters nested ternary mixes two different && short-circuit shapes for the PromQL vs SQL branches, making the partition hard to parse at a glance.
    • Fix: Early-return the PromQL partition in an if (isPromqlChartConfig(config)) block, then handle the SQL/configConsumesBroadcastFilters gate separately.

Reviewers (10): correctness, security, adversarial, testing, maintainability, project-standards, kieran-typescript, previous-comments, agent-native, learnings-researcher.

Testing gaps:

  • No positive assertion that a broadcast-enabled SQL adhoc filter's conditions reach broadcastedFilters (the memo uses the isFilterBroadcastEnabled predicate and the getSqlFilters wrapper, a path distinct from the tested getFilterQueriesForSource).
  • No test for a multi-series time-series PromQL tile where one queried series is valid and a queried sibling is unparseable (the whole-tile-abort path); existing coverage is single-expression or a Number display that queries only the first series.
  • No characterization test bounding lezer-promql acceptance against Prometheus-valid inputs (offset, @ modifiers, subqueries, reserved-word/UTF-8 metric names) that drive the abort above.
  • No test exercises the live query path (queryPromqlChartConfig) throwing to confirm it surfaces as a react-query error state rather than an unfiltered render.
  • No test covers a filter value containing a carriage return or tab.

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

@pulpdrew
pulpdrew merged commit 9205823 into main Oct 16, 2026
29 checks passed

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — 59cb26b6 Deployed Oct 16, 2026 by vercel[bot]
Preview – hyperdx-storybook — 59cb26b6 Deployed Oct 16, 2026 by vercel[bot]
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