serilog / serilog-extensions-logging

Serilog provider for Microsoft.Extensions.Logging
Apache License 2.0
307 stars 97 forks source link

Question/Discussion: Why was 'SerilogLoggerFactory' implemented? #183

Closed julealgon closed 3 years ago

julealgon commented 3 years ago

As I dig deeper and deeper into the logging infrastructure from Microsoft.Extensions.Logging, I end up finding more stuff that puzzles me about the way Serilog integrates with it.

I've now realized that Serilog has it's own implementation of ILoggerFactory, even though Microsoft's recommended approach for implementing custom loggers is to implement ILoggerProvider only.

I'd like to understand why the team decided to create a custom ILoggerFactory implementation that replaces the one from Microsoft.Extensions. As a result, the Serilog API becomes unwieldy, having multiple registration methods each doing a different registration approach: one adds a custom ILoggerProvider, another adds a custom ILoggerFactory which completely ignores the providers, etc.

I only realized this after consuming ILogger<T> explicitly in one of my classes and realizing it wasn't logging anything, even though the request logging on the API was working fine and writing to the console... turned out I was using AddSerilog on the ILoggerBuilder, which replaces the ILoggerProviders but keeps the original ILoggerFactory. Due to a misunderstanding on my part, I had removed the logger configuration flags from my appsettings.config file, which appear to be read by the original factory implementation. On the other hand, the request logging uses Serilog's ILogger directly and completely bypasses Microsoft.Extensions, which is why it was working in my scenario.

I understand this may sound like a weird question, but I'm just frustrated as a consumer of the library to always stumble upon these weird registration quirks on every project I leverage Serilog. The more I find out about them, the more I want to stay away from them to be honest and rely as much as I can on the significantly cleaner Microsoft.Extensions interfaces. As a consumer, it seems like Serilog want's to purposefully create integrations that encourage people not to use Microsoft's standard interface. I just don't quite understand that approach, do you think the native interface is flawed in any way that would justify this approach?

Sorry about the rant... please take that as negative feedback from me from a user experience point of view (even being a fan of Serilog as a library).

nblumhardt commented 3 years ago

Hey Juliano! Thanks for the notes, well-considered feedback is always welcome :+1:

Serilog doesn't want to steer clear of the MEL interfaces, but does want to avoid the MEL implementation of those interfaces because the implementation duplicates most of what Serilog is/does, and this leads to a worse overall experience, in my opinion.

There were some discussions way back around the time of this change, but you may have to go digging around on GitHub to find the original threads.

Cheers Nick

julealgon commented 3 years ago

Hey Nick

...but does want to avoid the MEL implementation of those interfaces because the implementation duplicates most of what Serilog is/does

Wouldn't one be able to argue the opposite here? That Serilog is duplicating most of what MEL does (in the factory)?

...and this leads to a worse overall experience, in my opinion.

I guess the question then becomes: what extra features is Serilog providing with the custom factory implementation that is not available in MEL?

There were some discussions way back around the time of this change, but you may have to go digging around on GitHub to find the original threads.

Got it. Yeah, I realize this custom factory implementation probably dates from way back then. Burden is on me for not properly understanding the whole flow until yesterday.

sungam3r commented 3 years ago

Wouldn't one be able to argue the opposite here? That Serilog is duplicating most of what MEL does (in the factory)?

Most likely the answer is simple. Serilog appeared before MEL. Therefore, another question is who duplicates whom 😄

MEL: изображение

Serilog: Initial commit Nicholas Blumhardt nblumhardt@nblumhardt.com 5bb93d5625beb18d5b5046ff0f9443fefd514d3f 14.02.2013 15:44:04

sungam3r commented 3 years ago

I would say the problem (if it can be called a problem at all) is choosing the primary abstraction. It can be an internal for the package (native) interface or an external one. In the second case, we get external dependency with all its shortcomings. In the first case, adapter classes to the external abstraction will inevitably arise (SerilogLoggerFactory). Personally I don't think Serilog core project should use MEL. Also see https://github.com/serilog/serilog/issues/1358

julealgon commented 3 years ago

Most likely the answer is simple. Serilog appeared before MEL. Therefore, another question is who duplicates whom 😄

The point I was making (while ignoring the dates purposefully) is that the libraries are supposed to conform to the API from the framework, even if it is created later IMHO. In that sense, it would be more productive if Serilog stopped writing new custom interfaces and implementations and leveraged those from MEL.

It would be pretty cool if Serilog, with its significant userbase, could reach out to Microsoft to improve on the MEL abstractions as needed and them leverage them, instead of creating things from scratch or reinventing them. Obviously, this is just my opinion here.

Personally I don't think Serilog core project should use MEL.

That's fine I guess.

I think I disagree in this case since the purpose of MEL is to be the definitive logging abstraction for .NET, so staying away from it creates unnecessary/duplicate/confusing abstractions and complications in my view, while also incentivizing people to use Serilog's own abstractions in a few places, dividing the community (it is very common to find people asking questions about Serilog and then seeing the Serilog interfaces all over their code). This is mostly due to how Serilog tutorials talk about it: they always seem to encourage people to use Serilog interfaces instead of MEL interfaces, which I find unfortunate.

Also see serilog/serilog#1358

Thanks for the link. As you can imagine, such discussion wouldn't even be needed if one was in favor of reusing MEL abstractions instead of creating custom ones, as there is already MEL.Abstractions package to deal exactly with that.

sungam3r commented 3 years ago

I think I disagree in this case since the purpose of MEL is to be the definitive logging abstraction for .NET

I'll only go along with that when it's called something like System.Logging.Abstractions. Until then it cannot be considered as definitive logging abstraction for .NET. It's Microsoft's abstraction for their codebase. Yes, I understand that it is ubiquitous but this is not a decisive argument for me. Although I have to admit that this is what happens in practice - there is sometimes no better solution than to actually use MEL as the base abstraction for logging.

nblumhardt commented 3 years ago

Good discussion; we could definitely spend many more hours on it :-)

Wouldn't one be able to argue the opposite here? That Serilog is duplicating most of what MEL does (in the factory)?

MEL was built in two parts; "interface MEL" came first (at v1, without a substantial implementation), while "implementation MEL" came afterwards (at v2, adding configuration, filters, a leveling system ...).

Interface MEL and Serilog's interfaces are just about equivalent. I prefer Serilog's interfaces, but the MEL ones are fine and if we chose to, we could implement them directly in Serilog without losing much.

Implementation MEL and Serilog is a different story, though. They're both logging libraries, in the same way that EntityFramework and NHibernate are both ORMs, or ASP.NET and ASP.NET MVC are both web frameworks. They have overlapping use cases, but the way they work is completely and utterly different. Trying to combine them, or more deeply-support one within the other, would be a confusing mess.

I guess the question then becomes: what extra features is Serilog providing with the custom factory implementation that is not available in MEL?

An obvious one might be the @ structure-capturing operator.

The ecosystem around is another, and it's more of a consequence of Serilog's design than might first appear. Serilog is built around a model of log events that makes it easy to construct plug-in components like enrichers, sinks, filters, scripting, and so-on. MEL is set up to squeeze more performance out of its core scenarios, and as a side-effect, these features are absent or not central to how MEL is used.

Choice is good :-)

sungam3r commented 3 years ago

Good explanation, agree with @nblumhardt . In any case, there is absolutely no problem whatsoever how these two ecosystems (MEL <-> Serilog) fit together. You can do this in different ways. Now this is done as it is. If you do it differently, then it will not add much value.

julealgon commented 3 years ago

If you do it differently, then it will not add much value.

@sungam3r I never said the libraries should be the same/do the same thing. My whole point with this issue was specifically with the factory implementation, which is the outside component that instantiates the loggers. My understanding is that most of the power of Serilog is in the ILogger implementation and everything that comes with it, not so much on the factory.

Of course, if everybody implements the same thing then sure, everybody would agree there is zero value there.

Choice is good :-)

Nick, not going to quote everything, you make good points. Just wanted to comment on this general statement here. Usually, I don't disagree, however with Serilog I've come to realize that the API gets more and more confusing the more you "learn" about it and use it. There are numerous ways to "register" Serilog. You can use the IHost registration (which now seems to have become the recommended approach), you can register a custom SerilogLoggerFactory, or you can register just the SerilogLoggerProvider. The factory uses the provider internally, but the provider also works by itself with MEL's interface (I think). Then in usage, you can rely on ILogger, but most examples online actually use the static logger instance (a "very bad thing TM" as far as I'm concerned for any decently sized application).

Then you have other components in the Serilog family which rely on Serilogs interface instead of MEL´s ones, like the request logging, which don't work with dependency injection "properly" and can result in all sorts of inconsistencies depending on how.

The whole point I'm trying to make here is that perhaps having less options sometimes ends up being better. The existing options all have one flaw/quirk or another. Some don't play well with MEL, some don't play well with other Serilog extensions, some don't work properly with dependency injection, etc. If all options worked flawlessly and provided equivalent results then sure, but right now I feel like having this many options is not leading (me at least) to the pit of success.

Again, I realize the above is talking about a lot of random stuff. I think the custom factory is just one of the components that make it unwieldy to use.

Having said all that, I do appreciate the openness in discussing this from you guys. Lack of support has never been a problem with Serilog, that's for sure.

sungam3r commented 3 years ago

If we talk about the redundancy of the API, then I am 100% sure that this is the result of evolution. It's always easy to grab and remove a bunch of old code. What's the price? Loss of backward compatibility. Relative to a static logger. I myself do not use it anywhere and do not recommend it to others. I prefer DI friendly approach everywhere. I will only be glad if it will be removed in the next major version. It will also simplify the code because the static logger support adds additional code branches and switches. Your reasoning about the general design makes sense, but from a practical point of view, without specific exact problems, it will remain just reasoning. It's easier to work with smaller tasks, even if there are many, to focus on specifics.

Some don't play well with MEL, some don't play well with other Serilog extensions

We need specifics. What exactly is not working well? I have been using the Serilog for quite a long time, including sending logs to it via MEL and via custom (my own) logging abstractions.

nblumhardt commented 3 years ago

Thanks for all the feedback. As @sungam3r suggests, we should probably close this off here, and consider specifics elsewhere :+1:

bdovaz commented 1 month ago

I revive this conversation 4 years later...

In my case I'm trying to use a new .NET 8 package:

https://www.nuget.org/packages/Microsoft.Extensions.Telemetry/

Serilog having its own implementation of the factory is preventing me from using it....

I created an issue about this.