Add DangerouslyWriteSupportLogsToDir support #1305
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
srnagar
reviewed
Dec 10, 2025
| command.Options.Add(ServiceOptionDefinitions.DangerouslyDisableHttpIncomingAuth); | ||
| command.Options.Add(ServiceOptionDefinitions.InsecureDisableElicitation); | ||
| command.Options.Add(ServiceOptionDefinitions.OutgoingAuthStrategy); | ||
| command.Options.Add(ServiceOptionDefinitions.DangerouslyEnableSupportLoggingToFolder); |
Member
There was a problem hiding this comment.
Usually Enable indicates a boolean. Since this takes a directory path, we should consider an alternate name.
Some suggestions:
--dangerously-write-support-logs-to-dir--unsafe-support-log-dir(also used in Azure Identity)--dangerously-log-to-dir
If we don't have to use "dangerously" prefix, then I am inclined to use the 2nd option.
| { | ||
| string? folderPath = commandResult.GetValueOrDefault<string?>(ServiceOptionDefinitions.DangerouslyEnableSupportLoggingToFolder.Name); | ||
|
|
||
| if (string.IsNullOrEmpty(folderPath)) |
Member
There was a problem hiding this comment.
Should we check for only null here? Empty string is invalid.
| } | ||
|
|
||
| // Validate the folder path is not empty or whitespace | ||
| if (string.IsNullOrWhiteSpace(folderPath)) |
Member
There was a problem hiding this comment.
Should we validate that the folder path is actually valid and not just check for whitespace?
| /// <param name="options">The server configuration options.</param> | ||
| private static void ConfigureSupportLogging(ILoggingBuilder logging, ServiceStartOptions options) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(options.SupportLoggingFolder)) |
Member
There was a problem hiding this comment.
Why is this check required as the validation is already done to ensure it is not null or empty?
Comment on lines
239
to
250
| logging.AddConsole(consoleOptions => | ||
| { | ||
| consoleOptions.LogToStandardErrorThreshold = LogLevel.Debug; | ||
| consoleOptions.FormatterName = Microsoft.Extensions.Logging.Console.ConsoleFormatterNames.Simple; | ||
| }); | ||
| logging.AddSimpleConsole(simple => | ||
| { | ||
| simple.ColorBehavior = Microsoft.Extensions.Logging.Console.LoggerColorBehavior.Disabled; | ||
| simple.IncludeScopes = true; | ||
| simple.SingleLine = true; | ||
| simple.TimestampFormat = "[yyyy-MM-dd HH:mm:ss.fff] "; | ||
| }); |
Member
There was a problem hiding this comment.
Why is console logging also enabled when we are logging to a file?
Comment on lines
31
to
56
| services.AddOptions<AzureMcpServerConfiguration>() | ||
| .Configure<IOptions<ServiceStartOptions>>((options, serviceStartOptions) => | ||
| { | ||
| // Assembly.GetEntryAssembly is used to retrieve the version of the server application as that is | ||
| // the assembly that will run the tool calls. | ||
| var entryAssembly = Assembly.GetEntryAssembly(); | ||
| if (entryAssembly != null) | ||
| { | ||
| options.Version = AssemblyHelper.GetAssemblyVersion(entryAssembly); | ||
| } | ||
|
|
||
| // Disable telemetry when support logging is enabled to prevent sensitive data from being sent | ||
| // to telemetry endpoints. Support logging captures debug-level information that may contain | ||
| // sensitive data, so we disable all telemetry as a safety measure. | ||
| if (!string.IsNullOrWhiteSpace(serviceStartOptions.Value.SupportLoggingFolder)) | ||
| { | ||
| options.IsTelemetryEnabled = false; | ||
| return; | ||
| } | ||
|
|
||
| // This environment variable can be used to disable telemetry collection entirely. This takes precedence | ||
| // over any other settings. | ||
| var collectTelemetry = Environment.GetEnvironmentVariable("AZURE_MCP_COLLECT_TELEMETRY"); | ||
|
|
||
| options.IsTelemetryEnabled = string.IsNullOrWhiteSpace(collectTelemetry) || (bool.TryParse(collectTelemetry, out var shouldCollect) && shouldCollect); | ||
| }); |
Member
There was a problem hiding this comment.
This code was moved to ServiceCollectionExtensions.cs. Please rebase and update the configuration there.
xiangyan99
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
[Provide a clear, concise description of the changes]Added support logging capability with
--dangerously-enable-support-loggingand--log-file-pathoptions for troubleshooting and support scenarios.[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 promptsconsolidated-tools.json/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline