Add support for binding FormatterOptions from config file

[mrek-msft]

PR "fixes" (actually add missing support) #80724 WebApplication.CreateBuilder ignores Logging:EventLog:SourceName. It does not require any change to public facing API, but I consider it behavioral "breaking" change. Do I need to follow some process? (I am new in this repo). Implementation is done to be on par with Console loggers.

There are few points which brings some questions in advance:

• Console loggers need to scope configuration under "FormatterOptions" which comes from layered design of ConsoleLogger. Current proposal do not require "FormatterOptions" for EventLogLogger but we can consider so for having options scoped the same accross loggers.
• For historical reason it is called Settings, not Options. I named it EventLogConfigureSettings to maintain convention, but we can consider EventLogConfigureOptions which will better indicate that it actually implements IConfigureOptions and will match Console logger convention. It is internal though.
• EventLogSettings has members Filter and EventLog which config binding source generator (or future ones) can mess with.
• Config is not bound when legacy depracted extension methods are used, but this is on par with Console loggers.

Fixes #80724

[Copilot]

Pull request overview

This PR adds configuration-based binding support for EventLogSettings so Logging:EventLog:* values (e.g., SourceName) are applied when the EventLog provider is added via ILoggingBuilder (aligning behavior with other loggers like Console).

Changes:

• Enables configuration binding source generation for the EventLog logging project and adds the needed logging-configuration dependency.
• Registers an IConfigureOptions<EventLogSettings> implementation (EventLogConfigureSettings) when AddEventLog() is used.
• Adds an internal EventLogSettings.Configure(IConfiguration) helper that performs configuration binding.

Reviewed changes

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

File	Description
src/libraries/Microsoft.Extensions.Logging.EventLog/src/Microsoft.Extensions.Logging.EventLog.csproj	Enables config binding generator and adds references required for provider configuration binding.
src/libraries/Microsoft.Extensions.Logging.EventLog/src/EventLoggerFactoryExtensions.cs	Wires EventLogConfigureSettings into DI when adding the provider.
src/libraries/Microsoft.Extensions.Logging.EventLog/src/EventLogSettings.cs	Adds internal configuration binding entry point for settings.
src/libraries/Microsoft.Extensions.Logging.EventLog/src/EventLogConfigureSettings.cs	New IConfigureOptions<EventLogSettings> to bind settings from provider configuration.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[tarekgh]

@copilot review code skill

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

[mrek-msft]

/ba-g Test failures on most recent Build Analysis run are all known except one timeout which does not relate to changes in this PR.

[stephentoub]

Code Review: PR #124176 - Add support for binding EventLogSettings from config file

Holistic Assessment

Motivation: This PR addresses a long-standing gap where EventLogSettings (SourceName, LogName, MachineName) could not be bound from configuration files, unlike other logging providers. This is a valid enhancement that aligns EventLog behavior with other providers like ConsoleLogger.

Approach: The implementation follows the established pattern used by ConsoleLoggerConfigureOptions, registering an IConfigureOptions to bind configuration and adding IOptionsChangeTokenSource for configuration reload support. Source-generated configuration binding is enabled to support trimming.

Net Positive: ✅ Yes - The changes address a real user need and follow established patterns.

---

Detailed Findings

⚠️ Potential Issues

1. Duplicate InterceptorsPreviewNamespaces in csproj (Line 10 & 12)

   • The InterceptorsPreviewNamespaces property is defined twice in the csproj file, which is redundant.
   • File: Microsoft.Extensions.Logging.EventLog.csproj, lines 10 and 12
   • Recommendation: Remove the duplicate line.

2. XML Documentation Claims Trimming Support, But Uses Runtime Binding

   • The remarks in EventLogConfigureOptions.cs state: "Uses source-generated configuration binding to allow ConfigurationBinder, and all its dependencies, to be trimmed."
   • However, _configuration.Bind(options) at line 29 is a call to the standard ConfigurationBinder.Bind extension method, not the source-generated intercepted version.
   • The source generator will intercept this call only if it can see the call site and the type being bound. Since EventLogSettings has complex members (like Filter delegate and EventLog object), the generator may skip binding for those properties - which is actually correct behavior.
   • Recommendation: Verify that the source generator is indeed intercepting this call by checking the generated code, or consider clarifying the remarks to note that only simple properties are bound.

3. Missing BOM on new file

   • EventLogConfigureOptions.cs doesn't have a BOM, while ConsoleLoggerConfigureOptions.cs does. This is a minor consistency issue - the file should probably match the convention of the codebase.

✅ Good Practices

1. Follows Console Logger Pattern: The implementation mirrors ConsoleLoggerConfigureOptions exactly, which ensures consistency across logging providers.

2. Proper Service Registration: The AddEventLog() method now calls:

   • �uilder.AddConfiguration() - ensures logging configuration is set up
   • Registers IConfigureOptions - enables configuration binding
   • Registers IOptionsChangeTokenSource - enables configuration reload

3. Tests Cover Key Scenarios: The two new tests verify:

   • Configuration values are read from Logging:EventLog:* section
   • Action-based configuration overrides config file values (proper options precedence)

4. Backward Compatibility: The AddEventLog(EventLogSettings settings) overload correctly does NOT register the configuration binder, since explicit settings are passed directly to the provider.

💡 Suggestions

1. Test Improvement: Combine into Theory

   • Per repository conventions, consider combining AddEventLog_SettingsFromConfiguration_IsReadFromLoggingConfiguration and AddEventLog_SettingsFromConfiguration_ActionOverridesConfigValues into a single [Theory] with multiple cases.

2. Consider null configuration section behavior

   • When Logging:EventLog section doesn't exist, Bind will be called on an empty configuration. This is fine (no-op), but might be worth a test case to document the expected behavior.

---

Summary

Verdict: ✅ Approve with minor fixes

The PR is well-structured and follows established patterns. Please address the duplicate InterceptorsPreviewNamespaces line in the csproj file. The implementation is consistent with how other logging providers handle configuration binding.

[stephentoub]

🤖 Copilot Code Review — PR #124176

Holistic Assessment

Motivation: This PR addresses a long-standing gap (#80724) where EventLogSettings properties (SourceName, LogName, MachineName) were not being bound from configuration files, unlike other logging providers such as ConsoleLogger. The issue is valid and well-documented.

Approach: The implementation follows the established pattern used by ConsoleLoggerConfigureOptions exactly—registering an IConfigureOptions<EventLogSettings> to bind configuration, adding IOptionsChangeTokenSource for configuration reload support, and enabling source-generated configuration binding for trimming compatibility. This is the correct approach.

Summary: ⚠️ Needs Changes. The implementation is fundamentally correct and follows established patterns. There is one required fix (duplicate line in csproj) and one design question for maintainers to consider.

---

Detailed Findings

⚠️ Duplicate InterceptorsPreviewNamespaces in csproj — Should fix

The diff shows InterceptorsPreviewNamespaces being set twice with identical values:

<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);Microsoft.Extensions.Configuration.Binder.SourceGeneration</InterceptorsPreviewNamespaces>
<EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator>
<InterceptorsPreviewNamespaces>$(InterceptorsPreviewNamespaces);Microsoft.Extensions.Configuration.Binder.SourceGeneration</InterceptorsPreviewNamespaces>

This appears to be a copy/paste error. The second instance on line 12 should be removed. Compare to Microsoft.Extensions.Logging.Console.csproj which only has one such entry.

Flagged by 3/3 models (GPT-5.2, Gemini 3 Pro, Claude Opus).

---

💡 AddEventLog(EventLogSettings) overload behavior — Consider documenting

The AddEventLog(EventLogSettings settings) overload (lines 76-84) does not call AddConfiguration() or register the configuration binding services, unlike the parameterless AddEventLog() overload. This means:

// This will have config binding
builder.Logging.AddEventLog();

// This will NOT have config binding
builder.Logging.AddEventLog(new EventLogSettings { ... });

This is likely intentional (the user is providing a pre-constructed settings object), but it creates an asymmetry in behavior. Consider either:

1. Documenting this behavior difference in XML docs, or
2. Having the overload also enable configuration binding for consistency

This is a design decision for maintainers. The current behavior matches how other providers handle explicit settings.

---

✅ Follows established ConsoleLogger pattern

The EventLogConfigureOptions class exactly mirrors the ConsoleLoggerConfigureOptions pattern:

• Same namespace (Microsoft.Extensions.Logging)
• Same [UnsupportedOSPlatform("browser")] attribute on constructor
• Same use of _configuration.Bind(options) via source-generated binding
• Same service registration order in the extension method

This ensures consistency across the codebase.

---

✅ Correct service registrations

The service registrations are correct and complete:

builder.AddConfiguration();
builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<ILoggerProvider, EventLogLoggerProvider>());
builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IConfigureOptions<EventLogSettings>, EventLogConfigureOptions>());
builder.Services.TryAddEnumerable(ServiceDescriptor.Singleton<IOptionsChangeTokenSource<EventLogSettings>, LoggerProviderOptionsChangeTokenSource<EventLogSettings, EventLogLoggerProvider>>());

Adding LoggerProviderOptionsChangeTokenSource ensures configuration reload flows correctly.

---

✅ SYSLIB warning suppressions are appropriate

The suppression of SYSLIB1100;SYSLIB1101 in the csproj is appropriate. EventLogSettings contains a Func<string, LogLevel, bool>? Filter property which the configuration binding source generator cannot bind, and would otherwise produce a warning.

---

✅ Test coverage is good

The PR adds two tests that verify:

1. Configuration values are correctly read from Logging:EventLog:* section
2. Action-based configuration (AddEventLog(options => ...)) takes precedence over config file values

Both tests follow the existing test patterns in the file.

---

Summary

Required fix: Remove the duplicate InterceptorsPreviewNamespaces line from Microsoft.Extensions.Logging.EventLog.csproj.

The rest of the implementation is correct and follows established patterns. The PR addresses a legitimate user need and is a net positive for the codebase.

---

Review generated with multi-model consensus (GPT-5.2, Gemini 3 Pro, Claude Opus 4.5)

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.