serilog / serilog-extensions-logging

Serilog provider for Microsoft.Extensions.Logging
Apache License 2.0
307 stars 97 forks source link

Feature Request: add a way to replace Microsoft's ILogger via a factory #179

Closed hari-hlubovac-carvana closed 9 months ago

hari-hlubovac-carvana commented 3 years ago

This extension method already provides a way to replace the logger, however with a static instance: https://github.com/serilog/serilog-extensions-logging/blob/dev/src/Serilog.Extensions.Logging/SerilogLoggerFactoryExtensions.cs#L40

It would be nice if there is one such method that accepts Func<Serilog.ILogger>, so that something like this can be done within Startup, for more flexibility:

public void Configure(IApplicationBuilder app, ILoggerFactory loggerFactory)
{
    loggerFactory.AddProvider(() => app.ApplicationServices.GetService<Serilog.ILogger>());
}

Of course, I could mimic, but I wasn't up to writing my own implementation of Microsoft.Extensions.Logging.ILogger or copying and maintaining yours: https://github.com/serilog/serilog-extensions-logging/blob/c5bad0c183ae10d448fcc3532095e298d57906a1/src/Serilog.Extensions.Logging/Extensions/Logging/SerilogLogger.cs

I think this would be a relative simple addition. If you agree and can't get to it, but if you accept PR's, I could craft something?

nblumhardt commented 3 years ago

Thanks for the note! Not 100% sure I understand your goal, here - is it that you would like to use services during logger initialization?

hari-hlubovac-carvana commented 3 years ago

I thought it'd be more flexible than

loggerFactory.AddProvider();

or

loggerFactory.AddProvider(logger);
nblumhardt commented 3 years ago

Thanks! Still not sure I understand though - flexible in order to do ....? (What logic would go in the callback that can't go in the enclosing method?).

https://github.com/serilog/serilog-extensions-hosting#inline-initialization could also be what you're looking for - let me know if this helps 👍

hari-hlubovac-carvana commented 3 years ago

Somehow, I went the direction of setting up the DI to deliver many ILogger instances "dynamically" - for example, each bound to current HttpContext, like so:

var defaultLogger = new Serilog.LoggerConfiguration()/*other-setup-here*/.CreateLogger();

services.AddTransient<Serilog.ILogger>(p =>
{
    var context = p.GetRequiredService<IHttpContextAccessor>().HttpContext;
    if (context == null)
        return defaultLogger;

    string name = typeof(Serilog.ILogger).FullName;

    Serilog.ILogger logger;
    if (context.Items.TryGetValue(name, out object o) && (logger = o as Serilog.ILogger) != null)
        return logger;

    context.Items[name] = logger = defaultLogger.ForContext("SomethingFromCurrentRequest", "SomeValueHere");
    return logger;                
});

(Although there are potential issues here for those dependencies utilizing ILogger, that are also set as singletons which initialization is deferred; so those would have to mind this setup).

And, so, this wouldn't play well with your ILoggerFactory.AddSerilog, as it's not an actual logger-provider (factory) that's being passed-in, but instead a concrete and fixed instance of ILogger.

That's my explanation, but I will accept other ways of making context-specific properties being included in logs. Perhaps scoped registration is appropriate, or maybe you'll have a quick link for me to take a look at.

Thank you.

nblumhardt commented 3 years ago

Hi! Using middleware, in conjunction with Enrich.FromLogContext() and LogContext.PushProperty(), is the usual way of achieving this. Let me know if this helps :+1: