Issue 623854: Support unit annotations in ts_mon metrics

Issue 623854: Support unit annotations in ts_mon metrics

For go-implementation, please find the following CLs: 
- https://codereview.chromium.org/2123853002
- https://codereview.chromium.org/2125943003

BUG=623854
Committed: https://chromium.googlesource.com/infra/infra/+/9bf7fbe68e4dc69611e78ee0b43c44c258c49d3b

[ddoman, 2016-06-29 06:53:52]

Description was changed from

==========
Issue 623854: Support unit annotations in ts_mon metrics

BUG=623854
==========

to

==========
Issue 623854: Support unit annotations in ts_mon metrics

BUG=623854
==========

[ddoman, 2016-06-29 06:53:52]

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

[ddoman, 2016-06-29 07:59:50]

ddoman@chromium.org changed reviewers:
+ chrishall@chromium.org, philwright@chromium.org

[agable, 2016-06-29 18:25:21]

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

[agable, 2016-06-29 18:25:23]

LGTM with some documentation nits. Also:
1) Make sure this gets into the Go implementation as well
(https://github.com/luci/luci-go/blob/master/common/tsmon/metric/metric.go)
2) I assume that sending units with metrics will cause the ACQ-SRE mon-api to
discard them, just like sending additional fields does. So you'll have to make
sure that this rolls out everywhere in unison to avoid dropping too many metric
points.

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/commo...
File infra_libs/ts_mon/common/metrics.py (right):

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/commo...
infra_libs/ts_mon/common/metrics.py:34: 
Please update this docstring to reference units as well.

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/commo...
infra_libs/ts_mon/common/metrics.py:474: """Metaclass to populate the enum
values of metrics_pb2.MetricsData.Units
nit: all our docstrings start with a single sentence that fits on a single line,
followed by a paragraph is more description is necessary.

[dsansome, 2016-06-30 06:25:45]

lgtm

Nice!

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/__ini...
File infra_libs/ts_mon/__init__.py (right):

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/__ini...
infra_libs/ts_mon/__init__.py:37: from infra_libs.ts_mon.common.metrics import
MetricsDataUnits
Sort alphabetically

[Sergey Berezin, 2016-06-30 19:21:25]

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

[Sergey Berezin, 2016-06-30 19:21:26]

drive by:

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/commo...
File infra_libs/ts_mon/common/test/metrics_test.py (right):

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/commo...
infra_libs/ts_mon/common/test/metrics_test.py:72: return str(p).splitlines()
It seems you're missing the new expectations file for this test. Please add it
to the CL.

[ddoman, 2016-07-06 03:35:37]

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/__ini...
File infra_libs/ts_mon/__init__.py (right):

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/__ini...
infra_libs/ts_mon/__init__.py:37: from infra_libs.ts_mon.common.metrics import
MetricsDataUnits
On 2016/06/30 06:25:44, dsansome wrote:
> Sort alphabetically

Done.

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/commo...
File infra_libs/ts_mon/common/metrics.py (right):

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/commo...
infra_libs/ts_mon/common/metrics.py:34: 
On 2016/06/29 18:25:23, agable wrote:
> Please update this docstring to reference units as well.

Done.

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/commo...
infra_libs/ts_mon/common/metrics.py:474: """Metaclass to populate the enum
values of metrics_pb2.MetricsData.Units
On 2016/06/29 18:25:23, agable wrote:
> nit: all our docstrings start with a single sentence that fits on a single
line,
> followed by a paragraph is more description is necessary.

Done.

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/commo...
File infra_libs/ts_mon/common/test/metrics_test.py (right):

https://codereview.chromium.org/2111473003/diff/40001/infra_libs/ts_mon/commo...
infra_libs/ts_mon/common/test/metrics_test.py:72: return str(p).splitlines()
On 2016/06/30 19:21:26, Sergey Berezin wrote:
> It seems you're missing the new expectations file for this test. Please add it
> to the CL.

Done.

[ddoman, 2016-07-06 05:45:16]

> 1) Make sure this gets into the Go implementation as well
> (https://github.com/luci/luci-go/blob/master/common/tsmon/metric/metric.go)

Just submitted the following CL. I'd appreciate if you can take a look at the
go-lang changes as well.
: https://codereview.chromium.org/2123853002

[ddoman, 2016-07-06 05:49:23]

Description was changed from

==========
Issue 623854: Support unit annotations in ts_mon metrics

BUG=623854
==========

to

==========
Issue 623854: Support unit annotations in ts_mon metrics

For go-implementation, please find the following CL:
https://codereview.chromium.org/2123853002

BUG=623854
==========

[Sergey Berezin, 2016-07-06 22:01:24]

LGTM for Python code - thanks, this is great!

Please split off the Go code in a separate CL - see comment.

https://codereview.chromium.org/2111473003/diff/80001/go/src/infra/monitoring...
File go/src/infra/monitoring/proxy/main.go (right):

https://codereview.chromium.org/2111473003/diff/80001/go/src/infra/monitoring...
go/src/infra/monitoring/proxy/main.go:45: types.MetricMetadata{},
I'd split the go changes into a separate CL. The reason is, luci-go is pinned in
infra.git, and you first need to land the Go CL, and only then modify the go
code in infra.git, together with the pin roll (in top-level DEPS file).

IIUC, the Python change is independent from the Go change, so they can land
separately.

[ddoman, 2016-07-07 05:06:13]

On 2016/07/06 22:01:24, Sergey Berezin wrote:
> LGTM for Python code - thanks, this is great!
> 
> Please split off the Go code in a separate CL - see comment.
> 
>
https://codereview.chromium.org/2111473003/diff/80001/go/src/infra/monitoring...
> File go/src/infra/monitoring/proxy/main.go (right):
> 
>
https://codereview.chromium.org/2111473003/diff/80001/go/src/infra/monitoring...
> go/src/infra/monitoring/proxy/main.go:45: types.MetricMetadata{},
> I'd split the go changes into a separate CL. The reason is, luci-go is pinned
in
> infra.git, and you first need to land the Go CL, and only then modify the go
> code in infra.git, together with the pin roll (in top-level DEPS file).
> 
> IIUC, the Python change is independent from the Go change, so they can land
> separately.

Good point. I split off the go changes into the following CL:
https://codereview.chromium.org/2125943003

[ddoman, 2016-07-07 05:06:39]

https://codereview.chromium.org/2111473003/diff/80001/go/src/infra/monitoring...
File go/src/infra/monitoring/proxy/main.go (right):

https://codereview.chromium.org/2111473003/diff/80001/go/src/infra/monitoring...
go/src/infra/monitoring/proxy/main.go:45: types.MetricMetadata{},
On 2016/07/06 22:01:24, Sergey Berezin wrote:
> I'd split the go changes into a separate CL. The reason is, luci-go is pinned
in
> infra.git, and you first need to land the Go CL, and only then modify the go
> code in infra.git, together with the pin roll (in top-level DEPS file).
> 
> IIUC, the Python change is independent from the Go change, so they can land
> separately.

Done.

[ddoman, 2016-07-07 05:07:26]

Description was changed from

==========
Issue 623854: Support unit annotations in ts_mon metrics

For go-implementation, please find the following CL:
https://codereview.chromium.org/2123853002

BUG=623854
==========

to

==========
Issue 623854: Support unit annotations in ts_mon metrics

For go-implementation, please find the following CLs: 
- https://codereview.chromium.org/2123853002
- https://codereview.chromium.org/2125943003

BUG=623854
==========

[Sergey Berezin, 2016-07-08 17:15:21]

LGTM, thanks!

[ddoman, 2016-07-11 00:35:59]

The CQ bit was checked by ddoman@chromium.org

[ddoman, 2016-07-11 00:35:59]

The patchset sent to the CQ was uploaded after l-g-t-m from agable@chromium.org,
dsansome@chromium.org
Link to the patchset: https://codereview.chromium.org/2111473003/#ps100001
(title: "Split off go changes to a separate CL:
https://codereview.chromium.org/2125943003")

[commit-bot: I haz the power, 2016-07-11 00:36:12]

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

[ddoman, 2016-07-11 01:25:18]

The CQ bit was unchecked by ddoman@chromium.org

[ddoman, 2016-07-11 01:27:06]

The CQ bit was checked by ddoman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-11 01:27:08]

Dry run: 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-11 01:29:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-11 01:29:51]

Dry run: This issue passed the CQ dry run.

[ddoman, 2016-07-11 01:54:09]

The CQ bit was unchecked by ddoman@chromium.org

[ddoman, 2016-07-11 01:54:20]

The CQ bit was checked by ddoman@chromium.org

[commit-bot: I haz the power, 2016-07-11 01:54:24]

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-11 01:57:22]

Description was changed from

==========
Issue 623854: Support unit annotations in ts_mon metrics

For go-implementation, please find the following CLs: 
- https://codereview.chromium.org/2123853002
- https://codereview.chromium.org/2125943003

BUG=623854
==========

to

==========
Issue 623854: Support unit annotations in ts_mon metrics

For go-implementation, please find the following CLs: 
- https://codereview.chromium.org/2123853002
- https://codereview.chromium.org/2125943003

BUG=623854

Committed:
https://chromium.googlesource.com/infra/infra/+/9bf7fbe68e4dc69611e78ee0b43c4...
==========

[commit-bot: I haz the power, 2016-07-11 01:57:24]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/infra/infra/+/9bf7fbe68e4dc69611e78ee0b43c4...