Added functional option to allow to customize DialContext() in HTTP client #291
Conversation
…lient Signed-off-by: Marco Pracucci <[email protected]>
|
@beorn7 WDYT about this alternative approach? |
config/http_config.go
Outdated
| // NewClient returns a http.Client using the specified http.RoundTripper. | ||
| func newClient(rt http.RoundTripper) *http.Client { | ||
| return &http.Client{Transport: rt} | ||
| } | ||
|
|
||
| // NewClientFromConfig returns a new HTTP client configured for the | ||
| // given config.HTTPClientConfig. The name is used as go-conntrack metric label. | ||
| func NewClientFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives, enableHTTP2 bool) (*http.Client, error) { | ||
| rt, err := NewRoundTripperFromConfig(cfg, name, disableKeepAlives, enableHTTP2) | ||
| func NewClientFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives, enableHTTP2 bool, optFuncs ...HTTPClientOption) (*http.Client, error) { |
There was a problem hiding this comment.
If this design makes sense to you, I would move disableKeepAlives and enableHTTP2 to functional options too.
There was a problem hiding this comment.
@roidelapluie I moved them to functional options. The default I've picked is both keepalive and HTTP2 enabled. To make it more explicit, the function to change these options allow only to disable it. WDYT?
|
|
||
| var dialContext func(ctx context.Context, network, addr string) (net.Conn, error) | ||
|
|
||
| if opts.dialContextFunc != nil { |
There was a problem hiding this comment.
Can we build up a list and append to the list when needed, to avoid the repetition?
There was a problem hiding this comment.
I tried that in first place but the data type is not exported. Not sure how I could do it.
There was a problem hiding this comment.
Which data type should have the slice?
Signed-off-by: Marco Pracucci <[email protected]>
|
@roidelapluie I understand this, but the |
|
It is an interface we can replicate I think, it should work |
@roidelapluie I'm sorry to bother you but I can't understand how we can replicate it. The interface |
|
Oh sorry, you are right. I will give this a final review later on. |
Thanks. If this change will be accepted, I will then open a PR to conntrack to propose to expose |
* Update to latest modules. * Use new syntax for http client[0]. [0]: prometheus/common#291 Signed-off-by: SuperQ <[email protected]>
In Cortex we have a use case where we would need to customize the dialer used by the HTTP client in the upstream Prometheus alertmanager. Specifically, we're trying to customise the dialer used by HTTP client used by Alertmanager notifiers (eg. webhook). Unfortunately we can't do it without changing it upstream so, in this PR, I'm proposing an alternative option to #290.
This PR is the base for the Alertmanager changes proposed in prometheus/alertmanager#2547.