Allow health checks to use any DI lifetime #466
Conversation
| "identity", | ||
| }, | ||
| // Exclude all checks, just return a 200. | ||
| Predicate = (check) => false, |
There was a problem hiding this comment.
Changing this to an arbitrary predicate rather than an explicit 'filter by name' seems like a good usability improvement. Since the set of checks has to be resolved per-request a lot of motivations for providing 'filter by name' seemed to melt away.
| /// The key maps to the <see cref="IHealthCheck.Name"/> property of the health check, and the value is the <see cref="IHealthCheck"/> | ||
| /// instance itself. | ||
| /// </remarks> | ||
| IReadOnlyDictionary<string, IHealthCheck> Checks { get; } |
There was a problem hiding this comment.
This has to go away now because they aren't singletons. The main use case we had for providing this was to support filtering. If someone wants to get the set of checks for an arbitrary reason they can create a scope and resolve them 😆
| throw new ArgumentNullException(nameof(builder)); | ||
| } | ||
|
|
||
| if (typeof(T).IsAssignableFrom(typeof(IHealthCheck))) |
There was a problem hiding this comment.
typeof(IHealthCheck).IsAssignableFrom(typeof(T))
There was a problem hiding this comment.
yeah going point. I think this was a bad copy-paste.
| /// provided implementation type <typeparamref name="T"/>. Using this method to register a health | ||
| /// check allows you to register a health check that depends on transient and scoped services. | ||
| /// </remarks> | ||
| public static IHealthChecksBuilder AddCheck<T>(this IHealthChecksBuilder builder) where T : class |
There was a problem hiding this comment.
Couldn't you do where T : class, IHealthCheck? Better than doing a runtime check
| Checks = healthChecks.ToDictionary(c => c.Name, StringComparer.OrdinalIgnoreCase); | ||
| if (predicate != null) | ||
| { | ||
| healthChecks = healthChecks.Where(predicate); |
There was a problem hiding this comment.
You could avoid the Linq call by doing this inside the foreach
| // If the health check does things like make Database queries using EF or backend HTTP calls, | ||
| // it may be valuable to know that logs it generates are part of a health check. So we start a scope. | ||
| using (_logger.BeginScope(new HealthCheckLogScope(check.Name))) | ||
| if (!names.Add(check.Name)) |
There was a problem hiding this comment.
This would print the same name multiple times if the check is registered more than twice. Think a GroupBy would be terse and more correct
| /// </remarks> | ||
| public ISet<string> HealthCheckNames { get; } = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | ||
| public Func<IHealthCheck, bool> Predicate { get; set; } |
There was a problem hiding this comment.
Eh, we considered it and thought this was less overloaded. If we use filter (or another term) other places in the framework then I want to change this.
| HealthCheckResult result; | ||
| try | ||
| { | ||
| _logger.LogTrace("Running health check: {healthCheckName}", healthCheck.Name); |
There was a problem hiding this comment.
This isn't the logging pattern we use in components.
This change allows registered IHealthCheck implementations to use any DI lifetime. This is necessary for scenarios like using EF which requires a scope. The works by having the health check service create a scope for each time it queries health checks. This scope does not overlap or share state with other scopes (the request scope) so there is no crosstalk between processing going on per-request in ASP.NET Core and the health check operation.
rynowak
force-pushed
the
rynowak/time-to-di
branch
from
76f965c
to
1d54c6c
Compare
|
Updated this and cleaned up the logging. I logged an issue to consider the logging further, I don't think what's there is good enough but I don't want to drill into as part of fixing DI. |
| _logger.LogError(ex, "Health check '{healthCheckName}' threw an unexpected exception", check.Name); | ||
| result = new HealthCheckResult(HealthCheckStatus.Failed, ex, ex.Message, data: null); | ||
| } | ||
| public static readonly EventId HealthCheckBegin = new EventId(100, "HealthCheckBegin"); |
This change allows registered IHealthCheck implementations to use any DI
lifetime. This is necessary for scenarios like using EF which requires a
scope.
The works by having the health check service create a scope for each
time it queries health checks. This scope does not overlap or share
state with other scopes (the request scope) so there is no crosstalk
between processing going on per-request in ASP.NET Core and the health
check operation.