Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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} \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is required and cannot be the default setting? Is there any similar flag available for the slack notification?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag already exists, I am just using it. See here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we'd only want this to happen on a triggered build. Since people are allowed to run custom comparisons which should not post (although it's not done often).

2>&1 | tee "$output_dir/sheets.txt"
else
echo "No microbenchmarks were run. Skipping comparison."
Expand Down
49 changes: 49 additions & 0 deletions pkg/cmd/roachprod-microbench/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -37,6 +38,7 @@ type compareConfig struct {
baselineDir string
sheetDesc string
threshold float64
postIssues bool
}

type slackConfig struct {
Expand Down Expand Up @@ -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()
Expand Down
41 changes: 41 additions & 0 deletions pkg/cmd/roachprod-microbench/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,47 @@ 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))

benchmarkName := regressions[0].benchmarkName
f := githubpost.MicrobenchmarkFailure(
pkgName,
benchmarkName,
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,
Expand Down
169 changes: 169 additions & 0 deletions pkg/cmd/roachprod-microbench/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
"github.com/cockroachdb/datadriven"
"github.com/cockroachdb/errors"
"github.com/stretchr/testify/require"
)

func TestCreatePostRequest(t *testing.T) {
Expand Down Expand Up @@ -55,6 +56,174 @@ 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
require.Contains(t, req.Labels, "O-microbench")
require.Contains(t, req.Labels, "C-performance")

// 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)
require.Contains(t, title, "pkg/sql")
require.Contains(t, title, "performance regression")

// Verify message contains key information
require.Contains(t, req.Message, "pkg/sql")
require.Contains(t, req.Message, "BenchmarkScan")
require.Contains(t, req.Message, "25.5%")
require.Contains(t, req.Message, "https://docs.google.com/spreadsheets/d/test123")
require.Contains(t, req.Message, "master -> release-24.1")
}

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
require.Contains(t, req.Message, "15 benchmark(s)")

// Should truncate to 10 and show "... and 5 more"
require.Contains(t, req.Message, "5 more regression(s)")

// First regression should be present
require.Contains(t, req.Message, "Benchmark0")

// 11th regression should not be listed individually
require.NotContains(t, req.Message, "Benchmark10")
}

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++ {
require.Contains(t, req.Message, fmt.Sprintf("Benchmark%d", i))
}

// Should not show truncation message for exactly 10
require.NotContains(t, req.Message, "more regression(s)")
}

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)
require.NoError(t, 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
require.Contains(t, title, "pkg/sql/exec")
require.Contains(t, title, "performance regression")

// 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 {
require.Contains(t, output, expected)
}

// Verify labels are correct
require.Contains(t, req.Labels, "O-microbench")
require.Contains(t, req.Labels, "C-performance")
}

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.
Expand Down
7 changes: 7 additions & 0 deletions pkg/cmd/roachprod-microbench/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 != "" {
Expand Down Expand Up @@ -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
}

Expand Down
Loading