Is it a good practice to have logger as a singleton?

Question

I had a habit to pass logger to constructor, like:

public class OrderService : IOrderService {
     public OrderService(ILogger logger) {
     }
}

But that is quite annoying, so I've used it a property this for some time:

private ILogger logger = NullLogger.Instance;
public ILogger Logger
{
    get { return logger; }
    set { logger = value; }
}

This is getting annoying too - it is not dry, I need to repeat this in every class. I could use base class, but then again - I'm using Form class, so would need FormBase, etc. So I think, what would be downside of having singleton with ILogger exposed, so veryone would know where to get logger:

    Infrastructure.Logger.Info("blabla");

UPDATE: As Merlyn correctly noticed, I've should mention, that in first and second examples I am using DI.

Answer 1

I put a logger instance in my dependency injection container, which then injects the logger into the classes which need one.

Answer 2

This is getting annoying too - it is not DRY

That's true. But there is only so much you can do for a cross-cutting concern that pervades every type you have. You have to use the logger everywhere, so you must have the property on those types.

So lets see what we can do about it.

Singleton

Singletons are terrible <flame-suit-on>.

I recommend sticking with property injection as you've done with your second example. This is the best factoring you can do without resorting to magic. It is better to have an explicit dependency than to hide it via a singleton.

But if singletons save you significant time, including all refactoring you will ever have to do (crystal ball time!), I suppose you might be able to live with them. If ever there were a use for a Singleton, this might be it. Keep in mind the cost if you ever want to change your mind will be about as high as it gets.

If you do this, check out other people's answers using the Registry pattern (see the description), and those registering a (resetable) singleton factory rather than a singleton logger instance.

There are other alternatives that might work just as well without as much compromise, so you should check them out first.

Visual Studio code snippets

You could use Visual Studio code snippets to speed up the entrance of that repetitive code. You will be able to type something like loggertab, and the code will magically appear for you.

Using AOP to DRY off

You could eliminate a little bit of that property injection code by using an Aspect Oriented Programming (AOP) framework like PostSharp to auto-generate some of it.

It might look something like this when you're done:

[InjectedLogger]
public ILogger Logger { get; set; }

You could also use their method tracing sample code to automatically trace method entrance and exit code, which might eliminate the need to add some of the logger properties all together. You could apply the attribute at a class level, or namespace wide:

[Trace]
public class MyClass
{
    // ...
}

// or

#if DEBUG
[assembly: Trace( AttributeTargetTypes = "MyNamespace.*",
    AttributeTargetTypeAttributes = MulticastAttributes.Public,
    AttributeTargetMemberAttributes = MulticastAttributes.Public )]
#endif

Answer 3

Good question. I believe in most projects logger is a singleton.

Some ideas just come to my mind:

• Use ServiceLocator (or an other Dependency Injection container if you already using any) which allows you to share logger across the services/classes, in this way you can instantiate logger or even multiple different loggers and share via ServiceLocator which is obviously would be a singleton, some kind of Inversion of Control. This approach gives you much flexibility over a logger instantiation and initialization process.
• If you need logger almost everywhere - implement extension methods for Object type so each class would be able to call logger's methods like LogInfo(), LogDebug(), LogError()

Answer 4

A singleton is a good idea. An even better idea is to use the Registry pattern, which gives a bit more control over instantiation. In my opinion the singleton pattern is too close to global variables. With a registry handling object creation or reuse there is room for future changes to instantiation rules.

The Registry itself can be a static class to give simple syntax to access the log:

Registry.Logger.Info("blabla");

Answer 5

A plain singleton is not a good idea. It makes it hard to replace the logger. I tend to use filters for my loggers (some "noisy" classes may only log warnings/errors).

I use singleton pattern combined with the proxy pattern for the logger factory:

public class LogFactory
{
    private static LogFactory _instance;

    public static void Assign(LogFactory instance)
    {
        _instance = instance;
    }

    public static LogFactory Instance
    {
        get { _instance ?? (_instance = new LogFactory()); }
    }

    public virtual ILogger GetLogger<T>()
    {
        return new SystemDebugLogger();
    }
}

This allows me to create a FilteringLogFactory or just a SimpleFileLogFactory without changing any code (and therefore complying to Open/Closed principle).

Sample extension

public class FilteredLogFactory : LogFactory
{
    public override ILogger GetLogger<T>()
    {
        if (typeof(ITextParser).IsAssignableFrom(typeof(T)))
            return new FilteredLogger(typeof(T));

        return new FileLogger(@"C:\Logs\MyApp.log");
    }
}

And to use the new factory

// and to use the new log factory (somewhere early in the application):
LogFactory.Assign(new FilteredLogFactory());

In your class that should log:

public class MyUserService : IUserService
{
    ILogger _logger = LogFactory.Instance.GetLogger<MyUserService>();

    public void SomeMethod()
    {
        _logger.Debug("Welcome world!");
    }
}

Answer 6

There is a book Dependency Injection in .NET. Based on what you need you should use interception.

In this book there is a diagram helping to decide whether to use Constructor injection, property injection, method injection, Ambient Context, Interception.

That's how one reasons using this diagram:

1. Do you have dependency or need it? - Need it
2. Is it cross-cutting concern? - Yes
3. Do you need an answer from it? - No

Use Interception

Answer 7

Another solution I personally find the easiest is to use a static Logger class. You can call it from any class method without having to change the class, e.g. add property injection etc. Its pretty simple and easy to use.

Logger::initialize ("filename.log", Logger::LEVEL_ERROR); // only need to be called once in your application

Logger::log ("my error message", Logger::LEVEL_ERROR); // to be used in every method where needed

Answer 8

If you want to look at a good solution for logging I suggest you look at google app engine with python where logging is as simple as import logging and then you can just logging.debug("my message") or logging.info("my message") which really keeps it as simple as it should.

Java didn't have a good solution for logging ie log4j should be avoided since it practically forces you to use singletons which as answered here is "terrible" and I've had horrible experience with trying to make logging output the same logging statement only once when I suspect that the reason for double logging was that I have one Singleton of the logging object in two classloaders in the same virtual machine(!)

I beg your pardon for not being so specific to C# but from what I've seen the solutions with C# look similar Java where we had log4j and we also should make it a singleton.

That's why I really liked the solution with GAE / python, it's as simple as it can be and you don't have to worry about classloaders, getting double logging statement or any design patterna at all for that matter.

I hope some of this information can be relevant to you and I hope that you want to take a look at I logging solution I recommend instead of that I bully down on how much problem Singleton get suspected due to the impossibility of having a real singleton when it must be instanciating in several classloaders.