Skip to content

feat(api): support ad hoc dashboard filters in the external API - #3659

Open
knudtty wants to merge 3 commits into
aaron/adhoc-follow-upfrom
aaron/adhoc-external-api
Open

knudtty wants to merge 3 commits into
aaron/adhoc-follow-upfrom
aaron/adhoc-external-api

Conversation

@knudtty

@knudtty knudtty commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #3658.

Summary

The external v2 dashboards API (/api/v2/dashboards) can now create, read, and update dashboards with ADHOC filters. Until now it hid them on GET and rejected them on POST/PUT. PromQL external API support is already on main, so PromQL ad hoc filters (sourceType: "promql") are included alongside SQL ones.

  • Schema: ADHOC is a new variant in the external filter schemas, with fields named as stored (sources, not sourceIds) so no translation is needed. Source IDs must be valid ObjectIds. As with STATIC_LIST, isBroadcastEnabled and isVariableEnabled each have one allowed value (true), so callers can omit them.
  • Saved values: savedFilterValues accepts and returns adhoc entries, which hold a filter's conditions.
  • Validation: every ID in sources and appliesToSourceIds must exist and be a kind the filter's sourceType supports: Log, Trace, or Session for sql, PromQL for promql. The internal API enforces the same rule.
  • MCP: unchanged. Its read paths pass a new hideAdhoc option to convertToExternalDashboard, and its input schemas are separate. MCP support is the next PR in this stack.
  • OpenAPI: adds the AdhocFilterInput, AdhocSavedFilterValue, and AdhocFilterCondition schemas and regenerates openapi.json.

How to reproduce

Starting point: yarn dev, with a personal API key from Team Settings and a Logs source ID.

On main:

  1. POST /api/v2/dashboards with filters: [{ "type": "ADHOC", "name": "Conditions", "sourceType": "sql", "sources": ["<logs-source-id>"] }] returns 400.
  2. GET on a dashboard that already has an ad hoc filter, created in the UI, returns filters: [].

On this branch:

  1. The same POST returns 200, and the filter comes back with isBroadcastEnabled: true and isVariableEnabled: true.
  2. Adding savedFilterValues: [{ "type": "adhoc", "name": "Conditions", "conditions": [{ "key": "ServiceName", "operator": "IN", "value": "api" }] }] stores and returns the conditions. Opening the dashboard shows them in the filter input.
  3. Pointing sources at a PromQL source while keeping sourceType: "sql" returns 400 with that source ID in the message.
  4. GET followed by PUT of the response body round-trips unchanged.

How to test on Vercel preview

N/A. API-only change.

Testing

  • make dev-int FILE=src/routers/external-api/__tests__/dashboards.int.test.ts passes, and covers GET, POST for SQL and PromQL, a GET → PUT round-trip, schema rejections, and source-kind rejections.
  • make dev-int FILE=src/mcp/__tests__/dashboards passes, so MCP behavior is unchanged.
  • New unit test for convertToExternalDashboard({ hideAdhoc }).

References

The v2 dashboards API now creates, reads, and updates ADHOC filters (SQL and
PromQL source types) and their adhoc saved conditions. Sources and
appliesToSourceIds must exist and match the filter's sourceType. MCP keeps
hiding ad hoc filters via convertToExternalDashboard's hideAdhoc option.
@knudtty
knudtty requested a review from a team as a code owner November 13, 2026 21:28
@vercel

vercel Bot commented Nov 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 Nov 13, 2026 10:01pm UTC
hyperdx-storybook Ready Ready Preview Nov 13, 2026 10:01pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d36609d

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

This PR includes changesets to release 3 packages
Name Type
@hyperdx/api Minor
@hyperdx/app Minor
@hyperdx/otel-collector Minor

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

* Returns an error message if an ad hoc filter reads from or applies to a
* source of a kind its `sourceType` can't query, or null if none does.
*/
function getAdhocFilterSourceKindError(

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.

🔵 minor — Ad hoc source-kind check copies the internal API's and drops its skip for unchanged references, so GET → PUT can fail

getFilterSourceIds and getAdhocFilterSourceKindError repeat getFilterSourceRefs/validateFilterSources in packages/api/src/routers/api/dashboards.ts:68-109. The internal copy skips references the stored filter with the same ID already had, so a source that was later deleted or changed kind doesn't block unrelated saves. This copy checks every ADHOC filter on each PUT (dashboards.ts:3738). So if a UI-created ad hoc filter points at a source that has since been deleted or changed kind, a verbatim GET → PUT now returns 400. That contradicts the PR's claim that "the internal API enforces the same rule". Fix: move the ref-building and existing-ref skip into one shared helper, for example in common-utils filters or packages/api/src/utils/, and pass existingDashboard.filters from the PUT path, as the heatmap and formula gates already do with existingTiles.

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

@github-actions

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

1 posted as inline comment(s) on the changed lines.


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

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. No P0/P1 defects: the new ad hoc filter write/read paths are type-safe (exhaustive satisfies never guards intact, McpSavedFilterValues widening to DashboardFilterValue[] is assignable), the strict Zod unknown-key rejection survives .extend(), source IDs are team-scoped through getMissingSources, and translateFilterToExternalFilter cannot throw on GET. Recommendations below.

🟡 P2 -- recommended

  • packages/api/src/mcp/tools/dashboards/patchDashboard.ts:106 -- An MCP patch/save write that sends savedFilterValues overwrites the whole array, silently dropping adhoc entries the MCP schema can't represent (and can't see, since reads pass hideAdhoc: true), so ad hoc saved conditions created via the external API are lost unrecoverably; the overwrite mechanism is pre-existing and MCP ad hoc support is a stated follow-up.
    • Fix: On MCP writes, merge rather than replace — preserve existing isAdhocFilterValue entries and swap only the MCP-visible sql/variable subset.
    • adversarial
  • packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts:8606 -- The appliesToSourceIds wrong-kind test asserts only .expect(400) without checking the message, so it cannot distinguish the appliesToSourceIds kind check from any other validation that returns 400 and would not catch a regression in that specific path.
    • Fix: Assert response.body.message contains the offending source ID, matching the sibling sources wrong-kind test at line 8603.
    • testing, correctness
🔵 P3 nitpicks (5)
  • packages/api/src/utils/zod.ts:196 -- externalAdhocFilterShape.sources (.min(1) only) and appliesToSourceIds have no .max(), unlike every other array input in the file (conditions .max(100), select .max(20), formulas .max(10)); a caller can submit arbitrarily large ObjectId arrays, bloating the stored dashboard document.
    • Fix: Cap both arrays (e.g. .max(50)), matching the file's convention of bounding every array input.
    • security, api-contract
  • packages/api/src/utils/externalApi.ts:243 -- getMcpCompatibleFilters/getMcpCompatibleFilterValues are named for one caller but are reached generically from the hideAdhoc branch of convertToExternalDashboard; the behavior is simply "drop ADHOC", so the MCP-coupled name reads less true than a behavior name.
    • Fix: Rename to a behavior-descriptive helper such as stripAdhocFilters/stripAdhocFilterValues.
    • maintainability, kieran-typescript
  • packages/api/src/routers/external-api/v2/utils/dashboards.ts:782 -- hideAdhoc defaults to false, so each of the six MCP call sites must remember to pass hideAdhoc: true; a future MCP tool that omits it would silently expose preview ad hoc filters.
    • Fix: Add a thin convertToMcpDashboard wrapper that bundles the MCP-only defaults (hidePromql, hideAdhoc: true) so MCP callers can't drop the flag.
    • maintainability
  • packages/api/src/routers/external-api/v2/dashboards.ts:3536 -- An adhoc savedFilterValues entry whose name matches no ADHOC filter's variableName is persisted and echoed on GET but dropped at load, so it looks like a saved default that never applies; the MCP path warns on orphaned variable entries but the REST path emits no equivalent signal.
    • Fix: Extend the orphaned-name warning (or add validation) to cover adhoc saved values on the external API, or document the silent-ignore.
    • adversarial
  • packages/api/src/routers/external-api/v2/dashboards.ts:2473 -- QueryExpressionFilterInput.whereLanguage documents default: "sql" but the underlying Zod schema has no default, inconsistent with the new AdhocFilterInput.whereLanguage which correctly omits a default for the same schema (pre-existing).
    • Fix: Drop the stale default: "sql" from QueryExpressionFilterInput.whereLanguage so both fields reflect the actual optional-no-default behavior.
    • api-contract

Reviewers (8): correctness, security, api-contract, adversarial, kieran-typescript, testing, maintainability, project-standards.

Testing gaps:

  • No PUT test actually mutates an ad hoc filter or asserts the PUT-persisted internal shape (the round-trip test only echoes the GET body).
  • No test confirms cross-sourceType operators (IN on a PromQL filter, =| on a SQL filter) are accepted-but-inert as documented.
  • No boundary tests for conditions at 100/101, variableName maxLength/pattern, or AdhocSavedFilterValue.name at 1024.
  • No test omits variableName to confirm read-time derivation still resolves the matching adhoc saved value.
  • No test exercises an MCP write against a dashboard already holding adhoc saved values (the data-loss path above is uncovered).

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

Copy link
Copy Markdown
Contributor

Duplicate code analysis

2 new clones introduced by this PR.

  • packages/api/src/mcp/tools/dashboards/getDashboardTile.ts:44-59 ↔ packages/api/src/mcp/tools/dashboards/queryTile.ts:71-86 — 16 lines
  • packages/api/src/mcp/tools/dashboards/queryTile.ts:64-83 ↔ packages/api/src/mcp/tools/dashboards/queryTiles.ts:170-189 — 20 lines

If one of these duplicates a helper that already exists, import it instead
of redefining it. If the duplication is deliberate, ignore this comment —
the check is advisory and never fails the build.


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

@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Nov 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI.

Why this tier:

  • Critical-path files (2) — tenancy, public API, or shipped database config:
    • packages/api/src/routers/external-api/v2/dashboards.ts
    • packages/api/src/routers/external-api/v2/utils/dashboards.ts

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 12
  • Production lines changed: 450 (+ 198 in test files, excluded from tier calculation)
  • Critical-path lines changed: 209
  • Branch: aaron/adhoc-external-api
  • Author: knudtty

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

@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 465 passed • 1 skipped • 1428s

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

Tests ran across 4 shards in parallel.

View full report →

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — d36609d0 Deployed Nov 13, 2026 by vercel[bot]
Preview – hyperdx-storybook — d36609d0 Deployed Nov 13, 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-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant