Ingester: split push and read circuit breakers #8315

duricanikolic · 2024-06-08T17:34:36Z

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.

Signed-off-by: Yuri Nikolic <[email protected]>

…it() Signed-off-by: Yuri Nikolic <[email protected]>

Signed-off-by: Yuri Nikolic <[email protected]>

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?

pkg/ingester/circuitbreaker.go

colega · 2024-06-11T11:18:05Z

pkg/ingester/circuitbreaker.go

 )

 type circuitBreakerMetrics struct {
 	circuitBreakerTransitions *prometheus.CounterVec
 	circuitBreakerResults     *prometheus.CounterVec
 }

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


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 {

I have removed all the occurrences of Fn in this source

colega · 2024-06-11T11:19:05Z

pkg/ingester/circuitbreaker.go

 	}

-	circuitBreakerTransitionsCounterFn := func(metrics *circuitBreakerMetrics, state circuitbreaker.State) prometheus.Counter {
-		return metrics.circuitBreakerTransitions.WithLabelValues(state.String())
+	circuitBreakerTransitionsCounterFn := func(metrics *circuitBreakerMetrics, requestType string, state circuitbreaker.State) prometheus.Counter {


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 · 2024-06-11T12:36:00Z

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 · 2024-06-11T12:44:07Z

pkg/ingester/circuitbreaker.go

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


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 · 2024-06-11T12:47:58Z

pkg/ingester/circuitbreaker.go

+	}
+	return func(duration time.Duration, err error) {
+		if pushAcquiredPermit {
+			_ = cb.push.finishRequest(duration, cb.push.cfg.RequestTimeout, err)


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.

A good catch. Thank you

colega · 2024-06-11T12:53:41Z

pkg/ingester/ingester.go

 	if err != nil {
 		return nil, err
 	}

 	finishReadRequest := func(err error) {
-		if acquiredCircuitBreakerPermit {
-			i.circuitBreaker.finishReadRequest(time.Since(start), err)
+		if finish != nil {


finish can't be nil, right?

You are right

colega · 2024-06-11T12:58:12Z

pkg/ingester/ingester.go

@@ -1004,8 +1006,8 @@ func (i *Ingester) FinishPushRequest(ctx context.Context) {
 	if st.requestSize > 0 {
 		i.inflightPushRequestsBytes.Sub(st.requestSize)
 	}
-	if st.acquiredCircuitBreakerPermit {
-		i.circuitBreaker.finishPushRequest(st.requestDuration, st.pushErr)
+	if st.requestFinish != nil {


Can this be nil?

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

Signed-off-by: Yuri Nikolic <[email protected]>

pkg/ingester/circuitbreaker.go

Signed-off-by: Yuri Nikolic <[email protected]>

pstibrany

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

pstibrany · 2024-06-11T14:27:07Z

pkg/ingester/circuitbreaker.go

+// 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) {


nit:do we need the comment here?

pstibrany · 2024-06-11T14:28:44Z

pkg/ingester/circuitbreaker.go

+// If it was not possible, the causing error is returned.
+func (cb *ingesterCircuitBreaker) tryReadAcquirePermit() (func(time.Duration, error), error) {
+	// If the read circuit breaker is not active, we don't try to acquire a permit.
+	if !cb.read.isActive() {


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

Yes, that's true

pstibrany · 2024-06-11T14:29:31Z

pkg/ingester/circuitbreaker.go

+// 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) {


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

colega · 2024-06-11T14:28:18Z

pkg/ingester/circuitbreaker.go

+// 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) {


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) {

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

pkg/ingester/errors_test.go

colega · 2024-06-11T14:31:42Z

pkg/ingester/ingester.go

@@ -356,7 +358,7 @@ type Ingester struct {
 	ingestPartitionID         int32
 	ingestPartitionLifecycler *ring.PartitionInstanceLifecycler

-	circuitBreaker *circuitBreaker
+	circuitBreaker *ingesterCircuitBreaker


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)

Signed-off-by: Yuri Nikolic <[email protected]>

* Ingester: splitting push and read circuit breakers Signed-off-by: Yuri Nikolic <[email protected]> * Improving TestIngester_StartReadRequest Signed-off-by: Yuri Nikolic <[email protected]> * Updating documentation Signed-off-by: Yuri Nikolic <[email protected]> * Fixing lint issues Signed-off-by: Yuri Nikolic <[email protected]> * Fixing documentation issues Signed-off-by: Yuri Nikolic <[email protected]> * Do not call tryAcquirePermit() on the push cb from tryReadAcquirePermit() Signed-off-by: Yuri Nikolic <[email protected]> * Rename prCircuitBreaker into ingesterCircuitBreaker Signed-off-by: Yuri Nikolic <[email protected]> * Rename label name path to request_type and label value write to push Signed-off-by: Yuri Nikolic <[email protected]> * Do not allow acquiring permit if cbs are not active Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> --------- Signed-off-by: Yuri Nikolic <[email protected]>

* Ingester: splitting push and read circuit breakers Signed-off-by: Yuri Nikolic <[email protected]> * Improving TestIngester_StartReadRequest Signed-off-by: Yuri Nikolic <[email protected]> * Updating documentation Signed-off-by: Yuri Nikolic <[email protected]> * Fixing lint issues Signed-off-by: Yuri Nikolic <[email protected]> * Fixing documentation issues Signed-off-by: Yuri Nikolic <[email protected]> * Do not call tryAcquirePermit() on the push cb from tryReadAcquirePermit() Signed-off-by: Yuri Nikolic <[email protected]> * Rename prCircuitBreaker into ingesterCircuitBreaker Signed-off-by: Yuri Nikolic <[email protected]> * Rename label name path to request_type and label value write to push Signed-off-by: Yuri Nikolic <[email protected]> * Do not allow acquiring permit if cbs are not active Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> --------- Signed-off-by: Yuri Nikolic <[email protected]> (cherry picked from commit ee069d2)

grafanabot · 2024-06-12T16:22:02Z

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.

* Ingester: splitting push and read circuit breakers Signed-off-by: Yuri Nikolic <[email protected]> * Improving TestIngester_StartReadRequest Signed-off-by: Yuri Nikolic <[email protected]> * Updating documentation Signed-off-by: Yuri Nikolic <[email protected]> * Fixing lint issues Signed-off-by: Yuri Nikolic <[email protected]> * Fixing documentation issues Signed-off-by: Yuri Nikolic <[email protected]> * Do not call tryAcquirePermit() on the push cb from tryReadAcquirePermit() Signed-off-by: Yuri Nikolic <[email protected]> * Rename prCircuitBreaker into ingesterCircuitBreaker Signed-off-by: Yuri Nikolic <[email protected]> * Rename label name path to request_type and label value write to push Signed-off-by: Yuri Nikolic <[email protected]> * Do not allow acquiring permit if cbs are not active Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> --------- Signed-off-by: Yuri Nikolic <[email protected]> (cherry picked from commit ee069d2)

* Ingester: splitting push and read circuit breakers Signed-off-by: Yuri Nikolic <[email protected]> * Improving TestIngester_StartReadRequest Signed-off-by: Yuri Nikolic <[email protected]> * Updating documentation Signed-off-by: Yuri Nikolic <[email protected]> * Fixing lint issues Signed-off-by: Yuri Nikolic <[email protected]> * Fixing documentation issues Signed-off-by: Yuri Nikolic <[email protected]> * Do not call tryAcquirePermit() on the push cb from tryReadAcquirePermit() Signed-off-by: Yuri Nikolic <[email protected]> * Rename prCircuitBreaker into ingesterCircuitBreaker Signed-off-by: Yuri Nikolic <[email protected]> * Rename label name path to request_type and label value write to push Signed-off-by: Yuri Nikolic <[email protected]> * Do not allow acquiring permit if cbs are not active Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> * Fixing review findings Signed-off-by: Yuri Nikolic <[email protected]> --------- Signed-off-by: Yuri Nikolic <[email protected]> (cherry picked from commit ee069d2) Co-authored-by: Đurica Yuri Nikolić <[email protected]>

duricanikolic added 2 commits June 8, 2024 19:30

Ingester: splitting push and read circuit breakers

a45c73e

Signed-off-by: Yuri Nikolic <[email protected]>

Improving TestIngester_StartReadRequest

9bdba4d

Signed-off-by: Yuri Nikolic <[email protected]>

duricanikolic self-assigned this Jun 8, 2024

duricanikolic requested review from jdbaldry and a team as code owners June 8, 2024 17:34

Updating documentation

e5bedfd

Signed-off-by: Yuri Nikolic <[email protected]>

duricanikolic force-pushed the yuri/ingester-server-cb branch from e3a12e2 to e5bedfd Compare June 8, 2024 17:35

duricanikolic added 5 commits June 8, 2024 19:55

Fixing lint issues

a7ccc1f

Signed-off-by: Yuri Nikolic <[email protected]>

Fixing documentation issues

d2686c6

Signed-off-by: Yuri Nikolic <[email protected]>

Do not call tryAcquirePermit() on the push cb from tryReadAcquirePerm…

b0d9cb5

…it() Signed-off-by: Yuri Nikolic <[email protected]>

Rename prCircuitBreaker into ingesterCircuitBreaker

2a002ef

Signed-off-by: Yuri Nikolic <[email protected]>

Rename label name path to request_type and label value write to push

887bd11

Signed-off-by: Yuri Nikolic <[email protected]>

duricanikolic force-pushed the yuri/ingester-server-cb branch from 3037b08 to 887bd11 Compare June 10, 2024 10:08

Do not allow acquiring permit if cbs are not active

bbb9a75

Signed-off-by: Yuri Nikolic <[email protected]>

pstibrany reviewed Jun 11, 2024

View reviewed changes

pkg/ingester/circuitbreaker.go Outdated Show resolved Hide resolved

pkg/ingester/circuitbreaker.go Outdated Show resolved Hide resolved

colega reviewed Jun 11, 2024

View reviewed changes

Fixing review findings

e20cdb4

Signed-off-by: Yuri Nikolic <[email protected]>

duricanikolic requested review from pstibrany and colega June 11, 2024 13:59

pstibrany reviewed Jun 11, 2024

View reviewed changes

pkg/ingester/circuitbreaker.go Outdated Show resolved Hide resolved

pkg/ingester/circuitbreaker.go Outdated Show resolved Hide resolved

pkg/ingester/circuitbreaker.go Outdated Show resolved Hide resolved

Fixing review findings

38d1ff9

Signed-off-by: Yuri Nikolic <[email protected]>

pstibrany reviewed Jun 11, 2024

View reviewed changes

colega reviewed Jun 11, 2024

View reviewed changes

Fixing review findings

c1bea5a

Signed-off-by: Yuri Nikolic <[email protected]>

colega approved these changes Jun 11, 2024

View reviewed changes

duricanikolic merged commit ee069d2 into main Jun 11, 2024
29 checks passed

duricanikolic deleted the yuri/ingester-server-cb branch June 11, 2024 15:15

duricanikolic added the backport r294 label Jun 12, 2024

grafanabot mentioned this pull request Jun 12, 2024

[r294] Ingester: split push and read circuit breakers #8356

Closed

duricanikolic added backport r294 and removed backport r294 labels Jun 12, 2024

grafanabot added the backport-failed label Jun 12, 2024

duricanikolic added backport r294 and removed backport-failed backport r294 labels Jun 12, 2024

grafanabot mentioned this pull request Jun 12, 2024

[r294] Ingester: split push and read circuit breakers #8358

Merged

duricanikolic mentioned this pull request Jun 13, 2024

Ingester: map circuitBreakerOpenError to codes.Unavailable #8362

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ingester: split push and read circuit breakers #8315

Ingester: split push and read circuit breakers #8315

duricanikolic commented Jun 8, 2024 •

edited

Loading

pstibrany left a comment

colega Jun 11, 2024

duricanikolic Jun 11, 2024

colega Jun 11, 2024

duricanikolic commented Jun 11, 2024

colega Jun 11, 2024

colega Jun 11, 2024

duricanikolic Jun 11, 2024

colega Jun 11, 2024

duricanikolic Jun 11, 2024

colega Jun 11, 2024

duricanikolic Jun 11, 2024

pstibrany left a comment

pstibrany Jun 11, 2024

pstibrany Jun 11, 2024

duricanikolic Jun 11, 2024

pstibrany Jun 11, 2024

colega Jun 11, 2024

duricanikolic Jun 11, 2024

colega Jun 11, 2024

grafanabot commented Jun 12, 2024

	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 {

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

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

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

Ingester: split push and read circuit breakers #8315

Ingester: split push and read circuit breakers #8315

Conversation

duricanikolic commented Jun 8, 2024 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duricanikolic commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafanabot commented Jun 12, 2024

duricanikolic commented Jun 8, 2024 •

edited

Loading