NickCraver / StackExchange.Exceptional

Error handler used for the Stack Exchange network
https://nickcraver.com/StackExchange.Exceptional/
Apache License 2.0
862 stars 170 forks source link

Prevent Emailing Duplicate Errors #62

Closed CptRobby closed 8 years ago

CptRobby commented 9 years ago

@NickCraver This fixes #61 without the downsides that you enumerated. I found out that my code didn't really have to change much to match the changes that you had made. I tried to keep the changes as clean as possible. Let me know if there's anything that you would like me to change. :wink:

Added a new PreventDuplicates configuration option to the ErrorEmailer with the related code changes necessary to prevent the ErrorEmailer from mailing notifications when the error was detected to be a duplicate by the ErrorStore as configured by the RollupThreshold value. One of those changes was the addition of a simple bool flag on Error called IsOriginalError, which should only be true when the error was not determined to be a duplicate of a previously stored value, or if it was queued and the queue didn't already contain a matching error.

Also contains a few minor bug fixes.

cocowalla commented 9 years ago

If I'm not mistaken, this uses a hash based on the whole stack trace to check for duplicates. The stack trace is likely to contain text that we expect to change even for the 'same' error, such as timestamps.

Please correct me if I'm wrong :)

CptRobby commented 9 years ago

@cocowalla Stack traces and calls to Exception.ToString() don't usually contain timestamps. It would though if you did something like this:

throw new Exception(string.format("Something went wrong at {0}", DateTime.Now));

Aside from just being a really bad exception message, including a timestamp in the message would in fact produce different hash values. But if you let Exceptional record the time for you, then you shouldn't have a problem. :wink:

CptRobby commented 8 years ago

@NickCraver Haven't heard from you and I was just wondering if this was something that you were interested in. :smiley:

NickCraver commented 8 years ago

Sorry, busy is a tremendous understatement for the past few months, but I'm trying hard to give al of our open source projects love this week. I merged in and changed your approach slightly here to detect the GUID difference rather than adding a new property that providers need to worry about. This should a) be a bit simpler and b) prevent breaks or unexpected downstream behavior in existing providers. Commit is here: https://github.com/NickCraver/StackExchange.Exceptional/commit/441681327cc314c53ae863b4095533df68f71be3

Thanks for the work!

CptRobby commented 8 years ago

@NickCraver It's understandable. Life is pretty busy for me too lately with trying to sell my house and such. :laughing:

I really like your modification though! I hadn't really paid much attention to the GUID property and didn't think of checking it for modification by the store/queue like that. I like how it isolated the setting of IsDuplicate to the ErrorStore.Log function instead of it being placed in each store's LogError function. Much cleaner and better separation of concerns. Thanks! :wink:

Please let me know if there are any other changes that I can help with. #31 perhaps? :grinning: