feat(hotspots): roll up MCP server tools into one row per server#427
feat(hotspots): roll up MCP server tools into one row per server#427willwashburn wants to merge 1 commit into
Conversation
Closes #424. `burn hotspots` already broke down per-tool cost, but a chatty MCP server (relaycast, github, ...) showed up as N independent rows — none individually expensive, collectively a hidden hotspot. The SDK now exposes `McpServerAggregation` + `aggregate_by_mcp_server`, which group any `mcp__<server>__<tool>` attribution by `<server>`, sum calls / tokens / ride-turns / cost, and carry the top three representative tool basenames (cost desc, name asc). The CLI surfaces the rollup as `mcpServers` in `--json` and renders a `Top MCP servers by cost` table when non-empty.
📝 WalkthroughWalkthroughThis PR implements a rollup feature for MCP server tool attribution in the hotspots analyzer. It introduces a new aggregation type that groups ChangesMCP server rollup feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 9-13: The Unreleased changelog entry ending with "(`#424`)" should
drop the PR/issue marker: edit the CHANGELOG.md entry for "burn hotspots" (the
Unreleased root) and remove the trailing " (`#424`)" so the line ends after
"`mcpServers` in `--json`." and no PR/issue/reference remains; keep the rest of
the text unchanged.
In `@packages/sdk-node/CHANGELOG.md`:
- Around line 7-10: Update the Unreleased changelog bullet in CHANGELOG.md by
removing the trailing issue/PR reference "(`#424`)" from the hotspots() entry;
keep the text and impact-first phrasing intact and ensure the entry still
mentions the new mcpServers field and description of per-server rollup of
mcp__<server>__<tool> calls (i.e., only delete the issue tag, do not change
function name hotspots() or field name mcpServers).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf01fdf1-bcca-40c3-b3e0-4686d7c14cb0
📒 Files selected for processing (9)
CHANGELOG.mdcrates/relayburn-cli/src/commands/hotspots.rscrates/relayburn-sdk/src/analyze.rscrates/relayburn-sdk/src/analyze/hotspots.rscrates/relayburn-sdk/src/lib.rscrates/relayburn-sdk/src/query_verbs.rspackages/sdk-node/CHANGELOG.mdpackages/sdk-node/src/index.d.tstests/fixtures/cli-golden/snapshots/hotspots-json.stdout.txt
| - `burn hotspots`: new MCP-server rollup that collapses every | ||
| `mcp__<server>__<tool>` tool call into one row per server, so a chatty | ||
| MCP server (e.g. relaycast) shows up as a single line instead of 50+. | ||
| Surfaced in the human renderer (when non-empty) and as `mcpServers` | ||
| in `--json`. (#424) |
There was a problem hiding this comment.
Drop the PR/issue marker from the root Unreleased entry.
The impact text is good; just remove (#424) to match changelog policy.
Suggested edit
- `burn hotspots`: new MCP-server rollup that collapses every
`mcp__<server>__<tool>` tool call into one row per server, so a chatty
MCP server (e.g. relaycast) shows up as a single line instead of 50+.
Surfaced in the human renderer (when non-empty) and as `mcpServers`
- in `--json`. (`#424`)
+ in `--json`.As per coding guidelines, changelog entries should “Drop issue/PR links, internal review notes, implementation backstory…”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `burn hotspots`: new MCP-server rollup that collapses every | |
| `mcp__<server>__<tool>` tool call into one row per server, so a chatty | |
| MCP server (e.g. relaycast) shows up as a single line instead of 50+. | |
| Surfaced in the human renderer (when non-empty) and as `mcpServers` | |
| in `--json`. (#424) | |
| - `burn hotspots`: new MCP-server rollup that collapses every | |
| `mcp__<server>__<tool>` tool call into one row per server, so a chatty | |
| MCP server (e.g. relaycast) shows up as a single line instead of 50+. | |
| Surfaced in the human renderer (when non-empty) and as `mcpServers` | |
| in `--json`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 9 - 13, The Unreleased changelog entry ending with
"(`#424`)" should drop the PR/issue marker: edit the CHANGELOG.md entry for "burn
hotspots" (the Unreleased root) and remove the trailing " (`#424`)" so the line
ends after "`mcpServers` in `--json`." and no PR/issue/reference remains; keep
the rest of the text unchanged.
| - `hotspots()` attribution result carries a new `mcpServers` field — | ||
| per-server rollup of `mcp__<server>__<tool>` tool calls with summed | ||
| cost/tokens/ride-turns and top representative tool basenames. (#424) | ||
|
|
There was a problem hiding this comment.
Remove the issue reference from the Unreleased bullet.
Keep the entry impact-first, but drop the trailing (#424) per changelog style rules.
Suggested edit
-- `hotspots()` attribution result carries a new `mcpServers` field —
- per-server rollup of `mcp__<server>__<tool>` tool calls with summed
- cost/tokens/ride-turns and top representative tool basenames. (`#424`)
+- `hotspots()` attribution result carries a new `mcpServers` field —
+ per-server rollup of `mcp__<server>__<tool>` tool calls with summed
+ cost/tokens/ride-turns and top representative tool basenames.As per coding guidelines, changelog entries should “Drop issue/PR links, internal review notes, implementation backstory…”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `hotspots()` attribution result carries a new `mcpServers` field — | |
| per-server rollup of `mcp__<server>__<tool>` tool calls with summed | |
| cost/tokens/ride-turns and top representative tool basenames. (#424) | |
| - `hotspots()` attribution result carries a new `mcpServers` field — | |
| per-server rollup of `mcp__<server>__<tool>` tool calls with summed | |
| cost/tokens/ride-turns and top representative tool basenames. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/sdk-node/CHANGELOG.md` around lines 7 - 10, Update the Unreleased
changelog bullet in CHANGELOG.md by removing the trailing issue/PR reference
"(`#424`)" from the hotspots() entry; keep the text and impact-first phrasing
intact and ensure the entry still mentions the new mcpServers field and
description of per-server rollup of mcp__<server>__<tool> calls (i.e., only
delete the issue tag, do not change function name hotspots() or field name
mcpServers).
Summary
Closes #424. Adds a new MCP-server rollup to
burn hotspots. A chatty MCP server (relaycast with 50+ tools, the GitHub server, etc.) used to spread its cost across N per-tool rows where none individually looked expensive. The rollup now collapses everymcp__<server>__<tool>call into one row per server so you can ask "how much did relaycast cost me?" without manual filtering.relayburn-sdk): newMcpServerAggregationstruct +aggregate_by_mcp_server(&[ToolAttribution]) -> Vec<_>inanalyze/hotspots.rs. Skips any tool whose name doesn't split cleanly intomcp__<server>__<tool>. Sumscall_count,initial_tokens,persistence_tokens,riding_turns,total_cost. Carries up to three representative tool basenames (cost desc, then name asc). Sort bytotal_costdesc,serverasc as tiebreaker. Re-exported fromanalyze.rsandlib.rs.mcp_servers: Vec<McpServerAggregation>field, populated from the same attribution loop the existing file / bash / subagent rollups consume.relayburn-cli):burn hotspots --jsonaddsmcpServersaftersubagents; human renderer adds aTop MCP servers by costsection gated on a non-empty rollup.@relayburn/sdk): newHotspotsMcpServerRowinterface, added toHotspotsAttributionResult.mcpServers. The napi-rs serde-json bridge already promotescallCount/ridingTurnstoBigInt.packages/sdk-node.Test plan
aggregates_by_mcp_server_groups_by_server_segment_and_sorts_by_costcovers: two MCP servers + non-MCP tools + malformedmcp__names (empty server / empty tool / missing separator), verifies the non-MCP and malformed rows are skipped, costs sum, top_tools are cost-sorted, and server order is total_cost desc.cargo test --workspacegreen (672 tests).hotspots-json.stdout.txtadds"mcpServers": []since the in-tree fixture has no MCP tools).BURN_GOLDEN=1 cargo test -p relayburn-cli --test goldengreen.pnpm run testgreen (including@relayburn/sdkconformance test with napi build).packages/sdk-node/src/index.d.tsreflects the new field.Generated by Claude Code