Use registered ILogger from DI container for SerilogLoggerProvider

[matherm-aboehm]

In commit ae4991f the registration of SerilogLoggerProvider was changed by using a factory instead of a direct instance, when it should be disposed by the DI container.
I want to request a new change for this line:

serilog-extensions-logging/src/Serilog.Extensions.Logging/SerilogLoggingBuilderExtensions.cs

Line 41 in d8c75ec

builder.Services.AddSingleton<ILoggerProvider, SerilogLoggerProvider>(_ => new SerilogLoggerProvider(logger, true));

To this:

builder.Services.AddSingleton<ILoggerProvider, SerilogLoggerProvider>(sp => new SerilogLoggerProvider(logger ?? sp.GetService<ILogger>(), true));

This would give us now the opportunity to also use a registered Serilog.ILogger from the service provider instead of providing it with a direct instance beforehand. And if it was not registered, the old behavior of using the static Serilog.Log would still be intact.

[nblumhardt]

Hi Andreas - makes sense; we're due soon for a 10.x release so could be something to explore there. Would need some careful testing but at a skim looks okay! :-)

[matherm-aboehm]

After thinking a bit more about it, it would be better to use a factory registration in all cases not only when dispose == true, so change the whole method to:

    public static ILoggingBuilder AddSerilog(this ILoggingBuilder builder, ILogger? logger = null, bool dispose = false)
    {
        if (builder == null) throw new ArgumentNullException(nameof(builder));

        builder.Services.AddSingleton<ILoggerProvider, SerilogLoggerProvider>(
            sp => new SerilogLoggerProvider(logger ?? sp.GetService<ILogger>(), dispose));

        builder.AddFilter<SerilogLoggerProvider>(null, LogLevel.Trace);

        return builder;
    }

With that, you would not hide the new behavior behind an unrelated parameter.
Also notice the passing of dispose to the constructor of SerilogLoggerProvider, so you would still have control over the disposal of the ILogger instance.

[matherm-aboehm]

Hi @nblumhardt, now that you have released v10.0 and you did not include this, I would like to ask what your plans are for this?