Store alerts in sheriff-o-matic's memcache.

Store alerts in sheriff-o-matic's memcache.

TEST=Tools/GardeningServer/tests.py
BUG=401358
R=eseidel@chromium.org, ojan@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180153

[dominicc (has gone to gerrit), 2014-08-13 05:06:08]

PTAL

[eseidel, 2014-08-13 05:12:22]

seems fine to me.

We'll eventually want to move this code to infra/ but that's a separate
discussion for another day.  :)  I'm just seeing patterns now between the two
repos which are silly to repeat (like needing special code for appengine based
tests).

[dominicc (has gone to gerrit), 2014-08-13 05:17:00]

Context for reviewers:

This is adapted from nannybot's /data URL; code is here:
https://github.com/eseidel/cycletimes/blob/master/nannybot/main.py

Some important differences:

- Alerts are stored in memcache. That is not a durable or consistent store. But
clients just need pretty fresh data any way, and this is frequently updated, so
memcache is enough. This is the premise of Issue 401358.

- "Ignore rules" were not ported. dstockwell's snoozable alerts obviate the
need. There is probably some clean-up possible in the Polymer code after
switching over to this.

To see this locally, you can:

1. Install the AppEngine SDK
2. dev_appserver.py Tools/GardeningServer
3. In a new working directory: fetch infra
4. cd infra
5. python run.py infra.tools.builder_alerts --use-cache
http://localhost:8080/alerts
6. Visit http://localhost:8080/alerts

[eseidel, 2014-08-13 05:30:27]

I don't think you want --use-cache. :)  We should just delete it.

[eseidel, 2014-08-13 05:36:09]

to be clear: --use-cache causes all request made by the script to be cached
indefinitely (on disk).  That means that if a build is in progress, it will
always be in progress.  If you run with --use-cache you'll just end up always
producing the same stale data forever. :)

Super useful for debugging/development.  Not useful for production.

when --use-cache is false (the default) we have a requests_cache installed, its
just in-memory and goes away at the end of the run.

[eseidel, 2014-08-13 05:43:31]

in any case, the right solution is for me to just update the help-text of the
script.  will do that tomorrow.

[eseidel, 2014-08-13 05:44:22]

https://codereview.chromium.org/465233002/diff/1/Tools/GardeningServer/alerts...
File Tools/GardeningServer/alerts_test.py (right):

https://codereview.chromium.org/465233002/diff/1/Tools/GardeningServer/alerts...
Tools/GardeningServer/alerts_test.py:29: def testGet_NoDataCached(self):
btw, I think blink follows pep8, so this should be test_get_no_data_cached

[dominicc (has gone to gerrit), 2014-08-13 06:03:32]

On 2014/08/13 05:36:09, eseidel wrote:
> to be clear: --use-cache causes all request made by the script to be cached
> indefinitely (on disk).  That means that if a build is in progress, it will
> always be in progress.  If you run with --use-cache you'll just end up always
> producing the same stale data forever. :)
> 
> Super useful for debugging/development.  Not useful for production.
> 
> when --use-cache is false (the default) we have a requests_cache installed,
its
> just in-memory and goes away at the end of the run.

Right... this is just to see this thing store and echo something. --use-cache is
useful for that.

In the end even that became tedious, so I just wrote a stub that posted
synthetic data to it.

[eseidel, 2014-08-13 06:11:30]

There used to be a --master-filter parameter which I had to make it run even
faster, but yeah, it can be annoying to wait for it to cycle just to post to
/data.

[ojan, 2014-08-13 06:13:25]

lgtm.

This is great. Thanks for doing this. Feel free to ignore my nits.

https://codereview.chromium.org/465233002/diff/20001/Tools/GardeningServer/al...
File Tools/GardeningServer/alerts.py (right):

https://codereview.chromium.org/465233002/diff/20001/Tools/GardeningServer/al...
Tools/GardeningServer/alerts.py:40: 'alerts': alerts['alerts']})
Bikeshed nit: I'd add a newline before the }

https://codereview.chromium.org/465233002/diff/20001/Tools/GardeningServer/al...
Tools/GardeningServer/alerts.py:45: ('/alerts', AlertsHandler)])
Ditto nit: newline before the ]

[dominicc (has gone to gerrit), 2014-08-13 06:25:00]

On 2014/08/13 06:13:25, ojan-only-code-yellow-reviews wrote:
> lgtm.
> 
> This is great. Thanks for doing this. Feel free to ignore my nits.
> 
>
https://codereview.chromium.org/465233002/diff/20001/Tools/GardeningServer/al...
> File Tools/GardeningServer/alerts.py (right):
> 
>
https://codereview.chromium.org/465233002/diff/20001/Tools/GardeningServer/al...
> Tools/GardeningServer/alerts.py:40: 'alerts': alerts['alerts']})
> Bikeshed nit: I'd add a newline before the }
> 
>
https://codereview.chromium.org/465233002/diff/20001/Tools/GardeningServer/al...
> Tools/GardeningServer/alerts.py:45: ('/alerts', AlertsHandler)])
> Ditto nit: newline before the ]

Thanks for the quick review.

I had it your way originally, but check-webkit-style complains about whitespace
before }. I'll put it back; you and I outvote the machine.

Sadly check-webkit-style doesn't understand all of the appengine module
importing magic, so it will be terminally unhappy with these files (eg it
doesn't know about the "google" in import google.appengine.mumble.)

[dominicc (has gone to gerrit), 2014-08-13 06:26:53]

The CQ bit was checked by dominicc@chromium.org

[commit-bot: I haz the power, 2014-08-13 06:27:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dominicc@chromium.org/465233002/60001

[dominicc (has gone to gerrit), 2014-08-13 06:38:59]

The CQ bit was unchecked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2014-08-13 06:39:34]

eseidel: can I get OWNERS approval for adding Tools/appengine_testing?

[eseidel, 2014-08-13 06:40:54]

lgtm.

[eseidel, 2014-08-13 06:43:21]

I could have sworn we already had app-engine test capabilies.  Maybe they died
when we removed QueueStatusServer:
e.g.
http://trac.webkit.org/browser/trunk/Tools/QueueStatusServer/handlers/queuest...

Then again, back then webkitpy knew how to pull down libraries for you.  These
days, infra/ knows how to do that (as part of virtualenv + gclient hooks).  I'm
not sure what webkitpy/blink's solution for python module dependencies is.

[dominicc (has gone to gerrit), 2014-08-13 06:53:44]

On 2014/08/13 06:43:21, eseidel wrote:
> I could have sworn we already had app-engine test capabilies.  Maybe they died
> when we removed QueueStatusServer:
> e.g.
>
http://trac.webkit.org/browser/trunk/Tools/QueueStatusServer/handlers/queuest...
> 
> Then again, back then webkitpy knew how to pull down libraries for you.  These
> days, infra/ knows how to do that (as part of virtualenv + gclient hooks). 
I'm
> not sure what webkitpy/blink's solution for python module dependencies is.

Would make sense to move this to infra and delete appengine_testing.

RebaselineLogServer's main_unittests survives without this stuff; it uses
dev_appserver.fix_sys_path. I could probably live with that and revert the
changes to testfilehandler_test.

[dominicc (has gone to gerrit), 2014-08-13 07:49:35]

Committed patchset #5 manually as 180153 (presubmit successful).

[dsinclair, 2014-08-13 15:27:55]

https://codereview.chromium.org/465233002/diff/80001/Tools/GardeningServer/ui...
File Tools/GardeningServer/ui/ct-failure-analyzer.html (right):

https://codereview.chromium.org/465233002/diff/80001/Tools/GardeningServer/ui...
Tools/GardeningServer/ui/ct-failure-analyzer.html:70: # FIXME: Change this to
http://sheriff-o-matic.appspot.com/alerts
This isn't valid javascript.

[Dirk Pranke, 2014-08-13 17:03:40]

On 2014/08/13 06:43:21, eseidel wrote:
> I could have sworn we already had app-engine test capabilies.  Maybe they died
> when we removed QueueStatusServer:
> e.g.
>
http://trac.webkit.org/browser/trunk/Tools/QueueStatusServer/handlers/queuest...
> 
> Then again, back then webkitpy knew how to pull down libraries for you.  These
> days, infra/ knows how to do that (as part of virtualenv + gclient hooks). 
I'm
> not sure what webkitpy/blink's solution for python module dependencies is.

test-webkitpy will check for tests in Tools/TestResultServer and run them *if*
it finds /usr/local/google_appengine installed on the machine (i.e., if you have
the GAE SDK installed locally). I don't believe the bots have ever had the SDK
installed.

Currently, webkitpy only has dependencies on 3-4 third party libs, and we just
checked them in to webkitpy/third_party. I got rid of the autoinstaller quite
some time ago.

As you say, these days we could also do something like virtualenv, or just check
in the SDK as a DEP.

I would probably be inclined to make testing this a separate step, though,
rather than lump it in as part of testing webkitpy.

[dominicc (has gone to gerrit), 2014-08-13 23:49:11]

On 2014/08/13 15:27:55, dsinclair wrote:
>
https://codereview.chromium.org/465233002/diff/80001/Tools/GardeningServer/ui...
> File Tools/GardeningServer/ui/ct-failure-analyzer.html (right):
> 
>
https://codereview.chromium.org/465233002/diff/80001/Tools/GardeningServer/ui...
> Tools/GardeningServer/ui/ct-failure-analyzer.html:70: # FIXME: Change this to
> http://sheriff-o-matic.appspot.com/alerts
> This isn't valid javascript.

Doh. Thank you.