Skip to content
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.

Sign up for GitHub

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

Fix LoggerProviderDebugView.CalculateEnabledLogLevel when the level is None #103209

Merged
merged 3 commits into from
Jun 11, 2024
Merged

Fix LoggerProviderDebugView.CalculateEnabledLogLevel when the level is None #103209

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de4d75a
Fix LoggerProviderDebugView.CalculateEnabledLogLevel when the level i…
joegoldman2 Jun 9, 2024
04df767
Apply suggestion from review
joegoldman2 Jun 10, 2024
015c5a5
Update src/libraries/Microsoft.Extensions.Logging/src/Logger.cs
tarekgh Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/libraries/Microsoft.Extensions.Logging/src/Logger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,11 @@ private sealed class LoggerProviderDebugView(string providerName, MessageLogger?
return null;
}

if (!logger.Value.MinLevel.HasValue)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right fix.

The fix should change the line 181:

MessageLogger? messageLogger = logger.MessageLoggers?.FirstOrDefault(messageLogger => messageLogger.Logger == loggerInfo.Logger);

To:

            MessageLogger? messageLogger = FirstOrNull(logger.MessageLoggers, loggerInfo.Logger);

            MessageLogger? FirstOrNull(MessageLogger[]? array, ILogger logger)
            {
                if (array is null || array.Length == 0)
                {
                    return null;
                }

                foreach (var item in array)
                {
                    if (item.Logger == logger)
                    {
                        return item;
                    }
                }

                return null;
            }

Copy link
Member

@tarekgh tarekgh Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To give some more details, in the failing example, logger.MessageLoggers will be empty array. The line:

MessageLogger? messageLogger = logger.MessageLoggers?.FirstOrDefault(messageLogger => messageLogger.Logger == loggerInfo.Logger);

will return messageLogger equal to the default. MessageLogger is a value type so the returned value will never be null and always has HasValue = true. Then we do new LoggerProviderDebugView(providerName, messageLogger) passing the default value. The LoggerProviderDebugView.CalculateEnabledLogLevel will do the check:

            private static LogLevel? CalculateEnabledLogLevel(MessageLogger? logger)
            {
                if (logger == null)
                {
                    return null;
                }

which will never be true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @JamesNK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @tarekgh. I didn't notice that MessageLogger was a struct, so FirstOrDefault won't return null. I applied and tested your suggestion.

Copy link
Member

@JamesNK JamesNK Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also do this:

MessageLogger? messageLogger = logger
    .MessageLoggers
    ?.Cast<MessageLogger?>()
    .FirstOrDefault(messageLogger => messageLogger.Logger == loggerInfo.Logger);

But adding the new method is fine. It avoids some struct generics + LINQ.

{
return LogLevel.None;
}

ReadOnlySpan<LogLevel> logLevels = stackalloc LogLevel[]
{
LogLevel.Critical,
Expand Down
Loading