Skip to content

Improve AbstractHealthAggregator to allow customization of aggregated details #4674

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

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Dec 4, 2015

ATM AbstractHealthAggregator allows customization of how aggregated status is calculated, but does not allow users to specify their own representation of aggregated details. And due to aggregate method being declared as final there is no elegant way of making such customization.

This PR adds abstract method to allow the subclasses to define how aggregated details are constructed.

Motivation:
In our current project we integrate with a proprietary external system. To monitor the connection(s) to this system we have implemented custom HealthIndicator. Usually there are multiple instances of this system, with number of instances being determined in runtime. To represent the health of connection we use CompositeHealthIndicator which is composed of multiple instances of our custom HealthIndicator which, by default (OrderedHealthAggregator is being used), results in the following health output:

{
    // other health checks omitted for brevity
    "custom": {
        "custom_1": {
            "status": "UP", 
            ...
        }, 
        "custom_2": {
            "status": "UP", 
            ...
        }, 
        "custom_3": {
            "status": "UP", 
            ...
        }, 
        "status": "UP"
    }, 
    "status": "UP"
}

However, due to dynamic nature of number of nested HealthIndicators the desired representation of details would be an array:

{
    // other health checks omitted for brevity
    "custom": {
        "nodes": [
            {
                "status": "UP", 
                ...
            }, 
            {
                "status": "UP", 
                ...
            }, 
            {
                "status": "UP", 
                ...
            }
        ], 
        "status": "UP"
    }, 
    "status": "UP"
}

I've signed the CLA.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 4, 2015
@philwebb philwebb added this to the 1.3.1 milestone Dec 9, 2015
@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 9, 2015
@philwebb philwebb closed this in f8090d9 Dec 11, 2015
philwebb added a commit that referenced this pull request Dec 11, 2015
* pr/4674:
  Add AbstractHealthAggregator.aggregateDetails
@philwebb
Copy link
Member

I tweaked the code a little so that the aggregateDetails method is implemented in AbstractHealthAggregator. It's protected, so you can still override it but I wanted to make sure existing subclasses didn't break on the upgrade.

Thanks for the suggestion and the pull-request!

@vpavic
Copy link
Contributor Author

vpavic commented Dec 11, 2015

Great, thanks Phil! 👍

The idea of moving aggregateDetails implementation into AbstractHealthAggregator came across my mind a day or two after creating the PR but I forgot to push the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants