37

The MS docs article "Introduction to Logging in ASP.NET Core" gives 2 examples of constructor injection

  • using ILogger

    private readonly ILogger _logger;   
    public TodoController(ILogger<TodoController> logger)  
    { 
        _logger = logger;   
    }
    
  • and ILoggerFactory

    private readonly ILogger _logger;  
    public TodoController( ILoggerFactory loggerFactory)  
    {  
        _logger = loggerFactory.CreateLogger<TodoController>();  
    }
    

My question is what should I pass to child classes called from my controller

  • pass ILoggerFactory to my child classes called from the controller and in each class call LoggerFactoryExtensions.CreateLogger<MyChildClass>() or

  • pass parent controller's ILogger<MyController> to each child class created from the controller and having non-generic parameter ILogger.

In logs I prefer to see separate category 'MyChildClass' for each class, rather than all classes use the category 'MyController' from the parent controller.
However CreateLogger in each object construction can be an expensive operation (e.g. see https://github.com/aspnet/Logging/issues/524)

Which option will you recommend? Can you suggest any other approach?

Nkosi
  • 235,767
  • 35
  • 427
  • 472
Michael Freidgeim
  • 26,542
  • 16
  • 152
  • 170
  • Have child classes inject their own logger and then inject the child class into the controller. That way the child loggers will get resolved when the child classes are being resolved and injected into the controller. – Nkosi Sep 02 '17 at 13:09
  • 7
    I woulds say that both injecting a `ILogger` and `ILoggerFactory` are bad approaches. Injecting a `ILoggerFactory` causes your constructors to [do too much](http://blog.ploeh.dk/2011/03/03/InjectionConstructorsshouldbesimple/), while `ILogger` just adds more noise to your code. Instead, you should inject a non-generic `ILogger` instead and let the DI infrastructure automatically inject the right logger for you. The reason however that the MS docs don't show this, is because the built-in container is too limited. It can't inject an `ILogger` for you that way. – Steven Sep 03 '17 at 11:27
  • @Steven: Will more advanced dependency injection libraries be able to pass to non-generic ILogger parameter a logger having category with the name of the current class? Or I will need to insert the category name in each constructor? – Michael Freidgeim Sep 03 '17 at 12:55
  • I think ALL popular DI libraries support this. – Steven Sep 03 '17 at 13:05
  • @steven, in order to log asp.net internal things - need to register a logger using `Microsoft.Extendions.DependencyInjection` - say Serilog - and then need to plug - say - autofac in (easy), and register Serilog again in autofac? – lev haikin Mar 01 '18 at 04:37
  • [This 2016 article on MSDN](https://msdn.microsoft.com/en-us/magazine/mt694089.aspx) recommends an individual ILogger instance to avoid "bottleneck as [thread] locks are taken and released." – Nick Alexeev Mar 12 '19 at 03:58
  • @NickAlexeev, thanks for a good article, but both methods in my question create individual ILogger instances. Static logger instance is not considered. – Michael Freidgeim Mar 13 '19 at 10:24
  • @steven what is the point of using ILogger and then saying it doesn't support configuring the right logger. when in fact that would in this case be ilogger – Wouter Nov 16 '20 at 21:09

2 Answers2

25

This is more of a design issue.

The controller should not be creating child classes anyway. That is not a concern the controller should be dealing with and goes against SRP (Single Responsibility Principle).

My question is what should I pass to child classes called from my controller

If your preference is to separate the loggers, then there really isn't any other choice here than to have child (dependent) classes have their own loggers.

Have child classes inject their own logger

public class TodoRepository : ITodoRepository {
    private readonly ILogger logger; 

    public TodoRepository(ILogger<TodoRepository> logger) { 
        this.logger = logger;   
    }
  
    //...
}

and then inject the child class into the controller.

public class TodoController : Controller {
    private readonly ILogger logger;
    private readonly ITodoRepository todoRepository;

    public TodoController(ILogger<TodoController> logger, ITodoRepository todoRepository) {
        this.logger = logger;   
        this.todoRepository = todoRepository;
    }

    //...
}

That way, the child loggers will get resolved when the child classes are being resolved and injected into the controller.

Pang
  • 9,564
  • 146
  • 81
  • 122
Nkosi
  • 235,767
  • 35
  • 427
  • 472
1

How to provide logger to the library is discussed in Should I take ILogger, ILogger<T>, ILoggerFactory or ILoggerProvider for a library?

If someone needs to access ILoggerFactory outside of Startup ( usually not recommended)

This factory could be stored as a static/global instance somewhere.

(From Using Microsoft.Extensions.Logging in EF Core)

Similar suggestion is in Accessing the Logging API Outside of a MVC Controller

Create a centrally located static class or project to hold and wrap around the main LoggerFactory reference

Michael Freidgeim
  • 26,542
  • 16
  • 152
  • 170