ApplicationInsights-dotnet icon indicating copy to clipboard operation
ApplicationInsights-dotnet copied to clipboard

ASP.NET Core configuration preferentially treats appsettings file instead of IConfiguration

Open cijothomas opened this issue 3 years ago • 4 comments
trafficstars

ASP.NET Core SDK only

Actual: AddApplicationInsightsTelemetry is "favoring" configuration from appsettings.json.

Expected: AddApplicationInsightsTelemetry should simply respect IConfiguration, which is fully controlled by application owner.

To Reproduce

See https://github.com/MicrosoftDocs/azure-docs/issues/90862

Rootcause: https://github.com/microsoft/ApplicationInsights-dotnet/blob/main/NETCORE/src/Microsoft.ApplicationInsights.AspNetCore/Extensions/DefaultApplicationInsightsServiceConfigureOptions.cs#L36-L43 ^ This first loads user's IConfiguration, then loads appsettings.json. This causes appsettings.json to override.

https://github.com/microsoft/ApplicationInsights-dotnet/blob/main/NETCORE/src/Microsoft.ApplicationInsights.WorkerService/DefaultApplicationInsightsServiceConfigureOptions.cs#L31https://github.com/microsoft/ApplicationInsights-dotnet/blob/main/NETCORE/src/Microsoft.ApplicationInsights.WorkerService/DefaultApplicationInsightsServiceConfigureOptions.cs#L31

WorkerService does not have this issue. So, I think this added to maintain backward compatibility when the feature was added to AddApplicationInsightsTelemetry() to automatically read IConfiguration.

cijothomas avatar Apr 08 '22 14:04 cijothomas

I think the reason is: Prior to ~2.15, AddApplicationInisghtsTelemetry() ONLY considered appsettings.json to look for config. in 2.15, it was modified to fetch user's IConfiguration and look for config inside it. Since the previous behavior was to only read settings from appsettings.json, in the new behavior, appsettings.json was treated "preferentially".

For a new user, this might be confusing/surprising, though it was done for a reason, which made sense for migrating users. At this stage, I think the best option, is to do nothing, and document that appsettings.json settings overrides anything else. Workarounds include: Use the overload AddApplicationInsightsTelemetry(IConfiguration).

cijothomas avatar Apr 08 '22 14:04 cijothomas

Being new to AppInsights myself, I had to digest all of this and realized that I should be using the method signature which takes in my version of IConfiguration. The documentation does call it out but should likely better reinforce this concept and explain the behavior. I would expect IConfiguration to be used by default and for it to be the sole source of truth and never overridden by re-reading from appsettings.json as I have overloads i.e. the typical per-environment file approach.

What I found after switching to that method signature was that I lost the ability to tweak the 'options' in conjunction with loading from the referenced instance of IConfiguration - which I had been leveraging that to explicitly set the Developer Mode based on my own specific logic.

It seems like now I have to find the equivalent way to explicitly set it in my appsettings.json file - or overload files - otherwise it looks like I'd have to set if by code where I resolve & use the TelemetryConfiguration/Channel (I think). I'd much rather do it once in the same place I'm registering things.

bhehe avatar Apr 26 '22 21:04 bhehe

... I lost the ability to tweak the 'options' in conjunction with loading from the referenced instance of IConfiguration

Perhaps we need an overload of AddApplicationInsightsTelemetry that takes an IConfiguration and a delegate (Action<ApplicationInsightsServiceOptions>)

Alternatively, does this work:

services.AddApplicationInsightsTelemetry(options => 
{
    configuration.Bind("ApplicationInsights", options);
    // TODO: Modify options.DeveloperMode as required
});

where configuration is your configuration root object. If your code doesn't already have it in hand, it may be available on the builder context object.

pharring avatar Apr 26 '22 22:04 pharring

Yes, having that in addition to the IConfiguration argument on the method would be very useful IMO. Also sounds like it should be fairly trivial to support.

I'll have to see if the other workaround can be used, but I'd much rather use the 'official' approach rather than trying to sort of hack at it - I'm assuming that internally it's just doing that binding to a named config section but I wouldn't want to assume that and try to recreate/stay in sync with what the official code is doing.

For now I'm just going to try and leverage the appsettings.json file and set it; Which I believe I found an example where it's just an element under "ApplicationInsights" with the expected name of "DeveloperMode" so I can make that work for me for now.

bhehe avatar Apr 27 '22 12:04 bhehe

This issue is stale because it has been open 300 days with no activity. Remove stale label or this will be closed in 7 days. Commenting will instruct the bot to automatically remove the label.

github-actions[bot] avatar Feb 22 '23 00:02 github-actions[bot]