[alerts-dispatcher] update to build again :) also fixes a concurrent map access

[alerts-dispatcher] update to build again :) also fixes a concurrent map access
and fixes some gatekeeper config interpretation problems.

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/4f6952e3ecdc32af1b70d3938c7ab8552e0d4808

[seanmccullough1, 2016-04-08 23:50:27]

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

[seanmccullough1, 2016-04-08 23:50:28]

This fixes some alerts-dispatcher issues and makes it closer to working with the
new alerts.json format.

[martiniss, 2016-04-11 21:33:34]

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

https://codereview.chromium.org/1874053002/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:278: log.Errorf("Error reading
gatekeeper json: %v", err)
Maybe update this error to say "Error reading gatekeeper tree json" ?

https://codereview.chromium.org/1874053002/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:312: if err != nil {
I think you need to handle the error before you close the result...?

https://codereview.chromium.org/1874053002/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:330: a.Gatekeeper =
analyzer.NewGatekeeperRules(*gk)
I can't seem to find this function... where is it?

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

https://codereview.chromium.org/1874053002/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/messages/gatekeeper.go:17: Categories       []string    
            `json:"categories"`
Move this back up, so no diff?

[seanmccullough, 2016-04-11 23:11:08]

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

[seanmccullough, 2016-04-11 23:11:08]

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

https://codereview.chromium.org/1874053002/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:278: log.Errorf("Error reading
gatekeeper json: %v", err)
On 2016/04/11 21:33:34, martiniss wrote:
> Maybe update this error to say "Error reading gatekeeper tree json" ?

Done.

https://codereview.chromium.org/1874053002/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:312: if err != nil {
On 2016/04/11 21:33:34, martiniss wrote:
> I think you need to handle the error before you close the result...?

defer runs its argument right before the return of the function (not here). f
itself is evaluated on this line, which may sound risky since f can be nil if
Open fails. However, Close is defined for nil os.File pointers:
https://golang.org/src/os/file_unix.go?s=3525:3553#L118 (many standard library
functions handle nil receiver values like this without panicking. It makes the
calling code more robust and less likely to break in some common situations).

https://codereview.chromium.org/1874053002/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:330: a.Gatekeeper =
analyzer.NewGatekeeperRules(*gk)
On 2016/04/11 21:33:34, martiniss wrote:
> I can't seem to find this function... where is it?

Forgot to add a file. Added.

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

https://codereview.chromium.org/1874053002/diff/20001/go/src/infra/monitoring...
go/src/infra/monitoring/messages/gatekeeper.go:17: Categories       []string    
            `json:"categories"`
On 2016/04/11 21:33:34, martiniss wrote:
> Move this back up, so no diff?

Done.

[martiniss, 2016-04-12 22:02:55]

lgtm

Looks like a fairly trivial refactoring change to me. Couple comments but
nothing major.

https://codereview.chromium.org/1874053002/diff/100001/go/src/infra/monitorin...
File go/src/infra/monitoring/analyzer/gatekeeper.go (right):

https://codereview.chromium.org/1874053002/diff/100001/go/src/infra/monitorin...
go/src/infra/monitoring/analyzer/gatekeeper.go:74: mc := mcs[0]
it feels like we're doing something wrong if we have to do this... can we change
the datastructure so that there's only one master config per master?

https://codereview.chromium.org/1874053002/diff/100001/go/src/infra/monitorin...
go/src/infra/monitoring/analyzer/gatekeeper.go:82: // Not clear that
builder_alerts even looks at the rest of these condtions
typo

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
File go/src/infra/monitoring/analyzer/analyzer.go (right):

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
go/src/infra/monitoring/analyzer/analyzer.go:113: rslck             *sync.Mutex
could you put a comment saying what resources this guards access to? just the
revision summaries?

Oh i get it; revision summaries lock. I thought it was a "ReSource" lock.

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
go/src/infra/monitoring/analyzer/analyzer.go:141: rslck:            
&sync.Mutex{},
 could you put a comment saying what resources this guards access to?

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
File go/src/infra/monitoring/analyzer/gatekeeper.go (right):

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
go/src/infra/monitoring/analyzer/gatekeeper.go:1: package analyzer
I think this should eventually be split out into its own package, which we can
use to re-implement gatekeeper in go for luci. But for now this is fine.

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
File go/src/infra/monitoring/dispatcher/dispatcher.go (left):

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
go/src/infra/monitoring/dispatcher/dispatcher.go:59: // Weird. Are there ever
multiple configs per master key?
Ah you had the same idea as me..

[seanmccullough, 2016-04-12 22:13:42]

thanks!

https://codereview.chromium.org/1874053002/diff/100001/go/src/infra/monitorin...
File go/src/infra/monitoring/analyzer/gatekeeper.go (right):

https://codereview.chromium.org/1874053002/diff/100001/go/src/infra/monitorin...
go/src/infra/monitoring/analyzer/gatekeeper.go:74: mc := mcs[0]
On 2016/04/12 22:02:54, martiniss wrote:
> it feels like we're doing something wrong if we have to do this... can we
change
> the datastructure so that there's only one master config per master?


I agree, but this reflects what's in the config files (which are owned by
gatekeeper). The master config section of the json file takes the value of an
array. I've only ever seen single values in it in practice though. I don't know
why that choice was made, or what would break in gatekeeper if we changed its
config to only accept single values here.

https://codereview.chromium.org/1874053002/diff/100001/go/src/infra/monitorin...
go/src/infra/monitoring/analyzer/gatekeeper.go:82: // Not clear that
builder_alerts even looks at the rest of these condtions
On 2016/04/12 22:02:54, martiniss wrote:
> typo

Done.

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
File go/src/infra/monitoring/analyzer/analyzer.go (right):

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
go/src/infra/monitoring/analyzer/analyzer.go:141: rslck:            
&sync.Mutex{},
On 2016/04/12 22:02:54, martiniss wrote:
>  could you put a comment saying what resources this guards access to?

Done.

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
File go/src/infra/monitoring/analyzer/gatekeeper.go (right):

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
go/src/infra/monitoring/analyzer/gatekeeper.go:1: package analyzer
On 2016/04/12 22:02:54, martiniss wrote:
> I think this should eventually be split out into its own package, which we can
> use to re-implement gatekeeper in go for luci. But for now this is fine.

Acknowledged.

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
File go/src/infra/monitoring/dispatcher/dispatcher.go (left):

https://codereview.chromium.org/1874053002/diff/140001/go/src/infra/monitorin...
go/src/infra/monitoring/dispatcher/dispatcher.go:59: // Weird. Are there ever
multiple configs per master key?
On 2016/04/12 22:02:54, martiniss wrote:
> Ah you had the same idea as me..

yeah, I don't get warm fuzzy feelings after parsing those files.

[seanmccullough, 2016-04-12 22:40:30]

The CQ bit was checked by seanmccullough@chromium.org

[seanmccullough, 2016-04-12 22:40:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
martiniss@chromium.org
Link to the patchset: https://codereview.chromium.org/1874053002/#ps160001
(title: "typos")

[commit-bot: I haz the power, 2016-04-12 22:40:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1874053002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1874053002/160001

[commit-bot: I haz the power, 2016-04-12 22:44:37]

Description was changed from

==========
[alerts-dispatcher] update to build again :) also fixes a concurrent map access
and fixes some gatekeeper config interpretation problems.

BUG=
==========

to

==========
[alerts-dispatcher] update to build again :) also fixes a concurrent map access
and fixes some gatekeeper config interpretation problems.

BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/4f6952e3ecdc32af1b70d3938c7ab...
==========

[commit-bot: I haz the power, 2016-04-12 22:44:37]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/infra/infra/+/4f6952e3ecdc32af1b70d3938c7ab...