NickCraver / StackExchange.Exceptional

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

Added option to ignore hosts in Web.config #47

Closed zeroef closed 9 years ago

zeroef commented 10 years ago

We noticed that you couldn't ignore errors by host (ex: localhost). We're using this code at work and thought it would benefit the community to have it added to the project.

NickCraver commented 10 years ago

First, thanks - I appreciate the request!

My concern here is this really leaves the realm of filters based on the exception and into ignores based on something in the environment (which leads to n lists).

I already expose a method of filtering in the current code you could use, by attaching to the ErrorStore.OnBeforeLog (code here and example here) - like this:

ErrorStore.OnBeforeLog += (sender, args) =>
{
  if (args.Error.Host == "localhost") args.Abort = true;
};

This is generic handling applied to aborting logging of the error, so I'm not sure a generic Func<Error, bool> (that defaults to return true) for a "should this be logged?" static check is warranted...but if it is I'd much rather go that route and leave it much more extensible overall.

zeroef commented 10 years ago

Hey Nick! Thanks for the quick response.

I agree that it sets us down the path of filtering on the environment instead of the exception, but disabling localhost errors is a feature that I could envision many developers using. We use a combination of Teamcity and Octopus Deploy for deployments and perform web.config modifications in a post deploy script. Having the ability to filter the host in the config file instead of the code allows us pass in variables specific to each environment (dev, qa, staging) from OD.

I know this example is specific to the way that my group does things, but I think explaining might shed light as to why we've done it that way, instead of attaching to OnBeforeLog

NickCraver commented 10 years ago

I don't want to seem obtuse here, but since the logging is local to the app domain (unless this app is a logging proxy?), aren't you just effectively disabling logging in that deployment? or locally? (whichever way the toggle is)

We also do web.config transforms here at Stack Exchange (via PowerShell in TeamCity). The way we do local vs. production is to just use a Memory store locally for easy testing and no dependencies - replaced with a SQL store and proper connection string for production deploys...would that approach work in your case?

If not, couldn't the same thing be achieved with OnBeforeLog, and app setting, and a string.Split()? This would still let you completely control all the filters via TeamCity replacements.

It's also worth noting "localhost" specifically wouldn't work for a very large portion of users to exclude local dev...it wouldn't work in our case for example, so I wouldn't want to add that specifically as an option - it makes too many (often false) assumptions.