-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(inputs.prometheus): use system wide proxy settings #11729
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Thanks for taking the time to put up a PR.
In Telegraf, we already have a standard HTTP client config that allows users to set the proxy as a part of their configuration. It would be good to use the same behavior across plugins.
Take a look at the HTTP plugin in how it creates a client with the additional httpconfig.HTTPClientConfig
.
As well, we would to add the configuration options for the proxy to the plugin configuration.
Hi, could you already review my changes? or anything else missing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erwiese, your change looks a lot better this way. Thanks for updating the PR! I have one request left to avoid recreating the client
over and over. Can you please take a look?
Shouldn't the unix socket client stuff also be added to Init in this case? It is also making a new client on each interval + URL. Both (http & unix) can be placed in the same Edit: be careful, It seems there is possibility of mixed types of URLs, some can be http and/or unix.. |
@Hipska someone could use unix and http URLs in one call, so I will not touch it. response_time: yes but breaking and not matter of this issue. |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 👍 This pull request doesn't change the Telegraf binary size 📦 Click here to get additional PR build artifactsArtifact URLs |
resolves #11728