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

Making HealthCheck.Result class extensible #663

Open
hmpatel opened this Issue Sep 16, 2014 · 15 comments

Comments

Projects
None yet
@hmpatel

hmpatel commented Sep 16, 2014

I want to extend the Result class to enhance the health checks module for providing additional details. For e.g. I would add another health state in there - MAINTENANCE to indicate my servers are in the maintenance mode and also provider an optional placeholder Map to provide any additional details. Currently, Result class have only healthy and unhealthy states and furthermore, there is no way to provide additional info other than message.

So, I tried to extend the HealthCheck class and add these details in there. However, due to absence of default no arg constructor in Result class, it's not possible to subclass Result.

So would like to propose this change or get the feedback.

Thanks!

@manikantag

This comment has been minimized.

Show comment
Hide comment
@manikantag

manikantag Oct 1, 2014

+100 for additional data.

In our case, we've multiple AD servers, which user can configure. So, our ADHealthCheckService will return single Result object, but with all AD server's status.

We can't extend as the Result class constructor is private and I've written my own equivalent classes for HealthCheck. Basically, I m not able to use the metric's HealthCheck classes.

manikantag commented Oct 1, 2014

+100 for additional data.

In our case, we've multiple AD servers, which user can configure. So, our ADHealthCheckService will return single Result object, but with all AD server's status.

We can't extend as the Result class constructor is private and I've written my own equivalent classes for HealthCheck. Basically, I m not able to use the metric's HealthCheck classes.

@manikantag

This comment has been minimized.

Show comment
Hide comment
@manikantag

manikantag Oct 1, 2014

By making Result constructor public, doors will be open for extensions.

manikantag commented Oct 1, 2014

By making Result constructor public, doors will be open for extensions.

@ryantenney ryantenney added this to the 4.0.0 milestone Oct 8, 2014

@ryantenney

This comment has been minimized.

Show comment
Hide comment
@ryantenney

ryantenney Oct 8, 2014

Member

Changes like this will have to wait for v4. I'll keep this open as a reminder to look into it at that time.

Member

ryantenney commented Oct 8, 2014

Changes like this will have to wait for v4. I'll keep this open as a reminder to look into it at that time.

@manikantag

This comment has been minimized.

Show comment
Hide comment
@manikantag

manikantag Oct 8, 2014

Thanks. When can we expect v4?

manikantag commented Oct 8, 2014

Thanks. When can we expect v4?

@tomaszalusky

This comment has been minimized.

Show comment
Hide comment
@tomaszalusky

tomaszalusky Dec 4, 2014

My +1 for extending Result. It would be handy for reporting more data or producing precise hints.
For instance: monitoring free space on drive, Result could report (1) how much free space is actually available, (2) how much space is enough to release to make probe healthy.

tomaszalusky commented Dec 4, 2014

My +1 for extending Result. It would be handy for reporting more data or producing precise hints.
For instance: monitoring free space on drive, Result could report (1) how much free space is actually available, (2) how much space is enough to release to make probe healthy.

@JonMR

This comment has been minimized.

Show comment
Hide comment
@JonMR

JonMR Feb 8, 2015

I'd also like to see this happen. I'd be open to creating a PR as well.

In our case, the two states of healthy and unhealthy aren't enough. For us, unhealthy means drop the service from the pool. I'd like a third state that indicates there's an issue but the service is still able to handle traffic.

Specific example: We have a pool of memcache servers that the service talks to. If it loses connectivity with one of those servers, it's not a big deal, but something to be alerted and investigated.

We could write a parser for the output, but I'd rather not have to do that for each case.

JonMR commented Feb 8, 2015

I'd also like to see this happen. I'd be open to creating a PR as well.

In our case, the two states of healthy and unhealthy aren't enough. For us, unhealthy means drop the service from the pool. I'd like a third state that indicates there's an issue but the service is still able to handle traffic.

Specific example: We have a pool of memcache servers that the service talks to. If it loses connectivity with one of those servers, it's not a big deal, but something to be alerted and investigated.

We could write a parser for the output, but I'd rather not have to do that for each case.

@marcvanandel

This comment has been minimized.

Show comment
Hide comment
@marcvanandel

marcvanandel May 6, 2015

+1 yes, please! It would be great if we can add addition (JSON) serializable objects to the Result to expose extra details. This would especially be great for a single check on multiple objects.

marcvanandel commented May 6, 2015

+1 yes, please! It would be great if we can add addition (JSON) serializable objects to the Result to expose extra details. This would especially be great for a single check on multiple objects.

@arnaud-deprez

This comment has been minimized.

Show comment
Hide comment
@arnaud-deprez

arnaud-deprez Apr 6, 2016

+1 too ! :-)

arnaud-deprez commented Apr 6, 2016

+1 too ! :-)

@zaphinath

This comment has been minimized.

Show comment
Hide comment
@zaphinath

zaphinath commented Apr 6, 2016

+1

@arnaud-deprez

This comment has been minimized.

Show comment
Hide comment
@arnaud-deprez

arnaud-deprez Apr 7, 2016

I think it would be possible to allow custom details by adding a Map<String,Object> as spring-boot-actuator does without breaking the API.
Like that, we wouldn't be forced to wait for version 4.0.0.
What do you think ?

arnaud-deprez commented Apr 7, 2016

I think it would be possible to allow custom details by adding a Map<String,Object> as spring-boot-actuator does without breaking the API.
Like that, we wouldn't be forced to wait for version 4.0.0.
What do you think ?

@ryantenney

This comment has been minimized.

Show comment
Hide comment
@ryantenney

ryantenney Apr 7, 2016

Member

Branch off 3.1-maintenance and I'd be happy to look at a pull request that adds that support for a 3.2 release.

Member

ryantenney commented Apr 7, 2016

Branch off 3.1-maintenance and I'd be happy to look at a pull request that adds that support for a 3.2 release.

@arnaud-deprez

This comment has been minimized.

Show comment
Hide comment
@arnaud-deprez

arnaud-deprez Apr 7, 2016

Okay, I'll take a look asap. Keep in touch.
So 3.2 is in master ?

arnaud-deprez commented Apr 7, 2016

Okay, I'll take a look asap. Keep in touch.
So 3.2 is in master ?

@arnaud-deprez

This comment has been minimized.

Show comment
Hide comment
@arnaud-deprez

arnaud-deprez Apr 8, 2016

So if I get it right, master is for 4.0.0.
I suppose I need to base my code on 3.1-maintenance and make a pull request on a new branch. Correct ?

arnaud-deprez commented Apr 8, 2016

So if I get it right, master is for 4.0.0.
I suppose I need to base my code on 3.1-maintenance and make a pull request on a new branch. Correct ?

@dhay

This comment has been minimized.

Show comment
Hide comment
@dhay

dhay May 16, 2016

Our use case is this: We use the health check framework to inform our on-call alerting. As is, we basically have two states: UP or DOWN. We wanted a third state, WARN, to indicate a problem with our application, but not worthy of waking someone up in the middle of the night.

So being able to have additional HealthCheck result types would be very helpful.

dhay commented May 16, 2016

Our use case is this: We use the health check framework to inform our on-call alerting. As is, we basically have two states: UP or DOWN. We wanted a third state, WARN, to indicate a problem with our application, but not worthy of waking someone up in the middle of the night.

So being able to have additional HealthCheck result types would be very helpful.

ryantenney added a commit that referenced this issue Jun 14, 2016

Merge pull request #943 from arnaud-deprez/metrics-663
METRICS-#663: add support for custom details in HealthCheck.Result
@cdeszaq

This comment has been minimized.

Show comment
Hide comment
@cdeszaq

cdeszaq Apr 12, 2017

This looks like changes for this was merged into 3.2.0 in #943, so is this issue complete?

cdeszaq commented Apr 12, 2017

This looks like changes for this was merged into 3.2.0 in #943, so is this issue complete?

@arteam arteam modified the milestones: 5.0.0, 4.0.0 Aug 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment