census-instrumentation / opencensus-go

A stats collection and distributed tracing framework
http://opencensus.io
Apache License 2.0
2.06k stars 327 forks source link

zpages: make z handlers easier to integrate #813

Open ashi009 opened 6 years ago

ashi009 commented 6 years ago

zpages is somewhat difficult to integrate.

In our setup, we have a dedicated http.Mux for handling internal debug requests. The middlewares on the mux will handle authentication etc. Along with zpages handlers, we have a bunch of other *z handlers for other purposes. To use the zpages, rpcz and tracez must bu nested under another http.Mux, and the additional prefix must be stripped to make Handle happy.

The exported APIs from zpages are Handle and a bunch of methods that are supposed to be used by zpages only. The Handle function registers 3 handlers on the provided http.Mux, and there is no way to register rpcz or tracez directly without copying code from zpages package, and also mock a public/ handler for the static resources.

IMHO the root cause for this is the existence of the public/ handler. It makes rpcz and tracez non-hermetic, and thus impossible to be exported directly.

The proposed changes are:

rakyll commented 6 years ago

/cc @ramonza

semistrict commented 6 years ago

We want to be able to add more pages to zpages in future without having everyone need to change their code. Also, we want to allow linking between pages which only makes sense if all of them are installed at known locations.

Could you install them at their own prefix, for example: /debug/oc/{rpcz,tracez,public}?

ashi009 commented 6 years ago

@ramonza

We have a debug handler registry, which allows us to attach metadata to the handlers, eg. the feature and the doc of the handler, the source location of where the handler was registered etc. All these metadata can be acquired from a **z/ handler.

For this reason, IMHO it's more convenient to make rpcz, tracez and future z handlers exported, so that people may choose what handler to register.

The linking part is tricky, but it can be solved either by making it configurable or adding a comment to say all these handlers must be registered under the same prefix with their original name. Even with the latter option, it still gives other people more flexibility on how to use this package.

semistrict commented 6 years ago

What if we generalized the signature of zpage.Handle to:

func Handle(mux interface { Handle(pattern string, handler http.Handler) }, pathPrefix string)

Then you could use a ServeMux as the mux or a custom registry that satisfies that interface?

rakyll commented 6 years ago

We shouldn't generalize the signature. It is very un-Go to provide an inline interface to provide either this or that. The proper solution is to export the handlers. If users are depending on the handlers, they are somewhat advanced and know what they are doing.