Web API Custom Error Logging

Question

The goal of our application was to use the same custom logging we created for our ASP.NET MVC applications. We implemented a solution that used some of the techniques in ElmahExceptionLogger:

WebApiApplication (Global.asax.cs)

protected void Application_Start()
{
    var serializerSettings = GlobalConfiguration.Configuration.Formatters.JsonFormatter.SerializerSettings;
    var contractResolver = (DefaultContractResolver)serializerSettings.ContractResolver;

    contractResolver.IgnoreSerializableAttribute = true;

    GlobalConfiguration.Configure(WebApiConfig.Register);
    GlobalConfiguration.Configuration.Services.Add(typeof(IExceptionLogger), new CustomExceptionLogger());
    GlobalConfiguration.Configuration.Filters.Add(new TokenAuthenticationAttribute());
}

CustomExceptionLogger

/// <summary>
/// Modified from ELMAH, 
/// URL: https://aspnet.codeplex.com/SourceControl/latest#Samples/WebApi/Elmah/Elmah.Server/ExceptionHandling/ElmahExceptionLogger.cs
/// </summary>
public class CustomExceptionLogger : ExceptionLogger
{
    private const string HttpContextBaseKey = "MS_HttpContext";

    public override void Log(ExceptionLoggerContext context)
    {
        HttpContext httpContext = GetHttpContext(context.Request); // Retrieve the current HttpContext instance for this request.
        NameValueCollection serverVariables = new NameValueCollection();

        LogException logException = new LogException();

        try
        {
            if (httpContext != null && httpContext.Request != null && httpContext.Request.ServerVariables != null)
                serverVariables = httpContext.Request.ServerVariables;

            logException.Log(context.Exception, serverVariables);
        }
        catch (Exception e)
        {
            // swallow error because writing to log is failing
        }
    }
    private static HttpContext GetHttpContext(HttpRequestMessage request)
    {
        HttpContextBase contextBase = GetHttpContextBase(request);

        if (contextBase == null)
        {
            return null;
        }

        return ToHttpContext(contextBase);
    }

    private static HttpContextBase GetHttpContextBase(HttpRequestMessage request)
    {
        if (request == null)
        {
            return null;
        }

        object value;

        if (!request.Properties.TryGetValue(HttpContextBaseKey, out value))
        {
            return null;
        }

        return value as HttpContextBase;
    }

    private static HttpContext ToHttpContext(HttpContextBase contextBase)
    {
        return contextBase.ApplicationInstance.Context;
    }
}

LogException

public class LogException
{
    public bool Log(Exception exception, NameValueCollection serverVariables)
    {
        var logService = new LogService();
        int loop1, loop2;
        StringBuilder serverVariableKeys = new StringBuilder();
        bool logged = false;

        if (HttpContext.Current != null)
        {
            // Get names of all keys into a string array
            string[] allKeys = serverVariables.AllKeys;

            for (loop1 = 0; loop1 < allKeys.Length; loop1++)
            {
                if (allKeys[loop1].ToUpper() != "ALL_HTTP" && allKeys[loop1].ToUpper() != "ALL_RAW")
                {
                    serverVariableKeys.Append(allKeys[loop1] + " : ");

                    string[] values = serverVariables.GetValues(allKeys[loop1]);

                    for (loop2 = 0; loop2 < values.Length; loop2++)
                    {
                        serverVariableKeys.AppendLine("Value " + loop2 + ": " + values[loop2]);
                    }
                }
            }
        }

        if (exception != null)
        {
            exception.Data.Add("ServerVariables", serverVariableKeys.ToString());
            logged = logService.LogException(exception, exception.TargetSite.Name);
        }

        return logged;
    }
}

I haven't include LogService source because this is specific to where it gets logged to, which can be a database or text file.

Any suggestions for improvement?

Answer 1

Here are my suggestions on your code.

CustomExceptionLogger

1. The constructor call for the serverVariables can be removed, since it is not used at all.
2. The empty catch statement gives a warning in Visual Studio, so I used a private static void SwallowException(Exception ex) to avoid having the warning.
3. In GetHttpContextBase, the two if can be combined in one.

In regards to #2, this might seem as an overkill for a totally harmless warning. I think it is important because if we leave these harmless warnings to appear, they can hide a warning that we would like to see.

public class CustomExceptionLogger : ExceptionLogger
{
    private const string HttpContextBaseKey = "MS_HttpContext";

    public override void Log(ExceptionLoggerContext context)
    {
        HttpContext httpContext = GetHttpContext(context.Request); // Retrieve the current HttpContext instance for this request.
        NameValueCollection serverVariables = null;

        LogException logException = new LogException();

        try
        {
            if (httpContext != null && httpContext.Request != null && httpContext.Request.ServerVariables != null)
                serverVariables = httpContext.Request.ServerVariables;

            logException.Log(context.Exception, serverVariables);
        }
        catch (Exception e)
        {
            SwallowException(e);
        }
    }

    private static HttpContext GetHttpContext(HttpRequestMessage request)
    {
        HttpContextBase contextBase = GetHttpContextBase(request);

        if (contextBase == null)
        {
            return null;
        }

        return ToHttpContext(contextBase);
    }

    private static HttpContextBase GetHttpContextBase(HttpRequestMessage request)
    {
        object value;

        if (request == null || !request.Properties.TryGetValue(HttpContextBaseKey, out value))
        {
            return null;
        }

        return value as HttpContextBase;
    }

    private static HttpContext ToHttpContext(HttpContextBase contextBase)
    {
        return contextBase.ApplicationInstance.Context;
    }

    private static void SwallowException(Exception ex)
    {
        ;
    }
}

LogException

1. The processing of the server variables should be in its own function. That can improve the readability of the Log(..) function.
2. In the AppendServerVariables (the new function), having foreach statements rather than for loops can reduce code and slightly improve speed; for example allKeys[loop1] is making a lookup every time it is used, while in the foreach you get a reference to it by default.
3. Also, the comparison that uses ToUpper() can be replaced with the use of Equals and StringComparison.OrdinalIgnoreCase which is faster.

Also, one would suggest that when calling .AppendLine(...) you could use string.Format(...) but for such small strings, concatenation is faster.

Regarding the check for HttpContext.Current if is null I assumed that exists to ensure that the serverVariables has a value. That can be replaced by checking the serverVariables variable directly.

public class LogException
{
    public bool Log(Exception exception, NameValueCollection serverVariables = null)
    {
        var logService = new LogService();

        if (serverVariables != null)
        {
            AppendServerVariables(exception, serverVariables);
        }

        return logService.LogException(exception, exception.TargetSite.Name);
    }

    private void AppendServerVariables(Exception exception, NameValueCollection serverVariables)
    {
        StringBuilder serverVariableKeys = new StringBuilder();

        foreach (string key in serverVariables.AllKeys)
        {
            if (!key.Equals("ALL_HTTP", StringComparison.OrdinalIgnoreCase) && !key.Equals("ALL_RAW", StringComparison.OrdinalIgnoreCase))
            {
                serverVariableKeys.Append(key + " : ");

                int index = 0;
                string[] values = serverVariables.GetValues(key);
                foreach (string value in values)
                {
                    serverVariableKeys.AppendLine("Value " + index.ToString() + ": " + value);
                    index += 1;
                }
            }
        }

        exception.Data.Add("ServerVariables", serverVariableKeys.ToString());
    }
}