Merge #80929 #80975

80929: authors: add pjtatlow to authors r=rickystewart a=pjtatlow

Release note: None

80975: outliers: extract a detector interface r=matthewtodd a=matthewtodd

This gives us a place to hang the upcoming work in #79451.

Release note: None

Co-authored-by: PJ Tatlow <pj@cockroachlabs.com>
Co-authored-by: Matthew Todd <todd@cockroachlabs.com>

AUTHORS

@@ -340,6 +340,7 @@ phelanm <phelanm@gmail.com>
 Philippe Laflamme <philippe.laflamme@gmail.com>
 phynalle <phynalism@gmail.com>
 Piotr Zurek <p.zurek@gmail.com>
+PJ Tatlow <pj@cockroachlabs.com> pjtatlow <pjtatlow@gmail.com> 
 pocockn <pocockn@hotmail.co.uk>
 Pooja Maniar <maniar.pooja36@gmail.com> <poojam@cockroachlabs.com>
 Poornima Malepati <poornima.malepati@gmail.com> Poornima <poornima.malepati@gmail.com>

pkg/sql/sqlstats/outliers/BUILD.bazel

@@ -4,7 +4,10 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "outliers",
-    srcs = ["outliers.go"],
+    srcs = [
+        "detector.go",
+        "outliers.go",
+    ],
     embed = [":outliers_go_proto"],
     importpath = "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/outliers",
     visibility = ["//visibility:public"],
@@ -21,9 +24,12 @@ go_library(
 
 go_test(
     name = "outliers_test",
-    srcs = ["outliers_test.go"],
+    srcs = [
+        "detector_test.go",
+        "outliers_test.go",
+    ],
+    embed = [":outliers"],
     deps = [
-        ":outliers",
         "//pkg/settings/cluster",
         "//pkg/sql/clusterunique",
         "//pkg/util/uuid",

pkg/sql/sqlstats/outliers/detector.go

@@ -0,0 +1,32 @@
+// Copyright 2022 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package outliers
+
+import "github.com/cockroachdb/cockroach/pkg/settings/cluster"
+
+type detector interface {
+	enabled() bool
+	isOutlier(*Outlier_Statement) bool
+}
+
+var _ detector = &latencyThresholdDetector{}
+
+type latencyThresholdDetector struct {
+	st *cluster.Settings
+}
+
+func (l latencyThresholdDetector) enabled() bool {
+	return LatencyThreshold.Get(&l.st.SV) > 0
+}
+
+func (l latencyThresholdDetector) isOutlier(s *Outlier_Statement) bool {
+	return l.enabled() && s.LatencyInSeconds >= LatencyThreshold.Get(&l.st.SV).Seconds()
+}

pkg/sql/sqlstats/outliers/detector_test.go

@@ -0,0 +1,53 @@
+// Copyright 2022 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package outliers
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/stretchr/testify/require"
+)
+
+func TestLatencyThresholdDetector(t *testing.T) {
+	t.Run("enabled false by default", func(t *testing.T) {
+		detector := latencyThresholdDetector{st: cluster.MakeTestingClusterSettings()}
+		require.False(t, detector.enabled())
+	})
+
+	t.Run("enabled true with nonzero threshold", func(t *testing.T) {
+		st := cluster.MakeTestingClusterSettings()
+		LatencyThreshold.Override(context.Background(), &st.SV, 1*time.Second)
+		detector := latencyThresholdDetector{st: st}
+		require.True(t, detector.enabled())
+	})
+
+	t.Run("isOutlier false when disabled", func(t *testing.T) {
+		detector := latencyThresholdDetector{st: cluster.MakeTestingClusterSettings()}
+		require.False(t, detector.isOutlier(&Outlier_Statement{LatencyInSeconds: 1}))
+	})
+
+	t.Run("isOutlier false when fast enough", func(t *testing.T) {
+		st := cluster.MakeTestingClusterSettings()
+		LatencyThreshold.Override(context.Background(), &st.SV, 1*time.Second)
+		detector := latencyThresholdDetector{st: st}
+		require.False(t, detector.isOutlier(&Outlier_Statement{LatencyInSeconds: 0.5}))
+	})
+
+	t.Run("isOutlier true beyond threshold", func(t *testing.T) {
+		st := cluster.MakeTestingClusterSettings()
+		LatencyThreshold.Override(context.Background(), &st.SV, 1*time.Second)
+		detector := latencyThresholdDetector{st: st}
+		require.True(t, detector.isOutlier(&Outlier_Statement{LatencyInSeconds: 1}))
+	})
+}

pkg/sql/sqlstats/outliers/outliers.go

@@ -45,7 +45,7 @@ const (
 // statement execution to determine which statements are outliers and
 // exposes the set of currently retained outliers.
 type Registry struct {
-	st *cluster.Settings
+	detector detector
 
 	// Note that this single mutex places unnecessary constraints on outlier
 	// detection and reporting. We will develop a higher-throughput system
@@ -65,7 +65,7 @@ func New(st *cluster.Settings) *Registry {
 			return size > maxCacheSize
 		},
 	}
-	r := &Registry{st: st}
+	r := &Registry{detector: latencyThresholdDetector{st: st}}
 	r.mu.statements = make(map[clusterunique.ID][]*Outlier_Statement)
 	r.mu.outliers = cache.NewUnorderedCache(config)
 	return r
@@ -98,7 +98,7 @@ func (r *Registry) ObserveTransaction(sessionID clusterunique.ID, txnID uuid.UUI
 
 	hasOutlier := false
 	for _, s := range statements {
-		if s.LatencyInSeconds > LatencyThreshold.Get(&r.st.SV).Seconds() {
+		if r.detector.isOutlier(s) {
 			hasOutlier = true
 		}
 	}
@@ -115,7 +115,7 @@ func (r *Registry) ObserveTransaction(sessionID clusterunique.ID, txnID uuid.UUI
 }
 
 func (r *Registry) enabled() bool {
-	return LatencyThreshold.Get(&r.st.SV) > 0
+	return r.detector.enabled()
 }
 
 // Reader offers read-only access to the currently retained set of outliers.