-
Notifications
You must be signed in to change notification settings - Fork 192
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
add httptrace option #788
add httptrace option #788
Conversation
graphaelli
commented
Jul 19, 2020
•
edited
Loading
edited
💔 Build FailedExpand 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
almost there!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cancel that, will test what happens in this case, not clear if gotconn is called again too
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.
Last thing! Then we just need to get to the bottom of the 1.9 failure. Didn't fail on master when I created a PR earlier, so I think it's related to the go.mod changes. There's also a goimports error.
Looks good! We can investigate the 1.9 failure separately... seems unrelated. |
woo, thanks for all of the guidance! |