Enable OTLP exporting as part of development build#2006
Enable OTLP exporting as part of development build#2006alzimmermsft wants to merge 3 commits intomicrosoft:mainfrom
Conversation
alzimmermsft
requested review from
JasonYeMSFT,
chidozieononiwu,
Copilot,
g2vinay,
msalaman,
tmeschter and
wbreza
There was a problem hiding this comment.
Pull request overview
This PR updates appsettings.Development.json for the Azure MCP Server to enable OTLP telemetry exporters by default for development builds, aiming to provide richer debugging without requiring manual environment setup. It also replaces the old AZURE_MCP_COLLECT_TELEMETRY key with the more specific AZURE_MCP_COLLECT_TELEMETRY_MICROSOFT key to prevent development data from polluting Microsoft's production telemetry.
Changes:
- Replaced
AZURE_MCP_COLLECT_TELEMETRY: falsewithAZURE_MCP_COLLECT_TELEMETRY_MICROSOFT: falseto prevent development telemetry from reaching Microsoft instrumentation. - Added
AZURE_MCP_ENABLE_OTLP_EXPORTER: trueto enable OTLP exporting by default for development.
| public static void InitializeConfigurationAndOptions(this IServiceCollection services) | ||
| public static void InitializeConfigurationOptionsAndOpenTelemetry(this IServiceCollection services) | ||
| { | ||
| #if DEBUG |
There was a problem hiding this comment.
The reason for having DOTNET_ENVIRONMENT is so we don't have to recompile to make test changes. We can rerun the application with different settings. We shouldn't have to if/def things based on the build type.
There was a problem hiding this comment.
There was a problem hiding this comment.
I was finding the DOTNET_ENVIRONMENT was blank when running debug builds, so we fell back to Production. If you know of a better way to fallback to Development when running a debug build do let me know.
| /// </summary> | ||
| /// <param name="services">Service Collection to add configuration logic to.</param> | ||
| public static void InitializeConfigurationAndOptions(this IServiceCollection services) | ||
| public static void InitializeConfigurationOptionsAndOpenTelemetry(this IServiceCollection services) |
There was a problem hiding this comment.
Is there a reason we have to configure open telemetry in this method as well? This method now has multiple responsibilities rather than just setting options.
There was a problem hiding this comment.
I couldn't find a cleaner way to get the IConfiguration singleton bound here when we called OpenTelemetryExtensions.ConfigureOpenTelemetry. If you know of a way to cleanly pull that out of the IServiceCollection I'd be more than happy to revert this change as I'm not a huge fan of it either.
| @@ -92,7 +93,7 @@ private static void EnableAzureMonitor(this IServiceCollection services) | |||
| #if RELEASE | |||
There was a problem hiding this comment.
Frankly, I think we could have avoided this by using default appsettings.json = false and used the running environment to set it or not. Because now if we want to test this in a debug environment, it's impossible.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Enables OTLP dev export and migrates telemetry config reads to IConfiguration.
Issues to address:
- appsettings.Development.json - Fabric and Template servers still have the old config keys; need the same update
- OpenTelemetryExtensions.cs:84 - APPLICATIONINSIGHTS_CONNECTION_STRING now readable from checked-in JSON (was env-only)
- ServiceCollectionExtensions.cs:251 - public API renamed without obsolete forwarders for external consumers
- OpenTelemetryExtensionsTests.cs:29 - mock config doesn't exercise the new IConfiguration code paths
| "AZURE_MCP_COLLECT_TELEMETRY": "false", | ||
| // Disable AZURE_MCP_COLLECT_TELEMETRY_MICROSOFT to prevent development telemetry from being collected by Microsoft | ||
| // instrumentation. This is to avoid polluting production telemetry with development data. | ||
| "AZURE_MCP_COLLECT_TELEMETRY_MICROSOFT": "false", |
There was a problem hiding this comment.
[HIGH] Inconsistency: Fabric and Template dev configs not updated
Both servers/Fabric.Mcp.Server/src/appsettings.Development.json and servers/Template.Mcp.Server/src/appsettings.Development.json still have the old AZURE_MCP_COLLECT_TELEMETRY: false and lack the new AZURE_MCP_COLLECT_TELEMETRY_MICROSOFT and AZURE_MCP_ENABLE_OTLP_EXPORTER keys. Their Program.cs files were updated to call the renamed method, but their dev configs are stale. Devs running Fabric or Template servers in Development will get different telemetry behavior than Azure.Mcp.Server.
| /// </summary> | ||
| /// <param name="services">Service Collection to add configuration logic to.</param> | ||
| public static void InitializeConfigurationAndOptions(this IServiceCollection services) | ||
| public static void InitializeConfigurationOptionsAndOpenTelemetry(this IServiceCollection services) |
There was a problem hiding this comment.
[MEDIUM] Breaking change: Public API renamed without backward-compat shim
InitializeConfigurationAndOptions is a public extension method. Renaming it to InitializeConfigurationOptionsAndOpenTelemetry (plus changing ConfigureOpenTelemetry's signature to require IConfiguration) is a source and binary break for any external consumer of Microsoft.Mcp.Core. Consider adding [Obsolete] forwarders that delegate to the new methods for one release, or confirm no external callers exist.
| }); | ||
|
|
||
| var userProvidedAppInsightsConnectionString = Environment.GetEnvironmentVariable("APPLICATIONINSIGHTS_CONNECTION_STRING"); | ||
| var userProvidedAppInsightsConnectionString = configuration["APPLICATIONINSIGHTS_CONNECTION_STRING"]; |
There was a problem hiding this comment.
[MEDIUM] Config: Connection string now readable from checked-in JSON
Switching from Environment.GetEnvironmentVariable to configuration[...] means APPLICATIONINSIGHTS_CONNECTION_STRING can now come from appsettings.json or appsettings.Development.json - not just env vars. If someone adds a connection string to a checked-in config file, it becomes a committed secret. The env-var-only approach was a natural guard against this. Consider keeping this particular value as env-var-only, or document that connection strings should only be set via env vars / user-secrets.
|
|
||
| // Act | ||
| services.ConfigureOpenTelemetry(); | ||
| services.ConfigureOpenTelemetry(Substitute.For<IConfiguration>()); |
There was a problem hiding this comment.
[MEDIUM] Tests: Mock config doesn't verify new behavior
Substitute.For<IConfiguration>() returns null for all reads, so this test only proves the method doesn't throw with empty config. No test verifies that setting AZURE_MCP_ENABLE_OTLP_EXPORTER to true actually registers OTLP exporters, or that APPLICATIONINSIGHTS_CONNECTION_STRING from config enables the user-provided exporter.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
One more item missed in my earlier review:
- OpenTelemetryExtensions.cs:29 -
ConfigureOpenTelemetrysignature also changed (added requiredIConfigurationparam) - separate public API break from the rename - OpenTelemetryExtensionsTests.cs:18 - doc comment still references old method name
InitializeConfigurationAndOptions
| private const string MicrosoftOwnedAppInsightsConnectionString = "InstrumentationKey=21e003c0-efee-4d3f-8a98-1868515aa2c9;IngestionEndpoint=https://centralus-2.in.applicationinsights.azure.com/;LiveEndpoint=https://centralus.livediagnostics.monitor.azure.com/;ApplicationId=f14f6a2d-6405-4f88-bd58-056f25fe274f"; | ||
|
|
||
| public static void ConfigureOpenTelemetry(this IServiceCollection services) | ||
| public static void ConfigureOpenTelemetry(this IServiceCollection services, IConfiguration configuration) |
There was a problem hiding this comment.
[MEDIUM] ConfigureOpenTelemetry signature is also a breaking change
Adding a required IConfiguration parameter changes this public extension method's signature. External consumers calling services.ConfigureOpenTelemetry() won't compile. Same concern as the InitializeConfigurationAndOptions rename - needs either an [Obsolete] parameterless overload or a default parameter value.
What does this PR do?
Enables OTLP exporters by default for development builds to allow for richer debugging without needing to configure the environment. Replaces
"AZURE_MCP_COLLECT_TELEMETRY": "false"with"AZURE_MCP_COLLECT_TELEMETRY_MICROSOFT": "false"which controls more specific behaviors, which we want, to allow custom Application Insights to be enabled without additional configuration in development builds.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.jsonbreaking-changelabel/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline