Skip to content

feat(outputs.wavefront): use common/http to configure http client#14136

Merged
powersj merged 1 commit intoinfluxdata:masterfrom
LukeWinikates:wavefront-http-connection-limits
Oct 23, 2023
Merged

feat(outputs.wavefront): use common/http to configure http client#14136
powersj merged 1 commit intoinfluxdata:masterfrom
LukeWinikates:wavefront-http-connection-limits

Conversation

@LukeWinikates
Copy link
Copy Markdown
Contributor

@LukeWinikates LukeWinikates commented Oct 17, 2023

Required for all PRs

resolves #14090

This PR adopts Telegraf's common/http struct to configure the http.Client used by the Wavefront output plugin. This should be a harmonious way of exposing a variety of HTTP client customizations and tuning parameters, including the idle connection settings requested in #14909.

I removed existing fields for TLS and HTTP Timeout configuration, because the HTTPClientConfig exposes the same fields.

question: I am not sure if the init() function needs to set a default HTTPClientConfig.
note: this is currently using a branch of the Wavefront SDK. I want to get feedback on the PR early. Prior to final review, we (the Wavefront Go SDK maintainers) will release a new SDK version, and I will update this PR accordingly.

@telegraf-tiger telegraf-tiger bot added area/wavefront feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Oct 17, 2023
@LukeWinikates LukeWinikates force-pushed the wavefront-http-connection-limits branch from c5439e7 to bf9926b Compare October 17, 2023 18:25
srebhan
srebhan approved these changes Oct 18, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@srebhan srebhan left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the nice upgrade @LukeWinikates! The code looks good, just two comments/requests from my side:

  1. Please update to the github.com/wavefronthq/wavefront-sdk-go v0.14.1 package as soon as it is released. IIRC dependabot will not touch git-versions so we need to do this manually...
  2. We now do have the possibility to include other configs in the sample.conf by creating a sample.conf.in and call the generator in the plugin file. Checkout the http input plugin for an example on TLS parameters... It would be nice if we could include those parameters here to present all options to the user... This can be a separate PR though!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Oct 18, 2023
powersj
powersj reviewed Oct 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thank you! I do want to see the docs and sample config updated as a part of the final PR please.

I was not planning on landing given your comment about doing a final release of the client library. Do you still want to hold off?

Thanks again!

@LukeWinikates
Copy link
Copy Markdown
Contributor Author

LukeWinikates commented Oct 18, 2023

@srebhan @powersj thanks for the fast feedback!

@powersj yes, I would like to wait to merge this until we ship an updated Wavefront SDK release with the new configuration options exposed.

  • I will get a new Wavefront SDK version released and update the PR with a proper released version
  • I will update the docs and sample config.

I will circle back after that. Thanks again!

@LukeWinikates LukeWinikates force-pushed the wavefront-http-connection-limits branch from bf9926b to b87c487 Compare October 19, 2023 22:28
@LukeWinikates
Copy link
Copy Markdown
Contributor Author

LukeWinikates commented Oct 19, 2023

@srebhan @powersj just updated this branch to use the Wavefront SDK version released today, plus an update to the sample config.

Looking at @srebhan's comment about config file includes, I considered the possibility of extracting a config template that covers TLS options and idle connection settings, but I didn't take the plunge yet. The least complicated option would be to make a template that covers all of the httpconfig.HTTPClientConfig fields, using the content from the HTTP output plugin. Then everything that uses httpconfig.HTTPClientConfig could embed that too.

I like the idea of having a single source of truth about all the configuration options for httpconfig.HTTPClientConfig. I'd like to keep it out of scope for this PR.

@telegraf-tiger
Copy link
Copy Markdown
Contributor

telegraf-tiger bot commented Oct 19, 2023

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_arm64.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz windows_i386.zip
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz

@srebhan srebhan requested a review from powersj October 23, 2023 08:16
powersj
powersj approved these changes Oct 23, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thank you!

@powersj powersj merged commit 3eed69f into influxdata:master Oct 23, 2023
@powersj powersj added this to the v1.29.0 milestone Oct 23, 2023
@Porkepix Porkepix mentioned this pull request Dec 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srebhan srebhan srebhan approved these changes

+1 more reviewer

@powersj powersj powersj approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@powersj powersj

Labels

area/wavefront feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.

Projects

None yet

Milestone

v1.29.0

Development

Successfully merging this pull request may close these issues.

Limit the number of HTTP connection using outputs.wavefront

3 participants

@LukeWinikates @powersj @srebhan