-
Notifications
You must be signed in to change notification settings - Fork 72
Use httpsnoop library for tracking status code #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use httpsnoop library for tracking status code #11
Conversation
…all possible interfaces.
r = r.WithContext(opentracing.ContextWithSpan(r.Context(), sp)) | ||
|
||
h.ServeHTTP(w, r) | ||
m := httpsnoop.CaptureMetrics(h, w, r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing about httpsnoop is that its API forces a lot more allocations than necessary. Using CaptureMetrics when you only need status code may be even more expensive. Could you try a mem benchmark before & after?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a benchmark in httpsnoop that seems to imply an overhead of about 500ns: https://github.com/felixge/httpsnoop#performance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just time overhead, but also memory allocations. The CaptureMetrics is a lot heavier than what's needed here if we only want to capture the status code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you accept a PR that just makes statusCodeTracker
implement http.{CloseNotifier,Flusher,Pusher,Hijacker}
by calling those interfaces' methods on the wrapped ResponseWriter
? (I.e., without pulling in all of httpsnoop
.)
(Motivation: At @sourcegraph we need to be able to flush the response body from a handler whose ResponseWriter
is wrapped in statusCodeTracker
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if you think this is a good idea, but I'm partial to this idea I suggested on the go issue about capturing status codes golang/go#18997 (comment) You can just add a method Wrap() http.ResponseWriter
that returns the underlying http.ResponseWriter
and it could become a defacto standard. Or maybe we should choose a better name first.
Is there any progress on this? Currently this issue prevents us from using websockets together with this middleware. a simple func (w *statusCodeTracker) Hijack() (net.Conn, *bufio.ReadWriter, error) {
return w.ResponseWriter.(http.Hijacker).Hijack()
} would be sufficient. |
superseded by #26, closing |
So that the wrapped ResponseWriter implements all possible interfaces.
Related to #10 and #7