warden-stack / Warden

Define "health checks" for your applications, resources and infrastructure. Keep your Warden on the watch.
https://getwarden.net
MIT License
616 stars 66 forks source link

Contribute a new Warden.Watchers.SqlCommon project? #115

Open WallaceKelly opened 8 years ago

WallaceKelly commented 8 years ago

I'd like to use Warden with an Oracle database.. The MsSqlWatcher only works with MS SQL.

I think it would be helpful to have a new SqlWatcher that is not MS SQL specific, but works with any ADO.NET provider.

I'm thinking of this approach...

  1. Copy over the Warden.Watchers.MsSql code into a new Warden.Watchers.SqlCommon project.
  2. Factor out the System.Data.SqlClient dependencies to use System.Data.Common instead.
  3. Refactor Warden.Watchers.MsSql to use the new Warden.Watchers.SqlCommon for most of its implementation (thus removing the newly duplicate code, but maintaining the same interfaces for the MS SQL-specific implementation.)

I'd like to get consensus before getting started. Thoughts?

spetz commented 8 years ago

Hi Wallace, I'd rather create a separate watcher for the Oracle (e.g. do not create a Watcher.Common project in order to avoid confusion) or refactor the already existing MsSql watcher into something like SqlWatcher where you could choose between the MSSQL, Oracle or maybe even the MySQL database if possible. However I think it might be difficult to achieve such a goal within a single Watcher project, therefore I wouldn't worry too much about creating a new Watcher even if it would use a similar codebase - the main advantage is that it could be developed separately.

WallaceKelly commented 8 years ago

choose between the MSSQL, Oracle or maybe even the MySQL database if possible. However I think it might be difficult to achieve such a goal

The ADO.NET framework already has this ability in the System.Data.Common.dll and its DbProviderFactory classes. Dapper takes advantage of this, for example. It works on IDbConnection, instead of SqlConnection (or MySqlConnection or OracleConnection, etc.) That way, Dapper can be used with any database that has an ADO.NET provider.

I think it would be quite easy to implement. If you would prefer, I'll submit a pull request that works with any ADO.NET provider (including Oracle providers). I will not touch MsSqlWatcher. You can look it over and decide later if it makes sense to refactor MsSqlWatcher or just leave it as-is.

How does that sound?

spetz commented 8 years ago

Sounds great, feel free to submit a PR and I'll be more than happy to review it :). You could name it something like Watcher.Sql (my first idea).

tiesont commented 8 years ago

@WallaceKelly Looks like the test(s) for MsSqlWatcher are pretty generic too, so that shouldn't be hard to adapt. Do you have an idea of how you're going to tell the watcher which vendor is in use?

I'd be happy to contribute, if you'd like any help.

WallaceKelly commented 8 years ago

Do you have an idea of how you're going to tell the watcher which vendor is in use?

Fortunately, we don't have to invent this. ADO.NET includes this functionality already. I've used it on many projects. I just searched the web for an example and found this... http://www.codeproject.com/Articles/55890/Don-t-hard-code-your-DataProviders

However, my search also revealed that there is has been some discussion about how best to support this functionality in .NET Core. See https://github.com/dotnet/corefx/issues/4571. My takeaway from that thread is that .NET Core is trying to match .NET Full's implementation as closely as possible. However, I think I'll have to investigate the status of that before proceeding.

WallaceKelly commented 8 years ago

I'd be happy to contribute, if you'd like any help.

@tiesont I have started at https://github.com/WallaceKelly/Warden/tree/sqlwatcher/src/Watchers/Warden.Watchers.Sql

I have not tested my changes yet, or looked at the unit tests. But, that is where I'll be working.

If you compare the MsSqlWatcher and SqlWatcher folders, you will see that they are nearly identical. The meat of the change was using .NET's DbProviderFactory instead of the following:

public Func<string, IDbConnection> ConnectionProvider { get; protected set; }

That change allows us to remove the dependency on System.Data.SqlClient.dll and the MS SQL specific classes. The new SqlWatcher API expects a DbProviderFactory to be passed in at initialization.

As an example, the MS SQL factory is available on .NET's static SqlClientFactory.Instance property. Consequently, the configuration might look like this...

var configuration = SqlWatcherConfiguration
    .Create(SqlClientFactory.Instance, "My Database",
        @"Data Source=.\sqlexpress;Initial Catalog=MyDatabase;Integrated Security=True")
    .WithQuery("select * from users where id = @id", new Dictionary<string, object> {["id"] = 1 })
    .EnsureThat(users => users.Any(user => user.Name == "admin"))
    .Build();

Since I'm interested in Oracle, I'm planning to use this data provider instead of the MS SQL one.

Does this all look like a good approach?

tiesont commented 8 years ago

@WallaceKelly Excellent. I'll check it out tonight. Also, in case I gave the wrong impression, I'm not part of this project, but I'm currently building something using this library and this issue caught my attention.

spetz commented 8 years ago

Looks good to me, looking forward to test it.

tiesont commented 8 years ago

@WallaceKelly How do you want to track what's being worked on? Using the issues tab seems easiest, but you don't have issues enabled on your fork. It might also be worth exploring the new Projects feature: https://github.com/universe-2016#projects

WallaceKelly commented 8 years ago

@tiesont Added issues to the fork and made you a collaborator.