NotDeadYetContributors / NotDeadYet

.NET application health checking made easy
72 stars 15 forks source link

Add nested healthcheck support. if a service has dependencies ,the Ch… #8

Closed guidebee closed 8 years ago

guidebee commented 8 years ago

Added nested HealthCheck result support. i.e if a service has dependency services which also require health check. ChildrenHealthCheckResults will contain detail children health check result.

{ "Status": "Okay", "Results": [ { "Status": "Okay", "ChildrenHealthCheckResults": null, "Name": "ApplicationIsRunning", "Description": "Checks whether the application is running. If this check can run then it should pass.", "ElapsedTime": "00:00:03.0078119", "IsParent": false }, { "Status": "Okay", "ChildrenHealthCheckResults": [ { "Status": "Okay", "ChildrenHealthCheckResults": null, "Name": "Success0", "Description": "Desc", "ElapsedTime": "00:00:00.0000001", "IsParent": false }, ... ... }

uglybugger commented 8 years ago

Thanks for this :) I'm not entirely sure I understand the purpose - what problem are we solving by having a hierarchy of health checks like this?

guidebee commented 8 years ago

a service may need dependent services to work properly.those services may have dependencies also. this forms service hierarchy. especially when using micro services design. so when do health check, it also need to do health check for those dependency services. need some way to report back the health check details to their parent. On 04/09/2016 4:48 PM, "Andrew Harcourt" notifications@github.com wrote:

Thanks for this :) I'm not entirely sure I understand the purpose - what problem are we solving by having a hierarchy of health checks like this?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/uglybugger/NotDeadYet/pull/8#issuecomment-244591114, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-CEjqgBMXJ_eplQqP5nBsF2Uo4686Iks5qmoX4gaJpZM4JVw5J .

uglybugger commented 8 years ago

Ahh, I see. Would that mean that the parent starts returning 503s when any of the services it depends upon start failing? What would you have the load balancer do in that situation?

guidebee commented 8 years ago

if any of the dependency service fail the parent will fail health check too,with the fail details report back. we will know what fail along the hierarchy. use load balancer or not depends on actual requirements On 04/09/2016 4:58 PM, "Andrew Harcourt" notifications@github.com wrote:

Ahh, I see. Would that mean that the parent starts returning 503s when any of the services it depends upon start failing? What would you have the load balancer do in that situation?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/uglybugger/NotDeadYet/pull/8#issuecomment-244591436, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-CEjU3khwDDR0kwlYyqOTAMKOFbuYGks5qmogXgaJpZM4JVw5J .

uglybugger commented 8 years ago

I'm not convinced that this is the best approach, either from an app ecosystem perspective or a software design one.

From an app ecosystem perspective, the behaviour of a load balancer is important. If a minor service failure causes an entire suite of applications to be progressively yanked from a load-balanced pool then that's probably not the intended result. A "warning" result rather than an outright failure will help here (#6) but we still need to be very careful to avoid unnecessary cascading failures.

From a software architecture perspective, could we solve this by writing a custom ServiceDependencyHealthCheck : IHealthCheck class which takes a list of dependencies and otherwise behaves like a normal IHealthCheck implementation? Having the health-checking infrastructure itself know about service dependencies a la the proposed approach seems to be a bit of overkill.

guidebee commented 8 years ago

the change mainly added the means for child service to report back its health check result. the actual implementation is left for developer. developer are free to choose how to design their healthcheck and the change is backward compatiable. On 04/09/2016 5:11 PM, "Andrew Harcourt" notifications@github.com wrote:

I'm not convinced that this is the best approach, either from an app ecosystem perspective or a software design one.

From an app ecosystem perspective, the behaviour of a load balancer is important. If a minor service failure causes an entire suite of applications to be progressively yanked from a load-balanced pool then that's probably not the intended result. A "warning" result rather than an outright failure will help here (#6 https://github.com/uglybugger/NotDeadYet/issues/6) but we still need to be very careful to avoid unnecessary cascading failures.

From a software architecture perspective, could we solve this by writing a custom ServiceDependencyHealthCheck : IHealthCheck class which takes a list of dependencies and otherwise behaves like a normal IHealthCheck implementation? Having the health-checking infrastructure itself know about service dependencies a la the proposed approach seems to be a bit of overkill.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/uglybugger/NotDeadYet/pull/8#issuecomment-244591885, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-CEo101_orfcFPSy-BAU09aVZx0PwCks5qmospgaJpZM4JVw5J .

uglybugger commented 8 years ago

I'm still not convinced that this feature is necessary in its current form. Service dependencies are relatively easy to test by creating a health check that interrogates them without having NotDeadYet itself have any knowledge of hierarchy. It would be different if we were trying to also create a dependency graph - which is not necessarily a bad idea, but also not addressed by this pull request and is a much larger issue.

I think the most sensible approach is to allow for a warning state (as per #6) and perhaps create a ServiceDependenciesHealthCheck or similar that assesses a list of services.