Repository navigation
feat: expose summary metrics through the mcp - #2855
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: c9419b0 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 |
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Additional context: 2 file(s) in private internal-tooling packages, excluded from the line count Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Greptile SummaryThe PR exposes summary metrics through MCP discovery while keeping builder tools restricted to renderer-supported metric kinds.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/api/src/mcp/tools/sources/describeMetric.ts | Adds summary discovery and a database-qualified raw-SQL redirect. |
| packages/api/src/mcp/tools/sources/metricKinds.ts | Separates discoverable metric kinds from kinds supported by builder queries. |
| packages/api/src/mcp/tools/sources/listMetrics.ts | Includes summary metrics in catalog scans, filters, and pagination. |
| packages/hdx-eval/src/scenarios/metric-saturation/generate.ts | Makes the summary metric and misleading database-timeout evidence load-bearing in the evaluation. |
| packages/hdx-eval/src/scenarios/metric-saturation/ground-truth.json | Adds summary adoption checks and calibrated database-exoneration grading. |
Reviews (4): Last reviewed commit: "review feedback" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
E2E Test Results✅ All tests passed • 276 passed • 1 skipped • 1098s
Tests ran across 4 shards in parallel. |
Deep ReviewScope: Intent (advisory): make OTel Core invariant verified directly: the builder/dashboard schemas still bind ✅ No critical issues found. 🟡 P2 -- recommended
🔵 P3 nitpicks (2)🔵 P3 nitpicks (2)
Reviewers (10 dispatched): correctness, adversarial, testing, maintainability, project-standards, api-contract, performance, kieran-typescript, agent-native, learnings-researcher. Note: at synthesis time only Testing gaps:
|
6b57ca4 to
34cff8d
Compare
34cff8d to
c9419b0
Compare
| 'JOINs, sub-queries, CTEs, querying tables not registered as sources, or looking at ' + | ||
| 'summary-type metrics (the metricTables.summary table on a metric source, which the ' + | ||
| 'builder tools cannot query).\n\n' + |
There was a problem hiding this comment.
Just in case you haven't seen it, there is also this PR that is intended to discourage agents from using raw SQL: #2840
Does adding this here (or the clickstack_sql elsewhere) have any effect on that goal / do we need to run any evals to measure the effect? cc @brandon-pereira
There was a problem hiding this comment.
I'm also curious, does the agent actually generate reasonably accurate queries for summary metrics? The metrics queries tend to be very complicated.
There was a problem hiding this comment.
does the agent actually generate reasonably accurate queries for summary metrics?
I think the queries are not completely accurate. Here's a couple of examples on separate runs of metric-saturation with Fable 5:
SELECT tostartofinterval(timeunix, interval 10 minute ) AS ts,
resourceattributes['service.name'] AS svc,
avg (sum/count),
max (arrayelement(valueatquantiles.value, length(valueatquantiles.value))),
sum (count)
FROM DEFAULT.eval_metric_saturation_otel_metrics_summary
WHERE metricname = 'db.client.operation.duration'
AND $__timefilter(timeunix)
GROUP BY ts,
svcSELECT tostartofinterval(timeunix, interval 10 minute ) AS ts,
resourceattributes['service.name'] AS svc,
max (arrayelement(valueatquantiles.value, indexof(valueatquantiles.quantile, 0.99 ))) AS p99_ms,
max (arrayelement(valueatquantiles.value, indexof(valueatquantiles.quantile, 0.5 ))) AS p50_ms,
sum (count) AS ops
FROM DEFAULT.eval_metric_saturation_otel_metrics_summary
WHERE metricname = 'db.client.operation.duration'
AND $__timefilter(timeunix)
GROUP BY ts,
svc
ORDER BY ts,
svc limit 100I'm no expert on summary metrics but according to Claude there are two common errors in these:
- sum(Count) treating a cumulative counter as a per-interval value. Neither query computes per-series deltas, which is what's actually needed.
- Aggregating summary quantiles across series. Query 1 does
max(last quantile), query 2 doesmax(p99)andmax(p50). Both give you "worst per-series value," not a true service-wide percentile.
Summary metrics are stored in ClickHouse and declared on metric sources, but
renderChartConfigcannot translate them — so the MCP hid them entirely:clickstack_list_metricsskipped the kind,clickstack_describe_metricrejected it, and an agent asking about them hit a dead end with no guidance. Agents could not even learn these metrics existed.This PR makes summary metrics discoverable through the MCP while keeping them un-queryable by the builder tools: every surface that lists or describes them directs agents to
clickstack_sqlinstead. No renderer changes; no SQL recipes baked into tool output.What changed
MCP discovery (
packages/api/src/mcp/)DISCOVERABLE_METRIC_KINDS(= queryable kinds +summary) feedingclickstack_list_metrics(scan, kind filter, cursor codec),clickstack_describe_sourcename sampling, andclickstack_describe_metric(kind:"summary"now returns attribute keys/values/unit/description, with aclickstack_sqlnext-step instead of a timeseries example)clickstack_timeseries/clickstack_table/ dashboard tile schemas still rejectsummary; their descriptions, pluslist_sources,clickstack_sql, and the dashboards prompt, now carry a one-line redirect: query the table inmetricTables.summaryviaclickstack_sqlEval verification (
packages/hdx-eval/)metric-saturationredesigned so the summary metric is load-bearing: misleading "Feature-store query timed out" WARN logs (and the reporter's prompt) blame the sharedproduct_catalogPostgres; the only exoneration is inventory-service's flatdb.client.operation.durationsummary, readable only via raw SQLdiscovered_summary_metric/queried_summary_via_sql(adoption),exonerates_database(positive), and a negation guard onfalse_blame_databaseso "not the database" doesn't trip itResults
12-run batch (
metric-saturation, claude-fable-5 + claude-opus-4-6, 3 runs/cell). The hyperdx arm has this PR's discovery + redirect; the clickhouse arm must find the summary table on its own.Every hyperdx-arm run followed the full path — discovery → redirect →
clickstack_sqlagainst the summary table — and reached the correct root cause ("JVM heap saturation, not the database"). Before this PR the same checks read 0/6 by construction: the metric name was invisible to agents.References