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
smg260 · Apr 24, 2023 · 06a8d34 · 06a8d34
1 parent b2be386
commit 06a8d34
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 11 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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)