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.

Solution 1

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

Solution 2

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

Solution 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()

Solution 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");

Solution 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!");
    }
}

View more solutions

Comments

[Sebastian Mach]

possible duplicate of Singleton logger, static logger, factory logger... how to log?

[Merlyn Morgan-Graham]

From the answers I'm seeing, it seems like people didn't realize you are injecting in both those examples. Can you make this clearer in the question? I've added tags to take care of some of it.

[User]

Read the comments of this article Dependency Injection Myth: Reference Passing by Miško Hevery of Google who is pretty big into testing and dependency injection. He says logging is kind of an exception where a singleton is okay unless you need to test the output of the logger (in which case use DI).

[Joel Goodwin]

First time I've come across the Registry pattern. Is it an over-simplification to say it's basically all the global variables & functions put under one umbrella?

[Anders Abel]

In its simplest form it is just an umbrella, but having it in a central place makes it possible to change it to something else (maybe a pool of objects, instance per-use, thread-local instance) when requirements change.

[Joel Goodwin]

Right. I've implemented something similar in VBA previously, although didn't know it was a pattern at the time.

[kgilden]

This is the correct way to do it - testing the logger becomes much more easier.

[Giedrius]

could you be more specific? Because I think that's what I did/do in 1 and 2 code samples in question?

[Giedrius]

Could you be more specific, what you had in mind talking about extension methods? Regarding using ServiceLocator, I'm wondering why it would be better, than property injection, like in sample 2.

[Daniel Rose]

The "using" class would look basically the same as what you already have. The actual value, however, does not have to be manually given to your class, since the DI container does that for you. This makes it much easier to replace the logger in a test, as compared to having a static singleton logger.

[sll]

@Giedrius : Regarding extension methods - you can create extension methods like public static void LogInfo(this Object instance, string message) so each class would pick it up, Regarding ServiceLocator - this allows you having logger as a regular class instance not a singleton, so you would give much flexibility

[Michael Brown]

Registry and ServiceLocator are pretty much one and the same. Most IoC frameworks are at their core an implementation of the Registry.

[GR7]

@Giedrius if you implement IoC by constructor injection and the DI framework that you're using has the ability to scan your constructors and automatically inject your dependencies (given you configured those dependencies on your DI framework bootstrap process), then the way you used to do it would work just fine. Also most DI frameworks should allow you to set each object's lifecycle scope.

[Merlyn Morgan-Graham]

Not upvoting, even though this is the right answer (IMO), because the OP already said they were doing this. Also, what is your rationale for using this vs using a Singleton? What about the issues the OP is having with repetitive code - how is that addressed by this answer?

[Daniel Rose]

@Merlyn The OP stated he has a logger as part of the constructor or as a public property. He did not state that he has a DI container inject the correct value. So I'm assuming this is not the case. Yes, there is some repeated code (but with an attributed auto property, for example, it is very little), but IMHO it is much better to explicitly show the dependency than to hide it via a (global) singleton.

[Merlyn Morgan-Graham]

@DanielRose: Ah, I guess I just recognized the pattern. It is straight out of the Castle.Windsor logging facility docs. I'll suggest fixes on the OP then.

[Merlyn Morgan-Graham]

@DanielRose: And you've changed my mind with "it is much better to explicitly show the dependency than to hide it via a (global) singleton". +1

[Piotr Perak]

ServiceLocator is not DI Container. And it is considered anti pattern.

[Niklas Rosencrantz]

+1 for Singletons are terrible. Try using singleton with multiple classloaders ie an application server environment where the dispatcher is the client where I've experienced horrible examples of double logging (but not so horrible as logging statements coming from the wrong place which some C++ programmer told me about)

[Merlyn Morgan-Graham]

@MikeBrown: Seems like the difference might be that a DI container (internally, service locator pattern) tends to be an instanced class, whereas the registry pattern uses a static class. Does that sound right?

[Merlyn Morgan-Graham]

The OP is already using DI with a DI container. The first example is ctor injection, the second is property injection. See their edit.

[Merlyn Morgan-Graham]

Nevermind my previous comment. I think I see what you're doing here. Can you provide an example of a class actually logging to this?

[Merlyn Morgan-Graham]

isn't the equivalent code in C# a Singleton (or a static class, which is potentially the same, potentially worse, as it offers even less flexibility)? using Logging; /* ... */ Logging.Info("my message");

[Merlyn Morgan-Graham]

And you can avoid singletons with the log4j pattern if you use dependency injection to inject your loggers. Plenty of libraries do this. You can also use wrappers that provide a generic interfaces so you can swap out your logging implementation at a later date, like netcommon.sourceforge.net or one of the extensions provided by DI container libraries like Castle.Windsor or Ninject

[Giedrius]

I think there's an mistake in first reasoning question (I think it should be "Do you have dependency or need it?"). Anyway, +1 for mentioning Interception, studying it's possibilities in Windsor and it looks very interesting. If you could additionally give example how you imagine it would fit here, it would be great.

[Merlyn Morgan-Graham]

+1; Definitely beats a naked singleton :) Still has some slight coupling and potential for static-scope/threading issues due to using static objects, but I imagine those are rare (you'd want to set Instance in the application root and I don't know why you'd reset it)

[jgauffin]

@MerlynMorgan-Graham: It's unlikely that you change a factory after you set it. Any modifications would be done in the implementing factory and you got full control over that. I wouldn't recommend this pattern as a universal solution for singletons, but it works well for factories since their API seldom changes. (so you could call it proxied abstract factory singleton, hehe, pattern mash-up)

[Merlyn Morgan-Graham]

+1; Interesting suggestion - would basically give AOP-like functionality without having to touch the types. My opinion (which is based on very limited exposure) is that interception via proxy generation (the type a DI library can provide as opposed to an AOP attribute-based library) feels like black magic, and can make it somewhat hard to understand what is going on. Is my understanding on how this works incorrect? Anyone had a different experience with it? Is it less scary than it sounds?

[Piotr Perak]

If you feel like Interception is to much 'magic' then you can just use decorator design pattern and implement it yourself. But after that you will probably realize that it's a waste of time.

[Merlyn Morgan-Graham]

I assumed this suggestion would automatically wrap each call in logging, as opposed to letting (making) the class log itself. Is that correct?

[Piotr Perak]

Yes. That's what decorator does.

[Domenic]

@NickRosencrantz +1 for the C++ horror story; I love spending a couple minutes contemplating the terribleness of singletons only to scroll down and go "Haha lol at least I don't have their problems."

[Onur]

@MichaelBrown The difference is where you use the registry/service locator. If it's just in the composition root, it's fine; if you use in many places, it's bad.

[Brennen Sprimont]

</flame-suit-on> I don't know how you lived in a flame suit for 5 years but I hope this helps.

[VivekDev]

But the question is weather to configure this as singleton or transient?

[The Godfather]

That's the problem! No one never ever uses logging.info("my message") in a program more complicated than hello world. Usually you do quite a lot of boilerplate initializing the logger - setting up the formatter, the level, setting up both file and console handlers. It's never ever logging.info("my message")!