aspnet / Diagnostics

[Archived] Diagnostics middleware for reporting info and handling exceptions and errors in ASP.NET Core, and diagnosing Entity Framework Core migrations errors. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
213 stars 108 forks source link

Nested paths screw up health check filters #511

Closed jmezach closed 6 years ago

jmezach commented 6 years ago

I'm not sure if this is even supposed to work, but it's just something I noticed while I was exploring the health checks feature. In my ConfigureServices I have the following:

services.AddHealthChecks()
        .AddCheck("MyDatabase",
                  new SqlConnectionHealthCheck("Data Source=localhost;Initial Catalog=my-dummy-database"),
                  tags: new[] { "ready" });

Obviously the SqlConnectionHealthCheck fails because it cannot connect to the database (in fact, there isn't even a SQL Server running on my machine). Now I have added the following to my Configure method:

app.UseHealthChecks("/health", new HealthCheckOptions {});

app.UseHealthChecks("/health/ready", new HealthCheckOptions
{
  Predicate = (check) => check.Tags.Contains("ready"), 
});

app.UseHealthChecks("/health/live", new HealthCheckOptions()
{
  Predicate = (check) => false,
});

In this setup I would expect /health to return Unhealthy, since it runs the SqlConnectionHealthCheck and it fails. This is indeed the case. The same goes for the /health/ready endpoint. However, I was expecting /health/live to return Healthy, but it does not. Instead it returns Unhealthy and looking at the log it seems to be executing the SqlConnectionHealthCheck regardless of the predicate.

Personally I would be fine with it if this is something that's not supported, but it might be useful to put this into the documentation somewhere so that users of this feature don't get tripped up.

rynowak commented 6 years ago

Thanks for the feedback. This is using the .Map middleware prefix which matches any suffix of the path you put in.

However seeing your code, I agree that this is really surprising. We'll look at changing this.

mkArtakMSFT commented 6 years ago

@rynowak, can you please assign a priority to this? Thanks!

rynowak commented 6 years ago

Fixed for the 2.2.0 release. Thanks for the feedback 👍

jmezach commented 6 years ago

Nice, looking forward to trying out the new bits.