Skip to content

Conversation

@rajkumar-rangaraj
Copy link
Member

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:

  • Added automatic configuration binding when IConfiguration is in DI container
  • Created DefaultApplicationInsightsServiceConfigureOptions in shared folder for both AspNetCore and WorkerService packages
  • Configuration flows from appsettings.json → ApplicationInsightsServiceOptions → AzureMonitorExporterOptions
  • Configuration precedence: environment variables > explicit config > appsettings.json

Supported properties:

  • ConnectionString, EnableAdaptiveSampling, EnableQuickPulseMetricStream, ApplicationVersion
  • RequestCollectionOptions (AspNetCore), DependencyCollectionOptions

Tests:

  • Added configuration tests to IntegrationTests.Tests and WorkerIntegrationTests.Tests
  • Tests cover property binding and configuration precedence

Cleanup:

  • Removed obsolete DeveloperMode and EndpointAddress environment variable handling
  • Removed unused classic SDK properties from test config files

Copilot AI review requested due to automatic review settings December 30, 2025 18:45
Copy link

Copilot AI left a 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 AddApplicationInsightsTelemetry methods
  • 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.

@harsimar
Copy link
Member

there are some api related errors (ie some removed configurations that are still part of the api files) in the CI

@rajkumar-rangaraj rajkumar-rangaraj merged commit 1648003 into main Dec 30, 2025
22 checks passed
@rajkumar-rangaraj rajkumar-rangaraj deleted the rajrang/supportConfig branch December 30, 2025 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants