net/http contrib

[gabsn]

net/http support for tracing thanks to @FreekingDean.

[FreekingDean]

Thank you gabsn for taking this on :)

[FreekingDean]

I have thought about this, and I wonder if we should build this into some form of a base handler for each of the router (httprouter/gorilla) packages. I know the main issue is that path routing would be messy as this does not take into account resource based paths: /users/:id in that this would cause a new resource for each user_id: /users/1, /users/2... I think we could alleviate that issue by allowing a path retrieval function passed into a private handler.

I'm not sure if I'm making sense so heres some code:

//net/http/http.go
func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
  ...
  tracedRequest, err := traceRequest(r, GetPath, h.tracer)
  ...
}

func GetPath(r *http.Request) string {
  return r.URL.Path
}

type PathFunc func(r *http.Request) string
func traceRequest(r *http.Request, pathFunc PathFunc, t *tracer.Tracer) (*http.Request, error) {
  ...
  span.SetMeta(ext.HTTPURL, pathFunc(r))
  ...
}

---

//gorilla/muxtrace/muxtrace.go
import(
  ...
  httptrace "github.com/DataDog/dd-trace-go/libs/net/http"
)
func (m *MuxTracer) TraceHandleFunc(handler http.HandlerFunc) http.HandlerFunc {
  ...
  tracedRequest, err := httptrace.traceRequest(r, GetPath, m.tracer)
  ...
}

func GetPath(r *http.Request) string {
  route := mux.CurrentRoute(req)
  path, err := route.GetPathTemplate()
  if err != nil {
    // when route doesn't define a path
    path = "unknown"
  }
  return path
}

I hope this makes sense and I'm not just rambling & incoherent. 😅

[gabsn]

@FreekingDean Yes you're right, with this implementation we end up with a new resource per url hit. I'll do a modification to be sure that 1 resource = 1 route.

[talwai]

call this contrib

[gabsn]

I'll do that in another PR so it will be more clear. (it's a change of +100 files)

[palazzem]

@gabsn probably I'm missing something, but did we merge a broken build? I see that it ended with a failure: https://circleci.com/gh/DataDog/dd-trace-go/677

Indeed, the current master is broken: https://circleci.com/gh/DataDog/dd-trace-go/679

[gabsn]

Yeah the problem is not related with this PR, it's because of this PR on the grpc repo. We need to improve our testing to avoid those kind of issues, because for the moment circleci uses the master branch of the dependencies instead of a pinned version.

[palazzem]

Yea @gabsn, we should definitely improve our testing. I already have a plan about it and will write something down very soon so that we can address also other issues.