Adopt telemetry and observability features from MCP SDK v0.6#327
Adopt telemetry and observability features from MCP SDK v0.6#327jongalloway merged 7 commits intomainfrom
Conversation
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive telemetry and observability documentation and enables the telemetry capability flag to leverage the MCP C# SDK v0.6.0-preview.1's built-in telemetry features. The changes primarily consist of documentation with minimal code modifications to enable the telemetry flag.
Changes:
- Created comprehensive telemetry documentation covering SDK telemetry, structured logging, and OpenTelemetry integration
- Enabled the
ServerCapabilities.Telemetryflag and updated related tests - Added example configuration files for logging and OpenTelemetry settings
- Updated performance baseline documentation with telemetry monitoring guidance
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/telemetry.md | New comprehensive telemetry documentation covering SDK v0.6 features, configuration, and best practices |
| doc/performance-baseline.md | Added telemetry-based performance monitoring section with request duration logging examples |
| README.md | Added telemetry documentation link to documentation section |
| appsettings.json | Default logging configuration with disabled OpenTelemetry placeholders for development |
| appsettings.Development.json | Debug-level logging configuration for local development |
| .gitignore | Added exclusions for production and local configuration files |
| DotNetMcp/Tools/Cli/DotNetCliTools.Misc.cs | Changed Telemetry flag from false to true with documentation comment |
| DotNetMcp/Server/ServerCapabilities.cs | Enhanced XML documentation for Telemetry property |
| DotNetMcp.Tests/Server/ServerCapabilitiesTests.cs | Updated test assertions to expect telemetry enabled |
| } | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
The documentation mentions using appsettings.json without clarifying the deployment context. The appsettings files at the repository root (appsettings.json, appsettings.Development.json) are not packaged with the NuGet tool (no CopyToOutputDirectory or Pack directive in the .csproj), so they're only available for local development of dotnet-mcp itself.
For end users who install via dnx Community.Mcp.DotNet, these files won't exist. Consider adding clarification:
- Note that the repository's appsettings files are for development only
- Recommend environment variables as the primary configuration method for end users (since they work regardless of installation method)
- If mentioning appsettings.json for end users, clarify they would need to create it in their working directory
For example, add a note after line 82:
"Note: The appsettings files in this repository are for local development. When using the installed tool (via dnx), environment variables are recommended for configuration since they work regardless of installation method. If you prefer configuration files, create appsettings.json in your working directory."
| > Note: The `appsettings.json` files in this repository are for local development of dotnet-mcp and are not packaged with the `Community.Mcp.DotNet` tool. When using the installed tool (for example via `dnx`), environment variables are the recommended way to configure logging and other settings, since they work regardless of installation method. If you prefer configuration files, create an `appsettings.json` in your working directory with the desired settings. |
There was a problem hiding this comment.
Added clarification note after line 82 explaining that appsettings files are for development only and recommending environment variables for end users. Fixed in dc0f311.
| .AddSource("DotNetMcp.*") | ||
| .AddSource("ModelContextProtocol.*"); |
There was a problem hiding this comment.
The AddSource method doesn't support wildcard patterns. The code shows .AddSource("DotNetMcp.*") and .AddSource("ModelContextProtocol.*"), but OpenTelemetry's AddSource expects exact ActivitySource names, not wildcard patterns like logging categories.
Unless the codebase creates ActivitySources with these exact literal names (including the ".*"), this configuration won't work as intended. If you want to capture all activities from these namespaces, you would need to explicitly add each ActivitySource name:
tracing
.AddSource("DotNetMcp")
.AddSource("ModelContextProtocol")Or if there are multiple sources, list them explicitly. Wildcard support is not a standard feature of OpenTelemetry's AddSource method.
| .AddSource("DotNetMcp.*") | |
| .AddSource("ModelContextProtocol.*"); | |
| .AddSource("DotNetMcp") | |
| .AddSource("ModelContextProtocol"); |
There was a problem hiding this comment.
Fixed - removed wildcard patterns from AddSource. Now using exact names "DotNetMcp" and "ModelContextProtocol". Fixed in dc0f311.
| .AddMeter("DotNetMcp.*") | ||
| .AddMeter("ModelContextProtocol.*"); |
There was a problem hiding this comment.
Same issue with AddMeter - wildcard patterns are not supported. The code shows .AddMeter("DotNetMcp.*") and .AddMeter("ModelContextProtocol.*"), but AddMeter expects exact meter names.
Change to:
metrics
.AddMeter("DotNetMcp")
.AddMeter("ModelContextProtocol")| .AddMeter("DotNetMcp.*") | |
| .AddMeter("ModelContextProtocol.*"); | |
| .AddMeter("DotNetMcp") | |
| .AddMeter("ModelContextProtocol"); |
There was a problem hiding this comment.
Fixed - removed wildcard patterns from AddMeter. Now using exact names "DotNetMcp" and "ModelContextProtocol". Fixed in dc0f311.
| ``` | ||
| info: ModelContextProtocol.Server.McpServer[LogRequestHandlerCompleted] | ||
| Request handler completed: resources/read (dotnet://info) in 45ms (cached) | ||
| ``` |
There was a problem hiding this comment.
The example log message shows "(cached)" as part of the output, but it's unclear if the MCP SDK v0.6 actually includes caching information in the log messages. The SDK provides request duration logging, but cache status may not be automatically included.
Verify that the SDK actually logs cache status in this format. If it doesn't, either remove "(cached)" from the example or clarify that this would require custom logging in the resource implementation.
There was a problem hiding this comment.
Updated example to remove "(cached)" and added a note clarifying that the SDK logs duration automatically but cache status would require custom logging. Fixed in dc0f311.
| var consoleExporter = openTelemetryConfig.GetValue<bool>("Traces:ConsoleExporter"); | ||
| if (consoleExporter) | ||
| tracing.AddConsoleExporter(); | ||
|
|
||
| var otlpEnabled = openTelemetryConfig.GetValue<bool>("Traces:OtlpExporter:Enabled"); | ||
| if (otlpEnabled) | ||
| { | ||
| var endpoint = openTelemetryConfig.GetValue<string>("Traces:OtlpExporter:Endpoint"); |
There was a problem hiding this comment.
Inconsistent configuration access pattern. The code mixes two different approaches:
- Line 163: Uses colon notation with
GetValue<bool>("Traces:ConsoleExporter") - Line 151: Uses
GetSection("Traces").GetValue<bool>("Enabled")
While both work, mixing patterns in the same method reduces code maintainability. For consistency and clarity, use the section-based approach throughout:
var tracesSection = openTelemetryConfig.GetSection("Traces");
var consoleExporter = tracesSection.GetValue<bool>("ConsoleExporter");
var otlpSection = tracesSection.GetSection("OtlpExporter");
var otlpEnabled = otlpSection.GetValue<bool>("Enabled");The same issue occurs in the metrics configuration section (lines 191, 195, 198).
There was a problem hiding this comment.
Fixed - now using consistent section-based configuration access throughout. Changed to use GetSection pattern for all configuration reads in both traces and metrics sections. Fixed in dc0f311.
| var consoleExporter = openTelemetryConfig.GetValue<bool>("Metrics:ConsoleExporter"); | ||
| if (consoleExporter) | ||
| metrics.AddConsoleExporter(); | ||
|
|
||
| var otlpEnabled = openTelemetryConfig.GetValue<bool>("Metrics:OtlpExporter:Enabled"); | ||
| if (otlpEnabled) | ||
| { | ||
| var endpoint = openTelemetryConfig.GetValue<string>("Metrics:OtlpExporter:Endpoint"); |
There was a problem hiding this comment.
Inconsistent configuration access pattern in the metrics section. Same issue as the tracing section - mixing colon notation (GetValue<bool>("Metrics:ConsoleExporter")) with section-based access (GetSection("Metrics").GetValue<bool>("Enabled")).
For consistency, use the section-based approach throughout.
There was a problem hiding this comment.
Fixed - now using consistent section-based configuration access in the metrics section matching the traces section. Fixed in dc0f311.
|
@copilot review and address comments |
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
| tracing | ||
| .AddSource("DotNetMcp") | ||
| .AddSource("ModelContextProtocol"); |
There was a problem hiding this comment.
The OpenTelemetry integration code references ActivitySource names "DotNetMcp" and "ModelContextProtocol" that don't exist in the codebase. The AddSource method subscribes to activities from these named sources, but no ActivitySource instances with these names have been created.
This code example won't capture any traces without implementing the ActivitySources first. Line 335 in the "Future Enhancements" section acknowledges this: "Custom ActivitySource for tool execution spans" is listed as a planned feature.
Consider either:
- Removing the .AddSource() calls from the example and adding a comment explaining that ActivitySources would need to be implemented first
- Adding a clear note before the code example explaining that this requires implementing ActivitySources in the application code
- Moving this to the "Future Enhancements" section as example code for when ActivitySources are implemented
| tracing | |
| .AddSource("DotNetMcp") | |
| .AddSource("ModelContextProtocol"); | |
| // NOTE: To capture custom spans from your application, you must first create corresponding | |
| // ActivitySource instances (e.g., new ActivitySource("DotNetMcp")) in your code and then | |
| // register them here. For example: | |
| // | |
| // tracing | |
| // .AddSource("DotNetMcp") | |
| // .AddSource("ModelContextProtocol"); |
There was a problem hiding this comment.
Added clear NOTE comments explaining that ActivitySource instances need to be implemented first. The AddSource calls are now commented out with explanation that they're for future use when ActivitySources are implemented (referenced in Future Enhancements section). Fixed in 0a2f6c6.
| metrics | ||
| .AddMeter("DotNetMcp") | ||
| .AddMeter("ModelContextProtocol"); |
There was a problem hiding this comment.
The OpenTelemetry integration code references Meter names "DotNetMcp" and "ModelContextProtocol" that don't exist in the codebase. The AddMeter method subscribes to metrics from these named meters, but no Meter instances with these names have been created.
This code example won't collect any metrics without implementing the Meters first. Line 336 in the "Future Enhancements" section acknowledges this: "Metrics for cache hit/miss rates" is listed as a planned feature.
Consider either:
- Removing the .AddMeter() calls from the example and adding a comment explaining that Meters would need to be implemented first
- Adding a clear note before the code example explaining that this requires implementing Meters in the application code
- Moving this to the "Future Enhancements" section as example code for when Meters are implemented
| metrics | |
| .AddMeter("DotNetMcp") | |
| .AddMeter("ModelContextProtocol"); | |
| // Register custom OpenTelemetry meters here once they are implemented, for example: | |
| // metrics.AddMeter("DotNetMcp").AddMeter("ModelContextProtocol"); |
There was a problem hiding this comment.
Added clear NOTE comments explaining that Meter instances need to be implemented first. The AddMeter calls are now commented out with explanation that they're for future use when Meters are implemented (referenced in Future Enhancements section). Fixed in 0a2f6c6.
|
@copilot review and address comments |
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
Telemetry and Observability Enhancement - COMPLETE ✅
Summary
Successfully adopted telemetry and observability features from MCP C# SDK v0.6.0-preview.1.
Recent Changes (Addressing PR Review Round 2)
Previous Changes (PR Review Round 1)
What Was Done
1. Documentation (Primary Deliverable) ✅
Created doc/telemetry.md - comprehensive guide covering:
Updated existing documentation:
2. Configuration Files ✅
3. Code Changes (Minimal) ✅
falsetotrueSDK v0.6 Features Documented
The MCP C# SDK v0.6.0-preview.1 provides (automatically):
Validation ✅
Benefits
✓ Better visibility into tool performance
✓ Standardized metrics compatible with OpenTelemetry ecosystem
✓ Easier debugging of slow/failing operations
✓ Foundation for future observability enhancements
✓ Clear user guidance on what's available now vs future enhancements
Security Summary
✅ No vulnerabilities discovered
✅ Configuration files properly secured (.gitignore)
✅ Privacy considerations documented
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.