Pass existing LoggerFactory into WebHostBuilder

[cwe1ss]

As described in #658

I'm motivated so I couldn't wait for your answer. :) Feel free to reject it if you think this is not correct or if you want something changed.

It allows to pass an existing ILoggerFactory into the WebHostBuilder constructor. I think this is better than a UseLoggerFactory method because the LoggerFactory is already created in the constructor right now.

I also added two tests. One checks if the host can actually resolve an ILoggerFactory and the second checks if the LoggerFactory passed in gets resolved correctly.
I added the tests at the top of the file because they are related to the constructor - I hope that's ok?!

BTW: Supporting a way to pass a LoggerFactor into the WebHostBuilder is also discussed here:
aspnet/Logging#346 ( @davidfowl @lodejard )

[dnfclas]

Hi @cwe1ss, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[cwe1ss]

Do you think that ConfigureLogging() should still be available if you accept this PR? Since you can now configure the logger factory before you create the WebHostBuilder, there's not really a reason to call this method.

[davidfowl]

I think we should support this but not via the constructor

[cwe1ss]

Ok. How about this:

• _loggerFactory will no longer be initialized in the constructor.
• A new method UseLoggerFactory(ILoggerFactory loggerFactory) allows people to set _loggerFactory. This way it's similar to other methods like UseStartup etc.
• BuildHostingServices will only add _loggerFactory if it is not null. If no loggerFactory was configured, the call to services.AddLogging() will add a new LoggerFactory automatically:

if (_loggerFactory != null) 
{
  services.AddSingleton(_loggerFactory);
}

services.AddLogging();

The existing method ConfigureLogging(Action<ILoggerFactory> configureLogging) is problematic because people could call it in the wrong order. I would remove it since the LoggerFactory can be configured before it is passed to UseLoggerFactory.

If you want to keep it, we would have to do the following:

• ConfigureLogging() no longer directly executes the action. Instead it stores it in a new variable called _configureLoggingDelegate.
• BuildHostingServices() would now always create the LoggerFactory. It would look like this:

var loggerFactory = _loggerFactory ?? new LoggerFactory();
if (_configureLoggingDelegate != null) 
{
  _configureLoggingDelegate(loggerFactory);
}

services.AddSingleton(_loggerFactory);
services.AddLogging();

If you disagree, what would you suggest?

[cwe1ss]

I changed the code. It now has a UseLoggerFactory method. I also kept ConfigureLogging and made it similar to ConfigureServices to make sure that it still can be called multiple times.

Should it be possible to call Build() multiple times without side effects? Right now, I'm setting _loggerFactory in the Build method and I'm executing the delegates afterwards. If someone would call Build a second time, these delegates would be executed again on the same variable. The alternative would be to create a local LoggerFactory variable.

[Tratcher]

Don't worry about Build being called more than once.

[davidfowl]

While you're at it. add ConfigureLogging back to IWebHostBuilder.

[cwe1ss]

I added ConfigureLogging and UseLoggerFactory to the interface. Note that this results in a new dependency on Microsoft.Extensions.Logging.Abstractions for the Abstractions package.

[davidfowl]

This needs a rebase

[cwe1ss]

hmm... I haven't done much merging/rebasing yet so I probably screwed it up. After trying a few things I ended up with duplicate commits. For that reason I did a complete rebase with git rebase -i dev and git push -f (because the push was denied otherwise).
The commit history looks good now, however the PR still says all checks have failed. Probably because it didn't execute it again?!?

Is it ok like this or should I do something else?

[JunTaoLuo]

merged in 4b16e83. Tested locally. Thanks @cwe1ss!

[cwe1ss]

thank you very much!