Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HealthCheck UI - Configurable Storage Providers (InMemory, SqlServer, Postgres, Sqlite) #455

Closed
CarlosLanderas opened this issue Mar 16, 2020 · 40 comments · Fixed by #477
Closed
Assignees
Labels
breaking-change enhancement New feature or request

Comments

@CarlosLanderas
Copy link
Contributor

CarlosLanderas commented Mar 16, 2020

What would you like to be added:

Health Check UI Storage Providers (SqlServer, Postgres, Sqlite)

We need to start working in configurable Health Checks Database providers.

Why is this needed:

Right now, we only support SQLite and this is very limiting. For example Azure App Service running under linux is not compatible

We are going to allow the user configuring it's own storage provider using Entity Framework Core.

The idea is making the UI DbContext public, configuring the default storage as "In Memory", (if we have no compatibility issues) and allow the user to override this default provider when ConfigureDbContext is configured by the user

@CarlosLanderas CarlosLanderas added the enhancement New feature or request label Mar 16, 2020
@CarlosLanderas CarlosLanderas self-assigned this Mar 16, 2020
@CarlosLanderas
Copy link
Contributor Author

CarlosLanderas commented Mar 16, 2020

Issues related to SQLite or requesting further storage providers:

#444
#338
#303
#226
#181
#161
#29
#22

@CarlosLanderas CarlosLanderas changed the title HealthCheck UI - Configurable Storage Providers HealthCheck UI - Configurable Storage Providers (InMemory, SqlServer, Postgres, Sqlite) Mar 16, 2020
@CarlosLanderas CarlosLanderas added the question Further information is requested label Mar 17, 2020
@CarlosLanderas
Copy link
Contributor Author

CarlosLanderas commented Mar 17, 2020

How do you think we should implement this feature?. Do you think we should add migrations for different providers (SqlServer, Postgres, Sqlite) inside the HealthChecks.UI assembly and expose methods such as:

 services
 .AddHealthChecksUI()
// This could use a default in memory storage
 services
 .AddHealthChecksUI()
 .AddUISqlServerStorage(connectionString)
 services
 .AddHealthChecksUI()
 .AddUIPostgresStorage(connectionString)

of just make our DbContext public and let the user create their own migrations for whatever provider?

services.AddDbContext<UIDbContext>(setup => {
   setup.UseSqlServer(connectionString, opt => opt.MigrationsAssembly("AssemblyName");
});

@devbrsa
Copy link
Contributor

devbrsa commented Mar 18, 2020

@CarlosLanderas I would say the first approach, better intention, easier to mock and extendable.

@devbrsa
Copy link
Contributor

devbrsa commented Mar 18, 2020

@CarlosLanderas how can I help you with such a feature?

@amccool
Copy link

amccool commented Mar 18, 2020

services
 .AddHealthChecksUI()
 .AddUISqlServerStorage(connectionString)

This style looks best. I personally like the extension method style on the AddHealthChecksUI(). However, using the connection string directly here will limit the mocking of the dbcontext/etc for testing (not sure it matters as much for the production side).

IMHO, I think you will want a combination of both. e.g. add the storage dependency, then add that dependency to the HealthCheck.UI dependency.

@CarlosLanderas
Copy link
Contributor Author

CarlosLanderas commented Mar 18, 2020

IMHO, the main difference between boths approaches is:

With the first one, we would be the ones creating the migrations inside the UI assembly and wiring everything internally (MigrationsAssembly, Database migrate execution, etc) and the user would be limited to the available provider migrations.

The second options requires more work for the user, but everything can be customized outside the UI (With the risk of doing it wrong as well).

If we stick with the second option and allow the user to create the Migrations and configure the Context provider and options we would still need to trigger the initial seed that feeds the storage with the configured Healthcheck and webhook endpoints in the UI internals.

@Phillip9587
Copy link

What's the status of this change?

@CarlosLanderas
Copy link
Contributor Author

Waiting for feedback and opinions

@Phillip9587
Copy link

I like the first option. The fluent style keeps your package simple. If you implement the standard data providers everyone will be happy i think.

The second option would make your package more complex to configure and it would not be so straightforward as it is right now.

@Skleni
Copy link

Skleni commented Apr 1, 2020

I also prefer the first option. Don't get me wrong, but getting migrations right is hard enough as it is...I don't want to have to think about other people's migrations as well (if that's possible).

@Odonno
Copy link
Contributor

Odonno commented Apr 1, 2020

Same. I prefer the first option to only allow the minimum options required, black box mode.

The only thing is the name of the extension method, I would prefer something like this to make it clear about the intended function:

services
  .AddHealthChecksUI(setup => 
  {
    setup.AddSqlServerStorage(connectionString);
  });  

@sungam3r
Copy link
Collaborator

sungam3r commented Apr 1, 2020

First option.

@udlose
Copy link

udlose commented Apr 1, 2020

First option - keeping things "black box" is extremely important.

@CarlosLanderas
Copy link
Contributor Author

Same. I prefer the first option to only allow the minimum options required, black box mode.

The only thing is the name of the extension method, I would prefer something like this to make it clear about the intended function:

services
  .AddHealthChecksUI(setup => 
  {
    setup.AddSqlServerStorage(connectionString);
  });  

Yes, of course this is another valid options. Thanks for the propoisal.
We are going to migrate all packages to 3.1 and start working on this feature

@devbrsa
Copy link
Contributor

devbrsa commented Apr 2, 2020

@CarlosLanderas are you accepting PRs only for bugfixes or improvements as well? Do you have a guideline over how to perform PRs?

@CarlosLanderas CarlosLanderas added in-progress and removed question Further information is requested labels Apr 10, 2020
@CarlosLanderas
Copy link
Contributor Author

CarlosLanderas commented Apr 10, 2020

Work has started. We have added a HealthChecksUIBuilder se we can properly chain all the dependencies.

You can follow work in this branch:

https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/tree/storage-providers

@kevbite
Copy link

kevbite commented Apr 10, 2020

@CarlosLanderas looking at the commits so far it seems that it's still very much dependant on Entity Framework Core, is there any plan on abstracting this away so other database providers that don't have an EF provider can be implemented?

@CarlosLanderas
Copy link
Contributor Author

@kevbite to be honest I don't think so (at least at the moment) as several parts in the UI use EF Core. (collector, middleware, seeding, etc). What DB provider are you thinking about?

@kevbite
Copy link

kevbite commented Apr 11, 2020

@CarlosLanderas I was thinking about building support for MongoDB, I know few products out there that only have MongoDB as backing store.
The alternative is to build a EF Core provider but that seems a bit of a roundabout way to achieve it.

@Odonno
Copy link
Contributor

Odonno commented Apr 11, 2020

Well, you can maybe propose different kind of storage models and create an abstraction layer inside the library so that whatever Database you choose, it should implement the required methods.

I believe that EF Core will be suitable for most scenarii but you can also offer a way to let developer create their own extension method which will return the right storage layer (class) for their use case.

As an example, see:

services
  .AddHealthChecksUI(setup => 
  {
    // Provided by a HealthChecks UI package
    // Will use an EF Core implementation (or whatever)
    setup.AddSqlServerStorage(connectionString);

    // Provided by anyone
    // Will use the according implementation to store data in a mongodb instance
    setup.AddMongoDbStorage(connectionString);
  });

@cesarbmx
Copy link

Getting Unable to find package AspNetCore.HealthChecks.UI.Core while installing the nuget

@CarlosLanderas
Copy link
Contributor Author

CarlosLanderas commented Apr 14, 2020

@cesarbmx, this has been solved. Could you try again?

@cesarbmx
Copy link

@CarlosLanderas - All working.
This is a great project. Delighted by this contribution. Thanks!

@CarlosLanderas
Copy link
Contributor Author

CarlosLanderas commented Apr 14, 2020

@cesarbmx great! Thank you for your feedback!

Check the updated docs to know to to disable migrations, limit history entries in the frontend, use collector interceptors and other goodies that have been added :)

@udlose
Copy link

udlose commented Apr 14, 2020

@CarlosLanderas any support for MySql planned?

@CarlosLanderas
Copy link
Contributor Author

@udlose, yes, at soon at is supported in 3.0. They support 2.2 right now if I am not wrong

@udlose
Copy link

udlose commented Apr 14, 2020

@udlose, yes, at soon at is supported in 3.0. They support 2.2 right now if I am not wrong

Can you please clarify what you are referring to? I am currently using MySql 5.7.12 in my .NET Core 3.1.3 application. I don't understand who the it and they you are referring to

@CarlosLanderas
Copy link
Contributor Author

CarlosLanderas commented Apr 14, 2020

@udlose , let me check it, and if possible I submit the provider tomorrow ;)

@CarlosLanderas
Copy link
Contributor Author

CarlosLanderas commented Apr 15, 2020

@udlose, migrations can not be created, they need to upgrade the provider as you can read in the link below. I forgot I faced this problem when tried to create the provider the first time:

Method 'get_Info' in type 'MySql.Data.EntityFrameworkCore.Infraestructure.MySQLOptionsExtension' from assembly 'MySql.Data.EntityFrameworkCore, Version=8.0.19.0, Culture=neutral,

PublicKeyToken=c5687fc88969c44d' does not have an implementation.

dotnet/efcore#17788 (comment)

@CarlosLanderas
Copy link
Contributor Author

CarlosLanderas commented Apr 15, 2020

@udlose in the issue, the suggest using

https://www.nuget.org/packages/Pomelo.EntityFrameworkCore.MySql

As entity framework team is no longer maintaining the MySql provider.

Pomelo is a fully featured provider. I am gonna give it a try

@CarlosLanderas
Copy link
Contributor Author

@udlose, here is the published MySql storage provider package:

https://www.nuget.org/packages/AspNetCore.HealthChecks.UI.MySql.Storage

@udlose
Copy link

udlose commented Apr 15, 2020

@udlose, here is the published MySql storage provider package:

https://www.nuget.org/packages/AspNetCore.HealthChecks.UI.MySql.Storage

@CarlosLanderas Thank you so much! I greatly appreciate your promptness :)

I'm not using EntityFramework, I assume that was an implementation/support blocker on your side?

I will pull the package and give it a try later this week or next (have another pressing item I need to complete first). What all do I need to do on my side to get the provider and database setup? We are running MySql in AWS. TIA

@CarlosLanderas
Copy link
Contributor Author

@udlose, it automatically runs migrations against the connection string database creating all the requires tables.

@udlose
Copy link

udlose commented Apr 20, 2020

@CarlosLanderas

  1. Can you describe how the Database Migration works? I have my connection string for my app pointing to my app database. Is the assumption that the tables for HealthChecks will be created in the same database as my application? I don't want to run a migration against my current application database as I am afraid the migration may cause issues to it.

  2. I host the MySql database in AWS and the account in the connection string has limited permissions. It certainly doesn't have permissions to create tables. How can I get the tables created? Is the assumption that my database connection string has permissions to create schema objects? If so, that isn't optimal as it would create a huge security issue.

@OskarKlintrot
Copy link

@CarlosLanderas Is there any way to run the migrations manually? Or better yet, generate them as you normaly do in EF? Applying migrations on startup seems a bit dangerous if you are using more than one instance...

@protradeshare-admin
Copy link

For Postgres DB, is it possible to configure any other schema then public?

@udlose
Copy link

udlose commented Jun 30, 2020

@CarlosLanderas can you please respond to my comment from April 20th?
#455 (comment)

@Odonno
Copy link
Contributor

Odonno commented Apr 13, 2021

Same question as @protradeshare-admin, how do we change the schema for the storage options (other than dbo for SQL Server so)?

@jeffersantoss
Copy link

Same question as @protradeshare-admin and @Odonno, I have more then one context and I intend to use the same database to store the health checks data, how can I change the schema for the storage? Because I already have a table named __EFMigrationsHistory in the public schema.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.