diff --git a/build/teamcity/cockroach/nightlies/microbenchmark_weekly.sh b/build/teamcity/cockroach/nightlies/microbenchmark_weekly.sh index 4af5657aca26..1fd3999cbb54 100755 --- a/build/teamcity/cockroach/nightlies/microbenchmark_weekly.sh +++ b/build/teamcity/cockroach/nightlies/microbenchmark_weekly.sh @@ -207,6 +207,7 @@ if [ -d "$output_dir/experiment" ] && [ "$(ls -A "$output_dir/experiment")" ] \ --sheet-desc="$sheet_description" \ ${influx_token:+--influx-token="$influx_token"} \ ${influx_host:+--influx-host="$influx_host"} \ + ${TRIGGERED_BUILD:+--post-issues} \ 2>&1 | tee "$output_dir/sheets.txt" else echo "No microbenchmarks were run. Skipping comparison." diff --git a/pkg/cmd/roachprod-microbench/compare.go b/pkg/cmd/roachprod-microbench/compare.go index 8e5e9d9c63a6..6bafc6c5bd16 100644 --- a/pkg/cmd/roachprod-microbench/compare.go +++ b/pkg/cmd/roachprod-microbench/compare.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/cmd/roachprod-microbench/google" "github.com/cockroachdb/cockroach/pkg/cmd/roachprod-microbench/model" "github.com/cockroachdb/cockroach/pkg/cmd/roachprod-microbench/util" + "github.com/cockroachdb/cockroach/pkg/roachprod/logger" "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" "github.com/cockroachdb/errors/oserror" @@ -37,6 +38,7 @@ type compareConfig struct { baselineDir string sheetDesc string threshold float64 + postIssues bool } type slackConfig struct { @@ -349,6 +351,53 @@ func (c *compare) compareUsingThreshold(comparisonResultsMap model.ComparisonRes return nil } +func (c *compare) postRegressionIssues( + links map[string]string, comparisonResultsMap model.ComparisonResultsMap, +) error { + loggerCfg := logger.Config{Stdout: os.Stdout, Stderr: os.Stderr} + l, err := loggerCfg.NewLogger("") + if err != nil { + return errors.Wrap(err, "failed to create logger for posting issues") + } + + for pkgName, comparisonResults := range comparisonResultsMap { + var regressions []regressionInfo + + for _, result := range comparisonResults { + for _, detail := range result.Comparisons { + comparison := detail.Comparison + metric := result.Metric + + // Check if this is a regression + isRegression := (comparison.Delta < 0 && metric.Better > 0) || + (comparison.Delta > 0 && metric.Better < 0) + + if isRegression && math.Abs(comparison.Delta) >= slackPercentageThreshold { + regressions = append(regressions, regressionInfo{ + benchmarkName: detail.BenchmarkName, + metricUnit: result.Metric.Unit, + percentChange: math.Abs(comparison.Delta), + formattedDelta: comparison.FormattedDelta, + }) + } + } + } + + if len(regressions) > 0 { + sheetLink := links[pkgName] + formatter, req := createRegressionPostRequest(pkgName, regressions, sheetLink, c.sheetDesc) + err := postBenchmarkIssue(c.ctx, l, formatter, req) + if err != nil { + log.Printf("failed to post regression issue for package %s: %v", pkgName, err) + continue + } + log.Printf("Posted regression issue for package: %s with %d regression(s)", pkgName, len(regressions)) + } + } + + return nil +} + func (c *compare) pushToInfluxDB(comparisonResultsMap model.ComparisonResultsMap) error { client := influxdb2.NewClient(c.influxConfig.host, c.influxConfig.token) defer client.Close() diff --git a/pkg/cmd/roachprod-microbench/github.go b/pkg/cmd/roachprod-microbench/github.go index 1eb3d891bdfd..620d1197e44a 100644 --- a/pkg/cmd/roachprod-microbench/github.go +++ b/pkg/cmd/roachprod-microbench/github.go @@ -43,6 +43,48 @@ func createBenchmarkPostRequest( return formatter, req } +// regressionInfo holds information about a single benchmark regression. +type regressionInfo struct { + benchmarkName string + metricUnit string + percentChange float64 + formattedDelta string +} + +// createRegressionPostRequest creates a post request for benchmark performance regressions. +func createRegressionPostRequest( + pkgName string, regressions []regressionInfo, sheetLink, description string, +) (issues.IssueFormatter, issues.PostRequest) { + // Build the regression summary message + var sb strings.Builder + sb.WriteString(fmt.Sprintf("Performance regressions detected in package %s\n\n", pkgName)) + sb.WriteString(fmt.Sprintf("Comparison: %s\n\n", description)) + sb.WriteString(fmt.Sprintf("Found %d benchmark(s) with regressions ≥20%%:\n\n", len(regressions))) + + for i, reg := range regressions { + if i >= 10 { // Limit to 10 regressions in the summary + sb.WriteString(fmt.Sprintf("... and %d more regression(s)\n", len(regressions)-i)) + break + } + sb.WriteString(fmt.Sprintf("• %s (%s): %s (%.1f%%)\n", + reg.benchmarkName, reg.metricUnit, reg.formattedDelta, reg.percentChange)) + } + + sb.WriteString(fmt.Sprintf("\nDetailed comparison: %s\n", sheetLink)) + + // Create a failure object for the package with all regressions + title := fmt.Sprintf("%s: performance regression", pkgName) + f := githubpost.MicrobenchmarkFailure( + pkgName, + title, + sb.String(), + ) + + formatter, req := githubpost.DefaultFormatter(context.Background(), f) + req.Labels = append(req.Labels, "O-microbench", "C-performance") + return formatter, req +} + // postBenchmarkIssue posts a benchmark issue to github. func postBenchmarkIssue( ctx context.Context, l *logger.Logger, formatter issues.IssueFormatter, req issues.PostRequest, diff --git a/pkg/cmd/roachprod-microbench/github_test.go b/pkg/cmd/roachprod-microbench/github_test.go index 300cb16147d1..002419a153d3 100644 --- a/pkg/cmd/roachprod-microbench/github_test.go +++ b/pkg/cmd/roachprod-microbench/github_test.go @@ -55,6 +55,232 @@ func TestCreatePostRequest(t *testing.T) { } // formatPostRequest emulates the behavior of the githubpost package. +func TestCreateRegressionPostRequestBasic(t *testing.T) { + regressions := []regressionInfo{ + { + benchmarkName: "BenchmarkScan", + metricUnit: "ns/op", + percentChange: 25.5, + formattedDelta: "+1.2ms", + }, + { + benchmarkName: "BenchmarkInsert", + metricUnit: "ns/op", + percentChange: 30.0, + formattedDelta: "+500µs", + }, + } + + formatter, req := createRegressionPostRequest( + "pkg/sql", + regressions, + "https://docs.google.com/spreadsheets/d/test123", + "master -> release-24.1", + ) + + // Verify labels + expectedLabels := []string{"O-microbench", "C-performance"} + for _, label := range expectedLabels { + found := false + for _, l := range req.Labels { + if l == label { + found = true + break + } + } + if !found { + t.Errorf("Expected label %q not found in %v", label, req.Labels) + } + } + + // Verify title contains package name + data := issues.TemplateData{ + PostRequest: req, + Parameters: req.ExtraParams, + CondensedMessage: issues.CondensedMessage(req.Message), + PackageNameShort: req.PackageName, + } + title := formatter.Title(data) + if !strings.Contains(title, "pkg/sql") { + t.Errorf("Title should contain package name: %s", title) + } + if !strings.Contains(title, "performance regression") { + t.Errorf("Title should contain 'performance regression': %s", title) + } + + // Verify message contains key information + if !strings.Contains(req.Message, "pkg/sql") { + t.Error("Message should contain package name") + } + if !strings.Contains(req.Message, "BenchmarkScan") { + t.Error("Message should contain benchmark name") + } + if !strings.Contains(req.Message, "25.5%") { + t.Error("Message should contain percentage") + } + if !strings.Contains(req.Message, "https://docs.google.com/spreadsheets/d/test123") { + t.Error("Message should contain sheet link") + } + if !strings.Contains(req.Message, "master -> release-24.1") { + t.Error("Message should contain comparison description") + } +} + +func TestCreateRegressionPostRequestMultiple(t *testing.T) { + // Test with multiple regressions + regressions := make([]regressionInfo, 15) + for i := 0; i < 15; i++ { + regressions[i] = regressionInfo{ + benchmarkName: fmt.Sprintf("Benchmark%d", i), + metricUnit: "ns/op", + percentChange: 20.0 + float64(i), + formattedDelta: fmt.Sprintf("+%dµs", i*100), + } + } + + _, req := createRegressionPostRequest( + "pkg/kv", + regressions, + "https://docs.google.com/spreadsheets/d/test456", + "baseline -> experiment", + ) + + // Should mention total count + if !strings.Contains(req.Message, "15 benchmark(s)") { + t.Error("Message should contain total regression count") + } + + // Should truncate to 10 and show "... and 5 more" + if !strings.Contains(req.Message, "5 more regression(s)") { + t.Errorf("Message should indicate truncation. Got: %s", req.Message) + } + + // First regression should be present + if !strings.Contains(req.Message, "Benchmark0") { + t.Error("Message should contain first benchmark") + } + + // 11th regression should not be listed individually + if strings.Contains(req.Message, "Benchmark10") { + t.Error("Message should not list 11th benchmark individually") + } +} + +func TestCreateRegressionPostRequestTruncation(t *testing.T) { + // Test the exact boundary at 10 regressions + regressions := make([]regressionInfo, 10) + for i := 0; i < 10; i++ { + regressions[i] = regressionInfo{ + benchmarkName: fmt.Sprintf("Benchmark%d", i), + metricUnit: "ns/op", + percentChange: 20.0, + formattedDelta: "+100µs", + } + } + + _, req := createRegressionPostRequest( + "pkg/storage", + regressions, + "https://docs.google.com/spreadsheets/d/test", + "test comparison", + ) + + // All 10 should be listed + for i := 0; i < 10; i++ { + if !strings.Contains(req.Message, fmt.Sprintf("Benchmark%d", i)) { + t.Errorf("Message should contain Benchmark%d", i) + } + } + + // Should not show truncation message for exactly 10 + if strings.Contains(req.Message, "more regression(s)") { + t.Error("Should not show truncation message for exactly 10 regressions") + } +} + +func TestCreateRegressionPostRequestFormat(t *testing.T) { + // Test the full formatted output of a regression issue + regressions := []regressionInfo{ + { + benchmarkName: "BenchmarkScan/rows=1000", + metricUnit: "ns/op", + percentChange: 25.5, + formattedDelta: "+1.2ms", + }, + { + benchmarkName: "BenchmarkInsert/batch=100", + metricUnit: "B/op", + percentChange: 30.0, + formattedDelta: "+5.0KB", + }, + } + + formatter, req := createRegressionPostRequest( + "pkg/sql/exec", + regressions, + "https://docs.google.com/spreadsheets/d/example123", + "v24.1.0 -> v24.2.0", + ) + + // Verify the full formatted message structure + output, err := formatPostRequest(formatter, req) + if err != nil { + t.Fatalf("Failed to format post request: %v", err) + } + + // Check title format + data := issues.TemplateData{ + PostRequest: req, + Parameters: req.ExtraParams, + CondensedMessage: issues.CondensedMessage(req.Message), + PackageNameShort: req.PackageName, + } + title := formatter.Title(data) + + // Verify title structure + if !strings.Contains(title, "pkg/sql/exec") { + t.Errorf("Title should contain package name, got: %s", title) + } + if !strings.Contains(title, "performance regression") { + t.Errorf("Title should contain 'performance regression', got: %s", title) + } + + // Verify formatted output contains all key elements + expectedElements := []string{ + "Performance regressions detected in package pkg/sql/exec", + "Comparison: v24.1.0 -> v24.2.0", + "Found 2 benchmark(s) with regressions ≥20%", + "BenchmarkScan/rows=1000", + "(ns/op): +1.2ms (25.5%)", + "BenchmarkInsert/batch=100", + "(B/op): +5.0KB (30.0%)", + "Detailed comparison: https://docs.google.com/spreadsheets/d/example123", + } + + for _, expected := range expectedElements { + if !strings.Contains(output, expected) { + t.Errorf("Formatted output should contain %q\nGot:\n%s", expected, output) + } + } + + // Verify labels are correct + if !containsLabel(req.Labels, "O-microbench") { + t.Errorf("Should have O-microbench label, got: %v", req.Labels) + } + if !containsLabel(req.Labels, "C-performance") { + t.Errorf("Should have C-performance label, got: %v", req.Labels) + } +} + +func containsLabel(labels []string, label string) bool { + for _, l := range labels { + if l == label { + return true + } + } + return false +} + func formatPostRequest(formatter issues.IssueFormatter, req issues.PostRequest) (string, error) { // These fields can vary based on the test env so we set them to arbitrary // values here. diff --git a/pkg/cmd/roachprod-microbench/main.go b/pkg/cmd/roachprod-microbench/main.go index d8f825457353..64dc118cc141 100644 --- a/pkg/cmd/roachprod-microbench/main.go +++ b/pkg/cmd/roachprod-microbench/main.go @@ -173,6 +173,12 @@ func makeCompareCommand() *cobra.Command { return err } } + if c.postIssues { + err = c.postRegressionIssues(links, comparisonResult) + if err != nil { + return err + } + } } if c.influxConfig.token != "" { @@ -207,6 +213,7 @@ func makeCompareCommand() *cobra.Command { cmd.Flags().StringVar(&config.influxConfig.token, "influx-token", config.influxConfig.token, "pass an InfluxDB auth token to push the results to InfluxDB") cmd.Flags().StringToStringVar(&config.influxConfig.metadata, "influx-metadata", config.influxConfig.metadata, "pass metadata to add to the InfluxDB measurement") cmd.Flags().Float64Var(&config.threshold, "threshold", config.threshold, "threshold in percentage value for detecting perf regression ") + cmd.Flags().BoolVar(&config.postIssues, "post-issues", config.postIssues, "post GitHub issues for performance regressions (requires env vars for github issues to be set)") return cmd }