make version of ts_mon compatible with appengine

create a version of ts_mon compatible with appengine apps

update older third_party repositories (apiclient, oauth2client, six, etc)

send monitoring metrics from som

fix bug in alerts-history

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/65322c283b681fc75ea3c0be07ef0f37a6f4fea0

[jshu, 2015-08-06 23:32:02]

jshu@google.com changed reviewers:
+ agable@chromium.org

[jshu, 2015-08-06 23:32:02]

wish there was a way for codereview to know when a file is copied over, but this
should work minus the credentials. 

the files that actually have significant changes are config.py, some of the init
files, and most of the files in appengine/sheriff-o-matic. also, I disabled
pylint on the entire init file in gae/ because it wanted me to do imports
starting with infra_libs.ts_mon.gae which would be the wrong path for the
symlinked version. not sure if pylint-disable is the correct way to handle that.

[jshu, 2015-08-10 20:08:27]

This is sends metrics to monarch that are visible on pcon. Ready for review.
However I do need to test whether chromium_build_logs still work after all the
updates. Plus we might want to move gae_ts_mon outside of sheriff-o-matic and
symlink it in since other apps may need it in the future.

[agable, 2015-08-10 22:23:58]

A few top-level organizational comments which I think will make the rest of the
CL easier to review and land. While you're working on these, I'll dive into the
changes to SoM and the new files in gae_ts_mon.

https://codereview.chromium.org/1260293009/diff/220001/appengine/chromium_bui...
File appengine/chromium_build_logs/third_party/apiclient/__init__.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine/chromium_bui...
appengine/chromium_build_logs/third_party/apiclient/__init__.py:1: """Retain
apiclient as an alias for googleapiclient."""
Since the symlink above is to appengine/third_party/googleapiclient, why is
chromium_build_logs/third_party/apiclient also being updated in this CL?

https://codereview.chromium.org/1260293009/diff/220001/appengine/chromium_bui...
File appengine/chromium_build_logs/third_party/six.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine/chromium_bui...
appengine/chromium_build_logs/third_party/six.py:1: """Utilities for writing
code that runs on Python 2 and 3"""
Similarly here: the symlink above is to appengine/third_party/six, not to
appegine/chromium_build_logs/third_party/six, so why is this file being added?

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/confi...
File appengine_module/config.proto (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/confi...
appengine_module/config.proto:1: syntax = "proto3";
This should be inside gae_ts_mon, not in the top level of appengine_module.

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/__init__.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/__init__.py:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
Make sure that there is also an appengine_module/gae_ts_mon/test directory
containing tests for all of these files.

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/gae (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/gae:1: ../../infra_libs/ts_mon/gae/
(See comment below) This should be a simlink to /common/ instead of /gae/

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/third_party/apiclient (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/third_party/apiclient:1:
../../../appengine/third_party/apiclient
Given that you have 8 symlinks all into appengine/third_party/*, I'd replace
that with a single symlink directly to appengine/third_party.

https://codereview.chromium.org/1260293009/diff/220001/infra_libs/ts_mon/gae/...
File infra_libs/ts_mon/gae/__init__.py (right):

https://codereview.chromium.org/1260293009/diff/220001/infra_libs/ts_mon/gae/...
infra_libs/ts_mon/gae/__init__.py:1: # Copyright 2015 The Chromium Authors. All
rights reserved.
Name this internal directory "common", rather than "gae", as it is used by both
ts_mon and gae_ts_mon.

https://codereview.chromium.org/1260293009/diff/220001/infra_libs/ts_mon/test...
File infra_libs/ts_mon/test/errors_test.py (right):

https://codereview.chromium.org/1260293009/diff/220001/infra_libs/ts_mon/test...
infra_libs/ts_mon/test/errors_test.py:4: 
All of the *_test files which test things inside ts_mon/common should move to
ts_mon/common/test/*_test.py

[agable, 2015-08-10 23:04:39]

Here's some more in-depth comments, interspersed with some more layout stuff and
some random nits.

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/alerts.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/alerts.py:42: def increment_monarch(endpoint):
Might be nice to implement this somewhere more general than "alerts.py", since
it is used by other handlers that don't otherwise import alerts.py.

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/alerts.py:45: logging.info(url)
Log a more useful message here :) or nothing at all

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/alerts.py:46: return urlfetch.fetch(url=url,
method=urlfetch.GET)
Using GET here makes me a very sad panda. You never had any luck getting the
POST request to go through? What about a PUT request? Maybe we can sit down and
work together to debug what's going wrong there.

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/app.yaml (left):

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/app.yaml:37: - url: /ts-internal-alerts
Why is this change in this CL?

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/cron.yaml (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/cron.yaml:1: 
nit: no empty line at top of file.

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/infra_libs/ts_mon (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/infra_libs/ts_mon:1: ../../../infra_libs/ts_mon/
The whole thing should work with just the symlink to gae_ts_mon. It should
contain (either directly or via symlinks) everything else. This symlink
shouldn't be necessary, and if it is currently necessary, something should
change so it isn't :)

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/monitoring.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/monitoring.py:24: access_count =
gae_ts_mon.CounterMetric('gae/access/count')
Rather than having metrics for these endpoints ("access" and "alerts"), we
should have a single "appengine url was accessed" metric. It should have fields
for
* Appengine app name (Target field)
* Appengine app version (Target field, hopefully)
* Appengine app instance (Target field)
* Url path (Metric field)
* Request path (Metric field)
* Request type GET/POST/etc (Metric field)

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/monitoring.py:41: ACCESS_COUNT = {'ts-alerts': 0,
Rather than keeping a dictionary here, you should be able to directly set values
on the GaugeMetrics created on line 25 and let interface.State keep track of
this mapping for you.

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/monitoring.py:48: ''' Called my cron jobs every 5
minutes to update alerts_count. '''
nit: """Called by cron... alerts_count."""

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/monitoring.py:67: logging.info('in post request')
nit: remove some of the extraneous logging before landing

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/config.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:32: endpoint: url (including file://,
pubsub://project/topic) to post monitoring
nit: remove reference to file:// urls; they don't work on appengine

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:33: metrics to. If set, overrides the
value in --ts-mon-config-file
nit: remove reference to command line flag

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:35: 'manual (only send when flush() is
called), or auto (send
nit: stray leading single quote

Also, does auto actually work on appengine? I don't think it does. That part of
this comment should be removed.

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:36: 'automatically every
--ts-mon-flush-interval-secs seconds)
nit: another stray leading quote

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:38: --ts-mon-flush=auto
nit: remove reference to command line flag

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:61: print >> sys.stderr, ('Argument
--ts-mon-task-service-name must be '
Use logging.error instead of print, and don't reference command line flags

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:63: sys.exit(2)
Eep don't sys.exit in appengine :)

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:65: print >> sys.stderr, ('Argument
--ts-mon-task-job-name must be provided '
Same re: logging here

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:73: if flush == 'auto':
Remove 'auto', since I don't think it works on appengine at all.

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/interface.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
You should be able to greatly simplify this file, and move the logic that is
currently in config.py into it. Some simplifications:
* Fix the docstring at the top to reflect how to use gae_ts_mon, rather than how
to use ts_mon (what symlinks to create, how to configure it, etc.)
* Remove all references to the flush thread
* The only flush modes should be "all" or "manual"

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/metrics.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/metrics.py:1: # Copyright 2015 The Chromium Authors.
All rights reserved.
I realize the imports are different, but: See if there's a way you can put this
file into ts_mon/common and symlink it. Since none of the Metric classes
themselves need to be modified for appengine, it's a shame to have to copy-paste
all the code here.

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/monitors.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/monitors.py:86: creds =
GoogleCredentials.get_application_default()
And the pubsub pipeline is accepting these credentials? They don't need to be
whitelisted? Sweet!

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/proto (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/proto:1:
../../ENV/lib/python2.7/site-packages/monacq/proto/
This symlink is risky. It works for you? Like, it actually gets uploaded to
appengine and works from there? That's crazy and cool and kinda terrifying
though I guess it makes sense.

We should find a better way to do this (symlinking to the repo, rather than to
something inside the virtualenv ENV/) but this may be fine for now.

https://codereview.chromium.org/1260293009/diff/220001/infra_libs/ts_mon/conf...
File infra_libs/ts_mon/config.proto (left):

https://codereview.chromium.org/1260293009/diff/220001/infra_libs/ts_mon/conf...
infra_libs/ts_mon/config.proto:1: syntax = "proto3";
This file should still be here. This is demonstrating the format that a ts_mon
configuration proto should take, so it should live here to document that format.

https://codereview.chromium.org/1260293009/diff/220001/infra_libs/ts_mon/gae/...
File infra_libs/ts_mon/gae/__init__.py (right):

https://codereview.chromium.org/1260293009/diff/220001/infra_libs/ts_mon/gae/...
infra_libs/ts_mon/gae/__init__.py:6: 
Actually, though, the contents of this file shouldn't be necessary at all. This
should be an empty __init__.py.

The point of ts_mon/__init__.py having all of these statements is so that it
exports an API. The classes/functions appearing in this file are the ones that
consumers can use. ts_mon/common/ doesn't need to export an API. No one should
be using it -- they should only use ts_mon/ or gae_ts_mon/, so *those* are what
should provide an api.

So the top-level ts_mon/__init__.py should continue to export all of these
things, but do it as
from common.distribution import Distribution
etc.

[jshu, 2015-08-11 21:57:18]

https://codereview.chromium.org/1260293009/diff/220001/appengine/chromium_bui...
File appengine/chromium_build_logs/third_party/apiclient/__init__.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine/chromium_bui...
appengine/chromium_build_logs/third_party/apiclient/__init__.py:1: """Retain
apiclient as an alias for googleapiclient."""
On 2015/08/10 at 22:23:58, agable wrote:
> Since the symlink above is to appengine/third_party/googleapiclient, why is
chromium_build_logs/third_party/apiclient also being updated in this CL?

appengine/third_party/googleapiclient symlinks to
chromium_build_logs/third_party/googleapiclient (maybe it should actually be the
other way around) so both are updated, and both need the alias apiclient code.
just wanted to update chromium_build_logs while I'm updating third party
repositories, it probably won't break anything. we can also delete this file
everywhere and change all instances of 'apiclient' to 'googleapiclient'

https://codereview.chromium.org/1260293009/diff/220001/appengine/chromium_bui...
File appengine/chromium_build_logs/third_party/six.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine/chromium_bui...
appengine/chromium_build_logs/third_party/six.py:1: """Utilities for writing
code that runs on Python 2 and 3"""
On 2015/08/10 at 22:23:58, agable wrote:
> Similarly here: the symlink above is to appengine/third_party/six, not to
appegine/chromium_build_logs/third_party/six, so why is this file being added?

same as other place, appengine/third_party symlinks from chromium_build_logs.
everything is being updated. but perhaps we want chromium_build_logs to symlink
from appengine/third_party instead

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/alerts.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/alerts.py:46: return urlfetch.fetch(url=url,
method=urlfetch.GET)
On 2015/08/10 at 23:04:37, agable wrote:
> Using GET here makes me a very sad panda. You never had any luck getting the
POST request to go through? What about a PUT request? Maybe we can sit down and
work together to debug what's going wrong there.

lol yea put didn't work either. ok.

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/app.yaml (left):

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/app.yaml:37: - url: /ts-internal-alerts
On 2015/08/10 at 23:04:37, agable wrote:
> Why is this change in this CL?
this endpoint was deleted earlier, should've already been gone

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/__init__.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/__init__.py:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/08/10 at 22:23:58, agable wrote:
> Make sure that there is also an appengine_module/gae_ts_mon/test directory
containing tests for all of these files.

should the tests be copied over? the test folder in ts_mon references
'infra_libs' a lot so it would need to be changed a bit.

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/interface.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/08/10 at 23:04:38, agable wrote:
> You should be able to greatly simplify this file, and move the logic that is
currently in config.py into it. Some simplifications:
> * Fix the docstring at the top to reflect how to use gae_ts_mon, rather than
how to use ts_mon (what symlinks to create, how to configure it, etc.)
> * Remove all references to the flush thread
> * The only flush modes should be "all" or "manual"

^ Did all those things, sorry I missed this file first time around. I actually
think it might be a lot better for code organization to leave config as it's own
file, since config initialize the options for all the singleton classes used in
ts_mon and interface is an interface for the methods we need to call.

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/monitors.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/monitors.py:86: creds =
GoogleCredentials.get_application_default()
On 2015/08/10 at 23:04:38, agable wrote:
> And the pubsub pipeline is accepting these credentials? They don't need to be
whitelisted? Sweet!

Sergey had to add it to the list of whitelisted appengine apps. Would it be
better to not have them need to be whitelisted? Since non-googlers probably
won't have access to ts_mon anyways. I could work with Sergey to change that.

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/proto (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/proto:1:
../../ENV/lib/python2.7/site-packages/monacq/proto/
On 2015/08/10 at 23:04:39, agable wrote:
> This symlink is risky. It works for you? Like, it actually gets uploaded to
appengine and works from there? That's crazy and cool and kinda terrifying
though I guess it makes sense.
> 
> We should find a better way to do this (symlinking to the repo, rather than to
something inside the virtualenv ENV/) but this may be fine for now.

I know I can't believe it worked!! and okay, where do you suggest we move the
proto repo?

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/third_party/apiclient (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/third_party/apiclient:1:
../../../appengine/third_party/apiclient
On 2015/08/10 at 22:23:58, agable wrote:
> Given that you have 8 symlinks all into appengine/third_party/*, I'd replace
that with a single symlink directly to appengine/third_party.

ideally good idea, but there are too many other libraries in third_party and
appengine won't allow it. when you try to deploy it will say:
05:00 PM Rolling back the update.
Error 400: --- begin server output ---
Max number of files and blobs is 10000.
--- end server output ---

[agable, 2015-08-12 22:18:09]

https://codereview.chromium.org/1260293009/diff/220001/appengine/chromium_bui...
File appengine/chromium_build_logs/third_party/apiclient/__init__.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine/chromium_bui...
appengine/chromium_build_logs/third_party/apiclient/__init__.py:1: """Retain
apiclient as an alias for googleapiclient."""
On 2015/08/11 at 21:57:18, jshu wrote:
> On 2015/08/10 at 22:23:58, agable wrote:
> > Since the symlink above is to appengine/third_party/googleapiclient, why is
chromium_build_logs/third_party/apiclient also being updated in this CL?
> 
> appengine/third_party/googleapiclient symlinks to
chromium_build_logs/third_party/googleapiclient (maybe it should actually be the
other way around) so both are updated, and both need the alias apiclient code.
just wanted to update chromium_build_logs while I'm updating third party
repositories, it probably won't break anything. we can also delete this file
everywhere and change all instances of 'apiclient' to 'googleapiclient'

Aha, I see. Then we should change this. You should put new versions of six and
googleapiclient into third_party. Then you should update chromium_build_logs to
point at those new versions *if* doing so doesn't break it. Having symlinks from
third_party *into* chromium_build_logs is just crazytimes. There's no reason to
risk breaking that app for now.

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/app.yaml (left):

https://codereview.chromium.org/1260293009/diff/220001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/app.yaml:37: - url: /ts-internal-alerts
On 2015/08/11 at 21:57:18, jshu wrote:
> On 2015/08/10 at 23:04:37, agable wrote:
> > Why is this change in this CL?
> this endpoint was deleted earlier, should've already been gone

Probably better to do it in a super simple separate CL.

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/__init__.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/__init__.py:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/08/11 at 21:57:18, jshu wrote:
> On 2015/08/10 at 22:23:58, agable wrote:
> > Make sure that there is also an appengine_module/gae_ts_mon/test directory
containing tests for all of these files.
> 
> should the tests be copied over? the test folder in ts_mon references
'infra_libs' a lot so it would need to be changed a bit.

Yes, the tests should be copied, and modified so that they test this code
(including not testing the removed code, and testing the added code (e.g.
config.py)).

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/monitors.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/monitors.py:86: creds =
GoogleCredentials.get_application_default()
On 2015/08/11 at 21:57:18, jshu wrote:
> On 2015/08/10 at 23:04:38, agable wrote:
> > And the pubsub pipeline is accepting these credentials? They don't need to
be whitelisted? Sweet!
> 
> Sergey had to add it to the list of whitelisted appengine apps. Would it be
better to not have them need to be whitelisted? Since non-googlers probably
won't have access to ts_mon anyways. I could work with Sergey to change that.

Whitelisting is fine for now.

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/alerts.py (right):

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/alerts.py:18: from google.appengine.api import modules
Unused import?

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/alerts_history.py (left):

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/alerts_history.py:71: else:
Keep cleanup changes like this in a separate, quick-to-review CL.

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/monitoring.py (right):

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/monitoring.py:1: #!/usr/bin/env python
This file (appengine module, really) is the meat of getting all this to work. In
order to make it easily re-usable, let's try something:
* Move this file inside appengine_module/gae_ts_mon
* Put monitoring.yaml inside gae_ts_mon
* Put a dispatch.yaml file in s-o-m which points at the monitoring module to
handle /monitoring requests (see
https://cloud.google.com/appengine/docs/python/modules/routing)

That way, for someone else to use this same thing, all they have to do is create
a symlink to gae_ts_mon and add the dispatch.yaml file. That'd be shiny.

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/monitoring.py:41: 'request_type': 'GET'})
The request type should be supplied by whoever calls the /monitoring endpoint
(because someone else is reporting a hit to their own endpoint, which may have
been a GET or a POST or something else), not hardcoded to GET.

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/monitoring.py:47: def post(self):
This appears to be unreachable / dead code. Sucks that we can't get POST to
work, but shouldn't leave this hanging around to confuse folks.

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/monitoring.py:62: 
nit: no empty line

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/utils.py (right):

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/utils.py:1: #!/usr/bin/env python
I'd sort the functions in this file into
a) Auth stuff
b) Datetime stuff
c) Monitoring stuff
(Or any other permutation of that order) rather than having them interleaved
like they are now.

https://codereview.chromium.org/1260293009/diff/300001/appengine_module/confi...
File appengine_module/config.proto (right):

https://codereview.chromium.org/1260293009/diff/300001/appengine_module/confi...
appengine_module/config.proto:1: syntax = "proto3";
See comment on earlier patchset: this file shouldn't live in appengine_module.
It should continue to live in infra_libs/ts_mon, and I don't think there's any
reason to copy it over for gae_ts_mon. And even if there is a reason to copy it
over for gae_ts_mon, then it should live in appengine_module/gae_ts_mon/ instead
of here.

https://codereview.chromium.org/1260293009/diff/300001/infra_libs/http_metric...
File infra_libs/http_metrics.py (right):

https://codereview.chromium.org/1260293009/diff/300001/infra_libs/http_metric...
infra_libs/http_metrics.py:8: request_bytes =
CumulativeDistributionMetric('http/request_bytes')
These all seem like great metrics to have built in to gae_ts_mon!

[jshu, 2015-08-14 20:47:57]

okay so this actually works. YAY. the gae_ts_mon module should be free standing
now and more generalized for other apps. read the README and let me know what
you think. also, I moved the updated third_party modules to
appengine/third_party which makes a lot more sense, and symlinked those to
chromium_build_logs. the tests in chromium_build_logs all pass so I don't think
it broke anything. right now sheriff-o-matic doesn't send any metrics -- will
add functionality for that later, since not completely sure what metrics to send
and how to send them yet. ideally I would like for the user to be eventually
able to just symlink gae_ts_mon and it will automatically start sending access
metrics from every endpoint without any other configurations, working on that
now.

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/__init__.py (right):

https://codereview.chromium.org/1260293009/diff/220001/appengine_module/gae_t...
appengine_module/gae_ts_mon/__init__.py:1: # Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/08/12 at 22:18:08, agable wrote:
> On 2015/08/11 at 21:57:18, jshu wrote:
> > On 2015/08/10 at 22:23:58, agable wrote:
> > > Make sure that there is also an appengine_module/gae_ts_mon/test directory
containing tests for all of these files.
> > 
> > should the tests be copied over? the test folder in ts_mon references
'infra_libs' a lot so it would need to be changed a bit.
> 
> Yes, the tests should be copied, and modified so that they test this code
(including not testing the removed code, and testing the added code (e.g.
config.py)).
got it. I've created empty placeholder files for tests, should I implement tests
in a different cl since this one is already so long?

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
File appengine/sheriff_o_matic/alerts_history.py (left):

https://codereview.chromium.org/1260293009/diff/300001/appengine/sheriff_o_ma...
appengine/sheriff_o_matic/alerts_history.py:71: else:
On 2015/08/12 at 22:18:08, agable wrote:
> Keep cleanup changes like this in a separate, quick-to-review CL.

so this actually fixes a bug that I created I refactored the db. if this line
stays in it'll say 500 server error whenever someone tries to acccess
/alerts-history. can it stay in since the app will break otherwise?

https://codereview.chromium.org/1260293009/diff/300001/appengine_module/confi...
File appengine_module/config.proto (right):

https://codereview.chromium.org/1260293009/diff/300001/appengine_module/confi...
appengine_module/config.proto:1: syntax = "proto3";
On 2015/08/12 at 22:18:08, agable wrote:
> See comment on earlier patchset: this file shouldn't live in appengine_module.
It should continue to live in infra_libs/ts_mon, and I don't think there's any
reason to copy it over for gae_ts_mon. And even if there is a reason to copy it
over for gae_ts_mon, then it should live in appengine_module/gae_ts_mon/ instead
of here.

whoops, don't know how that got here haha

https://codereview.chromium.org/1260293009/diff/300001/infra_libs/http_metric...
File infra_libs/http_metrics.py (right):

https://codereview.chromium.org/1260293009/diff/300001/infra_libs/http_metric...
infra_libs/http_metrics.py:8: request_bytes =
CumulativeDistributionMetric('http/request_bytes')
On 2015/08/12 at 22:18:08, agable wrote:
> These all seem like great metrics to have built in to gae_ts_mon!

good call

[agable, 2015-08-14 21:20:58]

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

[agable, 2015-08-14 21:21:00]

+sergey for one final sanity check.

Sergey: PTAL at the files in appengine_module/gae_ts_mon (especially
monitoring.py) and the changes in infra_libs/ts_mon.

Final comments:
* We want 100% code coverage at all times, so yes, let's make sure to get the
test files into gae_ts_mon/test.
* Yes, just adding the symlink to gae_ts_mon and adding a line or two in
dispatch.yaml would be the shiniest solution. One way to get there could be to
write a subclass of webapp2.RequestHandler which automatically sends metrics
every time it receives/responds to a request, and then have the apps use that
instead of directly using RequestHandler. That's slightly more invasive than
optimal but may be the best approach.

Assuming Sergey approves, I'm also LGTM.

[jshu, 2015-08-14 21:33:48]

On 2015/08/14 at 21:21:00, agable wrote:
> +sergey for one final sanity check.
> 
> Sergey: PTAL at the files in appengine_module/gae_ts_mon (especially
monitoring.py) and the changes in infra_libs/ts_mon.
> 
> Final comments:
> * We want 100% code coverage at all times, so yes, let's make sure to get the
test files into gae_ts_mon/test.
> * Yes, just adding the symlink to gae_ts_mon and adding a line or two in
dispatch.yaml would be the shiniest solution. One way to get there could be to
write a subclass of webapp2.RequestHandler which automatically sends metrics
every time it receives/responds to a request, and then have the apps use that
instead of directly using RequestHandler. That's slightly more invasive than
optimal but may be the best approach.
> 
> Assuming Sergey approves, I'm also LGTM.

Good idea, will look into subclassing RequestHandler

[Sergey Berezin, 2015-08-17 21:31:10]

On 2015/08/14 21:33:48, jshu wrote:
> On 2015/08/14 at 21:21:00, agable wrote:
> > +sergey for one final sanity check.
> > 
> > Sergey: PTAL at the files in appengine_module/gae_ts_mon (especially
> monitoring.py) and the changes in infra_libs/ts_mon.
> > 
> > Final comments:
> > * We want 100% code coverage at all times, so yes, let's make sure to get
the
> test files into gae_ts_mon/test.
> > * Yes, just adding the symlink to gae_ts_mon and adding a line or two in
> dispatch.yaml would be the shiniest solution. One way to get there could be to
> write a subclass of webapp2.RequestHandler which automatically sends metrics
> every time it receives/responds to a request, and then have the apps use that
> instead of directly using RequestHandler. That's slightly more invasive than
> optimal but may be the best approach.
> > 
> > Assuming Sergey approves, I'm also L GTM.
> 
> Good idea, will look into subclassing RequestHandler

I'm behind my reviews - but will finish it today. I do have comments; in
particular, be careful with subclassing RequestHandler, as components/auth
already uses a subclass AuthenticatedHandler, and this would prevent them being
used together.

[agable, 2015-08-17 21:33:09]

On 2015/08/17 at 21:31:10, sergeyberezin wrote:
> On 2015/08/14 21:33:48, jshu wrote:
> > On 2015/08/14 at 21:21:00, agable wrote:
> > > +sergey for one final sanity check.
> > > 
> > > Sergey: PTAL at the files in appengine_module/gae_ts_mon (especially
> > monitoring.py) and the changes in infra_libs/ts_mon.
> > > 
> > > Final comments:
> > > * We want 100% code coverage at all times, so yes, let's make sure to get
the
> > test files into gae_ts_mon/test.
> > > * Yes, just adding the symlink to gae_ts_mon and adding a line or two in
> > dispatch.yaml would be the shiniest solution. One way to get there could be
to
> > write a subclass of webapp2.RequestHandler which automatically sends metrics
> > every time it receives/responds to a request, and then have the apps use
that
> > instead of directly using RequestHandler. That's slightly more invasive than
> > optimal but may be the best approach.
> > > 
> > > Assuming Sergey approves, I'm also L GTM.
> > 
> > Good idea, will look into subclassing RequestHandler
> 
> I'm behind my reviews - but will finish it today. I do have comments; in
particular, be careful with subclassing RequestHandler, as components/auth
already uses a subclass AuthenticatedHandler, and this would prevent them being
used together.

This CL doesn't yet include any subclassing, so don't worry about that for now.

[jshu, 2015-08-17 23:02:25]

cleaned up this CL :) so this should all work except gae_ts_mon can't be
imported from appengine_module, so maybe we should talk about moving it into
luci/components. otherwise gae_ts_mon is freestanding and works, we just can't
use gae_ts_mon methods from the default module right now.

[Sergey Berezin, 2015-08-18 00:40:38]

LGTM + a bunch of nits. Also - empty test files? :-)

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/README (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/README:30:
gae_ts_mon.access_count.increment(fields={'url_path': url + '/' + key,
nit: It would be great to list all predefined metrics, or point where to find
them.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/README:33: gae_ts_mon.request_bytes.set(48)
nit: misleading example, since request_bytes is a cumulative distribution
metric. Use .add(48) instead.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/config.py (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:43: if endpoint.startswith('pubsub://'):
nit: I feel this logic should belong in ts_mon itself. It'll work now, but if we
switch to yet another future service later, we'll have to modify it in several
places.

If you plan to land it as is, please file a bug against Infra-Monitoring to fix
it later.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/interface.py (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:14:
gae_ts_mon.initialize(job_name='job', instance='12d2e1',
nit: is the instance value supposed to be a constant? Or should it be a real
instance ID?

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:26: target=t)
nit: indentation.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:30: gae_ts_mon.flush()
nit: I believe this should be called automatically by ts_mon? Or on GAE we
should manually set it up to be called, e.g. from a cron job?

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:46: METRICS_DATA_LENGTH_LIMIT = 5000
nit: we are reducing this to 1000 - see
https://codereview.chromium.org/1296863003/

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/monitoring.py (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/monitoring.py:35: endpoint =
'pubsub://chrome-infra-mon-pubsub/monacq'
nit: in general, it's a bad idea to hard-code endpoints. At the very least,
let's make it into a constant and define at the top of the file, for visibility.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/monitoring.py:46: self.response.write('Metrics
updated.')
nit: change it to logging.info(). Response will likely be lost in a cron job.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/monitors.py (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/monitors.py:62: # Copied from
acquisition_api.AcquisitionCredential.Load.
nit: I don't believe there is much overlap. It's actually rather generic Cloud
API client code. I'd remove the comment.

https://codereview.chromium.org/1260293009/diff/440001/infra_libs/ts_mon/moni...
File infra_libs/ts_mon/monitors.py (right):

https://codereview.chromium.org/1260293009/diff/440001/infra_libs/ts_mon/moni...
infra_libs/ts_mon/monitors.py:125: except
acquisition_api.AcquisitionApiRequestException as e: # pylint:
disable=undefined-variable
nit: any way to stay within 80 char limit? Maybe try it right above the line?

[jshu, 2015-08-18 01:15:55]

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/config.py (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:43: if endpoint.startswith('pubsub://'):
On 2015/08/18 at 00:40:37, Sergey Berezin wrote:
> nit: I feel this logic should belong in ts_mon itself. It'll work now, but if
we switch to yet another future service later, we'll have to modify it in
several places.
> 
> If you plan to land it as is, please file a bug against Infra-Monitoring to
fix it later.

thanks for drawing attention this, going to take out that if statement since I
set the endpoint in gae_ts_mon myself in another file so there is no point in
checking it. the logic is in ts_mon itself, it was just duplicated here because
they don't inherit form each other. but if i understand correctly someone should
update it since we are now only using pubsub? not sure if I would be the right
person to file the bug since I don't know much about that project and it has
little to do with gae_ts_mon.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/interface.py (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:14:
gae_ts_mon.initialize(job_name='job', instance='12d2e1',
On 2015/08/18 at 00:40:38, Sergey Berezin wrote:
> nit: is the instance value supposed to be a constant? Or should it be a real
instance ID?

should be a real instance id, which is an int. fixed.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:30: gae_ts_mon.flush()
On 2015/08/18 at 00:40:38, Sergey Berezin wrote:
> nit: I believe this should be called automatically by ts_mon? Or on GAE we
should manually set it up to be called, e.g. from a cron job?

called from a cron job. leaving that in to give them option of flushing manually
-- says in readme that ts_mon flushing automatically as well.

https://codereview.chromium.org/1260293009/diff/440001/infra_libs/ts_mon/moni...
File infra_libs/ts_mon/monitors.py (right):

https://codereview.chromium.org/1260293009/diff/440001/infra_libs/ts_mon/moni...
infra_libs/ts_mon/monitors.py:125: except
acquisition_api.AcquisitionApiRequestException as e: # pylint:
disable=undefined-variable
On 2015/08/18 at 00:40:38, Sergey Berezin wrote:
> nit: any way to stay within 80 char limit? Maybe try it right above the line?

don't think we actually need that comment

[jshu, 2015-08-18 01:21:53]

The CQ bit was checked by jshu@google.com

[jshu, 2015-08-18 01:21:54]

The patchset sent to the CQ was uploaded after l-g-t-m from agable@chromium.org,
sergeyberezin@chromium.org
Link to the patchset: https://codereview.chromium.org/1260293009/#ps460001
(title: "fix nits")

[commit-bot: I haz the power, 2015-08-18 01:22:06]

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

[commit-bot: I haz the power, 2015-08-18 01:25:34]

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

[commit-bot: I haz the power, 2015-08-18 01:25:36]

Try jobs failed on following builders:
  infra_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_presubmit...)

[jshu, 2015-08-18 17:13:11]

The CQ bit was checked by jshu@google.com

[jshu, 2015-08-18 17:13:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org, agable@chromium.org
Link to the patchset: https://codereview.chromium.org/1260293009/#ps480001
(title: "fix broken symlink")

[commit-bot: I haz the power, 2015-08-18 17:13:28]

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

[commit-bot: I haz the power, 2015-08-18 17:17:33]

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

[commit-bot: I haz the power, 2015-08-18 17:17:34]

Try jobs failed on following builders:
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[jshu, 2015-08-18 17:22:54]

The CQ bit was checked by jshu@google.com

[jshu, 2015-08-18 17:22:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org, agable@chromium.org
Link to the patchset: https://codereview.chromium.org/1260293009/#ps500001
(title: "fix init imports in gae_ts_mon")

[commit-bot: I haz the power, 2015-08-18 17:23:00]

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

[commit-bot: I haz the power, 2015-08-18 17:27:00]

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

[commit-bot: I haz the power, 2015-08-18 17:27:01]

Try jobs failed on following builders:
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[Sergey Berezin, 2015-08-18 17:31:38]

A few more comments

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/config.py (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:43: if endpoint.startswith('pubsub://'):
On 2015/08/18 01:15:55, jshu wrote:
> On 2015/08/18 at 00:40:37, Sergey Berezin wrote:
> > nit: I feel this logic should belong in ts_mon itself. It'll work now, but
if
> we switch to yet another future service later, we'll have to modify it in
> several places.
> > 
> > If you plan to land it as is, please file a bug against Infra-Monitoring to
> fix it later.
> 
> thanks for drawing attention this, going to take out that if statement since I
> set the endpoint in gae_ts_mon myself in another file so there is no point in
> checking it. the logic is in ts_mon itself, it was just duplicated here
because
> they don't inherit form each other. but if i understand correctly someone
should
> update it since we are now only using pubsub? not sure if I would be the right
> person to file the bug since I don't know much about that project and it has
> little to do with gae_ts_mon.

Filed http://crbug.com/522135

The danger of duplicating code is that it becomes inconsistent with time. We are
using pubsub exclusively right now, but I don't know what will happen, say, a
year from now.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/interface.py (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:14:
gae_ts_mon.initialize(job_name='job', instance='12d2e1',
On 2015/08/18 01:15:55, jshu wrote:
> On 2015/08/18 at 00:40:38, Sergey Berezin wrote:
> > nit: is the instance value supposed to be a constant? Or should it be a real
> instance ID?
> 
> should be a real instance id, which is an int. fixed.

Two comments then:

1. In README, let's use <instance id>, or the actual GAE code:
modules.get_current_instance_id(). Otherwise the example is confusing.

2. According to https://cloud.google.com/appengine/docs/python/modules/ GAE
instance IDs are unique base64-encoded strings, which likely never repeat. This
will create arbitrarily many streams in Monarch, which it's not designed to
handle. We may need to map the unique instance IDs to a small set of numbers,
e.g. to [0..#instances-1].

It's OK to land this CL like this, but please file a bug against
Infra-Monitoring to take care of this.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:30: gae_ts_mon.flush()
On 2015/08/18 01:15:55, jshu wrote:
> On 2015/08/18 at 00:40:38, Sergey Berezin wrote:
> > nit: I believe this should be called automatically by ts_mon? Or on GAE we
> should manually set it up to be called, e.g. from a cron job?
> 
> called from a cron job. leaving that in to give them option of flushing
manually
> -- says in readme that ts_mon flushing automatically as well.

I'd still mention what you said in the comment; otherwise it suggests that
flush() needs to be called.

[agable, 2015-08-18 18:50:48]

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

[commit-bot: I haz the power, 2015-08-18 18:51:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260293009/520001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260293009/520001

[commit-bot: I haz the power, 2015-08-18 18:55:23]

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

[commit-bot: I haz the power, 2015-08-18 18:55:25]

Dry run: Try jobs failed on following builders:
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[jshu, 2015-08-19 01:09:27]

The CQ bit was checked by jshu@google.com

[jshu, 2015-08-19 01:09:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org, agable@chromium.org
Link to the patchset: https://codereview.chromium.org/1260293009/#ps560001
(title: "fix remaining test problems")

[commit-bot: I haz the power, 2015-08-19 01:09:44]

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

[agable, 2015-08-19 01:14:42]

https://codereview.chromium.org/1260293009/diff/560001/infra_libs/ts_mon/inte...
File infra_libs/ts_mon/interface.py (right):

https://codereview.chromium.org/1260293009/diff/560001/infra_libs/ts_mon/inte...
infra_libs/ts_mon/interface.py:121: return
I don't think this does what we want: This means that the registration will be
successful, but the newly-created Metric object will not be added to the global
state, so when flush() is called any data points which have been added to it
won't get sent.

I think what we actually want to do is

here:
if metric == m:
  return m

and in the Metric constructor call interface.register() and if that returns
something then replace ourselves with that. Note that this may require
implementing __new__ as well as __init__.

[commit-bot: I haz the power, 2015-08-19 01:15:43]

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

[commit-bot: I haz the power, 2015-08-19 01:15:44]

Try jobs failed on following builders:
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[jshu, 2015-08-19 22:44:06]

The CQ bit was checked by jshu@google.com

[jshu, 2015-08-19 22:44:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org, agable@chromium.org
Link to the patchset: https://codereview.chromium.org/1260293009/#ps620001
(title: "fix test coverage on importerrors")

[commit-bot: I haz the power, 2015-08-19 22:44:23]

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

[commit-bot: I haz the power, 2015-08-19 22:49:21]

Committed patchset #32 (id:620001) as
https://chromium.googlesource.com/infra/infra/+/65322c283b681fc75ea3c0be07ef0...

[jshu, 2015-08-20 00:23:34]

please lgtm!

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/config.py (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/config.py:43: if endpoint.startswith('pubsub://'):
On 2015/08/18 at 17:31:38, Sergey Berezin wrote:
> On 2015/08/18 01:15:55, jshu wrote:
> > On 2015/08/18 at 00:40:37, Sergey Berezin wrote:
> > > nit: I feel this logic should belong in ts_mon itself. It'll work now, but
if
> > we switch to yet another future service later, we'll have to modify it in
> > several places.
> > > 
> > > If you plan to land it as is, please file a bug against Infra-Monitoring
to
> > fix it later.
> > 
> > thanks for drawing attention this, going to take out that if statement since
I
> > set the endpoint in gae_ts_mon myself in another file so there is no point
in
> > checking it. the logic is in ts_mon itself, it was just duplicated here
because
> > they don't inherit form each other. but if i understand correctly someone
should
> > update it since we are now only using pubsub? not sure if I would be the
right
> > person to file the bug since I don't know much about that project and it has
> > little to do with gae_ts_mon.
> 
> Filed http://crbug.com/522135
> 
> The danger of duplicating code is that it becomes inconsistent with time. We
are using pubsub exclusively right now, but I don't know what will happen, say,
a year from now.

Those two files are completely different.

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
File appengine_module/gae_ts_mon/interface.py (right):

https://codereview.chromium.org/1260293009/diff/440001/appengine_module/gae_t...
appengine_module/gae_ts_mon/interface.py:14:
gae_ts_mon.initialize(job_name='job', instance='12d2e1',
On 2015/08/18 at 17:31:38, Sergey Berezin wrote:
> On 2015/08/18 01:15:55, jshu wrote:
> > On 2015/08/18 at 00:40:38, Sergey Berezin wrote:
> > > nit: is the instance value supposed to be a constant? Or should it be a
real
> > instance ID?
> > 
> > should be a real instance id, which is an int. fixed.
> 
> Two comments then:
> 
> 1. In README, let's use <instance id>, or the actual GAE code:
modules.get_current_instance_id(). Otherwise the example is confusing.
> 
> 2. According to https://cloud.google.com/appengine/docs/python/modules/ GAE
instance IDs are unique base64-encoded strings, which likely never repeat. This
will create arbitrarily many streams in Monarch, which it's not designed to
handle. We may need to map the unique instance IDs to a small set of numbers,
e.g. to [0..#instances-1].
> 
> It's OK to land this CL like this, but please file a bug against
Infra-Monitoring to take care of this.

fixed