Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

feat: add an option to set the http.Client for the Sender#171

Merged
LukeWinikates merged 1 commit intomainfrom
http-client-option
Oct 18, 2023
Merged

feat: add an option to set the http.Client for the Sender#171
LukeWinikates merged 1 commit intomainfrom
http-client-option

Conversation

@LukeWinikates
Copy link
Copy Markdown
Contributor

@LukeWinikates LukeWinikates commented Oct 17, 2023

related to influxdata/telegraf#14090

This PR introduces a new Option, HTTPClient, allowing users to override the *http.Client used by the Sender.

This allows a variety of customization options, such as proxy settings, idle connection management, and TLS Options. It is intended to integrate well with Telegraf's http config, which can generate an http.Client from a standard toml configuration (https://github.com/influxdata/telegraf/blob/master/plugins/common/http/config.go)

This PR reimplements the existing Timeout and TLSConfig options in terms of configuration.HTTPClient, and eliminates the internal NewClient factory.

feedback request: the PR logs a warning if you try to use more than one of Timeout, TLSConfig, and HTTPClient for the same Sender, because subsequent calls will clobber the first call. Curious if anyone thinks this should work differently.

I had a change of heart about my initial solution to mixing Timeout, TLSConfig, and HTTPClient. I just pushed a second commit with a different solution. This allows Timeout and TLSConfig to be used together, but prints a warning if either are used after HTTPClient. The intended logic now is that if you set an HTTPClient, the Sender will just use that, but you can alternatively provide either, both, or neither of Timeout and TLSConfigOptions as easier shorthand methods for changing those two settings. In that case, we'll make an http.Client for you.

@LukeWinikates LukeWinikates force-pushed the http-client-option branch 2 times, most recently from d6d3584 to 4a32f84 Compare October 17, 2023 17:51
Copy link
Copy Markdown

@jbooherl jbooherl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the changes made. Was going to make very similar comments to how you changed things!

Copy link
Copy Markdown
Contributor

@jessepye jessepye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@LukeWinikates LukeWinikates merged commit 648ea1f into main Oct 18, 2023
@LukeWinikates LukeWinikates deleted the http-client-option branch October 18, 2023 18:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants