From 89a201215fda18fd643c15eae002fffd821ba76b Mon Sep 17 00:00:00 2001
From: Raphael 'kena' Poss <knz@thaumogen.net>
Date: Fri, 12 May 2023 22:17:11 +0200
Subject: [PATCH] tracing: preserve redactability of structured items in
 verbose traces

Prior to this patch, structured items added to verbose traces were
improperly converted to an unsafe string prior to recording.
This was making the entire structured payload redactable in the
recording representation.

This patch fixes the situation by delegating the representation
to the structured item itself, which may implement a SafeFormat
method.

Release note: None
---
 pkg/util/tracing/span_inner.go | 15 ++++++++++++---
 pkg/util/tracing/span_test.go  | 18 ++++++++++++++++++
 pkg/util/tracing/test_utils.go | 10 +++++++++-
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/pkg/util/tracing/span_inner.go b/pkg/util/tracing/span_inner.go
index 34f4a427b227..2c3cae8750fe 100644
--- a/pkg/util/tracing/span_inner.go
+++ b/pkg/util/tracing/span_inner.go
@@ -265,7 +265,10 @@ func (s *spanInner) RecordStructured(item Structured) {
 	}
 	s.crdb.recordStructured(item)
 	if s.hasVerboseSink() {
-		s.Record(item.String())
+		// Do not call .String() on the item, so that non-redactable bits
+		// in its representation are properly preserved by Recordf() in
+		// verbose recordings.
+		s.Recordf("%v", item)
 	}
 }
 
@@ -283,10 +286,16 @@ func (s *spanInner) Recordf(format string, args ...interface{}) {
 	} else {
 		// `fmt.Sprintf` when called on a logEntry will use the faster
 		// `logEntry.String` method instead of `logEntry.SafeFormat`.
-		// The additional use of `redact.Sprintf("%s",...)` is necessary
+		// The additional use of `redact.Sprint(...)` is necessary
 		// to wrap the result in redaction markers.
-		str = redact.Sprintf("%s", fmt.Sprintf(format, args...))
+		str = redact.Sprint(fmt.Sprintf(format, args...))
 	}
+	s.recordRedactable(format, args, str)
+}
+
+func (s *spanInner) recordRedactable(
+	format string, args []interface{}, str redact.RedactableString,
+) {
 	if s.otelSpan != nil {
 		// TODO(obs-inf): depending on the situation it may be more appropriate to
 		// redact the string here.
diff --git a/pkg/util/tracing/span_test.go b/pkg/util/tracing/span_test.go
index 7c721d6a8b82..8ed491d8f9dc 100644
--- a/pkg/util/tracing/span_test.go
+++ b/pkg/util/tracing/span_test.go
@@ -247,6 +247,24 @@ func TestImportRemoteRecordingMaintainsRightByteSize(t *testing.T) {
 	rootTrace.check(t)
 }
 
+func TestSpanRecordStructuredRedactable(t *testing.T) {
+	tr := NewTracer()
+	tr.SetRedactable(true)
+	sp := tr.StartSpan("root", WithRecording(tracingpb.RecordingVerbose))
+	defer sp.Finish()
+
+	payload := &tracingpb.OperationMetadata{Count: 123}
+	sp.RecordStructured(payload)
+	rec := sp.GetRecording(tracingpb.RecordingStructured)
+
+	// The following check fails if RecordStructured incorrectly
+	// serializes the event to a string.
+	checkRecordingWithRedact(t, rec, `
+		=== operation:root
+event:{count: 123, duration 0µs}
+structured:‹×›`, true)
+}
+
 func TestSpanRecordStructured(t *testing.T) {
 	tr := NewTracer()
 	sp := tr.StartSpan("root", WithRecording(tracingpb.RecordingStructured))
diff --git a/pkg/util/tracing/test_utils.go b/pkg/util/tracing/test_utils.go
index 519d2e78ff9b..a464b25f71ad 100644
--- a/pkg/util/tracing/test_utils.go
+++ b/pkg/util/tracing/test_utils.go
@@ -178,6 +178,10 @@ type T interface {
 //	        event:remote child 1
 //	`)
 func checkRecording(t T, rec tracingpb.Recording, expected string) {
+	checkRecordingWithRedact(t, rec, expected, false)
+}
+
+func checkRecordingWithRedact(t T, rec tracingpb.Recording, expected string, redactValues bool) {
 	normalize := func(rec string) string {
 		// normalize the string form of a recording for ease of comparison.
 		//
@@ -235,8 +239,12 @@ func checkRecording(t T, rec tracingpb.Recording, expected string) {
 		sortChildrenMetadataByName(rec[i].ChildrenMetadata)
 	}
 
+	actual := redact.Sprint(rec)
+	if redactValues {
+		actual = actual.Redact()
+	}
 	exp := normalize(expected)
-	got := normalize(string(redact.Sprint(rec)))
+	got := normalize(string(actual))
 	if got != exp {
 		diff := difflib.UnifiedDiff{
 			A:        difflib.SplitLines(exp),