10

This question is related to Steven’s answer - here. He proposed a very good logger wrapper. I will paste his code below:

public interface ILogger
{
    void Log(LogEntry entry);
}

public static class LoggerExtensions
{
    public static void Log(this ILogger logger, string message)
    {
        logger.Log(new LogEntry(LoggingEventType.Information,
            message, null));
    }

    public static void Log(this ILogger logger, Exception exception)
    {
        logger.Log(new LogEntry(LoggingEventType.Error, 
            exception.Message, exception));
    }

    // More methods here.
}

So, my question is what is the proper way to create implementation that proxies to log4net? Should I just add another Log extension method with type parameter and then create a switch inside? Use different log4net method in case of LoggingEventType ?

And second question, what is the best way to use it later in the code?

Because he wrote:

(…) you can easily create an ILogger implementation (…) and configure your DI container to inject it in classes that have a ILogger in their constructor.

Does that mean that every class that will log sth (so basically every), should have ILogger in its constructor?

Community
  • 1
  • 1
Tim Laax
  • 149
  • 1
  • 2
  • 12
  • 1
    "Does that mean that every class that will log sth (so basically every)". If every class uses a logger, you are seriously [logging way too much](https://stackoverflow.com/questions/9892137/windsor-pulling-transient-objects-from-the-container). – Steven Sep 02 '15 at 10:09
  • 2
    @Steven contrary to the upvotes and comments on that answer, I thoroughly disagree: you can never log too much. Especially with web services and Windows Services (i.e. backends), where there is little to no error reporting to a user facing a UI, logging is invaluable to troubleshooting issues. Sure, you can say "This code is SOLID", but if that code cannot be "post mortem debugged" by simply reading the log to analyze what the application was doing when it crashed, then what does SOLID bring you? Sure, there's unit tests to prevent issues, but code is never flawless. – CodeCaster Sep 02 '15 at 13:20
  • 1
    @CodeCaster: I'm afraid you misunderstood my answer. I'm not saying you shouldn't log. What I'm saying is that you should have less lines of code in your application that are concerned with logging. And you should probably throw exceptions more often. – Steven Sep 02 '15 at 13:22
  • @Steven that answer seems to assume you only want to log exceptions. That's what I disagree with. Decisions (`if() {...} else {...}`), element counts, return values and the likes are very useful when trying to figure out why a piece of code didn't do what it should. If you want less lines that do that, you could look into aspect oriented programming, for example using PostSharp, but again, you can't log too much. Of course `catch (Exception e) { Log(e); throw; }` is a silly pattern in general, that I agree with. – CodeCaster Sep 02 '15 at 13:25
  • 2
    @CodeCaster: The method I'm proposing in my answer is actually a form of AOP. If you define the right abstractions in your systems (which is what my answer is about), you'll find it easy to apply a few decorators that do logging for you. Mix this with Clean Code and and fail fast using exceptions, and you'll find that calls like `logger.Log("now we're in this if-branch")` and `logger.Log("customer is null")` become actually quite rare. – Steven Sep 02 '15 at 13:39
  • @Steven again, I'm talking about post mortem debugging by reading traces/logs. Not every situation that I wish to read about in such logs is a fail situation, so you don't want to exit the method nor throw an exception. Not all clean code practices are practical in our not-so-clean real world. – CodeCaster Sep 02 '15 at 13:40
  • @Steven wouldn't it be easier, better and cleaner to just use singleton pattern here? one global instance (in global namespace) that can be used by every class? I know global variables are evil, but in that case we're talking about just a single instance. – Tim Laax Sep 02 '15 at 16:48
  • 3
    It’s a design thing. A controlled set of abstractions and reduced cyclomatic complexity. All runtime data is either an input parameter or some form of stateful data being pulled from storage (database, memory etc.), all of which can be logged using aspects/decorators. Having details of the data and the sequence of method calls as the data moves through the object graph can often be enough to figure out what is going on without polluting whole swathes of the code with the same repeating line of code over and over again `x.Log("x was here");`. Like I said, it’s a design thing. – qujck Sep 02 '15 at 18:41
  • 3
    @TimLaax: Making that logger a singleton / ambient context, doesn't change the fact that it is a dependency; but it does make the dependency hidden. This makes it hard to test, mock, replace,decorate and intercept, hides the fact for any consumer that this dependency exist, and makes it impossible for the a tool (such as your DI library) to analyze the object graph for you. In my book, making the logger a singleton is absolutely not better nor cleaner. Making it a singleton is treatment of symptoms. You are injecting the logger in way too many classes: stop doing that. – Steven Sep 02 '15 at 19:46
  • @Steven actually, when I was starting my project (I started from logger design) I didn't think of logger as sth that needs to be tested. I thought of it like of sth... well, basically hidden tool, but flexible - so, easy to change its provider or output. After reading all those comments and answer I know I have a lot of reading because of few new things that came out here – Tim Laax Sep 02 '15 at 23:04
  • Take a look (this is an example with Microsoft Extensions DependencyInjection): https://github.com/carloscalvin/log4netInjectorAdapter – Carlos Calvin Palomares Apr 25 '18 at 22:24

1 Answers1

14

So, my question is what is the proper way to create implementation that proxies to log4net?

you should create something like:

public class Log4netAdapter : ILogger
{
    private readonly log4net.ILog m_Adaptee;

    public Log4netAdapter(log4net.ILog adaptee)
    {
        m_Adaptee = adaptee;
    }

    public void Log(LogEntry entry)
    {
        //Here invoke m_Adaptee
        if(entry.Severity == LoggingEventType.Debug)
            m_Adaptee.Debug(entry.Message, entry.Exception);
        else if(entry.Severity == LoggingEventType.Information)
            m_Adaptee.Info(entry.Message, entry.Exception);
        else if(entry.Severity == LoggingEventType.Warning)
            m_Adaptee.Warn(entry.Message, entry.Exception);
        else if(entry.Severity == LoggingEventType.Error)
            m_Adaptee.Error(entry.Message, entry.Exception);
        else
            m_Adaptee.Fatal(entry.Message, entry.Exception);
    }
}

Does that mean that every class that will log sth (so basically every), should have ILogger in its constructor?

As I understand from Stevens answer: Yes, you should do this.

what is the best way to use it later in the code?

If you are using a DI container, then just use the DI container to map ILogger to Log4netAdapter. You also need to register log4net.ILog, or just give an instance of log4net logger to the DI container to inject it to the Log4netAdapter constructor.

If you don't use a DI container, i.e., you use Pure DI, then you do something like this:

ILog log = log4net.LogManager.GetLogger("MyClass");

ILogger logging_adapter = new Log4netAdapter(log);

var myobject = new MyClass(other_dependencies_here, logging_adapter);
ErikE
  • 48,881
  • 23
  • 151
  • 196
Yacoub Massad
  • 27,509
  • 2
  • 36
  • 62
  • 1
    I have nothing much to add here (+1 for me), except as like I said in the comment that if you are injecting `ILogger` into almost all components in your system you are either [logging too much or you are violating the SOLID principles](https://stackoverflow.com/questions/9892137/windsor-pulling-transient-objects-from-the-container). – Steven Sep 02 '15 at 12:52
  • @Steven So this basically means, that in every class when error occurs (or some situation unwanted) I need to throw "new MyException" or rethrow exception that occured with just `throw;`. This is about errors/exceptions. What about info logging? Like in messaging server I want to log every comming and going message id. I have to add another parameter with logger to that class. Actually every class that needs to log "Info" or "Warning" and not "Error" has to have logger in its constructor (if it's not inherited). Do you agree with that last statement? – Tim Laax Sep 02 '15 at 23:04
  • @TimLaax you should read that referenced answet again closely. What the answer tells is that with the right set of abstractions, a single decorator can be created that can be wrapped around a large set of classes, that allows you to implement logging for debugging, monitoring and audit trailing in an uniform way and in a few lines of code. This prevents you from having to inject the logger everywhere. In case of errors, exceptions should be thrown. These can be logged in the top most layer of your application. – Steven Sep 03 '15 at 06:25
  • @Steven Thanks! I think I got the theory. But can you recommend a sample ready to use project with such logger used through DI container so I could see the practice side? – Tim Laax Sep 03 '15 at 10:58
  • 1
    @TimLaax: That's hard to advice. I can point you to my articles ([here](https://www.cuttingedge.it/blogs/steven/pivot/entry.php?id=91), [here](https://www.cuttingedge.it/blogs/steven/pivot/entry.php?id=92) and [here](https://www.cuttingedge.it/blogs/steven/pivot/entry.php?id=95)) about applying a design that allows you to apply cross-cutting concerns (such as logging) with ease, and you can look at this [example project](https://github.com/dotnetjunkie/solidservices) to get a small working example, but I'm not aware of ready-to-use sample projects that show logging specifically. – Steven Sep 03 '15 at 11:17
  • I will definitely read them. Thank you @Steven and everybody else for your help – Tim Laax Sep 04 '15 at 00:23
  • 1
    @Steven, why would the adapter accept an `ILog` at its constructor, and not instantiate a Log4Net object directly? Is that to allow testing the adapter? – Tsahi Asher Jun 27 '17 at 08:37
  • @TsahiAsher calling `LogManager.GetLogger` is absolutely valid. I see no disadvantage in doing so. – Steven Jun 27 '17 at 08:43
  • @yacoubmassad What do you do if you use DI? – Krptodr Dec 31 '19 at 18:48
  • 1
    Nevermind I found an example written by @Steven and it can be found here: https://stackoverflow.com/a/25113659/3311255 – Krptodr Dec 31 '19 at 20:44
  • @Steven, can you provide an example of using this example implementation using SimpleInjector as the DI Container? I can't register log4net.ILog in my container. My interface ILogger is implemented by Log4NetAdapter which accepts ILog in it's constructor. I then have constructors on my objects that I wish to log accepting a parameter of ILogger. ILogger is supposed to be injected, but has always failed to do so. – Krptodr Dec 31 '19 at 23:13
  • 1
    @Krptodr please post a new question here on SO with a full example and details and tag it with `simple-injector`. I will have a look after my hangover is gone :) – Steven Dec 31 '19 at 23:20