Web job logger implementation

Question

Interface for logger:

 public interface ILogger
        {
            void Log(Exception ex, string contextualMessage = "");

            void Log(string message);
        }

Interface for web job logger:

public interface IJobLogger : ILogger
{
    void SetWriter(TextWriter logWriter);
}

Web job logger:

 public class WebJobLogger : IJobLogger
    {
        private TextWriter writer;

        public WebJobLogger()
        {
            writer = TextWriter.Null;
        }

        public void SetWriter(TextWriter logWriter)
        {
            writer = logWriter;
        }

        public void Log(Exception ex, string contextualMessage = "")
        {
            writer.WriteLine("Logger: Exception thrown with message {0}, exception : {1}", contextualMessage, ex);
        }

        public void Log(string message)
        {
            writer.WriteLine("Logger: {0}", message);
        }
    }

Usage:

public async Task TestJob(
    [QueueTrigger(QueueName.StorageExcelImport)] StartImportCommand blobInfo,
    [Blob(Global.BlobContainerName + BlobReferenceName, FileAccess.Read)] Stream input,
    TextWriter logWriter)
{
    await logWriter.WriteLineAsync("Start");
    try
    {
        //Implemetation
    }
    catch (Exception ex)
    {
        //Set writer
        log.SetWriter(logWriter);
        //Log exception
        log.Log(ex);
    }
    await logWriter.WriteLineAsync(string.Format("File: '{0}' successfuly processed from blob to the database. ({1})", blobInfo.OriginalFileName, DateTime.UtcNow));
}

Dependency injection:

container.Register<ILogger, WebJobLogger>();

The problem with this is that I always have to call log.SetWriter(logWriter); before call log.Log(ex);. If I forget that, then will not work. The whole point of this is to mock logger in unit test. What do you think?

Answer 1

void Log(Exception ex, string contextualMessage = "");

This isn't bad, but seems perhaps a bit redundant, since exceptions already have messages. A more idiomatic option might be to wrap the exception in another, with the custom message you want. However, if a "contextual message" conceptually serves a different purpose than the message on an exception, what you have may be fine.

void SetWriter(TextWriter logWriter);

You're right that this is problematic. What it means is that your class can be in a "not fully initialized" state. Like you mentioned, that means you have to remember to initialize it. It also means that if you ever pass it around, you can't rely on your configuration staying the same- some other code might change the logWriter without you realizing.

There's a pretty common solution to this- just pass it into the constructor. That way a WebJobLogger always has one TextWriter for its entire lifetime, which won't be changed. I'm not immediately sure which IoC container you're using, but any mainstream one will have support for constructors with parameters- you just need to make sure the dependencies are bound too.

await logWriter.WriteLineAsync("Start");

Shouldn't you be using the ILogger.Log method for this too? It's confusing to access the underlying writer from the same different place at two different abstraction levels.