Add the ability to inject a custom round tripper

[joe-elliott]

We have the need to do some customization of the http request in the Prometheus remote write client:

https://github.com/prometheus/prometheus/blob/cb830b0a9c7819c4784226f08fbac21741254f1d/storage/remote/client.go#L124

This felt like a more appropriate place to add a custom round tripper than in the remote storage client directly. Hoping the maintainers agree. If not I'd be glad to discuss options and PR a different change.

[joe-elliott]

Could you suggest a way? I apologize, but I'm not seeing it.

[roidelapluie]

What are you trying to achieve with this? Do we miss useful options in our client?

[joe-elliott]

I think no matter how much you extend the client someone will have custom needs that can't be captured in a standard configuration. It's good that the base configuration covers normal authorization and other HTTP options, but these changes allow much more flexibility for any future customizations.

In our specific case we need to inject a custom auth header that would make no sense to add to the base options.

[roidelapluie]

I think no matter how much you extend the client someone will have custom needs that can't be captured in a standard configuration. It's good that the base configuration covers normal authorization and other HTTP options, but these changes allow much more flexibility for any future customizations.

The point is that this struct is not for everyone's use, actually this library is Prometheus internal. One of the issues you would face with this change is e.g. you would not be able to re-marshal the struct properly.

If we were to do this it would maybe belong to the NewHTTPClientFrom config method? And then passed in via the remote write?

[roidelapluie]

I'd like @brian-brazil opinion on this :)

[brian-brazil]

The point is that this struct is not for everyone's use, actually this library is Prometheus internal.

Indeed, a change like this complicates things for no benefit for Prometheus - and also as dead code is liable to be removed. From a Prometheus user standpoint, if you want to do something non-standard with the request you can use a HTTP proxy as no part of Prometheus is designed to be able to implement arbitrary protocols on top of HTTP.

I'd suggest getting whatever you're trying to talk to to use the existing standard HTTP configuration options that are provided, as anything remote write is talking to should be designed with remote write in mind.

What is it that you're actually trying to do?

[joe-elliott]

What is it that you're actually trying to do?

We are using the prometheus remote write client to write series to our backend. We need to be able to inject a custom auth header into the remote write requests.

[rfratto]

We are using the prometheus remote write client to write series to our backend. We need to be able to inject a custom auth header into the remote write requests.

I'd also like this for grafana/agent; I'd like to be able to change the User-Agent header to indicate requests coming are from Agent vs Prometheus for tracking purposes.

[roidelapluie]

What is it that you're actually trying to do?

We are using the prometheus remote write client to write series to our backend. We need to be able to inject a custom auth header into the remote write requests.

Why can't you use basic auth?

[joe-elliott]

Why can't you use basic auth?

B/c the backend in question doesn't support basic auth.

[roidelapluie]

Why can't you use basic auth?

B/c the backend in question doesn't support basic auth.

It is specific enough to understand prometheus remote write protocol but not basic auth 🤔

[joe-elliott]

My initial point stands:

I think no matter how much you extend the client someone will have custom needs that can't be captured in a standard configuration.

There's no point in grilling me about the specifics of this backend or why basic auth is not an option. There will always be needs a standard configuration can't handle.

[roidelapluie]

Sorry. I apologize for grilling you.

[gouthamve]

Hey, this came up in the OTel context as well! open-telemetry/opentelemetry-collector#1464 (comment)

But I sadly agree with Julien and Brian here, modifying this struct might be the wrong thing to do. The roundtripper would work for most use-cases, but what if there are some use-cases it can't satisfy (can't think of any right away :)) Can we just add the ability to set a custom Http client via a SetHTTPClient(c *http.Client) method?

Does that idea sound better Julien/Brian? I think as more and more projects adapt remote write, we'll see all of them having weird needs, and I think allowing the setting of a custom client would be a good middleground and even encourage them to use our client instead of rolling their own.

[roidelapluie]

The http client also controls the timeouts, the keepalives, ciphersuites, and many other things. Changing the client would allow to change all that, an I am concerned that, at the end, our client does not have proper defaults or if we do something wrong, someone else would just swap the http client instead of improving ours, and some non trivial issues (like prometheus/prometheus#7588) would be un-noticed on our side.

[joe-elliott]

Allowing a custom HTTPClient would technically work, but then we'd lose access to a lot of code that people would end up just copy/pasting.

If adding a custom round tripper here is not acceptable, perhaps adding it in the remote write client instead of pushing it all the way down to the HTTP client would be? I had initially considered that, but thought this flexibility might be welcome at the lowest level.

[brian-brazil]

As @roidelapluie says if your backend is designed to handle remote write but not any of the various standard HTTP auth mechanisms that Prometheus provides then that's getting a bit niche and out of scope. I politely suggest that you investigate using standard auth mechanisms, and if that doesn't work use a HTTP proxy as a Prometheus user would in the same scenario.

As for this change itself, config structs should only contain user-provided configuration - and not whatever else might be useful at a code level. We'll generally accept changes that have little to no impact (e.g. making the client field public), but we have both our own standard patterns and this is also an internal library.

Changing the client would allow to change all that, an I am concerned that, at the end, our client does not have proper defaults or if we do something wrong, someone else would just swap the http client instead of improving ours,

I agree, this needs to be kept as limited as possible for sanity.

[gouthamve]

Changing the client would allow to change all that, an I am concerned that, at the end, our client does not have proper defaults or if we do something wrong, someone else would just swap the http client instead of improving ours,

This is just for external users of our library, Prometheus would still use the same config and we'll still catch the same bugs as before. But my fear is that if we don't allow this, we won't have any external users at all that will find out we're doing something wrong. I don't see how it can be any worse than the status quo. See OTel for example, I haven't looked at the PRs but they likely rolled their own remote-write client.

[rfratto]

I'm not sure a SetHTTPClient or exposing the Client field would work for my specific needs. My code only interacts with remote_write through calling remote.NewStorage and so I would need something that bubbles all the way up to that level. I'm hesitant to propose a chain reaction of making things public just to access the underlying client for injecting a header (Storage -> WriteStorage -> Queues -> QueueManager.Client -> etc).

Unfortunately, without changing much of the code, it does seem like the PR as presented here provides the most flexible solution for unanticipated use cases.

[roidelapluie]

It turns out that your problem is very different and that 1. Prometheus should set its user agent 2. We should allow the name of the package to belong to the version package as well, then our user agent should take both from that package.

[rfratto]

@roidelapluie I'm happy with that! I'll back out of this PR and follow up with re-proposing your idea into prometheus/prometheus when I'm ready to contribute the change.

[brian-brazil]

I'm hesitant to propose a chain reaction of making things public just to

That's not generally a problem.

It turns out that your problem is very different and that

Or make that one constant public.

[roidelapluie]

Hey, this came up in the OTel context as well! open-telemetry/opentelemetry-collector#1464 (comment)

It does not seem like OTel needs the sharding and buffering features of the Prometheus client anyway.