Add optional circuit breaker to ingester client

[56quarters]

What this PR does

Add a circuit breaker when making gRPC requests to ingesters to avoid making
the request if the ingester is down or hitting per-instance limits. All other
errors are ignored by the circuit and result in the usual behaivor.

Which issue(s) this PR fixes or relates to

N/A

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[56quarters]

Note from testing: this wraps the HealthClient methods with a circuit breaker too which is probably not what we want.

ts=2023-08-04T19:51:10.560584491Z caller=pool.go:196 level=warn msg="removing ingester failing healthcheck" addr=XXX:9095 reason="circuit breaker is open"

[pracucci]

The circuit breaker interceptor knows the method too. What if configure the circuit breaker to only act on a list of configured methods, so that we're explicit on which gRPC methods we want the circuit breaker to apply?

[56quarters]

What do you think about only applying the interceptor to methods that contain cortex.Ingester? This would avoid the need to keep a list of methods up to date and allow non-ingester things like the health check to bypass it. Or would you prefer an explicit list of methods?

[56quarters]

I've added a list of methods to circuit-break in e312654, LMK what you think.

[56quarters]

Note that I've marked this functionality as experimental because we really do need to experiment with this at scale before enabling it by default. It's also possible the implementation will change completely (@jhalterman has some ideas about what this behavior would look like in an ideal world). However, I think it's at a point where it makes sense to include it in main.

[pracucci]

Good job! LGTM. I left few comments I would be glad if you could look at before merging, thanks!

[pracucci]

The circuit breaker interceptor knows the method too. What if configure the circuit breaker to only act on a list of configured methods, so that we're explicit on which gRPC methods we want the circuit breaker to apply?

[jhalterman]

This is nice overall. @56quarters and I talked about this - we should consider switching to a time based circuit breaker when one is available (hopefully soon). This would allow us to threshold off of recent failure % rather than consecutive failures (which is not great for dynamic systems).

That aside, I do have a few suggestions:

• Could we obscure some of the circuit breaker details, so that users don't need to have to know/care how that pattern works? A few things that could help with this:
  • We could rename circuit-breaker-max-consecutive-failures to circuit-breaker-failure-threshold, and use that same value for ReadyToTrip and MaxRequests (since both of these states are basically doing failure thresholding). That would allow us to remove the circuit-breaker-max-half-open-requests setting.
  • We could remove circuit-breaker-closed-interval and just use the default interval (0) since we're only thresholding off of consecutive failures, and the total number of successes/failures don't matter.
  • We should rename circuit-breaker-open-timeout to something like circuit-breaker-open-duration or circuit-breaker-half-open-delay since timeout implies the breaker might be open for <= that time when in practice it's = that time. Alternatively, we might consider calling this something like circuit-breaker-cooldown-period so that users don't have to know what open/half-open mean.

I'm also curious if you have an idea what settings we might use in prod?

[56quarters]

This is nice overall. @56quarters and I talked about this - we should consider switching to a time based circuit breaker when one is available (hopefully soon). This would allow us to threshold off of recent failure % rather than consecutive failures (which is not great for dynamic systems).

That aside, I do have a few suggestions:

• Could we obscure some of the circuit breaker details, so that users don't need to have to know/care how that pattern works? A few things that could help with this:

• We could rename circuit-breaker-max-consecutive-failures to circuit-breaker-failure-threshold, and use that same value for ReadyToTrip and MaxRequests (since both of these states are basically doing failure thresholding). That would allow us to remove the circuit-breaker-max-half-open-requests setting.

👍

• We could remove circuit-breaker-closed-interval and just use the default interval (0) since we're only thresholding off of consecutive failures, and the total number of successes/failures don't matter.

👍

• We should rename circuit-breaker-open-timeout to something like circuit-breaker-open-duration or circuit-breaker-half-open-delay since timeout implies the breaker might be open for <= that time when in practice it's = that time. Alternatively, we might consider calling this something like circuit-breaker-cooldown-period so that users don't have to know what open/half-open mean.

👍

I'm also curious if you have an idea what settings we might use in prod?

The defaults (10s cooldown, 10 consecutive failures) worked well in my testing. I'd like to make the defaults the appropriate settings for production to reduce the amount of tuning everyone has to do.

[jhalterman]

Nicely done 👍