Remove hard coded strings. Migrate to use IConfiguration #1112

conniey · 2025-11-07T21:17:45Z

What does this PR do?

[Provide a clear, concise description of the changes]

Related issues from migration

dotnet format resulting in IL2026 and IL3050 despite using source generation for configuration. (See: dotnet format raises native AOT warnings regardless of <EnableConfigurationBindingGenerator/> dotnet/sdk#45054)
DbConnectionStringBuilder annotations force invalid suppressions on library authors dotnet/runtime#121718
Unactionable trim warning when using MySqlStringConnectionBuilder mysql-net/MySqlConnector#1607

[Any additional context, screenshots, or information that helps reviewers]

GitHub issue number?

[Link to the GitHub issue this PR addresses]

Pre-merge Checklist

Copilot

Pull Request Overview

This PR refactors the configuration system to use appsettings.json files with the Host.CreateApplicationBuilder pattern, moving away from manual service collection construction. The changes enable AOT-compatible configuration binding and centralize server configuration (name, prefix, display name) in appsettings.json files.

Key changes:

Introduced appsettings.json files for server configuration across multiple servers
Updated AzureMcpServerConfiguration to include required properties (Prefix, Name, DisplayName) with data annotations
Refactored Program.cs files to use Host.CreateApplicationBuilder instead of direct ServiceCollection instantiation

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
servers/Template.Mcp.Server/src/appsettings.json	Added configuration file with server metadata
servers/Template.Mcp.Server/src/appsettings.Development.json	Added development logging configuration
servers/Template.Mcp.Server/src/Template.Mcp.Server.csproj	Added content items to copy appsettings files to output
servers/Template.Mcp.Server/src/Program.cs	Refactored to use Host.CreateApplicationBuilder and pass IHostEnvironment
servers/Fabric.Mcp.Server/src/Program.cs	Refactored to use Host.CreateApplicationBuilder and pass IHostEnvironment
servers/Azure.Mcp.Server/src/appsettings.json	Added configuration file with server metadata
servers/Azure.Mcp.Server/src/appsettings.Release.json	Added release configuration with Application Insights connection string
servers/Azure.Mcp.Server/src/appsettings.Development.json	Added development logging configuration with telemetry disabled
servers/Azure.Mcp.Server/src/Properties/launchSettings.json	Reformatted and added new "local-stdio" profile
servers/Azure.Mcp.Server/src/Program.cs	Refactored to use Host.CreateApplicationBuilder, added Release config loading, and host lifecycle
servers/Azure.Mcp.Server/src/Azure.Mcp.Server.csproj	Added conditional content items for configuration files based on build configuration
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Services/Telemetry/TelemetryServiceTests.cs	Updated test configuration instances with new required properties
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Commands/CommandFactoryTests.cs	Added server configuration setup and updated constructor calls
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Tools/UnitTests/ToolsListCommandTests.cs	Updated CommandFactory instantiation with server configuration
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/Commands/Discovery/ConsolidatedToolDiscoveryStrategyTests.cs	Updated strategy instantiation to pass required dependencies explicitly
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/CommandFactoryHelpers.cs	Added default server configuration creation in test helpers
core/Azure.Mcp.Core/src/Extensions/OpenTelemetryExtensions.cs	Refactored to accept IHostEnvironment, removed hardcoded connection string, added empty conditional blocks
core/Azure.Mcp.Core/src/Configuration/AzureMcpServerConfigurationValidator.cs	Added options validator for server configuration
core/Azure.Mcp.Core/src/Configuration/AzureMcpServerConfiguration.cs	Added required properties with data annotations and documentation
core/Azure.Mcp.Core/src/Commands/CommandFactory.cs	Updated to use server name from configuration instead of hardcoded value
core/Azure.Mcp.Core/src/Azure.Mcp.Core.csproj	Enabled configuration binding generator for AOT compatibility
core/Azure.Mcp.Core/src/Areas/Server/Commands/ServiceStartCommand.cs	Refactored to use Host.CreateApplicationBuilder pattern
core/Azure.Mcp.Core/src/Areas/Server/Commands/ServiceCollectionExtensions.cs	Extracted configuration setup, moved GetServerVersion method, added ConfigureMcpServerOptions
core/Azure.Mcp.Core/src/Areas/Server/Commands/Discovery/ConsolidatedToolDiscoveryStrategy.cs	Updated constructor to accept required dependencies as parameters

Comments suppressed due to low confidence (3)

core/Azure.Mcp.Core/src/Extensions/OpenTelemetryExtensions.cs:94

Empty block without comment.

        {

        }

core/Azure.Mcp.Core/src/Extensions/OpenTelemetryExtensions.cs:94

If-statement with an empty then-branch and no else-branch.

        else if(hostEnvironment.IsDevelopment())
        {

        }

core/Azure.Mcp.Core/src/Areas/Server/Commands/ServiceCollectionExtensions.cs:54

This assignment to mcpServerOptionsBuilder is useless, since its value is never read.

                    var mcpServerOptionsBuilder = services.AddOptions<McpServerOptions>();

core/Azure.Mcp.Core/src/Extensions/OpenTelemetryExtensions.cs

Copilot · 2025-11-07T21:23:22Z

core/Azure.Mcp.Core/src/Extensions/OpenTelemetryExtensions.cs

-        });
+
+        }
+        else if(hostEnvironment.IsDevelopment())


Missing space after 'if' keyword. Should be else if (hostEnvironment.IsDevelopment()) to follow C# coding conventions.

Suggested change

else if(hostEnvironment.IsDevelopment())

else if (hostEnvironment.IsDevelopment())

core/Azure.Mcp.Core/src/Extensions/OpenTelemetryExtensions.cs

servers/Azure.Mcp.Server/src/appsettings.Release.json

Copilot · 2025-11-07T21:23:24Z

servers/Azure.Mcp.Server/src/Program.cs

+            await InitializeServicesAsync(host.Services);
+
+            // Starts any IHostedServices
+            await host.StartAsync();


Starting the host before invoking the command will cause the application to hang indefinitely. The host.StartAsync() call starts background services and keeps them running. This should either be removed or the application lifecycle should be refactored to properly manage the host. Consider whether hosted services are needed here, or if the host should be started only when running in HTTP mode.

Suggested change

await host.StartAsync();

// await host.StartAsync(); // Removed to prevent application hang

core/Azure.Mcp.Core/src/Areas/Server/Commands/ServiceCollectionExtensions.cs

alzimmermsft · 2025-11-10T22:16:01Z

servers/Azure.Mcp.Server/src/appsettings.Development.json

@@ -0,0 +1,22 @@
+{
+    "AZURE_MCP_COLLECT_TELEMETRY": "false",


I think we want this to be true and AZURE_MCP_ENABLE_OTLP_EXPORTER. But we don't want to have the Application Insights connection string injected.

alzimmermsft · 2025-11-10T22:20:16Z

core/Azure.Mcp.Core/src/Commands/CommandFactory.cs

        _logger = logger;
-        _rootGroup = new CommandGroup(RootCommandGroupName, "Azure MCP Server");
+        _serverName = serverConfig.Value.Name;
+        _rootGroup = new CommandGroup(_serverName, serverConfig.Value.DisplayName);


Should we be using serverConfig.Value.Prefix for CommandGroup name?

Suggested change

_rootGroup = new CommandGroup(_serverName, serverConfig.Value.DisplayName);

_rootGroup = new CommandGroup(serverConfig.Value.Prefix, serverConfig.Value.DisplayName);

Won't this be changing azmcp to Azure.Mcp.Server

https://github.com/microsoft/mcp/pull/1112/files#diff-84afbeb80efc4b997ac0d0a1be89ae59bb0492f484a35fcd996a7797b654dc1aR3

But maybe we should rename prefix to RootCommandName?

alzimmermsft · 2025-11-10T22:24:03Z

core/Azure.Mcp.Core/src/Extensions/OpenTelemetryExtensions.cs

+
+            builder.AddAzureMonitorMetricExporter(options =>
+            {
+                options.ConnectionString = config.ApplicationInsightsConnectionString;


What does Azure Monitor do when the connection string is missing / null / empty?

Should configuration of Azure Monitor be guarded by the state of the connection string?

…ault required keywords for runtime option checks.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings November 7, 2025 21:17

conniey requested review from a team as code owners November 7, 2025 21:17

conniey requested review from JonathanCrd, alzimmermsft, hallipr, jairmyree, jongio, joshfree and sandeep-sen November 7, 2025 21:17

github-project-automation bot added this to Azure MCP Server Nov 7, 2025

github-project-automation bot moved this to Untriaged in Azure MCP Server Nov 7, 2025

Copilot AI reviewed Nov 7, 2025

View reviewed changes

conniey requested review from mattkohnms and ramnov as code owners November 10, 2025 12:05

conniey force-pushed the remove-azmcp branch from d08b719 to 1f6a28d Compare November 10, 2025 12:06

conniey changed the title ~~(wip) Remove hard coded strings. Use configuration.~~ Remove hard coded strings. Migrate to use IConfiguration Nov 10, 2025

joshfree assigned conniey Nov 10, 2025

joshfree added the tools-Telemetry label Nov 10, 2025

joshfree moved this from Untriaged to In Progress in Azure MCP Server Nov 10, 2025

joshfree added this to the 2025-11 milestone Nov 10, 2025

alzimmermsft reviewed Nov 10, 2025

View reviewed changes

conniey added 8 commits November 12, 2025 15:17

Change AzureMcpServerConfiguration to McpServerConfiguration. Add def…

a48c578

…ault required keywords for runtime option checks.

Move dependencies to constructor.

2677f9d

Split Discovery and ToolLoading into another method.

6055d8e

Use ApplicationBuilder

b1d0d38

Replace hardcoded values in CommandFactory with configured ones.

a1757cc

Add Options Validation.

8aa39c3

Remove Server Configuration logic from Open Telemetry.

864d38b

Fix Renamed back to AzureMcpServerConfiguration,.

7b612ce

conniey and others added 17 commits November 12, 2025 15:18

Enabled Strongly typed Configuration Binding for AOT support.

70f140c

Fix test breaks.

48a0daa

Add Local Development launch settings.

d70f4a4

Use ApplicationBuilder to Support environments.

78ac6db

Add template files.

3c84ce9

Update to ApplicationBuilder in Fabric.

0c09eb8

Rename to release.

8386e31

Optionally copy release and use appsettings.

03aca18

Move Otel and rest of McpServerConfiguration into one method.

598b57e

Add resource detector to support DI.

accc290

Configure OpenTelemetry exporter for logging, tracing, and metrics.

9e35aaf

Remove unneeded ConfigureOpenTelemetryLogger()

c609acb

Fix build breaks.

4eb836f

Apply suggestions from code review

b9177cd

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Use APplkicationInsights string

402b8b6

Add suppressions for IL2026 IL3050

f7abb2b

Fix build breaks from rebase.

5c33b01

conniey force-pushed the remove-azmcp branch from ecb7bc8 to 5c33b01 Compare November 13, 2025 00:10

conniey requested a review from a team as a code owner November 13, 2025 00:10

Undo fix

5c4518b

conniey mentioned this pull request Nov 13, 2025

Enable telemetry for HTTP mode #1150

Merged

19 tasks

AmosHersch approved these changes Nov 16, 2025

View reviewed changes

sbomer mentioned this pull request Nov 17, 2025

Unactionable trim warning when using MySqlStringConnectionBuilder mysql-net/MySqlConnector#1607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove hard coded strings. Migrate to use IConfiguration #1112

Remove hard coded strings. Migrate to use IConfiguration #1112

Uh oh!

conniey commented Nov 7, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Nov 7, 2025

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alzimmermsft Nov 10, 2025

Uh oh!

alzimmermsft Nov 10, 2025

Uh oh!

alzimmermsft Nov 10, 2025

Uh oh!

alzimmermsft Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	else if(hostEnvironment.IsDevelopment())
	else if (hostEnvironment.IsDevelopment())

	await host.StartAsync();
	// await host.StartAsync(); // Removed to prevent application hang

	_rootGroup = new CommandGroup(_serverName, serverConfig.Value.DisplayName);
	_rootGroup = new CommandGroup(serverConfig.Value.Prefix, serverConfig.Value.DisplayName);

Remove hard coded strings. Migrate to use IConfiguration #1112

Are you sure you want to change the base?

Remove hard coded strings. Migrate to use IConfiguration #1112

Uh oh!

Conversation

conniey commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Related issues from migration

GitHub issue number?

Pre-merge Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alzimmermsft Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

alzimmermsft Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

alzimmermsft Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

alzimmermsft Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

conniey commented Nov 7, 2025 •

edited

Loading