Should frameworks like Serilog, NLog implement their own ILoggerFactory?

[cwe1ss]

cc @304NotModified @nblumhardt

Since it is now possible to pass a custom ILoggerFactory into the WebHostBuilder by calling IWebHostBuilder.UseLoggerFactory(ILoggerFactory loggerFactory), I'm wondering if it would make more sense for 3rd party frameworks like Serilog, NLog to implement their own ILoggerFactory in addition to (or even instead of) a ILoggerProvider?

To me, it seems like ILoggerProvider is better for actual outputs like Console, EventLog, ... and ILoggerFactory would be better for frameworks. It's not really useful to combine a provider from M.E.L (e.g. Console) with sinks from Serilog because Serilog has a sink for each M.E.L-provider anyway. So, routing all logs through Microsoft.Extensions.Logging.dll seems like an unnecessary step.

If all logging frameworks would implement ILoggerFactory, we could also think about removing the assembly Microsoft.Extensions.Logging.dll from aspnet/hosting! The choice of a logging framework is the same as the choice of a server (e.g. Kestrel) IMO. This means, Microsoft.Extensions.Logging (which is just Microsoft's implementation) could be part of the template, but if someone wants to use Serilog/NLog he would remove the dependency and therefore wouldn't have Microsoft.Extensions.Logging.dll on disk anymore.

[304NotModified]

I think this is a nice idea, but I'm curious if there is enough time to change this. (from Microsoft's viewpoint)

[nblumhardt]

Interesting angle, thanks for looping me in. I can see how this could work, especially if the the whole AddProvider() and ILoggerProvider thing could be pushed down into Microsoft.Framework.Logging. To make it coherent I expect we'd need to propose and discuss other changes, and I think the 🚢 has sailed.

(I am, I have to admit, incredibly stoked that we have RC2 and RTM expected soon for the run-time portions of .NET Core and ASP.NET Core, and wouldn't hold that up for love or money :-))

[cwe1ss]

I personally also think that these concerns should have been split because Microsoft.Extensions.Logging.Abstractions will definitely become a default (next to LibLog) for library developers and I don't think that ILoggerProvider and ILoggerFactory.AddProvider should be a part of that assembly.
However, I also think that it is too late for this and that it most probably (and unfortunately) will never change due to post-RTM backwards compatibility restrictions.

But still, implementing an ILoggerFactory in Serilog, NLog, ... would be a great first step to allow people, who don't need other MS specific providers to completely circumvent that concept. Of course, you would still have to implement an ILoggerProvider because most people will use that (because that's what's in the samples), but I guess the effort for implementing the ILoggerFactory wouldn't be too big.

[artganify]

@cwe1ss I whole-heartly agree with this. To me, the ILoggerProvider is the actual factory for creating instances of ILogger and since frameworks like Serilog already have the capability to write to custom targets (sinks), it doesn't really make sense in that case to also have multiple providers. The ILoggerFactory is just ASP.NET's way to wire everything up in an ASP.NET web application. Now the thing is that it's not Microsoft.AspNet.Logging (despite being in the ASP.NET project) but rather Microsoft.Extensions.Logging and you're right, most people are definitely going to use it as a default. So +1 for e.g. using ILoggerFactory for frameworks, and ILoggerProvider for targets (or whatever it's going to be named then... maybe ILoggerTarget and ILogFacility?)

It might definitely be too late, but just in case, I really think we should create an issue addressing this. I mean, writing a logging abstraction framework should maybe take a week or two, where as the aspnet/logging project dates back to the beginning of 2014. All this time, over 400 commits for such an awkward situation?

[cwe1ss]

@davidfowl is there even a theoretical chance that any of the following might be considered at some point in the future?

• extracting provider specific interfaces from the abstractions-package (which probably would result in breaking every existing provider)
• making M.E.L completely optional (which probably would require every existing app to add the package to project.json and change its WebHostBuilder-code with something like .UseMicrosoftLogging())

If you see value in discussing/tracking any of these breaking changes, I would be happy to create separate issues with more details for them.

After your answer, I'll also close this issue, since implementing ILoggerFactory's is up to the 3rd party loggers and should probably be tracked in their repositories.

[PureKrome]

Anymore news/info about this issue? cc @davidfowl

[davidfowl]

Did any of the frameworks ever look at doing this to see what it would look like? I agree the interfaces are a bit messy as providers are really a specific implementation detail of our LoggerFactory implementation. Ideal world I guess would be:

public interface ILoggerFactory
{
    ILogger CreateLogger(string categoryName);
}

Get rid of the extension methods on ILoggerFactory that add providers and move them to LoggerFactory. Move all logging wire up in applications to the concrete LoggerFactory via some extension method on the IWebHostBuilder.

var host = new WebHostBuilder()
                  .UseLoggerFactory(loggerFactory =>
                   {
                         // Here we get a concrete `LoggerFactory`
                         loggerFactory.AddConsole();
                   })
                   .UseKestrel()
                   .UseStartup<Startup>()
                   .Build();

/cc @Eilon @muratg @DamianEdwards @glennc

I think we should do some thinking here for 2.0

[neman]

I created some abstraction for current project I'm working on

public interface ILoggerFactoryAdapter
{
       void AddLog(ILoggerFactory loggerFactory);
       void AddCorrelationProperty<T>(string propertyName, T value);
       //TODO:Refactoring tip, put in some other Interface
       void RefreshConfiguration();
}

so in Startup.cs Configure method I have something like

public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerFactory, ILoggerFactoryAdapter loggerFactoryAdapter)
{
...
loggerFactoryAdapter.AddLog(loggerFactory);
...
}

Basically, my Idea is that I can switch ILoggerFactoryAdapter implementation for any specific logger.

[nblumhardt]

@davidfowl we haven't done any experimentation with this for Serilog; agreed it sounds like an interesting opportunity to dig into.

If anyone's interested in PR'ing up (even some sketchy) changes for the Serilog provider, the repo is at: https://github.com/serilog/serilog-extensions-logging. I'd love to take a look but won't have any time in the next week or two.

[glennc]

I like this enough to move it into 2.0 and discuss it more with that timeframe in mind.

[davidfowl]

@BrennanConroy assigning this to you

[BrennanConroy]

I believe we have done the work on our side for this.

[cwe1ss]

@BrennanConroy yes, thank you very much!!!

[cwe1ss]

fyi: there might be a design issue with the changes introduced here and how aspnet/Hosting handles logging: aspnet/Hosting#1071