Skip to content

Allow HealthIndicator registration in runtime #4894

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

Closed
vpavic opened this issue Jan 7, 2016 · 11 comments
Closed

Allow HealthIndicator registration in runtime #4894

vpavic opened this issue Jan 7, 2016 · 11 comments
Labels
status: duplicate A duplicate of another issue

Comments

@vpavic
Copy link
Contributor

vpavic commented Jan 7, 2016

Currently HealthIndicator instances are required to be registered as Spring beans in order to provide health information. This means that Boot's Health information infrastructure cannot be easily used for health information on integration points which are not known when application context is created, or are simply dynamic by nature and can be changed in runtime.

IMO introduction of something like HealthIndicator registry would allow this use case while also retaining compatibility with current approach.

If this suggestion is accepted I'd be willing to work on a PR to provide such functionality.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 7, 2016
@vpavic
Copy link
Contributor Author

vpavic commented Jan 7, 2016

Requested feature would be similar to HealthCheckRegistry from Dropwizard Metrics.

@wilkinsona
Copy link
Member

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();

}

Note that the API deliberately doesn't make any promises about the ordering of the health indicators returned by getAll. It's likely that an implementation may want to order the health indicators (insertion order, Spring's @Order, etc) but I can't see a need for the API to constrain an implementation's options.

@wilkinsona wilkinsona added type: enhancement A general enhancement for: team-attention An issue we'd like other members of the team to review and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 18, 2016
@vpavic
Copy link
Contributor Author

vpavic commented Jan 18, 2016

OK, I'll create a new PR to address the registry implementation.

My idea in the original PR was also to leave the ordering out of the API. Regarding getAll, as discussed in the original PR, should we prefer to return a List of holder objects?

@wilkinsona
Copy link
Member

as discussed in the original PR, should we prefer to return a List of holder objects?

I'm still not sure about that. That's one reason why this is labelled as being for team discussion.

@snicoll
Copy link
Member

snicoll commented Jan 18, 2016

I much prefer Andy's proposal regarding getAll.

@wilkinsona
Copy link
Member

This means that Boot's Health information infrastructure cannot be easily used for health information on integration points which are not known when application context is created, or are simply dynamic by nature and can be changed in runtime

@vpavic Can you give us some concrete examples please? We're struggling to think of a real-world use case for this.

@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Jan 27, 2016
@vpavic
Copy link
Contributor Author

vpavic commented Jan 27, 2016

@wilkinsona I didn't know this was such an exotic requirement :)

We're having such use cases in virtually all of our projects.

We develop solutions for telcos and these projects regulary include a lot of integration with proprietary external systems. Having some sort of a healt indicator to monitor the status of connection is a must, but also we often have the requirement to register, unregister and reconfigure connections to those systems during runtime. When connector to some instance of these external systems is either discarded, or a new one is created, naturally this also affects the HealthIndicator.

Previously we've been using Dropwizard Metrics for our projects, and as mentioned in second post it has its registry so this is something we miss in Spring Boot and was inspiration for this issue and subsequent PR.

@wilkinsona
Copy link
Member

@vpavic

I didn't know this was such an exotic requirement :)

You're the first person who's asked for it.

When connector to some instance of these external systems is either discarded, or a new one is created, naturally this also affects the HealthIndicator.

Could this also be tackled with a custom health indicator that's aware of a dynamic set of connections?

@vpavic
Copy link
Contributor Author

vpavic commented Jan 29, 2016

@wilkinsona

You're the first person who's asked for it.

Yes, I'm aware of that. I jokingly referred to it as "exotic requirement" because it's a pattern seen in other libraries and one that definitely has its advantages.

Could this also be tackled with a custom health indicator that's aware of a dynamic set of connections?

No, I'm afraid not. That makes it hard to obtain a reference to individual HealthIndicators that make a dynamic set of connections since they wouldn't be registered in a central place with other HealthIndicators (unfortunately that central place is ApplicationContext ATM).

Your suggestion is part of the workaround we have in place at the moment in our project and I'm not quite happy with it.

@philwebb
Copy link
Member

We're going to need to redesign a large chunk of our actuator code to support reactive programming. I think this one should go on hold until that work has been completed.

@snicoll
Copy link
Member

snicoll commented Sep 25, 2017

Duplicate of #4965

@snicoll snicoll marked this as a duplicate of #4965 Sep 25, 2017
@snicoll snicoll closed this as completed Sep 25, 2017
@snicoll snicoll added status: duplicate A duplicate of another issue and removed priority: normal status: on-hold We can't start working on this issue yet type: enhancement A general enhancement labels Sep 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate A duplicate of another issue
Projects
None yet
Development

No branches or pull requests

5 participants