Merge #141601

141601: metric: Add functionality to disable aggregate metric reporting r=alyshanjahani-crl a=alyshanjahani-crl

Previously when server.child_metrics.enabled was true, the prometheus exporter
would include the time series with the child labels as well as the aggregate
time series. This behaviour is not ideal as it can lead to double counting.

For example, consider the following metric:
counter_foo{node_id="1"} 6
counter_foo{node_id="1", child_metric_label="bar"} 3
counter_foo{node_id="1", child_metric_label="xyz"} 3

The aggregate metric is always the sum of all the child metrics. If a user
were to export these metrics and query them like so:
sum(counter_foo) by node_id

They would get back 6+3+3=12 when really they should be getting back 6.

This commit introduces a cluster setting
server.child_metrics.include_aggregate.enabled which is false by default.

Fixes: https://cockroachlabs.atlassian.net/browse/CC-31395

Release note (ops change): Modifies the default behaviour of prometheus metric
reporting (/_status/vars) by not including the aggregate time series. This
prevents issues with double counting when querying metrics. Note that this
reporting behaviour can be toggled by a new cluster setting
server.child_metrics.include_aggregate.enabled. By default it is false, but
by setting it to true, the (undesired) behaviour of reporting the aggregate
timeseries is restored.

Co-authored-by: Alyshan Jahani <[email protected]>

Author: craig[bot]

docs/generated/settings/settings-for-tenants.txt

@@ -111,6 +111,7 @@ server.auth_log.sql_connections.enabled	boolean	false	if set, log SQL client con
 server.auth_log.sql_sessions.enabled	boolean	false	if set, log verbose SQL session authentication events to the SESSIONS log channel (note: may hinder performance on loaded nodes). Session start and end events are always logged regardless of this setting; disable the SESSIONS log channel to suppress them.	application
 server.authentication_cache.enabled	boolean	true	enables a cache used during authentication to avoid lookups to system tables when retrieving per-user authentication-related information	application
 server.child_metrics.enabled	boolean	false	enables the exporting of child metrics, additional prometheus time series with extra labels	application
+server.child_metrics.include_aggregate.enabled	boolean	true	include the reporting of the aggregate time series when child metrics are enabled. This cluster setting has no effect if child metrics are disabled.	application
 server.client_cert_expiration_cache.capacity	integer	1000	the maximum number of client cert expirations stored	application
 server.clock.forward_jump_check.enabled (alias: server.clock.forward_jump_check_enabled)	boolean	false	if enabled, forward clock jumps > max_offset/2 will cause a panic	application
 server.clock.persist_upper_bound_interval	duration	0s	the interval between persisting the wall time upper bound of the clock. The clock does not generate a wall time greater than the persisted timestamp and will panic if it sees a wall time greater than this value. When cockroach starts, it waits for the wall time to catch-up till this persisted timestamp. This guarantees monotonic wall time across server restarts. Not setting this or setting a value of 0 disables this feature.	application

docs/generated/settings/settings.html

@@ -141,6 +141,7 @@
 <tr><td><div id="setting-server-auth-log-sql-sessions-enabled" class="anchored"><code>server.auth_log.sql_sessions.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if set, log verbose SQL session authentication events to the SESSIONS log channel (note: may hinder performance on loaded nodes). Session start and end events are always logged regardless of this setting; disable the SESSIONS log channel to suppress them.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-authentication-cache-enabled" class="anchored"><code>server.authentication_cache.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>enables a cache used during authentication to avoid lookups to system tables when retrieving per-user authentication-related information</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-child-metrics-enabled" class="anchored"><code>server.child_metrics.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>enables the exporting of child metrics, additional prometheus time series with extra labels</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-server-child-metrics-include-aggregate-enabled" class="anchored"><code>server.child_metrics.include_aggregate.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>include the reporting of the aggregate time series when child metrics are enabled. This cluster setting has no effect if child metrics are disabled.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-client-cert-expiration-cache-capacity" class="anchored"><code>server.client_cert_expiration_cache.capacity</code></div></td><td>integer</td><td><code>1000</code></td><td>the maximum number of client cert expirations stored</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-clock-forward-jump-check-enabled" class="anchored"><code>server.clock.forward_jump_check.enabled<br />(alias: server.clock.forward_jump_check_enabled)</code></div></td><td>boolean</td><td><code>false</code></td><td>if enabled, forward clock jumps &gt; max_offset/2 will cause a panic</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-server-clock-persist-upper-bound-interval" class="anchored"><code>server.clock.persist_upper_bound_interval</code></div></td><td>duration</td><td><code>0s</code></td><td>the interval between persisting the wall time upper bound of the clock. The clock does not generate a wall time greater than the persisted timestamp and will panic if it sees a wall time greater than this value. When cockroach starts, it waits for the wall time to catch-up till this persisted timestamp. This guarantees monotonic wall time across server restarts. Not setting this or setting a value of 0 disables this feature.</td><td>Serverless/Dedicated/Self-Hosted</td></tr>

pkg/ccl/sqlproxyccl/server.go

@@ -149,7 +149,7 @@ func (s *Server) handleVars(w http.ResponseWriter, r *http.Request) {
 	contentType := expfmt.Negotiate(r.Header)
 	w.Header().Set(httputil.ContentTypeHeader, string(contentType))
 	scrape := func(pm *metric.PrometheusExporter) {
-		pm.ScrapeRegistry(s.metricsRegistry, true /* includeChildMetrics*/)
+		pm.ScrapeRegistry(s.metricsRegistry, true /* includeChildMetrics*/, true /* includeAggregateMetrics */)
 	}
 	if err := s.prometheusExporter.ScrapeAndPrintAsText(w, contentType, scrape); err != nil {
 		log.Errorf(r.Context(), "%v", err)

pkg/kv/kvserver/client_tenant_test.go

@@ -99,7 +99,7 @@ func TestTenantsStorageMetricsOnSplit(t *testing.T) {
 		})
 		ex := metric.MakePrometheusExporter()
 		scrape := func(ex *metric.PrometheusExporter) {
-			ex.ScrapeRegistry(store.Registry(), true /* includeChildMetrics */)
+			ex.ScrapeRegistry(store.Registry(), true /* includeChildMetrics */, true)
 		}
 		var in bytes.Buffer
 		if err := ex.ScrapeAndPrintAsText(&in, expfmt.FmtText, scrape); err != nil {

pkg/server/load_endpoint.go

@@ -89,7 +89,7 @@ func newLoadEndpoint(
 
 // Exporter for the load vars that are provided only by the load handler.
 func (le *loadEndpoint) scrapeLoadVarsIntoPrometheus(pm *metric.PrometheusExporter) {
-	pm.ScrapeRegistry(le.registry, true)
+	pm.ScrapeRegistry(le.registry, true, true)
 }
 
 // Handler responsible for serving the instant values of selected

pkg/server/status/recorder.go

@@ -89,6 +89,15 @@ var ChildMetricsEnabled = settings.RegisterBoolSetting(
 	false,
 	settings.WithPublic)
 
+// includeAggregateMetricsEnabled enables the exporting of the aggregate time series when child
+// metrics are enabled.
+var includeAggregateMetricsEnabled = settings.RegisterBoolSetting(
+	settings.ApplicationLevel, "server.child_metrics.include_aggregate.enabled",
+	"include the reporting of the aggregate time series when child metrics are enabled. This cluster setting "+
+		"has no effect if child metrics are disabled.",
+	true,
+	settings.WithPublic)
+
 // MetricsRecorder is used to periodically record the information in a number of
 // metric registries.
 //
@@ -336,15 +345,16 @@ func (mr *MetricsRecorder) ScrapeIntoPrometheus(pm *metric.PrometheusExporter) {
 		}
 	}
 	includeChildMetrics := ChildMetricsEnabled.Get(&mr.settings.SV)
-	pm.ScrapeRegistry(mr.mu.nodeRegistry, includeChildMetrics)
-	pm.ScrapeRegistry(mr.mu.appRegistry, includeChildMetrics)
-	pm.ScrapeRegistry(mr.mu.logRegistry, includeChildMetrics)
-	pm.ScrapeRegistry(mr.mu.sysRegistry, includeChildMetrics)
+	includeAggregateMetrics := includeAggregateMetricsEnabled.Get(&mr.settings.SV)
+	pm.ScrapeRegistry(mr.mu.nodeRegistry, includeChildMetrics, includeAggregateMetrics)
+	pm.ScrapeRegistry(mr.mu.appRegistry, includeChildMetrics, includeAggregateMetrics)
+	pm.ScrapeRegistry(mr.mu.logRegistry, includeChildMetrics, includeAggregateMetrics)
+	pm.ScrapeRegistry(mr.mu.sysRegistry, includeChildMetrics, includeAggregateMetrics)
 	for _, reg := range mr.mu.storeRegistries {
-		pm.ScrapeRegistry(reg, includeChildMetrics)
+		pm.ScrapeRegistry(reg, includeChildMetrics, includeAggregateMetrics)
 	}
 	for _, tenantRegistry := range mr.mu.tenantRegistries {
-		pm.ScrapeRegistry(tenantRegistry, includeChildMetrics)
+		pm.ScrapeRegistry(tenantRegistry, includeChildMetrics, includeAggregateMetrics)
 	}
 }
 

pkg/sql/mvcc_statistics_update_job_test.go

@@ -127,7 +127,7 @@ func TestTenantGlobalAggregatedLivebytes(t *testing.T) {
 			&in,
 			expfmt.FmtText,
 			func(ex *metric.PrometheusExporter) {
-				ex.ScrapeRegistry(r, true /* includeChildMetrics */)
+				ex.ScrapeRegistry(r, true /* includeChildMetrics */, true)
 			},
 		)
 		require.NoError(t, err)

pkg/testutils/metrictestutils/metrics_text.go

@@ -21,7 +21,7 @@ import (
 func GetMetricsText(registry *metric.Registry, re *regexp.Regexp) (string, error) {
 	ex := metric.MakePrometheusExporter()
 	scrape := func(ex *metric.PrometheusExporter) {
-		ex.ScrapeRegistry(registry, true /* includeChildMetrics */)
+		ex.ScrapeRegistry(registry, true /* includeChildMetrics */, true)
 	}
 	var in bytes.Buffer
 	if err := ex.ScrapeAndPrintAsText(&in, expfmt.FmtText, scrape); err != nil {

pkg/util/metric/aggmetric/BUILD.bazel

@@ -31,6 +31,7 @@ go_test(
     deps = [
         "//pkg/base",
         "//pkg/roachpb",
+        "//pkg/testutils",
         "//pkg/testutils/datapathutils",
         "//pkg/testutils/echotest",
         "//pkg/util/leaktest",

pkg/util/metric/aggmetric/agg_metric_test.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/echotest"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -24,6 +25,48 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestExcludeAggregateMetrics(t *testing.T) {
+	r := metric.NewRegistry()
+	counter := NewCounter(metric.Metadata{Name: "counter"}, "child_label")
+	counter.AddChild("xyz")
+	r.AddMetric(counter)
+
+	// Don't include child metrics, so only report the aggregate.
+	// Flipping the includeAggregateMetrics flag should have no effect.
+	testutils.RunTrueAndFalse(t, "includeChildMetrics=false,includeAggregateMetrics", func(t *testing.T, includeAggregateMetrics bool) {
+		pe := metric.MakePrometheusExporter()
+		pe.ScrapeRegistry(r, false, includeAggregateMetrics)
+		families, err := pe.Gather()
+		require.NoError(t, err)
+		require.Equal(t, 1, len(families))
+		require.Equal(t, "counter", families[0].GetName())
+		require.Equal(t, 1, len(families[0].GetMetric()))
+		require.Equal(t, 0, len(families[0].GetMetric()[0].GetLabel()))
+	})
+
+	testutils.RunTrueAndFalse(t, "includeChildMetrics=true,includeAggregateMetrics", func(t *testing.T, includeAggregateMetrics bool) {
+		pe := metric.MakePrometheusExporter()
+		pe.ScrapeRegistry(r, true, includeAggregateMetrics)
+		families, err := pe.Gather()
+		require.NoError(t, err)
+		require.Equal(t, 1, len(families))
+		require.Equal(t, "counter", families[0].GetName())
+		var labelPair *prometheusgo.LabelPair
+		if includeAggregateMetrics {
+			require.Equal(t, 2, len(families[0].GetMetric()))
+			require.Equal(t, 0, len(families[0].GetMetric()[0].GetLabel()))
+			require.Equal(t, 1, len(families[0].GetMetric()[1].GetLabel()))
+			labelPair = families[0].GetMetric()[1].GetLabel()[0]
+		} else {
+			require.Equal(t, 1, len(families[0].GetMetric()))
+			require.Equal(t, 1, len(families[0].GetMetric()[0].GetLabel()))
+			labelPair = families[0].GetMetric()[0].GetLabel()[0]
+		}
+		require.Equal(t, "child_label", *labelPair.Name)
+		require.Equal(t, "xyz", *labelPair.Value)
+	})
+}
+
 func TestAggMetric(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
@@ -32,7 +75,7 @@ func TestAggMetric(t *testing.T) {
 		var in bytes.Buffer
 		ex := metric.MakePrometheusExporter()
 		scrape := func(ex *metric.PrometheusExporter) {
-			ex.ScrapeRegistry(r, true /* includeChildMetrics */)
+			ex.ScrapeRegistry(r, true /* includeChildMetrics */, true)
 		}
 		require.NoError(t, ex.ScrapeAndPrintAsText(&in, expfmt.FmtText, scrape))
 		var lines []string