Skip to content

Commit 36fcd08

Browse files
committed
sqlstats: generalize insight.ConcurrentBufferIngester
This commit copies the ConcurrentBufferIngester from insights into the sqlstats pkg in preparation to have it consume the sql execution event and handle the writing to both sqlstats and insights systems, instead of just for insights. At this stage we are preserving how this ingester consumes events. The ingester is still only used for insights writing. Previously, the insights registry had the responsibility of buffering statements by session id and processing them once the entire transaction completes (via receicing a transaction event). The responsibility of buffering statements by session has now moved to the ingester. As a result we remove the `ObserveStatement` function from the insights registry which was only used to insert into the statement buffer. Part of: cockroachdb#141024 Epic: none Release note: None
1 parent d074347 commit 36fcd08

14 files changed

+569
-632
lines changed

Diff for: pkg/sql/conn_executor.go

+13-12
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,9 @@ type Server struct {
343343
// sqlStatsController is the control-plane interface for sqlStats.
344344
sqlStatsController *persistedsqlstats.Controller
345345

346+
// sqlStatsIngester provides the interface to consume stats about a sql execution.
347+
sqlStatsIngester *sslocal.SQLConcurrentBufferIngester
348+
346349
// schemaTelemetryController is the control-plane interface for schema
347350
// telemetry.
348351
schemaTelemetryController *schematelemetrycontroller.Controller
@@ -439,12 +442,6 @@ func NewServer(cfg *ExecutorConfig, pool *mon.BytesMonitor) *Server {
439442
metrics := makeMetrics(false /* internal */, &cfg.Settings.SV)
440443
serverMetrics := makeServerMetrics(cfg)
441444
insightsProvider := insights.New(cfg.Settings, serverMetrics.InsightsMetrics, cfg.InsightsTestingKnobs)
442-
// TODO(117690): Unify StmtStatsEnable and TxnStatsEnable into a single cluster setting.
443-
sqlstats.TxnStatsEnable.SetOnChange(&cfg.Settings.SV, func(_ context.Context) {
444-
if !sqlstats.TxnStatsEnable.Get(&cfg.Settings.SV) {
445-
insightsProvider.Writer().Clear()
446-
}
447-
})
448445
reportedSQLStats := sslocal.New(
449446
cfg.Settings,
450447
sqlstats.MaxMemReportedSQLStatsStmtFingerprints,
@@ -466,6 +463,13 @@ func NewServer(cfg *ExecutorConfig, pool *mon.BytesMonitor) *Server {
466463
reportedSQLStats,
467464
cfg.SQLStatsTestingKnobs,
468465
)
466+
sqlStatsIngester := sslocal.NewSQLConcurrentBufferIngester(insightsProvider)
467+
// TODO(117690): Unify StmtStatsEnable and TxnStatsEnable into a single cluster setting.
468+
sqlstats.TxnStatsEnable.SetOnChange(&cfg.Settings.SV, func(_ context.Context) {
469+
if !sqlstats.TxnStatsEnable.Get(&cfg.Settings.SV) {
470+
sqlStatsIngester.Clear()
471+
}
472+
})
469473
s := &Server{
470474
cfg: cfg,
471475
Metrics: metrics,
@@ -474,6 +478,7 @@ func NewServer(cfg *ExecutorConfig, pool *mon.BytesMonitor) *Server {
474478
pool: pool,
475479
localSqlStats: memSQLStats,
476480
reportedStats: reportedSQLStats,
481+
sqlStatsIngester: sqlStatsIngester,
477482
reportedStatsController: reportedSQLStatsController,
478483
insights: insightsProvider,
479484
reCache: tree.NewRegexpCache(512),
@@ -657,7 +662,7 @@ func (s *Server) Start(ctx context.Context, stopper *stop.Stopper) {
657662
// should be accounted for in their costs.
658663
ctx = multitenant.WithTenantCostControlExemption(ctx)
659664

660-
s.insights.Start(ctx, stopper)
665+
s.sqlStatsIngester.Start(ctx, stopper)
661666
s.sqlStats.Start(ctx, stopper)
662667

663668
s.schemaTelemetryController.Start(ctx, stopper)
@@ -1211,14 +1216,10 @@ func (s *Server) newConnExecutor(
12111216
ex.applicationStats = applicationStats
12121217
// We ignore statements and transactions run by the internal executor by
12131218
// passing a nil writer.
1214-
var writer *insights.ConcurrentBufferIngester
1215-
if !ex.sessionData().Internal {
1216-
writer = ex.server.insights.Writer()
1217-
}
12181219
ex.statsCollector = sslocal.NewStatsCollector(
12191220
s.cfg.Settings,
12201221
applicationStats,
1221-
writer,
1222+
s.sqlStatsIngester,
12221223
ex.phaseTimes,
12231224
s.localSqlStats.GetCounters(),
12241225
underOuterTxn,

Diff for: pkg/sql/sqlstats/insights/BUILD.bazel

-6
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ go_library(
77
srcs = [
88
"causes.go",
99
"detector.go",
10-
"ingester.go",
1110
"insights.go",
1211
"pool.go",
1312
"provider.go",
@@ -24,14 +23,12 @@ go_library(
2423
"//pkg/settings/cluster",
2524
"//pkg/sql/appstatspb",
2625
"//pkg/sql/clusterunique",
27-
"//pkg/sql/contention/contentionutils",
2826
"//pkg/sql/pgwire/pgerror",
2927
"//pkg/sql/sqlstats",
3028
"//pkg/util/cache",
3129
"//pkg/util/intsets",
3230
"//pkg/util/metric",
3331
"//pkg/util/quantile",
34-
"//pkg/util/stop",
3532
"//pkg/util/syncutil",
3633
"//pkg/util/uuid",
3734
"@com_github_cockroachdb_redact//:redact",
@@ -44,7 +41,6 @@ go_test(
4441
srcs = [
4542
"causes_test.go",
4643
"detector_test.go",
47-
"ingester_test.go",
4844
"insights_test.go",
4945
"registry_test.go",
5046
"store_test.go",
@@ -59,8 +55,6 @@ go_test(
5955
"//pkg/sql/pgwire/pgcode",
6056
"//pkg/sql/pgwire/pgerror",
6157
"//pkg/sql/sqlstats",
62-
"//pkg/util/leaktest",
63-
"//pkg/util/log",
6458
"//pkg/util/stop",
6559
"//pkg/util/uint128",
6660
"//pkg/util/uuid",

Diff for: pkg/sql/sqlstats/insights/ingester_test.go

-278
This file was deleted.

0 commit comments

Comments
 (0)