Enable AppEngine profiling endpoints.

Enable AppEngine profiling endpoints.

BUG=chromium:675485
TEST=None
R=vadimsh@chromium.org

Review-Url: https://codereview.chromium.org/2588643002
Committed: https://github.com/luci/luci-go/commit/3f27478caedc4593adf1ebc8f4ad71189eb325b9

[dnj, 2016-12-18 22:31:10]

PTAL

[Vadim Sh., 2016-12-18 22:44:38]

https://codereview.chromium.org/2588643002/diff/1/appengine/gaemiddleware/rou...
File appengine/gaemiddleware/routes.go (right):

https://codereview.chromium.org/2588643002/diff/1/appengine/gaemiddleware/rou...
appengine/gaemiddleware/routes.go:29: func InstallHandlers(r *router.Router,
base router.MiddlewareChain) {
this thing is supposed to be used only on 'default' module. Non-default modules
often don't even have auth system configured (they just use 'require: admin' in
module yaml), and RequireSuperuser check fails in them.

https://codereview.chromium.org/2588643002/diff/1/common/runtime/profiling/pr...
File common/runtime/profiling/profiler.go (right):

https://codereview.chromium.org/2588643002/diff/1/common/runtime/profiling/pr...
common/runtime/profiling/profiler.go:12: httpProf "net/http/pprof"
This import registers routes in default net/http router (in init()). Same as
used by GAE app. This can expose profiling info if user don't ever call
'InstallHandlers', or uses a router with some different suffix (not "/").

I think we should just follow standard lib: if an app wants profiling, it does
1) "import _ "net/http/pprof"
2) adds "/debug/pprof" as 'require: admin' to app.yaml

[dnj, 2016-12-18 22:54:41]

On 2016/12/18 22:44:38, Vadim Sh. wrote:
>
https://codereview.chromium.org/2588643002/diff/1/appengine/gaemiddleware/rou...
> File appengine/gaemiddleware/routes.go (right):
> 
>
https://codereview.chromium.org/2588643002/diff/1/appengine/gaemiddleware/rou...
> appengine/gaemiddleware/routes.go:29: func InstallHandlers(r *router.Router,
> base router.MiddlewareChain) {
> this thing is supposed to be used only on 'default' module. Non-default
modules
> often don't even have auth system configured (they just use 'require: admin'
in
> module yaml), and RequireSuperuser check fails in them.
> 
>
https://codereview.chromium.org/2588643002/diff/1/common/runtime/profiling/pr...
> File common/runtime/profiling/profiler.go (right):
> 
>
https://codereview.chromium.org/2588643002/diff/1/common/runtime/profiling/pr...
> common/runtime/profiling/profiler.go:12: httpProf "net/http/pprof"
> This import registers routes in default net/http router (in init()). Same as
> used by GAE app. This can expose profiling info if user don't ever call
> 'InstallHandlers', or uses a router with some different suffix (not "/").
> 
> I think we should just follow standard lib: if an app wants profiling, it does
> 1) "import _ "net/http/pprof"
> 2) adds "/debug/pprof" as 'require: admin' to app.yaml

Yeah, this scares me a bit b/c some apps expose "/" without restrictions, so it
feels like a leak waiting to happen. I was thinking that requiring some module
to consciously register the middleware may be a good idea.

I suppose we do use default HTTP handler in our GAE. I hate globals :(

[dnj, 2016-12-19 16:23:35]

Okay rewrote this to use net/http/pprof directly, exposed specifically for
LogDog.

[Vadim Sh., 2016-12-19 20:23:36]

lgtm

[dnj, 2016-12-19 21:44:18]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-12-19 21:44:29]

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-12-19 21:50:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482183858953080,
"parent_rev": "11577104592605b1e94476b2bb75385abcc45d8e", "commit_rev":
"3f27478caedc4593adf1ebc8f4ad71189eb325b9"}

[commit-bot: I haz the power, 2016-12-19 21:50:09]

Description was changed from

==========
Enable AppEngine profiling endpoints.

BUG=chromium:675485
TEST=None
R=vadimsh@chromium.org
==========

to

==========
Enable AppEngine profiling endpoints.

BUG=chromium:675485
TEST=None
R=vadimsh@chromium.org

Review-Url: https://codereview.chromium.org/2588643002
Committed:
https://github.com/luci/luci-go/commit/3f27478caedc4593adf1ebc8f4ad71189eb325b9
==========

[commit-bot: I haz the power, 2016-12-19 21:50:10]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-go/commit/3f27478caedc4593adf1ebc8f4ad71189eb325b9