Skip to content

refactor(mcp): split hyperdx_query into 5 display-type-specific tools - #2294

Merged
kodiakhq[bot] merged 10 commits into
mainfrom
brandon/query-split
May 19, 2026
Merged

refactor(mcp): split hyperdx_query into 5 display-type-specific tools#2294
kodiakhq[bot] merged 10 commits into
mainfrom
brandon/query-split

Conversation

@brandon-pereira

@brandon-pereirabrandon-pereira commented May 15, 2026

Copy link
Copy Markdown
Member

Summary

Replaces the monolithic hyperdx_query MCP tool with five narrow, display-type-specific tools. Each tool's schema contains only the parameters relevant to its use case — no displayType discriminator, no fields from other modes, no conditional required parameters.

  • hyperdx_timeseries — line + stacked_bar charts (shape field, defaults to line)
  • hyperdx_table — table + number + pie aggregations (shape field, auto-upgrades number/pietable when select.length > 1)
  • hyperdx_search — raw event/log row browsing
  • hyperdx_event_patterns — Drain algorithm pattern mining
  • hyperdx_sql — raw ClickHouse SQL (the only tool requiring connectionId)

hyperdx_query is removed from the tool surface.

What changed

New files (packages/api/src/mcp/tools/query/):

  • timeseries.ts, table.ts, search.ts, eventPatterns.ts, sql.ts — one tool registration per file
  • runEventPatterns.ts — extracted event pattern mining logic (pure code move, no logic changes)
  • schemas.ts — rewritten with shared schema fragments (sourceIdSchema, whereSchema, mcpSelectItemSchema, etc.) and targeted where/whereLanguage guidance with WRONG/RIGHT examples

Modified files:

  • index.ts — barrel now registers 5 tools instead of 1
  • queryTool.test.ts — updated to test all 5 tools: schema serialization, functional queries, auto-upgrade behavior, required-field rejection, seeded pattern mining
  • listSources.ts — description and usage hints reference new tool names
  • saveDashboard.ts — hint fixed from hyperdx_queryhyperdx_query_tile (pre-existing bug)
  • content.ts — prompt guide updated for new tool names and event patterns call signature

Breaking change

hyperdx_query is removed. Any external MCP consumer calling it by name will get a tool-not-found error. The five replacement tools cover all the same functionality with narrower, less error-prone schemas.

Changeset is a patch because MCP is still considered beta and subject to change.

Ref: HDX-4252

Replace the monolithic hyperdx_query tool (one schema carrying fields for
all display types) with five narrow tools, each with only its relevant fields:
- hyperdx_timeseries (line + stacked_bar)
- hyperdx_table (table + number + pie, with shape auto-upgrade)
- hyperdx_search (raw event browsing)
- hyperdx_event_patterns (Drain pattern mining)
- hyperdx_sql (raw ClickHouse SQL)
hyperdx_query is removed from the tool surface.
HDX-4252
@changeset-bot

changeset-botBot commented May 15, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: e426e17

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

This PR includes changesets to release 3 packages
NameType
@hyperdx/apiPatch
@hyperdx/appPatch
@hyperdx/otel-collectorPatch

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

vercelBot commented May 15, 2026

Copy link
Copy Markdown

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

ProjectDeploymentActionsUpdated (UTC)
hyperdx-ossReadyReadyPreview, CommentMay 19, 2026 6:23pm

Request Review

@github-actionsgithub-actionsBot added the review/tier-4 Critical — deep review + domain expert sign-off label May 15, 2026
@github-actions

github-actionsBot commented May 15, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Large diff: 1699 production lines changed (threshold: 1000)

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: 1699 (+ 373 in test files, excluded from tier calculation)
  • Branch: brandon/query-split
  • Author: brandon-pereira

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-actionsBot commented May 15, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 180 passed • 3 skipped • 1221s

StatusCount
✅ Passed180
❌ Failed0
⚠️ Flaky2
⏭️ Skipped3

Tests ran across 4 shards in parallel.

View full report →

- table.ts: narrow displayType to union type instead of string
- search.ts: remove redundant ?? '' after zod .default('')
- table.ts: note in shape description that groupBy is ignored for number
- sql.ts: note in description that results are always table rows
Also assert on sourceId in missing-field rejection tests to catch
schema-required-field regressions.
@brandon-pereirabrandon-pereira added review/tier-3 Standard — full human review required and removed review/tier-4 Critical — deep review + domain expert sign-off labels May 15, 2026
@brandon-pereira

Copy link
Copy Markdown
MemberAuthor

Downgrading tier - changed lots of production code but mostly just moving logic not changing logic.

@github-actions

github-actionsBot commented May 15, 2026

Copy link
Copy Markdown
Contributor

PR Review

✅ No critical issues found.

This is a well-scoped refactor — the split is clean, the per-tool schemas are noticeably easier to read than the unified hyperdx_query, error handling and tracing are consistent across all five handlers, and test coverage covers schema serialization, functional execution, the auto-upgrade path, seeded pattern mining, and required-field rejection. The bodyExpression allowlist + splitAndTrimWithBracket check in runEventPatterns.ts is good defensive hardening against injection through that field.

A few minor observations (non-blocking, defer if out of scope):

  • ⚠️runEventPatterns.ts:289whereSnippet builds Lucene syntax as ${bodyColumn}:"...". When bodyColumn is bracket-syntax (e.g. SpanAttributes['http.url'] — explicitly documented as a valid bodyExpression value), the resulting snippet is not parseable as Lucene (per WHERE_DESCRIPTION in schemas.ts:15). It works for the common Body / SpanName case, but the docstring telling agents to paste whereSnippet into a follow-up hyperdx_search will silently fail for any map-attribute body. Pre-existing in spirit, but worth either guarding (emit SQL snippet when bracket-syntax is detected) or noting in the description that whereSnippet is only valid for top-level body columns.
  • ℹ️ table.ts:81–87 — the auto-upgrade from number/pietable is silent. Consider including a hint in the response when an upgrade happens so the caller knows their requested shape changed (mirrors the existing "trimmed for context size" hint pattern in helpers.ts).
  • ℹ️ sql.ts:84–89displayType is hardcoded to 'table', so SQL-mode results can never render as a chart. This matches the prior behavior, but if it's intentional going forward it would be worth a one-liner in the tool description (the current text says "Results are always returned as table rows" which is close but reads as a result-shape statement, not a displayType constraint).
  • ℹ️ MCP.md:120 table still lists hyperdx_query_tile — that's correct (it's a different tool from the removed hyperdx_query), but worth a quick double-check that no doc / prompt references the old hyperdx_query name anywhere external (a repo grep confirms only the changeset mentions it, which is expected).

@hyperdxiohyperdxio deleted a comment from github-actionsBotMay 15, 2026
…rces description, simplify select passthrough
@github-actions

github-actionsBot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Deep Review

Scope: PR #2294 (head: brandon/query-split vs base 8a006265) — 15 files, ~1230 LOC added across the MCP tools/query/ tree, dashboard prompts, and tests.

Intent: Replace the single-tool hyperdx_query surface with five focused MCP tools, extract shared schema fragments, add try/catch around the two queryChartConfig call sites, and fix a stale hyperdx_queryhyperdx_query_tile hint string.

✅ No P0/P1 ship-blockers. The refactor preserves the prior eight display-type capabilities, error envelopes are consistent across the new tool handlers, and the runEventPatterns extraction is a clean code move. Findings below are P2 recommendations and P3 nits.

🟡 P2 -- recommended

  • packages/api/src/mcp/tools/query/timeseries.ts:36 -- the new tool advertises granularity and orderBy as inputs, but the underlying externalDashboardTimeChartConfigSchema strips unknown keys during the .transform() re-parse, so neither field reaches runConfigTile; an agent that sets granularity: "1 minute" gets default auto-bucketing back with no error.
    • Fix: Thread granularity and orderBy through buildTile config into the chart config inside runConfigTile (mirroring the searchOverrides path), or drop the fields from the tool's input schema so callers don't waste a turn on a silent no-op.
    • kieran-typescript
  • packages/api/src/mcp/tools/query/table.ts:82 -- when shape: "number" | "pie" is upgraded to "table" because select.length > 1, the response carries no signal of the rewrite, so an agent that requested a pie chart will narrate the multi-column rows as proportions that were never computed.
    • Fix: Attach a note field to the formatted result (analogous to the trim note in helpers.ts:93) like "Requested shape \"pie\" was upgraded to \"table\" because select has 2 items." so the upgrade is observable in-band rather than only via the schema description.
    • adversarial, api-contract
  • packages/api/src/mcp/__tests__/queryTool.test.ts:162 -- hyperdx_search and hyperdx_event_patterns have explicit "rejects calls missing sourceId" tests, but hyperdx_timeseries, hyperdx_table, and hyperdx_sql (missing connectionId and sql) have none, so a refactor that loosens any of those required fields would pass CI.
    • Fix: Add three mirror tests: one each calling hyperdx_timeseries / hyperdx_table without sourceId, and one each calling hyperdx_sql without connectionId and without sql, all asserting isError === true.
    • testing
  • packages/api/src/mcp/__tests__/queryTool.test.ts:225 -- the two new try/catch blocks added at helpers.ts:225 and :299 returning "ClickHouse query failed: <message>" have no test coverage; a future refactor that removes the wrapping would silently regress error reporting on the builder tools.
    • Fix: Mock ClickhouseClient.queryChartConfig to reject in a unit test, or use hyperdx_sql with semantically broken SQL (SELECT * FROM definitely_missing_table_xyz) and assert isError: true with the "ClickHouse query failed:" prefix.
    • testing, reliability
  • packages/api/src/mcp/tools/query/runEventPatterns.ts:95 -- the SAFE_BODY_EXPR_CHARS allowlist that guards against bodyExpression injection has no test that exercises the rejection branch; the regex is the project's last line of defense for caller-supplied column expressions and a future "cleanup" of the regex would not be caught.
    • Fix: Add a test that calls hyperdx_event_patterns with bodyExpression: "Body) OR (1=1" (and one with a multi-expression value) and asserts the documented isError: true response with the "must be a single column expression" message.
    • testing

🔵 P3 nitpicks

🔵 P3 nitpicks (7)
  • packages/api/src/mcp/tools/query/helpers.ts:239 -- err.message from queryChartConfig is echoed verbatim, and ClickHouse error strings commonly embed the executing query text including auto-resolved table/column names.
    • Fix: Truncate the message at the first newline or strip "while processing query: ..." before surfacing, matching whatever convention runEventPatterns.ts:209 agrees to.
  • packages/api/src/mcp/tools/query/sql.ts:84 -- buildTile() calls externalDashboardTileSchemaWithId.parse(...) unwrapped; a malformed connectionId (a name instead of an ObjectId) throws a ZodError outside the in-handler isError envelope this PR establishes.
    • Fix: Either tighten sourceIdSchema/the connectionId field to .refine(Types.ObjectId.isValid, ...) so Zod surfaces a clean message, or wrap the buildTile() call in try/catch returning a structured isError body.
    • reliability, adversarial
  • packages/api/src/mcp/tools/query/schemas.ts:132 -- the shared orderBySchema.describe() says "mainly useful for table shape", but the new timeseries.ts:36 reuses it; an agent reading the inputSchema for a time-series tool sees a parameter whose own description disclaims its applicability.
    • Fix: Define a separate timeseriesOrderBySchema (or drop orderBy from timeseriesSchema entirely, paired with the silent-strip fix above).
    • agent-native
  • packages/api/src/mcp/tools/query/sql.ts:84 -- the tool hardcodes displayType: 'table'; an agent asked to "produce a SQL-backed time-series chart" cannot do it in one call, and the description doesn't point at hyperdx_save_dashboard + hyperdx_query_tile as the workaround.
    • Fix: Append one line to the hyperdx_sql description: "For SQL-backed line/stacked_bar/pie tiles use hyperdx_save_dashboard with configType: 'sql'."
    • agent-native
  • packages/api/src/mcp/tools/query/index.ts:10 -- no standalone heatmap one-shot tool; the dashboard prompt at prompts/dashboards/content.ts:66 teaches heatmap usage, but reaching it requires a multi-step save+query dance.
    • Fix: Note the workaround in hyperdx_list_sources' description, or add a hyperdx_heatmap tool that reuses runConfigTile with displayType: 'heatmap' and the trace-source guard.
    • agent-native
  • packages/api/src/mcp/__tests__/queryTool.test.ts:270 -- the auto-upgrade tests only assert isError is falsy; they never check that the upgrade actually occurred (returned shape, groupBy preservation through the upgrade).
    • Fix: Parse the response JSON and assert the rendered shape is consistent with table semantics, and that a follow-up call with shape: 'pie' + groupBy returns rows grouped by the supplied column.
    • testing, adversarial, kieran-typescript
  • .changeset/split-query-tools.md:2 -- the removal of hyperdx_query ships as a patch bump; the changeset body itself doesn't flag the tool removal, so consumers scanning release notes by changeset alone may miss that the old tool name is gone.
    • Fix: Add a **Breaking:** hyperdx_query is removed; migrate to hyperdx_timeseries / hyperdx_table / hyperdx_search / hyperdx_event_patterns / hyperdx_sql. line to the changeset body, and consider adding a one-paragraph "Stability and versioning" section to MCP.md documenting the beta policy.
    • api-contract

Reviewers (10): correctness, testing, maintainability, project-standards, api-contract, adversarial, kieran-typescript, reliability, agent-native, learnings-researcher.

Testing gaps:

  • No coverage of the two new try/catch envelopes in helpers.ts (forced ClickHouse rejection) or the SAFE_BODY_EXPR_CHARS rejection branch in runEventPatterns.ts.
  • Required-field rejection tests exist for two of the five new tools; the other three (hyperdx_timeseries, hyperdx_table, hyperdx_sql) are uncovered.
  • Auto-upgrade tests in table.ts don't verify that the upgrade actually shaped the response, and no test feeds an event_patternswhereSnippet back through hyperdx_search to pin the documented cross-tool chain.
  • No docs/solutions/ learnings exist for HyperDX MCP server design; the only docs/solutions/ tree in the repo belongs to the vendored ce-plugin/ and is unrelated. Worth capturing after merge via /ce-compound.

@brandon-pereira
brandon-pereira requested review from a team and pulpdrew and removed request for a teamMay 19, 2026 13:58
pulpdrew
pulpdrew previously approved these changes May 19, 2026

@pulpdrewpulpdrew 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

Comment threadpackages/api/src/mcp/tools/query/schemas.ts Outdated
…e operator support
Address Drew's PR review comment: the WHERE_DESCRIPTION incorrectly
stated that map attribute access requires whereLanguage:"sql". In fact,
Lucene fully supports map attributes via dot notation
(e.g. SpanAttributes.http.method:GET).
Changes:
- Add Lucene dot-notation examples to WHERE_DESCRIPTION
- Add MAP ATTRIBUTES section showing syntax for each language
- Remove incorrect 'MAP ATTRIBUTE ACCESS requires sql' claim
- Update WHERE_LANGUAGE_DESCRIPTION to accurately reflect that Lucene
supports comparisons, wildcards, ranges, and map attributes natively
- Only recommend SQL for IN(...) lists, complex expressions, or
function calls
@kodiakhq
kodiakhqBot merged commit f5ae006 into mainMay 19, 2026
19 checks passed
@kodiakhq
kodiakhqBot deleted the brandon/query-split branch May 19, 2026 18:29
brandon-pereira added a commit that referenced this pull request May 22, 2026
…re flag
Remove eval-branch artifacts that duplicate functionality already in main:
- Remove splitQuery/ directory (parallel impl of split tools)
- Remove splitQueryTool.test.ts
- Remove unified hyperdx_query tool registration from query/index.ts
- Remove hyperdxQuerySchema, validateQueryInput from schemas.ts
- Remove useSplitQuery() feature flag from mcpServer.ts
- Remove 5 hyperdx_query tests from queryTool.test.ts
Main already has the split tools (timeseries, table, search, sql,
eventPatterns) from PR #2294. The richer shared schema descriptions
(where, whereLanguage, groupBy, valueExpression) are kept.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automergereview/tier-3Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@brandon-pereira@pulpdrew