[dotnet] Fix logging issue when log context level conflicts with logger already captured level

[nvborisenko]

User description

Description

In general each ILogger should be copied to LogContext.

Motivation and Context

Fixes #13839

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Bug fix, Tests

---

Description

• Fixed log level handling in LogContext to ensure proper logging.

• Added logic to initialize loggers with the correct level in LogContext.

• Introduced new tests to validate logging behavior and context-specific log levels.

• Ensured global log state reset in test setup and teardown.

---

Changes walkthrough 📝

	Relevant files
Bug fix	LogContext.cs Fix logger level handling in `LogContext`                                dotnet/src/webdriver/Internal/Logging/LogContext.cs Adjusted LogContext to initialize loggers with correct levels. Fixed IsEnabled method to check logger levels accurately. Improved logger initialization to respect context-specific levels. +5/-2
Tests	LogTest.cs Add tests for logging context behavior                                      dotnet/test/common/Internal/Logging/LogTest.cs Added test to validate message emission in logging contexts. Reset global log state in test setup and teardown. Enhanced test coverage for context-specific log levels. +24/-0

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

PR Reviewer Guide 🔍

(Review updated until commit 77e8a05)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 13839 - Partially compliant Compliant requirements: Fix the issue where FileLogHandler does not write logs to the file system. Ensure that log levels are handled correctly in LogContext. Add tests to validate the logging behavior and context-specific log levels. Reset global log state in test setup and teardown. Non-compliant requirements: Requires further human verification: Verify that the FileLogHandler writes logs correctly in a real-world scenario.

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The IsEnabled method's new condition includes _loggers?[logger.Issuer].Level. This could potentially throw a null reference exception if _loggers or logger.Issuer is null. Ensure proper null checks are in place. public bool IsEnabled(ILogger logger, LogEventLevel level) { return Handlers != null && level >= _level && level >= _loggers?[logger.Issuer].Level; } Test Coverage The new test ContextShouldEmitMessages verifies message emission but does not test for edge cases like invalid log levels or empty messages. Consider adding such tests for robustness. [Test] public void ContextShouldEmitMessages() { using var context = Log.CreateContext(LogEventLevel.Trace).Handlers.Add(testLogHandler); logger.Trace("test message"); Assert.That(testLogHandler.Events.Count, Is.EqualTo(1)); }

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to 77e8a05
Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	✅ Add null safety checks and proper dictionary access to prevent potential runtime exceptions Suggestion Impact: The commit added null safety check for the Level property access using the null conditional operator (?.), addressing part of the null reference concern code diff: - return Handlers != null && level >= _level && level >= _loggers?[logger.Issuer].Level; + return Handlers != null && level >= _level && level >= _loggers?[logger.Issuer]?.Level; The IsEnabled method could throw a NullReferenceException if _loggers is null or if the logger's issuer type is not found in the dictionary. Add null checks and fallback logic. dotnet/src/webdriver/Internal/Logging/LogContext.cs [104] -return Handlers != null && level >= _level && level >= _loggers?[logger.Issuer].Level; +return Handlers != null && level >= _level && (_loggers?.TryGetValue(logger.Issuer, out var loggerEntry) != true || level >= loggerEntry.Level); Apply this suggestion Suggestion importance[1-10]: 9 Why: The suggestion addresses a critical potential null reference exception in the IsEnabled method. The improved code properly handles dictionary access and null cases, which is essential for runtime stability.	9
	✅ Add null checks during dictionary initialization to prevent potential null reference exceptions Suggestion Impact: The commit addresses null reference concerns by extracting logger initialization into a separate CloneLoggers method with proper null handling code diff: - if (loggers is not null) - { - _loggers = new ConcurrentDictionary<Type, ILogger>(loggers.Select(l => new KeyValuePair<Type, ILogger>(l.Key, new Logger(l.Value.Issuer, level)))); - } + _loggers = CloneLoggers(loggers, level); if (handlers is not null) { @@ -101,7 +99,7 @@ public bool IsEnabled(ILogger logger, LogEventLevel level) { - return Handlers != null && level >= _level && level >= _loggers?[logger.Issuer].Level; + return Handlers != null && level >= _level && (_loggers?.TryGetValue(logger.Issuer, out var loggerEntry) != true || level >= loggerEntry?.Level); } public void EmitMessage(ILogger logger, LogEventLevel level, string message) @@ -158,5 +156,24 @@ Log.CurrentContext = _parentLogContext; } + + [return: NotNullIfNotNull(nameof(loggers))] + private static ConcurrentDictionary<Type, ILogger>? CloneLoggers(ConcurrentDictionary<Type, ILogger>? loggers, LogEventLevel minimumLevel) + { + if (loggers is null) + { + return null; + } + + var cloned = new Dictionary<Type, ILogger>(loggers.Count); + + foreach (KeyValuePair<Type, ILogger> logger in loggers) + { + var clonedLogger = new Logger(logger.Value.Issuer, minimumLevel); + cloned.Add(logger.Key, clonedLogger); + } + + return new ConcurrentDictionary<Type, ILogger>(cloned); + } The logger initialization in constructor could fail if any of the source loggers has a null Issuer. Add validation to handle this case. dotnet/src/webdriver/Internal/Logging/LogContext.cs [51] -_loggers = new ConcurrentDictionary<Type, ILogger>(loggers.Select(l => new KeyValuePair<Type, ILogger>(l.Key, new Logger(l.Value.Issuer, level)))); +_loggers = new ConcurrentDictionary<Type, ILogger>(loggers.Where(l => l.Value?.Issuer != null).Select(l => new KeyValuePair<Type, ILogger>(l.Key, new Logger(l.Value.Issuer, level)))); Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion prevents potential null reference exceptions during logger initialization by adding necessary null checks. This is important for system stability and proper error handling.	8
General	Enhance test coverage by validating log message content and level in addition to count The ContextShouldEmitMessages test only verifies message count but not the actual content or level. Add assertions to validate message content and correct log level. dotnet/test/common/Internal/Logging/LogTest.cs [182-184] -logger.Trace("test message"); +var message = "test message"; +logger.Trace(message); Assert.That(testLogHandler.Events.Count, Is.EqualTo(1)); +Assert.That(testLogHandler.Events[0].Level, Is.EqualTo(LogEventLevel.Trace)); +Assert.That(testLogHandler.Events[0].Message, Is.EqualTo(message)); Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion significantly improves test quality by adding comprehensive assertions that verify not just the count but also the content and level of log messages, ensuring the logging system works as intended.	7

---

Previous suggestions

Suggestions up to commit 029d0d6

Category	Suggestion	Score
General	Enhance test coverage by verifying log event properties beyond just count The test ContextShouldEmitMessages should verify the actual log level and message content, not just the count of events. Add assertions to check the event level is Trace and message content matches. dotnet/test/common/Internal/Logging/LogTest.cs [178-185] [Test] public void ContextShouldEmitMessages() { using var context = Log.CreateContext(LogEventLevel.Trace).Handlers.Add(testLogHandler); logger.Trace("test message"); Assert.That(testLogHandler.Events.Count, Is.EqualTo(1)); + Assert.That(testLogHandler.Events[0].Level, Is.EqualTo(LogEventLevel.Trace)); + Assert.That(testLogHandler.Events[0].Message, Is.EqualTo("test message")); } Suggestion importance[1-10]: 8 Why: The suggestion significantly improves test quality by verifying both the log level and message content, not just the count. This makes the test more robust and helps catch potential issues with log event properties.	8
Possible issue	Add validation to ensure log handler reset operations complete successfully The ResetGlobalLog method should verify the handlers were actually cleared and added successfully. Add assertions or error handling. dotnet/test/common/Internal/Logging/LogTest.cs [31-35] private void ResetGlobalLog() { Log.SetLevel(LogEventLevel.Info); - Log.Handlers.Clear().Handlers.Add(new ConsoleLogHandler()); + var handlers = Log.Handlers.Clear().Handlers; + handlers.Add(new ConsoleLogHandler()); + Assert.That(handlers.Count, Is.EqualTo(1)); + Assert.That(handlers[0], Is.TypeOf<ConsoleLogHandler>()); } Suggestion importance[1-10]: 4 Why: While adding assertions to verify handler reset is helpful for debugging, it's less critical since this is a private helper method used in test setup/teardown. The current implementation is functional even without the additional validation.	4

[qodo-merge-pro]

Persistent review updated to latest commit 77e8a05