From 72e1d27cea67bb45a81bbcf1a0e41201aef9cd64 Mon Sep 17 00:00:00 2001
From: Tobias Grieger <tobias.b.grieger@gmail.com>
Date: Mon, 13 Feb 2023 14:17:51 +0100
Subject: [PATCH] kvserver: log circuit breaker trip events at error severity

Informs https://github.com/cockroachdb/cockroach/issues/94691.

Epic: CRDB-23087
Release note: None
---
 pkg/kv/kvserver/replica_circuit_breaker.go | 13 +++--
 pkg/util/circuit/event_handler.go          | 62 ++++++++++++++++++----
 2 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/pkg/kv/kvserver/replica_circuit_breaker.go b/pkg/kv/kvserver/replica_circuit_breaker.go
index 22ade252ad7c..df280a231b2b 100644
--- a/pkg/kv/kvserver/replica_circuit_breaker.go
+++ b/pkg/kv/kvserver/replica_circuit_breaker.go
@@ -144,6 +144,7 @@ func newReplicaCircuitBreaker(
 		Name:       "breaker", // log bridge has ctx tags
 		AsyncProbe: br.asyncProbe,
 		EventHandler: &replicaCircuitBreakerLogger{
+			ambientCtx: ambientCtx,
 			EventHandler: &circuit.EventLogger{
 				Log: func(buf redact.StringBuilder) {
 					log.Infof(ambientCtx.AnnotateCtx(context.Background()), "%s", buf)
@@ -159,15 +160,19 @@ func newReplicaCircuitBreaker(
 
 type replicaCircuitBreakerLogger struct {
 	circuit.EventHandler
-	onTrip  func()
-	onReset func()
+	ambientCtx log.AmbientContext
+	onTrip     func()
+	onReset    func()
 }
 
-func (r replicaCircuitBreakerLogger) OnTrip(br *circuit.Breaker, prev, cur error) {
+func (r replicaCircuitBreakerLogger) OnTrip(b *circuit.Breaker, prev, cur error) {
 	if prev == nil {
 		r.onTrip()
 	}
-	r.EventHandler.OnTrip(br, prev, cur)
+	// Log directly from this method via log.Errorf.
+	var buf redact.StringBuilder
+	circuit.EventFormatter{}.OnTrip(b, prev, cur, &buf)
+	log.Errorf(r.ambientCtx.AnnotateCtx(context.Background()), "%s", buf)
 }
 
 func (r replicaCircuitBreakerLogger) OnReset(br *circuit.Breaker) {
diff --git a/pkg/util/circuit/event_handler.go b/pkg/util/circuit/event_handler.go
index 7d551409eff7..05b93c232be1 100644
--- a/pkg/util/circuit/event_handler.go
+++ b/pkg/util/circuit/event_handler.go
@@ -30,29 +30,73 @@ type EventLogger struct {
 	Log func(redact.StringBuilder)
 }
 
+func (d *EventLogger) maybeLog(buf redact.StringBuilder) {
+	if buf.Len() == 0 {
+		return
+	}
+	d.Log(buf)
+}
+
 // OnTrip implements EventHandler. If the previous error is nil, it logs the
 // error. If the previous error is not nil and has a different cause than the
 // current error, logs a message indicating the old and new error.
 func (d *EventLogger) OnTrip(b *Breaker, prev, cur error) {
 	var buf redact.StringBuilder
+	EventFormatter{}.OnTrip(b, prev, cur, &buf)
+	d.maybeLog(buf)
+}
+
+// OnProbeLaunched implements EventHandler. It is a no-op.
+func (d *EventLogger) OnProbeLaunched(b *Breaker) {
+	var buf redact.StringBuilder
+	EventFormatter{}.OnProbeLaunched(b, &buf)
+	d.maybeLog(buf)
+}
+
+// OnProbeDone implements EventHandler. It is a no-op.
+func (d *EventLogger) OnProbeDone(b *Breaker) {
+	var buf redact.StringBuilder
+	EventFormatter{}.OnProbeDone(b, &buf)
+	d.maybeLog(buf)
+}
+
+// OnReset implements EventHandler. It logs a message.
+func (d *EventLogger) OnReset(b *Breaker) {
+	var buf redact.StringBuilder
+	EventFormatter{}.OnReset(b, &buf)
+	d.maybeLog(buf)
+}
+
+// EventFormatter allows direct access to the translation of
+// EventHandler inputs to log line suitable for formatted output.
+// Its methods mirror the `EventHandler` interface, but with a
+// StringBuilder as the last argument, which will be written to.
+// Not all methods produce output. The caller should check if
+// anything was written to the buffer before logging it.
+type EventFormatter struct{}
+
+// OnTrip mirrors EventHandler.OnTrip.
+func (EventFormatter) OnTrip(b *Breaker, prev, cur error, buf *redact.StringBuilder) {
 	opts := b.Opts()
 	if prev != nil && !errors.Is(errors.Cause(prev), errors.Cause(cur)) {
 		buf.Printf("%s: now tripped with error: %s (previously: %s)", opts.Name, cur, prev)
 	} else {
 		buf.Printf("%s: tripped with error: %s", opts.Name, cur)
 	}
-	d.Log(buf)
+
 }
 
-// OnProbeLaunched implements EventHandler. It is a no-op.
-func (d *EventLogger) OnProbeLaunched(*Breaker) {}
+// OnProbeLaunched mirrors EventHandler.OnProbeLaunched.
+func (EventFormatter) OnProbeLaunched(*Breaker, *redact.StringBuilder) {
+	// No-op.
+}
 
-// OnProbeDone implements EventHandler. It is a no-op.
-func (d *EventLogger) OnProbeDone(*Breaker) {}
+// OnProbeDone mirrors EventHandler.OnProbeDone.
+func (EventFormatter) OnProbeDone(*Breaker, *redact.StringBuilder) {
+	// No-op.
+}
 
-// OnReset implements EventHandler. It logs a message.
-func (d *EventLogger) OnReset(b *Breaker) {
-	var buf redact.StringBuilder
+// OnReset mirrors EventHandler.OnReset.
+func (EventFormatter) OnReset(b *Breaker, buf *redact.StringBuilder) {
 	buf.Printf("%s: breaker reset", b.Opts().Name)
-	d.Log(buf)
 }