Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Remove ILoggerFactory.MinimumLevel #298

Closed
Eilon opened this issue Nov 20, 2015 · 11 comments
Closed

Remove ILoggerFactory.MinimumLevel #298

Eilon opened this issue Nov 20, 2015 · 11 comments
Assignees
Milestone

Comments

@Eilon
Copy link
Member

Eilon commented Nov 20, 2015

This property has two things going against it:

  1. Everyone thinks it does something other than what it does. (This can be mitigated by a rename of some sort.)
  2. It's not needed anymore. It was added to avoid sensitive information being logged inadvertently. This was later decided to be a non-issue.

This is teh codez: https://github.com/aspnet/Logging/blob/dev/src/Microsoft.Extensions.Logging.Abstractions/ILoggerFactory.cs#L14-L17

@lodejard @muratg @DamianEdwards

@lodejard
Copy link
Contributor

👍 this should have been removed a while ago!

@muratg
Copy link

muratg commented Nov 20, 2015

Agreed!

@JunTaoLuo 🎁

@JunTaoLuo
Copy link
Contributor

merged to dev

@joeaudette
Copy link

so is there a new way to set the loglevel?

@Eilon
Copy link
Member Author

Eilon commented Dec 3, 2015

@joeaudette the thing is, this never set the log level. That's why we're removing it. It just sets the minimum allowed log level. Each individual logger (e.g. DebugLogger, ConsoleLogger, EventLogLogger, SerilogLogger, etc.) has their own configuration and settings to decide what they track.

@joeaudette
Copy link

that makes sense, Thanks!

@joeaudette
Copy link

just to help others since breaking change announcements usually provide the old and the new way of doing things.
old way:

loggerFactory.MinimumLevel = LogLevel.Warning;
loggerFactory.AddConsole();

new way:

loggerFactory.AddConsole(minLevel: LogLevel.Warning);

@JunTaoLuo
Copy link
Contributor

@joeaudette thanks for the suggestion.

@sunsided
Copy link
Contributor

sunsided commented Dec 4, 2015

@joeaudette that makes things clearer indeed.

@Eilon
Copy link
Member Author

Eilon commented Dec 4, 2015

BTW, you always needed loggerFactory.AddConsole(minLevel: LogLevel.Warning); to get the right behavior. So in theory a properly-configured app should just remove the call to loggerFactory.MinimumLevel, but everyone was confused, so it was more complicated 😄

@joeaudette
Copy link

I'm sure that is true but some people might have thought it was working if the default level happened to correspond to the factory setting even though that didn't do anything. Illustrating the proper way to configure logging is the way to clear up confusion.

Myself I had implemented a custom database logger and provider with my own factory extension that did use the loglevel setting of the factory. So the announcement just saying that setting was removed because it didn't do anything left me initially confused.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants