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

Unintuitive: DebugLoggerFactoryExtensions.AddDebug defaults to LogLevel.Information #275

Closed
ashimoon opened this Issue Oct 25, 2015 · 11 comments

Comments

Projects
None yet
4 participants
@ashimoon

ashimoon commented Oct 25, 2015

This was a bit of a head scratcher, I added a Debug logger to my beta8 project using the AddDebug extension method here:

https://github.com/aspnet/Logging/blob/dev/src/Microsoft.Extensions.Logging.Debug/DebugLoggerFactoryExtensions.cs#L18

However, my log statements weren't being shown in the Debug output window. When I dug into the source I saw that the default log level for AddDebug is... LogLevel.Information.

Is this intentional? Shouldn't the default be LogLevel.Debug?

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Oct 28, 2015

Member

The logger providers are named based on where they write information to (e.g. console, Windows Event Log, etc.). The Debug logger provider writes information to the Debug output window. It is unrelated to the level of detail that is flowed to it, which is separately configurable.

Member

Eilon commented Oct 28, 2015

The logger providers are named based on where they write information to (e.g. console, Windows Event Log, etc.). The Debug logger provider writes information to the Debug output window. It is unrelated to the level of detail that is flowed to it, which is separately configurable.

@ashimoon

This comment has been minimized.

Show comment
Hide comment
@ashimoon

ashimoon Oct 28, 2015

@Eilon Thanks for the response, I think I get it - providers have configurable logging levels independent of logger logging levels, right?

However in the case of the Debug output window, what are your thoughts on the default logging level for that provider? I'm trying to approach this from the viewpoint of a newcomer to ASP.NET 5 - they want logging, they add a Debug logger, and then add a bunch of LogDebug statements and see nothing.

This is what happened to me anyways, and luckily I'm the type of guy that will dig into the source code to see what's happening and I figured it out pretty quickly, but I'm not sure it's going to be the most friendly experience for most users. Maybe I'm overthinking it, what do you think?

Can you give me a bit of context on how LogLevel.Information was chosen as the default log level for the Debug provider maybe? I bet there's a piece I'm missing here?

ashimoon commented Oct 28, 2015

@Eilon Thanks for the response, I think I get it - providers have configurable logging levels independent of logger logging levels, right?

However in the case of the Debug output window, what are your thoughts on the default logging level for that provider? I'm trying to approach this from the viewpoint of a newcomer to ASP.NET 5 - they want logging, they add a Debug logger, and then add a bunch of LogDebug statements and see nothing.

This is what happened to me anyways, and luckily I'm the type of guy that will dig into the source code to see what's happening and I figured it out pretty quickly, but I'm not sure it's going to be the most friendly experience for most users. Maybe I'm overthinking it, what do you think?

Can you give me a bit of context on how LogLevel.Information was chosen as the default log level for the Debug provider maybe? I bet there's a piece I'm missing here?

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Oct 28, 2015

Member

Yeah I do agree it can be a bit confusing - and you're not the first one to feel some confusion around it 😄

The reason for Information being the default is that as a whole we decided that the frameworks (ASP.NET + EF) should emit about ~10 log messages at the Information level or higher on a "typical" request (started, some DB queries, routing, MVC, view rendering, etc.).

If the default was Debug or Verbose then a typical request would have hundreds of log messages flowing through, and that would probably make the log output unusable in most cases.

Member

Eilon commented Oct 28, 2015

Yeah I do agree it can be a bit confusing - and you're not the first one to feel some confusion around it 😄

The reason for Information being the default is that as a whole we decided that the frameworks (ASP.NET + EF) should emit about ~10 log messages at the Information level or higher on a "typical" request (started, some DB queries, routing, MVC, view rendering, etc.).

If the default was Debug or Verbose then a typical request would have hundreds of log messages flowing through, and that would probably make the log output unusable in most cases.

@ashimoon

This comment has been minimized.

Show comment
Hide comment
@ashimoon

ashimoon Oct 28, 2015

@Eilon Okay that makes sense. I would agree with that. Also once a new user encounters this issue one time they will probably understand it pretty well going forward.

I'm still wondering if you have any thoughts on how it can be easier for newcomers? I'm thinking specifically about this piece of code that gets scaffolded automatically in a new ASP.NET 5 project:

loggerFactory.MinimumLevel = LogLevel.Information;
loggerFactory.AddConsole();
loggerFactory.AddDebug();

I ended up changing it to:

loggerFactory.MinimumLevel = LogLevel.Debug;
loggerFactory.AddConsole(LogLevel.Debug);
loggerFactory.AddDebug(LogLevel.Debug);

When I look at the updated block of code, I see immediately that there are provider vs. logger log levels. Before, it wasn't entirely clear what loggerFactory.MinimumLevel was for.

I'd be really curious to hear your thoughts on that since I would bet a ton of people will be adopting ASP.NET 5 for the first time since it is awesome sauce, and they will use the default template as a starting point for learning probably!

Is there a good way you can think of to make it obvious to new users that they can do something like this?

ashimoon commented Oct 28, 2015

@Eilon Okay that makes sense. I would agree with that. Also once a new user encounters this issue one time they will probably understand it pretty well going forward.

I'm still wondering if you have any thoughts on how it can be easier for newcomers? I'm thinking specifically about this piece of code that gets scaffolded automatically in a new ASP.NET 5 project:

loggerFactory.MinimumLevel = LogLevel.Information;
loggerFactory.AddConsole();
loggerFactory.AddDebug();

I ended up changing it to:

loggerFactory.MinimumLevel = LogLevel.Debug;
loggerFactory.AddConsole(LogLevel.Debug);
loggerFactory.AddDebug(LogLevel.Debug);

When I look at the updated block of code, I see immediately that there are provider vs. logger log levels. Before, it wasn't entirely clear what loggerFactory.MinimumLevel was for.

I'd be really curious to hear your thoughts on that since I would bet a ton of people will be adopting ASP.NET 5 for the first time since it is awesome sauce, and they will use the default template as a starting point for learning probably!

Is there a good way you can think of to make it obvious to new users that they can do something like this?

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Oct 28, 2015

Member

Well, for one, we're considering removing the MinimumLevel option to begin with - that should help reduce some confusion.

Beyond that, perhaps by changing the templates to say something like:

loggerFactory.AddConsole(minLevel: LogLevel.Information);
loggerFactory.AddDebug(minLevel: LogLevel.Information);

Would make it a bit clearer what that option is saying.

Member

Eilon commented Oct 28, 2015

Well, for one, we're considering removing the MinimumLevel option to begin with - that should help reduce some confusion.

Beyond that, perhaps by changing the templates to say something like:

loggerFactory.AddConsole(minLevel: LogLevel.Information);
loggerFactory.AddDebug(minLevel: LogLevel.Information);

Would make it a bit clearer what that option is saying.

@ashimoon

This comment has been minimized.

Show comment
Hide comment
@ashimoon

ashimoon Nov 4, 2015

Interesting, looks like you guys are adding some Configuration integration for Logging. Looks pretty cool. I see that the default logging level is now Verbose for non System/Microsoft loggers right? That might help a lot.

I also read in another issue that you guys are considering merging Debug and Verbose? Is that why you chose to use Verbose as the new default log level?

ashimoon commented Nov 4, 2015

Interesting, looks like you guys are adding some Configuration integration for Logging. Looks pretty cool. I see that the default logging level is now Verbose for non System/Microsoft loggers right? That might help a lot.

I also read in another issue that you guys are considering merging Debug and Verbose? Is that why you chose to use Verbose as the new default log level?

@Eilon

This comment has been minimized.

Show comment
Hide comment
@Eilon

Eilon Nov 4, 2015

Member

BTW the defaults themselves haven't changed per se, but the project templates will set some particular log levels.

Member

Eilon commented Nov 4, 2015

BTW the defaults themselves haven't changed per se, but the project templates will set some particular log levels.

@ashimoon

This comment has been minimized.

Show comment
Hide comment
@ashimoon

ashimoon Nov 4, 2015

Right, yes that is more accurate.

notice that the ILoggerFactory minimum level was removed from the template (web app) as well but it seems that the property still exists. Is that to discourage people to use that property?

ashimoon commented Nov 4, 2015

Right, yes that is more accurate.

notice that the ILoggerFactory minimum level was removed from the template (web app) as well but it seems that the property still exists. Is that to discourage people to use that property?

@muratg

This comment has been minimized.

Show comment
Hide comment
@muratg

muratg Dec 18, 2015

Member

MinimumLevel was removed as part of this: #298

Closing this per the discussion above.

Member

muratg commented Dec 18, 2015

MinimumLevel was removed as part of this: #298

Closing this per the discussion above.

@muratg muratg closed this Dec 18, 2015

@ashimoon

This comment has been minimized.

Show comment
Hide comment
@ashimoon

ashimoon Dec 19, 2015

I noticed that, Latest version of logging looks really nice and intuitive. Good stuff guys, thanks for following up.

ashimoon commented Dec 19, 2015

I noticed that, Latest version of logging looks really nice and intuitive. Good stuff guys, thanks for following up.

@MNF

This comment has been minimized.

Show comment
Hide comment
@MNF

MNF May 7, 2017

@muratg, I do not understand why removing MinimumLevel property made AddDebug default less confusing.
I suggest

  1. to mark loggerFactory.AddDebug(); without parameters as obsolete, explaining that recommended use is
    loggerFactory.AddDebug(minLevel: LogLevel.Information);
  2. In AddDebug(this ILoggerFactory factory, LogLevel minLevel) Summary explain that LogLevel.Debug can be too verbose and LogLevel.Information is recommended (as Eilon commented on Oct 29 2015)

Can you re-open the issue or prefer to raise a new one?

MNF commented May 7, 2017

@muratg, I do not understand why removing MinimumLevel property made AddDebug default less confusing.
I suggest

  1. to mark loggerFactory.AddDebug(); without parameters as obsolete, explaining that recommended use is
    loggerFactory.AddDebug(minLevel: LogLevel.Information);
  2. In AddDebug(this ILoggerFactory factory, LogLevel minLevel) Summary explain that LogLevel.Debug can be too verbose and LogLevel.Information is recommended (as Eilon commented on Oct 29 2015)

Can you re-open the issue or prefer to raise a new one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment