From 09ba34a94b8252949180a2cb461f100baa1ddd92 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Thu, 25 May 2023 13:44:41 +0200 Subject: [PATCH] execinfrapb: finer grain redactability for ComponentStats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before: ``` event:‹component:... net_rx:...› ``` After: ``` event:ComponentStats{ID: {‹6e92072b-31ed-4b90-b323-b55d2d2fe0a3› PROCESSOR 0 1}, execution time: 1.5s, batches output: 1, rows output: 1} ``` We can also later consider exposing the FlowID as well, with a suitable typedef and the `SafeValue` interface. Release note: None --- pkg/base/node_id.go | 3 ++ pkg/sql/execinfrapb/BUILD.bazel | 1 + pkg/sql/execinfrapb/component_stats.go | 20 +++++++++++++ pkg/sql/execinfrapb/component_stats.proto | 2 ++ pkg/sql/trace_test.go | 29 ++++++++++--------- .../lint/passes/redactcheck/redactcheck.go | 4 +++ 6 files changed, 46 insertions(+), 13 deletions(-) diff --git a/pkg/base/node_id.go b/pkg/base/node_id.go index b8fbb6b5520a..b0ef38f948af 100644 --- a/pkg/base/node_id.go +++ b/pkg/base/node_id.go @@ -184,6 +184,9 @@ func (s SQLInstanceID) String() string { return strconv.Itoa(int(s)) } +// SafeValue implements the redact.SafeValue interface. +func (s SQLInstanceID) SafeValue() {} + // SQLIDContainer is a variant of NodeIDContainer that contains SQL instance IDs. type SQLIDContainer NodeIDContainer diff --git a/pkg/sql/execinfrapb/BUILD.bazel b/pkg/sql/execinfrapb/BUILD.bazel index 67263d408748..77d73080f86e 100644 --- a/pkg/sql/execinfrapb/BUILD.bazel +++ b/pkg/sql/execinfrapb/BUILD.bazel @@ -60,6 +60,7 @@ go_library( "//pkg/util/uuid", "@com_github_cockroachdb_errors//:errors", "@com_github_cockroachdb_logtags//:logtags", + "@com_github_cockroachdb_redact//:redact", "@com_github_dustin_go_humanize//:go-humanize", "@com_github_gogo_protobuf//types", "@org_golang_google_grpc//:go_default_library", diff --git a/pkg/sql/execinfrapb/component_stats.go b/pkg/sql/execinfrapb/component_stats.go index 7b989c9a2768..d805fbb6b735 100644 --- a/pkg/sql/execinfrapb/component_stats.go +++ b/pkg/sql/execinfrapb/component_stats.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/optional" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb" + "github.com/cockroachdb/redact" "github.com/dustin/go-humanize" "github.com/gogo/protobuf/types" ) @@ -78,6 +79,25 @@ func (s *ComponentStats) StatsForQueryPlan() []string { return result } +// String implements fmt.Stringer and protoutil.Message. +func (s *ComponentStats) String() string { + return redact.StringWithoutMarkers(s) +} + +var _ redact.SafeFormatter = (*ComponentStats)(nil) + +// SafeValue implements redact.SafeValue. +func (ComponentID_Type) SafeValue() {} + +// SafeFormat implements redact.SafeFormatter. +func (s *ComponentStats) SafeFormat(w redact.SafePrinter, _ rune) { + w.Printf("ComponentStats{ID: %v", s.Component) + s.formatStats(func(key string, value interface{}) { + w.Printf(", %s: %v", redact.SafeString(key), value) + }) + w.SafeRune('}') +} + // formatStats calls fn for each statistic that is set. func (s *ComponentStats) formatStats(fn func(suffix string, value interface{})) { // Network Rx stats. diff --git a/pkg/sql/execinfrapb/component_stats.proto b/pkg/sql/execinfrapb/component_stats.proto index 85be3251f1ea..c45954c18712 100644 --- a/pkg/sql/execinfrapb/component_stats.proto +++ b/pkg/sql/execinfrapb/component_stats.proto @@ -63,6 +63,8 @@ message ComponentID { // Depending on the component, not all statistics apply. For all fields, the zero // value indicates that the particular stat is not available. message ComponentStats { + option (gogoproto.goproto_stringer) = false; + optional ComponentID component = 1 [(gogoproto.nullable) = false]; optional NetworkRxStats net_rx = 2 [(gogoproto.nullable) = false]; diff --git a/pkg/sql/trace_test.go b/pkg/sql/trace_test.go index da957d955c88..5f229a06c359 100644 --- a/pkg/sql/trace_test.go +++ b/pkg/sql/trace_test.go @@ -114,25 +114,28 @@ func TestTrace(t *testing.T) { // Check that stat collection from the above SELECT statement is output // to trace. We don't insert any rows in this test, thus the expected // num tuples value plus one is 1. - rows, err := sqlDB.Query( - "SELECT count(message) FROM crdb_internal.session_trace " + - "WHERE message LIKE '%component%num_tuples%value_plus_one:1%'", - ) + rows, err := sqlDB.Query("SELECT message FROM crdb_internal.session_trace") if err != nil { t.Fatal(err) } - if !rows.Next() { - t.Fatal("unable to retrieve count") - } - - var count int - if err := rows.Scan(&count); err != nil { + var trace strings.Builder + if err := func() error { + for rows.Next() { + var msg string + if err := rows.Scan(&msg); err != nil { + return err + } + fmt.Fprintln(&trace, msg) + } + return rows.Close() + }(); err != nil { t.Fatal(err) } - if err := rows.Close(); err != nil { - t.Fatal(err) + t.Logf("trace:\n%s", trace.String()) + if trace.Len() == 0 { + t.Fatalf("empty trace") } - if count == 0 { + if !strings.Contains(trace.String(), "ComponentStats") { t.Fatalf("no stat messages found") } diff --git a/pkg/testutils/lint/passes/redactcheck/redactcheck.go b/pkg/testutils/lint/passes/redactcheck/redactcheck.go index ced49a2ebf21..00ddddb11aa2 100644 --- a/pkg/testutils/lint/passes/redactcheck/redactcheck.go +++ b/pkg/testutils/lint/passes/redactcheck/redactcheck.go @@ -58,6 +58,7 @@ func runAnalyzer(pass *analysis.Pass) (interface{}, error) { "NodeIDContainer": {}, "SQLIDContainer": {}, "StoreIDContainer": {}, + "SQLInstanceID": {}, }, "github.com/cockroachdb/cockroach/pkg/cli/exit": { "Code": {}, @@ -134,6 +135,9 @@ func runAnalyzer(pass *analysis.Pass) (interface{}, error) { "IndexDescriptorVersion": {}, "MutationID": {}, }, + "github.com/cockroachdb/cockroach/pkg/sql/execinfrapb": { + "ComponentID_Type": {}, + }, "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirebase": { "FormatCode": {}, },