serilog / serilog-extensions-logging

Serilog provider for Microsoft.Extensions.Logging
Apache License 2.0
309 stars 99 forks source link

Consider making the AddFilter line optional #114

Closed epignosisx closed 5 years ago

epignosisx commented 6 years ago

Please consider making the AddFilter line here optional.

This is getting in the way of using the new filtering in MEL 2.0 for those of us that would prefer to go with that option over Serilog's for filtering log entries. Consider the following setup in a Program.cs:


Log.Logger = new LoggerConfiguration()
                .AddConfiguration(configuration) //some other settings not important for this issue
                .MinimumLevel.Verbose(); // set to lowest to rely on MEL filtering

var host = new WebHostBuilder()
    .UseKestrel()
    .UseContentRoot(Directory.GetCurrentDirectory())
    .UseIISIntegration()
    .ConfigureLogging((hostingContext, builder) => {
        builder.AddFilter(LoggerDynamicFiltering.Filter) //filters based on category & level for all logging providers
                  .AddSerilog(Log.Logger, false);
    })
    .UseStartup<Startup>()
    .Build();

Due to the algorithm behind MEL LoggerRuleSelector

// Filter rule selection:
// 1. Select rules for current logger type, if there is none, select ones without logger type specified
// 2. Select rules with longest matching categories
// 3. If there nothing matched by category take all rules without category
// 3. If there is only one rule use it's level and filter
// 4. If there are multiple rules use last
// 5. If there are no applicable rules use global minimal level

The Filter added by Serilog wins over my Filter. (This just caused me a whole afternoon of debugging to figure it out). Granted, the solution is simple once we understand how Serilog & MEL work (order is important):

.ConfigureLogging((hostingContext, builder) => {
    builder.AddSerilog(Log.Logger, false)
           .AddFilter<SerilogLoggerProvider>(LoggerDynamicFiltering.Filter);

})

However, it would have been more of a pain if I had more than one provider, since then I would need to have a catch-all filter for the other providers.

nblumhardt commented 6 years ago

Thanks for the suggestion, we can definitely take a look at it. Even with the switch to toggle the behavior, it's not particularly discoverable.

The newer Serilog.AspNetCore side-steps the whole issue of how to make a logging framework work within a logging framework - it's more efficient and overall a nicer experience if you have the opportunity to switch approaches.

epignosisx commented 6 years ago

Totally agree that it is not discoverable to have an extra parameter. In fact, I couldn’t come up with a good name for the parameter to recommend that would be self-documenting.

Before I arrived to the workaround I was using the Serilog.AspNetCore package, but it suffered of the same issue.

The reason I went with the MEL filtering is because they provide the ability to pass a “filtering function” based on category and level. This allows for more flexible options like being able to reload the filtering rules from say a DB. As far as I know, Serilog does not provide this flexibility, It has the LoggingLevelSwitch but only allows to filter based on Log Level, it does not take into consideration the Category, which is very helpful for large system where just going all the way down to Verbose will generate a very large number of unwanted entries.

tsimbalar commented 6 years ago

@epignosisx Actually, there is LoggingLevelSwitch's cousin LoggingFilterSwitch that lives in Serilog.Filters.Expressions and would probably meet your needs. This article by @nblumhardt is a great introduction : https://nblumhardt.com/2017/10/logging-filter-switch/

nblumhardt commented 6 years ago

I was using the Serilog.AspNetCore package, but it suffered of the same issue

Unfortunately, the Serilog.AspNetCore package depends on Serilog.Extensions.Logging, so it's possible to use the older API and still hit the issue. If you switch to UseSerilog() instead of AddSerilog(), the issue will not be present.

HTH, Nick