server, tracing: force redactability on tenant spans

Prior commits added the ability to set a flag on the Tracer to determine
whether Span's should be marked redactable or not. This commit adds
logic to selectively force redactability enabled on Spans for requests
that come from a non-system tenant.

The desired outcome is to preserve redactability for tenant traces while
preserving a non-redactable code path for requests using the system
tenant. This preserves existing functionality in production while
keeping the current performance hit of fine grained redaction from
propagating to non-tenant deployments.

Resolves cockroachdb#71694

Release note: None

pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go

@@ -24,7 +24,6 @@ import (
  "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"
 )
 
@@ -69,7 +68,6 @@ 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) {
@@ -117,20 +115,10 @@ SET tracing = off;
  }
  }
 
- 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))
- }
+ // Redactability is always enabled for tenants
+ 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))
  })
 }

pkg/server/node.go

@@ -1050,6 +1050,15 @@ func (n *Node) setupSpanForIncomingRPC(
  // remoteTrace case below.
  const opName = "/cockroach.roachpb.Internal/Batch"
  tr := n.storeCfg.AmbientCtx.Tracer
+
+ // We require redactability enabled on tenant requests.
+ // TODO(davidh): Once performance issues around redaction are
+ // resolved via #58610, this code can be removed so that all traces
+ // have redactability enabled.
+ if !tr.Redactable() && tenID != roachpb.SystemTenantID {
+ tr.SetRedactable(true)
+ }
+
  // newSpan is set if we end up creating a new span.
  var newSpan *tracing.Span
  parentSpan := tracing.SpanFromContext(ctx)

pkg/server/node_tenant_test.go

@@ -45,7 +45,10 @@ func TestRedactRecordingForTenant(t *testing.T) {
  Add("tag_sensitive", tagSensitive).
  Add("tag_not_sensitive", log.Safe(tagNotSensitive))
  ctx := logtags.WithTags(context.Background(), tags)
- ctx, sp := tracing.NewTracer().StartSpanCtx(ctx, "foo", tracing.WithRecording(tracing.RecordingVerbose))
+ tracer := tracing.NewTracer()
+ tracer.SetRedactable(true)
+ ctx, sp := tracer.StartSpanCtx(ctx, "foo", tracing.WithRecording(tracing.RecordingVerbose))
+ sp.SetVerbose(true)
 
  log.Eventf(ctx, "%s %s", msgSensitive, log.Safe(msgNotSensitive))
  sp.SetTag("all_span_tags_are_stripped", attribute.StringValue("because_no_redactability"))