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 |
🔵 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
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:
|
Review fixes for #2855: - Database-qualify the summary clickstack_sql redirect everywhere: new shared SUMMARY_SQL_REDIRECT constant + summarySqlRedirect(db, table) helper in metricKinds.ts replace the dozen hand-duplicated strings; clickstack_list_sources now emits each source's `database` so raw SQL cannot resolve tables against the connection's default database - Negative schema tests pin the discoverable-but-not-queryable boundary: metricType:"summary" is rejected by mcpSelectItemSchema and the dashboard tile schema while every queryable kind is accepted - Strengthen the metric-saturation exoneration linkage: the summary now carries server.address=postgres-primary (same host AND database as the misleading timeout logs), and prose no longer claims the flat summary "proves" the database healthy - it is strong cross-client evidence corroborated by the timeouts clustering in the subject's GC-stall windows - Rewrite all five false_blame_* checks as tempered-token patterns (deploy-regression house style): negations only disarm the penalty between trigger and target, so "the root cause is the database, but the deploy is innocent" now fires while "the database is healthy" phrasings stay safe; add a rubric test block covering positives, rule-outs, blame phrasings, and exonerates_database - Changeset body rewritten as prose with single-quoted package names
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.
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