Consider a way to configure logging provider using the DI system

[davidfowl]

This code always looks a little bizzare:

public void ConfigureServices(IServiceCollection services)
{
    ...
}

// This method gets called by the runtime. Use this method to configure the HTTP request pipeline.
public void Configure(IApplicationBuilder app, ILoggerFactory loggerFactory)
{
    loggerFactory.AddConsole();
    ...
}

The fact that hosting adds the default ILoggerFactory is fine, but having to resolve it in Configure to add logging providers seems anti to how we've integrated the rest of the stack (it feels bolted on). Also, configuring logging in ConfigureServices would mean the only thing we configure in Configure would be the http stuff (for the most part).

Proposal 1

The default logger factory should resolve an IEnumerable in its ctor.

public void ConfigureServices(IServiceCollection services)
{
    services.AddMvc();

    services.AddConsoleLogger(LogLevel.Debug);   
}

public LoggerFactory(IEnumerable<ILoggerProvider> loggerProviders)

The only downside if that more extension methods get added to IServiceCollection but meh.

Proposal 2

Add LoggerOptions and hang an IList off it.

public void ConfigureServices(IServiceCollection services)
{
    services.AddMvc();

    services.Configure<LoggerOptions>(options => 
    {
        options.Providers.AddConsole(LogLevel.Debug);
    });   
}

/cc @DamianEdwards @lodejard @rynowak

[lodejard]

All of those changes would make the logging configuration more complex, and also defer when logging "kicks in". By adding the logging provider as early as the startup class ctor in plain-old-c# you can also write messages in the configure services method, much earlier than when the options system and IOC is active.

Also with the Program.Main hosting model we might also want to add a way to pass an application-created ILoggerFactory in to the web host builder.

So yeah, end of the day I think a number of subsystems like IFileProvider, IConfiguration, ILoggerFactory are designed with IoC in mind, and may be added like services, but it's not a good idea to go the rest of the way and make their activation and building depend on DI or options.

[davidfowl]

All of those changes would make the logging configuration more complex, and also defer when logging "kicks in". By adding the logging provider as early as the startup class ctor in plain-old-c# you can also write messages in the configure services method, much earlier than when the options system and IOC is active.

That doesn't need to change. You can still inject the ILoggerFactory to create loggers but you don't need to add the providers there. We don't even have to block the fact that you can add providers at runtime I just think it should be doable via the DI system as well.

Also with the Program.Main hosting model we might also want to add a way to pass an application-created ILoggerFactory in to the web host builder.

That doesn't make it any cleaner but we should allow for that as well.

So yeah, end of the day I think a number of subsystems like IFileProvider, IConfiguration, ILoggerFactory are designed with IoC in mind, and may be added like services, but it's not a good idea to go the rest of the way and make their activation and building depend on DI or options.

IConfiguration maybe but IFileProvider feels different. You mostly want to be able to get a a specific IFileProvider so some indirection is needed. I don't see how this complicates the logger factory anymore than it is right now...

[davidfowl]

@lodejard What if we make this a concern of Hosting instead? We resolve IEnumerable<ILoggerProvider> and call AddProvider on the ILoggerFactory created in Hosting.

[lodejard]

I could see that as an extra step we add, right after the host gets control back from ConfigureServices and creates the service provider?

[davidfowl]

Yep

[muratg]

@davidfowl @lodejard do we have an agreement here?

cc @Eilon

[cwe1ss]

+10! I think it would be great if Logging could be configured as early as possible.

Ideally it should happen as the first thing in Program.cs to be able to log initialization exceptions. Just think of the case where you start the web application on a port that's already taken. It's not possible to log this right now, right?

[lodejard]

It's a reasonable thing to do, resolving ILoggerProvider from IoC after ConfigureServices returns.

Though if you want the logging as soon as possible you'd still take ILoggerFactory as a Startup class constructor argument and add providers there. @cwe1ss that would log server startup exceptions also.

[cwe1ss]

I didn't know that this works! great, thx!

[muratg]

Does aspnet/Hosting#661 resolve this?

[Rick-Anderson]

startup class ctor

@lodejard can you send me code to do this with the RC2 bits. The template generated code doesn't hook up logging until Configure

Should I open a bug with the template for initializing logging so late in the game?

[tuespetre]

Option 2, please (except maybe call it LoggingOptions.)

On second thought, no thank you on option 2 -- that would mean you'd have to implement IConfigureOptions<LoggerOptions> to get dependency injection for the providers themselves. Option 1, please!

[lodejard]

Yeah, both option 1 and 2 leave something to be desired.

THB this should be a hosting feature - after the application container is returned, the hosting library should resolve IEnumerable<ILoggerProvider> and add each to the ILoggerFactory.

[lodejard]

That said - I'm actually in favor of Option 4 - don't change it. This is actually one of the few cases DI population has some disadvantages - the sooner you add providers to logging the more information you will have re: your startup sequence. For example - if components are sending log messages during ConfigureServices - or if your IoC container population or resolution is throwing exceptions - you'll never see any of that if your logging providers are resolved out of your IoC container.

[tuespetre]

This is actually one of the few cases DI population has some disadvantages - the sooner you add providers to logging the more information you will have re: your startup sequence. For example - if components are sending log messages during ConfigureServices - or if your IoC container population or resolution is throwing exceptions - you'll never see any of that if your logging providers are resolved out of your IoC container.

Not all logging providers are created equal. During application startup, it may be beneficial to have a trace of critical information using a 'simple' provider like the console provider, but once the application has started, it can be far more valuable to stream logs into some kind of database like SQL Server or Elasticsearch -- such 'complex' providers could really make good use of DI.

So basically what @davidfowl said:

We don't even have to block the fact that you can add providers at runtime I just think it should be doable via the DI system as well.

[lodejard]

Okay, but isn't that already totally do-able?

https://gist.github.com/lodejard/93f00f23bd8ee3f5f9fb64232e9503a9

My File->New project template is a bit out of date, but you get the idea.

[tuespetre]

@lodejard that would indeed work, but the foreach loop is just noise when the DI system supports injection of IEnumerable<>s. I would at least want to keep that noise in the ConfigureServices method where things already have the tendency to get noisy (by registering a new singleton ILoggerFactory using the Func<IServiceProvider, ILoggerFactory> overload) but it's still just noise.

[gimlichael]

What is the status of this proposal?

[bklooste]

I dont think this should be done... logging with some providers should be configured before startup and DI even runs else you miss startup / DI config errors ! So we setup the logging in Program.cs and it then add it to the services injected in the webbuilder it than becomes available in Startup if people need it.
Encouraging this to be done via DI / startup / options continues this bad pattern - it should have no dependencies and be in the first few lines of main().

[davidfowl]

We're pursuing this for 2.0

[DamianEdwards]

@pakrym can we close this now?