Adding circuit breakers on ingester server side for write path

[duricanikolic]

What this PR does

Similarly to what was done in #5650, when circuit breakers were added to ingester client side, this PR adds circuit breakers to the server side.

Which issue(s) this PR fixes or relates to

Checklist

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

[pstibrany]

Thank you Yuri. I haven't reviewed tests yet, but I have enough comments to send this first batch now.

[pstibrany]

In internal discussion with Yuri, I proposed to change the direction of this PR, and use "Standalone Usage" of circuit breakers: try to acquire permit from circuit breaker in StartPushRequest, and then report success/failure after push request. To detect push request running for too long, we don't need context, just take timing of the push request, and report failure to CB if it's over specified timeout. I think by not using executor-pattern we can simplify the code quite a bit.

[pstibrany]

Haven't finished my review yet, because I've run out of time. Will continue tomorrow.

[pstibrany]

Next batch of comments.

[pstibrany]

It would be fine to just call i.FinishPushRequest. For that we need to preserve ctx returned by startPushRequest, but we need to do that anyway if we want to update pushRequestState in the defer statement below.

[duricanikolic]

I have applied your suggestion. The only thing to be aware of was if a call to this PushWithCleanup() actually acquired a permit (during a call to i.startPushRequest()). In that case, the value of pushRequestState.acquiredCircuitBreakerPermit also has to be updated in the defer statement below.

[duricanikolic]

BTW, since in the previous version of this implementation was:

_, shouldFinish, err := i.startPushRequest(ctx, reqSize)
if err != nil {
	return middleware.DoNotLogError{Err: err}
}
if shouldFinish {
	defer i.finishPushRequest(reqSize)
}

does it mean that we had a bug?

[colega]

I'd rename this to:

Suggested change

	func (cb *circuitBreaker) setActive() {
	func (cb *circuitBreaker) activate() {

I would expect setActive to receive an active bool arg to set active property to a desired value, but this method only activates the circuit breaker.

[colega]

We can make this shorter:

Suggested change

	if cb == nil {
	return false
	}
	return cb.active.Load()
	return cb != nil && cb.active.Load()

[colega]

Suggested change

	type circuitBreaker struct {
	// circuitBreaker abstracts the ingester's server-side circuit breaker functionality.
	// A nil *circuitBreaker is a valid noop implementation.
	type circuitBreaker struct {

[colega]

I would expect that if tryAcquirePermit returns false, nil, then I don't have permit to proceed. However, it's not like that.

I would try to refactor the method to either avoid having to read the comment to understand the return values (what's the first return value? prod code calls it "acquired" while tests call it "status") or at least to force the user to think about it.

The comment above says:

If it was not possible to acquire a permit, success flag false is returned. In this case no call to finishPushRequest is needed.

Which puts logic in the caller of this method who shouldn't care whether it's active or not, and I think we can do that better by just returning a callback:

Suggested change

	func (cb *circuitBreaker) tryAcquirePermit() (bool, error) {
	if !cb.isActive() {
	return false, nil
	}
	if !cb.cb.TryAcquirePermit() {
	cb.metrics.circuitBreakerResults.WithLabelValues(resultOpen).Inc()
	return false, newCircuitBreakerOpenError(cb.cb.RemainingDelay())
	}
	return true, nil
	}
	func (cb *circuitBreaker) tryAcquirePermit() (finish func(duration time.Duration, pushErr error), err error)
	if !cb.isActive() {
	return func(_ time.Duration, _ error,) {}, nil
	}
	if !cb.cb.TryAcquirePermit() {
	cb.metrics.circuitBreakerResults.WithLabelValues(resultOpen).Inc()
	return nil, newCircuitBreakerOpenError(cb.cb.RemainingDelay())
	}
	return cb.finishPushRequest, nil
	}

Then the caller doesn't have more logic than just:

finish, err := cb.tryAquirePermit()
if err != nil {
    return ..., err
}
// ...
finish(time.Since(t0), err)

[pstibrany]

While I understand your rationale (client just calls returned function), it looks like slightly over-engineered solution to a trivial problem, where a simple boolean works just fine.

[duricanikolic]

I would also keep the current implementation as it is.

[colega]

it looks like slightly over-engineered solution

Closures are first class citizens just like bools :)

to a trivial problem

The difference is that it removes the problem of having to decide things from the caller.

[colega]

Nit: can you move this method below finishPushRequest()?

[colega]

Given you're passing the entire cfg.CircuitBreakerConfig to newCircuitBreaker, and nil instance of *circuitBreaker is valid, can we move this logic to newCircuitBreaker?

Otherwise this method is taking decisions based on that entity's config and if it's false, it relies on that entity's nil behaviour.

[duricanikolic]

I have moved the logic in newCircuitBreaker().

[colega]

Same here, IMO, this logic should be inside of newCircuitBreaker.
This method shouldn't read circuit breaker's config and take decisions on it, it should just call i.circuitBreaker.onStarting() or something similar.

[pstibrany]

I don't think it should be inside of newCircuitBreaker specifically (how would we know when ingester finished starting?), but moving it to circuit breaker method sounds fine.

[colega]

Oh, yes, sorry, I wrote half comment and then realised that this wasn't part of newIngester anymore. I agree, it shouldn't be in newCircuitBreaker, but should be in a method of circuit breaker (as I stated in the second part of my comment 😂 )

[duricanikolic]

I have moved the logic in the circuitBreaker.activate() method: if cfg.InitialDelay is 0, the circuit breaker gets activated immediately. Otherwise it gets activated after the initial delay.

Then, ingester.startig() will just call i.circuitbreaker.activate(), which will activate the circuit breaker in the right moment. WDYT @colega?

[pstibrany]

Great job! Thank you very much for addressing all my feedback!