feat(reporter/http): uses an interface for http client

[jcchavezs]

This PR changes the interface for the http client being accepted in the http reporter which allows users to plug other clients (e.g. a client with resiliency patterns implemented) as in https://gist.github.com/jcchavezs/5d615ff7013311bea1555e448c4cce3e.

Questions to be addressed: Should this gist be part of the zipkin http reporter package?

Ping @basvanbeek @skaslev @pramodramdas

Closes #153

[coveralls]

Coverage increased (+2.7%) to 68.268% when pulling 5bb8a4b on uses_interface_for_http_client into bafd576 on master.

[coveralls]

Coverage decreased (-3.0%) to 72.473% when pulling 32e9ef5 on uses_interface_for_http_client into c29478e on master.

[skaslev]

Looks good to me though I was intending to send a PR with go-retryablehttp baked in the HTTP Reporter.

Questions to be addressed: Should this gist be part of the zipkin http reporter package?

Maybe exposing the gist as RetriableClient reporter option?

func RetriableClient(waitMin, waitMax time.Duration, maxRetries int) ReporterOption {
  // ....
}

[jcchavezs]

Hi @skaslev, my concern about exposing RetriableClient is that we then increase the surface of the API whereas accepting the client interface just provides an abstraction on top of standard library. Maybe that is also the answer for whether include the gist as part of the repo or not.

[skaslev]

Makes sense.

Another idea that comes to mind for the gist is to have the code in the docs/FAQ/examples so users who need retyablehttp will be able to find how to use it with zipkin-go easily.

[jcchavezs]

This change was the only piece I was not sure about but I think it is OK to do this change for two reasons:

1. From the documentation about the http.Client.Timeout:

// The Client cancels requests to the underlying Transport
// as if the Request's Context ended.
// ...
Timeout time.Duration

Which means that this produces the same effect as a context cancellation. What I am not sure if we should return the context.DeadlineExceeded error or a meaningful timeout error.

2. I did some benchmarking and I concluded there won't be a big impact on this change (actually context deadline seems to perform better):

BenchmarkClientTimeoutTimesout-8   	     100	  11147329 ns/op	   17279 B/op	     119 allocs/op
BenchmarkCtxDeadlineTimesout-8     	     100	  10803295 ns/op	   15984 B/op	     101 allocs/op
BenchmarkClientTimeoutSuccess-8    	    5409	    219080 ns/op	    4462 B/op	      59 allocs/op
BenchmarkCtxDeadlineSuccess-8      	    5875	    212182 ns/op	    3747 B/op	      48 allocs/op

So I think this can be merged. Any concern @basvanbeek ?

[basvanbeek]

Two minor nits otherwise LGTM

[basvanbeek]

Thanks @jcchavezs!!!