add httptrace option #788
add httptrace option #788
Conversation
graphaelli
commented
Jul 19, 2020
•
graphaelli
commented
💔 Build Failed
Expand to view the summary
Build stats
Test stats 🧪
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
There was a problem hiding this comment.
Looking good, thanks for this!
I think we should probably follow https://w3c.github.io/resource-timing/#processing-model for the span names, which is what we did for RUM breakdown metrics. Possible exception for separating TCP into TLS/Connect. Also, I'm not super keen on things like "Server Processing", which assumes too much knowledge on the part of the client. Better to instrument the server-side for for that.
WDYT about this?
- DNS: DNSDone - DNSStart
- Connect: ConnectDone - ConnectStart
- TLS: TLSHandshakeDone - TLSHandshakeStart
- Request: WroteRequest - GotConn
- Response: (response end) - GotFirstResponseByte
|
Thanks for the review, i am still planning to follow up on this! |
and including hostname in DNS lookup span name
and make a request tracer per roundtrip
|
|
per httptrace docs: // ConnectStart is called when a new connection's Dial begins. // If net.Dialer.DualStack (IPv6 "Happy Eyeballs") support is // enabled, this may be called multiple times.
Co-authored-by: Andrew Wilkins <axwalk@gmail.com>
|
@axw I don't understand the go 1.9 test failure, this is ready for review otherwise. |
module/apmhttp/clienttrace.go
Outdated
| }, | ||
|
|
||
| WroteRequest: func(info httptrace.WroteRequestInfo) { | ||
| r.Request = tx.StartSpan("Request", "http.request", parent) |
There was a problem hiding this comment.
Isn't this when we want to end the request span? I think we want to start it with GotConn
There was a problem hiding this comment.
You're right, this was not translated properly when converting it from Server Processing. Looking at this again and seeing:
// WroteRequest is called with the result of writing the
// request and any body. It may be called multiple times
// in the case of retried requests.
I think we should make multiple request spans in case of retries, so if r.Request != nil r.Request.End() - objections?
There was a problem hiding this comment.
Cancel that, will test what happens in this case, not clear if gotconn is called again too
There was a problem hiding this comment.
Now that the request span doesn't cover the server response time I'm missing that span representing the server time
I follow that a distributed trace into your own systems would show up there but this makes it non-trivial to monitor time in external services - any ideas here? Maybe an option for WithClientTrace to create spans for that external time? Is this not actually an issue?
There was a problem hiding this comment.
I follow that a distributed trace into your own systems would show up there but this makes it non-trivial to monitor time in external services - any ideas here?
Fair point, I was really thinking about wholly-owned distributed systems.
Going back to https://w3c.github.io/resource-timing/#processing-model, "Request" is actually responseStart - requestStart. How about we do the same? GotFirstResponseByte - GotConn? If it's really interesting to know when the client finished writing the request, we could add "marks" like the RUM agent does.
Co-authored-by: Andrew Wilkins <axwalk@gmail.com>
|
Looks good! We can investigate the 1.9 failure separately... seems unrelated. |
|
woo, thanks for all of the guidance! |
