From 9698b43c17240216375914089425ec8a5e044bde Mon Sep 17 00:00:00 2001
From: Andrii Vorobiov <and.vorobiov@gmail.com>
Date: Fri, 21 Apr 2023 00:55:44 +0300
Subject: [PATCH] server: fix proper call for StartHotRangesLoggingScheduler

This patch fixes an issue when structlogging.StartHotRangesLoggingScheduler
is guarded by `DiagnosticsReportingEnabled` cluster setting and doesn't
properly handles changes of this setting.
Now, when diagnostics reporting is enabled via cluster setting, Hot range
logging scheduler starts (if not started yet), and cancels previously
started scheduler if setting is disabled.

Release note: None
---
 pkg/server/server.go                       | 11 ++++++++---
 pkg/server/structlogging/BUILD.bazel       |  1 +
 pkg/server/structlogging/hot_ranges_log.go | 12 +++++++-----
 pkg/server/tenant.go                       | 11 ++++++++---
 4 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/pkg/server/server.go b/pkg/server/server.go
index 28630b1a8d47..50cd8fa47759 100644
--- a/pkg/server/server.go
+++ b/pkg/server/server.go
@@ -103,7 +103,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/goschedstats"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/netutil"
@@ -2055,8 +2054,14 @@ func (s *Server) AcceptClients(ctx context.Context) error {
 		return err
 	}
 
-	if logcrash.DiagnosticsReportingEnabled.Get(&s.ClusterSettings().SV) {
-		structlogging.StartHotRangesLoggingScheduler(ctx, s.stopper, s.status, *s.sqlServer.internalExecutor, s.ClusterSettings())
+	if err := structlogging.StartHotRangesLoggingScheduler(
+		ctx,
+		s.stopper,
+		s.status,
+		*s.sqlServer.internalExecutor,
+		s.ClusterSettings(),
+	); err != nil {
+		return err
 	}
 
 	s.sqlServer.isReady.Set(true)
diff --git a/pkg/server/structlogging/BUILD.bazel b/pkg/server/structlogging/BUILD.bazel
index 3c1f6b034ad1..ae7afd66e8bb 100644
--- a/pkg/server/structlogging/BUILD.bazel
+++ b/pkg/server/structlogging/BUILD.bazel
@@ -13,6 +13,7 @@ go_library(
         "//pkg/sql",
         "//pkg/util/log",
         "//pkg/util/log/eventpb",
+        "//pkg/util/log/logcrash",
         "//pkg/util/log/logpb",
         "//pkg/util/log/logutil",
         "//pkg/util/stop",
diff --git a/pkg/server/structlogging/hot_ranges_log.go b/pkg/server/structlogging/hot_ranges_log.go
index 3c17e64200c1..de1c137f5c98 100644
--- a/pkg/server/structlogging/hot_ranges_log.go
+++ b/pkg/server/structlogging/hot_ranges_log.go
@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
+	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logpb"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logutil"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
@@ -67,17 +68,18 @@ func StartHotRangesLoggingScheduler(
 	sServer serverpb.TenantStatusServer,
 	ie sql.InternalExecutor,
 	st *cluster.Settings,
-) {
+) error {
 	scheduler := hotRangesLoggingScheduler{
 		ie:      ie,
 		sServer: sServer,
 		st:      st,
 	}
-	scheduler.start(ctx, stopper)
+
+	return scheduler.start(ctx, stopper)
 }
 
-func (s *hotRangesLoggingScheduler) start(ctx context.Context, stopper *stop.Stopper) {
-	_ = stopper.RunAsyncTask(ctx, "hot-ranges-stats", func(ctx context.Context) {
+func (s *hotRangesLoggingScheduler) start(ctx context.Context, stopper *stop.Stopper) error {
+	return stopper.RunAsyncTask(ctx, "hot-ranges-stats", func(ctx context.Context) {
 		ticker := time.NewTicker(TelemetryHotRangesStatsInterval.Get(&s.st.SV))
 		defer ticker.Stop()
 
@@ -92,7 +94,7 @@ func (s *hotRangesLoggingScheduler) start(ctx context.Context, stopper *stop.Sto
 			case <-ctx.Done():
 				return
 			case <-ticker.C:
-				if !TelemetryHotRangesStatsEnabled.Get(&s.st.SV) {
+				if !logcrash.DiagnosticsReportingEnabled.Get(&s.st.SV) || !TelemetryHotRangesStatsEnabled.Get(&s.st.SV) {
 					continue
 				}
 				resp, err := s.sServer.HotRangesV2(ctx, &serverpb.HotRangesRequest{PageSize: ReportTopHottestRanges})
diff --git a/pkg/server/tenant.go b/pkg/server/tenant.go
index 6c66123d719a..0a107df1ced5 100644
--- a/pkg/server/tenant.go
+++ b/pkg/server/tenant.go
@@ -65,7 +65,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/admission"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
 	"github.com/cockroachdb/cockroach/pkg/util/netutil"
 	"github.com/cockroachdb/cockroach/pkg/util/schedulerlatency"
@@ -849,8 +848,14 @@ func (s *SQLServerWrapper) AcceptClients(ctx context.Context) error {
 		}
 	}
 
-	if logcrash.DiagnosticsReportingEnabled.Get(&s.ClusterSettings().SV) {
-		structlogging.StartHotRangesLoggingScheduler(ctx, s.stopper, s.sqlServer.tenantConnect, *s.sqlServer.internalExecutor, s.ClusterSettings())
+	if err := structlogging.StartHotRangesLoggingScheduler(
+		ctx,
+		s.stopper,
+		s.sqlServer.tenantConnect,
+		*s.sqlServer.internalExecutor,
+		s.ClusterSettings(),
+	); err != nil {
+		return err
 	}
 
 	s.sqlServer.isReady.Set(true)