Potential design issue with custom ILoggerFactory types and logger providers

[cwe1ss]

I think that the combination of obsoleting ILoggerFactory.AddProvider and allowing custom ILoggerFactory types with IWebHostBuilder.ConfigureLogging<T>() and ILoggerFactory.UseLoggerFactory(ILoggerFactory) introduced a (potentially big?!) design issue.

Consider the following scenario:

A logging framework like Serilog will use its own ILoggerFactory type and offers the following extension method:

public static IWebHostBuilder UseSerilog(this IWebHostBuilder webHostBuilder)
{
    SerilogLoggerFactory loggerFactory = new SerilogLoggerFactory();
    webHostBuilder.UseLoggerFactory(loggerFactory);

    return webHostBuilder;
}

A tracer like Application Insights wants to subscribe to ILogger-events. It uses the following code to register its services and its logger provider:

public static IWebHostBuilder UseApplicationInsights(this IWebHostBuilder webHostBuilder)
{
    webHostBuilder.ConfigureServices(services => { /* add DI services */ };

    webHostBuilder.ConfigureLogging<LoggerFactory>((context, loggerFactory) => 
    {
        loggerFactory.AddApplicationInsightsProvider();
    }

    return webHostBuilder;
}

An application developer wants to use both and builds his web host with the following code:

new WebHostBuilder()
    .UseKestrel()
    .UseContentRoot(Directory.GetCurrentDirectory())
    .UseSerilog()
    .UseApplicationInsights()
    .Build();

This will result in the Application Insights provider not being registered because the LoggerFactory types don't match. Obviously, a user would have a hard time troubleshooting this.

It also shows a fundamental issue in that tracers are no longer able to reliably register themselves as logging providers. This is a scenario I haven't really thought of in aspnet/Logging#420.

---

Given this missing link and the fact that LoggerFactory now already does a lot more with Configuration-based filtering, I no longer think it's useful/good for e.g Serilog to implement a custom ILoggerFactory.

Instead, it might be better to follow one of these solutions:

• Disallow custom ILoggerFactory types by using the concrete type LoggerFactory everywhere in IWebHostBuilder. This would still allow people to write derived types while keeping the logic of adding providers intact. The downside of this is that providers still have to depend on the Microsoft.Extensions.Logging implementation package.
• Undo obsoleting ILoggerFactory.AddProvider and use ILoggerFactory in IWebHostBuilder methods. This way, there would still be an Abstractions-contract that links loggers and providers together and no-one would have to depend on the implementation package. However, users could still call AddProvider in regular classes.
• Introduce something like ILoggerProviderFactory which contains AddProvider(), AddFilter(), UseConfiguration etc. This would completely separate loggers from providers. All logging methods on IWebHostBuilder would use this interface instead.

The last one might be the cleanest. This type could then be added to DI so that people could use it in Startup.cs. This would be an easier migration path for users than moving their logging configuration to the WebHostBuilder.

Your thoughts?

I'd be happy to create a PR should you want to do something here!

Edit: I've removed the ConfigureLogging() examples since they could still use the type constraint. It would just be constrained to the respective type.

[cwe1ss]

In case of a separate interface, it might be even better to call it ILoggingBuilder?! This way, there would be a common scheme for setting up "configuration" (IConfigurationBuilder) and "logging".

[davidfowl]

We're leaning towards decorating (wrapping) the ILoggerFactory instead of the above options.

[cwe1ss]

You'd have to decorate the concrete LoggerFactory, otherwise my example would still be broken. So I think this still requires what I've described in the first option (locking down IWebHostBuilder to LoggerFactory).

[davidfowl]

@cwe1ss Sorry, I don't follow. Why is that?

[cwe1ss]

Because Application Insights would still have to use ConfigureLogging<LoggerFactory>() (it doesn't know of any other type) and this would fail because factory is T typedFactory returns false if the custom loggerFactory decorates ILoggerFactory. Or am I missing something?

[davidfowl]

I don't even see how decorating would work via ConfigureLogger<T> that's just broken. We would expose some other method that basically let you set the ILoggerFactory:

builder.ConfigureLogging(context =>
{
     context.LoggerFactory  = new ApplicationInsightsLoggerFactory(context.LoggerFactory);
});

[cwe1ss]

But why would Application Insights use a custom logger factory?

That's the point... using a custom logger factory is broken right now and I'm not sure it even makes sense anymore.

[davidfowl]

But why would Application Insights use a custom logger factory?

Because it wants to see all of the logs.

That's the point... using a custom logger factory is broken right now and I'm not sure it even makes sense anymore.

@cwe1ss it's ok, we were aware of this when we made the change but did it anyways so we can get feedback in preview1. We're going to be revisiting this problem.

The conclusion we may come to is that ILoggerFactory isn't an abstraction at all.

[muratg]

cc @BrennanConroy @pakrym

[cwe1ss]

IMO this is resolved with #1089 - thx guys!