tracing: make full redaction of trace messages configurable

When we discovered that tenants could "trace into" KV, we considered this a security problem since no mechanism ensured that tenants could receive data they are allowed to see. The KV team reacted to this with cockroachdb#70562, which makes all verbose traces *redactable* and by redacting them at the KV-Tenant boundary. Serverless has shipped with this redactability in place in order to prevent data leakage across the boundary with KV and now needs to get back on the `release-21.2` branch to sync with the rest of development. Unfortunately, making traces redactable had a [significant] performance overhead. On simple selects, this overhead is in the 10-20% area (when verbose tracing is enabled). This presents a stability risk, since some customers run with verbose tracing enabled for all queries. Once they switch to 21.2, their system may be underprovisioned as a result of the newly grown tracing overhead in the `log.{Ve,E}vent{,f}` family of methods. The peformance hit comes via the additional costs of redacting log strings via the `redact.Sprintf` function. This PR changes makes trace redactability a choice. Because we needed to support mixed-version clusters in cockroachdb#70562 this is not as big a lift as might have been expected. The approach taken is that tracers and the spans they generate can be marked as "redactable". If the flag is enabled, logging events to the span will use full redaction via `redact.Sprintf`. If the flag is not enabled, we implement a manual "coarse redaction" of the entire message by surrounding it with redaction markers. We take a "fast path" by checking to see if there are any existing redaction markers to escape in the message, making the assumption that usually there aren't. This approach maintains redactability (to varying degrees) of traces while reducing the performance hit of tracing in the case where we don't want detailed redactability. A further commit will enable redactability on tenant-bound traces since we do want to maintain that functionality in multi-tenant deployments. Original issue investigation that kicked off concerns about performance impact of redaction: cockroachdb#70110 (comment) ---- Benchmarks The base for all 3 below benchmarks that's used is the code on `release-21.2` with the "drop traces for tenants" commit reverted. This reproduces a reasonable "base" state where tracing redaction isn't really introduced at all. Additionally, all benchmarks are run with modifications introduced in the first two commits in cockroachdb#71610 which force tracing on in benchmarks by running `SET CLUSTER SETTING trace.debug.enable = true` in the regular and multinode scenarios shown below. Below are tracing benchmarks for the conditional redaction that is in **this** commit. ``` ~/go/src/github.com/cockroachdb/cockroach make-traces-redactable-and-default-to-off--v2* ≡ 19:36:43 ❯ benchstat 83fc452-no-tracing-bench.txt 2bea5a2ac5-conditional-redaction-simple.txt name old time/op new time/op delta Tracing/Cockroach/tracing=f/Scan1-8 612µs ± 9% 591µs ±16% ~ (p=0.393 n=10+10) Tracing/Cockroach/tracing=f/Insert-8 785µs ± 4% 767µs ± 6% ~ (p=0.360 n=8+10) Tracing/Cockroach/tracing=t/Scan1-8 597µs ± 1% 542µs ± 1% -9.23% (p=0.000 n=10+9) Tracing/Cockroach/tracing=t/Insert-8 762µs ± 3% 739µs ± 8% ~ (p=0.315 n=10+10) Tracing/MultinodeCockroach/tracing=f/Scan1-8 1.05ms ±30% 0.92ms ± 5% -12.57% (p=0.000 n=9+9) Tracing/MultinodeCockroach/tracing=f/Insert-8 1.19ms ±15% 1.13ms ± 9% -5.39% (p=0.035 n=10+10) Tracing/MultinodeCockroach/tracing=t/Scan1-8 979µs ± 3% 945µs ± 6% -3.42% (p=0.024 n=9+9) Tracing/MultinodeCockroach/tracing=t/Insert-8 1.54ms ±48% 1.19ms ±16% -22.76% (p=0.011 n=10+10) name old alloc/op new alloc/op delta Tracing/Cockroach/tracing=f/Scan1-8 86.5kB ± 1% 90.2kB ± 3% +4.29% (p=0.000 n=9+9) Tracing/Cockroach/tracing=f/Insert-8 109kB ± 3% 111kB ± 0% +2.68% (p=0.005 n=9+7) Tracing/Cockroach/tracing=t/Scan1-8 86.0kB ± 1% 89.8kB ± 0% +4.42% (p=0.000 n=10+9) Tracing/Cockroach/tracing=t/Insert-8 108kB ± 3% 112kB ± 3% +3.05% (p=0.000 n=9+9) Tracing/MultinodeCockroach/tracing=f/Scan1-8 156kB ±37% 149kB ± 1% ~ (p=0.050 n=9+9) Tracing/MultinodeCockroach/tracing=f/Insert-8 154kB ±10% 154kB ± 3% ~ (p=0.382 n=8+8) Tracing/MultinodeCockroach/tracing=t/Scan1-8 145kB ± 1% 150kB ± 5% +3.61% (p=0.000 n=9+9) Tracing/MultinodeCockroach/tracing=t/Insert-8 157kB ±13% 183kB ±40% ~ (p=0.237 n=8+10) name old allocs/op new allocs/op delta Tracing/Cockroach/tracing=f/Scan1-8 1.44k ± 0% 1.50k ± 0% +4.22% (p=0.000 n=9+8) Tracing/Cockroach/tracing=f/Insert-8 1.64k ± 0% 1.72k ± 0% +4.39% (p=0.000 n=9+9) Tracing/Cockroach/tracing=t/Scan1-8 1.44k ± 0% 1.50k ± 0% +4.33% (p=0.000 n=8+7) Tracing/Cockroach/tracing=t/Insert-8 1.64k ± 0% 1.72k ± 0% +4.34% (p=0.000 n=9+9) Tracing/MultinodeCockroach/tracing=f/Scan1-8 2.41k ±11% 2.38k ± 1% ~ (p=0.150 n=10+9) Tracing/MultinodeCockroach/tracing=f/Insert-8 2.15k ± 1% 2.22k ± 1% +3.35% (p=0.000 n=8+8) Tracing/MultinodeCockroach/tracing=t/Scan1-8 2.31k ± 1% 2.40k ± 4% +4.19% (p=0.000 n=9+9) Tracing/MultinodeCockroach/tracing=t/Insert-8 2.17k ± 6% 2.33k ±12% +7.43% (p=0.012 n=8+10) ``` These statistics are the *base* hit we take on the redaction work **prior** to any conditional redaction being added. This is what we're trying to improve on. ``` ~/go/src/github.com/cockroachdb/cockroach make-traces-redactable-and-default-to-off--v2* ≡ 19:36:49 ❯ benchstat 83fc452-no-tracing-bench.txt dc42e37-after-redaction.txt name old time/op new time/op delta Tracing/Cockroach/tracing=f/Scan1-8 612µs ± 9% 693µs ± 2% +13.22% (p=0.000 n=10+10) Tracing/Cockroach/tracing=f/Insert-8 785µs ± 4% 878µs ± 1% +11.86% (p=0.000 n=8+9) Tracing/Cockroach/tracing=t/Scan1-8 597µs ± 1% 687µs ± 3% +15.05% (p=0.000 n=10+9) Tracing/Cockroach/tracing=t/Insert-8 762µs ± 3% 878µs ± 1% +15.35% (p=0.000 n=10+8) Tracing/MultinodeCockroach/tracing=f/Scan1-8 1.05ms ±30% 1.06ms ± 2% ~ (p=0.050 n=9+9) Tracing/MultinodeCockroach/tracing=f/Insert-8 1.19ms ±15% 1.28ms ±11% +7.85% (p=0.035 n=10+10) Tracing/MultinodeCockroach/tracing=t/Scan1-8 979µs ± 3% 1066µs ± 1% +8.87% (p=0.000 n=9+9) Tracing/MultinodeCockroach/tracing=t/Insert-8 1.54ms ±48% 1.26ms ± 2% ~ (p=0.515 n=10+8) name old alloc/op new alloc/op delta Tracing/Cockroach/tracing=f/Scan1-8 86.5kB ± 1% 98.0kB ± 1% +13.33% (p=0.000 n=9+9) Tracing/Cockroach/tracing=f/Insert-8 109kB ± 3% 125kB ± 1% +15.09% (p=0.000 n=9+9) Tracing/Cockroach/tracing=t/Scan1-8 86.0kB ± 1% 97.9kB ± 1% +13.90% (p=0.000 n=10+9) Tracing/Cockroach/tracing=t/Insert-8 108kB ± 3% 125kB ± 1% +15.53% (p=0.000 n=9+9) Tracing/MultinodeCockroach/tracing=f/Scan1-8 156kB ±37% 162kB ± 1% ~ (p=0.059 n=9+8) Tracing/MultinodeCockroach/tracing=f/Insert-8 154kB ±10% 197kB ±43% +28.54% (p=0.000 n=8+10) Tracing/MultinodeCockroach/tracing=t/Scan1-8 145kB ± 1% 162kB ± 1% +11.81% (p=0.000 n=9+9) Tracing/MultinodeCockroach/tracing=t/Insert-8 157kB ±13% 172kB ± 5% +9.84% (p=0.007 n=8+8) name old allocs/op new allocs/op delta Tracing/Cockroach/tracing=f/Scan1-8 1.44k ± 0% 1.66k ± 0% +15.59% (p=0.000 n=9+9) Tracing/Cockroach/tracing=f/Insert-8 1.64k ± 0% 1.95k ± 0% +18.48% (p=0.000 n=9+8) Tracing/Cockroach/tracing=t/Scan1-8 1.44k ± 0% 1.66k ± 0% +15.46% (p=0.000 n=8+9) Tracing/Cockroach/tracing=t/Insert-8 1.64k ± 0% 1.95k ± 1% +18.52% (p=0.000 n=9+9) Tracing/MultinodeCockroach/tracing=f/Scan1-8 2.41k ±11% 2.58k ± 1% ~ (p=0.105 n=10+8) Tracing/MultinodeCockroach/tracing=f/Insert-8 2.15k ± 1% 2.58k ±12% +20.04% (p=0.000 n=8+10) Tracing/MultinodeCockroach/tracing=t/Scan1-8 2.31k ± 1% 2.59k ± 1% +12.09% (p=0.000 n=9+9) Tracing/MultinodeCockroach/tracing=t/Insert-8 2.17k ± 6% 2.47k ± 1% +13.76% (p=0.000 n=8+8) ``` Touches cockroachdb#70562. Release note: None
dhartunian · Dec 6, 2021 · e06079d · e06079d
1 parent 5cd6c77
commit e06079d
Show file tree

Hide file tree

Showing 10 changed files with 169 additions and 25 deletions.
diff --git a/pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go b/pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go
@@ -19,17 +19,25 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/stretchr/testify/require"
 )
 
 // TestTenantTracesAreRedacted is an end-to-end version of
 // `kvserver.TestMaybeRedactRecording`.
 func TestTenantTracesAreRedacted(t *testing.T) {
 	defer leaktest.AfterTest(t)()
+	testutils.RunTrueAndFalse(t, "redactable", func(t *testing.T, redactable bool) {
+		testTenantTracesAreRedactedImpl(t, redactable)
+	})
+}
+
+func testTenantTracesAreRedactedImpl(t *testing.T, redactable bool) {
 	defer log.Scope(t).Close(t)
 	ctx := context.Background()
 
@@ -61,6 +69,7 @@ SET tracing = off;
 	var args base.TestClusterArgs
 	args.ServerArgs.Knobs.Store = knobs
 	tc := serverutils.StartNewTestCluster(t, 1, args)
+	tc.Server(0).Tracer().(*tracing.Tracer).SetRedactable(redactable)
 	defer tc.Stopper().Stop(ctx)
 
 	t.Run("system-tenant", func(t *testing.T) {
@@ -88,7 +97,9 @@ SET tracing = off;
 		defer tenDB.Close()
 		results := getTrace(t, tenDB)
 
+		const redactedMarkerString = "verbose trace message redacted"
 		var found bool
+		var foundRedactedMarker bool
 		for _, sl := range results {
 			for _, s := range sl {
 				if strings.Contains(s, sensitiveString) {
@@ -100,9 +111,26 @@ SET tracing = off;
 				if strings.Contains(s, visibleString) {
 					found = true
 				}
+				if strings.Contains(s, redactedMarkerString) {
+					foundRedactedMarker = true
+				}
 			}
 		}
-		require.True(t, found, "trace for tenant missing trace message '%q':\n%s",
-			visibleString, sqlutils.MatrixToStr(results))
+
+		if redactable {
+			// If redaction was on, we expect the tenant to see safe information in its
+			// trace.
+			require.True(t, found, "did not see expected trace message '%q':\n%s",
+				visibleString, sqlutils.MatrixToStr(results))
+			require.False(t, foundRedactedMarker, "unexpectedly found '%q':\n%s",
+				redactedMarkerString, sqlutils.MatrixToStr(results))
+		} else {
+			// Otherwise, expect the opposite: not even safe information makes it through,
+			// because it gets replaced with foundRedactedMarker.
+			require.False(t, found, "unexpectedly saw message '%q':\n%s",
+				visibleString, sqlutils.MatrixToStr(results))
+			require.True(t, foundRedactedMarker, "not not find expected message '%q':\n%s",
+				redactedMarkerString, sqlutils.MatrixToStr(results))
+		}
 	})
 }
diff --git a/pkg/server/node_tenant.go b/pkg/server/node_tenant.go
@@ -17,8 +17,13 @@ import (
 	"github.com/cockroachdb/redact"
 )
 
-var sRedactedMarker = redact.RedactableString(redact.EscapeBytes(nil))
+var sRedactedMarker = redact.RedactableString("verbose trace message redacted")
 
+// maybeRedactRecording will inspect all entries in the `Logs` field of
+// the recording and redact them if the recorded span has
+// `RedactableLogs` enabled. Otherwise, the field value is replaced with
+// a static marker.
+// The function also clears all tags.
 func maybeRedactRecording(tenID roachpb.TenantID, rec tracing.Recording) {
 	if tenID == roachpb.SystemTenantID {
 		return
@@ -32,18 +37,15 @@ func maybeRedactRecording(tenID roachpb.TenantID, rec tracing.Recording) {
 			record := &sp.Logs[j]
 			for k := range record.Fields {
 				field := &record.Fields[k]
-				if field.Key != tracingpb.LogMessageField {
-					// We don't have any of these fields, but let's not take any
-					// chances (our dependencies might slip them in).
+
+				if !sp.RedactableLogs {
+					// If we're handling a span that does not support redactability, all
+					// the containing information will be stripped.
 					field.Value = sRedactedMarker
 					continue
-				}
-				if !sp.RedactableLogs {
-					// If we're handling a span that originated from an (early patch
-					// release) 22.1 node, all the containing information will be
-					// stripped. Note that this is not the common path here, as most
-					// information in the trace will be from the local node, which
-					// always creates redactable logs.
+				} else if field.Key != tracingpb.LogMessageField {
+					// We don't have any of these fields, but let's not take any
+					// chances (our dependencies might slip them in).
 					field.Value = sRedactedMarker
 					continue
 				}

diff --git a/pkg/server/status.go b/pkg/server/status.go
@@ -677,7 +677,7 @@ func recordedSpansToTraceEvents(spans []tracingpb.RecordedSpan) []*serverpb.Trac
 					if i != 0 {
 						buf.WriteByte(' ')
 					}
-					fmt.Fprintf(&buf, "%s:%v", f.Key, f.Value)
+					fmt.Fprintf(&buf, "%s:%v", f.Key, f.Value.StripMarkers())
 				}
 				event.Message = buf.String()
 			}

diff --git a/pkg/util/log/log_entry.go b/pkg/util/log/log_entry.go
@@ -14,6 +14,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"strconv"
 	"strings"
 	"time"
 
@@ -129,6 +130,45 @@ func (e *logEntry) SafeFormat(w interfaces.SafePrinter, _ rune) {
 	}
 }
 
+// LegacyString is an old implementation of String(). It is still around
+// because it is significantly faster and sits in the hot path of verbose
+// tracing.
+func (e *logEntry) LegacyString() string {
+	entry := e.convertToLegacy()
+	if len(entry.Tags) == 0 && len(entry.File) == 0 && !entry.Redactable {
+		// Shortcut.
+		return entry.Message
+	}
+
+	var buf strings.Builder
+	if len(entry.File) != 0 {
+		buf.WriteString(entry.File)
+		buf.WriteByte(':')
+		// TODO(knz): The "canonical" way to represent a file/line prefix
+		// is: <file>:<line>: msg
+		// with a colon between the line number and the message.
+		// However, some location filter deep inside SQL doesn't
+		// understand a colon after the line number.
+		buf.WriteString(strconv.FormatInt(entry.Line, 10))
+		buf.WriteByte(' ')
+	}
+	if len(entry.Tags) > 0 {
+		buf.WriteByte('[')
+		buf.WriteString(entry.Tags)
+		buf.WriteString("] ")
+	}
+	buf.WriteString(entry.Message)
+	msg := buf.String()
+
+	if entry.Redactable {
+		// This is true when eventInternal is called from logfDepth(),
+		// ie. a regular log call. In this case, the tags and message may contain
+		// redaction markers. We remove them here.
+		msg = redact.RedactableString(msg).StripMarkers()
+	}
+	return msg
+}
+
 func (e *logEntry) String() string {
 	return redact.StringWithoutMarkers(e)
 }

diff --git a/pkg/util/log/trace.go b/pkg/util/log/trace.go
@@ -101,10 +101,17 @@ func getSpanOrEventLog(ctx context.Context) (*tracing.Span, *ctxEventLog, bool)
 }
 
 // eventInternal is the common code for logging an event to trace and/or event
-// logs. All entries passed to this method should be redactable.
+// logs. Entries passed to this method may or may not be redactable. However,
+// if sp.Redactable() is true then ideally they are for best results.
 func eventInternal(sp *tracing.Span, el *ctxEventLog, isErr bool, entry *logEntry) {
 	if sp != nil {
-		sp.Recordf("%s", entry)
+		if sp.Redactable() {
+			sp.Recordf("%s", entry)
+		} else {
+			// LegacyString is significantly faster than String since the latter
+			// round-trips through redaction. See BenchmarkEventf_WithVerboseTraceSpan.
+			sp.Recordf("%s", entry.LegacyString())
+		}
 		// TODO(obs-inf): figure out a way to signal that this is an error. We could
 		// use a different "error" key (provided it shows up in LightStep). Things
 		// like NetTraceIntegrator would need to be modified to understand the
@@ -162,7 +169,7 @@ func Event(ctx context.Context, msg string) {
 		severity.INFO, /* unused for trace events */
 		channel.DEV,   /* unused for trace events */
 		1,             /* depth */
-		true,          /* redactable */
+		sp.Redactable(),
 		msg)
 	eventInternal(sp, el, false /* isErr */, &entry)
 }
@@ -182,7 +189,7 @@ func Eventf(ctx context.Context, format string, args ...interface{}) {
 		severity.INFO, /* unused for trace events */
 		channel.DEV,   /* unused for trace events */
 		1,             /* depth */
-		true,          /* redactable */
+		sp.Redactable(),
 		format, args...)
 	eventInternal(sp, el, false /* isErr */, &entry)
 }
@@ -207,7 +214,7 @@ func vEventf(
 			severity.INFO, /* unused for trace events */
 			channel.DEV,   /* unused for trace events */
 			depth+1,
-			true, /* redactable */
+			sp.Redactable(),
 			format, args...)
 		eventInternal(sp, el, isErr, &entry)
 	}

diff --git a/pkg/util/tracing/crdbspan.go b/pkg/util/tracing/crdbspan.go
@@ -40,6 +40,7 @@ type crdbSpan struct {
 	traceID      uint64 // probabilistically unique
 	spanID       uint64 // probabilistically unique
 	parentSpanID uint64
+	redactable   bool // are verbose logs redactable?
 	goroutineID  uint64
 
 	startTime time.Time
@@ -285,7 +286,7 @@ func (s *crdbSpan) setTagLocked(key string, value interface{}) {
 	s.mu.tags[key] = value
 }
 
-func (s *crdbSpan) record(msg redact.RedactableString) {
+func (s *crdbSpan) record(redactableString redact.RedactableString) {
 	if s.recordingType() != RecordingVerbose {
 		return
 	}
@@ -299,7 +300,7 @@ func (s *crdbSpan) record(msg redact.RedactableString) {
 	logRecord := &tracingpb.LogRecord{
 		Time: now,
 		Fields: []tracingpb.LogRecord_Field{
-			{Key: tracingpb.LogMessageField, Value: msg},
+			{Key: tracingpb.LogMessageField, Value: redactableString},
 		},
 	}
 

diff --git a/pkg/util/tracing/span.go b/pkg/util/tracing/span.go
@@ -65,6 +65,17 @@ func (sp *Span) Tracer() *Tracer {
 	return sp.i.Tracer()
 }
 
+// Redactable returns true if this Span records
+// redactable logs.
+func (sp *Span) Redactable() bool {
+	if sp == nil || sp.i.isNoop() {
+		return false
+	}
+	// NB: doesn't matter what sp.Tracer().Redactable() returns now, this Span's
+	// redactability is constant across its lifetime.
+	return sp.i.crdb.redactable
+}
+
 // SetOperationName sets the name of the operation.
 func (sp *Span) SetOperationName(operationName string) {
 	if sp.done() {

diff --git a/pkg/util/tracing/span_inner.go b/pkg/util/tracing/span_inner.go
@@ -11,6 +11,7 @@
 package tracing
 
 import (
+	"fmt"
 	"strings"
 	"time"
 
@@ -228,13 +229,48 @@ func (s *spanInner) Recordf(format string, args ...interface{}) {
 	if !s.hasVerboseSink() {
 		return
 	}
-	str := redact.Sprintf(format, args...)
+	// If the span is redactable, we pay the cost of using the redaction
+	// library, otherwise we manually do a "coarse" redaction of the
+	// entire string to keep the non-redactable path fast.
+	// Currently we expect this cost to be paid on tenant spans only.
+	var str redact.RedactableString
+	if s.crdb.redactable {
+		str = redact.Sprintf(format, args...)
+	} else {
+		// Optimistically build the message as a fully redacted string
+		// with markers at the start and end but then read through it
+		// see if there are markers inside we need to escape. If so, we
+		// will pay the cost of calling `redact.Sprintf` but we expect that
+		// to essentially not happen.
+		b := strings.Builder{}
+		b.Write(redact.StartMarker())
+		// TODO(davidh): Unhandled error here, not sure what I can do though...log it?
+		fmt.Fprintf(&b, format, args...)
+		b.Write(redact.EndMarker())
+		str = redact.RedactableString(b.String())
+
+		doEscape := false
+		// Ignore first and last index since we put markers there ourselves
+		for i := 1; i < len(format)-1; i++ {
+			if str[i] == redact.StartMarker()[0] || str[i] == redact.EndMarker()[0] {
+				doEscape = true
+				break
+			}
+		}
+		if doEscape {
+			// Override string and redact it if we find there was a marker in
+			// there This will only run if this particular message had a
+			// redaction start/end marker inside
+			str = redact.Sprintf("%s", s)
+		}
+	}
 	if s.ot.shadowSpan != nil {
 		// TODO(obs-inf): depending on the situation it may be more appropriate to
-		// redact the string here.
+		// redact the string here. Note that redaction markers are not stripped
+		// here either
 		// See:
 		// https://github.com/cockroachdb/cockroach/issues/58610#issuecomment-926093901
-		s.ot.shadowSpan.LogFields(otlog.String(tracingpb.LogMessageField, str.StripMarkers()))
+		s.ot.shadowSpan.LogFields(otlog.String(tracingpb.LogMessageField, string(str)))
 	}
 	if s.netTr != nil {
 		s.netTr.LazyPrintf(format, args)

diff --git a/pkg/util/tracing/tracer.go b/pkg/util/tracing/tracer.go
@@ -131,6 +131,8 @@ type Tracer struct {
 	// True if tracing to the debug/requests endpoint. Accessed via t.useNetTrace().
 	_useNetTrace int32 // updated atomically
 
+	_redactable int32 // updated atomically
+
 	// Pointer to shadowTracer, if using one.
 	shadowTracer unsafe.Pointer
 
@@ -161,6 +163,22 @@ type Tracer struct {
 	testing *testingKnob
 }
 
+// Redactable returns true if the tracer is configured to emit
+// redactable logs.
+func (t *Tracer) Redactable() bool {
+	return atomic.LoadInt32(&t._redactable) != 0
+}
+
+// SetRedactable changes the redactability of the Tracer. This
+// affects any future trace spans created.
+func (t *Tracer) SetRedactable(to bool) {
+	var n int32
+	if to {
+		n = 1
+	}
+	atomic.StoreInt32(&t._redactable, n)
+}
+
 // NewTracer creates a Tracer. It initially tries to run with minimal overhead
 // and collects essentially nothing; use Configure() to enable various tracing
 // backends.
@@ -371,6 +389,7 @@ func (t *Tracer) startSpanGeneric(
 	helper.crdbSpan = crdbSpan{
 		traceID:      traceID,
 		spanID:       spanID,
+		redactable:   t.Redactable(),
 		goroutineID:  goroutineID,
 		startTime:    startTime,
 		parentSpanID: opts.parentSpanID(),

diff --git a/pkg/util/tracing/tracingpb/recorded_span.go b/pkg/util/tracing/tracingpb/recorded_span.go
@@ -58,7 +58,7 @@ func (l LogRecord) Msg() string {
 			return f.Value.StripMarkers()
 		}
 		if key == "error" {
-			return fmt.Sprint("error:", f.Value)
+			return fmt.Sprint("error:", f.Value.StripMarkers())
 		}
 	}
 	return ""