Introduce HealthIndicatorRegistry with support for invocation of HealthIndicators

[vpavic]

This PR introduces HealthIndicatorRegistry which handles registration and invocation of HealthIndicator instances. Registering new HealthIndicator instances (as well as unregistering) is now possible in runtime (see #4894).

The default implementation offers the option to run the registered health indicators in parallel therefore improving the performance of HealthEndpoint invocations in scenarios with multiple expensive HealthIndicators (see #2652).

[vpavic]

./mvnw clean install passed for me locally, closing and re-opening the PR to force a new Travis CI build.

[shakuzen]

Shouldn't this use a logger instead?

[vpavic]

Ugh, don't know how I managed to miss this - thanks!

[shakuzen]

Looks like a really nice enhancement. I look forward to being able to use it - especially health checks run in parallel. 👍

[wilkinsona]

Thanks for the PR, @vpavic.

To safely support registration at runtime, the registry needs to be thread-safe and the current implementation isn't. I suspect that plain old synchronization, rather than something like a read-write lock, will probably be sufficient.

[shakuzen]

@wilkinsona Would simply making this a ConcurrentHashMap alleviate the thread safety concerns regarding registration at runtime?

[wilkinsona]

Maybe :) It depends on the desired semantics. Using a concurrent hash map could result in an indicator being called after it's been removed from the registry. Similarly, the contract of getRegisteredNames needs to be tightened up. Is it intended as a view of keys that may change, or as a snapshot taken at the time of the call?

[vpavic]

getRegisteredNames is intended as a current snapshot.

[wilkinsona]

The current implementation provides a view onto the keys of the underlying map so its contents may change if an indicator is (un)registered.

[vpavic]

I see - I forgot to wrap key set into a new set.

[wilkinsona]

@vpavic Looking at this some more, two further thoughts have occurred to me:

• Do indicators need to be registered by name? It would simplify things quite a bit if they were not
• Should the registry be running the indicators? It feels like it has two responsibilities

[vpavic]

@wilkinsona I'd say the answer to both questions is yes.

Regarding registration by name, IMO registry should maintain a name, or key, for each of managed HealthIndicators. Take the HealthEndpoint as an example of registry user - originally it contained CompositeHealthIndicator which is again a map of HealthIndicators. If HealthEndpoint has a key to reference some HealthIndicator, I'd expect to be able to use the same key for that indicator in other parts of the system.

Invocation of indicator does make it two responsibilites, but it also seems somewhat natural, doesn't it? Having invocation service separated from the registry IMO seems more complicated for the users. Do you see any downsides of this approach?

[vpavic]

@wilkinsona, @shakuzen, thanks for your feedback, I've updated the PR accordingly.

[wilkinsona]

This isn't thread-safe

[wilkinsona]

Invocation of indicator does make it two responsibilites, but it also seems somewhat natural, doesn't it?

Not to me. We already have a class, CompositeHealthIndicator, that runs all of the known indicators. With this change, we have two classes that do that. IMO, the registry should just be a registry and the composite indicator should continue to be responsible for running them all.

[vpavic]

@wilkinsona CompositeHealthIndicator is just another implementation of HealthIndicator interface. I don't see where it is specified that its purpose it to run all of the known indicators, if you mean known as in known globally in an app? CompositeHealthIndicators can be used to provide custom health info just like any other HealthIndicator (see our use case in #4674). Secondly, CompositeHealthIndicator is quite opinionated with regard to how to represent the overall Health result since it uses a HealthAggregator.

Also, how does a user gets a reference to a "service" that invokes health indicator(s)? IMO there should be a dedicated interface which I could use within my app to invoke all HealthIndicators, or just the one that I'm interested in, and leave the aggregation aspect to the caller. Injecting and invoking HealthEndpoint does not meet the requirement since its aggregation might not suite users. That's why HealthIndicatorRegistry.runHealthIndicators returns a simple map of keys and Healths. And if you provide such service it makes sense to place this functionality into the registry itself, rather than having two services.

[wilkinsona]

CompositeHealthIndicator is quite opinionated with regard to how to represent the overall Health result since it uses a HealthAggregator.

HealthAggregator's an interface with an API that allows you to perform the aggregation however you like. IMO, CompositeHealthIndicator doesn't have an opinion at all. It's the HealthAggregator implementation that it delegates to that does.

IMO there should be a dedicated interface which I could use within my app to invoke all HealthIndicators, or just the one that I'm interested in, and leave the aggregation aspect to the caller

I'm not necessarily disagreeing with this, but I do disagree with the registry performing this role.

What happens if I want to get hold of all of the indicators and call them in a different way? The current API is cumbersome at best for accessing all of the registered indicators. It appears to be designed with the assumption that the way in which it calls all of the indicators will suit everyone.

[wilkinsona]

Here's a proposal for the registry API:

package org.springframework.boot.actuate.health;

import java.util.Map;

/**
 * A registry of {@link HealthIndicator}s.
 * <p>
 * Implementations <strong>must</strong> be thread-safe.
 *
 * @author Andy Wilkinson
 * @author Vedran Pavic
 */
public interface HealthIndicatorRegistry {

    /**
     * Registers the given {@code healthIndicator}, associating it with the given
     * {@code name}.
     * @param name the name of the indicator
     * @param healthIndicator the indicator
     * @throws IllegalStateException if an indicator with the given {@code name} is
     * already registered.
     */
    void register(String name, HealthIndicator healthIndicator);

    /**
     * Unregisters the {@code HealthIndicator} previously registered with the given
     * {@code name}.
     *
     * @param name the name of the indicator
     * @return the unregistered indicator, or {@code null} if no indicator was found in
     * the registry for the given {@code name}.
     */
    HealthIndicator unregister(String name);

    /**
     * Returns the health indicator registered with the given {@code name}.
     *
     * @param name the name of the indicator
     * @return the health indicator, or {@code null} if no indicator was registered with
     * the given {@code name}.
     */
    HealthIndicator get(String name);

    /**
     * Returns a snapshot of the registered health indicators and their names. The
     * contents of the map do not reflect subsequent changes to the registry.
     * @return the snapshot of registered health indicators
     */
    Map<String, HealthIndicator> getAll();

}

I'm not 100% sure on the return type for getAll. A List of some sort of holder that encapsulates the indicator and its name is a possible alternative.

[vpavic]

HealthAggregator's an interface with an API that allows you to perform the aggregation however you like. IMO, CompositeHealthIndicator doesn't have an opinion at all. It's the HealthAggregator implementation that it delegates to that does.

This is fine when considering CompositeHealthIndicator as a special implementation of HealthAggregator which is, as the name itself says, composite :) I'm just saying that a service that invokes all the indicators shouldn't deal with aggregation. The notion of aggregation itself would make that service opinionated, at least for me.

What happens if I want to get hold of all of the indicators and call them in a different way? The current API is cumbersome at best for accessing all of the registered indicators. It appears to be designed with the assumption that the way in which it calls all of the indicators will suit everyone.

OK, so it might be good thing to explore the possibilites of separate API for invocation of indicators, with something like invocation strategy to define the way indicators are invoked (sequentially, in parallel, partially etc.). WDYT?

The updated API looks concise. A List of registration holders seems better option for getAll.

[snicoll]

IMO, the registry should just be a registry and the composite indicator should continue to be responsible for running them all.

Me too.

[wilkinsona]

I'm just saying that a service that invokes all the indicators shouldn't deal with aggregation. The notion of aggregation itself would make that service opinionated, at least for me.

Got you. I agree that it should be possible to invoke all the indicators and access the individual results.

OK, so it might be good thing to explore the possibilites of separate API for invocation of indicators, with something like invocation strategy to define the way indicators are invoked (sequentially, in parallel, partially etc.). WDYT?

This is definitely worth exploring, although I'm not yet convinced that it's necessary. Let's sketch something out (either in a separate PR or in comments on #2652) and then take it from there.

I think the best thing to do with this PR is to close it as we should tackle the registry and the parallel invocation separately. I'll add some comments to #4894 for the registry API design.