Skip to content
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

Introduce HealthIndicatorRegistry #4965

Closed

Conversation

Projects
None yet
8 participants
@vpavic
Copy link
Member

commented Jan 18, 2016

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

I've signed the CLA.

@vpavic vpavic force-pushed the vpavic:health-indicator-registry branch from 9c87e41 to dbf7d54 Jan 18, 2016

@Override
public Map<String, HealthIndicator> getAll() {
return Collections.unmodifiableMap(
new HashMap<String, HealthIndicator>(this.healthIndicators));

This comment has been minimized.

Copy link
@wilkinsona

wilkinsona Jan 18, 2016

Member

Unfortunately, and somewhat surprisingly, this isn't thread-safe. It's doing a putAll under the covers, the documentation for which says:

The behavior of this operation is undefined if the specified map is modified while the operation is in progress.

With the current implementation of HashMap it should work – it iterates over the concurrent hash map's entry set and doing so is thread-safe – but that's an implementation detail that we shouldn't rely on. As a result, everything needs to be synchronized which means there's no longer any point in using ConcurrentHashMap.

This comment has been minimized.

Copy link
@vpavic

vpavic Jan 18, 2016

Author Member

I see, somewhat suprisingly indeed.

So you're suggesting we should ditch the ConcurrentHashMap and just synchronize all implemented operations on the healthIndicators Map itself?

Wouldn't this do the job with less code:

    @Override
    public Map<String, HealthIndicator> getAll() {
        synchronized (this.healthIndicators) {
            return Collections.unmodifiableMap(
                    new HashMap<String, HealthIndicator>(this.healthIndicators));
        }
    }

This comment has been minimized.

Copy link
@wilkinsona

wilkinsona Jan 18, 2016

Member

A single synchronized block doesn't stop other threads from mutating the map via register or unregister.

@vpavic vpavic force-pushed the vpavic:health-indicator-registry branch from dbf7d54 to c166a8a Jan 19, 2016

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2016

@wilkinsona I've updated the PR, please review the changes.

@wilkinsona

This comment has been minimized.

Copy link
Member

commented Jan 20, 2016

Looks good now. Thanks.

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2016

Any chance this makes is it into 1.4?

@philwebb philwebb force-pushed the spring-projects:master branch from 928511b to cd365bc May 14, 2016

@jgoldhammer

This comment has been minimized.

Copy link

commented May 16, 2016

+1 for 1.4

public void register(String name, HealthIndicator healthIndicator) {
Assert.notNull(healthIndicator, "HealthIndicator must not be null");
synchronized (this.healthIndicators) {
if (this.healthIndicators.get(name) != null) {

This comment has been minimized.

Copy link
@kartoffelsup

kartoffelsup May 16, 2016

Why not Assert.state(!this.healthIndicators.containsKey(name), text)?

This comment has been minimized.

Copy link
@vpavic

vpavic May 17, 2016

Author Member

Using Assert.state would require constructing exception message string on each invocation of register operation, vs just in scenarios where IllegalStateException indeed is thrown, which presents a certain overhead.

Other than that, IMO current code is more readable as well.

This comment has been minimized.

Copy link
@kartoffelsup

kartoffelsup May 17, 2016

Sounds reasonable. But why wouldn't you want to use containsKey?

This comment has been minimized.

Copy link
@vpavic

vpavic May 18, 2016

Author Member

Even though there is a check for null values on HealthIndicator registration, Map API itself permits null values so this approach IMO expresses better how we're using healthIndicators Map.

*/
public class DefaultHealthIndicatorRegistry implements HealthIndicatorRegistry {

private final Map<String, HealthIndicator> healthIndicators =

This comment has been minimized.

Copy link
@Aloren

Aloren May 17, 2016

Why not use ConcurrentHashMap?

This comment has been minimized.

Copy link
@wilkinsona

wilkinsona May 17, 2016

Member

That's how it was in a previous revision, but then getAll isn't thread-safe without using synchronized. At that point, there's no benefit to using ConcurrentHashMap.

This comment has been minimized.

Copy link
@vpavic

vpavic May 17, 2016

Author Member

Since operations on healthIndicators map require external synchronization anyway (see register operation as the most obvious example), using a ConcurrentHashMap would introduce an unnecessary overhead.

See comments on outdated diffs for more background.

This comment has been minimized.

Copy link
@Aloren

Aloren May 17, 2016

Hmm... Can't we just use putIfAbsent in register method? There is such method in ConcurrentHashMap, which is thread safe. Method getAll will be thread safe as well and will return snapshot of the map, of course, some of the updates will not be seen immediately. Is it critical?

This comment has been minimized.

Copy link
@vpavic

vpavic May 17, 2016

Author Member

That's true, I've forgot about that one. However, there's still @wilkinsona's comment on getAll, which was the original reason to drop ConcurrentHashMap.

As already mentioned, refer to the older comments on this PR.

This comment has been minimized.

Copy link
@Aloren

Aloren May 17, 2016

Why would getAll be not thread-safe? It is absolutely legal to iterate concurrent map. :) Updates are not guaranteed to be seen while iteration, but on next iteration they will be seen -- is that an issue you are talking about?

This comment has been minimized.

Copy link
@wilkinsona

wilkinsona May 17, 2016

Member

@Aloren Have you read the comments on the outdated diff from January 18? It's explained there. I'd link to it, but GitHub doesn't make it easy. You should be able to find it from here: #4965 (comment).

This comment has been minimized.

Copy link
@Aloren

Aloren May 17, 2016

@wilkinsona Looks like implementation in java 8 changed for HashMap. There should not be such issue now. Was not aware about that. Thanks for the link. Now it is clear.

@vpavic vpavic force-pushed the vpavic:health-indicator-registry branch 2 times, most recently from 6023907 to 5b9a69a Jun 12, 2016

@philwebb

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

I'm afraid we're probably not going to have time to get his in 1.4. We'll revisit it again for 1.5. Thanks!

@vpavic vpavic force-pushed the vpavic:health-indicator-registry branch 3 times, most recently from f11c11c to 289d6e5 Sep 3, 2016

@vpavic vpavic force-pushed the vpavic:health-indicator-registry branch from 289d6e5 to 8364464 Jan 18, 2017

@vpavic vpavic force-pushed the vpavic:health-indicator-registry branch from 8364464 to 2ca7acb Sep 24, 2017

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2017

I've adapted this PR to the new Actuator infra.

Any chance this gets a review/consideration for 2.0?

@snicoll

This comment has been minimized.

Copy link
Member

commented Sep 25, 2017

Thanks a lot @vpavic! I am not sure we'll have time to review this one for 2.0 still. I am flagging it now to get more feedback.

@vpavic

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2018

Thanks @snicoll, that's great news - yes, I started rebasing this shortly after master was switched to 2.1.x but didn't finish. I'll continue that effort this week and update the PR.

Introduce HealthIndicatorRegistry
This commit introduces HealthIndicatorRegistry which handles registration of HealthIndicator instances. Registering new HealthIndicator instances is now possible in runtime.

@vpavic vpavic force-pushed the vpavic:health-indicator-registry branch from 2ca7acb to a161f14 May 3, 2018

@vpavic

This comment has been minimized.

Copy link
Member Author

commented May 3, 2018

I've rebased the proposed changes onto the current master.

Regarding the reactive support:

Shouldn't be too hard to do (famous last words) by genericizing the registry to hold HealthIndicator or ReactiveHealthIndicator instances.

I didn't take on this yet since HealthIndicator and ReactiveHealthIndicator don't have any shared subtype and I didn't want to make HealthIndicatorRegistry too generic as that would basically make it an object registry. Did you have any concrete proposals on this?

@philwebb philwebb force-pushed the spring-projects:master branch from 86d82f9 to 7b120c1 May 4, 2018

snicoll added a commit to snicoll/spring-boot that referenced this pull request May 16, 2018

Introduce HealthIndicatorRegistry
This commit introduces HealthIndicatorRegistry which handles
registration of HealthIndicator instances. Registering new
HealthIndicator instances is now possible in runtime.

See spring-projectsgh-4965

snicoll added a commit to snicoll/spring-boot that referenced this pull request May 16, 2018

Polish "Introduce HealthIndicatorRegistry"
This commit is work in progress and polishes the initial submission by
making sure that the CompositeHealthIndicator is also a registry.

The current infrastructure prevents the composite to be registered as a
bean as it would be an extra HealthIndicator in the context and
therefore may be taken into account as a regular indicator. The registry
on the other hand must be registered in the context so that users can
manipulate its content.

Ultimately, both those objects share common base features that we should
not duplicate. This commit makes sure that the registry has a `health()`
method but does not implement HealthIndicator. The composite now extends
from the registry and implements HealthIndicator, simply delegating to
that parent method.

It's unclear at this point if we want to keep this arrangement as the
duplication of factories is a bit annoying.

See spring-projectsgh-4965
@snicoll

This comment has been minimized.

Copy link
Member

commented May 16, 2018

Alright, I've had an extensive review and a good chunk of polishing in 23585d2. Paging @wilkinsona for a review.

The main problem here is that CompositeHealthIndicator and HealthIndicatorRegistry share the same basic goal (even if the former only allows you to add indicators) while having a basic difference: the former must be a HealthIndicator and the latter must not. I didn't want to make them completely isolated as it feels we have two types to do the same thing so I went ahead with one extending from the other. This brings a bit of duplication in the factory that I don't like.

The other reason I went ahead with this approach is that we have a composite for the reactive support and we'll need a reactive registry as well. Once we figure out what the best arrangement for this is, we can easily adapt that to the reactive bits.

@snicoll snicoll self-assigned this May 16, 2018

@snicoll snicoll requested a review from wilkinsona May 16, 2018

wilkinsona added a commit to wilkinsona/spring-boot that referenced this pull request May 16, 2018

Introduce HealthIndicatorRegistry
This commit introduces HealthIndicatorRegistry which handles
registration of HealthIndicator instances. Registering new
HealthIndicator instances is now possible in runtime.

See spring-projectsgh-4965

wilkinsona added a commit to wilkinsona/spring-boot that referenced this pull request May 16, 2018

Polish "Introduce HealthIndicatorRegistry"
This commit is work in progress and polishes the initial submission by
making sure that the CompositeHealthIndicator is also a registry.

The current infrastructure prevents the composite to be registered as a
bean as it would be an extra HealthIndicator in the context and
therefore may be taken into account as a regular indicator. The registry
on the other hand must be registered in the context so that users can
manipulate its content.

Ultimately, both those objects share common base features that we should
not duplicate. This commit makes sure that the registry has a `health()`
method but does not implement HealthIndicator. The composite now extends
from the registry and implements HealthIndicator, simply delegating to
that parent method.

It's unclear at this point if we want to keep this arrangement as the
duplication of factories is a bit annoying.

See spring-projectsgh-4965

@snicoll snicoll modified the milestones: Backlog, 2.1.0.M1 May 16, 2018

snicoll added a commit that referenced this pull request May 16, 2018

Merge pull request #4965 from vpavic:health-indicator-registry
* pr/4965:
  Polish "Introduce HealthIndicatorRegistry"
  Introduce HealthIndicatorRegistry

snicoll added a commit that referenced this pull request May 16, 2018

Introduce HealthIndicatorRegistry
This commit introduces HealthIndicatorRegistry which handles
registration of HealthIndicator instances. Registering new
HealthIndicator instances is now possible in runtime.

See gh-4965

snicoll added a commit that referenced this pull request May 16, 2018

Polish "Introduce HealthIndicatorRegistry"
See gh-4965

Co-authored-by: Andy Wilkinson <awilkinson@pivotal.io>

@snicoll snicoll closed this in 2c176a3 May 16, 2018

@snicoll

This comment has been minimized.

Copy link
Member

commented May 16, 2018

Thank you very much @vpavic and @wilkinsona - This is now merged. There is a ReactiveHealthIndicatorRegistry companion for reactive-based apps.

@vpavic vpavic deleted the vpavic:health-indicator-registry branch May 16, 2018

@vpavic

This comment has been minimized.

Copy link
Member Author

commented May 29, 2018

Thanks for merging this @snicoll, I've finally managed to take a closer look at the subsequent changes to HealthIndicator infrastructure.

With the current arrangement, some scenarios that were previously simple to setup are now more complicated. For example, if one wants to use CompositeHealthIndicator as a custom HealthIndicator that combines other HealthIndicators (ones that don't provide any value on their own and are not registered as beans) it is now required to setup an additional HealthIndicatorRegistry, which shouldn't be registered as a bean (since it would otherwise disabled the default one). The need for this is also reflected in CompositeHealthIndicatorConfiguration and CompositeReactiveHealthIndicatorConfiguration.

Previously, this was possible by simply creating CompositeHealthIndicator either supplying a Map of HealthIndicators or using CompositeHealthIndicator#addHealthIndicator.

Additionally, with CompositeHealthIndicator being aware of HealthIndicatorRegistry things feel a bit tangled IMO. A CompositeHealthIndicator is still a HealthIndicator and as such I believe it shouldn't be aware of HealthIndicatorRegistry.

@snicoll

This comment has been minimized.

Copy link
Member

commented May 29, 2018

it is now required to setup an additional HealthIndicatorRegistry

It is as simple as new DefaultHealthIndicatorRegistry(indicators). Am I missing something or are you saying that a method that does that for you would be better?

it is now required to setup an additional HealthIndicatorRegistry, which shouldn't be registered as a bean

I am not sure I am following. You'd previously get the list of HealthIndicator in some way and you couldn't register them either as they'd pick it up by the main indicator. How is that different?

A CompositeHealthIndicator is still a HealthIndicator and as such I believe it shouldn't be aware of HealthIndicatorRegistry.

Sorry I am not following neither the argument nor the actual problem that it causes. Can you please explain that in more details?

@vpavic

This comment has been minimized.

Copy link
Member Author

commented May 29, 2018

It is as simple as new DefaultHealthIndicatorRegistry(indicators). Am I missing something or are you saying that a method that does that for you would be better?

I am not sure I am following. You'd previously get the list of HealthIndicator in some way and you couldn't register them either as they'd pick it up by the main indicator. How is that different?

Yes, either way to begin with there needs to be a HealthIndicator collection of some sort. Being forced to wrap it with a DefaultHealthIndicatorRegistry is an overhead.

Sorry I am not following neither the argument nor the actual problem that it causes. Can you please explain that in more details?

When some component is considered a registry, to me that sounds like a central place that manages instances of some lower level component. To have the managed component be aware of the registry is awkward and confusing, at least to me.

@snicoll

This comment has been minimized.

Copy link
Member

commented May 29, 2018

When some component is considered a registry, to me that sounds like a central place that manages instances of some lower level component.

I disagree with that but either way the current arrangement does not enforce it one way or the other. I read your feedback as a serious issue on your side and the only thing I can think of would be to remove the deprecation on the constructor that takes the Map. If you're happy with that, then I guess I am overreacting a bit. If you aren't then, sorry, but I still don't understand the problem.

@vpavic

This comment has been minimized.

Copy link
Member Author

commented May 29, 2018

I read your feedback as a serious issue on your side

Not sure what was that supposed to mean, but just to clarify - the feedback I've provided is solely about my observations on changes done vs the original state of this PR. I didn't suggest that something was broken or not usable, but rather that I found some things to be confusing, and that some use cases require more code and involve more components than they did before.

To go back to HealthIndicatorRegistry for a moment, since implementation of that contract is registered as a bean in the application context users typically interact with that contract by injecting in their own components and consuming the API. Creating other instances of such component/service is unusual and such arrangement could lead someone to create CompositeHealthIndicator with registry injected from the application context.

the only thing I can think of would be to remove the deprecation on the constructor that takes the Map

I wouldn't remove the deprecation, as there should be a clearly preferred way to construct CompositeHealthIndicator. The middle ground solution that I can propose would be to introduce an intermediary API that would contain only Map<String, HealthIndicator> getAll() operation and which would HealthIndicatorRegistry then extend. This has a nice benefit of the new API being @FunctionalInterface which can then help prevent instantiation of needless DefaultHealthIndicatorRegistry instances all over the place. If that's acceptable to you, I can put together a PR.

@snicoll

This comment has been minimized.

Copy link
Member

commented May 29, 2018

Creating other instances of such component/service is unusual and such arrangement could lead someone to create CompositeHealthIndicator with registry injected from the application context.

I disagree with the unusual part. Having said that, I get it now. Even if we have deprecated the method that allows you to register additional indicators, you can still do so by accessing the registry. Would that help if the composite returned a unmodifiable Map rather than the registry? This would be a good enough hint that you should provide your own registry rather than relying on an existing one.

The getRegistry method was added after this PR got merged by the way (to support component instance health indicator). I think that was a mistake.

Thanks for bearing with me :)

snicoll added a commit that referenced this pull request May 30, 2018

@snicoll

This comment has been minimized.

Copy link
Member

commented May 30, 2018

@vpavic I had a call with @wilkinsona to discuss this and we believe the current design is the best compromise at this point. Having a intermediate API that the registry implements will not fix the problem that someone may accidentally pass the registry from the context (as it would implement this contract). I have, however, remove the deprecation on the constructor that takes a Map as all the replacements are simply creating a registry out of the map. This makes it a better upgrade path for existing users.

As for the link between the registry and the composite, it is inevitable. Previously the composite was responsible of two things: 1. getting a list of health indicators with the ability to add more, and 2. providing a health indicator on the composite. The first responsibility has been moved to the registry but the registry is then responsible to provide the list of indicators since it's managing that state.

We've also extensively discussed the reason we have a getRegistry on the composite. We believe it has to stay as removing this link means you have to wrap it in some object that keep a link between the "sub-registry" and the composite, avoiding to expose a registry in the context that would disable the main one the auto-configuration generates. In the end, things are pretty consistent IMO, you get a list of health indicators and the only thing you register as a bean is the composite, allowing you to manage said composite.

@vpavic

This comment has been minimized.

Copy link
Member Author

commented May 30, 2018

Thanks for looking into this.

Having a intermediate API that the registry implements will not fix the problem that someone may accidentally pass the registry from the context (as it would implement this contract).

Right, that's why I referred to it as middle ground solution when I originally brought it up. My preference has always been for CompositeHealthIndicator to deal with Map<String, HealthIndicator> like it did before, but OK. I expect we'll touch on arrangement of these components again when I revisit #5066, which should be soon.

Regarding CompositeHealthIndicator#getRegistry, IMO there's no need to expose that method as public since its only consumer is HealthEndpoint, which is in the same package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.