In C# should my Common.Logging logger be an instance member or static?

Question

Looking a project that uses Common.Logging for .NET, I noticed that some classes declare the logger instance as a class static member. For instance:

public class HelloJob : IJob
{
    private static ILog _log = LogManager.GetLogger(typeof(HelloJob));

    public HelloJob()
    {
    }

    public virtual void  Execute(IJobExecutionContext context)
    {
        _log.Info(string.Format("Hello World! - {0}", System.DateTime.Now.ToString("r")));
    }
}

And in other classes the logger is declared as an instance member:

public class SimpleExample : IExample
{
    public virtual void Run()
    {
        ILog log = LogManager.GetLogger(typeof (SimpleExample));

        log.Info("------- Initializing ----------------------");

        // etc
    }
}    

Is there a reason to prefer one approach or the other?

In which cases is each approach recommended? Is it related to thread safety?

Would it be a problem if I just declared a "Logger" class with a static "logger" member and the whole project used that (apart from the issue that I would in practice have a global variable)?

Alluring

Answer 1

Most loggers are thread safe, and creating instances of them has very little overhead, both in terms of time and memory. So the real question needs to be what makes sense from a programming and maintainability standpoint.

On the one hand, since the logger is conceptually tied to your class, and not to the instance of the class, a lot of people prefer to keep it static. That's a perfectly valid argument. For example, if HelloWorldJob extends HelloJob, I think most people would expect the log message written by code in HelloJob to be tied to the HelloJob class, even though you have a more specific subclass instance. It's also nice to be able to access your logger from static methods, which wouldn't be possible if it's not on a static field.

On the other hand, there is no reason that your HelloJob should be responsible for getting its own logger instance. There's a lot to be said for using dependency injection instead (unit testability, additional configurability, and simpler code). So I personally suggest having your logger injected by a DI framework, in which case it would need to be referenced on a per-instance field.

public class HelloJob : IJob
{
    private readonly ILog _log;

    public HelloJob(ILog log)
    {
        _log = log;
    }
    ...
}

Your DI framework can set up the logger based on details it knows at run-time, or you can provide a fake or mocked logger in your unit tests to make sure the expected log messages are produced. Note that even though you are referring to a per-instance field, you're perfectly free to still use a per-class (or even a singleton) instance--those are just details that don't need to be part of this class's concern.

Update

Over time I've shifted my opinion more in the direction of using a static logger. More specifically, I really like using Fody Anotar libraries to generate the static logger (and other helpful code) for me, so all I have to write in my source code is logging statements like:

LogTo.Info("This is a message");

Cheekbone

LogManager.GetLogger is probably internally implemented via dependency injection; that is, the dependency is managed within the logger. I would not want my loggers stored as a "per-instance field" and getting that overhead for each instance created. Statically storing it once when the class is first loaded works for me. – Benumb

@ChrisSinclair: It's true that the instantiation details are handled by LogManager.GetLogger, and I like that Commons.Logging provides a common logging interface, leaving you free to switch the underlying frameworks, configurations, etc. However, this is an example of the Factory Pattern, not Dependency Injection. The Factory Pattern carries a lot of advantages over manual instantiation, but Dependency Injection still carries additional advantages. Your DI framework can still be set up to reuse a single logger for each type, and the overhead is practically immeasurable 90% of the time. – Cheekbone

The Common.Logging documentation states that public static LogManagers are thread safe, but instance methods are not, which seems to run counter to this answer. netcommon.sourceforge.net/docs/2.0.0/api/html/… – Downrange

@coderjoe: Since LogManager is a static class, you cannot create an instance of it. All the methods you can call on LogManager are static and thread-safe. The logger they create technically has no guarantees because it depends on the Adapter you're using, but all of the standard adapters produce thread-safe loggers, so you should be fine unless you're using a homebrew logger. Hence, "most" loggers are thread safe. – Cheekbone

@Cheekbone would be very interested when you updated your opinion? I am also looking for nice and easy logging best practices and there seem to be hardly alternatives to DI stuff like .NET core does with the generic hosts?! – Russell

@FalcoAlexander: My opinion changed slowly over the years. I updated this post in Feb 2020. These days I would recommend Serilog as a logging framework, using the appropriate Fody Anotar package so you can write simple code and still have it automatically include context as part of the compile step. While I think it's great that modern frameworks have built-in support for DI, I'm not sold on the value of injecting the logger--especially the way Azure Functions uses method injection (rather than constructor injection) to provide an ILogger, and you're expected to pass that around. Yuck. – Cheekbone