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: redirections make it hard to use the hooks #17152

Closed
rakyll opened this issue Sep 19, 2016 · 9 comments
Closed

net/http/httptrace: redirections make it hard to use the hooks #17152

rakyll opened this issue Sep 19, 2016 · 9 comments

Comments

@rakyll
Copy link
Contributor

@rakyll rakyll commented Sep 19, 2016

Consider the following program:

req, _ := http.NewRequest("GET", "https://google.com", nil)
trace := &httptrace.ClientTrace{
    DNSDone: func(dnsInfo httptrace.DNSDoneInfo) {
        fmt.Printf("DNS Info: %#v\n", dnsInfo)
    },
}
req = req.WithContext(httptrace.WithClientTrace(req.Context(), trace))
http.DefaultClient.Do(req)

https://google.com is redirected to https://www.google.com and making it harder to use the DNS hooks because DNSDoneInfo doesn't contain any information about the host it is trying to resolve.

It is also not documented anywhere that these hooks might be called more than once.

Should we think about a way to report the redirection or include enough details, so the user can determine for which actual request, the hooks are being invoked?

/cc @bradfitz

@rakyll rakyll added this to the Go1.8Maybe milestone Sep 19, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 19, 2016

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

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 19, 2016

Instead of documenting something overly-vague like they can be called more than once, I'd rather say that it's a Transport (RoundTrip) tracing mechanism, rather than a Client (Do) tracing mechanism, and the lifetime is a single request so if you're using Client, you're responsible for your own tracing within the CheckRedirect hook.

/cc @tombergan for thoughts.

@rakyll rakyll modified the milestones: Go1.8, Go1.8Maybe Sep 19, 2016
@rakyll
Copy link
Contributor Author

@rakyll rakyll commented Sep 19, 2016

I like the way you put it, and I will try to update the godoc representing more of the scope of the tracing. It would be nice to demonstrate a case with redirection to document the usage with Client, that will help us making the difference obvious.

The example I recently contributed does a Client.Do which might be misleading. We should, at least, modify it before we cut 1.8.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 19, 2016

The example I recently contributed does a Client.Do which might be misleading. We should, at least, modify it before we cut 1.8.

SGTM. And any other examples/docs until Client if so.

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 20, 2016

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

gopherbot pushed a commit that referenced this issue Sep 20, 2016
Tracing happens at the http.Trace level. Fix the example to demostrate
tracing in the lifecycle of a RoundTrip.

Updates #17152.

Change-Id: Ic7d7bcc550176189206185482e8962dbf1504ff1
Reviewed-on: https://go-review.googlesource.com/29431
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
rakyll added a commit to rakyll/go that referenced this issue Sep 20, 2016
…round trip

Updates golang#17152.

Change-Id: Iad357cae2534978d4def4f5f4a0efa68ae462acb
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 23, 2016

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

gopherbot pushed a commit that referenced this issue Sep 25, 2016
This sentence is partially guilty why httptrace is considered as an
http.Client tracing package. Removing the mention.

Updates #17152.

Change-Id: I69f78a6e10817db933f44e464a949ae896e44ec6
Reviewed-on: https://go-review.googlesource.com/29755
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit to golang/blog that referenced this issue Oct 4, 2016
Updates golang/go#17152.

Change-Id: I08d4174ba62f6dc4028bd17a911b92fc373b81e4
Reviewed-on: https://go-review.googlesource.com/29685
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
@rakyll
Copy link
Contributor Author

@rakyll rakyll commented Oct 25, 2016

I am going to close this bug given we published a blog post how to use the package. I don't think we should try to verbally explain the same points in the godoc. Warning about the ClientTrace is bound to RoundTripper and giving a link to the blog post might be better.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 26, 2016

@rakyll, you said you were going to close this bug but didn't. The rationale sounds fine provided you add a link to the blog post from the httptrace package doc. I don't see the link there now. Thanks.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 26, 2016

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

@gopherbot gopherbot closed this in 864859d Oct 28, 2016
@golang golang locked and limited conversation to collaborators Oct 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.