adds ClientTracer option to allow usage of httptrace package with ochttp

[basvanbeek]

This adds the option to use the httptrace package with the ochttp RountTripper for more granular annotation of http requests.

This solves #309.

[semistrict]

Don't use embedding here. There's no need to import the whole API surface of Span.

[semistrict]

Also, this doesn't need to be exported does it?

[basvanbeek]

We can lose the embedding but we should keep the export as it allows people to write their own func(*trace.Span) *httptrace.ClientTrace for selecting the set of hooks they want annotated by this ClientTrace.

[semistrict]

I don't understand why this needs to be public. If they want to use only a subset of the hooks, they can simply set the corresponding fields to nil in the returned ClientTrace. Likewise, if they want to use their own ClientTrace and copy across some of the fields, they can do this too by calling methods on ClientTrace directly.

[semistrict]

Call it NewClientTrace, like sync.Pool's New.

Also might want to pass in the http.Request in case they want to specialize based on some attribute of the request.

[basvanbeek]

ok

[semistrict]

I would mention that it adds annotations specifically. Also the word "instance" is a little odd, why not just "returns a httptrace.ClientTrace"?

Second, I would also delegate to the contextual httptrace.ClientTrace (if present) so this doesn't break any other instrumentation they might already have.

[basvanbeek]

Removed the word instance as requested.

Not sure I follow the second remark; the clientTracer uses http.WithClientTrace() to attach itself. This function should take care of playing nice with other instrumentation if I read the description right.

[semistrict]

The name here doesn't add much. Maybe NewSpanAnnotator?

[basvanbeek]

ok

[semistrict]

My point with the name is that this is not the ClientTrace itself - it is a constructor function for the ClientTrace - so I think the name "NewClientTrace" is more appropriate.

[basvanbeek]

NewClientTrace doesn't sit well with me as a property of the Transport struct but I'll admit I see your point on ClientTracer being not great either. I'll use NewClientTrace.

[semistrict]

I don't understand why this needs to be public. If they want to use only a subset of the hooks, they can simply set the corresponding fields to nil in the returned ClientTrace. Likewise, if they want to use their own ClientTrace and copy across some of the fields, they can do this too by calling methods on ClientTrace directly.

[semistrict]

I think we should call the field NewClientTrace instead of ClientTracer

[semistrict]

I don't understand why this needs to be public. If they want to use only a subset of the hooks, they can simply set the corresponding fields to nil in the returned ClientTrace. Likewise, if they want to use their own ClientTrace and copy across some of the fields, they can do this too by calling methods on ClientTrace directly.

[basvanbeek]

I guess it is a matter of opinion on what is the friendlier method for consumers.

Your preference is eliminating the hooks not desired:

func NewSpanAnnotatorWrapper(req *http.Request, s *trace.Span) *httptrace.ClientTrace {
    clientTrace := ochttp.NewSpanAnnotator(req, s)

    // deregister unwanted hooks
    clientTrace.GetConn = nil 
    clientTrace.Got100Continue = nil
    ...
    return clientTrace
}

...

transport := ochttp.Transport{
    NewClientTrace: NewSpanAnnotatorWrapper,
}

Mine is choosing the hooks that are:

func NewSpanAnnotator(req *http.Request, s *trace.Span) *httptrace.ClientTrace {
    spanAnnotator := ochttp.SpanAnnotator{s}

    // register wanted hooks
    return &httptrace.ClientTrace{
        GetConn:              spanAnnotator.PutIdleConn,
        GotFirstResponseByte: spanAnnotator.GotFirstResponseByte,
        DNSStart:             spanAnnotator.DNSStart,
        ...
    }
}

...

transport := ochttp.Transport{
    ClientTracer: NewSpanAnnotator,
}

Which reminds me why I wanted SpanAnnotator to embed *trace.Span or otherwise have it be an exported property. Bear in mind that the actual resulting *httptrace.ClientTrace or SpanAnnotator is created and contained within ochttp's traceTransport.RoundTrip().

[semistrict]

I still don't see why the type needs to be public. You could do this :

t := &ochttp.Transport {
    ClientTracer: func(r *http.Request, s *trace.Span) *httptrace.ClientTrace {
 ann := ochttp.NewSpanAnnotator(r,s)          
 return &httptrace.ClientTrace {
               GetConn: ann.GetConn
          }
    }
}

[semistrict]

Adding a type adds complexity and distraction to the godoc

[basvanbeek]

Yeah I seemed to have forgotten because the function outputs *httptrace.ClientTrace I can always wrap it with a new one. Good thing vacation is coming up for some fresh eyes.

Will not only unexport SpanAnnotator but all its methods can be unexported too.

[rakyll]

We don't often call fields after New* in Go. I'd rather wished this was named ClientTrace. Can we make this modification given there are no users of this yet?

[rakyll]

Not a huge deal if we are not doing it. I also don't think sync.Pool.New is well representative of the overall Go style in these cases.

[semistrict]

Do you have an example from the standard library of something that you think is representative? I couldn't think of anything except that.

I think it's fine to make changes since we haven't created a release yet.