[bisection] implement benchmark interface
This creates an interface for all benchmarks to implement.
We start with telemetry, which deprecates telemetry_exp.go.
It implements waterfall enabled gtests and story tags.
All of pinpoint.go is updated to get the appropriate benchmark
and Run() accordingly.
Bug: b/318863812, b/318863812, b/321299939
Change-Id: Ie267f07f31a15df54dde1633711fc959e36b3e1b
Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/814037
Reviewed-by: Hao Wu <haowoo@google.com>
Commit-Queue: Jeff Yoon <jeffyoon@google.com>
diff --git a/pinpoint/go/bot_configs/bot_configs.go b/pinpoint/go/bot_configs/bot_configs.go
index 91e2ebf..95e76f7 100644
--- a/pinpoint/go/bot_configs/bot_configs.go
+++ b/pinpoint/go/bot_configs/bot_configs.go
@@ -38,8 +38,11 @@
// Browser is the name of the Chrome browser to run
// when testing benchmarks
Browser string `json:"browser"`
- // Builder refers to the LUCI pool to build Chrome,
+ // Bucket refers to the LUCI pool to build Chrome,
// typically luci.chrome.try
+ Bucket string `json:"bucket"`
+ // Builder refers to the LUCI builder used to build Chrome,
+ // usually a compile builder.
Builder string `json:"builder"`
// Dimensions are used by Swarming to find the
// device pool to test benchmarks
@@ -50,6 +53,8 @@
// SwarmingServer is almost always
// https://chrome-swarming.appspot.com
SwarmingServer string `json:"swarming_server"`
+ // Bot is the original key used for this config.
+ Bot string
}
// GetBotConfig gets the config for a bot. Will only check internal
@@ -77,9 +82,14 @@
}
return BotConfig{}, fmt.Errorf(errMsg)
}
- if cfg.Alias != "" {
- return botConfigs[cfg.Alias], nil
+ alias := cfg.Alias
+ if alias != "" {
+ cfg = botConfigs[cfg.Alias]
+ cfg.Bot = alias
+ return cfg, nil
}
+
+ cfg.Bot = bot
return cfg, nil
}
diff --git a/pinpoint/go/bot_configs/bot_configs_test.go b/pinpoint/go/bot_configs/bot_configs_test.go
index 23428a0..0358b6c 100644
--- a/pinpoint/go/bot_configs/bot_configs_test.go
+++ b/pinpoint/go/bot_configs/bot_configs_test.go
@@ -40,12 +40,6 @@
externalOnly: false,
expectedError: true,
},
- {
- name: "check alias",
- bot: "Android Go",
- externalOnly: true,
- expectedError: false,
- },
} {
t.Run(fmt.Sprintf("[%d] %s", i, test.name), func(t *testing.T) {
bot, err := GetBotConfig(test.bot, test.externalOnly)
@@ -62,3 +56,9 @@
func TestValidateBotConfigs(t *testing.T) {
assert.NoError(t, validate())
}
+
+func TestGetBotConfig_AliasedBuilder_CorrectReference(t *testing.T) {
+ cfg, err := GetBotConfig("Android Go", true)
+ assert.NoError(t, err)
+ assert.Equal(t, "android-go-perf", cfg.Bot)
+}
diff --git a/pinpoint/go/pinpoint/pinpoint.go b/pinpoint/go/pinpoint/pinpoint.go
index 6a6319d..56aff75 100644
--- a/pinpoint/go/pinpoint/pinpoint.go
+++ b/pinpoint/go/pinpoint/pinpoint.go
@@ -193,7 +193,7 @@
c.tests = &testMetadata{
req: c.createRunBenchmarkRequest(jobID, cfg, target, req),
}
- tasks, err := c.scheduleRunBenchmark(ctx, pp.sc)
+ tasks, err := c.scheduleRunBenchmark(ctx, pp.sc, req)
if err != nil {
return resp, err
}
@@ -242,7 +242,7 @@
}
if res != nil {
spew.Dump(res)
- culprit, err := cdl.updateCommitsByResult(ctx, pp.sc, pp.mc, res, left, right)
+ culprit, err := cdl.updateCommitsByResult(ctx, pp.sc, pp.mc, res, left, right, req)
if err != nil {
return resp, skerr.Wrapf(err, "could not update commitDataList after compare")
}
@@ -259,7 +259,7 @@
}
if res != nil {
spew.Dump(res)
- culprit, err := cdl.updateCommitsByResult(ctx, pp.sc, pp.mc, res, left, right)
+ culprit, err := cdl.updateCommitsByResult(ctx, pp.sc, pp.mc, res, left, right, req)
if err != nil {
return resp, skerr.Wrapf(err, "could not update commitDataList after compare")
}
@@ -370,7 +370,7 @@
}
// scheduleRunBenchmark schedules run benchmark tests to swarming and returns the task IDs
-func (c *commitData) scheduleRunBenchmark(ctx context.Context, sc backends.SwarmingClient) ([]string, error) {
+func (c *commitData) scheduleRunBenchmark(ctx context.Context, sc backends.SwarmingClient, req *ppb.ScheduleBisectRequest) ([]string, error) {
if c.tests == nil || c.tests.req == nil {
return nil, skerr.Fmt("Cannot schedule benchmark runs without request")
}
@@ -381,11 +381,11 @@
}
if len(tasks) < maxSampleSize {
for i := 0; i < interval; i++ {
- task, err := run_benchmark.Run(ctx, sc, *c.tests.req)
+ task, err := run_benchmark.Run(ctx, sc, req, c.tests.req.Commit, c.tests.req.JobID, c.tests.req.Build, 1)
if err != nil {
return nil, skerr.Wrapf(err, "Could not start run benchmark task for request %v", c.tests.req)
}
- tasks = append(tasks, task)
+ tasks = append(tasks, task[0].TaskId)
}
}
return tasks, nil
@@ -513,8 +513,7 @@
// updateCommitsByResult takes the compare results and determines the next
// steps in the workflow. Changes are made to CommitDataList depending
// on what the compare verdict is.
-func (cdl *commitDataList) updateCommitsByResult(ctx context.Context, sc backends.SwarmingClient, mh midpoint.MidpointHandler, res *compare.CompareResults,
- left, right int) (*midpoint.Commit, error) {
+func (cdl *commitDataList) updateCommitsByResult(ctx context.Context, sc backends.SwarmingClient, mh midpoint.MidpointHandler, res *compare.CompareResults, left, right int, req *ppb.ScheduleBisectRequest) (*midpoint.Commit, error) {
if left < 0 || right >= len(cdl.commits) {
return nil, skerr.Fmt("cannot update commitDataList with left %d and right %d index out of bounds", left, right)
}
@@ -522,7 +521,7 @@
return nil, skerr.Fmt("cannot update commitDataList with left %d index >= right %d", left, right)
}
if res.Verdict == compare.Unknown {
- return nil, cdl.runMoreTestsIfNeeded(ctx, sc, left, right)
+ return nil, cdl.runMoreTestsIfNeeded(ctx, sc, left, right, req)
} else if res.Verdict == compare.Different {
return cdl.findMidpointOrCulprit(ctx, mh, left, right)
}
@@ -555,9 +554,9 @@
}
// runMoreTestsIfNeeded adds more run_benchmark tasks to the left and right commit
-func (cdl *commitDataList) runMoreTestsIfNeeded(ctx context.Context, sc backends.SwarmingClient, left, right int) error {
+func (cdl *commitDataList) runMoreTestsIfNeeded(ctx context.Context, sc backends.SwarmingClient, left, right int, req *ppb.ScheduleBisectRequest) error {
c := cdl.commits[left]
- tasks, err := c.scheduleRunBenchmark(ctx, sc)
+ tasks, err := c.scheduleRunBenchmark(ctx, sc, req)
if err != nil {
return skerr.Wrapf(err, "could not schedule more tasks for left commit [%d] %s", left, c.commit.GitHash[:7])
}
@@ -566,7 +565,7 @@
c.tests.isRunning = true
}
c = cdl.commits[right]
- tasks, err = c.scheduleRunBenchmark(ctx, sc)
+ tasks, err = c.scheduleRunBenchmark(ctx, sc, req)
if err != nil {
return skerr.Wrapf(err, "could not schedule more tasks for right commit [%d] %s", right, c.commit.GitHash[:7])
}
diff --git a/pinpoint/go/pinpoint/pinpoint_test.go b/pinpoint/go/pinpoint/pinpoint_test.go
index 624b4f1..b953c7a 100644
--- a/pinpoint/go/pinpoint/pinpoint_test.go
+++ b/pinpoint/go/pinpoint/pinpoint_test.go
@@ -352,7 +352,7 @@
sc := &backends.SwarmingClientImpl{
ApiClient: msc,
}
- tasks, err := c.scheduleRunBenchmark(ctx, sc)
+ tasks, err := c.scheduleRunBenchmark(ctx, sc, req)
So(err, ShouldBeNil)
So(tasks[0], ShouldEqual, "new_task")
So(len(tasks), ShouldEqual, interval)
@@ -364,7 +364,7 @@
sc := &backends.SwarmingClientImpl{
ApiClient: msc,
}
- tasks, err := c.scheduleRunBenchmark(ctx, sc)
+ tasks, err := c.scheduleRunBenchmark(ctx, sc, req)
So(err, ShouldErrLike, "Cannot schedule benchmark runs without request")
So(tasks, ShouldBeNil)
})
@@ -375,7 +375,7 @@
sc := &backends.SwarmingClientImpl{
ApiClient: msc,
}
- tasks, err := c.scheduleRunBenchmark(ctx, sc)
+ tasks, err := c.scheduleRunBenchmark(ctx, sc, req)
So(err, ShouldErrLike, "Cannot schedule benchmark runs without request")
So(tasks, ShouldBeNil)
})
@@ -406,7 +406,7 @@
sc := &backends.SwarmingClientImpl{
ApiClient: msc,
}
- tasks, err := c.scheduleRunBenchmark(ctx, sc)
+ tasks, err := c.scheduleRunBenchmark(ctx, sc, req)
So(err, ShouldErrLike, errMsg)
So(tasks, ShouldBeNil)
})
@@ -776,6 +776,7 @@
msc := swarmingMocks.NewApiClient(t)
c := mockhttpclient.NewURLMock().Client()
mmh := midpoint.New(ctx, c)
+ req := defaultRunRequest()
Convey(`Error`, t, func() {
cdl := commitDataList{
@@ -790,7 +791,7 @@
sc := &backends.SwarmingClientImpl{
ApiClient: msc,
}
- mid, err := cdl.updateCommitsByResult(ctx, sc, mmh, res, left, right)
+ mid, err := cdl.updateCommitsByResult(ctx, sc, mmh, res, left, right, req)
So(err, ShouldErrLike, "index out of bounds")
So(mid, ShouldBeNil)
})
@@ -800,7 +801,7 @@
sc := &backends.SwarmingClientImpl{
ApiClient: msc,
}
- mid, err := cdl.updateCommitsByResult(ctx, sc, mmh, res, left, right)
+ mid, err := cdl.updateCommitsByResult(ctx, sc, mmh, res, left, right, req)
So(err, ShouldErrLike, fmt.Sprintf("left %d index >= right %d", left, right))
So(mid, ShouldBeNil)
})
@@ -885,7 +886,7 @@
sc := &backends.SwarmingClientImpl{
ApiClient: msc,
}
- err = cdl.runMoreTestsIfNeeded(ctx, sc, left, right)
+ err = cdl.runMoreTestsIfNeeded(ctx, sc, left, right, req)
So(err, ShouldBeNil)
So(len(lcommit.tests.tasks), ShouldEqual, interval+2)
So(lcommit.tests.tasks[0], ShouldEqual, "old_left_task_1")
diff --git a/pinpoint/go/run_benchmark/BUILD.bazel b/pinpoint/go/run_benchmark/BUILD.bazel
index dd58ff9..2da6950 100644
--- a/pinpoint/go/run_benchmark/BUILD.bazel
+++ b/pinpoint/go/run_benchmark/BUILD.bazel
@@ -4,8 +4,10 @@
go_library(
name = "run_benchmark",
srcs = [
+ "benchmark_test_factory.go",
"run_benchmark.go",
- "telemetry_exp.go",
+ "swarming_helpers.go",
+ "telemetry.go",
],
importpath = "go.skia.org/infra/pinpoint/go/run_benchmark",
visibility = ["//visibility:public"],
@@ -15,6 +17,7 @@
"//go/util",
"//pinpoint/go/backends",
"//pinpoint/go/bot_configs",
+ "//pinpoint/proto/v1:proto",
"@org_chromium_go_luci//common/api/swarming/swarming/v1:swarming",
],
)
@@ -22,8 +25,9 @@
go_test(
name = "run_benchmark_test",
srcs = [
+ "benchmark_test_factory_test.go",
"run_benchmark_test.go",
- "telemetry_exp_test.go",
+ "telemetry_test.go",
],
embed = [":run_benchmark"],
deps = [
@@ -31,11 +35,9 @@
"//go/swarming",
"//go/swarming/mocks",
"//pinpoint/go/backends",
- "//pinpoint/go/bot_configs",
- "@com_github_smartystreets_goconvey//convey",
+ "//pinpoint/proto/v1:proto",
"@com_github_stretchr_testify//assert",
"@com_github_stretchr_testify//mock",
"@org_chromium_go_luci//common/api/swarming/swarming/v1:swarming",
- "@org_chromium_go_luci//common/testing/assertions",
],
)
diff --git a/pinpoint/go/run_benchmark/benchmark_test_factory.go b/pinpoint/go/run_benchmark/benchmark_test_factory.go
new file mode 100644
index 0000000..97836ca
--- /dev/null
+++ b/pinpoint/go/run_benchmark/benchmark_test_factory.go
@@ -0,0 +1,54 @@
+package run_benchmark
+
+import (
+ "strings"
+
+ "go.skia.org/infra/go/skerr"
+
+ bc "go.skia.org/infra/pinpoint/go/bot_configs"
+ ppb "go.skia.org/infra/pinpoint/proto/v1"
+)
+
+type BenchmarkTest interface {
+ GetCommand() []string
+}
+
+// NewBenchmarkTest returns a BenchmarkTest based on the request parameters.
+// The Configuration (bot) is used alongside the benchmark to determine the
+// isolate target for that combination. Based on the isolate target,
+func NewBenchmarkTest(req *ppb.ScheduleBisectRequest, commit string) (BenchmarkTest, error) {
+ bot, benchmark := req.Configuration, req.Benchmark
+ config, err := bc.GetBotConfig(bot, false)
+ if err != nil {
+ return nil, skerr.Wrapf(err, "Failed to fetch bot configs to create benchmark test")
+ }
+ target, err := bc.GetIsolateTarget(bot, benchmark)
+ if err != nil {
+ return nil, skerr.Wrapf(err, "Failed to get isolate target to create the benchmark test")
+ }
+
+ // These targets could have suffixes, especially for Android.
+ // For example, 'performance_test_suite_android_clank_monochrome_64_32_bundle'
+ if strings.Contains(target, "performance_test_suite") || strings.Contains(target, "telemetry_perf_tests") {
+ return &telemetryTest{
+ benchmark: req.Benchmark,
+ browser: config.Browser,
+ commit: commit,
+ story: req.Story,
+ storyTags: req.StoryTags,
+ }, nil
+ }
+
+ switch target {
+ case "performance_webview_test_suite":
+ return &telemetryTest{
+ benchmark: req.Benchmark,
+ browser: config.Browser,
+ commit: commit,
+ story: req.Story,
+ storyTags: req.StoryTags,
+ }, nil
+ default:
+ return nil, skerr.Fmt("Unsupported test target %s", target)
+ }
+}
diff --git a/pinpoint/go/run_benchmark/benchmark_test_factory_test.go b/pinpoint/go/run_benchmark/benchmark_test_factory_test.go
new file mode 100644
index 0000000..0c94a74
--- /dev/null
+++ b/pinpoint/go/run_benchmark/benchmark_test_factory_test.go
@@ -0,0 +1,32 @@
+package run_benchmark
+
+import (
+ "reflect"
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+
+ ppb "go.skia.org/infra/pinpoint/proto/v1"
+)
+
+func TestGetBenchmark_TelemetryTestBuilder_TelemetryTests(t *testing.T) {
+ expect := func(req *ppb.ScheduleBisectRequest, commit, want string) {
+ benchmark, _ := NewBenchmarkTest(req, commit)
+ assert.Equal(t, want, reflect.TypeOf(benchmark).String())
+ }
+
+ commit := "01bfa421eee3c76bbbf32510343e074060051c9f"
+ req := &ppb.ScheduleBisectRequest{
+ Configuration: "android-go-perf",
+ Benchmark: "components_perftests",
+ }
+ expect(req, commit, "*run_benchmark.telemetryTest")
+
+ req = &ppb.ScheduleBisectRequest{
+ Story: "story",
+ StoryTags: "all",
+ Configuration: "android-pixel2_webview-perf",
+ Benchmark: "performance_browser_tests",
+ }
+ expect(req, commit, "*run_benchmark.telemetryTest")
+}
diff --git a/pinpoint/go/run_benchmark/run_benchmark.go b/pinpoint/go/run_benchmark/run_benchmark.go
index 294bdb7..1d0cbc6 100644
--- a/pinpoint/go/run_benchmark/run_benchmark.go
+++ b/pinpoint/go/run_benchmark/run_benchmark.go
@@ -8,23 +8,24 @@
import (
"context"
- "fmt"
"slices"
- swarmingV1 "go.chromium.org/luci/common/api/swarming/swarming/v1"
"go.skia.org/infra/go/skerr"
"go.skia.org/infra/go/swarming"
"go.skia.org/infra/pinpoint/go/backends"
"go.skia.org/infra/pinpoint/go/bot_configs"
+
+ spb "go.chromium.org/luci/common/api/swarming/swarming/v1"
+ ppb "go.skia.org/infra/pinpoint/proto/v1"
)
-// A RunBenchmarkRequest defines the request arguments of the performance test
-// to swarming.
+// A RunBenchmarkRequest defines the request arguments of the performance test to swarming.
+// Note: This is being used in workflows/internal/run_benchmark.go.
type RunBenchmarkRequest struct {
// the Pinpoint job id
JobID string
// the swarming instance and cas digest hash and bytes location for the build
- Build *swarmingV1.SwarmingRpcsCASReference
+ Build *spb.SwarmingRpcsCASReference
// commit hash
Commit string
// device configuration
@@ -37,80 +38,6 @@
Target string
}
-// swarming request to run task
-// define static, constant fields here
-var swarmingReq = swarmingV1.SwarmingRpcsNewTaskRequest{
- BotPingToleranceSecs: 1200,
- ExpirationSecs: 86400,
- // EvaluateOnly omitted
- Name: "Pinpoint bisection run benchmark task",
- // ParentTaskId omitted
- // PoolTaskTemplate omitted
- Priority: 100,
- // define properties later
- PubsubTopic: "projects/chromeperf/topics/pinpoint-swarming-updates",
- PubsubUserdata: "UNUSED", // can populate later, see example swarming call log
- Realm: "chrome:pinpoint",
- // RequestUuid: omitted
- // Resultdb: omitted
- ServiceAccount: "chrome-tester@chops-service-accounts.iam.gserviceaccount.com",
- // define tags later
- // TaskSlices optional if properties defined
- User: "Pinpoint",
- // ForceSendFields: omitted
- // NullFields: omitted
-}
-
-func createSwarmingReq(req RunBenchmarkRequest) (
- *swarmingV1.SwarmingRpcsNewTaskRequest, error) {
- // TODO(b/318863812): add mapping from device + benchmark to the specific run test
- // currently catapult maps the device + benchmark to the target and then
- // the target dictates what test to run. We can map to target if that info
- // is useful on the UI, but for this, it's not relevant.
- // see GetIsolateTarget and _GenerateQuests here:
- // https://source.chromium.org/chromium/chromium/src/+/main:third_party/catapult/dashboard/dashboard/pinpoint/handlers/new.py;drc=8fe602e47f11cfdd79225696f1f6a5556b57c58c;l=466
- // TODO(b/321299939): create an interface for different runBenchmark types
- // and refactor telemetryExp to use that interface
- exp := telemetryExp{}
- cmd, err := exp.createCmd(req)
- if err != nil {
- return nil, skerr.Fmt("Unable to create run benchmark command due to %s\n", err)
- }
- dim := make([]*swarmingV1.SwarmingRpcsStringPair,
- len(req.Config.Dimensions))
- for i, kv := range req.Config.Dimensions {
- dim[i] = &swarmingV1.SwarmingRpcsStringPair{
- Key: kv["key"],
- Value: kv["value"],
- }
- }
-
- swarmingReq.Properties = &swarmingV1.SwarmingRpcsTaskProperties{
- CasInputRoot: &swarmingV1.SwarmingRpcsCASReference{
- CasInstance: req.Build.CasInstance,
- Digest: &swarmingV1.SwarmingRpcsDigest{
- Hash: req.Build.Digest.Hash,
- SizeBytes: req.Build.Digest.SizeBytes,
- },
- },
- // TODO(b/318863812): support user submitted extra_args.
- // This support is needed for pairwise executions, not bisection.
- Command: cmd,
- Dimensions: dim,
- ExecutionTimeoutSecs: 2700,
- IoTimeoutSecs: 2700,
- RelativeCwd: "out/Release",
- }
-
- // TODO(b/318863812): update swarming task tags to more appropriate tags.
- swarmingReq.Tags = []string{
- fmt.Sprintf("pinpoint_job_id:%s", req.JobID),
- fmt.Sprintf("build_cas:%s/%d", req.Build.Digest.Hash, req.Build.Digest.SizeBytes),
- }
-
- return &swarmingReq, nil
-}
-
var runningStates = []string{
swarming.TASK_STATE_PENDING,
swarming.TASK_STATE_RUNNING,
@@ -130,16 +57,29 @@
}
// Run schedules a swarming task to run the RunBenchmarkRequest.
-func Run(ctx context.Context, sc backends.SwarmingClient, req RunBenchmarkRequest) (string, error) {
- swarmingReq, err := createSwarmingReq(req)
+func Run(ctx context.Context, sc backends.SwarmingClient, req *ppb.ScheduleBisectRequest, commit string, jobID string, buildArtifact *spb.SwarmingRpcsCASReference, iter int) ([]*spb.SwarmingRpcsTaskRequestMetadata, error) {
+ test, err := NewBenchmarkTest(req, commit)
if err != nil {
- return "", skerr.Wrapf(err, "Could not create run test request")
+ return nil, skerr.Wrapf(err, "Failed to prepare benchmark test for execution")
}
- metadataResp, err := sc.TriggerTask(ctx, swarmingReq)
+ bot := req.Configuration
+ botConfig, err := bot_configs.GetBotConfig(bot, false)
if err != nil {
- return "", skerr.Fmt("trigger task %v\ncaused error: %s", req, err)
+ return nil, skerr.Wrapf(err, "Failed to create benchmark test object")
}
- return metadataResp.TaskId, nil
+ swarmingRequest := createSwarmingRequest(jobID, test.GetCommand(), buildArtifact, botConfig.Dimensions)
+
+ resp := make([]*spb.SwarmingRpcsTaskRequestMetadata, 0)
+ for i := 0; i < iter; i++ {
+ r, err := sc.TriggerTask(ctx, swarmingRequest)
+ if err != nil {
+ return nil, skerr.Wrapf(err, "benchmark task %d with request %v failed", i, r)
+ }
+
+ resp = append(resp, r)
+ }
+
+ return resp, nil
}
diff --git a/pinpoint/go/run_benchmark/run_benchmark_test.go b/pinpoint/go/run_benchmark/run_benchmark_test.go
index f825764..8f06981 100644
--- a/pinpoint/go/run_benchmark/run_benchmark_test.go
+++ b/pinpoint/go/run_benchmark/run_benchmark_test.go
@@ -4,16 +4,16 @@
"context"
"testing"
- . "github.com/smartystreets/goconvey/convey"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
- swarmingV1 "go.chromium.org/luci/common/api/swarming/swarming/v1"
- . "go.chromium.org/luci/common/testing/assertions"
+
"go.skia.org/infra/go/skerr"
"go.skia.org/infra/go/swarming"
"go.skia.org/infra/go/swarming/mocks"
"go.skia.org/infra/pinpoint/go/backends"
- "go.skia.org/infra/pinpoint/go/bot_configs"
+
+ swarmingV1 "go.chromium.org/luci/common/api/swarming/swarming/v1"
+ ppb "go.skia.org/infra/pinpoint/proto/v1"
)
var req = RunBenchmarkRequest{
@@ -31,37 +31,38 @@
}
var expectedErr = skerr.Fmt("some error")
-func TestRun(t *testing.T) {
+func TestRun_TelemetryTest_ValidExecution(t *testing.T) {
ctx := context.Background()
mockClient := mocks.NewApiClient(t)
sc := &backends.SwarmingClientImpl{
ApiClient: mockClient,
}
- Convey(`OK`, t, func() {
- cfg, err := bot_configs.GetBotConfig("linux-perf", true)
- So(err, ShouldBeNil)
- req.Config = cfg
- mockClient.On("TriggerTask", ctx, mock.Anything).
- Return(&swarmingV1.SwarmingRpcsTaskRequestMetadata{
- TaskId: "123",
- }, nil).Once()
- taskId, err := Run(ctx, sc, req)
- So(err, ShouldBeNil)
- So(taskId, ShouldEqual, "123")
- })
- Convey(`Return error`, t, func() {
- cfg, err := bot_configs.GetBotConfig("linux-perf", true)
- So(err, ShouldBeNil)
- req.Config = cfg
- mockClient.On("TriggerTask", ctx, mock.Anything).
- Return(&swarmingV1.SwarmingRpcsTaskRequestMetadata{
- TaskId: "123",
- }, expectedErr).Once()
- taskId, err := Run(ctx, sc, req)
- So(taskId, ShouldBeEmpty)
- So(err, ShouldErrLike, expectedErr)
- })
+ bisectReq := &ppb.ScheduleBisectRequest{
+ Story: "story",
+ StoryTags: "all",
+ Configuration: "android-pixel2_webview-perf",
+ Benchmark: "performance_browser_tests",
+ }
+
+ buildArtifact := &swarmingV1.SwarmingRpcsCASReference{
+ CasInstance: "instance",
+ Digest: &swarmingV1.SwarmingRpcsDigest{
+ Hash: "hash",
+ SizeBytes: 0,
+ },
+ }
+
+ commit := "64893ca6294946163615dcf23b614afe0419bfa3"
+
+ mockClient.On("TriggerTask", ctx, mock.Anything).
+ Return(&swarmingV1.SwarmingRpcsTaskRequestMetadata{
+ TaskId: "123",
+ }, nil).Once()
+ taskIds, err := Run(ctx, sc, bisectReq, commit, "id", buildArtifact, 1)
+ assert.NoError(t, err)
+ assert.Equal(t, 1, len(taskIds))
+ assert.Equal(t, "123", taskIds[0].TaskId)
}
func TestIsTaskStateFinished_GivenCompleteStates_ReturnsTrue(t *testing.T) {
diff --git a/pinpoint/go/run_benchmark/swarming_helpers.go b/pinpoint/go/run_benchmark/swarming_helpers.go
new file mode 100644
index 0000000..78b2ef3
--- /dev/null
+++ b/pinpoint/go/run_benchmark/swarming_helpers.go
@@ -0,0 +1,72 @@
+package run_benchmark
+
+import (
+ "fmt"
+
+ spb "go.chromium.org/luci/common/api/swarming/swarming/v1"
+)
+
+func convertDimensions(dimensions []map[string]string) []*spb.SwarmingRpcsStringPair {
+ // TODO(b/318863812): add mapping from device + benchmark to the specific run test
+ // currently catapult maps the device + benchmark to the target and then
+ // the target dictates what test to run. We can map to target if that info
+ // is useful on the UI, but for this, it's not relevant.
+ // see GetIsolateTarget and _GenerateQuests here:
+ // https://source.chromium.org/chromium/chromium/src/+/main:third_party/catapult/dashboard/dashboard/pinpoint/handlers/new.py;drc=8fe602e47f11cfdd79225696f1f6a5556b57c58c;l=466
+ // TODO(b/321299939): create an interface for different runBenchmark types
+ // and refactor telemetryExp to use that interface
+ dim := make([]*spb.SwarmingRpcsStringPair, len(dimensions))
+ for i, kv := range dimensions {
+ dim[i] = &spb.SwarmingRpcsStringPair{
+ Key: kv["key"],
+ Value: kv["value"],
+ }
+ }
+ return dim
+}
+
+func generateProperties(command []string, casRef *spb.SwarmingRpcsCASReference, dim []*spb.SwarmingRpcsStringPair) *spb.SwarmingRpcsTaskProperties {
+ return &spb.SwarmingRpcsTaskProperties{
+ CasInputRoot: casRef,
+ Command: command,
+ Dimensions: dim,
+ ExecutionTimeoutSecs: 2700,
+ IoTimeoutSecs: 2700,
+ RelativeCwd: "out/Release",
+ }
+}
+
+func generateTags(jobID string, hash string, sizeBytes int64) []string {
+ // TODO(b/318863812): update swarming task tags to more appropriate tags.
+ return []string{
+ fmt.Sprintf("pinpoint_job_id:%s", jobID),
+ fmt.Sprintf("build_cas:%s/%d", hash, sizeBytes),
+ }
+}
+
+func createSwarmingRequest(jobID string, command []string, casRef *spb.SwarmingRpcsCASReference, dimensions []map[string]string) *spb.SwarmingRpcsNewTaskRequest {
+ return &spb.SwarmingRpcsNewTaskRequest{
+ BotPingToleranceSecs: 1200,
+ ExpirationSecs: 86400,
+ // EvaluateOnly omitted
+ Name: "Pinpoint bisection run benchmark task",
+ // ParentTaskId omitted
+ // PoolTaskTemplate omitted
+ Priority: 100,
+ // define properties later
+ PubsubTopic: "projects/chromeperf/topics/pinpoint-swarming-updates",
+ // can populate later, see example swarming call log
+ PubsubUserdata: "UNUSED",
+ Realm: "chrome:pinpoint",
+ // RequestUuid: omitted
+ // Resultdb: omitted
+ ServiceAccount: "chrome-tester@chops-service-accounts.iam.gserviceaccount.com",
+ // define tags later
+ // TaskSlices optional if properties defined
+ User: "Pinpoint",
+ // ForceSendFields: omitted
+ // NullFields: omitted
+ Properties: generateProperties(command, casRef, convertDimensions(dimensions)),
+ Tags: generateTags(jobID, casRef.Digest.Hash, casRef.Digest.SizeBytes),
+ }
+}
diff --git a/pinpoint/go/run_benchmark/telemetry.go b/pinpoint/go/run_benchmark/telemetry.go
new file mode 100644
index 0000000..c59084e
--- /dev/null
+++ b/pinpoint/go/run_benchmark/telemetry.go
@@ -0,0 +1,116 @@
+package run_benchmark
+
+import (
+ "fmt"
+
+ "go.skia.org/infra/go/util"
+)
+
+var defaultExtraArgs = []string{
+ "-v",
+ "--upload-results",
+ "--output-format",
+ "histograms",
+ "--isolated-script-test-output",
+ "${ISOLATED_OUTDIR}/output.json",
+}
+
+// Please keep this executable-argument mapping synced with perf waterfall:
+// https://chromium.googlesource.com/chromium/src/+/main/tools/perf/core/bot_platforms.py
+var waterfallEnabledGtests = util.NewStringSet([]string{
+ "base_perftests",
+ "components_perftests",
+ "dawn_perf_tests",
+ "gpu_perftests",
+ "load_library_perf_tests",
+ "performance_browser_tests",
+ "sync_performance_tests",
+ "tracing_perftests",
+ "views_perftests",
+})
+
+type telemetryTest struct {
+ benchmark string
+ browser string
+ commit string
+ story string
+ storyTags string
+}
+
+// getCommand generates the command needed to execute Telemetry benchmark tests.
+// This function assumes that the request object has been validated, and that
+// required fields have been verified for.
+// TODO(b/318863812): support user submitted extra_args.
+// This support is needed for pairwise executions, not bisection.
+func (t *telemetryTest) GetCommand() []string {
+ cmd := []string{
+ "luci-auth",
+ "context",
+ "--",
+ "vpython3",
+ "../../testing/test_env.py",
+ "../../testing/scripts/run_performance_tests.py",
+ }
+
+ // Note(jeffyoon@): The logic below can likely be made more efficient, but
+ // for parity sake we retain the same order for arguments as Catapult's Pinpoint.
+ // Ref: https://source.chromium.org/chromium/chromium/src/+/main:third_party/catapult/dashboard/dashboard/pinpoint/models/quest/run_telemetry_test.py;drc=10f23074bdab2b425cb29a8224b63298fdac42b7;l=127
+ if _, ok := waterfallEnabledGtests[t.benchmark]; ok {
+ if t.benchmark == "performance_browser_tests" {
+ cmd = append(cmd, "browser_tests")
+ } else {
+ cmd = append(cmd, t.benchmark)
+ }
+ } else {
+ cmd = append(cmd, "../../tools/perf/run_benchmark")
+ }
+
+ if t.storyTags == "" {
+ cmd = append(cmd, "-d")
+ }
+
+ if _, ok := waterfallEnabledGtests[t.benchmark]; ok {
+ cmd = append(cmd, "--gtest-benchmark-name", t.benchmark, "--non-telemetry", "true")
+ switch t.benchmark {
+ case "base_perftests", "dawn_perf_tests", "sync_performance_tests":
+ cmd = append(cmd, "--test-launcher-jobs=1", "--test-launcher-retry-limit=0")
+ case "components_perftests", "views_perftests":
+ cmd = append(cmd, "--xvfb")
+ case "performance_browser_tests":
+ // Allow the full performance runs to take up to 60 seconds (rather
+ // than the default of 30 for normal CQ browser test runs).
+ cmd = append(cmd, "--full-performance-run", "--test-launcher-jobs=1",
+ "--test-launcher-retry-limit=0", "--ui-test-action-timeout=60000",
+ "--ui-test-action-max-timeout=60000", "--test-launcher-timeout=60000",
+ "--gtest_filter=*/TabCapturePerformanceTest.*:*/CastV2PerformanceTest.*")
+ default:
+ break
+ }
+ } else {
+ cmd = append(cmd, "--benchmarks", t.benchmark)
+ }
+
+ if t.story != "" {
+ // TODO(b/40635221): Note that usage of "--run-full-story-set" and "--story-filter"
+ // can be replaced with --story=<story> (no regex needed). See crrev/c/1869800.
+ cmd = append(cmd, "--story-filter", fmt.Sprintf("^%s$", t.story))
+ }
+
+ if t.storyTags != "" {
+ cmd = append(cmd, "--story-tag-filter", t.storyTags)
+ }
+
+ cmd = append(cmd, "--pageset-repeat", "1", "--browser", t.browser)
+ cmd = append(cmd, defaultExtraArgs...)
+
+ // For results2 to differentiate between runs, we need to add the
+ // Telemetry parameter `--results-label <change>` to the runs.
+ // TODO(jeffyoon@) deprecate this label once the UI is no longer dependant on this.
+ cmd = append(cmd, "--results-label", t.commit[:7])
+
+ // Note: Appending "--run-full-story-set" last per comment above to retain
+ // argument order. This is always appended in catapult regardless of whether
+ // a story is defined or not.
+ cmd = append(cmd, "--run-full-story-set")
+ return cmd
+}
diff --git a/pinpoint/go/run_benchmark/telemetry_exp.go b/pinpoint/go/run_benchmark/telemetry_exp.go
deleted file mode 100644
index f57ee7d..0000000
--- a/pinpoint/go/run_benchmark/telemetry_exp.go
+++ /dev/null
@@ -1,91 +0,0 @@
-package run_benchmark
-
-import (
- "fmt"
-
- "go.skia.org/infra/go/skerr"
- "go.skia.org/infra/go/util"
-)
-
-var defaultExtraArgs = []string{
- "-v",
- "--upload-results",
- "--output-format",
- "histograms",
- "--isolated-script-test-output",
- "${ISOLATED_OUTDIR}/output.json",
-}
-
-// TODO(b/318863812): implement benchmarks in _WATERFALL_ENABLED_GTEST_NAMES
-// crbug/1146949
-// Please keep this executable-argument mapping synced with perf waterfall:
-// https://chromium.googlesource.com/chromium/src/+/main/tools/perf/core/bot_platforms.py
-var waterfallEnabledGtests = util.NewStringSet([]string{
- "base_perftests",
- "components_perftests",
- "dawn_perf_tests",
- "gpu_perftests",
- "load_library_perf_tests",
- "performance_browser_tests",
- "sync_performance_tests",
- "tracing_perftests",
- "views_perftests",
-})
-
-// A telemetryExp contains the command for the test device
-// to run a telemetry benchmark on Chrome browser.
-// telemetryExp will support most devices.
-type telemetryExp struct{}
-
-// createCmd creates the command for the test device.
-func (t *telemetryExp) createCmd(req RunBenchmarkRequest) ([]string, error) {
- // TODO(b/318863812): implement benchmarks in _WATERFALL_ENABLED_GTEST_NAMES
- // Benchmarks in _WATERFALL_ENABLED_GTEST_NAMES are rarely used.
- // see: https://source.chromium.org/chromium/chromium/src/+/main:third_party/catapult/dashboard/dashboard/pinpoint/models/quest/run_telemetry_test.py;drc=10f23074bdab2b425cb29a8224b63298fdac42b7;l=96
- _, ok := waterfallEnabledGtests[req.Benchmark]
- if ok {
- return nil, skerr.Fmt("Benchmark %s is not yet implemented", req.Benchmark)
- }
- cmd := []string{
- "luci-auth",
- "context",
- "--",
- "vpython3",
- "../../testing/test_env.py",
- "../../testing/scripts/run_performance_tests.py",
- "../../tools/perf/run_benchmark",
- // TODO(b/318863812): Implement story_tags. Some of our entrenched users
- // are only familiar with running Pinpoint jobs with story tags. Although
- // some of this behavior is due to UI confusion, we should still support it
- // and ensure the UI makes it clear what the differences are.
- // see: https://source.chromium.org/chromium/chromium/src/+/main:third_party/catapult/dashboard/dashboard/pinpoint/models/quest/run_telemetry_test.py;drc=10f23074bdab2b425cb29a8224b63298fdac42b7;l=127
- "-d",
- }
- if req.Benchmark == "" {
- return nil, skerr.Fmt("Missing 'Benchmark' argument.")
- }
- cmd = append(cmd, "--benchmarks", req.Benchmark)
- if req.Story == "" {
- return nil, skerr.Fmt("Missing 'Story' argument.")
- }
- cmd = append(cmd, "--story-filter", fmt.Sprintf("^%s$", req.Story))
- cmd = append(cmd, "--pageset-repeat", "1")
- cmd = append(cmd, "--browser", req.Config.Browser)
- cmd = append(cmd, defaultExtraArgs...)
-
- // TODO(b/318863812): Add support for non-chromium commits
- // append sha for results2
- if req.Commit == "" {
- return nil, skerr.Fmt("Missing 'Commit' argument.")
- }
- cmd = append(cmd,
- "--results-label",
- fmt.Sprintf("chromium@%s", req.Commit[:7]))
-
- // unclear what happens if you write the flags in
- // a different order
- // see: https://source.chromium.org/chromium/chromium/src/+/main:third_party/catapult/dashboard/dashboard/pinpoint/models/quest/run_telemetry_test.py;drc=10f23074bdab2b425cb29a8224b63298fdac42b7;l=80
- cmd = append(cmd, "--run-full-story-set")
-
- return cmd, nil
-}
diff --git a/pinpoint/go/run_benchmark/telemetry_exp_test.go b/pinpoint/go/run_benchmark/telemetry_exp_test.go
deleted file mode 100644
index d582f55..0000000
--- a/pinpoint/go/run_benchmark/telemetry_exp_test.go
+++ /dev/null
@@ -1,70 +0,0 @@
-package run_benchmark
-
-import (
- "fmt"
- "testing"
-
- "github.com/stretchr/testify/assert"
-)
-
-func TestCreateCmd(t *testing.T) {
- for i, test := range []struct {
- name string
- req RunBenchmarkRequest
- expectedErr string
- }{
- {
- name: "create command works",
- req: RunBenchmarkRequest{
- Benchmark: "benchmark",
- Story: "story",
- Commit: "64893ca6294946163615dcf23b614afe0419bfa3",
- },
- expectedErr: "",
- },
- {
- name: "benchmark error",
- req: RunBenchmarkRequest{
- Story: "story",
- Commit: "64893ca6294946163615dcf23b614afe0419bfa3",
- },
- expectedErr: "Benchmark",
- },
- {
- name: "story error",
- req: RunBenchmarkRequest{
- Benchmark: "benchmark",
- Commit: "64893ca6294946163615dcf23b614afe0419bfa3",
- },
- expectedErr: "Story",
- },
- {
- name: "commit error",
- req: RunBenchmarkRequest{
- Benchmark: "benchmark",
- Story: "story",
- },
- expectedErr: "Commit",
- },
- {
- name: "base_perftests not yet implemented",
- req: RunBenchmarkRequest{
- Benchmark: "base_perftests",
- Story: "story",
- Commit: "64893ca6294946163615dcf23b614afe0419bfa3",
- },
- expectedErr: "base_perftests is not yet implemented",
- },
- } {
- t.Run(fmt.Sprintf("[%d] %s", i, test.name), func(t *testing.T) {
- e := telemetryExp{}
- cmd, err := e.createCmd(test.req)
- if test.expectedErr == "" {
- assert.Contains(t, cmd, test.req.Benchmark)
- assert.Contains(t, cmd, fmt.Sprintf("^%s$", test.req.Story))
- } else {
- assert.ErrorContains(t, err, test.expectedErr)
- }
- })
- }
-}
diff --git a/pinpoint/go/run_benchmark/telemetry_test.go b/pinpoint/go/run_benchmark/telemetry_test.go
new file mode 100644
index 0000000..f915fb1
--- /dev/null
+++ b/pinpoint/go/run_benchmark/telemetry_test.go
@@ -0,0 +1,71 @@
+package run_benchmark
+
+import (
+ "testing"
+
+ "github.com/stretchr/testify/assert"
+
+ ppb "go.skia.org/infra/pinpoint/proto/v1"
+)
+
+func TestGetCommand_WaterfallGTest_TestCommand(t *testing.T) {
+ commit := "01bfa421eee3c76bbbf32510343e074060051c9f"
+
+ req := &ppb.ScheduleBisectRequest{
+ Configuration: "android-go-perf",
+ Benchmark: "components_perftests",
+ }
+
+ b, err := NewBenchmarkTest(req, commit)
+ assert.NoError(t, err)
+
+ cmd := b.GetCommand()
+ // stories w/o story tags should have -d set
+ assert.Contains(t, cmd, "-d")
+ assert.NotContains(t, cmd, "--story-tag-filter")
+
+ // Should not contain run_benchmark because it's a waterfall
+ // enabled GTest.
+ assert.NotContains(t, cmd, "../../tools/perf/run_benchmark")
+}
+
+func TestGetCommand_PerfBrowserTestWithStory_StoryTestCommand(t *testing.T) {
+ commit := "01bfa421eee3c76bbbf32510343e074060051c9f"
+
+ req := &ppb.ScheduleBisectRequest{
+ Story: "story",
+ StoryTags: "all",
+ Configuration: "android-pixel2_webview-perf",
+ Benchmark: "performance_browser_tests",
+ }
+
+ b, err := NewBenchmarkTest(req, commit)
+ assert.NoError(t, err)
+
+ cmd := b.GetCommand()
+ // stories w/o story tags should have -d set
+ assert.Contains(t, cmd, "--story-filter")
+ assert.Contains(t, cmd, "--story-tag-filter")
+
+ // Special gtest filter for performance_browser_tests
+ assert.Contains(t, cmd, "--gtest_filter=*/TabCapturePerformanceTest.*:*/CastV2PerformanceTest.*")
+}
+
+func TestGetCommand_NonWaterfallEnabledGTest_TestCommand(t *testing.T) {
+ commit := "01bfa421eee3c76bbbf32510343e074060051c9f"
+
+ req := &ppb.ScheduleBisectRequest{
+ Configuration: "android-pixel2_webview-perf",
+ Benchmark: "random_test",
+ }
+
+ b, err := NewBenchmarkTest(req, commit)
+ assert.NoError(t, err)
+
+ cmd := b.GetCommand()
+
+ // Non waterfall enabled gtest should have run_benchmark
+ assert.Contains(t, cmd, "../../tools/perf/run_benchmark")
+ assert.Contains(t, cmd, "--benchmarks")
+ assert.Contains(t, cmd, "random_test")
+}
diff --git a/pinpoint/go/workflows/internal/BUILD.bazel b/pinpoint/go/workflows/internal/BUILD.bazel
index 1fd88ec..fb1270c 100644
--- a/pinpoint/go/workflows/internal/BUILD.bazel
+++ b/pinpoint/go/workflows/internal/BUILD.bazel
@@ -15,6 +15,7 @@
"//pinpoint/go/build_chrome",
"//pinpoint/go/run_benchmark",
"//pinpoint/go/workflows",
+ "//pinpoint/proto/v1:proto",
"@io_temporal_go_sdk//activity",
"@io_temporal_go_sdk//temporal",
"@io_temporal_go_sdk//workflow",
diff --git a/pinpoint/go/workflows/internal/run_benchmark.go b/pinpoint/go/workflows/internal/run_benchmark.go
index aa12df2..e7efe90 100644
--- a/pinpoint/go/workflows/internal/run_benchmark.go
+++ b/pinpoint/go/workflows/internal/run_benchmark.go
@@ -10,6 +10,7 @@
"go.skia.org/infra/pinpoint/go/backends"
"go.skia.org/infra/pinpoint/go/run_benchmark"
"go.skia.org/infra/pinpoint/go/workflows"
+ ppb "go.skia.org/infra/pinpoint/proto/v1"
"go.temporal.io/sdk/activity"
"go.temporal.io/sdk/temporal"
"go.temporal.io/sdk/workflow"
@@ -68,7 +69,6 @@
}
resp.CAS = cas
-
return &resp, nil
}
@@ -82,7 +82,23 @@
return "", skerr.Wrap(err)
}
- return run_benchmark.Run(ctx, sc, params.Request)
+ // TODO(jeffyoon@) - this is a workaround to generate the request object s.t. the refactor
+ // does not obstruct the current run_benchmark workflow. Once the request spec defined
+ // by the proto is in full use, this can be deprecated.
+ //
+ // This only fills in the minimum set required to run the benchmark.
+ req := &ppb.ScheduleBisectRequest{
+ // run_benchmark.RunBenchmarkRequest doesn't support story tags
+ Configuration: params.Request.Config.Bot,
+ Benchmark: params.Request.Benchmark,
+ Story: params.Request.Story,
+ }
+
+ taskIds, err := run_benchmark.Run(ctx, sc, req, params.Request.Commit, params.Request.JobID, params.Request.Build, 1)
+ if err != nil {
+ return "", err
+ }
+ return taskIds[0].TaskId, nil
}
// WaitTaskFinishedActivity polls the task until it finishes or errors. Returns the status
