WFE: Track in-flight HTTP requests by endpoint using a gauge #7758
Conversation
There was a problem hiding this comment.
I think this would be better implemented in metrics/measured_http. That way we get automatic extraction of the endpoint, and the measurement code is more nicely separated from the WFE code. Also, we get the same benefit for other web servers in Boulder, i.e. the OCSP responder.
beautifulentropy
force-pushed
the
wfe-inflight-http
branch
from
f464729
to
d39f4ba
Compare
Thanks! I wish I had noticed this existed earlier. |
beautifulentropy
changed the title
| }, "server", opts...) | ||
| } | ||
|
|
||
| func (h *MeasuredHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
| begin := h.clk.Now() | ||
| rwws := &responseWriterWithStatus{w, 0} | ||
|
|
||
| subHandler, pattern := h.Handler(r) | ||
| if pattern != "/" { |
There was a problem hiding this comment.
We're mostly concerned about endpoints that dispatch RPCs, I knew we could safely discard "/". We could probably discard others as well.
There was a problem hiding this comment.
Fair enough. I think on balance, having fewer lines lines of code is slightly more appealing than economizing on the one timeseries, but I can really go either way on this one.
There was a problem hiding this comment.
Agreed. I think that excluding "/" makes sense if this were implemented in the WFE, where we could also exclude other endpoints we don't care about, like "/build". But here in this helper library that may be used by other webservers that do care about the root path, excluding it is likely to lead to confusion in the future.
| }, "server", opts...) | ||
| } | ||
|
|
||
| func (h *MeasuredHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
| begin := h.clk.Now() | ||
| rwws := &responseWriterWithStatus{w, 0} | ||
|
|
||
| subHandler, pattern := h.Handler(r) | ||
| if pattern != "/" { |
There was a problem hiding this comment.
Agreed. I think that excluding "/" makes sense if this were implemented in the WFE, where we could also exclude other endpoints we don't care about, like "/build". But here in this helper library that may be used by other webservers that do care about the root path, excluding it is likely to lead to confusion in the future.
|
@aarongable I agree with this argument. I want to get this merged before our next tag, I'll write a follow-up that can be merged whenever. |
No description provided.