Ingester: split push and read circuit breakers

[duricanikolic]

What this PR does

In #8180 and #8285 we introduced an ingester server-side circuit breaker for both push and read requests together. This PR splits the introduced circuit breaker into 2 different circuit breakers: one for push and the other for read requests only.

They can be enabled by -ingester.push-circuit-breaker.enabled and -ingester.read-circuit-breaker.enabled configuration options. Other configuration options are:
- -ingester.push-circuit-breaker.failure-threshold-percentage
- -ingester.push-circuit-breaker.failure-execution-threshold
- -ingester.push-circuit-breaker.thresholding-period
- -ingester.push-circuit-breaker.cooldown-period
- -ingester.push-circuit-breaker.initial-delay
- -ingester.push-circuit-breaker.request-timeout
- -ingester.read-circuit-breaker.failure-threshold-percentage
- -ingester.read-circuit-breaker.failure-execution-threshold
- -ingester.read-circuit-breaker.thresholding-period
- -ingester.read-circuit-breaker.cooldown-period
- -ingester.read-circuit-breaker.initial-delay
- -ingester.read-circuit-breaker.request-timeout

The following experimental configuration options introduced in #8180 and #8285 have been removed:
- -ingester.push-circuit-breaker.enabled
- -ingester.push-circuit-breaker.failure-threshold-percentage
- -ingester.push-circuit-breaker.failure-execution-threshold
- -ingester.push-circuit-breaker.thresholding-period
- -ingester.push-circuit-breaker.cooldown-period
- -ingester.push-circuit-breaker.initial-delay
- -ingester.push-circuit-breaker.push-timeout
- -ingester.push-circuit-breaker.read-timeout

Which issue(s) this PR fixes or relates to

Fixes #

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]

I don't see much value for adding *ingesterCircuitBreaker, it makes everything more cumbersome.

I’d suggest to drop ingesterCircuitBreaker type, and simply use two *circuitBreaker implementations directly from ingester. If you want to use callback to release permit, then do it at (cb *circuitBreaker) tryAcquirePermit() method, not in (cb *ingesterCircuitBreaker) tryPushAcquirePermit().

I’d also make newCircuitBreakerMetrics work with single circuit breaker only (by adding ConstLabel request_type to all metrics — we can register multiple metrics with same name/help/labels, but different const label. example.

WDYT?

[colega]

Nit:

Suggested change

	func newCircuitBreakerMetrics(r prometheus.Registerer, currentStateFn func(string) circuitbreaker.State, requestTypes []string) *circuitBreakerMetrics {
	func newCircuitBreakerMetrics(r prometheus.Registerer, currentState func(string) circuitbreaker.State, requestTypes []string) *circuitBreakerMetrics {

[duricanikolic]

I have removed all the occurrences of Fn in this source

[colega]

Suggested change

	circuitBreakerTransitionsCounterFn := func(metrics *circuitBreakerMetrics, requestType string, state circuitbreaker.State) prometheus.Counter {
	circuitBreakerTransitionsCounter := func(metrics *circuitBreakerMetrics, requestType string, state circuitbreaker.State) prometheus.Counter {

[duricanikolic]

I don't see much value for adding *ingesterCircuitBreaker, it makes everything more cumbersome.

I’d suggest to drop ingesterCircuitBreaker type, and simply use two *circuitBreaker implementations directly from ingester. If you want to use callback to release permit, then do it at (cb *circuitBreaker) tryAcquirePermit() method, not in (cb *ingesterCircuitBreaker) tryPushAcquirePermit().

I’d also make newCircuitBreakerMetrics work with single circuit breaker only (by adding ConstLabel request_type to all metrics — we can register multiple metrics with same name/help/labels, but different const label. example.

WDYT?

In a slack conversation @pstibrany and I agreed to keep the ingesterCircuitBreaker struct and to get rid of some checks.

[colega]

Suggested change

	circuitBreakerCurrentStateGaugeFn := func(requestType string, state circuitbreaker.State) prometheus.GaugeFunc {
	circuitBreakerCurrentStateGauge := func(requestType string, state circuitbreaker.State) prometheus.GaugeFunc {

or

Suggested change

	circuitBreakerCurrentStateGaugeFn := func(requestType string, state circuitbreaker.State) prometheus.GaugeFunc {
	circuitBreakerCurrentStateGaugeFunc := func(requestType string, state circuitbreaker.State) prometheus.GaugeFunc {

If we want to be consistent with the type name.

[colega]

Why do we need to pass to cb.push.foo() a config from cb.push.cfg?
I think we can refactor finishRequest to read it's own config.

[duricanikolic]

A good catch. Thank you

[colega]

finish can't be nil, right?

[duricanikolic]

You are right

[colega]

Can this be nil?

[duricanikolic]

You are, again, right. It cannot be nil.

[pstibrany]

haven't checked tests yet, but code changes look good to me.

[pstibrany]

nit:do we need the comment here?

[pstibrany]

nit: this is now checked twice, right? once here, second time in tryAcquirePermit.

[duricanikolic]

Yes, that's true

[pstibrany]

can we simplify comment and just say that it's also checking push CB (but doesn't get permit)

[colega]

Suggested change

	// tryPushAcquirePermit tries to acquire a permit to use the push circuit breaker and returns whether a permit was acquired.
	// If it was possible, tryPushAcquirePermit returns a function that should be called to release the acquired permit.
	// If it was not possible, the causing error is returned.
	func (cb *ingesterCircuitBreaker) tryPushAcquirePermit() (func(time.Duration, error), error) {
	return cb.push.tryAcquirePermit()
	}
	// tryReadAcquirePermit tries to acquire a permit to use the read circuit breaker and returns whether a permit was acquired.
	// If it was possible, tryReadAcquirePermit returns a function that should be called to release the acquired permit.
	// If it was not possible, the causing error is returned.
	func (cb *ingesterCircuitBreaker) tryReadAcquirePermit() (func(time.Duration, error), error) {
	// tryAcquirePushPermit tries to acquire a permit to use the push circuit breaker and returns whether a permit was acquired.
	// If it was possible, tryAcquirePushPermit returns a function that should be called to release the acquired permit.
	// If it was not possible, the causing error is returned.
	func (cb *ingesterCircuitBreaker) tryAcquirePushPermit() (func(time.Duration, error), error) {
	return cb.push.tryAcquirePermit()
	}
	// tryAcquireReadPermit tries to acquire a permit to use the read circuit breaker and returns whether a permit was acquired.
	// If it was possible, tryAcquireReadPermit returns a function that should be called to release the acquired permit.
	// If it was not possible, the causing error is returned.
	func (cb *ingesterCircuitBreaker) tryAcquireReadPermit() (func(time.Duration, error), error) {

[duricanikolic]

I have changed the method names. They actually sound better.

[colega]

There's no need for this to be a pointer, and no need to return a pointer from newIngesterCircuitBreaker: if we just hold a ingesterCircuitBreaker struct here, we'll also handle the zero-valued case gracefully (as nil cb's inside of ingesterCircuitBreaker are already valid)

[grafanabot]

The backport to r294 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-8315-to-r294 origin/r294
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x ee069d247c7c22848ee668027c432a97e120cd9f
# Push it to GitHub
git push --set-upstream origin backport-8315-to-r294
git switch main
# Remove the local backport branch
git branch -D backport-8315-to-r294

Then, create a pull request where the base branch is r294 and the compare/head branch is backport-8315-to-r294.