-
Notifications
You must be signed in to change notification settings - Fork 22
[WIP] Dependency Injected HealthChecks #659
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
Conversation
- Add WithHealthCheck<T>() generic methods for DI-resolved health checks - Support both simple registration and template-based configuration - Implement DiResolvedHealthCheck<T> wrapper for lazy DI resolution
…ption' if the health check type is not registered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detailed my changes to #658
| /// </summary> | ||
| /// <exception cref="ArgumentNullException"></exception> | ||
| public IAkkaHealthCheck HealthCheck | ||
| public Func<IServiceProvider,IAkkaHealthCheck> Factory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of a breaking change here - decided to use the Microsoft.Extensions.HealthCheck pattern here of passing in the IServiceProvider so we can resolve any CTOR dependencies a healthcheck might require. This addresses my comment on #658 (comment) but without requiring us to do any updates to Akka.DependencyInjection.
I prefer it this way because the health checks infrastructure is coupled to the IServiceCollection anyway - no point in having a separate abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think the impact of this change should be minimal.
| /// <param name="timeout">An optional <see cref="TimeSpan"/> representing the timeout of the check.</param> | ||
| /// <exception cref="ArgumentNullException">Thrown if <see cref="name"/> is null.</exception> | ||
| /// <exception cref="ArgumentOutOfRangeException">Thrown if a negative timeout other than <see cref="System.Threading.Timeout.InfiniteTimeSpan"/> is used.</exception> | ||
| internal AkkaHealthCheckRegistration(string name, Func<IServiceProvider, IAkkaHealthCheck> factory, HealthStatus? failureStatus, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CTOR overload which takes an Func<IServiceProvider, IAkkaHealthCheck> delegate . I don't see much value in making it public but I could be persuaded either way - the extension methods on the AkkaConfigurationBuilder use this for DI'd health check registrations.
| HealthChecks[registration.Name] = registration; | ||
| return this; | ||
|
|
||
| static T GetServiceOrCreateInstance(IServiceProvider sp) => ActivatorUtilities.GetServiceOrCreateInstance<T>(sp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the big time-saver - it uses the ActivatorUtilities from MSFT.EXT.DI to handle the IServiceProvider resolution / instantiation of CTOR arguments for us, so we're not having to write that code ourselves. This is the same thing that powers the DI'd Props in Akka.DependencyInjection.
| // func for lazily instantiating the health check registration | ||
| Func<IServiceProvider, IHealthCheck> adapter = provider => | ||
| new HealthCheckAdapter(registration.HealthCheck, provider.GetRequiredService<ActorSystem>()); | ||
| new HealthCheckAdapter(registration.Factory(provider), provider.GetRequiredService<ActorSystem>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the health check adapter only slightly here - it's still a delayed instantiation until the health checks actually get invoked.
|
I need to add a tiny bit of documentation for this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes
Supersedes #658
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):