Merge cockroachdb#62546

62546: catalog: add telemetry for descriptor validation errors r=postamar a=postamar This commit adds `sql.schema.validation_errors.*` telemetry keys to descriptor validation errors. Fixes cockroachdb#61786. Release note: None Co-authored-by: Marius Posta <[email protected]>
fqazi · Mar 31, 2021 · a49cd0a · a49cd0a
2 parents 99c29ef + edcf5a8
commit a49cd0a
Show file tree

Hide file tree

Showing 12 changed files with 256 additions and 69 deletions.
diff --git a/pkg/server/telemetry/BUILD.bazel b/pkg/server/telemetry/BUILD.bazel
@@ -9,6 +9,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/server/telemetry",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/sql/catalog/catconstants",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
         "//pkg/util/metric",

diff --git a/pkg/server/telemetry/features.go b/pkg/server/telemetry/features.go
@@ -13,8 +13,10 @@ package telemetry
 import (
 	"fmt"
 	"math"
+	"strings"
 	"sync/atomic"
 
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
@@ -247,9 +249,13 @@ func RecordError(err error) {
 		default:
 			prefix = "othererror." + code.String() + "."
 		}
-
 		for _, tk := range tkeys {
-			Count(prefix + tk)
+			prefixedTelemetryKey := prefix + tk
+			if strings.HasPrefix(tk, catconstants.ValidationTelemetryKeyPrefix) {
+				// Descriptor validation errors already have their own prefixing scheme.
+				prefixedTelemetryKey = tk
+			}
+			Count(prefixedTelemetryKey)
 		}
 	}
 }
diff --git a/pkg/sql/catalog/BUILD.bazel b/pkg/sql/catalog/BUILD.bazel
@@ -22,6 +22,7 @@ go_library(
         "//pkg/kv",
         "//pkg/roachpb",
         "//pkg/settings/cluster",
+        "//pkg/sql/catalog/catconstants",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",

diff --git a/pkg/sql/catalog/catalogkv/catalogkv.go b/pkg/sql/catalog/catalogkv/catalogkv.go
@@ -165,7 +165,7 @@ func descriptorFromKeyValue(
 	} else {
 		desc = b.BuildImmutable()
 	}
-	err = catalog.Validate(ctx, dg, validationLevel, desc).CombinedError()
+	err = catalog.Validate(ctx, dg, catalog.ValidationReadTelemetry, validationLevel, desc).CombinedError()
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/sql/catalog/catconstants/constants.go b/pkg/sql/catalog/catconstants/constants.go
@@ -198,3 +198,7 @@ const (
 	PgExtensionSpatialRefSysTableID
 	MinVirtualID = PgExtensionSpatialRefSysTableID
 )
+
+// ValidationTelemetryKeyPrefix is the prefix of telemetry keys pertaining to
+// descriptor validation failures.
+const ValidationTelemetryKeyPrefix = "sql.schema.validation_errors."
diff --git a/pkg/sql/catalog/dbdesc/database_test.go b/pkg/sql/catalog/dbdesc/database_test.go
@@ -287,7 +287,8 @@ func TestValidateCrossDatabaseReferences(t *testing.T) {
 			descs.Namespace[namespaceKey] = desc.GetID()
 		}
 		expectedErr := fmt.Sprintf("%s %q (%d): %s", desc.DescriptorType(), desc.GetName(), desc.GetID(), test.err)
-		if err := catalog.Validate(ctx, descs, catalog.ValidationLevelAllPreTxnCommit, desc).CombinedError(); err == nil {
+		results := catalog.Validate(ctx, descs, catalog.NoValidationTelemetry, catalog.ValidationLevelAllPreTxnCommit, desc)
+		if err := results.CombinedError(); err == nil {
 			if test.err != "" {
 				t.Errorf("%d: expected \"%s\", but found success: %+v", i, expectedErr, test.desc)
 			}

diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go
@@ -1528,7 +1528,13 @@ func (tc *Collection) ValidateUncommittedDescriptors(ctx context.Context, txn *k
 		return nil
 	}
 	bdg := catalogkv.NewOneLevelUncachedDescGetter(txn, tc.codec())
-	return catalog.Validate(ctx, bdg, catalog.ValidationLevelAllPreTxnCommit, descs...).CombinedError()
+	return catalog.Validate(
+		ctx,
+		bdg,
+		catalog.ValidationWriteTelemetry,
+		catalog.ValidationLevelAllPreTxnCommit,
+		descs...,
+	).CombinedError()
 }
 
 // User defined type accessors.

diff --git a/pkg/sql/catalog/schemadesc/schema_desc_test.go b/pkg/sql/catalog/schemadesc/schema_desc_test.go
@@ -172,7 +172,8 @@ func TestValidateCrossSchemaReferences(t *testing.T) {
 		descs.Descriptors[test.dbDesc.ID] = dbdesc.NewBuilder(&test.dbDesc).BuildImmutable()
 		expectedErr := fmt.Sprintf("%s %q (%d): %s", desc.DescriptorType(), desc.GetName(), desc.GetID(), test.err)
 		const validateCrossReferencesOnly = catalog.ValidationLevelCrossReferences &^ (catalog.ValidationLevelCrossReferences >> 1)
-		if err := catalog.Validate(ctx, descs, validateCrossReferencesOnly, desc).CombinedError(); err == nil {
+		results := catalog.Validate(ctx, descs, catalog.NoValidationTelemetry, validateCrossReferencesOnly, desc)
+		if err := results.CombinedError(); err == nil {
 			if test.err != "" {
 				t.Errorf("%d: expected \"%s\", but found success: %+v", i, expectedErr, test.desc)
 			}

diff --git a/pkg/sql/catalog/tabledesc/validate_test.go b/pkg/sql/catalog/tabledesc/validate_test.go
@@ -1485,7 +1485,8 @@ func TestValidateCrossTableReferences(t *testing.T) {
 		desc := NewBuilder(&test.desc).BuildImmutable()
 		expectedErr := fmt.Sprintf("%s %q (%d): %s", desc.DescriptorType(), desc.GetName(), desc.GetID(), test.err)
 		const validateCrossReferencesOnly = catalog.ValidationLevelCrossReferences &^ (catalog.ValidationLevelCrossReferences >> 1)
-		if err := catalog.Validate(ctx, descs, validateCrossReferencesOnly, desc).CombinedError(); err == nil {
+		results := catalog.Validate(ctx, descs, catalog.NoValidationTelemetry, validateCrossReferencesOnly, desc)
+		if err := results.CombinedError(); err == nil {
 			if test.err != "" {
 				t.Errorf("%d: expected \"%s\", but found success: %+v", i, expectedErr, test.desc)
 			}