4
\$\begingroup\$

I've been tasked with updating our current error logging system to prevent the logging of similar errors (from a single client program) that were probably caused by the same event. The code here is what I have added in order to accomplish this. Are there any obvious issues I am missing or is there a simpler way to accomplish this? I need to deal with multiple threads.

Here is the simple error object we use:

public class ErrorObject
{
    //to use when sending an object to a background worker
    public ErrorObject(Exception exception, string additionalinfo,DateTime? dateTime = null,bool showTouser = false)
    {
        Exception = exception;
        AdditionalInfo = additionalinfo;
        Time = dateTime ?? DateTime.Now;
    }

    public Exception Exception { get; set; }
    public string AdditionalInfo { get; set; }
    public DateTime Time { get; set; }
    public bool ShownToUser { get; set; }
}

Other code calls this function to log errors.

public static Int32 LogError(Exception ex, bool runInBackground = true, string additionalInfo = "",
    bool ShownToUser = false)
{

    int retVal = -1;
    if (runInBackground)
    {
        ErrorObject newError = new ErrorObject(ex,additionalInfo,DateTime.Now,ShownToUser);

        ThreadPool.QueueUserWorkItem(LogErrorInBackground, newError);
    }
    else
    {
        try
        {   //Do a Sql Insert in current thread (slow for remote labs...)
            retVal = InsertError(ex, additionalInfo, ShownToUser);
        }
        catch{}
    }
    return retVal;
}


//Store 4 previous errors to compare against
private static Queue<ErrorObject> PreviousErrors = new Queue<ErrorObject>(4);
private static object PreviousErrorLock = new object();

private static void LogErrorInBackground(object state)
{
    ErrorObject error = (ErrorObject) state;

    if (! QueueNewError(error))
    {
        InsertError(error.Exception, error.AdditionalInfo, error.ShownToUser);
    }
}


/// <summary>Adds an error to the PreviousErrors Queue and removes the error at the front of the list.</summary>
/// <returns>Returns true if the error is a duplicate of any other previous errors in the queue.</returns>
private static bool QueueNewError(ErrorObject newError)
{
    //Assume the new error is not a duplicate
    bool isDuplicate = false;

    lock (PreviousErrorLock)
    {

        foreach (ErrorObject previousError in PreviousErrors)
        {
            if (previousError.AdditionalInfo == newError.AdditionalInfo &&
            Math.Abs((previousError.Time - newError.Time).TotalSeconds) < 3 &&
            previousError.Exception.GetType() == newError.Exception.GetType() &&
            previousError.Exception.Message == newError.Exception.Message &&
            previousError.Exception.TargetSite == newError.Exception.TargetSite)
            {
                isDuplicate = true;
                break;//Exit the loop
            }
        }

        if( PreviousErrors.Count > 0)
            PreviousErrors.Dequeue();
        PreviousErrors.Enqueue(new ErrorObject(newError.Exception, newError.AdditionalInfo, newError.Time));
    }
    return isDuplicate;
}
\$\endgroup\$
  • \$\begingroup\$ After more testing I found that i'm not recording 4 most recent errors I need to adjust the Dequeue() operation to be if( PreviousErrors.Count > 3) PreviousErrors.Dequeue(); \$\endgroup\$ – GER Jul 17 '14 at 16:15
2
\$\begingroup\$
ErrorObject newError = new ErrorObject(…);

This is one of the cases where most people will probably agree that you can use var here. Doing that doesn't lose any information (you can still see at a glance that newError is an ErrorObject) and it makes the code shorter and more DRY.


return retVal;

In general, you shouldn't use integer retVals. If you need to report errors, use exceptions. If you want to report success or failure, use a bool. If you want to report multiple possible results, use an enum. If you want to report some kind of count (though I don't see how would that make sense here), at least name your variable something like count, not retVal.


//Store 4 previous errors to compare against
private static Queue<ErrorObject> PreviousErrors = new Queue<ErrorObject>(4);

This sounds like a job for a simple reusable custom collection. Something like:

class BoundedQueue<T> : IEnumerable<T>
{
    private readonly int count;
    private readonly Queue<T> queue;

    public BoundedQueue(int count)
    {
        this.count = count;
        this.queue = new Queue<T>(count);
    }

    public void Enqueue(T item)
    {
        if (queue.Count == count)
            queue.Dequeue();

        queue.Enqueue(item);
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public IEnumerator<T> GetEnumerator()
    {
        return queue.GetEnumerator();
    }
}

lock (PreviousErrorLock)

Since you want to lock access to the PreviousErrors object, you should use that in the lock, you don't need another object for this. (An object used just for locking makes sense when you logically want to do something like lock (this), but that's not the case here.)


foreach (ErrorObject previousError in PreviousErrors)
{
    if (previousError.AdditionalInfo == newError.AdditionalInfo &&
    Math.Abs((previousError.Time - newError.Time).TotalSeconds) < 3 &&
    previousError.Exception.GetType() == newError.Exception.GetType() &&
    previousError.Exception.Message == newError.Exception.Message &&
    previousError.Exception.TargetSite == newError.Exception.TargetSite)
    {
        isDuplicate = true;
        break;//Exit the loop
    }
}

You should extract this equality testing code into a separate method, possibly as ErrorObject.Equals(). With that, this code would simplify to:

bool isDuplicate = PreviousErrors.Contains(newError);

break;//Exit the loop

You don't need this comment. Everyone knows what break does inside a loop.


Also, I find the queue logic suspicious: when you detect a duplicate, you add it to the queue anyway. So if the sequence of exceptions is something like 1, 2, 2, 2, 2, 1, then you're going to report 1 twice, even though it's among the 2 most recent error kinds.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ In reference to the Queue logic. The thought was that if the same 'type' of error is separated by several others, then it could potentially be from a different Exception event and we would want to capture it. \$\endgroup\$ – GER Jul 17 '14 at 18:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.