-
Notifications
You must be signed in to change notification settings - Fork 293
Add automatic configuration reading from ApplicationInsights section in appsettings.json #3064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…in appsettings.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds automatic configuration reading from the "ApplicationInsights" section in appsettings.json for the shimmed SDK, matching the behavior of the classic 2.x SDK. The implementation uses the .NET Options pattern to automatically bind configuration values when IConfiguration is in the DI container.
Key changes:
- Automatic configuration binding via inline lambda in
AddApplicationInsightsTelemetrymethods - Removal of obsolete DeveloperMode property and related environment variable handling
- Configuration precedence implementation: environment variables > explicit config > appsettings.json
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| NETCORE/test/WorkerIntegrationTests.Tests/content/*.json | Test configuration files for WorkerService integration tests |
| NETCORE/test/IntegrationTests.Tests/content/*.json | Test configuration files for AspNetCore integration tests |
| NETCORE/test/WorkerIntegrationTests.Tests/ConfigurationTests.cs | Integration tests for WorkerService configuration binding |
| NETCORE/test/IntegrationTests.Tests/AspNetCoreConfigurationTests.cs | Integration tests for AspNetCore configuration binding |
| NETCORE/test/WorkerIntegrationTests.Tests/WorkerIntegrationTests.Tests.csproj | Project file updated to copy test config files |
| NETCORE/test/IntegrationTests.Tests/IntegrationTests.Tests.csproj | Project file updated to copy test config files |
| NETCORE/src/Shared/Extensions/DefaultApplicationInsightsServiceConfigureOptions.cs | Modified to remove DeveloperMode handling (but appears unused) |
| NETCORE/src/Shared/Extensions/ApplicationInsightsServiceOptions.cs | Removed DeveloperMode property |
| NETCORE/src/Shared/Extensions/ApplicationInsightsExtensions.cs | Removed DeveloperMode and EndpointAddress environment variable handling |
| NETCORE/src/Microsoft.ApplicationInsights.WorkerService/ApplicationInsightsExtensions.cs | Added inline configuration binding for automatic appsettings.json reading |
| NETCORE/src/Microsoft.ApplicationInsights.AspNetCore/Extensions/ApplicationInsightsExtensions.cs | Added inline configuration binding for automatic appsettings.json reading |
Comments suppressed due to low confidence (2)
NETCORE/src/Shared/Extensions/DefaultApplicationInsightsServiceConfigureOptions.cs:5
- The conditional namespace declaration creates two different types in different namespaces based on compilation symbols. This means DefaultApplicationInsightsServiceConfigureOptions exists in Microsoft.AspNetCore.Hosting for AspNetCore and Microsoft.ApplicationInsights.WorkerService for WorkerService. Since this class is not being registered anywhere in the visible code, this dual-namespace approach adds unnecessary complexity for unused code. Consider removing this class entirely or properly registering it if it's intended to be used.
NETCORE/src/Shared/Extensions/DefaultApplicationInsightsServiceConfigureOptions.cs:40 - The DefaultApplicationInsightsServiceConfigureOptions class appears to be unused dead code. The new implementation in ApplicationInsightsExtensions uses an inline lambda with Configure instead of registering this IConfigureOptions implementation. This class should either be removed or the implementation should be updated to actually register it. If the intention is to keep it for backward compatibility or future use, it needs to be registered somewhere in the service collection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NETCORE/test/WorkerIntegrationTests.Tests/content/config-connection-string.json
Outdated
Show resolved
Hide resolved
...t/WorkerIntegrationTests.Tests/content/config-connection-string-and-instrumentation-key.json
Outdated
Show resolved
Hide resolved
NETCORE/test/IntegrationTests.Tests/content/config-connection-string.json
Outdated
Show resolved
Hide resolved
...RE/test/IntegrationTests.Tests/content/config-connection-string-and-instrumentation-key.json
Outdated
Show resolved
Hide resolved
|
there are some api related errors (ie some removed configurations that are still part of the api files) in the CI |
Enables the shimmed SDK to automatically read configuration from the "ApplicationInsights" section in appsettings.json, matching the behavior of the classic 2.x SDK.
Changes:
DefaultApplicationInsightsServiceConfigureOptionsin shared folder for both AspNetCore and WorkerService packagesSupported properties:
Tests:
Cleanup: