-
Notifications
You must be signed in to change notification settings - Fork 246
loggerFactory.WithFilters doesn't work as expected #525
Comments
I wouldn't have expected that to work. What should work is either:
or
|
@justcurious Yeah, well, that's just, like, your opinion, man. a) It is not obvious from the API that one call to AddStuff behaves different than WithFilter. In most fluent builders you modify the original or build a new one. But doing both is kinda... |
@pakrym is this also supposed to be fixed now? |
@HaoK It's just me but from the looks of things with the changes to global filters, yup ;) |
We removed old |
Cool, we should close this issue as fixed in preview 2 then :) |
Fixed as pard of #626 . |
Hi @pakrym , |
@RobertoPrevato what version of the package do you have? AddFilter is on LoggerBuilder ( see sample at https://docs.microsoft.com/en-us/aspnet/core/fundamentals/logging/?tabs=aspnetcore2x#log-filtering) |
@pakrym, thanks for answering. I installed the latest version ("Microsoft.Extensions.Logging.Filter" Version="1.1.2"). I am actually using this package for a NET Core 2.0 console application, not an ASP.NET application; does that make any difference? |
Harsh title, I know, but hey, at least I was surprised and had to look at your code to figure it actually does only create a new FilteredLoggerFactory instance ;)
Given the following code
All looks fine right?
Expected result would be no log message because the filter should filter away the info message.
Unfortunately,
WithFilter
only creates a new logger factory and in case you don't use that one, the messages do not get filtered.This also applies to use cases where you want to add filtering to in your Startup (Configure) method
This also does nothing of course.
This is not really a very critial issue because you can still inject/replace the ILoggerFactory during
ConfigureServices
, but still, it is not very friendly ;)I guess it would be better if the base logger factory would know about the filters and WithFilters just adds those.
The text was updated successfully, but these errors were encountered: