Support dependency injection into sinks, enrichers, filters, etc.

[nblumhardt]

Assuming, we'd like to initialize Serilog in the first few lines of Program.Main(), and independently of ASP.NET Core/other hosting code, how can we support sinks and other pipeline elements that depend on the framework and components that are initialized later?

For example, in #84 there's a discussion regarding the use of Application Insights configuration from dependency injection in a Serilog sink.

LateInitSink in this gist demonstrates a possible approach:

        var signalRSink = new LateInitSink();

        Log.Logger = new LoggerConfiguration()
            .WriteTo.Sink(signalRSink)
            .CreateLogger();

        Log.Information("Sadly, nobody will get this");

        // ... resolve that hub ...
        signalRSink.Init(wt => wt.Console());

        Log.Information("Ah so nice to be loggin' again");

@khellang suggested using a name like DependencyInjectedSink instead of LateInitSink, so make the API more searchable/discoverable.

We could, finalize/ship an implementation of this approach.

The need to also support enrichers, filters, and so-on, could be met by including a range of similar types for those cases.

As an alternative, or extension of this idea, we could create a wrapper API that mimics LoggerConfiguration and sets up the full range of extension points for DI (rough sketch):

// Initialize what we can, early on. The `deferred` variable
// will hold a `DeferredLoggerConfiguration` that's hooked
// into late-init sinks, enrichers, etc. by a new `CreateLogger()`
// extension-based overload.
Log.Logger = new LoggerConfiguration()
    .WriteTo.Console()
    .CreateLogger(out var deferred);

// Later, post-DI-setup, or elsewhere in the app:
deferred
    .AddSink(writeTo => writeTo.File(...))
    .AddEnricher(enrich => enrich.WithProperty(...))
    .UpdateLogger();

This API should be possible using some extension points already in 2.9.0; it would be nicer to support deferred.WriteTo.File() etc. but we'd need new APIs to enable it.

Thoughts?

See also #130.

[martinjt]

I'd much prefer to add something like

Log.Logger = new LoggerConfiguration()
    .WriteTo.Console()
    .AddAspNetCoreDIEnricher()
    .CreateLogger()

then

services.AddTransient<IEnricher, RequestEnricher>();
services.AddSingleton<IEnricher, EnvironmentEnricher>();

services.AddSingleton<ISink, ConsoleSink>();

I've no idea how to achieve it though.

The thought pattern is that it's a model that a lot AspNetCore programmers will understand. It would make the cognitive load a lot less.

[nblumhardt]

@martinjt thanks for the note. I think supporting sinks and enrichers directly from DI is a worthwhile consideration, although it won't work for most existing Serilog sinks because they're not exposed as objects directly (so the above would be additive, in some way, I guess).

It's tricky to achieve a cleaner syntax without some object linking the first and second parts of the configuration process (deferred, in my example), but hopefully that can be done in a fairly clean manner.

[nblumhardt]

Another note, it would be good to explore the intersection of this and LoggerProviderCollection, and whether these could be combined into a single API that would allow both existing providers, and sinks etc., to be consumed from DI.

[tsimbalar]

Mmm I have some concerns about the API being a bit mind-twisting, but it's sort of an advanced scenario, so maybe that's just fine.

Another approach that may or may not work ...

var resolveContext = new ResolveContext();
Log.Logger = new LoggerConfiguration()
    .WriteTo.Console()
    .ResolveLater(resolveContext, (cfg, ctx) => 
        cfg
          .Enrich.WithProperty("ResolvedLater", ctx.Resolve<string>("TheValueForResolveLater"))
          .WriteTo.Sink(ctx.Resolve<MyCustomSink>())
    )
    .CreateLogger();

// and then later on ...
resolveContext.Set<MyCustomSink>(new MySinkCOnfiguredLater());
resolveContext.Set("TheValueForResolveLater", "The value to show in the logs");
// notify people that relied on this `resolveContext` that the values are now ready
resolveContext.Update();

( oh my, I haven't written any C# in way too long, and my brain writes TypeScript U_U ... )

so somehow promoting some sort of ResolveContext which is pretty much a dictionary and/or a Service Locator where we can put how some given keys or types are resolved to an instance.

This also means we can use multiple ResolveContext instances if for some reason dependencies become available at a different time 🤔

I guess it could then be built upon the existing DI features from AspNetCore or something else ?

maybe that does not make much sense, so this is just a brainstorming exercise :)

[nblumhardt]

@tsimbalar I like that API 👍 - will give that option some more thought.

Rather than build it directly on the container APIs, passing a HostingContext or other similar type that also gives access to configuration, etc., might be worth considering, too.

[alesdvorakcz]

Support for dependencies in enrichers, etc would be really appreciated. Can I ask if this is still considered feature in near future?

[nblumhardt]

@alesdvorakcz thanks for the nudge. Yes, it's still being considered - just a matter of someone finding the time to dig in deeply/write some code. Cheers!

[ralphhendriks]

Over the last days I have been struggling to figure out the current most correct way of configuring the Application Insights sink in an ASP.NET Core 3.1 application, preferrably without deprecation warnings. Reading serilog-contrib/serilog-sinks-applicationinsights#121 and related brought me to this issue.

I am all for a DI-friendly API design. However, I would be very reluctant to develop serilog's API design to contain any explicit notion of DI, and MS.DI in particular. Several third party DI containers are non-conforming to the MS.DI interfaces, such as https://github.com/simpleinjector/SimpleInjector, whose main author @dotnetjunkie is actively involved in many discussion around this topic. With that in mind, I am not a fan of the proposed .AddAspNetCoreDIEnricher() API suggestions.

The main problem is quite well summarized by @cmeeren:

• The Application Insights telemetry config is set up and registered with DI in ConfigureServices (so that it picks up all telemetry initializers and processors that are also registered), and therefore only available after it has ConfigureServices has run
• The telemetry config is needed when configuring Serilog
• UseSerilog is configured before ConfigureServices is called

Given Microsoft's design philosophy, where the composition root of the application is on the level of the IHost, there is now a timing problem in setting up the Serilog logging pipeline. Ideally configuring logging would be about the first thing to do after application startup, which is before building and running the host. However, some logging pipeline components might have dependencies first set up in the composition root.

IMHO a good approach would be to do all fail-safe/isolated logger setup directly after application startup (e.g. configuring a console sink for logging to stdout), and then being able to either add configuration to that same logger, or to derive from it (and update the static instance) in the composition root. A clean API for this might be to add an overload of LoggerConfiguration that would take an existing, already configured Logger. However, I am not sufficiently familiar with the internals of serilog to know whether this can safely be implemented.

It would be great if we can take this discussion forward. (@nblumhardt pardon the indirect nudge 😉)

[nblumhardt]

@ralphhendriks thanks for the nudge :-) ... Does the LateInitSink API from the first example work for you? Just trying to map out the space a bit - appreciate the help!

[ralphhendriks]

@nblumhardt Sorry for the delay, it took me a few days to think about this and also discuss some aspects with a colleague (:wave: @Red-F).

I agree that the idea of the LateInitSink would be the least worst alternative now, although it still gives the uncomfortable feeling of being unnecessary complex. Btw, I would consider DeferredInitSink a more precise name. In my previous comment I suggested changing the core API of Serilog, which is something that I would indeed approach with caution. The DeferredInitSink (an maybe also a DeferredInitEnricher) could be in a separate NuGet-package, thus allowing for relatively unobtrusive experimentation with these concepts. Only once we are really sure that this is a necessary capability they could be promoted to the core API/package.

I would think that MS is facing these issues as well for the built-in logging capabilities in ASP.NET Core. I remember reading a comment in a doc that after the change to the IHost-abstraction in .NET Core 3.0 logging early in the program startup is now no longer possible, but I can't find that right now. Let me spend some spare time over the next days to dig into how MS is thinking about this issue.

[ralphhendriks]

Found that serilog/serilog-extensions-hosting#14 is also related. It would definitely be interesting to hear @davidfowl's opinion here.

[julealgon]

I'd like to share my two cents here. Currently, most if not all sinks are created via the recommended extension method approach. I believe this does not scale properly and is hard to adapt to DI.

Instead, I propose we decouple the sink creation from the sink configuration. Have those extension methods just define the sink type and options (maybe use the idiomatic IOptions<T> at this point) but defer the actual construction of the sinks to a new activator/factory abstraction.

Then, once this is in place, we can have a simple activator-based factory and a DI based one that are installed automatically depending on which Serilog packages are referenced. Sinks that currently don't depend on DI will work fine with a simple activator approach, while more modern ones that rely on registered services can be built using ActivatorUtilities.CreateInstance<T>(IServiceProvider, ...) which constructs the instance by resolving any dependencies from the container. These activator implementations could be completely hidden from consumers leaving the API clean, while we could still offer the option of passing custom activator implementations if needed. This same strategy could be used to create other serilog elements as well, like enrichers, filters, etc.

I think this decoupling would lead into a clean syntax (basically keeping what exists today for configuring the sinks) without the need for decorator/wrapper "dependency-aware sinks" or those hard to read lambdas by default.

[g7ed6e]

Hi, i thought to this a bit and I finally came to the fact that the Serilog package itself should expose a way to register an abstraction which responsibility is to provide dependencies, let's say IDependencyResolver. Then, still in the Serilog main package the extension method LoggerSinkConfiguration.Sink<TSink> should be modified to open the constraint on TSink to class, ILogEventSink instead of ILogEventSink, new allowing the implementation to use previously registered IDependencyResolver.
This way, other packages like Serilog.Settings.Configuration should be able to later use the IDependencyResolver.
Once done, It would be easy to package simples IOC's container specific packages that would adapt to IDependencyResolver.

Finally it would lead to this kind of code in the Program.cs of an asp.net core app:

Host.CreateDefaultBuilder(args)
                .ConfigureServices(
                    (context, services) =>
                    {
                        services.AddTransient<IService, Service>();
                    })
                .ConfigureLogging(
                    (context, builder) =>
                    {
                        var provider = builder.Services.BuildServiceProvider();
                        builder.AddSerilog(new LoggerConfiguration()
                                           .ResolvesDependenciesFrom.MSDI(provider)
                                           .ReadFrom.Configuration(context.Configuration)
                                           .CreateLogger());
                    })
                .ConfigureWebHostDefaults(
                webBuilder =>
                    {
                        webBuilder.UseStartup<Startup>();
                    });

Please see this commit https://github.com/g7ed6e/serilog/commit/7fb606c62e9afc37d000c0f5917905f383f298de

[davidfowl]

                    var provider = builder.Services.BuildServiceProvider();

That line will create a different DI container than the one the application uses. You'll end up with 2 sets of singletons and nobody is disposing the container here either so your disposable objects don't get disposed.

[ralphhendriks]

@g7ed6e The abstraction IDependencyResolver that you are suggesting is, in essence, a conforming container. See this blog post by @ploeh that goes into a lot of the nuances of why he considers this an antipattern.

I am specifically worried about your remark

Once done, It would be easy to package simples IOC's container specific packages that would adapt to IDependencyResolver.

This would lead to a range of integration packages being added to the already quite substantial amount of Serilog(-related) NuGet-packages available. On top of the arguments put forward by @ploeh, this would overwhelm developers with too many possibilities and may even cause quite some confusion. (Think for example about a developer employing one of the conformist third party DI containers. Has they to cross-wire the logging configuration using your proposed .MSDI(provider) extension? Or is a similar extension specific for the DI container of their liking a better choice. Why?) Serilog has quite some exposure in the modern .NET world, and I value a lot that its maintainers promote thoughtful object oriented design. In all honesty I think that your suggestion might lead away from that.

I quite like @julealgon's observation that sink (enrichter, etc.) creation and configuration are two distinct things. However, going this route would probably mean a major and breaking API change to the core assembly, thus impacting all users of Serilog, not just those integrating with ASP.NET Core. I wonder what @nblumhardt thinks of this. I would consider @julealgon's suggestion in #169 as a first, and far more low-key step in the right direction for improving the integration with ASP.NET Core BTW.

[ploeh]

In case someone starts to read the blog post that @ralphhendriks linked to (Conforming Container), but doesn't make it all the way to the end of it, it can seem like it's all criticism and no constructive advice. I should have found a better way to structure that and subsequent articles.

The constructive advice does exist, however. For a a library like Serilog, I'd surmise that DI-Friendly Library contains most of the relevant advice I could think of back then. I'm happy to assist with particulars, if I can.

[nblumhardt]

I've ended up with a proof-of-concept based on https://github.com/serilog/serilog-reload that adds an Action<IHostBuilderContext, IServiceProvider, LoggerConfiguration> overload to UseSerilog(), and detects ReloadableLogger behind-the-scenes (for better or worse) so that an initial, "bootstrap" logging configuration can be replaced and frozen once sinks etc. are configured from DI:

public static class Program
{
    public static void Main(string[] args)
    {
        Log.Logger = new LoggerConfiguration()
            .WriteTo.Console()
            .CreateReloadableLogger(); // <-- ⭐

        Log.Information("Starting up");
        
        try
        {
            CreateHostBuilder(args).Build().Run();
        }
        catch (Exception ex)
        {
            Log.Fatal(ex, "Unhandled exception");
        }
        finally
        {
            Log.CloseAndFlush();
        }
    }

    public static IHostBuilder CreateHostBuilder(string[] args) =>
        Host.CreateDefaultBuilder(args)
            .UseSerilog((context, services, cfg) => cfg
                .MinimumLevel.Override("Microsoft", LogEventLevel.Warning)
                .MinimumLevel.Override("System", LogEventLevel.Warning)
                .WriteTo.Sink(new SinkWithDependencies(new Lazy<IHttpClientFactory>(
                    () => services.GetRequiredService<IHttpClientFactory>()))) // <-- ⭐
                .WriteTo.Console())
            .ConfigureWebHostDefaults(webBuilder => { webBuilder.UseStartup<Startup>(); });
}

Hangs together without too much magic, and would support Enrich.FromServices(services), WriteTo.Services(service), etc, in a straightforward way.

It might seem strange to bake in special handling for ReloadableLogger, but, the whole set of problems around startup and logging configuration are intertwined, so I think the next step forward for this library is to solve them all, together. (Reloading is required when the initial pipeline uses a file sink, for example, or if long-lived contextual loggers end up created from the initial pipeline.)

A couple more notes - I think we should writeToProviders by default in this configuration, and also, as you'll see in the example, register the Serilog ILogger consistently, pushing harder on a "just works" set of defaults.

Example code is in: https://github.com/nblumhardt/serilog-reload/tree/dev/example/WebExample - opinions, ideas, and PRs welcome.