HiveBrain v1.2.0
Get Started
← Back to all entries
principlecsharpMinor

Logging strategy setup

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
loggingsetupstrategy

Problem

I finally set up my logging infrastructure to work as desired, however I feel like I had to do quite a lot things just to fulfill a few requirements. Now I'm worried if my approach has major drawbacks, e.g. in performance or stability, so I need your help!

Requirements

  • Logging to multiple output sources, depending on configuration


and environment (e.g. Console or Windows Service)

  • Pre-evaluation of logging conditions for formatting, which means: If the message won't be logged, nothing gets formatted or converted to improve performance and memory usage



  • Abstraction of logging implementation for use within domain & infrastructure layer (application is split up according to the Onion Architecture)



  • Ability to inject logging abstraction into classes (DI)



Framework
I've chosen log4net as the framework behind because of it's straight-forward configuration and variety of listeners (appenders)

Implementation

Basically, everytime I need to have logging abilities in a class, I request a custom ILogger interface via the constructor. This interface will be served using dependency injection with Ninject as my IoC container. The interface, among with extension methods etc. is implemented in a "Core" library every other project can reference to (library for cross cutting concerns).

The actual logger (log4net adapter) is implemented in a infrastructure project called "Infrastructure.Logging". The adapter itself requests a implementation of the log4net facade ILog via constructor, which will also be served by Ninject. It routes the calls to ILogger forward to log4net with the appropriate log4net level.

Everything gets hooked up at the start of the application. More than 4 appenders will be registered, which can be activated/deactivated using log4net property filters. Also, the ILog interface is bound to a log4net logging instance using a method binding to LogManager.

MyApp.Core

ILogger

```
public enum EventSeverity
{
Informational,
Warning,

Solution

Using extension methods and the interface makes me think you should use an abstract class to encapsulate the logging framework you use. I put the Write method and the overloads protected but you could put it public back if you need. Also, as you'll see it, I added brackets to your one line's if. I believe that even if it adds some lines of code, it gives a much better readability to your code.

public abstract class Logger
{
    public abstract bool IsDebugEnabled { get; }
    public abstract bool IsVerboseEnabled { get; }
    public abstract bool IsInformationalEnabled { get; }
    public abstract bool IsWarningEnabled { get; }
    public abstract bool IsFatalEnabled { get; }
    public abstract bool IsErrorEnabled { get; }

    protected abstract void Write(string message, EventSeverity severity);
    protected abstract void Write(string message, Exception exception, EventSeverity severity);

    public void LogInformational(string message)
    {
       if (IsInformationalEnabled)
       {
          Write(message, EventSeverity.Informational);
       }
    }

    public void LogInformational(string format, params object[] args)
    {
       if (IsInformationalEnabled)
       {
          Write(String.Format(format, args), EventSeverity.Informational);
       }
    }

    public void Warning(string message)
    {
       if (IsWarningEnabled)
       {
          Write(message, EventSeverity.Warning);
       }
    }

    //etc..
}


Edit :

I think you should keep your interface as clear as possible. Your contract should define the minimum you need, which is both Write methods. The other methods, as you mentioned, might not be supported by your logging framework, which means they wouldn't comply to the interface. Also, these methods have a little problem, let me give you an example. If IsInformationalEnabled = false and you call LogInformational, you have no feedback that the information wasn't logged, which is a problem. I think your Write method should deal with this, to keep it as simple as possible. Look at the following approach, I think it is an okay way to abstract your logger.

public abstract class Logger
{
    protected abstract bool IsDebugEnabled { get; }
    protected abstract bool IsVerboseEnabled { get; }
    protected abstract bool IsInformationalEnabled { get; }
    protected abstract bool IsWarningEnabled { get; }
    protected abstract bool IsFatalEnabled { get; }
    protected abstract bool IsErrorEnabled { get; }

    public void Write(string message, EventSeverity severity)
    {
       ValidateSeverityEnabled(severity);
       WriteToLog(message,severity);
    } 

    public void Write(string message, Exception exception, EventSeverity severity)
    {
       ValidateSeverityEnabled(severity);
       WriteToLog(message, exception, severity);
    }

    protected abstract void WriteToLog(string message, EventSeverity severity);
    protected abstract void WriteToLog(string message, Exception exception, EventSeverity severity);

    private void ValidateSeverityEnabled(EventSeverity severity)
    {
       switch (severity)
       { 
          case EventSeverity.Informational:
             if (!IsInformationalEnabled)
                throw new InvalidOperationException("Informational log disabled");
             break;
          case EventSeverity.Warning:
             if (!IsWarningEnabled)
                throw new InvalidOperationException("Warning log disabled");
             break;
          case EventSeverity.Error:
             if (!IsErrorEnabled)
                throw new InvalidOperationException("Error log disabled");
             break;
          case EventSeverity.Fatal:
             if (!IsFatalEnabled)
                throw new InvalidOperationException("Fatal log disabled");
             break;
          case EventSeverity.Debug:
             if (!IsDebugEnabled)
                throw new InvalidOperationException("Debug log disabled");
             break;
          case EventSeverity.Verbose:
             if (!IsVerboseEnabled)
                throw new InvalidOperationException("Verbose log disabled");
             break;
       }
    }
}

Code Snippets

public abstract class Logger
{
    public abstract bool IsDebugEnabled { get; }
    public abstract bool IsVerboseEnabled { get; }
    public abstract bool IsInformationalEnabled { get; }
    public abstract bool IsWarningEnabled { get; }
    public abstract bool IsFatalEnabled { get; }
    public abstract bool IsErrorEnabled { get; }

    protected abstract void Write(string message, EventSeverity severity);
    protected abstract void Write(string message, Exception exception, EventSeverity severity);

    public void LogInformational(string message)
    {
       if (IsInformationalEnabled)
       {
          Write(message, EventSeverity.Informational);
       }
    }

    public void LogInformational(string format, params object[] args)
    {
       if (IsInformationalEnabled)
       {
          Write(String.Format(format, args), EventSeverity.Informational);
       }
    }

    public void Warning(string message)
    {
       if (IsWarningEnabled)
       {
          Write(message, EventSeverity.Warning);
       }
    }

    //etc..
}
public abstract class Logger
{
    protected abstract bool IsDebugEnabled { get; }
    protected abstract bool IsVerboseEnabled { get; }
    protected abstract bool IsInformationalEnabled { get; }
    protected abstract bool IsWarningEnabled { get; }
    protected abstract bool IsFatalEnabled { get; }
    protected abstract bool IsErrorEnabled { get; }

    public void Write(string message, EventSeverity severity)
    {
       ValidateSeverityEnabled(severity);
       WriteToLog(message,severity);
    } 

    public void Write(string message, Exception exception, EventSeverity severity)
    {
       ValidateSeverityEnabled(severity);
       WriteToLog(message, exception, severity);
    }

    protected abstract void WriteToLog(string message, EventSeverity severity);
    protected abstract void WriteToLog(string message, Exception exception, EventSeverity severity);

    private void ValidateSeverityEnabled(EventSeverity severity)
    {
       switch (severity)
       { 
          case EventSeverity.Informational:
             if (!IsInformationalEnabled)
                throw new InvalidOperationException("Informational log disabled");
             break;
          case EventSeverity.Warning:
             if (!IsWarningEnabled)
                throw new InvalidOperationException("Warning log disabled");
             break;
          case EventSeverity.Error:
             if (!IsErrorEnabled)
                throw new InvalidOperationException("Error log disabled");
             break;
          case EventSeverity.Fatal:
             if (!IsFatalEnabled)
                throw new InvalidOperationException("Fatal log disabled");
             break;
          case EventSeverity.Debug:
             if (!IsDebugEnabled)
                throw new InvalidOperationException("Debug log disabled");
             break;
          case EventSeverity.Verbose:
             if (!IsVerboseEnabled)
                throw new InvalidOperationException("Verbose log disabled");
             break;
       }
    }
}

Context

StackExchange Code Review Q#57909, answer score: 5

Revisions (0)

No revisions yet.