Merge #143915

143915: microbench-ci: per metric retry r=tbg,DarrylWong,golgeek a=herkolategan

Previously,
- The retry confirmation logic considered all metrics of a benchmark together, which could lead to false positives when different metrics regressed on different retries.
- The comparison step recalculated regression status instead of using the already-available marker files.

This PR addresses both issues by:
- Making retry logic track each metric independently, ensuring the same metric must regress consistently across retries
- Simplifying the comparison step to use marker files instead of recalculating the regression status.

Epic: None
Release note: None

Co-authored-by: Herko Lategan <[email protected]>

Author: craig[bot]

pkg/cmd/microbench-ci/benchmark.go

@@ -55,6 +55,10 @@ func (b *Benchmark) sanitizedName() string {
 	return sanitizeRe.ReplaceAllString(strings.TrimPrefix(b.Name, "Benchmark"), "_")
 }
 
+func (b *Benchmark) markerName(status Status) string {
+	return b.sanitizedName() + "." + strings.ToUpper(status.String())
+}
+
 func (b *Benchmark) binaryName() string {
 	return b.sanitizedPackageName()
 }

pkg/cmd/microbench-ci/compare.go

@@ -86,18 +86,6 @@ func (c *CompareResult) status(metricName string) Status {
 	return status
 }
 
-// top returns the top status of all metrics in the comparison.
-func (c *CompareResult) top() Status {
-	topStatus := NoChange
-	for metric := range c.MetricMap {
-		status := c.status(metric)
-		if status > topStatus {
-			topStatus = status
-		}
-	}
-	return topStatus
-}
-
 // compare compares the metrics of a benchmark between two revisions. Only the
 // specified last number of lines of the benchmark logs are considered. If lines
 // is 0, it considers the entire logs.
@@ -153,12 +141,6 @@ func (b Benchmarks) compareBenchmarks() (CompareResults, error) {
 		if err != nil {
 			return nil, err
 		}
-		if compareResult.top() != NoChange {
-			compareResult, err = benchmark.compare(0)
-			if err != nil {
-				return nil, err
-			}
-		}
 		compareResults = append(compareResults, compareResult)
 	}
 	return compareResults, nil

pkg/cmd/microbench-ci/run.go

@@ -77,7 +77,12 @@ func (b *Benchmark) run() error {
 			return err
 		}
 	}
-	status := NoChange
+
+	status := make(map[string]Status)
+	for _, metric := range b.Metrics {
+		status[metric.Name] = NoChange
+	}
+
 	tries := 0
 	for retries := 0; retries < b.Retries; retries++ {
 		log.Printf("Running benchmark %q for %d iterations (attempt %d)", b.Name, b.Count, retries+1)
@@ -98,35 +103,59 @@ func (b *Benchmark) run() error {
 		if err != nil {
 			return err
 		}
-		currentStatus := compareResult.top()
-		// If the benchmark shows no change, we can stop running additional retry
-		// iterations. Retries are run once a benchmark has regressed or improved,
-		// on the first try, to ensure that the change is not a fluke.
-		if currentStatus == NoChange {
-			status = currentStatus
-			break
+
+		// Loop through all metrics and check if they have a consistent status.
+		// As long as one metric maintains a consistent status (other than
+		// NoChange), we continue running additional retry iterations.
+		keepRunning := false
+		for _, metric := range b.Metrics {
+			currentStatus := compareResult.status(metric.Name)
+			// If this is the first run, we set the status to the current
+			// status. Only continue running if a metric has regressed or
+			// improved.
+			if retries == 0 {
+				status[metric.Name] = currentStatus
+				if currentStatus != NoChange {
+					keepRunning = true
+				}
+				continue
+			}
+
+			// On a retry, if any metric continues having a consistent state (other
+			// than NoChange), we run additional retries.
+			if status[metric.Name] != NoChange {
+				if status[metric.Name] == currentStatus {
+					keepRunning = true
+				} else {
+					// The metric previously indicated a change, but now
+					// indicates a change in the opposite direction. Reset the
+					// status to NoChange to indicate that the benchmark did not
+					// change.
+					status[metric.Name] = NoChange
+				}
+			}
 		}
 
-		// If this is the first run, we set the status to the current status.
-		// Otherwise, we check if the regression or improvement is persistent and
-		// continue running until all retries are exhausted. If the status flips
-		// from a regression to an improvement or vice versa, we set the status to
-		// NoChange and stop running, as the results are inconclusive.
-		if status == NoChange {
-			status = currentStatus
-		} else if status != currentStatus {
-			// If the benchmark shows a different change, the results are inconclusive.
-			status = NoChange
+		if !keepRunning {
+			// Reset all metrics to NoChange to indicate that the benchmark did
+			// not change, and we can stop running additional retries.
+			for _, metric := range b.Metrics {
+				status[metric.Name] = NoChange
+			}
 			break
 		}
 	}
 
-	// Write change marker file if the benchmark changed.
-	if status != NoChange {
-		marker := strings.ToUpper(status.String())
-		err := os.WriteFile(path.Join(suite.artifactsDir(New), b.sanitizedName()+"."+marker), nil, 0644)
-		if err != nil {
-			return err
+	// Write change marker file (per metric) if the benchmark changed. It's
+	// possible that both a regression and an improvement occurred, for
+	// different metrics of the same benchmark, in which case both markers will
+	// be written.
+	for _, metric := range b.Metrics {
+		if status[metric.Name] != NoChange {
+			err := os.WriteFile(path.Join(suite.artifactsDir(New), b.markerName(status[metric.Name])), nil, 0644)
+			if err != nil {
+				return err
+			}
 		}
 	}