sqltelemetry: make SQL telemetry more uniform, w/ linter

pkg/cmd/docgen/funcs.go

@@ -267,12 +267,14 @@ func linkArguments(t string) string {
 
 func linkTypeName(s string) string {
 	s = strings.TrimSuffix(s, "{}")
+	s = strings.TrimSuffix(s, "{*}")
 	name := s
 	switch s {
 	case "timestamptz":
 		s = "timestamp"
 	}
 	s = strings.TrimSuffix(s, "[]")
+	s = strings.TrimSuffix(s, "*")
 	switch s {
 	case "int", "decimal", "float", "bool", "date", "timestamp", "interval", "string", "bytes",
 		"inet", "uuid", "collatedstring", "time":

pkg/server/telemetry/features.go

@@ -64,6 +64,18 @@ func Inc(c Counter) {
 	atomic.AddInt32(c, 1)
 }
 
+// GetCounterOnce returns a counter from the global registry,
+// and asserts it didn't exist previously.
+func GetCounterOnce(feature string) Counter {
+	counters.RLock()
+	_, ok := counters.m[feature]
+	counters.RUnlock()
+	if ok {
+		panic("counter already exists: " + feature)
+	}
+	return GetCounter(feature)
+}
+
 // GetCounter returns a counter from the global registry.
 func GetCounter(feature string) Counter {
 	counters.RLock()

pkg/server/updates_test.go

@@ -545,12 +545,12 @@ func TestReportUsage(t *testing.T) {
 
 		// Although the query is executed 10 times, due to plan caching
 		// keyed by the SQL text, the planning only occurs once.
-		"sql.ops.cast.text::inet":  1,
-		"sql.ops.bin.jsonb - text": 1,
-		"sql.builtins.crdb_internal.force_assertion_error(msg: string) -> int": 1,
-		"sql.ops.array.ind":     1,
-		"sql.ops.array.cons":    1,
-		"sql.ops.array.flatten": 1,
+		"sql.plan.ops.cast.string::inet":                                            1,
+		"sql.plan.ops.bin.jsonb - string":                                           1,
+		"sql.plan.builtins.crdb_internal.force_assertion_error(msg: string) -> int": 1,
+		"sql.plan.ops.array.ind":                                                    1,
+		"sql.plan.ops.array.cons":                                                   1,
+		"sql.plan.ops.array.flatten":                                                1,
 
 		"unimplemented.#33285.json_object_agg":          10,
 		"unimplemented.pg_catalog.pg_stat_wal_receiver": 10,

pkg/sql/pgwire/server.go

@@ -33,6 +33,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirebase"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/util/contextutil"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
@@ -478,7 +479,7 @@ func (s *Server) ServeConn(ctx context.Context, conn net.Conn) error {
 
 	if version != version30 {
 		if version == versionCancel {
-			telemetry.Count("pgwire.unimplemented.cancel_request")
+			telemetry.Inc(sqltelemetry.CancelRequestCounter)
 			_ = conn.Close()
 			return nil
 		}
@@ -563,7 +564,8 @@ func parseOptions(ctx context.Context, data []byte) (sql.SessionArgs, error) {
 			} else {
 				if !exists {
 					if _, ok := sql.UnsupportedVars[key]; ok {
-						telemetry.Count("unimplemented.pgwire.parameter." + key)
+						counter := sqltelemetry.UnimplementedClientStatusParameterCounter(key)
+						telemetry.Inc(counter)
 					}
 					log.Warningf(ctx, "unknown configuration parameter: %q", key)
 				} else {

pkg/sql/pgwire/types.go

@@ -31,6 +31,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/types"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
 	"github.com/cockroachdb/cockroach/pkg/util/ipaddr"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -275,7 +276,7 @@ func (b *writeBuffer) writeBinaryDatum(
 				// The above encoding is not correct for Infinity, but since that encoding
 				// doesn't exist in postgres, it's unclear what to do. For now use the NaN
 				// encoding and count it to see if anyone even needs this.
-				telemetry.Count("pgwire.#32489.binary_decimal_infinity")
+				telemetry.Inc(sqltelemetry.BinaryDecimalInfinityCounter)
 			}
 
 			return

pkg/sql/sem/tree/eval.go

@@ -36,6 +36,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/types"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/util/arith"
 	"github.com/cockroachdb/cockroach/pkg/util/bitarray"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
@@ -1339,7 +1340,7 @@ var BinOps = map[BinaryOperator]binOpOverload{
 			Fn: func(_ *EvalContext, left Datum, right Datum) (Datum, error) {
 				rval := MustBeDInt(right)
 				if rval < 0 || rval >= 64 {
-					telemetry.Count("sql.large_lshift_argument")
+					telemetry.Inc(sqltelemetry.LargeLShiftArgumentCounter)
 					return nil, pgerror.NewErrorf(pgerror.CodeInvalidParameterValueError, "shift argument out of range")
 				}
 				return NewDInt(MustBeDInt(left) << uint(rval)), nil
@@ -1377,7 +1378,7 @@ var BinOps = map[BinaryOperator]binOpOverload{
 			Fn: func(_ *EvalContext, left Datum, right Datum) (Datum, error) {
 				rval := MustBeDInt(right)
 				if rval < 0 || rval >= 64 {
-					telemetry.Count("sql.large_rshift_argument")
+					telemetry.Inc(sqltelemetry.LargeRShiftArgumentCounter)
 					return nil, pgerror.NewErrorf(pgerror.CodeInvalidParameterValueError, "shift argument out of range")
 				}
 				return NewDInt(MustBeDInt(left) >> uint(rval)), nil

pkg/sql/sem/tree/function_definition.go

@@ -14,7 +14,7 @@
 
 package tree
 
-import "github.com/cockroachdb/cockroach/pkg/server/telemetry"
+import "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 
 // FunctionDefinition implements a reference to the (possibly several)
 // overloads for a built-in function.
@@ -126,7 +126,7 @@ func NewFunctionDefinition(
 			props.AmbiguousReturnType = true
 		}
 		// Produce separate telemetry for each overload.
-		def[i].counter = telemetry.GetCounter("sql.builtins." + name + def[i].Signature(false))
+		def[i].counter = sqltelemetry.BuiltinCounter(name, def[i].Signature(false))
 
 		overloads[i] = &def[i]
 	}

pkg/sql/sem/tree/operators.go

@@ -16,11 +16,9 @@ package tree
 
 import (
 	"fmt"
-	"strings"
 
-	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/types"
-	"github.com/lib/pq/oid"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 )
 
 // This file implements the generation of unique names for every
@@ -29,22 +27,10 @@ import (
 // The historical first purpose of generating these names is to be used
 // as telemetry keys, for feature usage reporting.
 
-// Scalar operators will be counter as sql.ops.<kind>.<lhstype> <opname> <rhstype>.
-const unOpCounterNameFmt = "sql.ops.un.%s %s"
-const cmpOpCounterNameFmt = "sql.ops.cmp.%s %s %s"
-const binOpCounterNameFmt = "sql.ops.bin.%s %s %s"
-
-// Cast operators will be counted as sql.ops.cast.<fromtype>::<totype>.
-const castCounterNameFmt = "sql.ops.cast.%s::%s"
-
-// All casts that involve arrays will also be counted towards this
-// feature counter.
-var arrayCastCounter = telemetry.GetCounter("sql.ops.cast.arrays")
-
 // Detailed counter name generation follows.
 //
 // We pre-allocate the counter objects upfront here and later use
-// Inc(), to avoid the hash map lookup in telemetry.Count() upon type
+// Inc(), to avoid the hash map lookup in telemetry.Count upon type
 // checking every scalar operator node.
 
 // The logic that follows is also associated with a related feature in
@@ -67,22 +53,7 @@ var arrayCastCounter = telemetry.GetCounter("sql.ops.cast.arrays")
 //    in distsql.
 //
 
-// makeOidName generates a short name for the given type OID.
-func makeOidName(op fmt.Stringer, tOid oid.Oid) (string, bool) {
-	oidName, ok := oid.TypeName[tOid]
-	if !ok {
-		return "", false
-	}
-	name := strings.ToLower(oidName)
-	if strings.HasPrefix(name, "timestamp") {
-		name = "ts" + name[9:]
-	}
-	return name, true
-}
-
 func init() {
-	seen := map[string]struct{}{}
-
 	// Label the unary operators.
 	for op, overloads := range UnaryOps {
 		if int(op) >= len(unaryOpName) || unaryOpName[op] == "" {
@@ -91,15 +62,7 @@ func init() {
 		opName := unaryOpName[op]
 		for _, impl := range overloads {
 			o := impl.(*UnaryOp)
-			uname, ok := makeOidName(op, o.Typ.Oid())
-			if !ok {
-				continue
-			}
-			name := fmt.Sprintf(unOpCounterNameFmt, opName, uname)
-			if _, ok := seen[name]; ok {
-				panic(fmt.Sprintf("duplicate name: %q", name))
-			}
-			o.counter = telemetry.GetCounter(name)
+			o.counter = sqltelemetry.UnaryOpCounter(opName, o.Typ.String())
 		}
 	}
 
@@ -111,16 +74,9 @@ func init() {
 		opName := comparisonOpName[op]
 		for _, impl := range overloads {
 			o := impl.(*CmpOp)
-			lname, lok := makeOidName(op, o.LeftType.Oid())
-			rname, rok := makeOidName(op, o.RightType.Oid())
-			if !lok || !rok {
-				continue
-			}
-			name := fmt.Sprintf(cmpOpCounterNameFmt, lname, opName, rname)
-			if _, ok := seen[name]; ok {
-				panic(fmt.Sprintf("duplicate name: %q", name))
-			}
-			o.counter = telemetry.GetCounter(name)
+			lname := o.LeftType.String()
+			rname := o.RightType.String()
+			o.counter = sqltelemetry.CmpOpCounter(opName, lname, rname)
 		}
 	}
 
@@ -132,16 +88,9 @@ func init() {
 		opName := binaryOpName[op]
 		for _, impl := range overloads {
 			o := impl.(*BinOp)
-			lname, lok := makeOidName(op, o.LeftType.Oid())
-			rname, rok := makeOidName(op, o.RightType.Oid())
-			if !lok || !rok {
-				continue
-			}
-			name := fmt.Sprintf(binOpCounterNameFmt, lname, opName, rname)
-			if _, ok := seen[name]; ok {
-				panic(fmt.Sprintf("duplicate name: %q", name))
-			}
-			o.counter = telemetry.GetCounter(name)
+			lname := o.LeftType.String()
+			rname := o.RightType.String()
+			o.counter = sqltelemetry.BinOpCounter(opName, lname, rname)
 		}
 	}
 }
@@ -153,29 +102,11 @@ func annotateCast(toType types.T, fromTypes []types.T) []castInfo {
 	for i, fromType := range fromTypes {
 		ci[i].fromT = fromType
 	}
-	var rname string
-	if toType.FamilyEqual(types.FamArray) {
-		rname = "array"
-	} else {
-		var rok bool
-		rname, rok = makeOidName(nil, toType.Oid())
-		if !rok {
-			return ci
-		}
-	}
+	rname := toType.String()
 
 	for i, fromType := range fromTypes {
-		var lname string
-		if fromType.FamilyEqual(types.FamArray) {
-			lname = "array"
-		} else {
-			var lok bool
-			lname, lok = makeOidName(nil, fromType.Oid())
-			if !lok {
-				continue
-			}
-		}
-		ci[i].counter = telemetry.GetCounter(fmt.Sprintf(castCounterNameFmt, lname, rname))
+		lname := fromType.String()
+		ci[i].counter = sqltelemetry.CastOpCounter(lname, rname)
 	}
 	return ci
 }

pkg/sql/sem/tree/type_check.go

@@ -25,6 +25,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/types"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/cockroach/pkg/util/duration"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -413,7 +414,7 @@ func isCastDeepValid(castFrom, castTo types.T) (bool, telemetry.Counter) {
 	if castTo.FamilyEqual(types.FamArray) && castFrom.FamilyEqual(types.FamArray) {
 		ok, c := isCastDeepValid(castFrom.(types.TArray).Typ, castTo.(types.TArray).Typ)
 		if ok {
-			telemetry.Inc(arrayCastCounter)
+			telemetry.Inc(sqltelemetry.ArrayCastCounter)
 		}
 		return ok, c
 	}
@@ -474,8 +475,6 @@ func (expr *CastExpr) TypeCheck(ctx *SemaContext, _ types.T) (TypedExpr, error)
 	return nil, pgerror.NewErrorf(pgerror.CodeCannotCoerceError, "invalid cast: %s -> %s", castFrom, expr.Type)
 }
 
-var arraySubscriptTelemetryCounter = telemetry.GetCounter("sql.ops.array.ind")
-
 // TypeCheck implements the Expr interface.
 func (expr *IndirectionExpr) TypeCheck(ctx *SemaContext, desired types.T) (TypedExpr, error) {
 	for i, t := range expr.Indirection {
@@ -505,7 +504,7 @@ func (expr *IndirectionExpr) TypeCheck(ctx *SemaContext, desired types.T) (Typed
 	expr.Expr = subExpr
 	expr.typ = arrType.Typ
 
-	telemetry.Inc(arraySubscriptTelemetryCounter)
+	telemetry.Inc(sqltelemetry.ArraySubscriptCounter)
 	return expr, nil
 }
 
@@ -971,8 +970,6 @@ func (f *WindowFrame) TypeCheck(ctx *SemaContext, windowDef *WindowDef) error {
 	return nil
 }
 
-var ifErrTelemetryCounter = telemetry.GetCounter("sql.ops.iferr")
-
 // TypeCheck implements the Expr interface.
 func (expr *IfErrExpr) TypeCheck(ctx *SemaContext, desired types.T) (TypedExpr, error) {
 	var typedCond, typedElse TypedExpr
@@ -1006,7 +1003,7 @@ func (expr *IfErrExpr) TypeCheck(ctx *SemaContext, desired types.T) (TypedExpr,
 	expr.ErrCode = typedErrCode
 	expr.typ = retType
 
-	telemetry.Inc(ifErrTelemetryCounter)
+	telemetry.Inc(sqltelemetry.IfErrCounter)
 	return expr, nil
 }
 
@@ -1267,8 +1264,6 @@ func (expr *Tuple) TypeCheck(ctx *SemaContext, desired types.T) (TypedExpr, erro
 var errAmbiguousArrayType = pgerror.NewErrorf(pgerror.CodeIndeterminateDatatypeError, "cannot determine type of empty array. "+
 	"Consider annotating with the desired type, for example ARRAY[]:::int[]")
 
-var arrayConstructorTelemetryCounter = telemetry.GetCounter("sql.ops.array.cons")
-
 // TypeCheck implements the Expr interface.
 func (expr *Array) TypeCheck(ctx *SemaContext, desired types.T) (TypedExpr, error) {
 	desiredParam := types.Any
@@ -1294,12 +1289,10 @@ func (expr *Array) TypeCheck(ctx *SemaContext, desired types.T) (TypedExpr, erro
 		expr.Exprs[i] = typedSubExprs[i]
 	}
 
-	telemetry.Inc(arrayConstructorTelemetryCounter)
+	telemetry.Inc(sqltelemetry.ArrayConstructorCounter)
 	return expr, nil
 }
 
-var arrayFlattenTelemetryCounter = telemetry.GetCounter("sql.ops.array.flatten")
-
 // TypeCheck implements the Expr interface.
 func (expr *ArrayFlatten) TypeCheck(ctx *SemaContext, desired types.T) (TypedExpr, error) {
 	desiredParam := types.Any
@@ -1314,7 +1307,7 @@ func (expr *ArrayFlatten) TypeCheck(ctx *SemaContext, desired types.T) (TypedExp
 	expr.Subquery = subqueryTyped
 	expr.typ = types.TArray{Typ: subqueryTyped.ResolvedType()}
 
-	telemetry.Inc(arrayFlattenTelemetryCounter)
+	telemetry.Inc(sqltelemetry.ArrayFlattenCounter)
 	return expr, nil
 }