Skip to content

Commit

Permalink
roachprod-microbench: add threshold flag to compare command
Browse files Browse the repository at this point in the history
Epic: none

Release note: None

As part of fixing cockroachdb#106661, we need to run microbenchmarks in a CI job and compare the microbenchmarks with that of the latest master branch commit.

For this purpose we want to re-use `roachprod-microbench` compare command. However, as of current implementation of the compare command, it doesn't do threshold comparison and this PR aims to add that capability using a flag.

It is a follow up change to cockroachdb#126810 which exposed `roachprod-microbench` capability of cleaning the benchmark output.
  • Loading branch information
sambhav-jain-16 committed Jul 12, 2024
1 parent d51907a commit 3bc231b
Show file tree
Hide file tree
Showing 3 changed files with 121 additions and 29 deletions.
107 changes: 103 additions & 4 deletions pkg/cmd/roachprod-microbench/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ type compareConfig struct {
slackUser string
slackChannel string
slackToken string
threshold float64
}

type compare struct {
Expand All @@ -50,8 +51,23 @@ const (
packageSeparator = "→"
slackPercentageThreshold = 20.0
slackReportMax = 3
skipComparison = math.MaxFloat64
)

type PackageComparison struct {
PackageName string
Metrics []MetricComparison
}

type MetricComparison struct {
MetricKey string
Result string
}

type ComparisonReport struct {
PackageComparisons []PackageComparison
}

const slackCompareTemplateScript = `
{{ range .Metrics }}*Top {{ len .Changes }} significant change(s) for metric: {{ .MetricName }}*
{{ range .Changes }}• {{ .BenchmarkName }} {{.ChangeSymbol}}{{ .PercentChange }}
Expand All @@ -66,15 +82,19 @@ func newCompare(config compareConfig) (*compare, error) {
return nil, err
}
ctx := context.Background()
service, err := google.New(ctx)
if err != nil {
return nil, err
var service *google.Service
if config.threshold == skipComparison {
service, err = google.New(ctx)
if err != nil {
return nil, err
}
}
return &compare{compareConfig: config, service: service, packages: packages, ctx: ctx}, nil
}

func defaultCompareConfig() compareConfig {
return compareConfig{
threshold: skipComparison, // Skip comparison by default
slackUser: "microbench",
slackChannel: "perf-ops",
}
Expand Down Expand Up @@ -113,6 +133,35 @@ func (c *compare) readMetrics() (map[string]*model.MetricMap, error) {
return metricMaps, nil
}

func (c *compare) createComparisons(
metricMap model.MetricMap, oldID string, newID string,
) map[string]map[string]*model.Comparison {

metricKeys := make([]string, 0, len(metricMap))
for sheetName := range metricMap {
metricKeys = append(metricKeys, sheetName)
}

comparisonMap := make(map[string]map[string]*model.Comparison, len(metricKeys))
for metricKey, metric := range metricMap {

// Compute comparisons for each benchmark present in both runs.
comparisons := make(map[string]*model.Comparison)
for name := range metric.BenchmarkEntries {
comparison := metric.ComputeComparison(name, oldID, newID)
if comparison != nil {
comparisons[name] = comparison
}
}

if len(comparisons) != 0 {
comparisonMap[metricKey] = comparisons
}
}

return comparisonMap
}

func (c *compare) publishToGoogleSheets(
metricMaps map[string]*model.MetricMap,
) (map[string]string, error) {
Expand All @@ -122,7 +171,9 @@ func (c *compare) publishToGoogleSheets(
if c.sheetDesc != "" {
sheetName = fmt.Sprintf("%s (%s)", sheetName, c.sheetDesc)
}
url, err := c.service.CreateSheet(c.ctx, sheetName, *metricMap, "old", "new")

comparisonMap := c.createComparisons(*metricMap, "old", "new")
url, err := c.service.CreateSheet(c.ctx, sheetName, *metricMap, comparisonMap, "old", "new")
if err != nil {
return nil, errors.Wrapf(err, "failed to create sheet for %s", pkgGroup)
}
Expand Down Expand Up @@ -241,6 +292,54 @@ func (c *compare) postToSlack(
)
}

func (r *ComparisonReport) getReportString() string {
var builder strings.Builder
for _, pkgError := range r.PackageComparisons {
builder.WriteString(fmt.Sprintf("Package: %s\n", pkgError.PackageName))
for _, metricError := range pkgError.Metrics {
builder.WriteString(fmt.Sprintf("\tMetric: %s, Result: %s\n", metricError.MetricKey, metricError.Result))
}
}
return builder.String()
}

func (c *compare) compareUsingThreshold(metricMap map[string]*model.MetricMap) error {

report := ComparisonReport{}

for pkgName, metricMap := range metricMap {
var metrics []MetricComparison
comparisonMap := c.createComparisons(*metricMap, "old", "new")

for metricKey, comparisons := range comparisonMap {
for benchmarkName, comparison := range comparisons {
// If Delta is more negative than the threshold, then there's a concerning perf regression
if comparison.Delta+(c.threshold*100) < 0 {
metrics = append(metrics, MetricComparison{
MetricKey: metricKey,
Result: fmt.Sprintf("%s : %s", benchmarkName, comparison.FormattedDelta),
})
}
}
}

if len(metrics) > 0 {
report.PackageComparisons = append(report.PackageComparisons, PackageComparison{
PackageName: pkgName,
Metrics: metrics,
})
}
}

if len(report.PackageComparisons) > 0 {
reportString := report.getReportString()
return errors.Newf("There are benchmark regressions of > %.2f%% in the following packages \n %s\n",
c.threshold*100, reportString)
}

return nil
}

func processReportFile(builder *model.Builder, id, pkg, path string) error {
file, err := os.Open(path)
if err != nil {
Expand Down
37 changes: 12 additions & 25 deletions pkg/cmd/roachprod-microbench/google/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,38 +79,25 @@ func (srv *Service) testServices(ctx context.Context) error {

// CreateSheet creates a new Google spreadsheet with the provided metric data.
func (srv *Service) CreateSheet(
ctx context.Context, name string, metricMap model.MetricMap, oldID, newID string,
ctx context.Context,
name string,
metricMap model.MetricMap,
comparisonMap map[string]map[string]*model.Comparison,
oldID, newID string,
) (string, error) {
var s sheets.Spreadsheet
s.Properties = &sheets.SpreadsheetProperties{Title: name}

// Sort sheets by name in reverse order. This ensures `sec/op` is the first
// sheet and metric in the summary.
sheetNames := make([]string, 0, len(metricMap))
for sheetName := range metricMap {
sheetNames = append(sheetNames, sheetName)
}
sort.Sort(sort.Reverse(sort.StringSlice(sheetNames)))

// Raw data sheets.
sheetInfos, idx := make([]rawSheetInfo, 0, len(metricMap)), 0
for _, sheetName := range sheetNames {
for sheetName, comparisons := range comparisonMap {
metric := metricMap[sheetName]
// Compute comparisons for each benchmark present in both runs.
comparisons := make(map[string]*model.Comparison)
for name := range metric.BenchmarkEntries {
comparison := metric.ComputeComparison(name, oldID, newID)
if comparison != nil {
comparisons[name] = comparison
}
}
// Only generate a sheet if there are comparisons to show.
if len(comparisons) != 0 {
sh, info := srv.createRawSheet(metric, comparisons, oldID, newID, idx)
s.Sheets = append(s.Sheets, sh)
sheetInfos = append(sheetInfos, info)
idx++
}

sh, info := srv.createRawSheet(metric, comparisons, oldID, newID, idx)
s.Sheets = append(s.Sheets, sh)
sheetInfos = append(sheetInfos, info)
idx++

}

// Pivot table overview sheet. Place in front.
Expand Down
6 changes: 6 additions & 0 deletions pkg/cmd/roachprod-microbench/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,11 @@ func makeCompareCommand() *cobra.Command {
return err
}

// if the threshold is set, we just want to report it and not move further
if config.threshold != skipComparison {
return c.compareUsingThreshold(metricMaps)
}

links, err := c.publishToGoogleSheets(metricMaps)
if err != nil {
return err
Expand All @@ -166,6 +171,7 @@ func makeCompareCommand() *cobra.Command {
cmd.Flags().StringVar(&config.slackToken, "slack-token", config.slackToken, "pass a slack token to post the results to a slack channel")
cmd.Flags().StringVar(&config.slackUser, "slack-user", config.slackUser, "slack user to post the results as")
cmd.Flags().StringVar(&config.slackChannel, "slack-channel", config.slackChannel, "slack channel to post the results to")
cmd.Flags().Float64Var(&config.threshold, "threshold", config.threshold, "threshold for detecting perf regression")
return cmd
}

Expand Down

0 comments on commit 3bc231b

Please sign in to comment.