Lazily enable zpages at runtime #774
Conversation
semistrict
force-pushed
the
lazy-zpages
branch
3 times, most recently
from
e1819e0
to
d4a01b3
Compare
| @@ -31,6 +30,7 @@ import ( | |||
| "go.opencensus.io/zpages" | |||
| "golang.org/x/net/context" | |||
| "google.golang.org/grpc" | |||
| "net/http" | |||
zpages/zpages.go
Outdated
| Handler = NewHandler("") | ||
| } | ||
|
|
||
| // GetHandler returns a handler that serves the z-pages. |
zpages/zpages.go
Outdated
| } | ||
|
|
||
| // GetHandler returns a handler that serves the z-pages. | ||
| func NewHandler(prefix string) http.Handler { |
There was a problem hiding this comment.
We shouldn't have the prefix here.
It is coupling the responsibilities of a mux and a handler into a handler.
There was a problem hiding this comment.
I this this is preferable to using StripPrefix. It is much more readable, does not allocate a new request object, and does not try to fake the URL (like StripPrefix does).
|
Thanks @rakyll updated. I prefer the approach with taking a prefix as explained above and would like to keep it unless you feel strongly that it's wrong. After looking at what StripPrefix does, I don't think it's a good idea to recommend its use here if there's an straightforward way to avoid it. |
|
So one way to fix this would be to not care what the prefix is and just match based on the last component of the URL. What do you think about that? Something like: parts := path.Split(req.URL)
page := parts[len(parts-1)]
switch page {
case "rpcz":
...
default:
return http.NotFoundHandler().Handle(...)
}Then we wouldn't need to take a prefix parameter at all and it would work regardless of the path it is installed at? |
zpages/zpages.go
Outdated
| } | ||
|
|
||
| // NewHandler returns a handler that serves the z-pages. | ||
| func NewHandler(prefix string) http.Handler { |
There was a problem hiding this comment.
@rakyll If this were changed to http.ServerMux I think that would work for what I referenced in #783. The http.Handler strips the .Handle* functions and while I could simply cast the return to a ServerMux it feels dirty as I'm relying on knowledge of the implementation detail rather than a public API contract. (Moved this comment from the exported var as I didn’t notice the deprecation comment)
zpages/zpages.go
Outdated
| } | ||
|
|
||
| // AppendHandlers adds the z-pages to the given ServeMux rooted at pathPrefix. | ||
| func AppendHandlers(pathPrefix string, mux *http.ServeMux) *http.ServeMux { |
There was a problem hiding this comment.
Since it's returning a *ServeMux and there's no '/' specified on the mux it's probably ok to drop the mux parameter and create it in the method. /public is a fs folder so additional assets could be added at run-time and (famous last words) hopefully no one would want sub-paths under /tracez and /rpcz.
What are peoples thoughts on two functions?
func zpages.NewPrefixed(pathPrefix string) *ServeMuxfunc zpages.New() *ServeMux
The second basically calls the first with "" and would provide behaviour similar to the original zpages.Handler.
There was a problem hiding this comment.
I think constructing our own ServeMux reduces the flexibility, in particular to add to the DefaultServeMux. Renamed the method to Handle and no longer return the ServeMux to avoid ambiguity.
I don't think we need a separate non-prefixed call. The user can just pass an empty string explicitly, I don't think it's worth adding a convenience method because I would think that the prefixed form would actually be more common in practice.
| http.Handle("/debug/", http.StripPrefix("/debug", zpages.Handler)) | ||
| log.Fatal(http.ListenAndServe(":8081", nil)) | ||
| mux := zpages.AppendHandlers("/debug", http.NewServeMux()) | ||
| log.Fatal(http.ListenAndServe("127.0.0.1:8081", mux)) |
There was a problem hiding this comment.
Like the change to 127.0.0.1 as it doesn't trigger FW notifications on OS X. 👍
|
@rakyll are you ok with the new function? |
zpages/example_test.go
Outdated
| @@ -23,6 +23,6 @@ import ( | |||
|
|
|||
| func Example() { | |||
| // Both /debug/tracez and /debug/rpcz will be served. | |||
zpages/zpages.go
Outdated
| @@ -32,15 +32,40 @@ package zpages // import "go.opencensus.io/zpages" | |||
|
|
|||
| import ( | |||
| "net/http" | |||
| "sync" | |||
|
|
|||
|
LGTM |
See: #772