sqlstats/insights: extract proto into a separate package

This allows us to break dependency of `serverpb` on `sql/parser` (which
was recently introduced when we added support for SQL Commenter query
tags).

Release note: None

Author: yuzefovich

pkg/BUILD.bazel

@@ -360,6 +360,7 @@ ALL_TESTS = [
     "//pkg/server/pgurl:pgurl_test",
     "//pkg/server/privchecker:privchecker_test",
     "//pkg/server/profiler:profiler_test",
+    "//pkg/server/serverpb:serverpb_disallowed_imports_test",
     "//pkg/server/serverpb:serverpb_test",
     "//pkg/server/serverrules:serverrules_test",
     "//pkg/server/settingswatcher:settingswatcher_test",
@@ -2373,6 +2374,7 @@ GO_TARGETS = [
     "//pkg/sql/sqlstats/insights/integration:integration_test",
     "//pkg/sql/sqlstats/insights:insights",
     "//pkg/sql/sqlstats/insights:insights_test",
+    "//pkg/sql/sqlstats/insightspb:insightspb",
     "//pkg/sql/sqlstats/persistedsqlstats/sqlstatstestutil:sqlstatstestutil",
     "//pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil:sqlstatsutil",
     "//pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil:sqlstatsutil_test",

pkg/gen/protobuf.bzl

@@ -72,7 +72,7 @@ PROTOBUF_SRCS = [
     "//pkg/sql/sem/idxtype:idxtype_go_proto",
     "//pkg/sql/sem/semenumpb:semenumpb_go_proto",
     "//pkg/sql/sessiondatapb:sessiondatapb_go_proto",
-    "//pkg/sql/sqlstats/insights:insights_go_proto",
+    "//pkg/sql/sqlstats/insightspb:insightspb_go_proto",
     "//pkg/sql/sqlstats/persistedsqlstats:persistedsqlstats_go_proto",
     "//pkg/sql/stats:stats_go_proto",
     "//pkg/sql/types:types_go_proto",

pkg/protos.bzl

@@ -38,7 +38,7 @@ SERVER_PROTOS = [
     "//pkg/sql/sem/idxtype:idxtype_proto",
     "//pkg/sql/sem/semenumpb:semenumpb_proto",
     "//pkg/sql/sessiondatapb:sessiondatapb_proto",
-    "//pkg/sql/sqlstats/insights:insights_proto",
+    "//pkg/sql/sqlstats/insightspb:insightspb_proto",
     "//pkg/sql/types:types_proto",
     "//pkg/sql/vecindex/vecpb:vecpb_proto",
     "//pkg/storage/enginepb:enginepb_proto",
@@ -132,7 +132,7 @@ PROTO_FILES = [
     "//pkg/sql/sessiondatapb:session_data.proto",
     "//pkg/sql/sessiondatapb:session_migration.proto",
     "//pkg/sql/sessiondatapb:session_revival_token.proto",
-    "//pkg/sql/sqlstats/insights:insights.proto",
+    "//pkg/sql/sqlstats/insightspb:insights.proto",
     "//pkg/sql/types:types.proto",
     "//pkg/sql/vecindex/vecpb:vec.proto",
     "//pkg/storage/enginepb:file_registry.proto",

pkg/server/BUILD.bazel

@@ -278,7 +278,7 @@ go_library(
         "//pkg/sql/sqlliveness/slinstance",
         "//pkg/sql/sqlliveness/slprovider",
         "//pkg/sql/sqlstats",
-        "//pkg/sql/sqlstats/insights",
+        "//pkg/sql/sqlstats/insightspb",
         "//pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil",
         "//pkg/sql/sqlstats/sslocal",
         "//pkg/sql/stats",
@@ -567,6 +567,7 @@ go_test(
         "//pkg/sql/sessiondata",
         "//pkg/sql/sqlstats",
         "//pkg/sql/sqlstats/insights",
+        "//pkg/sql/sqlstats/insightspb",
         "//pkg/sql/sqlstats/persistedsqlstats/sqlstatstestutil",
         "//pkg/sql/tablemetadatacache",
         "//pkg/sql/tablemetadatacache/util",

pkg/server/serverpb/BUILD.bazel

@@ -3,6 +3,7 @@
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
 load("@rules_proto//proto:defs.bzl", "proto_library")
+load("//pkg/testutils:buildutil/buildutil.bzl", "disallowed_imports_test")
 
 proto_library(
     name = "serverpb_proto",
@@ -33,7 +34,7 @@ proto_library(
         "//pkg/server/status/statuspb:statuspb_proto",
         "//pkg/sql/appstatspb:appstatspb_proto",
         "//pkg/sql/contentionpb:contentionpb_proto",
-        "//pkg/sql/sqlstats/insights:insights_proto",
+        "//pkg/sql/sqlstats/insightspb:insightspb_proto",
         "//pkg/storage/enginepb:enginepb_proto",
         "//pkg/ts/catalog:catalog_proto",
         "//pkg/util:util_proto",
@@ -88,7 +89,7 @@ go_proto_library(
         "//pkg/server/status/statuspb",
         "//pkg/sql/appstatspb",
         "//pkg/sql/contentionpb",
-        "//pkg/sql/sqlstats/insights",
+        "//pkg/sql/sqlstats/insightspb",
         "//pkg/storage/enginepb",
         "//pkg/ts/catalog",
         "//pkg/util",
@@ -126,3 +127,8 @@ go_test(
     embed = [":serverpb"],
     deps = ["@com_github_stretchr_testify//assert"],
 )
+
+disallowed_imports_test(
+    "serverpb",
+    disallowed_list = ["//pkg/sql/parser"],
+)

pkg/server/serverpb/status.proto

@@ -23,7 +23,7 @@ import "server/serverpb/index_recommendations.proto";
 import "server/status/statuspb/status.proto";
 import "sql/appstatspb/app_stats.proto";
 import "sql/contentionpb/contention.proto";
-import "sql/sqlstats/insights/insights.proto";
+import "sql/sqlstats/insightspb/insights.proto";
 import "storage/enginepb/key_registry.proto";
 import "storage/enginepb/mvcc.proto";
 import "storage/enginepb/stats.proto";

pkg/server/status.go

@@ -74,7 +74,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlinstance"
-	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insights"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insightspb"
 	"github.com/cockroachdb/cockroach/pkg/util/grpcutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/httputil"
@@ -395,18 +395,18 @@ func (b *baseStatusServer) localExecutionInsights(
 ) (*serverpb.ListExecutionInsightsResponse, error) {
 	var response serverpb.ListExecutionInsightsResponse
 
-	highContentionInsights := make(map[uuid.UUID]insights.Insight, 0)
+	highContentionInsights := make(map[uuid.UUID]insightspb.Insight, 0)
 	reader := b.sqlServer.pgServer.SQLServer.GetInsightsReader()
-	reader.IterateInsights(ctx, func(ctx context.Context, insight *insights.Insight) {
+	reader.IterateInsights(ctx, func(ctx context.Context, insight *insightspb.Insight) {
 		if insight == nil {
 			return
 		}
 
 		// Copy statements slice - these insights objects can be read concurrently.
 		insightsCopy := *insight
-		insightsCopy.Statements = make([]*insights.Statement, len(insight.Statements))
+		insightsCopy.Statements = make([]*insightspb.Statement, len(insight.Statements))
 		copy(insightsCopy.Statements, insight.Statements)
-		if insight.Transaction != nil && slices.Contains(insight.Transaction.Causes, insights.Cause_HighContention) {
+		if insight.Transaction != nil && slices.Contains(insight.Transaction.Causes, insightspb.Cause_HighContention) {
 			// Collect high contention insights seperately, they need additional validation / filtering.
 			// If it is valid we will add it to the response later.
 			highContentionInsights[insightsCopy.Transaction.ID] = insightsCopy
@@ -437,7 +437,7 @@ func (b *baseStatusServer) localExecutionInsights(
 // events that are related to the passed in (contention) insights. The insights that have
 // valid (resolved BlockingTxnFingerprintID) contention events are returned.
 func validContentionInsights(
-	registry *contention.Registry, contentionInsights map[uuid.UUID]insights.Insight,
+	registry *contention.Registry, contentionInsights map[uuid.UUID]insightspb.Insight,
 ) (map[uuid.UUID]bool, error) {
 	valid := make(map[uuid.UUID]bool, len(contentionInsights))
 	err := registry.ForEachEvent(func(event *contentionpb.ExtendedContentionEvent) error {

pkg/server/status_test.go

@@ -30,6 +30,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/contention"
 	"github.com/cockroachdb/cockroach/pkg/sql/contentionpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insights"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insightspb"
 	tablemetadatacacheutil "github.com/cockroachdb/cockroach/pkg/sql/tablemetadatacache/util"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -534,7 +535,7 @@ func TestLocalExecutionInsights(t *testing.T) {
 		id1 := uuid.MakeV4()
 		id2 := uuid.MakeV4()
 		id3 := uuid.MakeV4()
-		insights := map[uuid.UUID]insights.Insight{
+		insights := map[uuid.UUID]insightspb.Insight{
 			id1: {},
 			id2: {},
 			id3: {},

pkg/sql/sqlstats/insights/BUILD.bazel

@@ -1,5 +1,3 @@
-load("@rules_proto//proto:defs.bzl", "proto_library")
-load("@io_bazel_rules_go//proto:def.bzl", "go_proto_library")
 load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
@@ -14,7 +12,6 @@ go_library(
         "store.go",
         "util.go",
     ],
-    embed = [":insights_go_proto"],
     importpath = "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insights",
     visibility = ["//visibility:public"],
     deps = [
@@ -25,6 +22,7 @@ go_library(
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/sqlcommenter",
         "//pkg/sql/sqlstats",
+        "//pkg/sql/sqlstats/insightspb",
         "//pkg/util/cache",
         "//pkg/util/intsets",
         "//pkg/util/metric",
@@ -55,33 +53,10 @@ go_test(
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/sqlstats",
+        "//pkg/sql/sqlstats/insightspb",
         "//pkg/util/stop",
         "//pkg/util/uint128",
         "//pkg/util/uuid",
         "@com_github_stretchr_testify//require",
     ],
 )
-
-proto_library(
-    name = "insights_proto",
-    srcs = ["insights.proto"],
-    strip_import_prefix = "/pkg",
-    visibility = ["//visibility:public"],
-    deps = [
-        "@com_github_gogo_protobuf//gogoproto:gogo_proto",
-        "@com_google_protobuf//:duration_proto",
-        "@com_google_protobuf//:timestamp_proto",
-    ],
-)
-
-go_proto_library(
-    name = "insights_go_proto",
-    compilers = ["//pkg/cmd/protoc-gen-gogoroach:protoc-gen-gogoroach_compiler"],
-    importpath = "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insights",
-    proto = ":insights_proto",
-    visibility = ["//visibility:public"],
-    deps = [
-        "//pkg/util/uuid",  # keep
-        "@com_github_gogo_protobuf//gogoproto",
-    ],
-)

pkg/sql/sqlstats/insights/causes.go

@@ -5,26 +5,31 @@
 
 package insights
 
-import "github.com/cockroachdb/cockroach/pkg/settings/cluster"
+import (
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/insightspb"
+)
 
 type causes struct {
 	st *cluster.Settings
 }
 
 // examine will append all causes of the statement's problems to buf and
 // return the result. Buf allows the slice to be pooled.
-func (c *causes) examine(buf []Cause, stmt *Statement) (result []Cause) {
+func (c *causes) examine(
+	buf []insightspb.Cause, stmt *insightspb.Statement,
+) (result []insightspb.Cause) {
 	result = buf
 	if len(stmt.IndexRecommendations) > 0 {
-		result = append(result, Cause_SuboptimalPlan)
+		result = append(result, insightspb.Cause_SuboptimalPlan)
 	}
 
 	if stmt.Contention != nil && *stmt.Contention >= LatencyThreshold.Get(&c.st.SV) {
-		result = append(result, Cause_HighContention)
+		result = append(result, insightspb.Cause_HighContention)
 	}
 
 	if stmt.Retries >= HighRetryCountThreshold.Get(&c.st.SV) {
-		result = append(result, Cause_HighRetryCount)
+		result = append(result, insightspb.Cause_HighRetryCount)
 	}
 
 	return result