From 0d1cb9d2996726a63e6dcf8338a3d844fbf8fcff Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 29 Apr 2022 14:43:36 +1000 Subject: [PATCH 1/9] tree: remove ALTER TYPE references to telemetry Release note: None --- pkg/sql/alter_type.go | 2 +- pkg/sql/sem/tree/alter_type.go | 46 +++++++++++++++------------------- 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/pkg/sql/alter_type.go b/pkg/sql/alter_type.go index fcc75ce388ed..3d562be75a77 100644 --- a/pkg/sql/alter_type.go +++ b/pkg/sql/alter_type.go @@ -93,7 +93,7 @@ func (p *planner) AlterType(ctx context.Context, n *tree.AlterType) (planNode, e } func (n *alterTypeNode) startExec(params runParams) error { - telemetry.Inc(n.n.Cmd.TelemetryCounter()) + telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra("type", n.n.Cmd.TelemetryName())) typeName := tree.AsStringWithFQNames(n.n.Type, params.p.Ann()) eventLogDone := false diff --git a/pkg/sql/sem/tree/alter_type.go b/pkg/sql/sem/tree/alter_type.go index 23da3897a299..fa179cf8d4cb 100644 --- a/pkg/sql/sem/tree/alter_type.go +++ b/pkg/sql/sem/tree/alter_type.go @@ -10,11 +10,6 @@ package tree -import ( - "github.com/cockroachdb/cockroach/pkg/server/telemetry" - "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" -) - // AlterType represents an ALTER TYPE statement. type AlterType struct { Type *UnresolvedObjectName @@ -32,9 +27,8 @@ func (node *AlterType) Format(ctx *FmtCtx) { type AlterTypeCmd interface { NodeFormatter alterTypeCmd() - // TelemetryCounter returns the telemetry counter to increment - // when this command is used. - TelemetryCounter() telemetry.Counter + // TelemetryName returns the counter name to use for telemetry purposes. + TelemetryName() string } func (*AlterTypeAddValue) alterTypeCmd() {} @@ -75,9 +69,9 @@ func (node *AlterTypeAddValue) Format(ctx *FmtCtx) { } } -// TelemetryCounter implements the AlterTypeCmd interface. -func (node *AlterTypeAddValue) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("type", "add_value") +// TelemetryName implements the AlterTypeCmd interface. +func (node *AlterTypeAddValue) TelemetryName() string { + return "add_value" } // AlterTypeAddValuePlacement represents the placement clause for an ALTER @@ -101,9 +95,9 @@ func (node *AlterTypeRenameValue) Format(ctx *FmtCtx) { ctx.FormatNode(&node.NewVal) } -// TelemetryCounter implements the AlterTypeCmd interface. -func (node *AlterTypeRenameValue) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("type", "rename_value") +// TelemetryName implements the AlterTypeCmd interface. +func (node *AlterTypeRenameValue) TelemetryName() string { + return "rename_value" } // AlterTypeDropValue represents an ALTER TYPE DROP VALUE command. @@ -117,9 +111,9 @@ func (node *AlterTypeDropValue) Format(ctx *FmtCtx) { ctx.FormatNode(&node.Val) } -// TelemetryCounter implements the AlterTypeCmd interface. -func (node *AlterTypeDropValue) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("type", "drop_value") +// TelemetryName implements the AlterTypeCmd interface. +func (node *AlterTypeDropValue) TelemetryName() string { + return "drop_value" } // AlterTypeRename represents an ALTER TYPE RENAME command. @@ -133,9 +127,9 @@ func (node *AlterTypeRename) Format(ctx *FmtCtx) { ctx.FormatNode(&node.NewName) } -// TelemetryCounter implements the AlterTypeCmd interface. -func (node *AlterTypeRename) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("type", "rename") +// TelemetryName implements the AlterTypeCmd interface. +func (node *AlterTypeRename) TelemetryName() string { + return "rename" } // AlterTypeSetSchema represents an ALTER TYPE SET SCHEMA command. @@ -149,9 +143,9 @@ func (node *AlterTypeSetSchema) Format(ctx *FmtCtx) { ctx.FormatNode(&node.Schema) } -// TelemetryCounter implements the AlterTypeCmd interface. -func (node *AlterTypeSetSchema) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("type", "set_schema") +// TelemetryName implements the AlterTypeCmd interface. +func (node *AlterTypeSetSchema) TelemetryName() string { + return "set_schema" } // AlterTypeOwner represents an ALTER TYPE OWNER TO command. @@ -165,7 +159,7 @@ func (node *AlterTypeOwner) Format(ctx *FmtCtx) { ctx.FormatNode(&node.Owner) } -// TelemetryCounter implements the AlterTypeCmd interface. -func (node *AlterTypeOwner) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("type", "owner") +// TelemetryName implements the AlterTypeCmd interface. +func (node *AlterTypeOwner) TelemetryName() string { + return "owner" } From 00ccea0fcd942977964e01fd43a1294913af820f Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 29 Apr 2022 14:48:03 +1000 Subject: [PATCH 2/9] tree: move REFRESH MATERIALIZED / DROP VIEW telemetry reference Release note: None --- pkg/sql/drop_view.go | 5 ++++- pkg/sql/refresh_materialized_view.go | 3 ++- pkg/sql/sem/tree/create.go | 8 -------- pkg/sql/sem/tree/drop.go | 14 -------------- 4 files changed, 6 insertions(+), 24 deletions(-) diff --git a/pkg/sql/drop_view.go b/pkg/sql/drop_view.go index 45d49d713c66..c1a6a47da354 100644 --- a/pkg/sql/drop_view.go +++ b/pkg/sql/drop_view.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" "github.com/cockroachdb/errors" @@ -90,7 +91,9 @@ func (p *planner) DropView(ctx context.Context, n *tree.DropView) (planNode, err func (n *dropViewNode) ReadingOwnWrites() {} func (n *dropViewNode) startExec(params runParams) error { - telemetry.Inc(n.n.TelemetryCounter()) + telemetry.Inc(sqltelemetry.SchemaChangeDropCounter( + tree.GetTableType(false /* isSequence */, true /* isView */, n.n.IsMaterialized), + )) ctx := params.ctx for _, toDel := range n.td { diff --git a/pkg/sql/refresh_materialized_view.go b/pkg/sql/refresh_materialized_view.go index db46b896a4bc..ff813a570a55 100644 --- a/pkg/sql/refresh_materialized_view.go +++ b/pkg/sql/refresh_materialized_view.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" ) type refreshMaterializedViewNode struct { @@ -79,7 +80,7 @@ func (n *refreshMaterializedViewNode) startExec(params runParams) error { // results of the view query into the new set of indexes, and then change the // set of indexes over to the new set of indexes atomically. - telemetry.Inc(n.n.TelemetryCounter()) + telemetry.Inc(sqltelemetry.SchemaRefreshMaterializedView) // Inform the user that CONCURRENTLY is not needed. if n.n.Concurrently { diff --git a/pkg/sql/sem/tree/create.go b/pkg/sql/sem/tree/create.go index 96fdef78168a..927eac412664 100644 --- a/pkg/sql/sem/tree/create.go +++ b/pkg/sql/sem/tree/create.go @@ -24,12 +24,10 @@ import ( "strconv" "strings" - "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/lexbase" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/roleoption" - "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/pretty" "github.com/cockroachdb/errors" @@ -1984,12 +1982,6 @@ type RefreshMaterializedView struct { RefreshDataOption RefreshDataOption } -// TelemetryCounter returns the telemetry counter to increment -// when this command is used. -func (node *RefreshMaterializedView) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaRefreshMaterializedView -} - // RefreshDataOption corresponds to arguments for the REFRESH MATERIALIZED VIEW // statement. type RefreshDataOption int diff --git a/pkg/sql/sem/tree/drop.go b/pkg/sql/sem/tree/drop.go index ab0cf2bb2cc4..5b34871fdbdc 100644 --- a/pkg/sql/sem/tree/drop.go +++ b/pkg/sql/sem/tree/drop.go @@ -19,11 +19,6 @@ package tree -import ( - "github.com/cockroachdb/cockroach/pkg/server/telemetry" - "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" -) - // DropBehavior represents options for dropping schema elements. type DropBehavior int @@ -133,15 +128,6 @@ func (node *DropView) Format(ctx *FmtCtx) { } } -// TelemetryCounter returns the telemetry counter to increment -// when this command is used. -func (node *DropView) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeDropCounter( - GetTableType( - false /* isSequence */, true, /* isView */ - node.IsMaterialized)) -} - // DropSequence represents a DROP SEQUENCE statement. type DropSequence struct { Names TableNames From 5711b04f683ed33c8abc23394cbaf43e68b330f3 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 29 Apr 2022 14:54:13 +1000 Subject: [PATCH 3/9] tree: fix SET/RESET storage parameter to use the correct telemetry tag Woops - wrong name used! Intending to backport this to v22.1. Release note: None --- pkg/sql/sem/tree/alter_table.go | 4 ++-- pkg/sql/testdata/telemetry/ttl | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/sql/sem/tree/alter_table.go b/pkg/sql/sem/tree/alter_table.go index 705bfb86e45a..817eb0223c73 100644 --- a/pkg/sql/sem/tree/alter_table.go +++ b/pkg/sql/sem/tree/alter_table.go @@ -617,7 +617,7 @@ type AlterTableSetStorageParams struct { // TelemetryCounter returns the telemetry counter to increment // when this command is used. func (node *AlterTableSetStorageParams) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounter("set_storage_param") + return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "set_storage_param") } // Format implements the NodeFormatter interface. @@ -635,7 +635,7 @@ type AlterTableResetStorageParams struct { // TelemetryCounter returns the telemetry counter to increment // when this command is used. func (node *AlterTableResetStorageParams) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounter("set_storage_param") + return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "set_storage_param") } // Format implements the NodeFormatter interface. diff --git a/pkg/sql/testdata/telemetry/ttl b/pkg/sql/testdata/telemetry/ttl index ce4683235247..40a856178e7d 100644 --- a/pkg/sql/testdata/telemetry/ttl +++ b/pkg/sql/testdata/telemetry/ttl @@ -1,4 +1,5 @@ feature-allowlist +sql.schema.alter_table\..* sql.schema.table_storage_parameter.* sql.row_level_ttl.created sql.row_level_ttl.dropped @@ -14,21 +15,25 @@ sql.schema.table_storage_parameter.ttl_select_batch_size.set feature-usage ALTER TABLE tbl SET (ttl_delete_batch_size = 200) ---- +sql.schema.alter_table.set_storage_param sql.schema.table_storage_parameter.ttl_delete_batch_size.set feature-usage ALTER TABLE tbl RESET (ttl_select_batch_size) ---- +sql.schema.alter_table.set_storage_param sql.schema.table_storage_parameter.ttl_select_batch_size.reset feature-usage ALTER TABLE tbl RESET (ttl) ---- sql.row_level_ttl.dropped +sql.schema.alter_table.set_storage_param sql.schema.table_storage_parameter.ttl.reset feature-usage ALTER TABLE tbl SET (ttl_expire_after = '10 hours') ---- sql.row_level_ttl.created +sql.schema.alter_table.set_storage_param sql.schema.table_storage_parameter.ttl_expire_after.set From f10515928be87eddba48fbb7f009c9f754117c9a Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 29 Apr 2022 15:06:46 +1000 Subject: [PATCH 4/9] tree: fix ALTER TABLE references to telemetry Release note: None --- pkg/sql/alter_table.go | 2 +- pkg/sql/alter_table_owner.go | 6 +- pkg/sql/alter_table_set_schema.go | 6 +- pkg/sql/sem/tree/alter_table.go | 144 ++++++++++++++---------------- 4 files changed, 79 insertions(+), 79 deletions(-) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index b6fb95028376..58755b7a9aa6 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -165,7 +165,7 @@ func (n *alterTableNode) startExec(params runParams) error { } for i, cmd := range n.n.Cmds { - telemetry.Inc(cmd.TelemetryCounter()) + telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra("table", cmd.TelemetryName())) if !n.tableDesc.HasPrimaryKey() && !isAlterCmdValidWithoutPrimaryKey(cmd) { return errors.Newf("table %q does not have a primary key, cannot perform%s", n.tableDesc.Name, tree.AsString(cmd)) diff --git a/pkg/sql/alter_table_owner.go b/pkg/sql/alter_table_owner.go index 07d5f16c8959..7aa38926b9b4 100644 --- a/pkg/sql/alter_table_owner.go +++ b/pkg/sql/alter_table_owner.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/decodeusername" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" ) @@ -79,7 +80,10 @@ func (p *planner) AlterTableOwner(ctx context.Context, n *tree.AlterTableOwner) } func (n *alterTableOwnerNode) startExec(params runParams) error { - telemetry.Inc(n.n.TelemetryCounter()) + telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra( + tree.GetTableType(n.n.IsSequence, n.n.IsView, n.n.IsMaterialized), + n.n.TelemetryName(), + )) ctx := params.ctx p := params.p tableDesc := n.desc diff --git a/pkg/sql/alter_table_set_schema.go b/pkg/sql/alter_table_set_schema.go index ccdf97683660..6db32ee8d9a6 100644 --- a/pkg/sql/alter_table_set_schema.go +++ b/pkg/sql/alter_table_set_schema.go @@ -21,6 +21,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" ) @@ -96,7 +97,10 @@ func (p *planner) AlterTableSetSchema( } func (n *alterTableSetSchemaNode) startExec(params runParams) error { - telemetry.Inc(n.n.TelemetryCounter()) + telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra( + tree.GetTableType(n.n.IsSequence, n.n.IsView, n.n.IsMaterialized), + n.n.TelemetryName(), + )) ctx := params.ctx p := params.p tableDesc := n.tableDesc diff --git a/pkg/sql/sem/tree/alter_table.go b/pkg/sql/sem/tree/alter_table.go index 817eb0223c73..0bd400032b6d 100644 --- a/pkg/sql/sem/tree/alter_table.go +++ b/pkg/sql/sem/tree/alter_table.go @@ -51,9 +51,8 @@ func (node *AlterTableCmds) Format(ctx *FmtCtx) { // AlterTableCmd represents a table modification operation. type AlterTableCmd interface { NodeFormatter - // TelemetryCounter returns the telemetry counter to increment - // when this command is used. - TelemetryCounter() telemetry.Counter + // TelemetryName returns the counter name to use for telemetry purposes. + TelemetryName() string // Placeholder function to ensure that only desired types // (AlterTable*) conform to the AlterTableCmd interface. alterTableCmd() @@ -113,9 +112,9 @@ type AlterTableAddColumn struct { ColumnDef *ColumnTableDef } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableAddColumn) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "add_column") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableAddColumn) TelemetryName() string { + return "add_column" } // Format implements the NodeFormatter interface. @@ -209,9 +208,9 @@ type AlterTableAddConstraint struct { ValidationBehavior ValidationBehavior } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableAddConstraint) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "add_constraint") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableAddConstraint) TelemetryName() string { + return "add_constraint" } // Format implements the NodeFormatter interface. @@ -231,9 +230,9 @@ type AlterTableAlterColumnType struct { Using Expr } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableAlterColumnType) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "alter_column_type") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableAlterColumnType) TelemetryName() string { + return "alter_column_type" } // Format implements the NodeFormatter interface. @@ -265,9 +264,9 @@ type AlterTableAlterPrimaryKey struct { StorageParams StorageParams } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableAlterPrimaryKey) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "alter_primary_key") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableAlterPrimaryKey) TelemetryName() string { + return "alter_primary_key" } // Format implements the NodeFormatter interface. @@ -287,9 +286,9 @@ type AlterTableDropColumn struct { DropBehavior DropBehavior } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableDropColumn) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "drop_column") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableDropColumn) TelemetryName() string { + return "drop_column" } // Format implements the NodeFormatter interface. @@ -311,9 +310,9 @@ type AlterTableDropConstraint struct { DropBehavior DropBehavior } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableDropConstraint) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "drop_constraint") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableDropConstraint) TelemetryName() string { + return "drop_constraint" } // Format implements the NodeFormatter interface. @@ -333,9 +332,9 @@ type AlterTableValidateConstraint struct { Constraint Name } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableValidateConstraint) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "validate_constraint") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableValidateConstraint) TelemetryName() string { + return "validate_constraint" } // Format implements the NodeFormatter interface. @@ -350,9 +349,9 @@ type AlterTableRenameColumn struct { NewName Name } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableRenameColumn) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "rename_column") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableRenameColumn) TelemetryName() string { + return "rename_column" } // Format implements the NodeFormatter interface. @@ -369,9 +368,9 @@ type AlterTableRenameConstraint struct { NewName Name } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableRenameConstraint) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "rename_constraint") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableRenameConstraint) TelemetryName() string { + return "rename_constraint" } // Format implements the NodeFormatter interface. @@ -394,9 +393,9 @@ func (node *AlterTableSetDefault) GetColumn() Name { return node.Column } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableSetDefault) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "set_default") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableSetDefault) TelemetryName() string { + return "set_default" } // Format implements the NodeFormatter interface. @@ -423,9 +422,9 @@ func (node *AlterTableSetOnUpdate) GetColumn() Name { return node.Column } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableSetOnUpdate) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "set_on_update") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableSetOnUpdate) TelemetryName() string { + return "set_on_update" } // Format implements the NodeFormatter interface. @@ -451,9 +450,9 @@ func (node *AlterTableSetVisible) GetColumn() Name { return node.Column } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableSetVisible) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "set_visible") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableSetVisible) TelemetryName() string { + return "set_visible" } // Format implements the NodeFormatter interface. @@ -478,9 +477,9 @@ func (node *AlterTableSetNotNull) GetColumn() Name { return node.Column } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableSetNotNull) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "set_not_null") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableSetNotNull) TelemetryName() string { + return "set_not_null" } // Format implements the NodeFormatter interface. @@ -501,9 +500,9 @@ func (node *AlterTableDropNotNull) GetColumn() Name { return node.Column } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableDropNotNull) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "drop_not_null") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableDropNotNull) TelemetryName() string { + return "drop_not_null" } // Format implements the NodeFormatter interface. @@ -524,9 +523,9 @@ func (node *AlterTableDropStored) GetColumn() Name { return node.Column } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableDropStored) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "drop_stored") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableDropStored) TelemetryName() string { + return "drop_stored" } // Format implements the NodeFormatter interface. @@ -542,9 +541,9 @@ type AlterTablePartitionByTable struct { *PartitionByTable } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTablePartitionByTable) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "partition_by") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTablePartitionByTable) TelemetryName() string { + return "partition_by" } // Format implements the NodeFormatter interface. @@ -582,9 +581,9 @@ type AlterTableSetAudit struct { Mode AuditMode } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableSetAudit) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "set_audit") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableSetAudit) TelemetryName() string { + return "set_audit" } // Format implements the NodeFormatter interface. @@ -598,9 +597,9 @@ type AlterTableInjectStats struct { Stats Expr } -// TelemetryCounter implements the AlterTableCmd interface. -func (node *AlterTableInjectStats) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "inject_stats") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableInjectStats) TelemetryName() string { + return "inject_stats" } // Format implements the NodeFormatter interface. @@ -614,10 +613,9 @@ type AlterTableSetStorageParams struct { StorageParams StorageParams } -// TelemetryCounter returns the telemetry counter to increment -// when this command is used. -func (node *AlterTableSetStorageParams) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "set_storage_param") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableSetStorageParams) TelemetryName() string { + return "set_storage_param" } // Format implements the NodeFormatter interface. @@ -632,10 +630,9 @@ type AlterTableResetStorageParams struct { Params NameList } -// TelemetryCounter returns the telemetry counter to increment -// when this command is used. -func (node *AlterTableResetStorageParams) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "set_storage_param") +// TelemetryName implements the AlterTableCmd interface. +func (node *AlterTableResetStorageParams) TelemetryName() string { + return "set_storage_param" } // Format implements the NodeFormatter interface. @@ -696,12 +693,10 @@ func (node *AlterTableSetSchema) Format(ctx *FmtCtx) { ctx.FormatNode(&node.Schema) } -// TelemetryCounter returns the telemetry counter to increment +// TelemetryName returns the telemetry counter to increment // when this command is used. -func (node *AlterTableSetSchema) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra( - GetTableType(node.IsSequence, node.IsView, node.IsMaterialized), - "set_schema") +func (node *AlterTableSetSchema) TelemetryName() string { + return "set_schema" } // AlterTableOwner represents an ALTER TABLE OWNER TO command. @@ -714,13 +709,10 @@ type AlterTableOwner struct { IsSequence bool } -// TelemetryCounter returns the telemetry counter to increment +// TelemetryName returns the telemetry counter to increment // when this command is used. -func (node *AlterTableOwner) TelemetryCounter() telemetry.Counter { - return sqltelemetry.SchemaChangeAlterCounterWithExtra( - GetTableType(node.IsSequence, node.IsView, node.IsMaterialized), - "owner_to", - ) +func (node *AlterTableOwner) TelemetryName() string { + return "owner_to" } // Format implements the NodeFormatter interface. From 5bebeebf7bd52ecce2e40a6639e55cd9811acbaa Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 29 Apr 2022 15:11:41 +1000 Subject: [PATCH 5/9] tree: move HoistConstraints telemetry reference Release note: none --- pkg/sql/alter_table.go | 4 +++- .../schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel | 1 + .../scbuild/internal/scbuildstmt/alter_table.go | 6 +++++- pkg/sql/sem/tree/alter_table.go | 7 ++----- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 58755b7a9aa6..7588e365661f 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -98,7 +98,9 @@ func (p *planner) AlterTable(ctx context.Context, n *tree.AlterTable) (planNode, tree.Name(tableDesc.GetName()), tree.Name(tableDesc.GetName())) } - n.HoistAddColumnConstraints() + n.HoistAddColumnConstraints(func() { + telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "add_column.references")) + }) // See if there's any "inject statistics" in the query and type check the // expressions. diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel index 11387b7ff2ad..66aee372ee9b 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel @@ -19,6 +19,7 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scbuild/internal/scbuildstmt", visibility = ["//pkg/sql/schemachanger/scbuild:__subpackages__"], deps = [ + "//pkg/server/telemetry", "//pkg/settings/cluster", "//pkg/sql/catalog", "//pkg/sql/catalog/catpb", diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go index 6b35c4e4e41b..1fbea3770b47 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table.go @@ -13,12 +13,14 @@ package scbuildstmt import ( "reflect" + "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/errors" ) @@ -52,7 +54,9 @@ func init() { func AlterTable(b BuildCtx, n *tree.AlterTable) { // Hoist the constraints to separate clauses because other code assumes that // that is how the commands will look. - n.HoistAddColumnConstraints() + n.HoistAddColumnConstraints(func() { + telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "add_column.references")) + }) // Check if an entry exists for the statement type, in which // case. It's either fully or partially supported. Check the commands // first, since we don't want to do extra work in this transaction diff --git a/pkg/sql/sem/tree/alter_table.go b/pkg/sql/sem/tree/alter_table.go index 0bd400032b6d..fc698272e986 100644 --- a/pkg/sql/sem/tree/alter_table.go +++ b/pkg/sql/sem/tree/alter_table.go @@ -13,9 +13,7 @@ package tree import ( "strings" - "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/lex" - "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" ) // AlterTable represents an ALTER TABLE statement. @@ -145,8 +143,7 @@ func (node *AlterTableAddColumn) Format(ctx *FmtCtx) { // CockroachDB and Postgres, but not necessarily other SQL databases: // // ALTER TABLE t ADD COLUMN a INT CHECK (a < b) -// -func (node *AlterTable) HoistAddColumnConstraints() { +func (node *AlterTable) HoistAddColumnConstraints(onHoistedFKConstraint func()) { var normalizedCmds AlterTableCmds for _, cmd := range node.Cmds { @@ -185,7 +182,7 @@ func (node *AlterTable) HoistAddColumnConstraints() { } normalizedCmds = append(normalizedCmds, constraint) d.References.Table = nil - telemetry.Inc(sqltelemetry.SchemaChangeAlterCounterWithExtra("table", "add_column.references")) + onHoistedFKConstraint() } } } From 94fe056cbc7f1245266ca7ea293650b370ee239a Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 29 Apr 2022 15:18:01 +1000 Subject: [PATCH 6/9] tree: move builtins telemetry counter to builtins Release note: None --- pkg/sql/sem/builtins/all_builtins.go | 8 +++++++- pkg/sql/sem/tree/function_definition.go | 7 +------ pkg/sql/sem/tree/overload.go | 6 ++---- pkg/sql/sem/tree/type_check.go | 4 ++-- 4 files changed, 12 insertions(+), 13 deletions(-) diff --git a/pkg/sql/sem/builtins/all_builtins.go b/pkg/sql/sem/builtins/all_builtins.go index 562602c4acbe..88490ebc2046 100644 --- a/pkg/sql/sem/builtins/all_builtins.go +++ b/pkg/sql/sem/builtins/all_builtins.go @@ -14,7 +14,9 @@ import ( "fmt" "sort" + "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/sql/types" ) @@ -59,7 +61,7 @@ func init() { } else if def.props.Class == tree.WindowClass { AllWindowBuiltinNames = append(AllWindowBuiltinNames, name) } - for _, overload := range def.overloads { + for i, overload := range def.overloads { fnCount := 0 if overload.Fn != nil { fnCount++ @@ -84,6 +86,10 @@ func init() { name, fnCount, )) } + c := sqltelemetry.BuiltinCounter(name, overload.Signature(false)) + def.overloads[i].OnTypeCheck = func() { + telemetry.Inc(c) + } } } diff --git a/pkg/sql/sem/tree/function_definition.go b/pkg/sql/sem/tree/function_definition.go index e2054b207a62..d3572cdfb37f 100644 --- a/pkg/sql/sem/tree/function_definition.go +++ b/pkg/sql/sem/tree/function_definition.go @@ -10,10 +10,7 @@ package tree -import ( - "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" - "github.com/lib/pq/oid" -) +import "github.com/lib/pq/oid" // FunctionDefinition implements a reference to the (possibly several) // overloads for a built-in function. @@ -162,8 +159,6 @@ func NewFunctionDefinition( // Builtins with a preferred overload are always ambiguous. props.AmbiguousReturnType = true } - // Produce separate telemetry for each overload. - def[i].counter = sqltelemetry.BuiltinCounter(name, def[i].Signature(false)) overloads[i] = &def[i] } diff --git a/pkg/sql/sem/tree/overload.go b/pkg/sql/sem/tree/overload.go index 8dced1a07a17..e033c494f3b0 100644 --- a/pkg/sql/sem/tree/overload.go +++ b/pkg/sql/sem/tree/overload.go @@ -17,7 +17,6 @@ import ( "math" "strings" - "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/volatility" @@ -134,9 +133,8 @@ type Overload struct { // statement which will be executed as a common table expression in the query. SQLFn SQLFnOverload - // counter, if non-nil, should be incremented upon successful - // type check of expressions using this overload. - counter telemetry.Counter + // OnTypeCheck is incremented every time this overload is type checked. + OnTypeCheck func() // SpecializedVecBuiltin is used to let the vectorized engine // know when an Overload has a specialized vectorized operator. diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index 5d91d242d081..4a2878c366ba 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -1209,8 +1209,8 @@ func (expr *FuncExpr) TypeCheck( if err := semaCtx.checkVolatility(overloadImpl.Volatility); err != nil { return nil, pgerror.Wrapf(err, pgcode.InvalidParameterValue, "%s()", def.Name) } - if overloadImpl.counter != nil { - telemetry.Inc(overloadImpl.counter) + if overloadImpl.OnTypeCheck != nil { + overloadImpl.OnTypeCheck() } return expr, nil } From c4967586e2780279ac87721b58cb2e3aba140bab Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 29 Apr 2022 15:33:24 +1000 Subject: [PATCH 7/9] tree: move operator telemetry initialization to sql package Release note: None --- pkg/sql/sem/tree/eval.go | 14 ++---- pkg/sql/sem/tree/expr.go | 7 +-- pkg/sql/sem/tree/operators.go | 86 ---------------------------------- pkg/sql/sem/tree/type_check.go | 15 +++--- pkg/sql/telemetry.go | 66 ++++++++++++++++++++++++++ 5 files changed, 80 insertions(+), 108 deletions(-) delete mode 100644 pkg/sql/sem/tree/operators.go create mode 100644 pkg/sql/telemetry.go diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index 96c0a2dab4c9..f52e93d8d498 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -14,7 +14,6 @@ import ( "fmt" "github.com/cockroachdb/cockroach/pkg/geo" - "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/cast" @@ -69,9 +68,8 @@ type UnaryOp struct { types TypeList retType ReturnTyper - // counter, if non-nil, should be incremented every time the - // operator is type checked. - counter telemetry.Counter + // OnTypeCheck is called when the op is type checked. + OnTypeCheck func() } func (op *UnaryOp) params() TypeList { @@ -224,9 +222,7 @@ type BinOp struct { types TypeList retType ReturnTyper - // counter, if non-nil, should be incremented every time the - // operator is type checked. - counter telemetry.Counter + OnTypeCheck func() } func (op *BinOp) params() TypeList { @@ -1289,9 +1285,7 @@ type CmpOp struct { // Datum return type is a union between *DBool and dNull. EvalOp BinaryEvalOp - // counter, if non-nil, should be incremented every time the - // operator is type checked. - counter telemetry.Counter + OnTypeCheck func() // If NullableArgs is false, the operator returns NULL // whenever either argument is NULL. diff --git a/pkg/sql/sem/tree/expr.go b/pkg/sql/sem/tree/expr.go index 504709ad2023..c3e631a70d7d 100644 --- a/pkg/sql/sem/tree/expr.go +++ b/pkg/sql/sem/tree/expr.go @@ -1129,7 +1129,8 @@ const ( var _ = NumUnaryOperatorSymbols -var unaryOpName = [...]string{ +// UnaryOpName is the mapping of unary operators to names. +var UnaryOpName = [...]string{ UnaryMinus: "-", UnaryPlus: "+", UnaryComplement: "~", @@ -1138,10 +1139,10 @@ var unaryOpName = [...]string{ } func (i UnaryOperatorSymbol) String() string { - if i > UnaryOperatorSymbol(len(unaryOpName)-1) { + if i > UnaryOperatorSymbol(len(UnaryOpName)-1) { return fmt.Sprintf("UnaryOp(%d)", i) } - return unaryOpName[i] + return UnaryOpName[i] } // UnaryExpr represents a unary value expression. diff --git a/pkg/sql/sem/tree/operators.go b/pkg/sql/sem/tree/operators.go deleted file mode 100644 index cba5e912c7ce..000000000000 --- a/pkg/sql/sem/tree/operators.go +++ /dev/null @@ -1,86 +0,0 @@ -// Copyright 2019 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package tree - -import ( - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treebin" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp" - "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" - "github.com/cockroachdb/errors" -) - -// This file implements the generation of unique names for every -// operator overload. -// -// The historical first purpose of generating these names is to be used -// as telemetry keys, for feature usage reporting. - -// 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 -// checking every scalar operator node. - -// The logic that follows is also associated with a related feature in -// PostgreSQL, which may be implemented by CockroachDB in the future: -// exposing all the operators as unambiguous, non-overloaded built-in -// functions. For example, in PostgreSQL, one can use `SELECT -// int8um(123)` to apply the int8-specific unary minus operator. -// This feature can be considered in the future for two reasons: -// -// 1. some pg applications may simply require the ability to use the -// pg native operator built-ins. If/when this compatibility is -// considered, care should be taken to tweak the string maps below -// to ensure that the operator names generated here coincide with -// those used in the postgres library. -// -// 2. since the operator built-in functions are non-overloaded, they -// remove the requirement to disambiguate the type of operands -// with the ::: (annotate_type) operator. This may be useful -// to simplify/accelerate the serialization of scalar expressions -// in distsql. -// - -func init() { - // Label the unary operators. - for op, overloads := range UnaryOps { - if int(op) >= len(unaryOpName) || unaryOpName[op] == "" { - panic(errors.AssertionFailedf("missing name for operator %q", op.String())) - } - opName := unaryOpName[op] - for _, impl := range overloads { - o := impl.(*UnaryOp) - o.counter = sqltelemetry.UnaryOpCounter(opName, o.Typ.String()) - } - } - - // Label the comparison operators. - for op, overloads := range CmpOps { - opName := treecmp.ComparisonOpName(op) - for _, impl := range overloads { - o := impl.(*CmpOp) - lname := o.LeftType.String() - rname := o.RightType.String() - o.counter = sqltelemetry.CmpOpCounter(opName, lname, rname) - } - } - - // Label the binary operators. - for op, overloads := range BinOps { - opName := treebin.BinaryOpName(op) - for _, impl := range overloads { - o := impl.(*BinOp) - lname := o.LeftType.String() - rname := o.RightType.String() - o.counter = sqltelemetry.BinOpCounter(opName, lname, rname) - } - } -} diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index 4a2878c366ba..42360c63a7c0 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -353,9 +353,8 @@ func (expr *BinaryExpr) TypeCheck( return nil, pgerror.Wrapf(err, pgcode.InvalidParameterValue, "%s", expr.Operator) } - // Register operator usage in telemetry. - if binOp.counter != nil { - telemetry.Inc(binOp.counter) + if binOp.OnTypeCheck != nil { + binOp.OnTypeCheck() } expr.Left, expr.Right = leftTyped, rightTyped @@ -883,9 +882,8 @@ func (expr *ComparisonExpr) TypeCheck( return nil, pgerror.Wrapf(err, pgcode.InvalidParameterValue, "%s", expr.Operator) } - // Register operator usage in telemetry. - if cmpOp.counter != nil { - telemetry.Inc(cmpOp.counter) + if cmpOp.OnTypeCheck != nil { + cmpOp.OnTypeCheck() } expr.Left, expr.Right = leftTyped, rightTyped @@ -1496,9 +1494,8 @@ func (expr *UnaryExpr) TypeCheck( return nil, pgerror.Wrapf(err, pgcode.InvalidParameterValue, "%s", expr.Operator) } - // Register operator usage in telemetry. - if unaryOp.counter != nil { - telemetry.Inc(unaryOp.counter) + if unaryOp.OnTypeCheck != nil { + unaryOp.OnTypeCheck() } expr.Expr = exprTyped diff --git a/pkg/sql/telemetry.go b/pkg/sql/telemetry.go new file mode 100644 index 000000000000..654ad42a2869 --- /dev/null +++ b/pkg/sql/telemetry.go @@ -0,0 +1,66 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package sql + +import ( + "github.com/cockroachdb/cockroach/pkg/server/telemetry" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treebin" + "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp" + "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/errors" +) + +// init injects the requisite telemetry components into the tree package. +// We pre-allocate the counter objects upfront here and later use +// Inc(), to avoid the hash map lookup in telemetry.Count upon type +// checking every scalar operator node. +func init() { + for op, overloads := range tree.UnaryOps { + if int(op) >= len(tree.UnaryOpName) || tree.UnaryOpName[op] == "" { + panic(errors.AssertionFailedf("missing name for operator %q", op.String())) + } + opName := tree.UnaryOpName[op] + for _, impl := range overloads { + o := impl.(*tree.UnaryOp) + c := sqltelemetry.UnaryOpCounter(opName, o.Typ.String()) + o.OnTypeCheck = func() { + telemetry.Inc(c) + } + } + } + + for op, overloads := range tree.BinOps { + opName := treebin.BinaryOpName(op) + for _, impl := range overloads { + o := impl.(*tree.BinOp) + lname := o.LeftType.String() + rname := o.RightType.String() + c := sqltelemetry.BinOpCounter(opName, lname, rname) + o.OnTypeCheck = func() { + telemetry.Inc(c) + } + } + } + + for op, overloads := range tree.CmpOps { + opName := treecmp.ComparisonOpName(op) + for _, impl := range overloads { + o := impl.(*tree.CmpOp) + lname := o.LeftType.String() + rname := o.RightType.String() + c := sqltelemetry.CmpOpCounter(opName, lname, rname) + o.OnTypeCheck = func() { + telemetry.Inc(c) + } + } + } +} From 5134c64dd360bd771963bda6e68ea09077b1a93d Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 29 Apr 2022 15:40:35 +1000 Subject: [PATCH 8/9] sql: move some assorted TypeCheck telemetry incrementers to tree Release note: None --- pkg/sql/sem/tree/type_check.go | 25 +++++++++++++++++++++---- pkg/sql/telemetry.go | 13 +++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index 42360c63a7c0..d4dd65fc6442 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -31,6 +31,15 @@ import ( "golang.org/x/text/language" ) +// OnTypeCheck* functions are hooks which get called if not nil and the +// type checking of the relevant function is made. +var ( + OnTypeCheckArraySubscript func() + OnTypeCheckIfErr func() + OnTypeCheckArrayConstructor func() + OnTypeCheckArrayFlatten func() +) + // SemaContext defines the context in which to perform semantic analysis on an // expression syntax tree. type SemaContext struct { @@ -667,7 +676,9 @@ func (expr *IndirectionExpr) TypeCheck( expr.Expr = subExpr expr.typ = typ.ArrayContents() - telemetry.Inc(sqltelemetry.ArraySubscriptCounter) + if OnTypeCheckArraySubscript != nil { + OnTypeCheckArraySubscript() + } return expr, nil } @@ -1248,7 +1259,9 @@ func (expr *IfErrExpr) TypeCheck( expr.ErrCode = typedErrCode expr.typ = retType - telemetry.Inc(sqltelemetry.IfErrCounter) + if OnTypeCheckIfErr != nil { + OnTypeCheckIfErr() + } return expr, nil } @@ -1604,7 +1617,9 @@ func (expr *Array) TypeCheck( expr.Exprs[i] = typedSubExprs[i] } - telemetry.Inc(sqltelemetry.ArrayConstructorCounter) + if OnTypeCheckArrayConstructor != nil { + OnTypeCheckArrayConstructor() + } return expr, nil } @@ -1624,7 +1639,9 @@ func (expr *ArrayFlatten) TypeCheck( expr.Subquery = subqueryTyped expr.typ = types.MakeArray(subqueryTyped.ResolvedType()) - telemetry.Inc(sqltelemetry.ArrayFlattenCounter) + if OnTypeCheckArrayFlatten != nil { + OnTypeCheckArrayFlatten() + } return expr, nil } diff --git a/pkg/sql/telemetry.go b/pkg/sql/telemetry.go index 654ad42a2869..d112a897fe29 100644 --- a/pkg/sql/telemetry.go +++ b/pkg/sql/telemetry.go @@ -63,4 +63,17 @@ func init() { } } } + + tree.OnTypeCheckIfErr = func() { + telemetry.Inc(sqltelemetry.IfErrCounter) + } + tree.OnTypeCheckArrayConstructor = func() { + telemetry.Inc(sqltelemetry.ArrayConstructorCounter) + } + tree.OnTypeCheckArraySubscript = func() { + telemetry.Inc(sqltelemetry.ArraySubscriptCounter) + } + tree.OnTypeCheckArrayFlatten = func() { + telemetry.Inc(sqltelemetry.ArrayFlattenCounter) + } } From c2138b765a55c571b708fe53b04aa3c6432617e5 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 29 Apr 2022 17:12:11 +1000 Subject: [PATCH 9/9] tree: move cast telemetry counter to sql Make the map of counters public and only referencing a function, and move initialization to the sql package. Release note: None --- pkg/sql/BUILD.bazel | 1 + pkg/sql/sem/tree/BUILD.bazel | 3 -- pkg/sql/sem/tree/type_check.go | 56 +++++++++++----------------------- pkg/sql/telemetry.go | 22 +++++++++++++ 4 files changed, 40 insertions(+), 42 deletions(-) diff --git a/pkg/sql/BUILD.bazel b/pkg/sql/BUILD.bazel index 0ca73310f421..530e805da185 100644 --- a/pkg/sql/BUILD.bazel +++ b/pkg/sql/BUILD.bazel @@ -215,6 +215,7 @@ go_library( "tablewriter_insert.go", "tablewriter_update.go", "tablewriter_upsert_opt.go", + "telemetry.go", "telemetry_logging.go", "temporary_schema.go", "tenant.go", diff --git a/pkg/sql/sem/tree/BUILD.bazel b/pkg/sql/sem/tree/BUILD.bazel index 18261937a2cd..40f60275b074 100644 --- a/pkg/sql/sem/tree/BUILD.bazel +++ b/pkg/sql/sem/tree/BUILD.bazel @@ -62,7 +62,6 @@ go_library( "name_part.go", "name_resolution.go", "object_name.go", - "operators.go", "overload.go", "parse_array.go", "parse_string.go", # keep @@ -120,7 +119,6 @@ go_library( deps = [ "//pkg/geo", "//pkg/geo/geopb", - "//pkg/server/telemetry", "//pkg/sql/lex", "//pkg/sql/lexbase", "//pkg/sql/pgwire/pgcode", @@ -135,7 +133,6 @@ go_library( "//pkg/sql/sem/tree/treewindow", "//pkg/sql/sem/volatility", "//pkg/sql/sessiondatapb", - "//pkg/sql/sqltelemetry", "//pkg/sql/types", "//pkg/util", "//pkg/util/bitarray", diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index d4dd65fc6442..730379ebee7b 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -15,13 +15,11 @@ import ( "fmt" "strings" - "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/sem/cast" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp" "github.com/cockroachdb/cockroach/pkg/sql/sem/volatility" - "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/duration" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" @@ -451,7 +449,7 @@ func resolveCast( if err != nil { return err } - telemetry.Inc(getCastCounter(fromFamily, toFamily)) + onCastTypeCheckHook(fromFamily, toFamily) return nil case toFamily == types.EnumFamily && fromFamily == types.EnumFamily: @@ -460,7 +458,7 @@ func resolveCast( if !castFrom.Equivalent(castTo) { return invalidCastError(castFrom, castTo) } - telemetry.Inc(getCastCounter(fromFamily, toFamily)) + onCastTypeCheckHook(fromFamily, toFamily) return nil case toFamily == types.TupleFamily && fromFamily == types.TupleFamily: @@ -489,7 +487,7 @@ func resolveCast( return err } } - telemetry.Inc(getCastCounter(fromFamily, toFamily)) + onCastTypeCheckHook(fromFamily, toFamily) return nil default: @@ -505,48 +503,28 @@ func resolveCast( } return err } - telemetry.Inc(getCastCounter(fromFamily, toFamily)) + onCastTypeCheckHook(fromFamily, toFamily) return nil } } -// castCounterType represents a cast from one family to another. -type castCounterType struct { - from, to types.Family +// CastCounterType represents a cast from one family to another. +type CastCounterType struct { + From, To types.Family } -// castCounterMap is a map of cast counter types to their corresponding -// telemetry counters. -var castCounters map[castCounterType]telemetry.Counter +// OnCastTypeCheck is a map of CastCounterTypes to their hook. +var OnCastTypeCheck map[CastCounterType]func() -// Initialize castCounters. -func init() { - castCounters = make(map[castCounterType]telemetry.Counter) - for fromID := range types.Family_name { - for toID := range types.Family_name { - from := types.Family(fromID) - to := types.Family(toID) - var c telemetry.Counter - switch { - case from == types.ArrayFamily && to == types.ArrayFamily: - c = sqltelemetry.ArrayCastCounter - case from == types.TupleFamily && to == types.TupleFamily: - c = sqltelemetry.TupleCastCounter - case from == types.EnumFamily && to == types.EnumFamily: - c = sqltelemetry.EnumCastCounter - default: - c = sqltelemetry.CastOpCounter(from.Name(), to.Name()) - } - castCounters[castCounterType{from, to}] = c - } +// onCastTypeCheckHook performs the registered hook for the given cast on type check. +func onCastTypeCheckHook(from, to types.Family) { + // The given map has not been populated, so do not expect any telemetry. + if OnCastTypeCheck == nil { + return } -} - -// getCastCounter returns the telemetry counter for the cast from one family to -// another family. -func getCastCounter(from, to types.Family) telemetry.Counter { - if c, ok := castCounters[castCounterType{from, to}]; ok { - return c + if f, ok := OnCastTypeCheck[CastCounterType{From: from, To: to}]; ok { + f() + return } panic(errors.AssertionFailedf( "no cast counter found for cast from %s to %s", from.Name(), to.Name(), diff --git a/pkg/sql/telemetry.go b/pkg/sql/telemetry.go index d112a897fe29..2f17533b07f9 100644 --- a/pkg/sql/telemetry.go +++ b/pkg/sql/telemetry.go @@ -16,6 +16,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treebin" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/errors" ) @@ -76,4 +77,25 @@ func init() { tree.OnTypeCheckArrayFlatten = func() { telemetry.Inc(sqltelemetry.ArrayFlattenCounter) } + tree.OnCastTypeCheck = make(map[tree.CastCounterType]func()) + for fromID := range types.Family_name { + for toID := range types.Family_name { + from := types.Family(fromID) + to := types.Family(toID) + var c telemetry.Counter + switch { + case from == types.ArrayFamily && to == types.ArrayFamily: + c = sqltelemetry.ArrayCastCounter + case from == types.TupleFamily && to == types.TupleFamily: + c = sqltelemetry.TupleCastCounter + case from == types.EnumFamily && to == types.EnumFamily: + c = sqltelemetry.EnumCastCounter + default: + c = sqltelemetry.CastOpCounter(from.Name(), to.Name()) + } + tree.OnCastTypeCheck[tree.CastCounterType{From: from, To: to}] = func() { + telemetry.Inc(c) + } + } + } }