Pinpoint: Kick off a job from Gerrit + post results to Gerrit

catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..

https://chromium.googlesource.com/catapult

BSD 3-Clause "New" or "Revised" License

1.93k stars 564 forks source link

Pinpoint: Kick off a job from Gerrit + post results to Gerrit #4356

Open mlippautz opened 6 years ago

mlippautz commented 6 years ago

"User journey" issue for perf try runs.

I repeatedly find myself in the situation where I want to get a rough performance overview of a couple of microbenchmarks on a few platforms (e.g. speedometer2 on win,mac,linux) before submitting to avoid revert/reland cycles when there are obvious perf regressions.

It would be really helpful if pinpoint would integrate with gerrit in a way where it a) Announces the posted job on the CL through a comment b) Announces that the job is done, maybe indicating whether a difference was found or not

This could be triggered by providing a job on a CL without bug id (?)

Right now I use the dashboard that is provided when logged in to keep track of everything which sort of does the minimal job. It gets rather tedious though if you want to share the results with others.

simonhatch commented 6 years ago

@dave-2 This might also be useful for @nedn 's use case, submitting benchmark changes to detect flakiness.

camillobruni commented 6 years ago

For investigation purposes I've been thinking about idea b) as well, already just for normal pinpoint jobs.

anniesullie commented 6 years ago

@dave-2 I took a look at the perf try job part of this today. It looks like:

In job.py, in the _Complete method, if self.auto_explore is not set, this is a perf tryjob and the CL should be updated.
To update the CL, we need to know the patch server and number, which are calculated in patch.AsDict(). There are a couple ways we could do this:
- Refer directly to self.state.AsDict()['state'][1]['change']['patch'] in job._Complete, which feels hacky since it encodes the specific index in state and assumes knowledge deep into state.AsDict().
- Refer directly to self.state._changes[1].patch.AsDict() which feels hacky for the same reason
- Try to abstract better, maybe by adding state.GetPatch() (not sure what the right name for a perf try job patch is?) and change.GetCodeReviewInfo() (again naming could be better, but something that returns the server and number?)

Any suggestions for getting the right server and change number for gerrit in a non-hacky way?

anniesullie commented 6 years ago

The bisect part of this is blocked on #4123.

dave-2 commented 6 years ago

I would recommend creating a new top-level field(s) in Job similar to bug_id that contains the Gerrit server and change id. Then you don't have to dig into the state. That would maintain the data flow (Job → JobState and not JobState → Job) and the abstractions (Job is responsible for integration with external services and JobState is responsible for actually running the Job)