change api http.client to interface

[tsipo]

Hi there,
The current implementation uses http.Client
The only function which is called for it is Do(req *http.Request) (*http.Response, error)
It is desirable to allow other Client implementations, e.g. github.com/hashicorp/go-retryablehttp/Client which allows retries on various HTTP status codes like 429 (it has its own Request struct but it's easy to wrap it in another struct that will implement the above-mentioned interface). Many commercial solutions implementing Prometheus API (e.g. Grafana Cloud) have rate limiting. Thank you.

[ArthurSens]

Sounds like a valid use case :)

Could you sign your commit to make CI happy?

[tsipo]

@ArthurSens Apologies, I just saw the CI complaint and did that :-)

[tsipo]

Hello all, can anyone please update on the status of this one? Thanks.

[ArthurSens]

LGTM

I believe the only way we can break this is if we end up adding new methods to the HttpClient interface. I don't think we'll need anything more than Do() in the near future though 🤔

I'll keep this open for another week just in case @kakkoyun or @bwplotka want to express a different opinion

[tsipo]

LGTM

I believe the only way we can break this is if we end up adding new methods to the HttpClient interface. I don't think we'll need anything more than Do() in the near future though 🤔

I'll keep this open for another week just in case @kakkoyun or @bwplotka want to express a different opinion

Thank you @ArthurSens.

FWIW I have tested it recently quite a bit using the commit to densify-dev/retryable-prometheus-client (referred to above) and the sample code I have included in my comment on @kakkoyun 's requested change. It works just fine.

In case you need to add new functions to the HttpClient interface you are most welcome to ask for my assistance with it, shouldn't be too difficult 😄 .

[bwplotka]

Why not using simply http.RoundTripper?

[bwplotka]

Also if anything this is not following style guide (should be HTTPClient) and does not have commentary as public interface

[bwplotka]

It's bit different method name, but still.. RoundTrip(*Request) (*Response, error)

In theory the api/http packages are experimental so we can change them with breaking changes.

[bwplotka]

But even with all of this the use case mentioned was possible without this change I believe:

There is even documentation how to do it here: github.com/hashicorp/go-retryablehttp/Client

[tsipo]

Hi @bwplotka , thanks for your comments.

Your suggested solution (sorry, somehow I missed that in hashicorp's docs) to use their StandardClient() converter to the library's http.Client works - I have just tried that out and saw retries. So the use-case is possible without this change.

In view of that, you can decide. I still don't think that adding the interface is a bad idea as this is what the Prometheus client requires (only the Do() function). It's true that hashicorp's client gives you a converter to the standard client, but you are now relying on that. I'll leave it up to you.

[bwplotka]

Hm, I think we could revert this. Additional interface is not needed, http.Client has RoundTripper which is replaceable. Plus the mentioned hashicorp project has a converter to http.Client: https://github.com/hashicorp/go-retryablehttp?tab=readme-ov-file#getting-a-stdlib-httpclient-with-retries

[bwplotka]

But even with all of this the use case mentioned was possible without this change I believe:

There is even documentation how to do it here: github.com/hashicorp/go-retryablehttp/Client

[bwplotka]

It's easy to miss but it's very common to create a new http (std Go) Client with Roundtripper which has the same signature as Do (just different name):

&http.Client{Transport: <round tripper with `RoundTrip(*Request) (*Response, error)` method>}

[ArthurSens]

Thanks Bartek for taking a deeper look at this, and apologies for also missing that the use case was already possible 😅

I agree that having multiple ways for doing the same thing confusing, let's revert it 👍

[tsipo]

Thank you @bwplotka and @ArthurSens for your comments.
I think the main reason is confusion / ambiguity in the design decisions of Go HTTP client and the way these are used, in general and in this repository too.

It's true - as @bwplotka pointed out - that you can easily wrap a fully-fledged Client struct with a RoundTripper which delegates its RoundTrip() function to the Client's Do() function, and then use that RoundTripper as a new Client's Transport. That's exactly what hashicorp's StandardClient() function does BTW.

The question is - and that is maybe a bit philosophical - is that the right thing to do?

The documentation of RoundTripper.RoundTrip() says (l. 118-119):
// RoundTrip executes a single HTTP transaction, returning
// a Response for the provided Request.
Moreover, its documentation states
For control over HTTP client headers, redirect policy, and other settings, create a Client
and
For control over proxies, TLS configuration, keep-alives, compression, and other settings, create a Transport

In practice we see many cases, including in this repo, where a RoundTripper is used to set HTTP headers (e.g. various authz headers).
The retry RoundTripper takes it one step further. Should it still be considered a single HTTP transaction? From the PoV of the user probably yes, as they don't care the Client may need to retry till they get the final response. But from the PoV of the Client itself? It may be multiple HTTP requests, after each one the response is drained before the retry takes place. Question of transactions and meta-transactions, I guess.

Merry Christmas!

[bwplotka]

Oh yea, I agree there might be something to improve in Go, even if it's just documentation. I read the documentation you referred differently (maybe you are right)--I read it as:

For control over HTTP client headers, redirect policy, and other settings, create a Client

Those are the things to supply via request with configurable headers and some specific implementation of Client settings. This is what you can configure on client and this is the "source" of settings.

For control over proxies, TLS configuration, keep-alives, compression, and other settings, create a Transport

This is what you can configure with specific implementation of transport.

However... we don't talk about any std mechanisms here -- we are extending the client and transport and here. I think this part of docs does not mention extensibility topic at all, only what's in std types.

The extensibility is done via RoundTripper. Historically it was used for much more than in the docs, perhaps overused, but it allowed huge innovation e.g. client side load balancing and other crazy things (e.g. hashicorp retries).

Plus if you look on http.Client implementation it only checks http.Request request, sets std headers, sets deadlines 12, calls RoundTripper and handles response, redirects.

Thus, generally it makes no point to extend client by interfacing Do(...)--in fact it will also limit extensibility as RoundTrippers are commonly chained, and http.Client creates an amazing starting point of that chain via Do, so RoundTrippers can be chained. (:

Merry Christmas!