From 3bc231b486d83588b6a9017945cd1cc564c04c7a Mon Sep 17 00:00:00 2001 From: Sambhav Jain Date: Mon, 8 Jul 2024 16:17:55 +0530 Subject: [PATCH] roachprod-microbench: add threshold flag to compare command Epic: none Release note: None As part of fixing https://github.com/cockroachdb/cockroach/issues/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 https://github.com/cockroachdb/cockroach/pull/126810 which exposed `roachprod-microbench` capability of cleaning the benchmark output. --- pkg/cmd/roachprod-microbench/compare.go | 107 +++++++++++++++++- .../roachprod-microbench/google/service.go | 37 ++---- pkg/cmd/roachprod-microbench/main.go | 6 + 3 files changed, 121 insertions(+), 29 deletions(-) diff --git a/pkg/cmd/roachprod-microbench/compare.go b/pkg/cmd/roachprod-microbench/compare.go index ec330b9abefa..b32e9b7c38f3 100644 --- a/pkg/cmd/roachprod-microbench/compare.go +++ b/pkg/cmd/roachprod-microbench/compare.go @@ -37,6 +37,7 @@ type compareConfig struct { slackUser string slackChannel string slackToken string + threshold float64 } type compare struct { @@ -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 }} @@ -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", } @@ -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) { @@ -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) } @@ -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 { diff --git a/pkg/cmd/roachprod-microbench/google/service.go b/pkg/cmd/roachprod-microbench/google/service.go index 2fc61f006730..d0e8dcecf6a3 100644 --- a/pkg/cmd/roachprod-microbench/google/service.go +++ b/pkg/cmd/roachprod-microbench/google/service.go @@ -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. diff --git a/pkg/cmd/roachprod-microbench/main.go b/pkg/cmd/roachprod-microbench/main.go index 1c050c7b5c6d..4e8207cfc3a2 100644 --- a/pkg/cmd/roachprod-microbench/main.go +++ b/pkg/cmd/roachprod-microbench/main.go @@ -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 @@ -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 }