[som] add tsmon to alerts-dispatcher

[som] add tsmon to alerts-dispatcher

BUG=632085
Committed: https://chromium.googlesource.com/infra/infra/+/a40ca80a430ad77142b49a5526af9ce3cdeaa916

[seanmccullough1, 2016-07-28 22:50:20]

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

[seanmccullough1, 2016-07-28 22:50:20]

LMK if there's a more appropriate reviewer.

[Vadim Sh., 2016-07-28 23:08:47]

Please run "gclient sync" to pull in most recent luci-go revision. 
metric.New* functions now require types.MetricMetadata as second argument to
specify units. In you case it would be just types.MetricMetadata{} (since all
metrics are counter-like).

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

https://codereview.chromium.org/2190163002/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:84: iterations =
metric.NewCounter("alerts-dispatcher/iterations",
nit: I think we use '_' as separators in /chrome/infra/... namespace, can't find
a metric that uses '-'.

https://codereview.chromium.org/2190163002/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:85: "Number if iterations of
the main polling loop per run.", field.String("status"))
'per run' is misleading, there'll be no (easy) way to figure out when different
runs happen

This metrics would allow to graph stuff like "number of iterations per minute"
(regardless of what run they happened in).

https://codereview.chromium.org/2190163002/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:88: alertCount =
metric.NewInt("alerts-dispatcher/alert-count",
I think this should be counter too. Will be easier to plot that way (as a rate
of alerts per time interval). Not sure... IIUC, iterations will keep
regenerating same alerts over and over again? Maybe NewInt is better in that
case...

https://codereview.chromium.org/2190163002/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:294: tsFlags.Flush = "auto"
"manual"? Since tsmon.Flush is used manually below.

(or remove tsmon.Flush there)

https://codereview.chromium.org/2190163002/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:431: tsmon.Flush(ctx)
how often would this be called? I think tsmon doesn't like very frequent
flushes.

[seanmccullough1, 2016-07-29 00:47:56]

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

https://codereview.chromium.org/2190163002/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:84: iterations =
metric.NewCounter("alerts-dispatcher/iterations",
On 2016/07/28 23:08:37, Vadim Sh. wrote:
> nit: I think we use '_' as separators in /chrome/infra/... namespace, can't
find
> a metric that uses '-'.

Done.

https://codereview.chromium.org/2190163002/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:85: "Number if iterations of
the main polling loop per run.", field.String("status"))
On 2016/07/28 23:08:37, Vadim Sh. wrote:
> 'per run' is misleading, there'll be no (easy) way to figure out when
different
> runs happen
> 
> This metrics would allow to graph stuff like "number of iterations per minute"
> (regardless of what run they happened in).

Done.

https://codereview.chromium.org/2190163002/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:88: alertCount =
metric.NewInt("alerts-dispatcher/alert-count",
On 2016/07/28 23:08:37, Vadim Sh. wrote:
> I think this should be counter too. Will be easier to plot that way (as a rate
> of alerts per time interval). Not sure... IIUC, iterations will keep
> regenerating same alerts over and over again? Maybe NewInt is better in that
> case...

Exactly the latter. The number of *new* alerts since the previous iteration is
not easily determined from the information we have here.

https://codereview.chromium.org/2190163002/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:294: tsFlags.Flush = "auto"
On 2016/07/28 23:08:37, Vadim Sh. wrote:
> "manual"? Since tsmon.Flush is used manually below.
> 
> (or remove tsmon.Flush there)

Done.

https://codereview.chromium.org/2190163002/diff/40001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:431: tsmon.Flush(ctx)
On 2016/07/28 23:08:37, Vadim Sh. wrote:
> how often would this be called? I think tsmon doesn't like very frequent
> flushes.

Done.

[Vadim Sh., 2016-07-29 01:03:06]

lgtm

[seanmccullough1, 2016-07-29 01:03:35]

The CQ bit was checked by seanmccullough@google.com

[commit-bot: I haz the power, 2016-07-29 01:03:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 01:17:23]

Description was changed from

==========
[som] add tsmon to alerts-dispatcher

BUG=632085
==========

to

==========
[som] add tsmon to alerts-dispatcher

BUG=632085

Committed:
https://chromium.googlesource.com/infra/infra/+/a40ca80a430ad77142b49a5526af9...
==========

[commit-bot: I haz the power, 2016-07-29 01:17:24]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/a40ca80a430ad77142b49a5526af9...

[dsansome, 2016-08-26 08:44:52]

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

[dsansome, 2016-08-26 08:44:53]

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

https://codereview.chromium.org/2190163002/diff/80001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:429: iterations.Add(ctx, 1,
"success")
Why not do this in looper instead, and use the same metric definitions as
infra/libs/service_utils/outer_loop.py?

[seanmccullough1, 2016-08-26 19:48:23]

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

https://codereview.chromium.org/2190163002/diff/80001/go/src/infra/monitoring...
go/src/infra/monitoring/dispatcher/dispatcher.go:429: iterations.Add(ctx, 1,
"success")
On 2016/08/26 08:44:52, dsansome wrote:
> Why not do this in looper instead, and use the same metric definitions as
> infra/libs/service_utils/outer_loop.py?

+1 for doing it in looper, Not sure about using the same names since those imply
where the metric is coming from (i.e. outer_loop is in the metric name)

[dsansome, 2016-08-29 00:02:38]

On 2016/08/26 19:48:23, seanmccullough1 wrote:
>
https://codereview.chromium.org/2190163002/diff/80001/go/src/infra/monitoring...
> File go/src/infra/monitoring/dispatcher/dispatcher.go (right):
> 
>
https://codereview.chromium.org/2190163002/diff/80001/go/src/infra/monitoring...
> go/src/infra/monitoring/dispatcher/dispatcher.go:429: iterations.Add(ctx, 1,
> "success")
> On 2016/08/26 08:44:52, dsansome wrote:
> > Why not do this in looper instead, and use the same metric definitions as
> > infra/libs/service_utils/outer_loop.py?
> 
> +1 for doing it in looper, Not sure about using the same names since those
imply
> where the metric is coming from (i.e. outer_loop is in the metric name)

Hmm yeah... it would let us use the same monitoring console templates though. 
*shrug*