0

I've been reviewing and refactoring a collegue's code for developing a console application that executes a set of jobs. I'd like opinions on how to improve the entry point to the system which feels like it could be a little more robust. We use NLog for logging which is configured to automatically display on the console as well as the log file. Likewise, I have a catch (Exception ex) to try and cleanly deal with and log any exceptions that slip through - in theory it should never be hit but it's always best to handle these things cleanly where possible.

I'm particularly unhappy with the logging style of having {0}: at the start of each _logger.Info() call but if refactored into its own function like so LogMe(methodName, "text to be logged"), I'm not really saving all that much typing. Keep in mind I'm deliberately leaving out the code that keeps the thread alive, etc. This is beyond the scope of what I'm looking for.

Can the following be improved upon or is this as 'good' as it can reasonably be without extensive effort/refactoring?

static void Main(string[] args)
{
    string methodName = string.Format("{0}.Main()", typeof(Program).FullName);
    try
    {
        _logger.Info("{0}: Launched", methodName);
        IKernel kernel = IOC.SetupKernel();

        _logger.Info("{0}: Reading job schedules from the configuration file");
        JobScheduleSection scheduleSection = (JobScheduleSection)ConfigurationManager.GetSection("jobScheduleSection");
        if (scheduleSection == null)
        {
            _logger.Warn("{0}: No job schedule section found in configuration file", methodName);
            return;
        }

        List<IJobSchedule> schedules = scheduleSection.JobSchedules.ToList();
        if (schedules == null)
        {
            _logger.Info("{0}: No job schedules found", methodName);
            return;
        }
        _logger.Info("{0}: Found {1} job schedules", methodName, schedules.Count);

        _logger.Info("{0}: Kicking Launcher...", methodName);
        Launcher launcher = new Launcher(kernel, schedules);
        launcher.LaunchSchedulerService();
    }
    catch (Exception ex)
    {
        _logger.ErrorException(string.Format("{0}: An unhandled exception occurred", methodName), ex);
    }
    finally
    {
        _logger.Info("{0}: Exited. Program complete.");
    }
}
DiskJunky
  • 4,750
  • 3
  • 37
  • 66
  • 3
    Wondering if this is best suited to the Code Review stack exchange: codereview.stackexchange.com .... – Arran Mar 05 '13 at 15:10
  • @Arran, possibly but it isn't my intention to review it exactly - but I'll move it if asked – DiskJunky Mar 05 '13 at 15:14

3 Answers3

3

The way I have done this is to create a wrapper class for NLog, which will wrap each log method, and obuscate methodName away and use the StackTrace object to get the method name. Then you don't have to write it every time; the method name of the method that calls the Logging wrapper method is injected automatically.

It will look cleaner as you won't have {0} and methodName everywhere.

You can take it even one step further, and create a logging wrapper class that takes the log string and an Action, executes the Action, and calls the log object using the StackTrace object all in one shot.

I've used this for executing time actions and logging them, it is convenient to do all in one call and saves on repetitive code. My method, ExecuteTimedAction(string logString, Action actionToExecute) uses a stopwatch, logs a start string, starts stopwatch, executes the method (Action delegate), stops the stopwatch, and logs again with both logs having a time stamp, name of the assembly, and name of the method that the call initiated from.

The code for getting the method is simple, use the StackTrace object, and get the StackFrame of the previous call.

        var stackTrace = new StackTrace();
        var callingMethodName = stackTrace.GetFrame(2).GetMethod().Name;

Note I have 2 hardcoded above but that is because of an additional wrapper call; if you are calling directly, then you may need GetFrame(1) instead. Best way is to use immediate window and try out the different frames, or just loop through it to see what you get, using GetFrames() method of StackTrace object.

I'm looking now into keeping the params for the string format and appending the first Param for the log wrapper. It can be done something like this:

public static class LogWrapper
{
    private static Logger _logger // where Logger assumes that is the actual NLog logger, not sure if it is the right name but this is for example

    public static void Info(string logString, object[] params)
    {
        // Just prepend the method name and then pass the string and the params to the NLog object
        _logger.Info(
            string.Concat(
                GetMethodName(),
                ": ",
                logString
            ),
            params
        );
    }

    public static void Warn(string logString, object[] params)
    {
        // _logger.Warn(
        //  You get the point ;)
        // )
    }

    private static string GetMethodName()
    {
        var stackTrace = new StackTrace(); // Make sure to add using System.Diagnostics at the top of the file
        var callingMethodName = stackTrace.GetFrame(2).GetMethod().Name; // Possibly a different frame may have the correct method, might not be 2, might be 1, etc.
    }
}

Then in your calling code, the _logger member becomes LoggerWrapper, not Logger, and you call it exactly the same way but you remove the {0} from the code. You'd need to check for nulls and maybe if there are no other params, have a method overload that just calls without the params; I am not sure if NLog supports that, so you have to check this.

... EDIT:

Just for point of interest I use this type of code in common library type of assemblies that might be referenced by a bunch of assemblies, so I can get the information such as calling assembly, method name, etc., without hardcoding it or worrying about it in my logging code. It also makes sure anyone else using the code doesn't have to worry about it. They just call Log() or Warn() or whatever and the assembly is automatically saved in the logs.

Here's an example (I know you said overkill for you but food for thought for the future if you might need something like this). In this example, I was only logging the assembly, not the method name but it can easily be combined.

    #region :           Execute Timed Action                        :

    public static T ExecuteTimedAction<T>(string actionText, Func<T> executeFunc)
    {
        return ExecuteTimedAction<T>(actionText, executeFunc, null);
    }

    /// <summary>
    /// Generic method for performing an operation and tracking the time it takes to complete (returns a value)
    /// </summary>
    /// <typeparam name="T">Generic parameter which can be any Type</typeparam>
    /// <param name="actionText">Title for the log entry</param>
    /// <param name="func">The action (delegate method) to execute</param>
    /// <returns>The generic Type returned from the operation's execution</returns>

    public static T ExecuteTimedAction<T>(string actionText, Func<T> executeFunc, Action<string> logAction)
    {
        string beginText = string.Format("Begin Execute Timed Action: {0}", actionText);

        if (null != logAction)
        {
            logAction(beginText);
        }
        else
        {
            LogUtil.Log(beginText);
        }

        Stopwatch stopWatch = Stopwatch.StartNew();
        T t = executeFunc(); // Execute the action
        stopWatch.Stop();

        string endText = string.Format("End Execute Timed Action: {0}", actionText);
        string durationText = string.Format("Total Execution Time (for {0}): {1}", actionText, stopWatch.Elapsed);

        if (null != logAction)
        {
            logAction(endText);
            logAction(durationText);                
        }
        else
        {
            LogUtil.Log(endText);
            LogUtil.Log(durationText);
        }

        return t;
    }

    public static void ExecuteTimedAction(string actionText, Action executeAction)
    {
        bool executed = ExecuteTimedAction<bool>(actionText, () => { executeAction(); return true; }, null);
    }

    /// <summary>
    /// Method for performing an operation and tracking the time it takes to complete (does not return a value)
    /// </summary>
    /// <param name="actionText">Title for the log entry</param>
    /// <param name="action">The action (delegate void) to execute</param>

    public static void ExecuteTimedAction(string actionText, Action executeAction, Action<string> logAction)
    {
        bool executed = ExecuteTimedAction<bool>(actionText, () => { executeAction(); return true; }, logAction);
    }

    #endregion

Then the Log function looks something like this, as you can see my log function is not hardcoded into the ExecuteTimedAction, so I can pass any log action to it.

In the log class I save the Entry assembly name once in a static variable and use it for all the logs...

private static readonly string _entryAssemblyName = Assembly.GetEntryAssembly().GetName().Name;

Hope this gives you enough food for thought on some refactoring!

Dmitriy Khaykin
  • 5,238
  • 1
  • 20
  • 32
  • interesting approach wrapping the logger - do you have some brief sample code? The stopwatch approach is a little overkill for what we need right not but it's definitely something I'll keep in mind for more atomically, task-based sections as the logger is already configured to log the date/time for each entry – DiskJunky Mar 05 '13 at 15:30
  • interesting! What's the performance hit on this or is there one? Is an exception thrown if the project is compiled for release? – DiskJunky Mar 05 '13 at 15:49
  • Just added some samples. No idea on the performance hit, but if the logging and code readability/maintenance is more important than then performance you have to make the call if it is worth it. The main overhead is using the stackFrame object, but I have never benchmarked it. It does make the code a lot more readable and since I am not writing stock trading applications, I think the small hit can be acceptable. This is always a trade-off in programming. – Dmitriy Khaykin Mar 05 '13 at 15:57
  • @DiskJunky - ok, I think I added enough to go by for some ideas :) good luck. – Dmitriy Khaykin Mar 05 '13 at 16:17
2

I don't particularly like that way of wrapping NLog. There is no reason for GetMethodName. NLog has the ability to provide the method name and class name automatically (by configuring the Layout correctly). When wrapping NLog (or log4net for that matter), the key is to implement the logging methods (Info, Trace, Debug) in terms of NLog.Logger.Log. One of the parameters to Log is the type of the logger (i.e. the type of the NLog wrapper). When NLog wants to write out the method name, it simply traverses up the stack trace until it finds that type. That will be the boundary between the "logger" and the application. One more step up the stack trace, and you can get the stack from for the call site. With that NLog can log the method name and the class name.

Also, the problem with a static NLog wrapper is that you lose the ability to have a logger name. Typically, the pattern for retrieving a logger is to have code like this in every class from which you might want to log:

public class MyClassFromWhichIWantToLog
{
  private static readonly Logger _logger = LogManager.GetCurrentClassLogger();

  public void DoSomething()
  {
    _logger.Info("Hello!");
  }
}

LogManager.GetCurrentClassLogger returns an instance of Logger whose "name" is the fully qualified class name of the class. Because we are using a static class variable to hold the logger, there is one logger instance per type (i.e. all instances of MyClassFromWhichIWantToLog will share the same instance of Logger). Because the logger is named for its class, you have much greater control over how your logging output is generated. You can configure NLog (via NLog.config) such that all loggers log all of the time. Or you can configure it such that only certain loggers log (or some loggers log at one level and others log at a different level). Let's say that you have a program that has various components. They all seem to be working fine, but you have to implement a new component. During development you might want to turn its logging way up (i.e. get more information), while turning other parts of your program way down (i.e. get miminal information from the parts of your program that are working ok). Also, you can redirect your logging by logger name (e.g. send all logging messages from a certain class or namespace to a certain logging Target (maybe the Debugger target if you are debugging that part of your program) and send others (including those going to the Debugger) to your output file or database). If you have a static logger wrapper, you have lost the ability to control your logging on a per class or per namespace basis.

Look at my answer to this question:

How to retain callsite information when wrapping NLog

My answer provides the source code (directly from NLog's source repository) for a NLog wrapper that maintains the correct call site info. Note that the example from NLog is more to illustrate how to extend NLog.Logger (by adding "EventID") rather than to wrap it. If you ignore the EventID stuff, you will see that the key is to pass the type of your wrapper to NLog's Logger.Log method.

Here is a very stripped down NLog wrapper (only a single method (Info)) that should wrap NLog correctly such that the call site info is preserved.

  public class MyLogger
  {
    public MyLogger(Logger logger)
    {
      _logger = logger;
    }

    private Logger _logger;
    private void WriteMessage(LogLevel level, string message)
    {
      //
      // Build LogEvent here...
      //
      LogEventInfo logEvent = new LogEventInfo(logLevel, context.Name, message);
      logEvent.Exception = exception;

      //
      // Pass the type of your wrapper class here...
      //
      _logger.Log(typeof(MyLogger), logEvent);
    }

    public void Info(string message)
    {
      WriteMessage(LogLevel.Info, message);
    }
  }

You would use it like this:

public class MyClassWhereIWantToUseLogging
{
  private static readonly _logger = new MyLogger(LogManager.GetCurrentClassLogger());

  public void DoSomething()
  {
    _logger.Info("Hello!"); //If you log call site info, you should class name and method name.
  }
}

For more NLog info, see this popular (if I do say so myself ;-)) NLog post:

Most useful NLog configurations

Community
  • 1
  • 1
wageoghe
  • 27,390
  • 13
  • 88
  • 116
1

Update

I found a much cleaner solution to this instead of trying to extend the NLog class or otherwise creating methods/method overloads. NLog supported the following field added to the NLog.config file deployed with your application;

layout="${callsite}"

This can be applied to whichever target is appropriate for you, CSV, Console, Email, etc. In a CSV, configured like;

<target name="CSVFile" xsi:type="File"  fileName="${basedir}/Logging/BullorBear.Identity.API-${date:format=yyyy-MM-dd}.csv" 
        archiveEvery="Day" maxArchiveFiles="28">
  <layout xsi:type="CSVLayout">
    <column name="Index" layout="${counter}" />
    <column name="Time" layout="${longdate}" />
    <column name="Callsite" layout="${callsite}" />
    <column name="Severity" layout="${level:uppercase=true}" />
    <column name="Detail" layout="${message}" />
    <column name="Exception" layout="${exception:format=ToString}" />
  </layout>
</target>

Output;

Index,Time,Callsite,Severity,Detail,Exception
1,2013-03-12 12:35:07.6890,ProjectName.Controllers.SomeController.SomeMethod,INFO,Authenticating...,
DiskJunky
  • 4,750
  • 3
  • 37
  • 66