code review 35480043: dashboard/app: benchmarking support (data model + build...

dashboard/app: benchmarking support (data model + builder handlers)
Adds new fields PerfBenchmarks and Benchmark to Commit.
Adds new entities -- CommitRun, PerfResult, PerfMetricRun, PerfTodo and PerfConfig.
Adds new "benchmark-go-commit" todo.
Adds perf-result handler.
The design doc:
https://docs.google.com/a/google.com/document/d/10KInCeTYmpk7UZZLDGNktsisjctQnfm7FfOtr8RbFks/pub

[dvyukov, 2013-12-01 13:29:29]

Hello adg@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/dvyukov-go-perf-dashboard

[dvyukov, 2013-12-03 07:25:34]

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handle...
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handle...
dashboard/app/build/handler.go:117: cr, err := GetCommitRun(c, com.Num)
This does not work as of now -- it fails with "transaction touches several
entity groups", because can go to a subrepo, but CommitRuns are currently always
descendants of main Package.
We have 2 choices:
1. Do not update CommitRun's for commits that does not need benchmarking (in
particular go to subrepos)
2. Make CommitRun descendants on a proper Package

I think we need to do both.
There is no particular value in CommitRun for non-benchmark commits. And we can
always recreate them solely on server (no need for builder interaction, all info
is already on the server).

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handle...
dashboard/app/build/handler.go:337: t := datastore.NewQuery("Commit").
this needs to scan CommitRun's

[dvyukov, 2013-12-03 16:32:07]

PTAL

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handle...
File dashboard/app/build/handler.go (right):

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handle...
dashboard/app/build/handler.go:117: cr, err := GetCommitRun(c, com.Num)
On 2013/12/03 07:25:35, dvyukov wrote:
> This does not work as of now -- it fails with "transaction touches several
> entity groups", because can go to a subrepo, but CommitRuns are currently
always
> descendants of main Package.
> We have 2 choices:
> 1. Do not update CommitRun's for commits that does not need benchmarking (in
> particular go to subrepos)
> 2. Make CommitRun descendants on a proper Package
> 
> I think we need to do both.
> There is no particular value in CommitRun for non-benchmark commits. And we
can
> always recreate them solely on server (no need for builder interaction, all
info
> is already on the server).

Done

https://codereview.appspot.com/35480043/diff/90001/dashboard/app/build/handle...
dashboard/app/build/handler.go:337: t := datastore.NewQuery("Commit").
On 2013/12/03 07:25:35, dvyukov wrote:
> this needs to scan CommitRun's

Done.

[adg, 2013-12-04 03:10:40]

Looks pretty good

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go
File dashboard/app/build/build.go (right):

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:100: Benchmark bool // Needs benchmarking?
BenchmarkPending?

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:110: PerfBenchmarks []string `datastore:",noindex"`
s/PerfBenchmarks/PerfResults/

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:234: func GetCommitRun(c appengine.Context,
CommitNum int) (*CommitRun, error) {
commitNum

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:272: func GetCommits(c appengine.Context,
StartCommitNum, N int) ([]*Commit, error) {
startCommitNum, n int

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:355: // Descendant of Package.
Shouldn't this be a descendant of Commit?

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:393: func GetPerfMetricRun(c appengine.Context,
Builder, Benchmark, Metric string, CommitNum int) (*PerfMetricRun, error) {
lowercase args please, here and elsewhere

[adg, 2013-12-04 03:11:45]

Can you add some tests to test.go?

[dvyukov, 2013-12-04 08:30:49]

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build.go
File dashboard/app/build/build.go (right):

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:100: Benchmark bool // Needs benchmarking?
On 2013/12/04 03:10:40, adg wrote:
> BenchmarkPending?

It was irritating me as well.
It's not a temporal property of a commit, so I think BenchmarkPending is a bad
name. E.g. when a new benchmark is added, all commits need benchmarking again.
PerfTodo tracks pending benchmarks.
I've renamed it to NeedsBenchmarking.

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:110: PerfBenchmarks []string `datastore:",noindex"`
On 2013/12/04 03:10:40, adg wrote:
> s/PerfBenchmarks/PerfResults/

Done.

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:234: func GetCommitRun(c appengine.Context,
CommitNum int) (*CommitRun, error) {
On 2013/12/04 03:10:40, adg wrote:
> commitNum

Done.

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:272: func GetCommits(c appengine.Context,
StartCommitNum, N int) ([]*Commit, error) {
On 2013/12/04 03:10:40, adg wrote:
> startCommitNum, n int

Done.

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:355: // Descendant of Package.
On 2013/12/04 03:10:40, adg wrote:
> Shouldn't this be a descendant of Commit?

Everything else here is descendant of Package, including Result which is the
same as PerfResult in this regard. I've done the same.
Maybe it does not matter lot on our scale.

https://codereview.appspot.com/35480043/diff/130001/dashboard/app/build/build...
dashboard/app/build/build.go:393: func GetPerfMetricRun(c appengine.Context,
Builder, Benchmark, Metric string, CommitNum int) (*PerfMetricRun, error) {
On 2013/12/04 03:10:40, adg wrote:
> lowercase args please, here and elsewhere 

Done.

[dvyukov, 2013-12-04 08:32:24]

PTAL

[dvyukov, 2013-12-04 11:21:59]

On 2013/12/04 03:11:45, adg wrote:
> Can you add some tests to test.go?

Done

[adg, 2013-12-04 21:51:38]

LGTM

https://codereview.appspot.com/35480043/diff/180001/dashboard/app/build/test.go
File dashboard/app/build/test.go (right):

https://codereview.appspot.com/35480043/diff/180001/dashboard/app/build/test....
dashboard/app/build/test.go:229: defer func() {
what does this accomplish?

[dvyukov, 2013-12-05 02:09:45]

https://codereview.appspot.com/35480043/diff/180001/dashboard/app/build/test.go
File dashboard/app/build/test.go (right):

https://codereview.appspot.com/35480043/diff/180001/dashboard/app/build/test....
dashboard/app/build/test.go:229: defer func() {
On 2013/12/04 21:51:38, adg wrote:
> what does this accomplish?

w/o this it crashes in net/http guts when the handler finishes
actually I am not sure how kosher this is
but at least it works right now, as opposed to current code

[adg, 2013-12-05 02:21:10]

On 5 December 2013 13:09, <dvyukov@google.com> wrote:

> w/o this it crashes in net/http guts when the handler finishes
> actually I am not sure how kosher this is
> but at least it works right now, as opposed to current code
>

That's weird. Oh well. We can fix it and use appengine/aetest to make all
this a bit easier.

Andrew

[dvyukov, 2013-12-05 02:21:20]

*** Submitted as c137fbe89798 ***

dashboard/app: benchmarking support (data model + builder handlers)
Adds new fields PerfBenchmarks and Benchmark to Commit.
Adds new entities -- CommitRun, PerfResult, PerfMetricRun, PerfTodo and
PerfConfig.
Adds new "benchmark-go-commit" todo.
Adds perf-result handler.
The design doc:
https://docs.google.com/a/google.com/document/d/10KInCeTYmpk7UZZLDGNktsisjctQ...

R=adg
CC=golang-dev
https://codereview.appspot.com/35480043