Benefits of Log4j singleton wrapper?

Question

I have recently inherited some Java code and need to integrate it into a project that I am working on. My project is a service agent that processes and transforms XML messages. While looking through the new code, I discovered the following logging class:

import org.apache.log4j.BasicConfigurator;
import org.apache.log4j.Level;
import org.apache.log4j.Logger;

public class MyLogger {

    private static MyLogger instance = null;
    protected final static Logger log = Logger.getLogger(MyLogger.class);

    private MyLogger() {
        super();
    }

    public static MyLogger getInstance(){
        if(instance  == null){
            instance  = new MyLogger();
            BasicConfigurator.configure();
            log.setLevel(Level.ALL);
        }
        return instance;
    }

    public void info(String myclass, String msg) {
        log.info("[" + myclass + "] " + msg);

    }

    public void error(String myclass, String msg, Exception ce) {               
        log.error("[" + myclass + "] " + msg, ce);      
    }

    public void warning(String myclass, String msg) {
        log.warn("[" + myclass + "] " + msg);
    }    
}

This class basically wraps log4j with (another) singleton. All of the logging in the classes that I need to integrate look something like this:

public class MyClass {
   private final static MyLogger log = MyLogger.getInstance();
   private final static String myclass = MyClass.class.getName();

   ...

   log.info(myclass, "Information message...");   
}

I do not see any obvious benefit to using an extra class for logging, thus I am considering refactoring this code to remove the MyLogger class and log in the following fashion:

import org.apache.log4j.Logger;

public class MyClass {
   private static Logger log = Logger.getLogger(MyClass.class);

   ...

   log.info("Information Message...");     
}

This would make the logging mechanism consistent across the project. Before I do this, I would like to know if there are any benefits to wrapping Log4j with a singleton class that I may be missing. Thanks!

EDIT: Thanks everyone for the helpful answers - I pickup up several new insights from each. Accepted Nathan Hughes' answer for pointing out lost functionality by leaving the class intact - I had been assuming that the biggest downside to leaving the singleton alone was simply code bloat. I will trash the class.

Prem

What logging backend is used in the project? – Vote

It seems like the author of this wanted to use log4j without any of the benefits of log4j's features – Brewington

@FractalizeR, if you read the code org.apache.log4j.Logger. – Iphagenia

@FractalizeR - we are currently using SyslogAppenders to send all log files to a central server. – Prem

Answer 1

Get rid of it. Using this monstrosity means all logging that goes through it will be listed with the same logger (MyLogger) and method (which is why the arguments to its methods include the class of the thing being logged). That means not only do you have to add any class, method, and line number information to each logger call, but you can't do any filtering on log levels for different packages the way you could using the typical log4j approach with classes as loggers.

This thing is a piece of junk and you are better off without it.

If you are considering wrapping the logger, first make sure you are familiar with the features your logger is providing, and be aware of how configurable your logger is. You may be able to get the functionality you want by replacing one of the logger components. For instance if youre required to emit logs as json in some arbitrary format, you may be able to find an encoder to do that. Extending your logger with custom components can provide the functionality you need without restricting you the way using a wrapper does.

Remitter

He is adding the class name as part of the log.info/debug methods. – Sterlingsterlitamak

@Sterlingsterlitamak that isn't very valuable though - one of the main benefits of log4j/slf4j/etc is the ability to configure logging per logger. Having all logging in the application done through one logger removes this very valuable feature. – Brewington

Yep. I just realized that and edited my post as well. Thanks @matt – Sterlingsterlitamak

This is not entirely true, yes the way he is using the logger will cause that but log4j includes a log method meant for wrappers where you provide from the wrapper the class name and then it knows to filter that out when logging class/method/line – Zerelda

What is the ideal way then to create a wrapper on top on log4j2. Just create a wrapper, add custom logic and not make it singleton? – Squire

@MehulParmar: i added to the answer to try to address your question. – Remitter

Answer 2

The only benefit that I could see is that it would be easy to swap out the log4j implementation with another logging implementation, or get the logging to do something much more customised such as log to one of your own databases.

That said, I would still refactor the code to use log4j directly. Or, more likely in my case, to use SLF4J.

Phenomenal

+1 - I suspect that this was the developer's intent. He tended to view almost any 3rd party imports statement as a "code smell" that required decoupling. – Prem

+1 for the recommendation to use SLF4J -- it's everything that the posted code wanted to be, except done correctly. – Jolty

@Daniel - yes, we started using it in a project recently and I haven't looked back since. Generally a well designed logging framework, or facade :) – Phenomenal

Answer 3

One thing your inherited code does that log4j does is make thing non-thread safe. Since there is no locking in getInstance() you can potentially hand out more than one instance and break the singleton intentions of the code.

You also lose the ability to set logging levels for each class depending on what you are doing.

Myrica

Answer 4

The only flaw I can tell is, because of this declaration:

protected final static Logger log = Logger.getLogger(MyLogger.class);

The logger is, essentially, hooked to object MyLogger and all logging information/errors/warnings, etc. will be "linked" to MyLogger. You have no idea which object added the logging information, none whatsoever.

The only advantage I see with this Singleton, is:

• Single instantation: You never have to worry of declaring a static final Logger implementation all the time.
• You don't have to worry what type of Logger you are using. One can change the type of Logger only in this Singleton class. Any further changes to logging is done only in the Singleton.

I have seen this in my company as well, but I don't suggest that type of setup. Rather use SLF4J or the Java Logging Framework.

Iphagenia

How to add custom logic using SLF4J? Do we need to maintain a map in log4j2 wrapper? – Squire