Repository navigation
feat(mcp): record which source each MCP tool call reads - #3522
brandon-pereira wants to merge 3 commits into
Conversation
The more analytics we have on MCP usage, the better. Tools now set mcp.source.id, mcp.source.kind and mcp.source.ids on the tool span, add a low-cardinality source_kind attribute to the tool duration and error metrics, and tag the queries that read a source with it in log_comment.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: f108b8c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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-4
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
PR Review
3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor 2 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep ReviewScope: ✅ No critical issues found. The core mechanism is sound and well-guarded. Verified directly: nested 🟡 P2 — recommended
🔵 P3 nitpicks (4)
Reviewers (7): testing, maintainability, performance, reliability, kieran-typescript, project-standards, previous-comments. (The correctness and adversarial reviewers were still running at synthesis time; findings reflect the seven that completed plus a direct correctness/concurrency pass over Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
E2E Test Results✅ All tests passed • 462 passed • 1 skipped • 1513s
Tests ran across 4 shards in parallel. |
…t lookup Folds the source lookup, kind check and client lookup duplicated between the trace waterfall and breakdown handlers into loadTraceSource, and switches list_metrics and describe_metric to getMcpClickhouseClient (jscpd).
| * Load the trace source a trace tool reads and a client for its connection, or | ||
| * the error to return. | ||
| */ | ||
| export async function loadTraceSource( |
There was a problem hiding this comment.
🔵 minor — loadTraceSource duplicates loadHealthSource (serviceHealth/source.ts)
loadHealthSource in packages/api/src/mcp/tools/query/serviceHealth/source.ts:53 already does the same thing: getSource, recordMcpSource, check kind === Trace, and build the tool-name error. It also has a better not-found message (sourceNotFound suggests the right ID when the caller passed a name or table). Build loadHealthSource on top of loadTraceSource, or move sourceNotFound into loadTraceSource, so the trace tools share one loader and one not-found message.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const id = source._id.toString(); | ||
| if (scope.sources.has(id)) return; | ||
| scope.sources.set(id, source.kind); | ||
| if (scope.sources.size === 1) { |
There was a problem hiding this comment.
🔵 minor — Primary source_kind is nondeterministic for clickstack_query_tiles
queryTiles.ts:279 runs runConfigTile for several tiles at once through PQueue. Whichever tile's getSource resolves first becomes mcp.source.id/mcp.source.kind and the metric's source_kind. On a dashboard with both log and trace tiles, the same call can be counted under log on one run and trace on the next. For batch tools, pick the primary source deterministically: record sources in tile order before the tiles run, or use a fixed label such as mixed when more than one kind is seen.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Summary
Records which source each MCP tool call reads. Why: more analytics on MCP usage.
mcp.source.idandmcp.source.kindhold the first source the tool resolved.mcp.source.idslists every distinct source, for tools that touch several (clickstack_query_tiles, the trace waterfall with its linked log source). They're set through a newrecordMcpSourcehelper inmcp/utils/tracing.ts. The helper finds the tool span through anAsyncLocalStoragescope set bywithToolTracing, not through the active span, so the attributes land on the tool span even when a child span is active.hyperdx.mcp.tool.duration_msandhyperdx.mcp.tool.errorsget asource_kindattribute (log | trace | session | metric | promql, ornone). The source id never goes on a metric.withQueryAttribution({ source }), solog_commentcarries the source id and keeps the tool's surface, label and trace. The attribution schema and version are unchanged.Call sites:
runConfigTile(builder and raw SQL paths, which covers search/table/timeseries/query_tile(s)/sql),clickstack_sql, describe_source, list_metrics, describe_metric, event patterns / emerging signals, event deltas, denoise, trace waterfall (trace and log sources), trace breakdown and service health.Review tip: hide whitespace. Several handlers had their post-resolution body wrapped in
withQueryAttribution, which re-indents them. Nothing else changed in those blocks.Tests:
tracing.test.ts: span attributes, staying on the tool span under a nested child span, multi-source ids,source_kindon both metrics (including the throw path), and isolation between concurrent tool calls.sourceAttribution.test.ts: runsrunConfigTileinsidewithToolTracingagainst the real node ClickHouse client (network stubbed) and checks thatlog_commentcarries the source with surface/label/trace intact, including per-tile sources in a batch.How to reproduce
Starting point:
yarn dev, an MCP client connected to/api/mcp.On
main:clickstack_tableagainst a log source.mcp.tool.clickstack_tablespan has no source attributes, and the query'slog_commentinsystem.query_loghas nosource.On this branch:
mcp.source.id,mcp.source.kind=logandmcp.source.ids. The tool metrics carrysource_kind=log, andlog_commentincludes"source":"<id>".