add logging-level support #1123
Conversation
core/Azure.Mcp.Core/src/Areas/Server/Options/ServiceOptionDefinitions.cs
Show resolved
Hide resolved
xiangyan99
requested review from
JasonYeMSFT,
Copilot,
fanyang-mono,
jairmyree,
jongio,
joshfree,
sandeep-sen and
wbreza
There was a problem hiding this comment.
Pull Request Overview
This PR adds logging customization capabilities to the azmcp server start command, enabling users to control log verbosity and output destinations.
Key Changes:
- Added
--log-leveloption to set minimum logging level (Trace, Debug, Information, Warning, Error, Critical, None) - Added
--log-file-pathoption to write logs to a specified file with automatic directory creation - Implemented Azure SDK EventSource log forwarding to capture Azure SDK diagnostic events
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
servers/Azure.Mcp.Server/docs/azmcp-commands.md |
Documents the new --log-level and --log-file-path options with security warnings |
servers/Azure.Mcp.Server/CHANGELOG.md |
Records the new logging customization features |
core/Azure.Mcp.Core/src/Areas/Server/Options/ServiceStartOptions.cs |
Adds LogLevel and LogFilePath properties to configuration options |
core/Azure.Mcp.Core/src/Areas/Server/Options/ServiceOptionDefinitions.cs |
Defines CLI option definitions for log level and log file path |
core/Azure.Mcp.Core/src/Areas/Server/Commands/ServiceStartCommand.cs |
Implements logging configuration logic and integrates new options |
core/Azure.Mcp.Core/src/Areas/Server/Services/SimpleFileLoggerProvider.cs |
Implements a simple file logger provider with thread-safe file writing |
core/Azure.Mcp.Core/src/Logging/AzureSdkEventSourceLogForwarder.cs |
Forwards Azure SDK EventSource events to logging infrastructure |
core/Azure.Mcp.Core/src/Logging/AzureSdkLogForwarderHostedService.cs |
Hosted service to manage the lifecycle of the Azure SDK log forwarder |
core/Azure.Mcp.Core/src/Extensions/OpenTelemetryExtensions.cs |
Integrates Azure SDK logging with configurable event levels |
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/SimpleFileLoggerProviderTests.cs |
Comprehensive tests for the file logger provider |
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/ServiceStartCommandTests.cs |
Tests for parsing and binding the new logging options |
| { | ||
| Directory.CreateDirectory(directory); | ||
| } | ||
| File.AppendAllText(_filePath, logEntry + Environment.NewLine); |
There was a problem hiding this comment.
Using File.AppendAllText for each log entry causes frequent file open/close operations, which is inefficient for high-frequency logging. Consider using a StreamWriter with auto-flush or implement buffering for better performance.
There was a problem hiding this comment.
@xiangyan99, this is probably not a bad idea and can be implemented in a way where the provider owns the stream for its lifetime and the stream is opened in a way where deletion of the file is prevented. That way we don't need to check directory existence and open a new file handle every time we're trying to log.
There was a problem hiding this comment.
Additionally, it might be good to do the I/O on a background thread. Have the Log method write to a buffer, and use a BackgroundService to write to that file as the buffer fills.
.../Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/SimpleFileLoggerProviderTests.cs
Show resolved
Hide resolved
.../Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/SimpleFileLoggerProviderTests.cs
Show resolved
Hide resolved
.../Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/SimpleFileLoggerProviderTests.cs
Show resolved
Hide resolved
.../Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/SimpleFileLoggerProviderTests.cs
Show resolved
Hide resolved
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/ServiceStartCommandTests.cs
Show resolved
Hide resolved
|
PR changes look good to me. We should definitely add docs for how to access logging for the customers. Especially with scenarios - What happens when only one of the params is set:
|
| // Get or create a logger for this event source | ||
| if (!_loggers.TryGetValue(eventData.EventSource.Name, out var logger)) | ||
| { | ||
| logger = _loggerFactory.CreateLogger($"Azure.SDK.{eventData.EventSource.Name}"); |
There was a problem hiding this comment.
Do we need to worry about concurrent execution of this method?
There was a problem hiding this comment.
In the file logging case, it is thread-safe.
| DefaultValueFactory = _ => Options.OutgoingAuthStrategy.NotSet | ||
| }; | ||
|
|
||
| public static readonly Option<string?> LogLevel = new( |
There was a problem hiding this comment.
Can we avoid creating a new argument for log levels? Controlling log levels with configurations is built into the ILogger infrastructure with log categories and log levels. These can both be controlled using any number of sources of configurations, including command line and environment variables.
| // If LogLevel.None is specified, clear all providers to disable logging | ||
| if (logLevel == LogLevel.None) | ||
| { | ||
| logging.ClearProviders(); |
There was a problem hiding this comment.
@xiangyan99, please chat with @srnagar on the changes that he's making for logging. We're trying to improve the situation for remote MCP servers, and Srikanta's changes similarly have a switch to shut off all telemetry.
What does this PR do?
[Provide a clear, concise description of the changes]azmcp server startcommand:--log-leveloption to set the minimum logging level (Trace, Debug, Information, Warning, Error, Critical, None)--log-file-pathoption to write logs to a specified file with automatic directory creation and thread-safe file handlingUser can use
azmcp server start --log-level Debug --log-file-path c:\logs\azmcp.logto enable debug logging
[Any additional context, screenshots, or information that helps reviewers]GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.md.\eng\scripts\Update-AzCommandsMetadata.ps1to update tool metadata in azmcp-commands.md (required for CI)ToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test prompts/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline