Maintain overall max request timeout (Optional)

[karthikmuralidharan]

Health Checks are called frequently, hence latency should ideally be low. Since most checks are to dependencies over the network, we need to make sure to timeout if the calls are not served within time.

While it's the responsibility of each checker func to maintain it's own TTL, having a max timeout value for a health check request might be a good idea.

This could be an optional feature.

[samnegri]

Hi, I'd like to work on this issue 😄

[karthikmuralidharan]

@samnegri Sure, go ahead.

[karthikmuralidharan]

@samnegri Might be good to use context with deadline. Thoughts ?

https://golang.org/pkg/context/#WithDeadline

[samnegri]

Seems good! I'll check it out after I leave work

[pcasaretto]

Would it make sense for checkers to accept a context.Context ?

[karthikmuralidharan]

@pcasaretto I think so and I had the same thought as you did, but context is passed into checkers are mostly for request cancellation on the source (like cancelling the database query, close the tcp connection etc..).

Since checker funcs shouldn't be IO intensive in any way, I thought of leaving it out for sake of not breaking the interface. I'm open to suggestions though.

[pcasaretto]

I see no other way of signaling cancellation. I'd suggest we break the interface while you haven't publicly version 1.
In terms of implementation, what should happen with checks that timeout?

[karthikmuralidharan]

@pcasaretto Apologies for the delayed response.

I see no other way of signaling cancellation. I'd suggest we break the interface while you haven't publicly version 1.

Yes. I agree. If you'd like to send a cancellation signal to the underlying checker implementation in order to clear up the resources, then I can't think of a cleaner approach than sending in the context object.

If the goal is to simply return/ignore the results of the checker once the timeout has occurred, that can be done without the need for context. But that means resources will be locked up indefinitely unless the checker implementation has it's own timeout.

Since this project's pretty new, I think we can break the interface in favour of the cleaner approach.

In terms of implementation, what should happen with checks that timeout?
In case the timeout is exceeded, and let's say the database check was still pending, we'd have to signal the cancellation and return to handling the error responses.

Because certain implementations (custom like database) may choose not to handle context, I'm thinking we can't wait for the functions to return I suppose (Need to play around with the behaviour of context a little more personally :) ).

We append to the errors map of the pending checkers saying max check time exceeded.

What do you think ?

[pcasaretto]

Sounds good to me!
Expect a PR soon.