Skip to content
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

net/http/httptrace: add hooks for errors and response body closed #16400

Closed
dominikh opened this issue Jul 17, 2016 · 8 comments
Closed

net/http/httptrace: add hooks for errors and response body closed #16400

dominikh opened this issue Jul 17, 2016 · 8 comments

Comments

@dominikh
Copy link
Member

The current version of httptrace allows tracking most states of a request's live cycle, except for an overall error (all dials failed or protocol errors) and the closing of the response body. Both of these things can already be tracked by writing a custom RoundTripper, however if httptrace had hooks for these things, it would allow for the full instrumentation (e.g. metrics and tracing) of requests without needing to split it up between two mechanisms.

/cc @bradfitz

@bradfitz bradfitz added this to the Go1.8Maybe milestone Jul 17, 2016
@msterle
Copy link

msterle commented Aug 5, 2016

I also submitted a WIP for an error hook at https://go-review.googlesource.com/#/c/25544/.

This is my first contribution to the golang project, so please let me know if I should do anything differently in the future, or if you have any tips.

@gopherbot
Copy link

CL https://golang.org/cl/25544 mentions this issue.

@bradfitz
Copy link
Contributor

bradfitz commented Sep 8, 2016

@dominikh, if you're interested in this, please help do the code review and write the docs for what the hook would look like. See https://golang.org/cl/25544

@dominikh
Copy link
Member Author

@bradfitz Here are the docs for the new hooks the way I imagined them. Let me know if you agree/disagree with me.

type ClientTrace struct {
    // GetConnFailed is called when no connection could be obtained.
    // It will be called at most once per request. The individual Dial
    // errors will be reported by calls to ConnectDone.
    GetConnFailed func()

    // BodyClosed is called after the response body has been closed.
    // If err is nil, no read or close error occured. If err is
    // non-nil, it contains the first error that was returned by Read
    // or Close.
    BodyClosed func(err error)
}

This should allow complete instrumentation of a request, without needing a custom RoundTripper or user-code after Close. If the request couldn't be made at all, either GetConnFailed or WroteRequest with a non-nil Err in WroteRequestInfo will be called. If the request could be made, but an error occured while reading from the body, BodyClosed will be called with a non-nil error. If no errors occured, BodyClosed will be called with a nil error.

@dominikh
Copy link
Member Author

I'm not sure what to do with errors such as "nil Request.URL" or "nil Request.Header" though. I would be tempted to report these via the WroteRequestInfo hook, too. Alternatively, a third hook would need to be added. Ideas?

@dominikh
Copy link
Member Author

I'll withdraw my feature request. It would need to add tracing to the Client itself, while not actually solving any problems. My original intention was to avoid implementing a custom RoundTripper. However, even with these new hooks, a RoundTripper may be necessary to handle redirects correctly. And once one has a custom RoundTripper, the missing hooks can be implemented manually.

@gopherbot
Copy link

CL https://golang.org/cl/25510 mentions this issue.

@bradfitz
Copy link
Contributor

Closing per @dominikh's comment. Happy to revisit this in the future after people have more experience with httptrace and its features & shortcomings. It was designed to be extensible, so I'm happy to extend it later.

@golang golang locked and limited conversation to collaborators Sep 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants