circuit: add probing-based circuit breaker

[tbg]

These are initially for use in #71806 but were originally conceived of
for #70485, which we are not currently prioritizing.

Importantly, this circuit breaker does not recruit a fraction of
requests to do the probing, which is desirable for both PR #71806 and
PR #70485; requests recruited as probes tend to incur high latency and
errors, and we don't want SQL client traffic to experience those.

Release note: None

[cockroach-teamcity]

This change is 

[erikgrinaker]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker and @tbg)

---

-- commits, line 5 at r1:
Consider a link to the relevant issue(s) too.

---

pkg/util/circuit/circuitbreaker.go, line 77 at r1 (raw file):

// Signal is low-overhead and thus suitable for use in hot code paths.
func (b *Breaker) Signal() (chan struct{}, func() error) {
	return b.errAndCh().ch, b.definitelyErr

If the caller has a need to see the tripping error, shouldn't we make sure to propagate the correct one?

---

pkg/util/circuit/circuitbreaker.go, line 91 at r1 (raw file):

// already tripped), the error that would be returned from subsequent calls to
// Err() is returned.
func (b *Breaker) Report(err error) error {

nit: maybe give the return value a name here, to clarify that, unusually, an error response indicates success.

Why do we even return the error here? Looks like there's only a single test call that checks it. Do we expect anyone to care about it?

---

pkg/util/circuit/circuitbreaker.go, line 106 at r1 (raw file):

	}
	opts := b.Opts()
	err = opts.ShouldTrip(err)

nit: let's allow this to be unspecified too.

---

pkg/util/circuit/circuitbreaker.go, line 220 at r1 (raw file):

// from Breaker.Err(), i.e. `errors.Is(err, ErrBreakerOpen()) can be
// used to check whether an error originated from some Breaker.
func ErrBreakerOpen() error {

nit: why is this a function, rather than exporting errBreakerOpen?

---

pkg/util/circuit/circuitbreaker.go, line 224 at r1 (raw file):

}

type errAndCh struct {

nit: since we now protect this with a mutex, I don't think there's any point in having this type anymore? We can just use separate fields in Breaker.

---

pkg/util/circuit/options.go, line 18 at r1 (raw file):

type Options struct {
	// Name is the name of a Breaker and will be mentioned in the errors
	// that particular Breaker generates.

nit: "that a particular"

---

pkg/util/circuit/options.go, line 24 at r3 (raw file):

	// `Breaker.Report`. In particular, it can return nil to ignore certain
	// errors.
	ShouldTrip func(err error) error

nit: "should" implies a boolean response, and the error semantics here are unusual. Consider clarifying the name, or using two separate functions: one to decide whether to trip the breaker, and one to annotate the error.

[tbg]

Thanks for the review! I made changes that I would call substantial and I'm pretty happy with the result.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)

---

pkg/util/circuit/circuitbreaker.go, line 91 at r1 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

nit: maybe give the return value a name here, to clarify that, unusually, an error response indicates success.

Why do we even return the error here? Looks like there's only a single test call that checks it. Do we expect anyone to care about it?

The original motivation for returning the error was so that the caller could log the annotated error. But this is sort of going the wrong way, we don't want to make it look like that caller failed on the breaker, so it is better to log the un-wrapped error. Removed the error return which also means the linter won't yell at us for not handling it.

---

pkg/util/circuit/circuitbreaker.go, line 220 at r1 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

nit: why is this a function, rather than exporting errBreakerOpen?

So that you can't do circuit.ErrBreakerOpen = something. I realize there's plenty of precedent for ignoring that "problem". Switched to var ErrBreakerOpen.

---

pkg/util/circuit/circuitbreaker.go, line 224 at r1 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

nit: since we now protect this with a mutex, I don't think there's any point in having this type anymore? We can just use separate fields in Breaker.

This was initially left-over from the unsafe.Pointer approach, but in the meantime I have removed this type and found that I needed to re-add it as it seems like the best way to get error stability from Signal() as well as avoiding allocations in that path.

---

pkg/util/circuit/options.go, line 24 at r3 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

nit: "should" implies a boolean response, and the error semantics here are unusual. Consider clarifying the name, or using two separate functions: one to decide whether to trip the breaker, and one to annotate the error.

I removed this option altogether, as I don't anticipate a need for it (and if we ever do, easy enough to add back).

[erikgrinaker]

Nice, I really like the changes!

Reviewed 30 of 30 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/util/circuit/circuitbreaker.go, line 114 at r4 (raw file):

	// We get to write the error since we have exclusive access via b.mu.
	b.mu.errAndCh.err = storeErr
	close(b.mu.errAndCh.ch)

I had to read this a couple of times before I got why this closes the initial channel on the first error. Maybe add a comment clarifying how only on the nil -> !nil transition do we close the channel that actually has any waiters. After that, we close it pre-emptively for later callers.

---

pkg/util/circuit/circuitbreaker.go, line 115 at r4 (raw file):

	b.mu.errAndCh.err = storeErr
	close(b.mu.errAndCh.ch)
	b.mu.Unlock()

nano-optimization (?): we can unlock before closing.

---

pkg/util/circuit/circuitbreaker.go, line 213 at r4 (raw file):

	select {
	case <-eac.ch:
		eac.maybeTriggerProbe()

Why are we triggering the probe here?

[erikgrinaker]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tbg)

---

pkg/util/circuit/circuitbreaker.go, line 115 at r4 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

nano-optimization (?): we can unlock before closing.

Actually, we clearly can't without another reference to the channel. Doesn't seem worth it, nevermind.

[tbg]

TFTR! Will merge on green (tomorrow).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)

---

pkg/util/circuit/circuitbreaker.go, line 213 at r4 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

Why are we triggering the probe here?

I wrote more comments about how this works in general. Basically, when a probe has returned and failed to heal the breaker, we're going to launch the next one only reactively, after the next client bounces off the circuit breaker (which means a call to Err() that returns non-nil). I think this behavior makes sense (if a probe wants to keep trying for 10 minutes, it can do that rather easily), added a test for it as well.

[tbg]

... or today..

bors r=erikgrinaker

[craig]

Build succeeded:

• GitHub CI (Cockroach)