Merge cockroachdb#101959

101959: server: fix proper call for StartHotRangesLoggingScheduler r=koorosh a=koorosh 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 Resolves: cockroachdb#101933 TODO: Manually backport to `23.1.x` with cockroachdb#101927 Co-authored-by: Andrii Vorobiov <[email protected]>
miretskiy · Apr 24, 2023 · 3c23346 · 3c23346
2 parents 1f3419e + 9698b43
commit 3c23346
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)