search

dotnet-architecture / HealthChecks

Experimental Health Checks for building services, such as with ASP.NET Core
Other
454 stars 124 forks source link

Healthcheck services should cache the results #12

Open ycrumeyrolle opened 7 years ago

ycrumeyrolle commented 7 years ago

Each check should be cacheable for a period of time. For example http endpoint checks may be cached for 10 seconds, database checks may be cached for 2 seconds, ...

bradwilson commented 7 years ago

This was just pushed up in https://github.com/aspnet/HealthChecks/commit/4f655747b4d15a961c12d5f4bae72772d4e4aa61. Let us know what you think of the usage! We'll get something integrated into the sample soon.

ycrumeyrolle commented 7 years ago

I think that cache management should be done by the HealthChekService, not by the base class HealthCheck itself. This allow to not be implement by each IHealthCheck implementation.

bradwilson commented 7 years ago

The flip side is that by pushing caching logic into the health check, it allows complex health checks to use their own caching logic rather than being stuck with the fairly simplistic one that we've implemented by default.

ycrumeyrolle commented 7 years ago

I see your point of view, but I do not see any complex caching logic need with my current use cases (hundreds of monitored applications). I would better see a common logic in the service, and a way to override the default behavior. For example with an ICheckCache.

In my custom implementation of the HealthChecks, I have separate the IHealthCheck into two separate classes. One for the settings, one for handling the check. This allows to separate the responsibility and to have settings as a base class VS check handler as an interface. In this case the overload of the specific behaviour would be in the settings class, not in the check implementation. See https://github.com/ycrumeyrolle/HealthCheck/blob/master/src/AspNetCore.HealthCheck.Abstractions/IHealthWatcher.cs andhttps://github.com/ycrumeyrolle/HealthCheck/blob/master/src/AspNetCore.HealthCheck.Abstractions/WatchSettings.cs

Is it required to have a lock or pseudo lock? In case of concurrent requests, the check will occur twice, which should not be a problem.

And last question. Should we cache the failures?

bradwilson commented 7 years ago

Is it required to have a lock or pseudo lock?

We do have a lock.

In case of concurrent requests, the check will occur twice, which should not be a problem.

Our locking code prevents this.

Should we cache the failures?

Definitely. Hammering a down service is a great way to ensure it never stands back up again.

bradwilson commented 7 years ago

In PR #50, the caching responsibility is moved out of the health check itself and into the service (by way of a helper class).