feat(mcp): improve metric discovery, add quiet-saturation eval scenario - #2861
feat(mcp): improve metric discovery, add quiet-saturation eval scenario#2861karl-power wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 8a7ad67 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Additional context: 8 file(s) in private internal-tooling packages, excluded from the line count Review process: Full human review — logic, architecture, edge cases. Stats
|
Greptile SummaryThe PR makes metric names discoverable directly from the MCP source catalog and adds a quiet-saturation evaluation scenario.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/api/src/mcp/tools/sources/listSources.ts | Adds team-scoped, timeout-bounded metric preview collection and direct-query guidance to the source catalog. |
| packages/api/src/mcp/tools/sources/metricNames.ts | Extracts metric-name sampling into a shared helper and widens sparse-metric discovery from 24 hours to 30 days. |
| packages/api/src/mcp/tools/sources/describeSource.ts | Replaces the local metric sampler with the shared widening-lookback implementation. |
| packages/hdx-eval/src/grading/programmatic.ts | Evaluates informational adoption checks while excluding their zero weight from adoption scoring. |
| packages/hdx-eval/src/reports/aggregate.ts | Propagates informational adoption metadata into aggregate reporting. |
| packages/hdx-eval/src/reports/markdown.ts | Labels informational checks and explains their exclusion from adoption scores. |
| packages/hdx-eval/src/scenarios/quiet-saturation/generate.ts | Generates deterministic telemetry for diagnosing a gradual connection-pool leak and its distractors. |
| packages/hdx-eval/src/scenarios/quiet-saturation/ground-truth.json | Defines outcome, judge, and metric-adoption criteria for the new scenario. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[clickstack_list_sources] --> B[Load team sources and connections]
B --> C[Build source summaries]
C --> D[Sample metric tables concurrently]
D --> E[Try 24-hour lookback]
E -->|Empty| F[Try 30-day lookback]
E -->|Names found| G[Attach metricNamesPreview]
F -->|Names found| G
F -->|Empty or timeout| H[Return summary without preview]
G --> I[Return catalog and metrics usage guidance]
H --> I
Reviews (3): Last reviewed commit: "feat(mcp): improve metric discovery, add..." | Re-trigger Greptile
Deep Review✅ No critical issues found. The change is defensively built: metric-name preview sampling in No P0/P1/P2 findings with a concrete failure mode surfaced from the completed analysis. 🔵 P3 nitpicks (1)
Reviewers (1): security (completed, no findings). The full panel (correctness, performance, reliability, adversarial, testing, maintainability, api-contract, project-standards, agent-native, learnings) was dispatched but had not returned at synthesis time; the assessment above reflects the security reviewer plus direct analysis of every changed production file ( Testing gaps:
Residual risk: under a quiet environment where the 24h window is empty across many metric sources, every kind falls through to a 30d scan; the client-side 3s budget bounds |
E2E Test Results✅ All tests passed • 280 passed • 1 skipped • 1093s
Tests ran across 4 shards in parallel. |
2e3c3f7 to
3b1a262
Compare
3b1a262 to
8a7ad67
Compare
pulpdrew
left a comment
There was a problem hiding this comment.
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.
| const kindColumns = | ||
| cachedColumns ?? | ||
| (await metadata.getColumns({ databaseName, tableName, connectionId })); | ||
| const columnNames = new Set(kindColumns.map(c => c.name)); |
There was a problem hiding this comment.
Could we not rely on the caching already done in metadata.getColumns here? Wondering if we can get rid of this extra cachedColumns argument
| /** | ||
| * Fetch MetricUnit and MetricDescription for a batch of metric names. | ||
| * Uses `anyLast` so the most-recent value wins when a metric has changed | ||
| * unit/description over time. | ||
| */ |
There was a problem hiding this comment.
I'm not sure how important this is in regards to units and descriptions, but anyLast will not reliably return the most recent (by timestamp or insertion order). If you want the most recent, you'd need argMax(<column of interest>, <timestamp column>)
| if (rest.signal.aborted) break; | ||
| const samples = await sampleMetricNamesForKind({ | ||
| ...rest, | ||
| dateRange: [new Date(now.getTime() - windowMs), now], |
There was a problem hiding this comment.
Small optimization, but you could set the end of the date range to the start of the previous window instead of now - since you know there is no data in the previous shorter window, there's no need to rescan it. This probably matters little when the timestamp is early in the ordering or partition key.
| const METRIC_NAME_LOOKBACK_WINDOWS_MS: readonly number[] = [ | ||
| 24 * 60 * 60 * 1000, // 24 hours | ||
| 30 * 24 * 60 * 60 * 1000, // 30 days | ||
| ]; |
There was a problem hiding this comment.
IMO 24 hours strikes me as kind of long for the initial window, especially with a 3s limit - the UI often defaults to ranges much shorter than that. Since this is best-effort (from my understanding), it seems like surfacing metrics from a smaller window may be enough, and likely much more performant at large scales.
| const timeoutId = setTimeout( | ||
| () => controller.abort(), | ||
| METRIC_PREVIEW_TIMEOUT_MS, | ||
| ); |
There was a problem hiding this comment.
Are we able to set max_execution_time + timeout_overflow_mode = break instead of (or in addition to) an abort controller for these queries? If so that would allow us to return the information clickhouse was able to query within the 3s, instead of no data if the query times out.
| async function runWithConcurrency( | ||
| tasks: Array<() => Promise<void>>, | ||
| limit: number, | ||
| signal: AbortSignal, | ||
| ): Promise<void> { | ||
| let next = 0; | ||
| const workers = Array.from( | ||
| { length: Math.min(limit, tasks.length) }, | ||
| async () => { | ||
| while (next < tasks.length && !signal.aborted) { | ||
| const task = tasks[next++]; | ||
| try { | ||
| await task(); | ||
| } catch { | ||
| // Best-effort: individual sampling failures never fail the call. | ||
| } | ||
| } | ||
| }, | ||
| ); | ||
| await Promise.all(workers); | ||
| } |
There was a problem hiding this comment.
Would it be simpler to use PQueue here like we do elsewhere for bounding concurrency?
| for (const entry of entries) { | ||
| const client = clients.get(entry.connectionId); | ||
| if (!client) continue; | ||
| for (const [kind, tableName] of Object.entries(entry.metricTables)) { |
There was a problem hiding this comment.
issue: I think the preview offers summary metrics that the query tools reject.
Onboarding fills in all five metric tables, summary included. The loop here samples every one of them, so metricNamesPreview.summary lands in the response, and the new metricsUsage note tells the agent everything in the preview is queryable with clickstack_timeseries / clickstack_table.
Could we do something like this?
for (const [kind, tableName] of Object.entries(entry.metricTables)) {
if (!QUERYABLE_METRIC_KINDS.includes(kind as QueryableMetricKind)) continue;
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 includemetricNamesPreview— 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 viaclickstack_table/clickstack_timeserieswithmetricType + 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 fromdescribeSource.tsinto 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_sourceuses 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 vsmain, 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:
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 insertslist_metrics/describe_sourcedetours or trace-grinds first.Headline metrics from the second batch (which ran exactly this minimal build):
distinguishes_true_onset)