Skip to content

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

Merged
kodiakhq[bot] merged 6 commits into
mainfrom
karl/improve-metric-mcp-adoption
Aug 27, 2026
Merged

feat(mcp): improve metric discovery, add quiet-saturation eval scenario#2861
kodiakhq[bot] merged 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: fdeeac5

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 27, 2026 1:50am
hyperdx-storybook Ready Ready Preview Aug 27, 2026 1:50am

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: 687 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: 687 (+ 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 directly discoverable from MCP source listings and adds widening lookback sampling shared with source description. It also adds the quiet-saturation evaluation scenario and informational adoption-check reporting.

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 preview collection and direct-query guidance to source discovery.
packages/api/src/mcp/tools/sources/metricNames.ts Extracts shared metric-name sampling, widening lookbacks, and optional unit and description enrichment.
packages/api/src/mcp/tools/sources/describeSource.ts Reuses the shared widening-lookback sampler for metric source descriptions.
packages/hdx-eval/src/scenarios/quiet-saturation/generate.ts Adds deterministic telemetry for diagnosing gradual connection-pool saturation.
packages/hdx-eval/src/grading/programmatic.ts Evaluates informational adoption checks while excluding them from the weighted adoption score.
packages/hdx-eval/src/reports/aggregate.ts Carries informational adoption metadata into aggregate reports.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[clickstack_list_sources] --> B[Metric source tables]
  B --> C[Sample metric names]
  C --> D{Recent names found?}
  D -->|No| E[Widen lookback]
  E --> C
  D -->|Yes| F[metricNamesPreview]
  F --> G[Direct table or timeseries query]
Loading

Reviews (9): 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

Scope: MCP metric discovery (clickstack_list_sources metric-name preview, new shared metricNames.ts sampler, describeSource.ts reuse) plus the hdx-eval quiet-saturation scenario and informational adoption checks. Base cf8e7e72.

✅ No critical issues found. The concurrency/abort/timeout design in attachMetricNamePreviews is sound (per-task try/catch swallows individual sampling failures, Promise.race(drained, abortedPromise) bounds wall-clock, clearTimeout in finally, cached promise is always awaited under a guard so no unhandled rejection), the enrichment SQL is parameterised via the chSql Identifier/String DSL (no injection surface introduced), and the all-informational division is guarded (programmatic.ts:111). Every one of the 20 prior review comments — cache-key collision, summary-kind leakage, shared-cache truncation, anyLastargMax, PQueue, break-mode timeout, 1h initial window — is resolved in the current code.

🟡 P2 — recommended

  • packages/api/src/mcp/tools/sources/listSources.ts:114 — the new integration tests cover recent-data, wider-lookback fallback, summary exclusion, and empty-tables, but not the wall-clock-budget path: nothing forces the 3s AbortController to fire and asserts the catalog still returns with metricNamesPreview omitted, nor exercises the per-(connection,db,table,timestamp) dedup cache.
    • Fix: add an integration test that makes preview sampling exceed the budget and asserts list_sources still returns successfully with the preview absent, plus one asserting two sources on the same physical table trigger a single sample.
🔵 P3 nitpicks (2)
  • packages/hdx-eval/src/grading/programmatic.ts:111 — when every adoption check is informational, totalWeight is 0 and the score falls to 0 rather than a neutral/full value; harmless today because no scenario is all-informational, but the result is surprising if one ever is.
    • Fix: treat an all-informational check set as a full/neutral score and document the intent, or assert at least one scoring check exists during rubric validation.
  • packages/api/src/mcp/tools/sources/describeSource.ts:357 — the per-kind metric-name sampling runs DISCOVERABLE_METRIC_KINDS (5) in Promise.all, each doing the widening lookback (up to 3 sequential windows) with no server-side break cap, so a cold/empty metric source can issue many queries under the 10s budget.
    • Fix: consider bounding the describe-path sampling the same way the preview path does, or capping the lookback breadth for metric sources.

Reviewers (2): previous-comments, orchestrator diff analysis (correctness, reliability, performance, testing, security/injection lenses).

Testing gaps: timeout-omission path and dedup cache in attachMetricNamePreviews are unverified; all-informational adoption-check edge is untested.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

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

Status Count
✅ Passed 315
❌ Failed 0
⚠️ Flaky 4
⏭️ 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

Copy link
Copy Markdown
Contributor Author

Thanks @pulpdrew all of your points addressed in fc770da

pulpdrew
pulpdrew previously approved these changes Aug 17, 2026

@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

Comment thread packages/api/src/mcp/tools/sources/listSources.ts Outdated
Comment thread packages/api/src/mcp/tools/sources/listSources.ts
@kodiakhq
kodiakhq Bot merged commit 892cc65 into main Aug 27, 2026
27 checks passed
@kodiakhq
kodiakhq Bot deleted the karl/improve-metric-mcp-adoption branch August 27, 2026 01:54
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