Skip to content

feat(mcp): improve metric discovery, add quiet-saturation eval scenario - #2861

Open
karl-power wants to merge 6 commits into
mainfrom
karl/improve-metric-mcp-adoption
Open

feat(mcp): improve metric discovery, add quiet-saturation eval scenario#2861
karl-power wants to merge 6 commits into
mainfrom
karl/improve-metric-mcp-adoption

Conversation

@karl-power

@karl-power karl-power commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Why

Eval transcripts showed agents solving investigations without ever touching metrics — not because metrics weren't useful, but because they were the only signal behind a discovery wall. After list_sources, logs and traces are queryable immediately (key columns are in the catalog), while a metric source showed only opaque table names: querying it cost 1–3 extra calls (describe_source/list_metrics) just to learn what exists. Under a turn budget, agents rationally skipped it — even when explicitly nudged (we tested this: prompt- and output-level hints naming the exact metrics were ignored in 7/7 deliveries when metrics weren't on the efficient path).

What changed

  • clickstack_list_sources: metric sources now include metricNamesPreview — up to 10 recently-reported metric names per kind, sampled from the team's own tables — plus a usage note that metrics are queried directly via clickstack_table/clickstack_timeseries with metricType + metricName, no describe hop needed. Best-effort under a 3s wall-clock budget with a concurrency pool and per-table dedup; omitted silently on timeout.
  • metricNames.ts (new): the metric-name sampler extracted from describeSource.ts into a shared module, with a widening lookback (24h → 30d, first non-empty window wins) so sparse or batch-emitted metrics still produce a sample. describe_source uses the same sampler, so its per-kind sample gets the lookback too.

Measured wins

Benchmarked with the hdx-eval framework on quiet-saturation (connection-pool leak where metric history is the efficient diagnostic path), branch vs main, identical seed/anchor/prompts, claude-fable-5 + claude-opus-4-6, 3 runs/cell, two independent batches.

First tool call whose args name a target metric (the load-bearing pool gauges), every run, both batches:

arm batch 1 batch 2 range
branch / fable-5 6, 5, 4 5, 5, 6 4–6
branch / opus-4-6 6, 7, 6 6, 7, 7 6–7
main / fable-5 8, 10, 9 8, 8, 8 8–10
main / opus-4-6 16, 14, 13 14, 13, 21 13–21

Zero overlap across all 24 runs (branch ≤7, main ≥8; p ≈ 0.001 by permutation): the preview removes the discovery hops entirely (list_sources → query), where main inserts list_metrics/describe_source detours or trace-grinds first.

Headline metrics from the second batch (which ran exactly this minimal build):

branch/fable main/fable branch/opus main/opus
Combined score 96% 91% 82% 84%
LLM-judge (weighted) 96% 92% 92% 84%
Programmatic checks 96% 96% 97% 93%
Pre-symptom onset attributed (distinguishes_true_onset) 100% 100% 67% 33%
Tool calls (mean) 13.0 13.0 19.7 20.7
Tool errors (mean) 0.0 0.3 2.3 0.7
Metric adoption 100% 100% 100% 100%
  • fable-5: +5pp combined here, +9pp in batch 1 (12.3 vs 15.7 calls) — consistently equal-or-fewer calls, zero tool errors, faster wall clock.
  • opus-4-6: the freed budget converts to depth rather than speed — judge +8pp and the pre-symptom onset attributed twice as often. Its combined score dips below main only because of a 12pp tool-error penalty from unrelated agent-side query-syntax mistakes (one run retried an identical malformed select 4×).
  • Metric adoption is 100% on both arms — the change lowers the cost of the metric path rather than nudging inclination; agents reach the load-bearing signal roughly twice as fast.

@changeset-bot

changeset-bot Bot commented Aug 11, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9588258

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

This PR includes changesets to release 4 packages
Name Type
@hyperdx/hdx-eval Patch
@hyperdx/api Patch
@hyperdx/app 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 11, 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 17, 2026 4:31pm
hyperdx-storybook Ready Ready Preview Aug 17, 2026 4:31pm

Request Review

@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Aug 11, 2026
@github-actions

github-actions Bot commented Aug 11, 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: 683 production lines changed (Tier 2 max: < 250)

Additional context: 8 file(s) in private internal-tooling packages, excluded from the line count

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

Stats
  • Production files changed: 3
  • Production lines changed: 683 (+ 638 in test files, excluded from tier calculation)
  • Branch: karl/improve-metric-mcp-adoption
  • Author: karl-power

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

@greptile-apps

greptile-apps Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes metric names immediately discoverable from clickstack_list_sources and adds a quiet-saturation evaluation scenario for measuring metric-assisted investigations.

  • Extracts metric-name sampling into a shared widening-lookback implementation used by source listing and description.
  • Adds bounded, concurrent, best-effort metric preview collection with query guidance.
  • Adds the quiet-saturation telemetry generator, grading rubric, reporting support, and tests.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/api/src/mcp/tools/sources/listSources.ts Adds bounded concurrent metric-name previews and direct-query guidance; the revised cache key resolves both previously reported cache defects.
packages/api/src/mcp/tools/sources/metricNames.ts Centralizes metric-name sampling, widening lookback windows, and optional unit and description enrichment.
packages/api/src/mcp/tools/sources/describeSource.ts Reuses the shared metric-name sampler so sparse metrics benefit from the wider lookback.
packages/hdx-eval/src/scenarios/quiet-saturation/generate.ts Adds deterministic telemetry for a connection-pool saturation investigation.
packages/hdx-eval/src/grading/programmatic.ts Adds informational adoption checks that are reported without affecting adoption scores.
packages/hdx-eval/src/reports/markdown.ts Labels informational checks and explains their exclusion from adoption scoring.

Sequence Diagram

sequenceDiagram
  participant Agent
  participant ListSources as clickstack_list_sources
  participant Metadata
  participant ClickHouse
  Agent->>ListSources: Request source catalog
  ListSources->>Metadata: Load metric table metadata
  par Bounded sampling by table
    Metadata->>ClickHouse: Sample MetricName with widening lookback
    ClickHouse-->>Metadata: Recent metric names
  end
  Metadata-->>ListSources: Per-kind samples
  ListSources-->>Agent: Sources + metricNamesPreview + metricsUsage
  Agent->>ClickHouse: Query metricType + metricName directly
Loading

Reviews (7): Last reviewed commit: "Merge branch 'main' into karl/improve-me..." | Re-trigger Greptile

Comment thread packages/api/src/mcp/tools/sources/listSources.ts Outdated
Comment thread packages/hdx-eval/src/scenarios/quiet-saturation/generate.ts
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Deep Review

No critical issues found. No P0/P1: SQL is fully parameterized through the chSql DSL (identifiers, metric names, and date bounds all bind as params), team scoping is preserved, and the preview path is best-effort — every failure is swallowed so clickstack_list_sources always returns. The findings below are hardening and coverage recommendations on an otherwise careful change.

🟡 P2 -- recommended

  • packages/api/src/mcp/tools/sources/listSources.ts:98 -- Each clickstack_list_sources call constructs a PreviewClickhouseClient per connection (and describeSource.ts:178 a ClickhouseClient) that is never closed, unlike runEventPatterns.ts:196, so keep-alive sockets accumulate on the agent's highest-frequency, previously CH-free entry-point call.
    • Fix: Close every created client in a finally block after the sampling race completes, and mirror it in describeSource.
    • reliability, adversarial, performance
  • packages/api/src/mcp/tools/sources/listSources.ts:94 -- Credential resolution runs as a single Promise.all over getConnectionById before the abort/timeout is armed; the if (!connection) return guard handles a null result but not a throw, so one connection that errors rejects the whole batch, drops metricNamesPreview for every source, and orphans clients already created for earlier connections.
    • Fix: Isolate each connection resolution with its own try/catch (or Promise.allSettled), and arm the wall-clock guard before resolving credentials.
    • reliability, adversarial
  • packages/api/src/mcp/tools/sources/metricNames.ts:168 -- The new sampler's argMax unit/description enrichment executes under describe_source but no test asserts the resulting unit/description values, and the preview timeout/abort race and per-table dedup cache have no coverage.
    • Fix: Add unit tests for sampleMetricNamesForKind enrichment (values plus the query-failure fallback), the window-widening loop, and the abort/dedup behavior of attachMetricNamePreviews.
    • testing, kieran-typescript, project-standards, performance, adversarial
🔵 P3 nitpicks (11)
  • packages/api/src/mcp/tools/sources/listSources.ts:121 -- Preview results are correlated back to summaries via an object-identity Map and mutated in place; a later refactor that clones or serializes a summary between construction and mutation would silently drop metricNamesPreview with no compiler error.
    • Fix: Key the previews map by sourceId and merge into summaries by id, or build summaries only after sampling completes.
    • maintainability, kieran-typescript
  • packages/api/src/mcp/tools/sources/metricNames.ts:156 -- The lookback widens only when a window returns zero rows, but under timeout_overflow_mode=break an empty result can mean "ran out of time," so a slow table escalates 1h → 24h → 30d into progressively heavier scans it can least afford.
    • Fix: Distinguish a break-truncated result from a genuinely empty one, or stop widening once the previous window consumed most of the budget.
    • adversarial
  • packages/api/src/mcp/tools/sources/listSources.ts:195 -- Internal metadata strategy/settings sub-queries are issued without the abort signal, so after the 3s race resolves, abandoned sampling tasks keep running against ClickHouse; the queue is never cleared or paused on abort.
    • Fix: Thread the abort signal into the metadata sub-queries and clear/pause the queue when the budget expires.
    • adversarial
  • packages/api/src/mcp/tools/sources/metricNames.ts:70 -- The aggressive max_execution_time/break override on PreviewClickhouseClient also applies to shared metadata/system queries, and a break-truncated strategy result cached in the singleton MetadataCache could be reused by the normal query path, degrading its strategy selection.
    • Fix: Use a separate client or cache namespace for the preview path so partial system-query results cannot leak into normal queries.
    • adversarial
  • packages/api/src/mcp/tools/sources/describeSource.ts:364 -- Metric-name sampling now issues up to three sequential widening queries per kind as the final stage under the 10s budget, so it is most expensive precisely for the sparse sources the widening was added to help.
    • Fix: Verify sparse-metric sources reliably get a populated metricNames block, or budget stage 5 explicitly rather than letting it inherit only leftover time.
    • correctness, performance, api-contract
  • packages/api/src/mcp/tools/sources/listSources.ts:202 -- metricNamesPreview (kind → string[]) and describe_source's metricNames (kind → {name, unit?, description?}[]) diverge in shape under near-identical names, undocumented in either tool description.
    • Fix: Document the shape difference in both tool descriptions, or align the two shapes.
    • api-contract
  • packages/api/src/mcp/tools/sources/listSources.ts:328 -- The metricsUsage note tells agents metrics are queryable with "no describe call needed," but the preview is built with enrich: false, so it omits unit/description and an agent may build queries on wrong unit assumptions.
    • Fix: Soften the wording to note describe is only needed when unit or semantics matter for interpretation.
    • agent-native
  • packages/api/src/mcp/tools/sources/listSources.ts:218 -- The tool description documents metricNamesPreview but not the new conditional top-level metricsUsage field.
    • Fix: Add a line documenting the metricsUsage note, matching the existing metricNamesPreview pattern.
    • api-contract
  • packages/hdx-eval/src/grading/programmatic.ts:111 -- An adoption check set composed only of informational checks leaves totalWeight === 0 and scores 0 instead of full credit; latent today because the new quiet-saturation scenario includes a scoring check.
    • Fix: Treat an all-informational (or empty) scoring set as full credit rather than 0.
    • correctness
  • packages/api/src/mcp/tools/sources/listSources.ts:110 -- The abort/timeout boilerplate is hand-rolled a fourth time (after listMetrics, describeMetric, describeSource) as a diverging resolve-based variant, re-deriving the same timer-leak and double-abort edge cases.
    • Fix: Extract a shared timeout-signal helper covering both the reject-on-timeout and best-effort/partial-result variants.
    • maintainability
  • packages/hdx-eval/README.md -- The Scenarios table was not given a quiet-saturation row though the scenario is registered in scenarios/index.ts; every other scenario has a row.
    • Fix: Add the quiet-saturation row to the Scenarios table.
    • project-standards

Pre-existing (not introduced by this PR, noted for awareness): the shared MetadataCache has no TTL/eviction and this preview path newly drives it on the hot path via per-call now date-range keys; fetchMetricNameEnrichments carries over an unchecked as { data: EnrichmentRow[] } cast on the ClickHouse response; and describeSource's map-key value sampling relies on SqlString.escape (MySQL-oriented but adequate for ClickHouse quoting).


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

Testing gaps:

  • No test exercises the 3s wall-clock backstop with a slow/hung sampling query to confirm list_sources returns within budget and in-flight queries are actually abandoned.
  • No test proves PreviewClickhouseClient's max_execution_time/break override reaches ClickHouse and yields partial results rather than an error.
  • No test covers the per-(connection, db, table, tsExpr) dedup cache firing once when two sources share a physical table, nor a connection-resolution failure leaving other sources' previews intact.
  • No direct unit test of sampleMetricNamesWithLookback exhausting all three windows (asserting the widest-last call sequence and [] result) or of rubric.ts informational-adoption-check validation branches.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 296 passed • 1 skipped • 968s

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

Tests ran across 4 shards in parallel.

View full report →

@pulpdrew pulpdrew 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.

I see my comments in packages/api/src/mcp/tools/sources/metricNames.ts are on code that was just relocated. It'd be nice to fix them but we can treat them as non-blocking.

Comment thread packages/api/src/mcp/tools/sources/metricNames.ts Outdated
Comment thread packages/api/src/mcp/tools/sources/metricNames.ts
Comment thread packages/api/src/mcp/tools/sources/metricNames.ts Outdated
Comment thread packages/api/src/mcp/tools/sources/metricNames.ts
Comment thread packages/api/src/mcp/tools/sources/listSources.ts
Comment thread packages/api/src/mcp/tools/sources/listSources.ts Outdated
Comment thread packages/api/src/mcp/tools/sources/listSources.ts
Comment thread packages/api/src/mcp/tools/sources/listSources.ts Outdated
@karl-power
karl-power force-pushed the karl/improve-metric-mcp-adoption branch from b3395ca to 2ced399 Compare August 17, 2026 12:46
@karl-power

Copy link
Copy Markdown
Contributor Author

Thanks @pulpdrew all of your points addressed in fc770da

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

});
clients.set(connectionId, {
clickhouseClient,
metadata: getMetadata(clickhouseClient),

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.

issue (blocking): IMO using the existinggetMetadata causes the preview client's 2.5s cap and break mode to reach entries other tools read.

The preview client stops queries at 2.5s and returns whatever it got with no error. It also looks up table schemas on the way, and those go in the shared cache.

The issue here might be that columns that exist seem like they don't which seems like it would lead to some inconsistencies in the MCP outputs. This would remain incorrect until a process restart.

A private cache should contain this potential issue:

import { Metadata, MetadataCache } from '@hyperdx/common-utils/dist/core/metadata';

// in the clients Map type
metadata: Metadata;

// here
metadata: new Metadata(clickhouseClient, new MetadataCache()),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants