Merge cockroachdb#89841

89841: server: fix tracing benchmark r=andreimatei a=andreimatei

A tracing benchmark had been inadvertently polluted by creating test cluster settings in the benchmark loop. This patch fixes it, and cleans up the code under test so that it no longer awkwardly stashes the cluster settings.

Fixes cockroachdb#89202

With this fix, the benchmark shows an expected degradation since 22.1 caused by one new alloc, which is worth it. It's interesting that these results are with the 22.1 benchmark built with go1.19. With 22.1 built with 1.17 and master built with 1.19, the new numbers are significantly better than 22.1.

```
name                                  old time/op    new time/op    delta
SetupSpanForIncomingRPC/traceInfo-32     304ns ± 1%     382ns ± 2%   +25.41%  (p=0.008 n=5+5)
SetupSpanForIncomingRPC/grpcMeta-32     1.21µs ± 3%    1.18µs ± 2%    -2.87%  (p=0.024 n=5+5)
SetupSpanForIncomingRPC/no_parent-32     311ns ± 4%     384ns ± 5%   +23.49%  (p=0.008 n=5+5)

name                                  old alloc/op   new alloc/op   delta
SetupSpanForIncomingRPC/traceInfo-32     48.0B ± 0%     80.0B ± 0%   +66.67%  (p=0.008 n=5+5)
SetupSpanForIncomingRPC/grpcMeta-32       592B ± 0%      624B ± 0%    +5.41%  (p=0.008 n=5+5)
SetupSpanForIncomingRPC/no_parent-32     48.0B ± 0%     80.0B ± 0%   +66.67%  (p=0.008 n=5+5)

name                                  old allocs/op  new allocs/op  delta
SetupSpanForIncomingRPC/traceInfo-32      1.00 ± 0%      2.00 ± 0%  +100.00%  (p=0.008 n=5+5)
SetupSpanForIncomingRPC/grpcMeta-32       7.00 ± 0%      8.00 ± 0%   +14.29%  (p=0.008 n=5+5)
SetupSpanForIncomingRPC/no_parent-32      1.00 ± 0%      2.00 ± 0%  +100.00%  (p=0.008 n=5+5)
```

Release note: None
Epic: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>

pkg/server/admin.go

@@ -3070,7 +3070,16 @@ func (s *adminServer) SendKVBatch(
 
 	ctx, sp := s.server.node.setupSpanForIncomingRPC(ctx, roachpb.SystemTenantID, ba)
 	var br *roachpb.BatchResponse
-	defer func() { sp.finish(br) }()
+	// NB: wrapped to delay br evaluation to its value when returning.
+	defer func() {
+		var redact redactOpt
+		if redactServerTracesForSecondaryTenants.Get(&s.server.ClusterSettings().SV) {
+			redact = redactIfTenantRequest
+		} else {
+			redact = dontRedactEvenIfTenantRequest
+		}
+		sp.finish(br, redact)
+	}()
 	br, pErr := s.server.db.NonTransactionalSender().Send(ctx, *ba)
 	if br == nil {
 		br = &roachpb.BatchResponse{}

pkg/server/bench_test.go

@@ -15,7 +15,6 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
-	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
@@ -58,10 +57,8 @@ func BenchmarkSetupSpanForIncomingRPC(b *testing.B) {
 
 			b.ResetTimer()
 			for i := 0; i < b.N; i++ {
-				_, sp := setupSpanForIncomingRPC(
-					ctx, roachpb.SystemTenantID, ba, tr, cluster.MakeTestingClusterSettings(),
-				)
-				sp.finish(nil /* br */)
+				_, sp := setupSpanForIncomingRPC(ctx, roachpb.SystemTenantID, ba, tr)
+				sp.finish(nil /* br */, dontRedactEvenIfTenantRequest)
 			}
 		})
 	}

pkg/server/node.go

@@ -1064,7 +1064,15 @@ func (n *Node) batchInternal(
 	var reqSp spanForRequest
 	ctx, reqSp = n.setupSpanForIncomingRPC(ctx, tenID, args)
 	// NB: wrapped to delay br evaluation to its value when returning.
-	defer func() { reqSp.finish(br) }()
+	defer func() {
+		var redact redactOpt
+		if redactServerTracesForSecondaryTenants.Get(&n.storeCfg.Settings.SV) {
+			redact = redactIfTenantRequest
+		} else {
+			redact = dontRedactEvenIfTenantRequest
+		}
+		reqSp.finish(br, redact)
+	}()
 	if log.HasSpanOrEvent(ctx) {
 		log.Eventf(ctx, "node received request: %s", args.Summary())
 	}
@@ -1163,12 +1171,18 @@ type spanForRequest struct {
 	sp            *tracing.Span
 	needRecording bool
 	tenID         roachpb.TenantID
-	settings      *cluster.Settings
 }
 
+type redactOpt bool
+
+const (
+	redactIfTenantRequest         redactOpt = true
+	dontRedactEvenIfTenantRequest redactOpt = false
+)
+
 // finish finishes the span. If the span was recording and br is not nil, the
 // recording is written to br.CollectedSpans.
-func (sp *spanForRequest) finish(br *roachpb.BatchResponse) {
+func (sp *spanForRequest) finish(br *roachpb.BatchResponse, redactOpt redactOpt) {
 	var rec tracingpb.Recording
 	// If we don't have a response, there's nothing to attach a trace to.
 	// Nothing more for us to do.
@@ -1188,8 +1202,7 @@ func (sp *spanForRequest) finish(br *roachpb.BatchResponse) {
 		// Even if the recording sent to a tenant is redacted (anything sensitive
 		// is stripped out of the verbose messages), structured payloads
 		// stay untouched.
-		needRedaction := sp.tenID != roachpb.SystemTenantID &&
-			redactServerTracesForSecondaryTenants.Get(&sp.settings.SV)
+		needRedaction := sp.tenID != roachpb.SystemTenantID && redactOpt == redactIfTenantRequest
 		if needRedaction {
 			redactRecording(rec)
 		}
@@ -1213,15 +1226,11 @@ func (sp *spanForRequest) finish(br *roachpb.BatchResponse) {
 func (n *Node) setupSpanForIncomingRPC(
 	ctx context.Context, tenID roachpb.TenantID, ba *roachpb.BatchRequest,
 ) (context.Context, spanForRequest) {
-	return setupSpanForIncomingRPC(ctx, tenID, ba, n.storeCfg.AmbientCtx.Tracer, n.storeCfg.Settings)
+	return setupSpanForIncomingRPC(ctx, tenID, ba, n.storeCfg.AmbientCtx.Tracer)
 }
 
 func setupSpanForIncomingRPC(
-	ctx context.Context,
-	tenID roachpb.TenantID,
-	ba *roachpb.BatchRequest,
-	tr *tracing.Tracer,
-	settings *cluster.Settings,
+	ctx context.Context, tenID roachpb.TenantID, ba *roachpb.BatchRequest, tr *tracing.Tracer,
 ) (context.Context, spanForRequest) {
 	var newSpan *tracing.Span
 	parentSpan := tracing.SpanFromContext(ctx)
@@ -1265,7 +1274,6 @@ func setupSpanForIncomingRPC(
 		needRecording: needRecordingCollection,
 		tenID:         tenID,
 		sp:            newSpan,
-		settings:      settings,
 	}
 }
 

pkg/server/node_tenant_test.go

@@ -25,11 +25,11 @@ import (
 	"go.opentelemetry.io/otel/attribute"
 )
 
-// TestMaybeRedactRecording verifies that redactRecording strips
-// sensitive details for recordings consumed by tenants.
+// Check that redactRecording strips sensitive details from recordings.
 //
-// See kvccl.TestTenantTracesAreRedacted for an end-to-end test of this.
-func TestRedactRecordingForTenant(t *testing.T) {
+// See kvccl.TestTenantTracesAreRedacted for an end-to-end test of tenant trace
+// redaction.
+func TestRedactRecording(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
 	const (