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.

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

Conversation

joegoldman2
Copy link
Contributor

Fixes #103208.

I didn't find any unit test for this class, I tested the fix manually.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 9, 2024
@skyoxZ
Copy link
Contributor

skyoxZ commented Jun 9, 2024

Why logger.Value.MinLevel is null? Maybe there's some deeper problem.

@skyoxZ
Copy link
Contributor

skyoxZ commented Jun 10, 2024

The log level in MessageLogger is nullable and when the filters are applied, the result may be a null level

I'm curious if a null level means None, All, or Undefined. Why it was returning Trace.

@@ -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

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.

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

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.

@tarekgh
Copy link
Member

tarekgh commented Jun 11, 2024

/ba-g the failure is already tracked with the issue dotnet/dnceng#3007.

@tarekgh tarekgh merged commit 41d4c46 into dotnet:main Jun 11, 2024
79 of 83 checks passed
@tarekgh tarekgh added this to the 9.0.0 milestone Jun 11, 2024
@joegoldman2 joegoldman2 deleted the fix/103208 branch June 12, 2024 04:35
@github-actions github-actions bot locked and limited conversation to collaborators Jul 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Logging community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LoggerProviderDebugView doesn't seem to report the correct log level
4 participants