Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Use httpsnoop library for tracking status code #11

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 3 additions & 13 deletions nethttp/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,11 @@ package nethttp
import (
"net/http"

"github.com/felixge/httpsnoop"
opentracing "github.com/opentracing/opentracing-go"
"github.com/opentracing/opentracing-go/ext"
)

type statusCodeTracker struct {
http.ResponseWriter
status int
}

func (w *statusCodeTracker) WriteHeader(status int) {
w.status = status
w.ResponseWriter.WriteHeader(status)
}

type mwOptions struct {
opNameFunc func(r *http.Request) string
}
Expand Down Expand Up @@ -68,12 +59,11 @@ func Middleware(tr opentracing.Tracer, h http.Handler, options ...MWOption) http
ext.HTTPMethod.Set(sp, r.Method)
ext.HTTPUrl.Set(sp, r.URL.String())
ext.Component.Set(sp, "net/http")
w = &statusCodeTracker{w, 200}
r = r.WithContext(opentracing.ContextWithSpan(r.Context(), sp))

h.ServeHTTP(w, r)
m := httpsnoop.CaptureMetrics(h, w, r)
Copy link
Collaborator

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?

Copy link
Contributor

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

Copy link
Collaborator

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

Copy link

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.)

Copy link

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.


ext.HTTPStatusCode.Set(sp, uint16(w.(*statusCodeTracker).status))
ext.HTTPStatusCode.Set(sp, uint16(m.Code))
sp.Finish()
}
return http.HandlerFunc(fn)
Expand Down