serilog / serilog-extensions-logging

Serilog provider for Microsoft.Extensions.Logging
Apache License 2.0
313 stars 100 forks source link

Please respect Microsoft Log Filtering #130

Closed AceHack closed 5 years ago

AceHack commented 5 years ago

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/logging/?view=aspnetcore-2.1#log-filtering

Currently, Serilog completely ignores Microsoft log filtering.

For example, the config below is respected using Microsoft's console logger but Ignored using serilog.

  "Logging": {
    "LogLevel": {
      "Microsoft": "Warning",
      "System": "Warning",
      "Default": "Warning"
    }
  },
nblumhardt commented 5 years ago

Hi Aaron, thanks for the note; it's a reasonable request but doesn't fit with the overall direction of the Serilog project at this point. Replying here to your comment on #98 also as that thread is closed.

Serilog's a complete logging system, it's not just a plug-in for MEL. Trying to make Serilog fill the gaps in MEL is a non-goal of the project.

MEL version 2.x added many features that overlap with Serilog's, and after looking at options for integrating in a fine-grained way, we realised that this would create a more complex, inefficient, and fragile Serilog/MEL integration. Hence the decision to pursue plugging in at a higher level as an alternative to MEL via UseSerilog(), under the Microsoft.Extensions.Logging.Abstractions interfaces like ILogger.

Although this doesn't then support MEL+Serilog seamlessly, it does mean that when using MEL, you get a consistent, efficient logging implementation, and when using Serilog, you get the same. It's not great having to make a choice, but, whichever of the two options you choose, you should get a clean, efficient, understandable logging pipeline (rather than MEL vs a mish-mash of MEL+Serilog bits).

Hope this makes sense, and you're able to figure out a viable way forwards.

AceHack commented 5 years ago

@nblumhardt I do hear what you are saying but it does make the use case of using serilog as only a sink for MEL basically a non-starter. The direction of this project does seem like pre .NET core thinking in my opinion, especially pre .NET core 2.0 thinking. @davidfowl I'm curious on your thoughts of serilog basically taking over and making normal MEL stuff break, like MEL configuration, other MEL log sinks, etc... It basically takes over ILoggerFactory and just does things completely differently.

nblumhardt commented 5 years ago

it does make the use case of using serilog as only a sink for MEL basically a non-starter.

👍 this is exactly what I'm saying 😄 - we don't intend to support this scenario.

MEL is a fine logging framework to get up and running with, but it's only a slice of what Serilog (and NLog) offers. The project has bigger goals than being a repository of sinks for MEL, and by focusing on the end-to-end scenario we keep Serilog (and the whole ecosystem) healthier.

davidfowl commented 5 years ago

I think to play nicely with MEL, Serilog should maintain both an ILoggerProvider and ILoggerFactory. I don't want to get philosophical but overriding the ILoggerFactory does have big downsides that @nblumhardt has chosen to deal with as a library author, namely the fact that ILoggerFactory has an AddProvider and the fact that ILoggerProviders can be added via DI makes it a little less cooperative to take over the factory.

That said, as long as there's an option to plug in an ILoggerProvider, that should be the thing to use (unless there's some obvious downside I'm missing)

AceHack commented 5 years ago

@davidfowl Would you expect any ILoggerProvider to respect things like MEL log filtering?

davidfowl commented 5 years ago

Yes. Though there should be a way for the ILoggerProvider to opt-out if it has its own built in filtering.

nblumhardt commented 5 years ago

I think to play nicely with MEL, Serilog should maintain both an ILoggerProvider and ILoggerFactory.

We currently do 👍

as long as there's an option to plug in an ILoggerProvider, that should be the thing to use (unless there's some obvious downside I'm missing)

Sounds good on the surface, but is miserable to really work with. MEL v2 isn't geared towards providers being full logging frameworks with configuration, level control, filtering, contextual enrichment, etc. etc., and so if you try to use these it's a confusing mess.

davidfowl commented 5 years ago

Sounds good on the surface, but is miserable to really work with. MEL v2 isn't geared towards providers being full logging frameworks with configuration, level control, filtering, contextual enrichment, etc. etc., and so if you try to use these it's a confusing mess.

I believe it. We tried to add more consistency across the lower level logger providers and may have made it more painful for providers that have a full feature set like serilog and nlog etc.

cc @pakrym

nblumhardt commented 5 years ago

@glennc and I spent a while trying to come up with a good direction for this last year but didn't reach a conclusion. It'd be great to take a fresh look at it, but there are peripheral issues around things like documentation that I don't have good answers for: directing users to the right APIs becomes complicated depending on where Serilog is running, for example.

Implementing ILoggerFactory.AddProvider() on the Serilog side, somehow, could make a measurable improvement, but I haven't managed to spike anything out, yet - what do you think?

pakrym commented 5 years ago

I think most features we've added are opt-in except for the filtering and that one is easy to opt-out of. We also made improvements in 3.0 to decrease the overhead of built-in Logger implementation.

@nblumhardt Do you remember anything else that made it hard from MEL architecture point of view?

nblumhardt commented 5 years ago

Hey @pakrym 👋 thanks for the note.

The big picture needs attention ahead of any of the APIs or architecture details.

As you know, people care a lot about logging being understandable, predictable, reliable, and efficient.

Taking the ILoggerProvider route with Serilog in v2, when you look at it from a user point of view, just isn't that great. Using two logging frameworks, with two sets of level control, configuration, filtering, etc. is a bit silly - bearable, if you have to do it in order to get access to sinks, but it's not something you'd ever choose given any alternative. The whole thing looks and feels janky.

Implementing ILoggerFactory is just the least-bad option, right now. It breaks some odd integrations here and there, but the core of the approach is solid: you just use Serilog (as you chose to) and let the framework consume it through the MEL APIs like ILogger. There's one set of levels, filters, configuration, etc, and everything works pretty nicely.

To break out of this we need someone at Microsoft to really value the Serilog/NLog/other alternative logging framework scenario. Not just to make it mechanically possible, but to make it a great experience, free of non-starter compromises, and to ask the questions to figure out what those are. If the will is there, someone needs to own it, figure out what a really workable system looks like, and drive the thing. There's heaps of goodwill and energy in the Serilog project to help make that happen, but no way for us to move it forward unless someone on the Microsoft end is really serious about taking the lead.

Maybe the key is to improve ILoggerFactory so that alternative implementations don't break other consumers?

No one is motivated to spend time and effort on something that will ultimately be a poor experience that's difficult to debug, explain, document, and support, but if someone can present a vision for a great Serilog + ASP.NET Core experience, I'm pretty sure it will get traction over here.

AceHack commented 5 years ago

@nblumhardt What I'm asking for is better integration with ILoggerProvider like having your ILoggerProvider respect MEL filters as this is what this is asking for. I don't want to use a 3rd party ILoggerFactory, I want to use the one built into MEL. My vote is for improving ILoggerProvider.

davidfowl commented 5 years ago

To break out of this we need someone at Microsoft to really value the Serilog/NLog/other alternative logging framework scenario. Not just to make it mechanically possible, but to make it a great experience, free of non-starter compromises, and to ask the questions to figure out what those are. If the will is there, someone needs to own it, figure out what a really workable system looks like, and drive the thing. There's heaps of goodwill and energy in the Serilog project to help make that happen, but no way for us to move it forward unless someone on the Microsoft end is really serious about taking the lead.

This is extremely loaded and really adds nothing to the conversation. Instead, I suggest we focus on specifics instead of broad generalizations like this. You’re talking to the people responsible for this logging library and if we didn’t care about the experience, we wouldn’t respond. Heck we even used Serilog in some of the providers we shipped in the past. So it’s not fair to claim that “Microsoft” doesn’t care about 3rd party loggers, it’s just untrue. Like you we have lots of priorities and limited resources. If it looks like we don’t care, it’s likely because we’re working something deemed higher priority at the moment.

I personally spent time trying to figure out how if serilog and NLog should indeed be an ILoggerProvider or and ILoggerFactory (and we even looked at removing AddProvider but it was too big of a breaking change to stomach).

So here’s what we’re left with. If seilog continues to promote replacing the factory the I suggest it take over all of the functionality to enable having side by side MEL loggers. It’s the only way I see it not breaking compatibility with how things are currently being approached.

nblumhardt commented 5 years ago

Hey @davidfowl - I'm very sorry the wording of my reply came over poorly. I didn't intend to imply that no one at Microsoft cares or has spent effort on this, which I am aware is far from the case. It's not a personal criticism: the MEL v2 APIs that we're discussing weren't built with Serilog's scenario at the center - standardization across providers, for example, was definitely higher on the list, and as you suggest, it led to the experience of using Serilog with MEL being compromised. Getting the Serilog scenarios higher on the list does seem to need some tenacious championing from somewhere :-)

Your team has limited resources - I totally understand, and I'm enjoying all of the other great stuff you've been shipping - but the Serilog project has just about none by comparison, and we're not close enough to the decision-making to be able to solve this problem in a better way than what we've managed ourselves with ILoggerFactory.

Replacing ILoggerFactory is the only approach that maintains a good user experience. We all want clean, straightforward, bloat-free infrastructure when we set up projects. Actively recommending against it, when the alternative is a glued-together mix of the two frameworks, makes Serilog look bad and leaves the problem on our shoulders. It's rock/hard place at this end :-)

All I'm trying to say is, if ILoggerFactory is going to remain the best way for Serilog to have a first-class ASP.NET Core story, we need your support behind it. I agree with your conclusion:

If Serilog continues to promote replacing the factory the I suggest it take over all of the functionality to enable having side by side MEL loggers.

But I don't want to do this out on a limb - let's figure out together how to make that approach awesome, it surely has to be possible.

Hope this comes across better, it's the best wording I can manage; I've hit serious GitHub-issue-comment-fatigue, I can only imagine how badly you must also suffer that. Thanks for the reply!

nblumhardt commented 5 years ago

Hey all! @davidfowl, @pakrym - hope you're kicking off a great Friday (or winding down after a great Thursday, or whatever day it is where you happen to be).

I've spiked out https://github.com/serilog/serilog-extensions-logging/pull/132

It's patchy, skips over a heap of functionality, has no tests, and ignores perf... But - it might be a start?

nblumhardt commented 5 years ago

Maybe grace requires that I stop flogging a dead horse 😅

I'll keep working on #132 if anyone is interested in trying to improve our current ILoggerFactory with me, but I'll also lend what support I can if anyone is keen to extend our ILoggerProvider to be a "better citizen" in MEL as envisaged by the team behind it.

Closing now in the interests of conserving everyone's time and energy; I'll post here if I manage to get some fresh perspective on the problem, so please also feel free to chime in below if you have anything to add. All channels open, too, if anyone's keen to chat about this offline. 👋

AceHack commented 5 years ago

So is this closed with a will do? Or won’t do? If will do then do you have a timeline? Thanks.

adamchester commented 5 years ago

@AceHack I think it’s closed with a “doing the best we can”.

Please do continue the discussion here or #132 👍

Based on comments above, we don’t have a timeline yet.