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

Support for MySQL #49

Closed mattwoberts closed 10 years ago

mattwoberts commented 10 years ago

Added a MySqlErrorStore for logging to a mysql database.

Most of the code is in the new MySqlErrorStore.cs file which fires SQL against a mysql db via the MySqlConnection.

I also added the relevant sql scripts to generate the exceptions table (for both sql server and mysql) to a DbScripts folder, but can happily remove these if you prefer.

NickCraver commented 10 years ago

First, thanks for the contrib!

The intent has always been for new ErrorStore implementations that carry a dependency to be a sibling project rather than added directly in. Also, they'd be a separate nuget package, StackExchange.Exceptional.MySQL in this case. This keeps Exceptional core very lightweight and dependency free for the vast majority of users.

That being said, this wasn't clear and the switch to load types made it downright impossible. I've just improved on that in this commit so that it'll just load the StackExchange.Exceptional.MySQL.dll and grab the MySQLErrorStore out of it automatically. The user steps will just be to grab that nuget package and use the MySQL type in the web.config.

Do you mind switching this pull to be a sibling project that references StackExchange.Exceptional? Once that happens, I can pull it in and create the second nuget package for others to use. If there's another extensibility point I'm missing, please let me know and we'll fix that up. I want to (reasonably) support all the error stores people want.

mattwoberts commented 10 years ago

Hi Nick. No problem, I'm do this now. For simplicity I'm adding a new project inside the same git repo, if that's not what you want then let me know.

NickCraver commented 10 years ago

Hey @mattwoberts, definitely want it in the same repo, thanks! I'll make some movements on the shared classes there so that there need not be n copies. The intent in internals there was to not expose and conflict with people including the StackExchange.Exceptional namespace, not to hide them from other store providers.

I'll pull this in now, shuffle things around and get nuget setup for it today or tomorrow (busy day!). If I do a pre-release nuget package based on the merge code, would you be able to test it against a MySQL instance?

NickCraver commented 10 years ago

I have merged in and removed the duplicated classes, but I'm curious bout the LogError() function - is there a reason you split it into 2 queries for every logged error? This introduces race conditions and inaccurate logs so I want to remove this, but I'm not sure what MySQL constraints you ran into doing it in the same atomic way as SQL server.

Can you help me out in understanding this a bit better please?

mattwoberts commented 10 years ago

@NickCraver Hmm, it was bourne out of nesessity - getting the out parameter from the update statement was playing up, and also mySQL doesn't support a limit 1 (the equivilent of a top 1) in a sub-select, so it wasn't possible to port that clever code.

I did have it noted down though as something to look at - and you're right, it does introduce a race condition. Let me have another look at it later today and see if I can change it.

Also - thanks for shuffling things so that the internals I need (dapper and extensions) are available to the mySQL proj

mattwoberts commented 10 years ago

Something I've noticed while messing about - your GetErrorStores code that does this to get the assembly location: var path = typeof (ErrorStore).Assembly.Location;

When run from the samples app, comes back as something like C:\Windows\Microsoft.NET\Framework\v4.0.30319\Temporary ASP.NET Files\root\4982c560\4f2dcb5a\assembly\dl3\1ce3b73d\fb85c67b_4cb1cf01. Only the StackExchange.Exceptional.dll is in there, despite all the references that exist from the web app. It was my understanding that ALL dll's from the bin folder should be shadow copied into here, but this isn't happening, so it's not finding the MySQL Error Store in the samples app

mattwoberts commented 10 years ago

Re: Previous comment, I think I've found a solution. Will include it with the PR