How do I improve this logging mechanism?

Question

There are quite a few things I am considering:

1. I will have to check for null values
2. I will have serious trouble persisting it in the database.

How do I improve this or re-factor this for better results?

public class ErrorLog
    {
        public static void LogError(Exception e)
        {
            var exceptionLog = new Dictionary<String, String>
                                   {
                                       {"Inner Exception", e.InnerException.Message},
                                       {"Message", e.Message},
                                       {"Source", e.Source},
                                       {"StackTrace", e.StackTrace},
                                       {"MethodName",e.TargetSite.Name}
                                   };
            foreach (KeyValuePair<String, String> kvp in e.Data)
                exceptionLog.Add(kvp.Key, kvp.Value);
        }
    }

Update with the new and simpler class

  public class ErrorLog
    {
        public static void LogError(Exception e)
        {
            var innerExcpetionMessage = e.InnerException == null ? "Null" : e.InnerException.Message;
            using (var connection = new SqlConnection(ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString))
            {                
                    var cmd = new SqlCommand("ApplicationErrorLog", connection) { CommandType = System.Data.CommandType.StoredProcedure };
                    cmd.Parameters.AddWithValue("@Type", e.GetType().Name);
                    cmd.Parameters.AddWithValue("@InnerException", innerExcpetionMessage);
                    cmd.Parameters.AddWithValue("@Message", e.Message);
                    cmd.Parameters.AddWithValue("@Source", e.Source);
                    cmd.Parameters.AddWithValue("@StackTrace", e.StackTrace);
                    cmd.Parameters.AddWithValue("@MethodName", e.TargetSite.Name);
                    connection.Open();
                    cmd.ExecuteNonQuery();
                    connection.Close();
                }

            }
        }
    }

Now what if there is an error in updating this log, how will I log that?

Answer 1

Why would you have serious trouble with persistance?

Just do null checks for InnerException and TargetSite, rest will be inserted as null or string empty depending on your db setup.

And why not just simplify it as:

var innerExceptionMessage = e.InnerException == null ? string.Empty : e.InnerException.Message;
var targetSiteMessage     = e.TargetSite == null ? string.Empty : e.TargetSite.Message;

exceptionLog.Add("Inner Exception", innerExceptionMessage);
exceptionLog.Add("Message", e.Message);
exceptionLog.Add("Source", e.Source);
exceptionLog.Add("StackTrace", e.StackTrace);
exceptionLog.Add("MethodName",targetSiteMessage);

Keep it simple :)

Answer 2

I would pose several suggestions:

1. First, you should add exception handling code to your logging. You may not want your logging code raising unhandled exceptions (which, depending where it happens in your code, could try to recursively log again). At the very least, it would give you the opportunity to write something out about why it failed and perhaps include the original exception to some backup log destination like system event logs, console output, or a file.
2. Second, I would implement your database logging as a listener class for a separate logging class. As this is C#, you may take a look at the TraceSource class. Your database logger would be written to extend TraceListener and then attached to the Listeners collection on the trace object. This gives you the freedom to attach multiple types of listeners that can log to various different outputs, each potentially having different filtering. This can even be done via app.config file.
3. You may also want to use DBNull instead of "Null" or string.Empty for values which are unavailable, assuming your stored procedure and logging schema support it.

Answer 3

You're doing ugly things here. The "new and simpler class" tightly couples you to SQL and persistence concerns. A dictionary of String String is an implementation concern, and more likely the basis for a Class.

A better approach would be to go with your original approach, then define a separate method to do the database write that takes in the dictionary/class as a parameter.

Answer 4

Just a quick comment on @Mattias's answer. I think you would want to capture the original exception message if the InnerException is null. If so, you could write:

var innerExceptionMessage = e.InnerException == null ? string.Empty : e.InnerException.Message;

as

var innerExceptionMessage = (e.InnerException ?? e).Message ?? string.Empty;