Merge #143926

143926: roachtest: Add unit labels in clusterstatscollector aggregate output r=sambhav-jain-16 a=csgourav

This change enhances the clusterstatscollector's aggregated metrics by adding two new descriptive fields, Unit and is_higher_better, which are used in plotting aggregated metrics.

Updates the way BenchmarkFns are specified in tests, those are the ones responsible for generating one summarized value for one test run.

Epic: none
Fixes: CRDB-48909

Co-authored-by: Gourav Kumar <[email protected]>

Author: craig[bot]

pkg/cmd/roachtest/clusterstats/BUILD.bazel

@@ -63,6 +63,7 @@ go_test(
         "@com_github_prometheus_client_golang//api/prometheus/v1:prometheus",
         "@com_github_prometheus_common//model",
         "@com_github_stretchr_testify//require",
+        "//pkg/cmd/roachtest/roachtestutil",
     ],
 )
 

pkg/cmd/roachtest/clusterstats/exporter.go

@@ -72,7 +72,7 @@ type StatExporter interface {
 		from time.Time,
 		to time.Time,
 		queries []AggQuery,
-		benchmarkFns ...func(map[string]StatSummary) (string, float64),
+		benchmarkFns ...func(map[string]StatSummary) *roachtestutil.AggregatedMetric,
 	) (*ClusterStatRun, error)
 }
 
@@ -93,8 +93,9 @@ type StatSummary struct {
 // stat information collected during the run. This struct is mirrored in
 // cockroachdb/roachperf for deserialization.
 type ClusterStatRun struct {
-	Total map[string]float64     `json:"total"`
-	Stats map[string]StatSummary `json:"stats"`
+	Total            map[string]float64                        `json:"total"`
+	Stats            map[string]StatSummary                    `json:"stats"`
+	BenchmarkMetrics map[string]roachtestutil.AggregatedMetric `json:"-"` // Not serialized to JSON
 }
 
 // statsWriter writes the stats buffer to the file. This is used in unit test
@@ -137,13 +138,47 @@ func (r *ClusterStatRun) serializeOpenmetricsOutRun(
 	return statsWriter(ctx, t, c, report, dest)
 }
 
+// createReport returns a ClusterStatRun struct that encompases the results of
+// the run.
+func createReport(
+	summaries map[string]StatSummary,
+	summaryStats map[string]float64,
+	benchmarkMetrics map[string]roachtestutil.AggregatedMetric,
+) *ClusterStatRun {
+	testRun := ClusterStatRun{
+		Stats:            make(map[string]StatSummary),
+		BenchmarkMetrics: benchmarkMetrics,
+	}
+
+	for tag, summary := range summaries {
+		testRun.Stats[tag] = summary
+	}
+	testRun.Total = summaryStats
+	return &testRun
+}
+
+// serializeOpenmetricsReport serializes the passed in statistics into an openmetrics
+// parseable performance artifact format.
 func serializeOpenmetricsReport(r ClusterStatRun, labelString *string) (*bytes.Buffer, error) {
 	var buffer bytes.Buffer
 
 	// Emit summary metrics from Total
 	for metricName, value := range r.Total {
 		buffer.WriteString(roachtestutil.GetOpenmetricsGaugeType(metricName))
-		buffer.WriteString(fmt.Sprintf("%s{%s} %f %d\n", util.SanitizeMetricName(metricName), *labelString, value, timeutil.Now().UTC().Unix()))
+
+		// Add labels from benchmark metrics if available
+		additionalLabels := ""
+		if benchmarkMetric, ok := r.BenchmarkMetrics[metricName]; ok {
+			additionalLabels += fmt.Sprintf(",unit=\"%s\"", util.SanitizeValue(benchmarkMetric.Unit))
+			additionalLabels += fmt.Sprintf(",is_higher_better=\"%t\"", benchmarkMetric.IsHigherBetter)
+		}
+
+		buffer.WriteString(fmt.Sprintf("%s{%s%s} %f %d\n",
+			util.SanitizeMetricName(metricName),
+			*labelString,
+			additionalLabels,
+			value,
+			timeutil.Now().UTC().Unix()))
 	}
 
 	// Emit histogram metrics from Stats
@@ -179,20 +214,6 @@ func serializeOpenmetricsReport(r ClusterStatRun, labelString *string) (*bytes.B
 	return &buffer, nil
 }
 
-// createReport returns a ClusterStatRun struct that encompases the results of
-// the run.
-func createReport(
-	summaries map[string]StatSummary, summaryStats map[string]float64,
-) *ClusterStatRun {
-	testRun := ClusterStatRun{Stats: make(map[string]StatSummary)}
-
-	for tag, summary := range summaries {
-		testRun.Stats[tag] = summary
-	}
-	testRun.Total = summaryStats
-	return &testRun
-}
-
 // Export collects, serializes and saves a roachperf file, with statistics
 // collect from - to time, for the AggQuery(s) given. The format is described
 // in the doc.go and the AggQuery definition. In addition to the AggQuery(s),
@@ -208,20 +229,37 @@ func (cs *clusterStatCollector) Export(
 	from time.Time,
 	to time.Time,
 	queries []AggQuery,
-	benchmarkFns ...func(summaries map[string]StatSummary) (string, float64),
+	benchmarkFns ...func(summaries map[string]StatSummary) *roachtestutil.AggregatedMetric,
 ) (testRun *ClusterStatRun, err error) {
 	l := t.L()
 	summaries := cs.collectSummaries(ctx, l, Interval{From: from, To: to}, queries)
 
-	summaryValues := make(map[string]float64)
-	for _, scalarFn := range benchmarkFns {
-		t, result := scalarFn(summaries)
-		summaryValues[t] = result
+	// Cache this value to avoid calling the function multiple times
+	isOpenMetricsEnabled := t.ExportOpenmetrics()
+
+	// Initialize benchmarkMetrics as nil when OpenMetrics is disabled
+	var benchmarkMetrics map[string]roachtestutil.AggregatedMetric
+	if isOpenMetricsEnabled {
+		benchmarkMetrics = make(map[string]roachtestutil.AggregatedMetric)
+	}
+
+	// Summary values for total are always collected
+	summaryValues := map[string]float64{}
+	for _, benchMarkFn := range benchmarkFns {
+		benchmarkMetric := benchMarkFn(summaries)
+		if benchmarkMetric != nil {
+			summaryValues[benchmarkMetric.Name] = float64(benchmarkMetric.Value)
+
+			// Only populate BenchmarkMetrics when OpenMetrics export is enabled
+			if isOpenMetricsEnabled {
+				benchmarkMetrics[benchmarkMetric.Name] = *benchmarkMetric
+			}
+		}
 	}
 
-	testRun = createReport(summaries, summaryValues)
+	testRun = createReport(summaries, summaryValues, benchmarkMetrics)
 	if !dryRun {
-		err = testRun.SerializeOutRun(ctx, t, c, t.ExportOpenmetrics())
+		err = testRun.SerializeOutRun(ctx, t, c, isOpenMetricsEnabled)
 	}
 	return testRun, err
 }

pkg/cmd/roachtest/clusterstats/exporter_test.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
@@ -199,9 +200,15 @@ var (
 	}
 
 	benchMarkFn = func(totalKey string, totalValue float64) func(
-		summaries map[string]StatSummary) (string, float64) {
-		return func(summaries map[string]StatSummary) (string, float64) {
-			return totalKey, totalValue
+		summaries map[string]StatSummary) *roachtestutil.AggregatedMetric {
+		return func(summaries map[string]StatSummary) *roachtestutil.AggregatedMetric {
+			return &roachtestutil.AggregatedMetric{
+				Name:             totalKey,
+				Value:            roachtestutil.MetricPoint(totalValue),
+				Unit:             "count",
+				IsHigherBetter:   true,
+				AdditionalLabels: nil,
+			}
 		}
 	}
 )
@@ -347,13 +354,21 @@ func TestExport(t *testing.T) {
 			benchMarkFn("t3", 404),
 		)
 		require.Nil(t, err)
-		require.Equal(t, ClusterStatRun{
-			Stats: map[string]StatSummary{
-				fooStat.Query: makeExpectedTs(fooTS, fooAggQuery, 5, "1", "2", "3"),
-				barStat.Query: makeExpectedTs(barTS, barAggQuery, 5, "1", "2", "3"),
-			},
-			Total: map[string]float64{"t1": 203, "t3": 404},
-		}, *testRun)
+
+		// Compare individual fields instead of the whole struct
+		require.Equal(t, map[string]float64{"t1": 203, "t3": 404}, testRun.Total)
+		require.Equal(t, map[string]StatSummary{
+			fooStat.Query: makeExpectedTs(fooTS, fooAggQuery, 5, "1", "2", "3"),
+			barStat.Query: makeExpectedTs(barTS, barAggQuery, 5, "1", "2", "3"),
+		}, testRun.Stats)
+
+		// Verify BenchmarkMetrics
+		require.NotNil(t, testRun.BenchmarkMetrics)
+		require.Len(t, testRun.BenchmarkMetrics, 2)
+		require.Contains(t, testRun.BenchmarkMetrics, "t1")
+		require.Contains(t, testRun.BenchmarkMetrics, "t3")
+		require.Equal(t, float64(203), float64(testRun.BenchmarkMetrics["t1"].Value))
+		require.Equal(t, float64(404), float64(testRun.BenchmarkMetrics["t3"].Value))
 	})
 	t.Run("multi tag, multi stat, 5 ticks with openmetrics false", func(t *testing.T) {
 		c := getClusterStatCollector(ctx, ctrl, []expectPromRangeQuery{

pkg/cmd/roachtest/clusterstats/openmetrics_expected.txt

@@ -1,7 +1,7 @@
 # TYPE t1 gauge
-t1{cloud="gce",owner="roachtest_mock",test="mock_name",test_run_id="mock_id"} 203.000000 1727690493
+t1{cloud="gce",owner="roachtest_mock",test="mock_name",test_run_id="mock_id",unit="count",is_higher_better="true"} 203.000000 1727690493
 # TYPE t3 gauge
-t3{cloud="gce",owner="roachtest_mock",test="mock_name",test_run_id="mock_id"} 404.000000 1727690493
+t3{cloud="gce",owner="roachtest_mock",test="mock_name",test_run_id="mock_id",unit="count",is_higher_better="true"} 404.000000 1727690493
 # TYPE foo_count gauge
 foo_count{cloud="gce",owner="roachtest_mock",test="mock_name",test_run_id="mock_id",agg_tag="sum(foo)"} 123.000000 1608854400
 foo_count{cloud="gce",owner="roachtest_mock",test="mock_name",test_run_id="mock_id",agg_tag="sum(foo)"} 246.000000 1608854410

pkg/cmd/roachtest/tests/allocation_bench.go

@@ -16,6 +16,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/clusterstats"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
@@ -395,28 +396,46 @@ func runAllocationBenchSample(
 		true, /* dryRun */
 		startTime, endTime,
 		joinSummaryQueries(resourceMinMaxSummary, overloadMaxSummary, rebalanceCostSummary),
-		func(stats map[string]clusterstats.StatSummary) (string, float64) {
+		func(stats map[string]clusterstats.StatSummary) *roachtestutil.AggregatedMetric {
 			ret, name := 0.0, "cpu(%)"
 			if stat, ok := stats[cpuStat.Query]; ok {
 				ret = roundFraction(arithmeticMean(stat.Value), 1, 2)
 			}
-			return name, ret
+			return &roachtestutil.AggregatedMetric{
+				Name:             name,
+				Value:            roachtestutil.MetricPoint(ret),
+				Unit:             "percent",
+				IsHigherBetter:   false,
+				AdditionalLabels: nil,
+			}
 		},
-		func(stats map[string]clusterstats.StatSummary) (string, float64) {
+		func(stats map[string]clusterstats.StatSummary) *roachtestutil.AggregatedMetric {
 			ret, name := 0.0, "write(%)"
 			if stat, ok := stats[ioWriteStat.Query]; ok {
 				ret = roundFraction(arithmeticMean(stat.Value), 1, 2)
 			}
-			return name, ret
+			return &roachtestutil.AggregatedMetric{
+				Name:             name,
+				Value:            roachtestutil.MetricPoint(ret),
+				Unit:             "percent",
+				IsHigherBetter:   false,
+				AdditionalLabels: nil,
+			}
 		},
-		func(stats map[string]clusterstats.StatSummary) (string, float64) {
+		func(stats map[string]clusterstats.StatSummary) *roachtestutil.AggregatedMetric {
 			rebalanceMb := 0.0
 			values := stats[rebalanceSnapshotSentStat.Query].Value
 			if len(values) > 0 {
 				startMB, endMB := values[0], values[len(values)-1]
 				rebalanceMb = roundFraction(endMB-startMB, 1024, 2)
 			}
-			return "cost(gb)", rebalanceMb
+			return &roachtestutil.AggregatedMetric{
+				Name:             "cost(gb)",
+				Value:            roachtestutil.MetricPoint(rebalanceMb),
+				Unit:             "GB",
+				IsHigherBetter:   false,
+				AdditionalLabels: nil,
+			}
 		},
 	)
 }

pkg/cmd/roachtest/tests/allocator.go

@@ -120,11 +120,25 @@ func registerAllocator(r registry.Registry) {
 				// up-replication began, until the last rebalance action taken.
 				// The up replication time, is the time taken to up-replicate
 				// alone, not considering post up-replication rebalancing.
-				func(stats map[string]clusterstats.StatSummary) (string, float64) {
-					return "t-balance(s)", endTime.Sub(startTime).Seconds() - allocatorStableSeconds
+				func(stats map[string]clusterstats.StatSummary) *roachtestutil.AggregatedMetric {
+					balanceTime := endTime.Sub(startTime).Seconds() - allocatorStableSeconds
+					return &roachtestutil.AggregatedMetric{
+						Name:             "t-balance(s)",
+						Value:            roachtestutil.MetricPoint(balanceTime),
+						Unit:             "seconds",
+						IsHigherBetter:   false,
+						AdditionalLabels: nil,
+					}
 				},
-				func(stats map[string]clusterstats.StatSummary) (string, float64) {
-					return "t-uprepl(s)", replicateTime.Sub(startTime).Seconds()
+				func(stats map[string]clusterstats.StatSummary) *roachtestutil.AggregatedMetric {
+					upReplTime := replicateTime.Sub(startTime).Seconds()
+					return &roachtestutil.AggregatedMetric{
+						Name:             "t-uprepl(s)",
+						Value:            roachtestutil.MetricPoint(upReplTime),
+						Unit:             "seconds",
+						IsHigherBetter:   false,
+						AdditionalLabels: nil,
+					}
 				},
 			)
 			return err

pkg/cmd/roachtest/tests/cdc.go

@@ -143,15 +143,22 @@ func (ct *cdcTester) startStatsCollection() func() {
 			startTime,
 			endTime,
 			[]clusterstats.AggQuery{sqlServiceLatencyAgg, changefeedThroughputAgg, cpuUsageAgg},
-			func(stats map[string]clusterstats.StatSummary) (string, float64) {
+			func(stats map[string]clusterstats.StatSummary) *roachtestutil.AggregatedMetric {
 				// TODO(jayant): update this metric to be more accurate.
 				// It may be worth plugging in real latency values from the latency
 				// verifier here in the future for more accuracy. However, it may not be
 				// worth the added complexity. Since latency verifier failures will show
 				// up as roachtest failures, we don't need to make them very apparent in
 				// roachperf. Note that other roachperf stats, such as the aggregate stats
 				// above, will be accurate.
-				return "Total Run Time (mins)", endTime.Sub(startTime).Minutes()
+				duration := endTime.Sub(startTime).Minutes()
+				return &roachtestutil.AggregatedMetric{
+					Name:             "Total Run Time (mins)",
+					Value:            roachtestutil.MetricPoint(duration),
+					Unit:             "minutes",
+					IsHigherBetter:   false,
+					AdditionalLabels: nil,
+				}
 			},
 		)
 		if err != nil {

pkg/cmd/roachtest/tests/online_restore.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/clusterstats"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
@@ -366,13 +367,13 @@ func exportStats(ctx context.Context, rd restoreDriver, restoreStats restoreStat
 		restoreStats.workloadStartTime,
 		endTime,
 		[]clusterstats.AggQuery{sqlServiceLatencyP95Agg, queriesThroughputAgg},
-		func(stats map[string]clusterstats.StatSummary) (string, float64) {
+		func(stats map[string]clusterstats.StatSummary) *roachtestutil.AggregatedMetric {
 			var timeToHealth time.Time
 			healthyLatencyRatio := 1.25
 			n := len(stats[latencyQueryKey].Value)
 			rd.t.L().Printf("aggregating latency over %d data points", n)
 			if n == 0 {
-				return "", 0
+				return nil // Return nil for no data points
 			}
 			healthyLatency := stats[latencyQueryKey].Value[n-1]
 			latestHealthyValue := healthyLatency
@@ -394,7 +395,14 @@ func exportStats(ctx context.Context, rd restoreDriver, restoreStats restoreStat
 			description := "Time to within 1.25x of regular p95 latency (mins)"
 			rd.t.L().Printf("%s: %.2f minutes, compared to link + download phase time %.2f", description, rto, fullRestoreTime)
 			rd.t.L().Printf("Latency at Recovery Time %.0f ms; at end of test %.0f ms", latestHealthyValue, healthyLatency)
-			return description, rto
+
+			return &roachtestutil.AggregatedMetric{
+				Name:             description,
+				Value:            roachtestutil.MetricPoint(rto),
+				Unit:             "minutes",
+				IsHigherBetter:   false,
+				AdditionalLabels: nil,
+			}
 		},
 	)
 	if err != nil {