dispatcher: fix test result parsing, build ranges for failure alerts, other fixes

dispatcher: fix test result parsing, build ranges for failure alerts, other fixes

This actually covers several thing:

- parse gatekeeper_trees.json
- add params to limit the scope of analysis by tree, builder etc
- fix a race condition on build cache warming
- fix a pointer aliasing problem in build cache warming
- adds some named consts for alert severity
- fixes test result json parsing for deep test trees (e.g. webkit's)
- bundles build failures by failure key and build range, instead of an alert for each individual build failure

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/547e6c05302c0687f18a9daed6176d1847e95087

[seanmccullough, 2015-05-06 20:22:18]

seanmccullough@chromium.org changed reviewers:
+ vadimsh@google.com

[seanmccullough, 2015-05-06 20:22:18]

It's starting to find a lot of the same alerts that show up on sheriff-o-matic
now.  Still a ways to go though.

[Vadim Sh., 2015-05-06 21:40:18]

vadimsh@chromium.org changed reviewers:
+ vadimsh@chromium.org

[Vadim Sh., 2015-05-06 21:40:19]

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
File go/src/infra/monitoring/analyzer/analyzer.go (right):

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/analyzer/analyzer.go:226: return
pathParts[len(pathParts)-2], nil
check that len(pathParts) is >= 2. Also it would be a good place to put a
comment about expected structure of the URL (I don't know why -2 exactly...)

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/analyzer/analyzer.go:266: ba := b
IIUC, b is already a copy of whatever is in messages.Builds so it should be safe
to just use &b below (not 100% sure, worth conducting an experiment...)

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/analyzer/analyzer.go:351: firstFailureInstance[alr.Key]
= alr.Extension.(messages.BuildFailure).Builders[0].FirstFailure
This type assertion looks rather magical... Perhaps it would be better to be
prepared to handle other types of extensions? e.g.

if ext, ok := alr.Extension.(message.BuildFailure); ok {
  ... = ext.Buiders[0].FirstFailure
}

Or add a comment to stepFailureAlerts noting that it MUST return alerts with
BuildFailure extension only.

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/analyzer/analyzer.go:369: lastBuild =
a.bCache[cacheKeyForBuild(mn, bn, lastBuildID)]
:= and remove var above

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
File go/src/infra/monitoring/messages/alerts.go (right):

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/messages/alerts.go:89: // Build number of first failure.
nit: FirstFailure is build number ...

same for LatestFailure

[seanmccullough, 2015-05-06 22:16:20]

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
File go/src/infra/monitoring/analyzer/analyzer.go (right):

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/analyzer/analyzer.go:226: return
pathParts[len(pathParts)-2], nil
On 2015/05/06 21:40:18, Vadim Sh. wrote:
> check that len(pathParts) is >= 2. Also it would be a good place to put a
> comment about expected structure of the URL (I don't know why -2 exactly...)

To lop the /json off the http://.../{master.name}/json suffix.
Added check and comment.

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/analyzer/analyzer.go:266: ba := b
On 2015/05/06 21:40:18, Vadim Sh. wrote:
> IIUC, b is already a copy of whatever is in messages.Builds so it should be
safe
> to just use &b below (not 100% sure, worth conducting an experiment...)
Taking the address of a range var always gives you the same address.
I found this out the hard way :) https://play.golang.org/p/s7QAhTHx_2

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/analyzer/analyzer.go:351: firstFailureInstance[alr.Key]
= alr.Extension.(messages.BuildFailure).Builders[0].FirstFailure
On 2015/05/06 21:40:18, Vadim Sh. wrote:
> This type assertion looks rather magical... Perhaps it would be better to be
> prepared to handle other types of extensions? e.g.
> 
> if ext, ok := alr.Extension.(message.BuildFailure); ok {
>   ... = ext.Buiders[0].FirstFailure
> }
> 
> Or add a comment to stepFailureAlerts noting that it MUST return alerts with
> BuildFailure extension only.

Added a check and log an error if it fails. Not sure yet if this should handle
non-BuildFailure extensions.

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/analyzer/analyzer.go:369: lastBuild =
a.bCache[cacheKeyForBuild(mn, bn, lastBuildID)]
On 2015/05/06 21:40:18, Vadim Sh. wrote:
> := and remove var above

Done.

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
File go/src/infra/monitoring/messages/alerts.go (right):

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/messages/alerts.go:89: // Build number of first failure.
On 2015/05/06 21:40:19, Vadim Sh. wrote:
> nit: FirstFailure is build number ...
> 
> same for LatestFailure

Done.

[Vadim Sh., 2015-05-06 22:30:23]

lgtm with typo fix

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
File go/src/infra/monitoring/analyzer/analyzer.go (right):

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/analyzer/analyzer.go:266: ba := b
On 2015/05/06 22:16:20, seanmccullough wrote:
> On 2015/05/06 21:40:18, Vadim Sh. wrote:
> > IIUC, b is already a copy of whatever is in messages.Builds so it should be
> safe
> > to just use &b below (not 100% sure, worth conducting an experiment...)
> Taking the address of a range var always gives you the same address.
> I found this out the hard way :) https://play.golang.org/p/s7QAhTHx_2

:) Good to know. Go pointers + garbage collection still look somewhat magical to
me... E.g. in C++ I'd expect "ba" to be stack allocated and &ba have same
address in each loop iteration, sort of like "b" behaves.

https://codereview.chromium.org/1125263004/diff/40001/go/src/infra/monitoring...
File go/src/infra/monitoring/messages/alerts.go (right):

https://codereview.chromium.org/1125263004/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/messages/alerts.go:89: // FirstFailure is th build
number of first failure.
typo: th

[seanmccullough, 2015-05-06 22:43:13]

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
File go/src/infra/monitoring/analyzer/analyzer.go (right):

https://codereview.chromium.org/1125263004/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/analyzer/analyzer.go:266: ba := b
On 2015/05/06 22:30:23, Vadim Sh. wrote:
> On 2015/05/06 22:16:20, seanmccullough wrote:
> > On 2015/05/06 21:40:18, Vadim Sh. wrote:
> > > IIUC, b is already a copy of whatever is in messages.Builds so it should
be
> > safe
> > > to just use &b below (not 100% sure, worth conducting an experiment...)
> > Taking the address of a range var always gives you the same address.
> > I found this out the hard way :) https://play.golang.org/p/s7QAhTHx_2
> 
> :) Good to know. Go pointers + garbage collection still look somewhat magical
to
> me... E.g. in C++ I'd expect "ba" to be stack allocated and &ba have same
> address in each loop iteration, sort of like "b" behaves.

Thankfully there aren't quite as many wtf moments with go pointers, but they do
happen.

https://codereview.chromium.org/1125263004/diff/40001/go/src/infra/monitoring...
File go/src/infra/monitoring/messages/alerts.go (right):

https://codereview.chromium.org/1125263004/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/messages/alerts.go:89: // FirstFailure is th build
number of first failure.
On 2015/05/06 22:30:23, Vadim Sh. wrote:
> typo: th

Done.

[seanmccullough, 2015-05-06 22:43:21]

The CQ bit was checked by seanmccullough@chromium.org

[seanmccullough, 2015-05-06 22:43:21]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/1125263004/#ps60001
(title: "fixed typo in comments")

[commit-bot: I haz the power, 2015-05-06 22:43:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1125263004/60001

[commit-bot: I haz the power, 2015-05-06 22:46:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/547e6c05302c0687f18a9daed6176...