Skip to content

Commit 6bf0691

Browse files
committed
microbench-ci: combine compare and post
Previously, the microbench-ci tool had separate commands for comparing benchmark results and posting them to GitHub. This change combines the compare and post functionality into a single command, simplifying the CI workflow by reducing the number of commands needed. When `--post` is specified, the tool will both generate the comparison summary and conditionally post it as a GitHub comment. Previously, a single comment was posted or updated on the PR for each invocation of the tool. Now, a new comment will be posted for each invocation of the tool if either the `X-perf-check` label is present or if the benchmark results have significant changes. Epic: None Release note: None
1 parent 345c50c commit 6bf0691

File tree

10 files changed

+174
-350
lines changed

10 files changed

+174
-350
lines changed

.github/workflows/microbenchmarks-ci.yaml

Lines changed: 6 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -72,32 +72,11 @@ jobs:
7272
compare:
7373
runs-on: [self-hosted, basic_runner_group]
7474
timeout-minutes: 30
75-
needs: [base, run-group-1, run-group-2]
76-
steps:
77-
- name: Checkout
78-
uses: actions/checkout@v4
79-
- run: ./build/github/get-engflow-keys.sh
80-
shell: bash
81-
- name: Unique Build ID
82-
run: echo "BUILD_ID=${{ github.run_id }}-${{ github.run_attempt }}" >> $GITHUB_ENV
83-
- name: Compare and Post
84-
run: ./build/github/microbenchmarks/compare.sh
85-
env:
86-
BASE_SHA: ${{ needs.base.outputs.merge_base }}
87-
HEAD_SHA: ${{ env.HEAD }}
88-
- name: Clean up
89-
run: ./build/github/cleanup-engflow-keys.sh
90-
shell: bash
91-
if: always()
92-
post:
93-
runs-on: [ self-hosted, basic_runner_group ]
94-
if: contains(github.event.pull_request.labels.*.name, 'T-microbench-post') # TODO: remove this check once results are confirmed to be stable on CI.
95-
timeout-minutes: 30
9675
permissions:
9776
contents: read
9877
pull-requests: write
99-
issues: write
100-
needs: [ base, compare ]
78+
issues: write
79+
needs: [base, run-group-1, run-group-2]
10180
steps:
10281
- name: Checkout
10382
uses: actions/checkout@v4
@@ -106,12 +85,13 @@ jobs:
10685
- name: Unique Build ID
10786
run: echo "BUILD_ID=${{ github.run_id }}-${{ github.run_attempt }}" >> $GITHUB_ENV
10887
- name: Compare and Post
109-
run: ./build/github/microbenchmarks/post.sh
88+
run: ./build/github/microbenchmarks/compare.sh
11089
env:
90+
BASE_SHA: ${{ needs.base.outputs.merge_base }}
11191
HEAD_SHA: ${{ env.HEAD }}
11292
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
113-
REPO: "cockroachdb/cockroach"
114-
PR_NUMBER: ${{ github.event.pull_request.number }}
93+
GITHUB_REPO: "cockroachdb/cockroach"
94+
GITHUB_PR_NUMBER: ${{ github.event.pull_request.number }}
11595
- name: Clean up
11696
run: ./build/github/cleanup-engflow-keys.sh
11797
shell: bash

build/github/microbenchmarks/compare.sh

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@ done
2828
--github-summary="$output_dir/summary.md" \
2929
--build-id="$BUILD_ID" \
3030
--old="$BASE_SHA" \
31-
--new="$HEAD_SHA"
31+
--new="$HEAD_SHA" \
32+
--post
3233

3334
cat "$output_dir/summary.md" > "$GITHUB_STEP_SUMMARY"
3435

build/github/microbenchmarks/post.sh

Lines changed: 0 additions & 20 deletions
This file was deleted.

pkg/cmd/microbench-ci/compare_test.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package main
77

88
import (
9+
"fmt"
910
"os"
1011
"os/exec"
1112
"path"
@@ -107,13 +108,13 @@ func TestRunAndCompare(t *testing.T) {
107108
require.NoError(t, err)
108109
results, err := suite.Benchmarks.compareBenchmarks()
109110
require.NoError(t, err)
110-
err = results.writeGitHubSummary(config.GitHubSummaryPath)
111+
summary, err := results.githubSummary()
111112
require.NoError(t, err)
112113
err = results.writeJSONSummary(config.SummaryPath)
113114
require.NoError(t, err)
114-
data, err := os.ReadFile(config.GitHubSummaryPath)
115-
require.NoError(t, err)
116-
return string(data)
115+
return summary
116+
case "changed":
117+
return fmt.Sprintf("%t", suite.changeDetected())
117118
case "json":
118119
data, err := os.ReadFile(config.SummaryPath)
119120
require.NoError(t, err)

pkg/cmd/microbench-ci/github.go

Lines changed: 74 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -9,60 +9,102 @@ import (
99
"context"
1010
"fmt"
1111
"os"
12+
"strconv"
1213
"strings"
1314

1415
"github.com/cockroachdb/errors"
1516
"github.com/google/go-github/v61/github"
1617
"golang.org/x/oauth2"
1718
)
1819

19-
// CommentTag is used to identify an existing comment.
20-
const CommentTag = "<!-- microbench-ci -->"
20+
type GitHubConfig struct {
21+
client *github.Client
22+
Owner string
23+
Repo string
24+
PrNum int
25+
}
26+
27+
const (
28+
CommentTag = "<!-- microbench-ci -->"
29+
PerfLabel = "X-perf-check"
30+
)
2131

22-
// post posts or updates a comment on a GitHub PR, with the given summary text,
23-
// that has the CommentTag marker in it.
24-
func post(summaryText, githubRepository string, prNumber int) error {
32+
// NewGithubConfig creates a new GitHubConfig from the environment variables.
33+
func NewGithubConfig() (*GitHubConfig, error) {
2534
token := os.Getenv("GITHUB_TOKEN")
2635
if token == "" {
27-
return errors.New("GITHUB_TOKEN is not set, this command is meant to be run in a GitHub Action")
36+
return nil, errors.New("GITHUB_TOKEN is not set")
2837
}
2938

30-
repoInfo := strings.Split(githubRepository, "/")
31-
if len(repoInfo) != 2 {
32-
return errors.New("invalid GitHub repository flag")
33-
}
34-
owner := repoInfo[0]
35-
repo := repoInfo[1]
36-
3739
ctx := context.Background()
3840
ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token})
3941
tc := oauth2.NewClient(ctx, ts)
4042
client := github.NewClient(tc)
4143

42-
// Check for an existing comment
43-
comments, _, err := client.Issues.ListComments(ctx, owner, repo, prNumber, nil)
44+
githubRepository := os.Getenv("GITHUB_REPO")
45+
if githubRepository == "" {
46+
return nil, errors.New("GITHUB_REPO is not set")
47+
}
48+
prNumber := os.Getenv("GITHUB_PR_NUMBER")
49+
if prNumber == "" {
50+
return nil, errors.New("GITHUB_PR_NUMBER is not set")
51+
}
52+
prNumberInt, err := strconv.Atoi(prNumber)
4453
if err != nil {
45-
return err
54+
return nil, errors.Newf("invalid GITHUB_PR_NUMBER format %s", prNumber)
4655
}
47-
var existingComment *github.IssueComment
48-
for _, comment := range comments {
49-
if strings.Contains(comment.GetBody(), CommentTag) {
50-
existingComment = comment
51-
break
52-
}
56+
repoInfo := strings.Split(githubRepository, "/")
57+
if len(repoInfo) != 2 {
58+
return nil, errors.Newf("invalid GitHub repository flag, %s", githubRepository)
5359
}
60+
owner := repoInfo[0]
61+
repo := repoInfo[1]
62+
63+
return &GitHubConfig{
64+
client: client,
65+
Owner: owner,
66+
Repo: repo,
67+
PrNum: prNumberInt,
68+
}, nil
69+
}
5470

71+
// postComment posts a comment on a GitHub PR, with the given summary text, that
72+
// has the CommentTag marker in it.
73+
func (c *GitHubConfig) postComment(summaryText string) error {
74+
ctx := context.Background()
5575
commentBody := github.String(fmt.Sprintf("%s\n%s", summaryText, CommentTag))
56-
if existingComment != nil {
57-
// Update the existing comment
58-
existingComment.Body = commentBody
59-
_, _, err = client.Issues.EditComment(ctx, owner, repo, existingComment.GetID(), existingComment)
60-
return err
61-
}
76+
_, _, err := c.client.Issues.CreateComment(
77+
ctx, c.Owner, c.Repo, c.PrNum,
78+
&github.IssueComment{
79+
Body: commentBody,
80+
},
81+
)
82+
return err
83+
}
6284

63-
// Create a new comment
64-
_, _, err = client.Issues.CreateComment(ctx, owner, repo, prNumber, &github.IssueComment{
65-
Body: commentBody,
66-
})
85+
// addLabel adds a label to a GitHub PR.
86+
func (c *GitHubConfig) addLabel(label string) error {
87+
ctx := context.Background()
88+
_, _, err := c.client.Issues.AddLabelsToIssue(
89+
ctx, c.Owner, c.Repo, c.PrNum,
90+
[]string{label},
91+
)
6792
return err
6893
}
94+
95+
// hasLabel checks if a GitHub PR has a label.
96+
func (c *GitHubConfig) hasLabel(label string) (bool, error) {
97+
ctx := context.Background()
98+
labels, _, err := c.client.Issues.ListLabelsByIssue(
99+
ctx, c.Owner, c.Repo, c.PrNum, &github.ListOptions{PerPage: 100},
100+
)
101+
if err != nil {
102+
return false, err
103+
}
104+
for _, l := range labels {
105+
if *l.Name == label {
106+
return true, nil
107+
}
108+
}
109+
return false, nil
110+
}

0 commit comments

Comments
 (0)