diff --git a/pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go b/pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go index f8de97f2d154..755fb8386e8a 100644 --- a/pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go +++ b/pkg/ccl/kvccl/kvtenantccl/tenant_trace_test.go @@ -19,6 +19,7 @@ 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/server" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" @@ -72,6 +73,8 @@ SET tracing = off; tc.Server(0).TracerI().(*tracing.Tracer).SetRedactable(redactable) defer tc.Stopper().Stop(ctx) + // Queries from the system tenant will receive unredacted traces + // since the tracer will not have the redactable flag set. t.Run("system-tenant", func(t *testing.T) { db := tc.ServerConn(0) defer db.Close() @@ -97,7 +100,6 @@ 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 { @@ -111,26 +113,29 @@ SET tracing = off; if strings.Contains(s, visibleString) { found = true } - if strings.Contains(s, redactedMarkerString) { + if strings.Contains(s, string(server.TraceRedactedMarker)) { foundRedactedMarker = true } } } + // In both cases we don't expect to see the `TraceRedactedMarker` + // since that's only shown when the server is in an inconsistent + // state or if there's a version mismatch between client and server. 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)) + string(server.TraceRedactedMarker), 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)) + require.False(t, foundRedactedMarker, "unexpectedly found '%q':\n%s", + string(server.TraceRedactedMarker), sqlutils.MatrixToStr(results)) } }) } diff --git a/pkg/kv/txn.go b/pkg/kv/txn.go index 433231e61b58..f19b6d479e0c 100644 --- a/pkg/kv/txn.go +++ b/pkg/kv/txn.go @@ -965,7 +965,7 @@ func (txn *Txn) exec(ctx context.Context, fn func(context.Context, *Txn) error) if err == nil { if !txn.IsCommitted() { err = txn.Commit(ctx) - log.Eventf(ctx, "client.Txn did AutoCommit. err: %v\n", err) + log.Eventf(ctx, "client.Txn did AutoCommit. err: %v", err) if err != nil { if !errors.HasType(err, (*roachpb.TransactionRetryWithProtoRefreshError)(nil)) { // We can't retry, so let the caller know we tried to diff --git a/pkg/kv/txn_test.go b/pkg/kv/txn_test.go index 31a9c9372075..21966ff11147 100644 --- a/pkg/kv/txn_test.go +++ b/pkg/kv/txn_test.go @@ -68,7 +68,6 @@ func TestTxnVerboseTrace(t *testing.T) { `(?s)`+ `.*event:[^:]*:\d+ inside txn\n`+ `.*event:[^:]*:\d+ client\.Txn did AutoCommit\. err: \n`+ - `.*\n`+ `.*event:[^:]*:\d+ txn complete.*`, dump) if err != nil { diff --git a/pkg/server/node.go b/pkg/server/node.go index 128f98ee7a5c..c6520dad819d 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -1050,6 +1050,7 @@ func (n *Node) setupSpanForIncomingRPC( // remoteTrace case below. const opName = "/cockroach.roachpb.Internal/Batch" tr := n.storeCfg.AmbientCtx.Tracer + // newSpan is set if we end up creating a new span. var newSpan *tracing.Span parentSpan := tracing.SpanFromContext(ctx) diff --git a/pkg/server/node_tenant.go b/pkg/server/node_tenant.go index d3f4ca6d89eb..b80735ce5b95 100644 --- a/pkg/server/node_tenant.go +++ b/pkg/server/node_tenant.go @@ -18,7 +18,8 @@ import ( "github.com/cockroachdb/redact" ) -const sRedactedMarker = redact.RedactableString("verbose trace message redacted") +// TraceRedactedMarker is used to replace logs that weren't redacted. +const TraceRedactedMarker = redact.RedactableString("verbose trace message redacted") // redactRecordingForTenant redacts the sensitive parts of log messages in the // recording if the tenant to which this recording is intended is not the system @@ -49,7 +50,7 @@ func redactRecordingForTenant(tenID roachpb.TenantID, rec tracing.Recording) err 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 + field.Value = TraceRedactedMarker continue } if !sp.RedactableLogs { @@ -58,7 +59,7 @@ func redactRecordingForTenant(tenID roachpb.TenantID, rec tracing.Recording) err // 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. - field.Value = sRedactedMarker + field.Value = TraceRedactedMarker continue } field.Value = field.Value.Redact() diff --git a/pkg/util/tracing/tracer.go b/pkg/util/tracing/tracer.go index 00e7082d8118..42b6013d71e6 100644 --- a/pkg/util/tracing/tracer.go +++ b/pkg/util/tracing/tracer.go @@ -86,6 +86,18 @@ const ( spanKindTagKey = "span.kind" ) +// TODO(davidh): Once performance issues around redaction are +// resolved via #58610, this setting can be removed so that all traces +// have redactability enabled. +var enableTraceRedactable = settings.RegisterBoolSetting( + "trace.redactable", + "set to true to enable redactability for unstructured events "+ + "in traces and to redact traces sent to tenants. "+ + "Set to false to coarsely mark unstructured events as redactable "+ + " and eliminate them from tenant traces.", + false, +) + var enableNetTrace = settings.RegisterBoolSetting( "trace.debug.enable", "if set, traces for recent requests can be seen at https:///debug/requests", @@ -412,6 +424,9 @@ func (t *Tracer) Configure(ctx context.Context, sv *settings.Values) { jaegerAgentAddr := jaegerAgent.Get(sv) otlpCollectorAddr := openTelemetryCollector.Get(sv) zipkinAddr := ZipkinCollector.Get(sv) + enableRedactable := enableTraceRedactable.Get(sv) + + t.SetRedactable(enableRedactable) var nt int32 if enableNetTrace.Get(sv) { @@ -494,6 +509,7 @@ func (t *Tracer) Configure(ctx context.Context, sv *settings.Values) { openTelemetryCollector.SetOnChange(sv, reconfigure) ZipkinCollector.SetOnChange(sv, reconfigure) jaegerAgent.SetOnChange(sv, reconfigure) + enableTraceRedactable.SetOnChange(sv, reconfigure) } func createOTLPSpanProcessor(