isolateserver: add checkbox to enable ts_mon

isolateserver: add checkbox to enable ts_mon

R=maruel@chromium.org
BUG=408424
TEST= ran unit tests, and tested on dev_appserver instance.

Committed: https://github.com/luci/luci-py/commit/50b5ee37a0f23143dd71028676cbc4c36abe797d

[Sergey Berezin, 2016-02-18 20:56:15]

Description was changed from

==========
isolateserver: add checkbox to enable ts_mon

R=maruel@chromium.org
BUG=408424
==========

to

==========
isolateserver: add checkbox to enable ts_mon

R=maruel@chromium.org
BUG=408424
TEST= ran unit tests, and tested on dev_appserver instance.
==========

[Sergey Berezin, 2016-02-18 20:56:51]

PTAL

[Sergey Berezin, 2016-02-19 02:14:04]

Uploaded a demo to isolateserver-dev, and toggled the box. At some point,
metrics should begin to show up (when instances restart):
http://shortn/_nqq4nGzuh9

[M-A Ruel, 2016-02-19 02:44:40]

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/config.py
File appengine/isolate/config.py (right):

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/config.py...
appengine/isolate/config.py:58: @classmethod
This code belongs to the frontend in my opinion, not here.

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/handlers_...
File appengine/isolate/handlers_frontend.py (right):

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/handlers_...
appengine/isolate/handlers_frontend.py:65: 
you can add the function here as a @staticmethod, not a classmethod.

[Sergey Berezin, 2016-02-19 20:35:32]

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/config.py
File appengine/isolate/config.py (right):

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/config.py...
appengine/isolate/config.py:58: @classmethod
On 2016/02/19 02:44:40, M-A Ruel wrote:
> This code belongs to the frontend in my opinion, not here.

IMHO, keeping the type conversion closer to the definition of the parameters is
more intuitive. You only need to modify one class (and one file) when updating
the types, and the cast can be reused in other places, not just in the handler.

But the way it's set up now, the handler still needs to know all the parameters
anyway, so I don't feel strongly about it. Your call.

[M-A Ruel, 2016-02-19 20:37:27]

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/config.py
File appengine/isolate/config.py (right):

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/config.py...
appengine/isolate/config.py:58: @classmethod
On 2016/02/19 20:35:32, Sergey Berezin wrote:
> On 2016/02/19 02:44:40, M-A Ruel wrote:
> > This code belongs to the frontend in my opinion, not here.
> 
> IMHO, keeping the type conversion closer to the definition of the parameters
is
> more intuitive. You only need to modify one class (and one file) when updating
> the types, and the cast can be reused in other places, not just in the
handler.
> 
> But the way it's set up now, the handler still needs to know all the
parameters
> anyway, so I don't feel strongly about it. Your call.

Yes but this function is purely related to form like behavior. If it were an API
for example, the format would be different. Because it is about a web form, this
is leaking the abstraction layer too low, that's why it should be in
handlers_frontend.

[Sergey Berezin, 2016-02-19 20:46:52]

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/config.py
File appengine/isolate/config.py (right):

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/config.py...
appengine/isolate/config.py:58: @classmethod
On 2016/02/19 20:37:26, M-A Ruel wrote:
> On 2016/02/19 20:35:32, Sergey Berezin wrote:
> > On 2016/02/19 02:44:40, M-A Ruel wrote:
> > > This code belongs to the frontend in my opinion, not here.
> > 
> > IMHO, keeping the type conversion closer to the definition of the parameters
> is
> > more intuitive. You only need to modify one class (and one file) when
updating
> > the types, and the cast can be reused in other places, not just in the
> handler.
> > 
> > But the way it's set up now, the handler still needs to know all the
> parameters
> > anyway, so I don't feel strongly about it. Your call.
> 
> Yes but this function is purely related to form like behavior. If it were an
API
> for example, the format would be different. Because it is about a web form,
this
> is leaking the abstraction layer too low, that's why it should be in
> handlers_frontend.

OK, done.

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/handlers_...
File appengine/isolate/handlers_frontend.py (right):

https://codereview.chromium.org/1708283003/diff/1/appengine/isolate/handlers_...
appengine/isolate/handlers_frontend.py:65: 
On 2016/02/19 02:44:40, M-A Ruel wrote:
> you can add the function here as a @staticmethod, not a classmethod.

Done.

[M-A Ruel, 2016-02-19 20:51:49]

https://codereview.chromium.org/1708283003/diff/20001/appengine/isolate/handl...
File appengine/isolate/handlers_frontend.py (right):

https://codereview.chromium.org/1708283003/diff/20001/appengine/isolate/handl...
appengine/isolate/handlers_frontend.py:73: def to_str(value):
Remove.

https://codereview.chromium.org/1708283003/diff/20001/appengine/isolate/handl...
appengine/isolate/handlers_frontend.py:78: }.get(param_name, to_str)
s/to_str/str/

[Sergey Berezin, 2016-02-19 23:20:34]

https://codereview.chromium.org/1708283003/diff/20001/appengine/isolate/handl...
File appengine/isolate/handlers_frontend.py (right):

https://codereview.chromium.org/1708283003/diff/20001/appengine/isolate/handl...
appengine/isolate/handlers_frontend.py:73: def to_str(value):
On 2016/02/19 20:51:49, M-A Ruel wrote:
> Remove.

Done.

https://codereview.chromium.org/1708283003/diff/20001/appengine/isolate/handl...
appengine/isolate/handlers_frontend.py:78: }.get(param_name, to_str)
On 2016/02/19 20:51:49, M-A Ruel wrote:
> s/to_str/str/

Good catch, thx! done.

[M-A Ruel, 2016-02-19 23:24:30]

lgtm

[Sergey Berezin, 2016-02-19 23:26:39]

The CQ bit was checked by sergeyberezin@chromium.org

[commit-bot: I haz the power, 2016-02-19 23:26:48]

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

[commit-bot: I haz the power, 2016-02-19 23:28:17]

Description was changed from

==========
isolateserver: add checkbox to enable ts_mon

R=maruel@chromium.org
BUG=408424
TEST= ran unit tests, and tested on dev_appserver instance.
==========

to

==========
isolateserver: add checkbox to enable ts_mon

R=maruel@chromium.org
BUG=408424
TEST= ran unit tests, and tested on dev_appserver instance.

Committed:
https://github.com/luci/luci-py/commit/50b5ee37a0f23143dd71028676cbc4c36abe797d
==========

[commit-bot: I haz the power, 2016-02-19 23:28:17]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-py/commit/50b5ee37a0f23143dd71028676cbc4c36abe797d

[Sergey Berezin, 2016-02-20 00:11:38]

(posted this comment on the wrong CL :-)
Deployed it on isolateserver-dev; will check with you on Monday for pushing to
the production version. Thanks!