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

proposal: net/http: Transport tracing hooks #56241

Open
krishna15898 opened this issue Oct 14, 2022 · 3 comments
Open

proposal: net/http: Transport tracing hooks #56241

krishna15898 opened this issue Oct 14, 2022 · 3 comments
Labels
Milestone

Comments

@krishna15898
Copy link

krishna15898 commented Oct 14, 2022

The current HTTP client instrumentation library httptrace tracks the life cycle of a single request with hooks present at different stages in the outgoing HTTP Request's journey. This limits the use of httptrace in many ways which we believe could be solved by having an instrumentation library which works at a “transport level”. This means instead of placing hooks in the request’s context, one defines these hooks for the Transport. This would also support the current functionality of httptrace.

Proposal

Here is a basic example of the proposed change. Let’s take the example of the GotConn hook in httptrace.ClientTrace. The current method for adding and using this hook is

req, _ := http.NewRequest("GET", "http://example.com", nil)
trace := &httptrace.ClientTrace{
	GotConn: func(connInfo httptrace.GotConnInfo) {
		fmt.Printf("Got Conn: %+v\n", connInfo)
	},
}
req = req.WithContext(httptrace.WithClientTrace(req.Context(), trace))
resp, _ := http.DefaultClient.Do(req)

We propose restructuring http.Transport to contain the hooks instead of ClientTrace. Transport contains a new struct Stats which contains the hooks and other data like counts of idle/active connections and waiting requests.

type Stats struct {
	GotConn func(connInfo GotConnInfo)
        
	// among other hooks and data
}

type Transport struct {
	Stats *Stats
	// and all other fields to stay the same
}

And one would make a request in the following way:-

req, _ := http.NewRequest("GET", "http://example.com", nil)
stats := &http.Stats{
	GotConn: func(connInfo http.GotConnInfo) {
		fmt.Printf("Got Conn: %+v\n", connInfo)
	},
}
transport := http.Transport{Stats: stats}
client := http.Client{Transport: transport}
resp, _ := client.Do(req)

Within Transport, when a request has received a connection, the GotConn hook would run as below. This is in contrast to extracting ClientTrace from the Request’s context and using its hooks.

func (t *Transport) methodName() {
    // a connection has been delivered to the request
    if t.Stats.GotConn != nil {
	  t.Stats.GotConn(GotConnInfo{…})
    }
…
}

The above basic proposal can be extended to all the hooks already present in httptrace. We propose the following additional hooks to be added to http.Stats above:-

  1. IdleConnWaitIn, IdleConnWaitOut, ConnsPerHostWaitIn, and ConnsPerHostWaitOut - These hooks will be called when a request (wantConn) enters/exits the idleConnWait and connsPerHostWait queues. For example -
type IdleConnWaitInInfo struct {
    ConnectMethodKey string
    EventTime time.Time
    RemainingInQueue int
}
IdleConnWaitIn(info IdleConnWaitInInfo)

These hooks could take as parameters connectMethodKey of the queue, time of entry/exit, and the number of remaining active wantConns in the queues after the event (an active wantConnis one which has not been delivered a persistConn yet and its context has not exceeded its deadline. Note that this is not equal to the number of wantConns in the queues as some of them could have been delivered a persistConn in another goroutine or their context could have exceeded its deadline.) The IdleConnWaitOut and ConnsPerHostWaitOut hooks could also have an extra parameter error which is nil if the wantConns were removed from the queues because they were successfully delivered a persistConn. If not, error gives the reason for their removal from the queues, for example, if the request was cancelled or its context deadline expired.

  1. IdleConnIn, IdleConnOut - IdleConnIn serves the same purpose as the existing PutIdleConn hook from the httptrace package. IdleConnOut is the related hook for when a connection is removed from the idleConn queue. These hooks could take as parameters connectMethodKey of the queue and time of entry/exit. IdleConnOut could take an additional parameter error which is nil if the idle connection was delivered to a wantConn. If not, it represents the reason it was removed from the queue, for example, being too old to be reused.

  2. ConnReused - This hook is called after serving a request, if the connection is directly delivered to a wantConn waiting in idleConnWait instead of being added to the idleConn queue. Currently, there is only one hook related to connections, PutIdleConn, which records the latter. ConnReused could take as parameters connectMethodKey of the persistConn (cacheKey field) and time of delivery.

  3. MaxIdleConnsHit, MaxIdleConnsPerHost, MaxConnsPerHostHit - When configured capacities of queues like MaxIdleConns, MaxIdleConnsPerHost, MaxConnsPerHost are hit, these hooks should be able to take a "snapshot of the state" of the HTTP Client. They could take as parameters the counts of active wantConns in idleConnWait and connsPerHostWait queues for each connectMethodKey and the length of idleConn queues for each connectMethodKey. They could also take as parameters the state of all active connections like their TCP connection state( this is an optional feature for which we may have to make external calls). These hooks are useful for the user to understand where their server's bottlenecks or the server they are connecting to.

Note: I have already implemented this during an internship.

@gopherbot gopherbot added this to the Proposal milestone Oct 14, 2022
@seankhliao
Copy link
Member

seankhliao commented Oct 16, 2022

cc @neild

@seankhliao seankhliao changed the title proposal: net/http: HTTP Client Instrumentation proposal: net/http: Transport tracing hooks Oct 18, 2022
@neild
Copy link
Contributor

neild commented Oct 22, 2022

This proposal is for two unrelated things: A way to run per-request trace hooks for every request on an http.Transport, and a new set of per-request trace hooks that seem largely redundant with httptrace.ClientTrace.

You need to separate these parts. Do you want to propose a way to run ClientTrace hooks for every request on a connection? Or new ClientTrace hooks? Or hooks which aren't associated with requests at all?

We don't want to have two entirely different per-request tracing mechanisms. Any new tracing hooks need to fit in with what exists already.

@krishna15898
Copy link
Author

krishna15898 commented Oct 26, 2022

Correct, this proposal was to introduce trace hooks that would run for all request (and connection) activities on an http.Transport. The hooks I have mentioned above are the new ones that are not already present in httptrace.ClientTrace but the proposal intends to continue the use of already existing per-request hooks in ClientTrace as well.

So basically I want to propose a way to run ClientTrace + other new hooks for all activities of the http.Transport with a single mechanism. These activities could be related to requests (as in existing hooks of ClientTrace) or connections (not supported by ClientTrace).

But I guess it is not possible to replace httptrace with the new mechanism as it is a part of the go library. And adding the remaining new hooks with an entirely new mechanism would leave us with two different mechanisms as you mentioned.

Parts of the proposal that still fits into what exists already are per-request hooks that can be added to ClientTrace which are in point 1. in the original post, i.e. IdleConnWaitIn, IdleConnWaitOut, ConnsPerHostWaitIn, and ConnsPerHostWaitOut which would

  1. Add more information about the stages of a request's journey before it gets a connection - what were the blockers for the request from getting a connection right away
  2. If the request does not get a connection, would give information as to why - whether its context exceeded its deadline or the request was canceled, etc. Currently, if a request does not get a connection, one needs to use the error from Transport.RoundTrip

PS. This is not a part of this proposal but the above hooks would also enable us to track the request's waiting time in i) getting a connection, ii) idleConnWait, and iii) connsPerHostWait. This could be done by adding a field entryTime to wantConn similar to persistConn.idleAt which gives the information of GotConnInfo.IdleTime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants