2

I am struggling with a lifestyle issue when injecting my logger.

NLog expects instances to be created "Instance per Dependency", as described here - https://simpleinjector.readthedocs.io/en/latest/lifetimes.html#instance-per-dependency.

Logger creation is done using a factory method LogManager.GetCurrentClassLogger(). Using this pattern allows the logger to obtain the name of the caller in order to include it in logged messages.

Simple Injector tells me that my Transient logger cannot be injected into a singleton. But I cannot change the lifestyle to Singleton because I would then lose the context information about which class is calling the logger.

My implementation is below:

I have written a factory that creates the NLog instance and returns it in a wrapper that implements my own ILogger abstraction:

internal class LoggerFactory : ILoggerFactory
{
    public ILogger GetLogger()
    {
       return new NLogLogger(LogManager.GetCurrentClassLogger());
    }
}

I registered this with Simple Injector thus:

container.RegisterSingleton<ILoggerFactory, LoggerFactoryInstance>();
container.Register<ILogger>(
    () => container.GetInstance<ILoggerFactory>().GetLogger());

And I depend on the ILogger interface in my classes that need logging:

public class MyClass
{
    private readonly ILogger _logger;
    public MyClass(ILogger logger)
    {
        _logger = logger;
    }
}

Do I have to inject the factory? I would rather not have to resolve the logger everywhere I want to use it, but perhaps this is what Simple Injector would expect?

How are other people dealing with this? Is there a better way?

freshr
  • 955
  • 1
  • 9
  • 27
  • 'NLog expects instances to be created "Instance per Dependency"'. Where did you read this? To my knowledge, you loggers can be singleton. – Steven Mar 23 '17 at 15:29
  • My statement was perhaps a bit blunt - I'm sure it can be used either way. But I wanted to make use of "LogManager.GetCurrentClassLogger()", which automatically obtains the name of the calling class and method. If the logger is a singleton, it cannot make use of this feature. (Which may be a valid compromise, depending on your needs...) – freshr Mar 24 '17 at 12:14
  • The `Logger` solves this problem. – Steven Mar 24 '17 at 12:19

1 Answers1

5

You should use context based injection as described here. This means that you create a generic Logger<T> implementation for (ideally your own defined) ILogger and register it using the context of the consumer:

container.RegisterConditional(
    typeof(ILogger),
    c => typeof(Logger<>).MakeGenericType(c.Consumer.ImplementationType),
    Lifestyle.Singleton,
    c => true);

Such Logger<T> implementation would forward the call to NLog while using the supplied T:

public class Logger<T> : ILogger
{
    private readonly NLogLogger logger = new NLogLogger(typeof(T));

    public void Log(LogEntry entry)
    {
        logger.Log(...);
    }
}
Steven
  • 166,672
  • 24
  • 332
  • 435
  • 1
    That's a great idea. Thanks! – freshr Mar 24 '17 at 12:07
  • I've looked at this in more detail and now am not so sure. The 'RegisterConditional' documentation says that the instance is cached according to the lifestyle. This means that I coud have many logger instances cached in memory, depending on where they get used. Have I got that right? – freshr Mar 24 '17 at 14:31
  • @freshr: Since `Logger` is generic, registering it as `Singleton` means, that you'll have exactly 1 instance per closed `T`. So that also means one NLog logger per consumer. This however is quite obvious if you think about it, and should not be a problem. Besides, if you inject an `ILogger` in many classes in your system you [might be doing something wrong](https://stackoverflow.com/a/9915056/264697). – Steven Mar 24 '17 at 14:43
  • 1
    I agree it should not be a problem to have these Logger instances left in memory and too many indicates a problem. I am not keen on it though, so I am just injecting a logger factory where needed for testing. I have learned a lot while investigating this, and from reading your blog. Many many thanks. – freshr Mar 24 '17 at 17:17
  • If you inject a factory, please resd this as well: https://cuttingedge.it/blogs/steven/pivot/entry.php?id=100 – Steven Mar 24 '17 at 18:09