Added net/http-tracer #84
Conversation
|
Hmm the failure in here seems like is related to elasticsearch? Is this a known testing issue? |
|
|
||
| return func(writer http.ResponseWriter, req *http.Request) { | ||
|
|
||
| // bail our if tracing isn't enabled. |
|
@furmmon Is this something that would be worth using or should I fork? |
|
By fork I meant to say put this into its own package *** |
|
|
||
| // span will create a span for the given request. | ||
| func (h *HttpTracer) trace(req *http.Request) (*http.Request, *tracer.Span) { | ||
| resource := req.Method + " " + req.URL.Path |
There was a problem hiding this comment.
This will create a unique resource for each new path, which can become problematic as soon as you have path parameters (eg. "/article/:id") in some routes, as is the case for most APIs nowadays.
There was a problem hiding this comment.
I think you are correct, but is there a way to do that with the default go handler? I'm not sure Go's net/http package natively supports pattern matching the way httprouter and gorilla/mux do.
There was a problem hiding this comment.
AFAIK, net/http doesn't implement pattern matching and named path parameters as most 3rd party go routers do. I therefore think that with the current go ecosystem, we need one integration per HTTP router (cf. my current PR to integrate github.com/julienschmidt/httprouter, or any of the other router integrations), your implementation would still work for the default http mux, but not as a generic go http integration.
There was a problem hiding this comment.
That makes sense. This is not as much a replacement/catchall then, and instead just something that works with the base net/http functions. I wonder if we should use this from the other two to build the span & have a resource builder func that can build the pattern for the endpoint.
\braindump
There was a problem hiding this comment.
Hello, thank you for that PR FreekingDean. I actually found a nice pattern matching function in net/url. You can try this on the playground:
package main
import (
"fmt"
"net/url"
)
func main() {
u, _ := url.Parse("/books?author=:author&subject=")
fmt.Println(u.Path)
// /books
u, _ = url.Parse("/search?q=net%2Furl&rlz=1C5CHFA_enUS730US731&oq=net%2Furl&aqs=chrome..69i57j69i58j69i61l3j69i59.1391j0j7&sourceid=chrome&ie=UTF-8#q=golang+parse+url+path")
fmt.Println(u.Path)
// /search
}
There was a problem hiding this comment.
Thanks @furmmon! That is awesome. I think the main issue would be resource pattern matching when a parameter is in the path: www.api.com/apps/:app_id/get_resource_info In this case the methods I provide would not be sufficient to pull out the relevant path and instead create a new resource for each call to that endpoint:
/apps/1/get_resource_info
/apps/2/get_resource_info
/apps/3/get_resource_info
In this case I think it may be best to have path be absolute in this base case (IE the way I described) but also allow individual libraries to supply the path. (For use with gomux & httprouter)
There was a problem hiding this comment.
edit - comment outdated
There was a problem hiding this comment.
Thank you @FreekingDean for that Pull Request. Nice work, I've added some little comments. Could you also rebase on the current prod branch, it should pass the test.
| func Example() { | ||
| mux := http.NewServeMux() | ||
| mux.Handle("/users", handler) | ||
| mux.Handle("/anything", handler) |
There was a problem hiding this comment.
handler can't be used as a http.Handler, use http.HandlerFunc(handler) it will create an http.Handler struct
| return r | ||
| } | ||
|
|
||
| ctx := tracer.ContextWithSpan(r.Context(), span) |
There was a problem hiding this comment.
This way of setting the span into the context is deprecated, we changed to
ctx := span.Context(r.Context())
|
|
||
| // span will create a span for the given request. | ||
| func (h *HttpTracer) trace(req *http.Request) (*http.Request, *tracer.Span) { | ||
| resource := req.Method + " " + req.URL.Path |
There was a problem hiding this comment.
edit - comment outdated
|
closing in favor of #102 |
…ataDog#84) * uds: chage Mutex to RWMutex for fast already-connected path PR DataDog#62 added in a `Mutex` around the Unix socket writer's `Write` method, to protect the logic that conditionally connected the socket if there was not one already. However, because of the lock placement, it serialises all statsd writes globally, which is likely to be a performance bottleneck for highly parallel systems. This commit turns that Mutex into a `RWMutex`. In the case where there is already a socket set, only a `RLock` is needed. In the case where the socket needs to be connected, the a full `Lock` is performed to protect the swapping of `w.conn`. Note that this change, I believe, will make `SetWriteTimeout` meaningless; multiple goroutines will call `conn.SetWriteDeadline` in parallel, and the behaviour of that is to extend the deadline for future _and all in-flight_ requests. If new calls to `SetWriteDeadline` keep getting made, the deadline on an existing blocked call to `Write` will be continuously extended. I'm not really sure how to fix this or whether it's worth fixing though. * [rebase] moving uds to uds_blocking for rebase * [uds][blocking] fixing bad struct name after rebase
This adds a generic handler for a net/http server.
I think it may be good to have gorilla use this as the core tracer in the future.