Handle multiple modifications to distribution metrics correctly.

Handle multiple modifications to distribution metrics correctly.

Give up trying to combine modifications into one - it's difficult with
distribution metrics where you can't just sum the deltas.  Instead keep a list
of modifications and apply them all at once, in order.

This also does away with the confusing wrapper wrapper wrapper functions.

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/f208d8061b8aec3db4ef0084bab03f780133042a

[dsansome, 2015-12-16 03:41:57]

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

[dsansome, 2015-12-16 03:41:57]

I might get this right eventually...

[Sergey Berezin, 2015-12-17 22:28:51]

Overall good, but I have a couple of comments.

https://codereview.chromium.org/1531573003/diff/1/appengine_module/gae_ts_mon...
File appengine_module/gae_ts_mon/memcache_metric_store.py (right):

https://codereview.chromium.org/1531573003/diff/1/appengine_module/gae_ts_mon...
appengine_module/gae_ts_mon/memcache_metric_store.py:189: assert
len(modifications) > 0
While useful for debugging, I'm generally afraid of asserts in code run in live
services. I'd rather just return entry unchanged if there is nothing to do.

If it's an actual invariant you want to enforce, I'd also log an error and write
tests to check it's not called with empty modifications.

https://codereview.chromium.org/1531573003/diff/1/appengine_module/gae_ts_mon...
appengine_module/gae_ts_mon/memcache_metric_store.py:202: values =
targets[target_key]
nit: values = targets.setdefault(target_key, {})

https://codereview.chromium.org/1531573003/diff/1/appengine_module/gae_ts_mon...
appengine_module/gae_ts_mon/memcache_metric_store.py:214: if delta < 0:
Technically, you can add a negative number to a distribution (it'll go to a
single "-Inf" bucket, but it's not an error). Perhaps, this check is better done
in modify_fn itself.

[dsansome, 2015-12-18 00:54:33]

https://codereview.chromium.org/1531573003/diff/1/appengine_module/gae_ts_mon...
File appengine_module/gae_ts_mon/memcache_metric_store.py (right):

https://codereview.chromium.org/1531573003/diff/1/appengine_module/gae_ts_mon...
appengine_module/gae_ts_mon/memcache_metric_store.py:189: assert
len(modifications) > 0
On 2015/12/17 22:28:50, Sergey Berezin wrote:
> While useful for debugging, I'm generally afraid of asserts in code run in
live
> services. I'd rather just return entry unchanged if there is nothing to do.
> 
> If it's an actual invariant you want to enforce, I'd also log an error and
write
> tests to check it's not called with empty modifications.

Done.

https://codereview.chromium.org/1531573003/diff/1/appengine_module/gae_ts_mon...
appengine_module/gae_ts_mon/memcache_metric_store.py:202: values =
targets[target_key]
On 2015/12/17 22:28:50, Sergey Berezin wrote:
> nit: values = targets.setdefault(target_key, {})

Done.

https://codereview.chromium.org/1531573003/diff/1/appengine_module/gae_ts_mon...
appengine_module/gae_ts_mon/memcache_metric_store.py:214: if delta < 0:
On 2015/12/17 22:28:50, Sergey Berezin wrote:
> Technically, you can add a negative number to a distribution (it'll go to a
> single "-Inf" bucket, but it's not an error). Perhaps, this check is better
done
> in modify_fn itself.

Done.

[Sergey Berezin, 2015-12-18 01:35:53]

LGTM, thanks!

[dsansome, 2015-12-18 01:54:33]

The CQ bit was checked by dsansome@chromium.org

[commit-bot: I haz the power, 2015-12-18 01:54:45]

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

[commit-bot: I haz the power, 2015-12-18 01:57:16]

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

[commit-bot: I haz the power, 2015-12-18 01:57:17]

Try jobs failed on following builders:
  Infra Linux Trusty 64 Tester on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...)

[dsansome, 2015-12-18 02:03:11]

The CQ bit was checked by dsansome@chromium.org

[dsansome, 2015-12-18 02:03:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org
Link to the patchset: https://codereview.chromium.org/1531573003/#ps80001
(title: "Add a missing test for coverage")

[commit-bot: I haz the power, 2015-12-18 02:03:19]

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

[commit-bot: I haz the power, 2015-12-18 02:05:55]

Description was changed from

==========
Handle multiple modifications to distribution metrics correctly.

Give up trying to combine modifications into one - it's difficult with
distribution metrics where you can't just sum the deltas.  Instead keep a list
of modifications and apply them all at once, in order.

This also does away with the confusing wrapper wrapper wrapper functions.

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

to

==========
Handle multiple modifications to distribution metrics correctly.

Give up trying to combine modifications into one - it's difficult with
distribution metrics where you can't just sum the deltas.  Instead keep a list
of modifications and apply them all at once, in order.

This also does away with the confusing wrapper wrapper wrapper functions.

BUG=

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

[commit-bot: I haz the power, 2015-12-18 02:05:55]

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