feat: adds MCP metrics to OTEL - #5237
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughMCP execution now attaches missing tracers, classifies tool failures with sentinel errors, enriches MCP spans with standardized attributes, and exports MCP operation duration metrics with governance and error dimensions. ChangesMCP observability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPEntryPoint
participant MCPManager
participant MCPTool
participant OtelPlugin
participant MetricsExporter
MCPEntryPoint->>MCPManager: execute MCP request with tracer context
MCPManager->>MCPTool: run tool through plugin pipeline
MCPTool-->>MCPManager: result or classified error
MCPManager->>OtelPlugin: complete enriched MCP span
OtelPlugin->>MetricsExporter: record operation duration and attributes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
b54484c to
efa1e88
Compare
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "feat: adds MCP metrics to OTEL" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/bifrost.go (1)
2826-2831: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep operation-specific span state out of reusable contexts.
The standalone entrypoints can pass the shared
bifrost.ctx, while both MCP span starters overwrite its current span ID. Concurrent or subsequent calls can consequently receive stale or cross-request parent spans.
core/bifrost.go#L2826-L2831: create a request-scoped context instead of usingbifrost.ctxdirectly.core/bifrost.go#L2845-L2850: apply the same isolation to the Responses entrypoint.core/mcp/pluginpipeline.go#L88-L93: propagate the operation span through scoped state rather than permanently mutating the caller context.core/mcp/pluginpipeline.go#L302-L307: apply the same fix to connection spans.Based on learnings,
BifrostContext.SetValuemutates the shared context in place.🤖 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 `@core/bifrost.go` around lines 2826 - 2831, Isolate operation-specific MCP span state from reusable contexts. In core/bifrost.go lines 2826-2831, update ExecuteChatMCPTool to derive a request-scoped context instead of assigning bifrost.ctx directly; apply the same isolation to the Responses entrypoint at lines 2845-2850. In core/mcp/pluginpipeline.go lines 88-93 and 302-307, propagate span state through scoped context values without mutating the caller’s shared BifrostContext.Sources: Path instructions, Learnings
🧹 Nitpick comments (1)
core/mcp/toolmanager.go (1)
713-718: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPreserve the underlying
callErrin the error chain.Using
%vflattens the client error, so callers can classify the sentinel but cannot useerrors.Is/errors.Ason the original cause. Wrap both errors, for example with Go’s multiple%wsupport.As per coding guidelines, Go errors should be explicitly wrapped.
🤖 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 `@core/mcp/toolmanager.go` around lines 713 - 718, Update the MCP tool-call failure return in the surrounding tool execution method to preserve both callErr and ErrMCPToolCallFailed in the error chain by using multiple %w wrapping. Keep the existing timeout handling and error message context unchanged.Source: Coding guidelines
🤖 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 `@core/mcp/pluginpipeline.go`:
- Around line 316-322: Update the deferred span-finalization logic around
setMCPGovernanceSpanAttrs and tracer.EndSpan to track operation and post-hook
failures, ending failed connection spans with an error status and the relevant
error details instead of always using SpanStatusOk. Preserve successful
operations as SpanStatusOk and retain the existing nil guards.
- Around line 110-119: Update the tool argument and result attribute capture in
the response handling flow around GetToolArguments and the corresponding result
logic so content is not exported by default. Require an explicit opt-in before
setting these attributes, redact sensitive fields, and truncate or reject values
exceeding the configured size limit while preserving existing nil handling.
---
Outside diff comments:
In `@core/bifrost.go`:
- Around line 2826-2831: Isolate operation-specific MCP span state from reusable
contexts. In core/bifrost.go lines 2826-2831, update ExecuteChatMCPTool to
derive a request-scoped context instead of assigning bifrost.ctx directly; apply
the same isolation to the Responses entrypoint at lines 2845-2850. In
core/mcp/pluginpipeline.go lines 88-93 and 302-307, propagate span state through
scoped context values without mutating the caller’s shared BifrostContext.
---
Nitpick comments:
In `@core/mcp/toolmanager.go`:
- Around line 713-718: Update the MCP tool-call failure return in the
surrounding tool execution method to preserve both callErr and
ErrMCPToolCallFailed in the error chain by using multiple %w wrapping. Keep the
existing timeout handling and error message context unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ce9ae58e-b73e-4c8a-a658-0acf404a802e
📒 Files selected for processing (10)
core/bifrost.gocore/mcp/error_classify_test.gocore/mcp/pluginpipeline.gocore/mcp/toolmanager.gocore/schemas/bifrost.gocore/schemas/mcp.gocore/schemas/trace.goplugins/otel/main.goplugins/otel/mcp_metrics_test.goplugins/otel/metrics.go
efa1e88 to
66bfb1c
Compare
Merge activity
|
66bfb1c to
6e9c118
Compare
6e9c118 to
1f83507
Compare
## Summary MCP operations were missing OTel tracing spans and the `mcp.client.operation.duration` metric because the tracer was never stamped onto request-scoped contexts for standalone MCP tool executions, and the plugin pipeline emitted a single flat span rather than following the nested span layout used by the LLM path. ## Changes - Added `ensureMCPTracerContext` to stamp the tracer onto the `BifrostContext` for `ExecuteChatMCPTool` and `ExecuteResponsesMCPTool` calls, which bypass the chat/responses flow that normally sets it. - Restructured span creation in `RunWithPluginPipeline` and `runConnectWithPluginPipeline` to match the LLM path: PreHook spans chain under the inbound HTTP span, the op span is created after PreHooks complete (nesting under the last one), and PostHook spans nest under the op span. Short-circuited requests produce no op span. - Added `SpanKindMCPTool` for `tools/call` spans, distinct from `SpanKindMCPClient` used for lifecycle ops (ping, list_tools, connect). - Added `mcp.method.name` (`tools/call`, `tools/list`, `ping`) and `network.transport` (`pipe` for stdio, `tcp` for HTTP/SSE) OTel semconv attributes on MCP spans. - Added `bifrost.mcp.tool.duration_ms` to carry the raw `CallTool` wire latency so the duration metric measures the wire round-trip rather than span wall-time (which includes PostHooks). - Added `error.type` classification on failed MCP spans via sentinel errors (`ErrMCPToolTimeout`, `ErrMCPToolCallFailed`) wrapped with `%w` at the `CallTool` site, classified by `mcpErrorType` via `errors.Is`. - Added `setMCPGovernanceSpanAttrs` to copy governance identity (`bifrost.virtual_key_id`, `team_id`, etc.) from context onto MCP spans for per-tenant metric breakdowns. - Added `mcp.client.operation.duration` histogram in the OTel metrics exporter with semconv-recommended bucket boundaries. `recordMCPMetricsFromTrace` emits one sample per MCP span, preferring wire latency over wall-time and appending `error.type` on failures. - Preserved the original `error` value on `ErrorField.Error` (json:`"-"`) when wrapping `opErr` as `BifrostError` so `errors.Is` chains work through PostHook boundaries. - Added `OTelMethodName()` on `MCPRequestType` and `OTelNetworkTransport()` on `MCPConnectionType` for semconv mapping. - Added `connectionTypeForClientName` to resolve a client's transport by name for the `network.transport` attribute. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/mcp/... ./plugins/otel/... ``` `TestMCPErrorTypeClassification` validates sentinel-based `error.type` classification including `auth_required`, `timeout`, `tool_error`, and the `_OTHER` catch-all. `TestRecordMCPMetricsFromTraceRecordsBothKinds` validates that `mcp.client.operation.duration` emits one data point per enriched MCP span (`SpanKindMCPTool` and `SpanKindMCPClient`), uses wire latency over wall-time when available, tags `error.type` on failures, and skips un-enriched and non-MCP spans. `TestBuildMCPSpanAttrsSemconvAndGovernance` validates that semconv dimensions and flat-named governance labels are present and that absent optional dimensions are omitted. `TestMCPRequestTypeOTelMethodName` and `TestMCPConnectionTypeOTelNetworkTransport` pin the semconv mapping tables. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. Governance identity values copied to span attributes are already present in the request context and are not new data exposures. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary MCP operations were missing OTel tracing spans and the `mcp.client.operation.duration` metric because the tracer was never stamped onto request-scoped contexts for standalone MCP tool executions, and the plugin pipeline emitted a single flat span rather than following the nested span layout used by the LLM path. ## Changes - Added `ensureMCPTracerContext` to stamp the tracer onto the `BifrostContext` for `ExecuteChatMCPTool` and `ExecuteResponsesMCPTool` calls, which bypass the chat/responses flow that normally sets it. - Restructured span creation in `RunWithPluginPipeline` and `runConnectWithPluginPipeline` to match the LLM path: PreHook spans chain under the inbound HTTP span, the op span is created after PreHooks complete (nesting under the last one), and PostHook spans nest under the op span. Short-circuited requests produce no op span. - Added `SpanKindMCPTool` for `tools/call` spans, distinct from `SpanKindMCPClient` used for lifecycle ops (ping, list_tools, connect). - Added `mcp.method.name` (`tools/call`, `tools/list`, `ping`) and `network.transport` (`pipe` for stdio, `tcp` for HTTP/SSE) OTel semconv attributes on MCP spans. - Added `bifrost.mcp.tool.duration_ms` to carry the raw `CallTool` wire latency so the duration metric measures the wire round-trip rather than span wall-time (which includes PostHooks). - Added `error.type` classification on failed MCP spans via sentinel errors (`ErrMCPToolTimeout`, `ErrMCPToolCallFailed`) wrapped with `%w` at the `CallTool` site, classified by `mcpErrorType` via `errors.Is`. - Added `setMCPGovernanceSpanAttrs` to copy governance identity (`bifrost.virtual_key_id`, `team_id`, etc.) from context onto MCP spans for per-tenant metric breakdowns. - Added `mcp.client.operation.duration` histogram in the OTel metrics exporter with semconv-recommended bucket boundaries. `recordMCPMetricsFromTrace` emits one sample per MCP span, preferring wire latency over wall-time and appending `error.type` on failures. - Preserved the original `error` value on `ErrorField.Error` (json:`"-"`) when wrapping `opErr` as `BifrostError` so `errors.Is` chains work through PostHook boundaries. - Added `OTelMethodName()` on `MCPRequestType` and `OTelNetworkTransport()` on `MCPConnectionType` for semconv mapping. - Added `connectionTypeForClientName` to resolve a client's transport by name for the `network.transport` attribute. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/mcp/... ./plugins/otel/... ``` `TestMCPErrorTypeClassification` validates sentinel-based `error.type` classification including `auth_required`, `timeout`, `tool_error`, and the `_OTHER` catch-all. `TestRecordMCPMetricsFromTraceRecordsBothKinds` validates that `mcp.client.operation.duration` emits one data point per enriched MCP span (`SpanKindMCPTool` and `SpanKindMCPClient`), uses wire latency over wall-time when available, tags `error.type` on failures, and skips un-enriched and non-MCP spans. `TestBuildMCPSpanAttrsSemconvAndGovernance` validates that semconv dimensions and flat-named governance labels are present and that absent optional dimensions are omitted. `TestMCPRequestTypeOTelMethodName` and `TestMCPConnectionTypeOTelNetworkTransport` pin the semconv mapping tables. ## Breaking changes - [ ] Yes - [x] No ## Related issues ## Security considerations None. Governance identity values copied to span attributes are already present in the request context and are not new data exposures. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
MCP operations were missing OTel tracing spans and the
mcp.client.operation.durationmetric because the tracer was never stamped onto request-scoped contexts for standalone MCP tool executions, and the plugin pipeline emitted a single flat span rather than following the nested span layout used by the LLM path.Changes
ensureMCPTracerContextto stamp the tracer onto theBifrostContextforExecuteChatMCPToolandExecuteResponsesMCPToolcalls, which bypass the chat/responses flow that normally sets it.RunWithPluginPipelineandrunConnectWithPluginPipelineto match the LLM path: PreHook spans chain under the inbound HTTP span, the op span is created after PreHooks complete (nesting under the last one), and PostHook spans nest under the op span. Short-circuited requests produce no op span.SpanKindMCPToolfortools/callspans, distinct fromSpanKindMCPClientused for lifecycle ops (ping, list_tools, connect).mcp.method.name(tools/call,tools/list,ping) andnetwork.transport(pipefor stdio,tcpfor HTTP/SSE) OTel semconv attributes on MCP spans.bifrost.mcp.tool.duration_msto carry the rawCallToolwire latency so the duration metric measures the wire round-trip rather than span wall-time (which includes PostHooks).error.typeclassification on failed MCP spans via sentinel errors (ErrMCPToolTimeout,ErrMCPToolCallFailed) wrapped with%wat theCallToolsite, classified bymcpErrorTypeviaerrors.Is.setMCPGovernanceSpanAttrsto copy governance identity (bifrost.virtual_key_id,team_id, etc.) from context onto MCP spans for per-tenant metric breakdowns.mcp.client.operation.durationhistogram in the OTel metrics exporter with semconv-recommended bucket boundaries.recordMCPMetricsFromTraceemits one sample per MCP span, preferring wire latency over wall-time and appendingerror.typeon failures.errorvalue onErrorField.Error(json:"-") when wrappingopErrasBifrostErrorsoerrors.Ischains work through PostHook boundaries.OTelMethodName()onMCPRequestTypeandOTelNetworkTransport()onMCPConnectionTypefor semconv mapping.connectionTypeForClientNameto resolve a client's transport by name for thenetwork.transportattribute.Type of change
Affected areas
How to test
go test ./core/mcp/... ./plugins/otel/...TestMCPErrorTypeClassificationvalidates sentinel-basederror.typeclassification includingauth_required,timeout,tool_error, and the_OTHERcatch-all.TestRecordMCPMetricsFromTraceRecordsBothKindsvalidates thatmcp.client.operation.durationemits one data point per enriched MCP span (SpanKindMCPToolandSpanKindMCPClient), uses wire latency over wall-time when available, tagserror.typeon failures, and skips un-enriched and non-MCP spans.TestBuildMCPSpanAttrsSemconvAndGovernancevalidates that semconv dimensions and flat-named governance labels are present and that absent optional dimensions are omitted.TestMCPRequestTypeOTelMethodNameandTestMCPConnectionTypeOTelNetworkTransportpin the semconv mapping tables.Breaking changes
Related issues
Security considerations
None. Governance identity values copied to span attributes are already present in the request context and are not new data exposures.
Checklist
docs/contributing/README.mdand followed the guidelines