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

Jump to bottom

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
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8

Comments

@rakyll
Copy link
Contributor

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

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

@bradfitz
Copy link
Contributor

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 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

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.

@rakyll rakyll mentioned this issue Sep 19, 2016
Open
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Sep 20, 2016
@rakyll
net/http/httptrace: fix bad tracing example
f964810 
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
@rakyll
net/http/httptrace: note that ClientTrace hooks are invoked at every …
ccba151 
…round trip

Updates golang#17152.

Change-Id: Iad357cae2534978d4def4f5f4a0efa68ae462acb
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Sep 25, 2016
@rakyll @bradfitz
net/http/httptrace: remove the mention of http client
893edc9 
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
@rakyll
content: add introducing http tracing post
cc9ef23 
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>
@quentinmit quentinmit added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Oct 7, 2016
@rakyll
Copy link
Contributor Author

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 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

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

@tombergan tombergan mentioned this issue Dec 14, 2016
Open
@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.
Assignees
No one assigned
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
5 participants
@bradfitz @rsc @rakyll @quentinmit @gopherbot