feat(mcp): add deeper OpenTelemetry integration for MCP tool calls#3493
Conversation
Each MCP tool call now appears as its own transaction in Elastic
Observability instead of collapsing under POST /docs/_mcp/.
- McpSpanRenameProcessor renames the ASP.NET Core server span to
"tools/call {tool}" in OnEnd, after the route-based name is set
- mcp.method.name and mcp.tool.name semconv tags added to server span
- Meter added with mcp.tool.calls counter and mcp.tool.duration
histogram, both dimensioned by tool name, method, profile, outcome
- OTel registration consolidated into AddDocumentationOpenTelemetry
(replaces Extensions.cs and EuidEnrichmentExtensions.cs)
- Aspire bumped to 13.4.3; fixes ExecuteCommandContext.Arguments
required member in integration test bootstrap
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR consolidates OpenTelemetry configuration across the documentation platform by replacing separate hardcoded extension methods with a parameterizable Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/Elastic.Documentation.Api/OpenTelemetry/OpenTelemetryExtensions.cs (1)
25-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHealth-check traffic is still being traced.
Program.csmaps health checks on/healthand/alive, but this filter only excludes/docs/_api/v1. That means probe traffic still lands in tracing and skews the API telemetry. Filter the actual health endpoints instead.🤖 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 `@src/api/Elastic.Documentation.Api/OpenTelemetry/OpenTelemetryExtensions.cs` around lines 25 - 30, The current aspNetCoreOptions.Filter in OpenTelemetryExtensions.cs only excludes "/docs/_api/v1" so health-check probes mapped in Program.cs (endpoints "/health" and "/alive") are still traced; update the filter lambda (aspNetCoreOptions.Filter) to inspect httpContext.Request.Path (or use httpContext.Request.Path.StartsWithSegments) and return false for "/health" and "/alive" in addition to "/docs/_api/v1" so those probe endpoints are not included in traces (adjust the path comparison to handle nulls and optional trailing slashes).
🧹 Nitpick comments (1)
src/tooling/docs-builder/Program.cs (1)
23-23: 💤 Low valueUnused variable
argh.The result of
GlobalCliOptions.TryParseArghis captured but never used. If the boolean return value isn't needed, consider discarding it with_ = GlobalCliOptions.TryParseArgh(...).-var argh = GlobalCliOptions.TryParseArgh(args, out var cliOptions); +_ = GlobalCliOptions.TryParseArgh(args, out var cliOptions);🤖 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 `@src/tooling/docs-builder/Program.cs` at line 23, The local variable `argh` is unused after calling GlobalCliOptions.TryParseArgh; replace its capture with a discard to avoid the unused-variable warning by calling GlobalCliOptions.TryParseArgh(args, out var cliOptions) and discarding the boolean result (use `_ = GlobalCliOptions.TryParseArgh(...)`) so only `cliOptions` remains available for subsequent code.
🤖 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 `@aspire/Properties/launchSettings.json`:
- Around line 10-11: In launchSettings.json's https profile update the
mismatched environment variables so they are consistent: change
"ASPNETCORE_ENVIRONMENT": "dev" to "ASPNETCORE_ENVIRONMENT": "Development" (or
alternatively make DOTNET_ENVIRONMENT "dev") so both ASPNETCORE_ENVIRONMENT and
DOTNET_ENVIRONMENT match; ensure the https profile in launchSettings.json is the
one being edited.
In `@src/api/Elastic.Documentation.Api/Program.cs`:
- Around line 17-18: Replace reading
Environment.GetEnvironmentVariable("ENVIRONMENT") with the host builder's
environment name: after calling CreateSlimBuilder(args) read
builder.Environment.EnvironmentName and pass that value into
AddElasticDocsApiServices instead of the raw env var; update the variable used
where AddElasticDocsApiServices(environment) is invoked (and the other
occurrence at the second spot) so they receive
builder.Environment.EnvironmentName to ensure the service sees the actual host
environment.
---
Outside diff comments:
In `@src/api/Elastic.Documentation.Api/OpenTelemetry/OpenTelemetryExtensions.cs`:
- Around line 25-30: The current aspNetCoreOptions.Filter in
OpenTelemetryExtensions.cs only excludes "/docs/_api/v1" so health-check probes
mapped in Program.cs (endpoints "/health" and "/alive") are still traced; update
the filter lambda (aspNetCoreOptions.Filter) to inspect httpContext.Request.Path
(or use httpContext.Request.Path.StartsWithSegments) and return false for
"/health" and "/alive" in addition to "/docs/_api/v1" so those probe endpoints
are not included in traces (adjust the path comparison to handle nulls and
optional trailing slashes).
---
Nitpick comments:
In `@src/tooling/docs-builder/Program.cs`:
- Line 23: The local variable `argh` is unused after calling
GlobalCliOptions.TryParseArgh; replace its capture with a discard to avoid the
unused-variable warning by calling GlobalCliOptions.TryParseArgh(args, out var
cliOptions) and discarding the boolean result (use `_ =
GlobalCliOptions.TryParseArgh(...)`) so only `cliOptions` remains available for
subsequent code.
🪄 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: Enterprise
Run ID: 70bdb370-6073-44c5-a254-35c6142e172d
📒 Files selected for processing (17)
Directory.Packages.propsaspire/Properties/launchSettings.jsonsrc/Elastic.Documentation.ServiceDefaults/AppDefaultsExtensions.cssrc/Elastic.Documentation.ServiceDefaults/Extensions.cssrc/Elastic.Documentation.ServiceDefaults/Telemetry/EuidEnrichmentExtensions.cssrc/Elastic.Documentation.ServiceDefaults/Telemetry/OpenTelemetryRegistrationExtensions.cssrc/Elastic.Documentation.ServiceDefaults/Telemetry/VersionHelper.cssrc/api/Elastic.Documentation.Api/OpenTelemetry/OpenTelemetryExtensions.cssrc/api/Elastic.Documentation.Api/Program.cssrc/api/Elastic.Documentation.Mcp.Remote/Program.cssrc/api/Elastic.Documentation.Mcp.Remote/SseKeepAliveMiddleware.cssrc/api/Elastic.Documentation.Mcp.Remote/Telemetry/McpSpanRenameProcessor.cssrc/api/Elastic.Documentation.Mcp.Remote/Telemetry/McpToolTelemetry.cssrc/tooling/docs-builder/DocumentationTooling.cssrc/tooling/docs-builder/Program.cstests-integration/Elastic.Documentation.IntegrationTests/Search/SearchBootstrapFixture.cstests/Elastic.Documentation.Api.Tests/Fixtures/ApiWebApplicationFactory.cs
💤 Files with no reviewable changes (3)
- src/Elastic.Documentation.ServiceDefaults/Telemetry/EuidEnrichmentExtensions.cs
- src/Elastic.Documentation.ServiceDefaults/Extensions.cs
- src/tooling/docs-builder/DocumentationTooling.cs
|
So necessary, thanks! |
Why
MCP tool calls were invisible in Elastic Observability — every call collapsed into a single
POST /docs/_mcp/transaction because all tools share one HTTP route. There was also no metrics data for tool calls at all.What
Each tool call now appears as its own named transaction (e.g.
tools/call search_docs) in APM, and tool-level metrics are exported.Tracing:
McpSpanRenameProcessorrenames the ASP.NET Core server span inOnEnd— after the route-based name is set — totools/call {tool}. The standard OTel MCP semconv attributesmcp.method.nameandmcp.tool.nameare written to the server span byMcpToolTelemetry.StartActivityso the processor can read them.Metrics: A
Meter(Elastic.Documentation.Api.McpTools) was added with two instruments —mcp.tool.calls(counter) andmcp.tool.duration(histogram) — both dimensioned by tool name, method, server profile, and outcome. They're recorded inLogCompletion, which all three tool classes already call in theirfinallyblocks, so no per-tool changes were needed.OTel registration cleanup:
Extensions.csandEuidEnrichmentExtensions.cswere replaced by a unifiedAddDocumentationOpenTelemetryextension that all three services (docs-api, docs-mcp, docs-builder CLI) now use consistently.Aspire 13.4.3 bump: Fixes a new
requiredmember (ExecuteCommandContext.Arguments) in the integration test bootstrap.