Skip to content
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: add proxy support for outputs/cloudwatch (#11388) #11399

Merged
merged 4 commits into from Jul 13, 2022
Merged

feat: add proxy support for outputs/cloudwatch (#11388) #11399

merged 4 commits into from Jul 13, 2022

Conversation

scholz
Copy link
Contributor

@scholz scholz commented Jun 28, 2022

Required for all PRs:

resolves #11388

Used proxy code from inputs/cloudwatch to add proxy to outputs cloudwatch.
Did extensive (manual) testing to confirm functionality; will add details to issue.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jun 28, 2022
@scholz scholz changed the title feat: add proxy support for inputs/cloudwatch (#11388) feat: add proxy support for outputs/cloudwatch (#11388) Jun 28, 2022
@srebhan
Copy link
Contributor

srebhan commented Jun 29, 2022

@scholz thanks for looking into this! Would it make sense to completely switch to common/HTTPClientConfig to create the client? This way you could save even more code...

@srebhan srebhan self-assigned this Jun 29, 2022
@srebhan srebhan added the cloud Issues or requests around cloud environments label Jun 29, 2022
@scholz
Copy link
Contributor Author

scholz commented Jun 29, 2022

@srebhan : thanks for the suggestion. I will take another look at it asap.

@scholz
Copy link
Contributor Author

scholz commented Jun 30, 2022

Ok I removed some code and switched to httpconfig. As said in the issue I am new to go so bear with me for any oddities I may have inserted.

Also note that it seems that now timeouts only happen in the httpclient create context and therefore errors looks different (and I think less informative) than before:

previously when the proxy host wasnt responding the error was this:
2022-06-28T13:00:12.175280261Z 2022-06-28T13:00:12Z E! [agent] Error writing to outputs.cloudwatch: operation error CloudWatch: PutMetricData, exceeded maximum number of attempts, 3, https response error StatusCode: 0, RequestID: , request send failed, Post "https://monitoring.eu-central-1.amazonaws.com/": proxyconnect tcp: dial tcp 212.62.95.45:1080: i/o timeout

now it's this:
2022-06-30T20:46:24.280273225Z 2022-06-30T20:46:24Z E! [agent] Error writing to outputs.cloudwatch: operation error CloudWatch: PutMetricData, exceeded maximum number of attempts, 3, https response error StatusCode: 0, RequestID: , request send failed, Post "https://monitoring.eu-central-1.amazonaws.com/": context deadline exceeded (Client.Timeout exceeded while awaiting headers)

What would be a good way to improve this? Would it help to reintroduce the timeout variable in the aws struct and define it to some shorter time than the ctx timeout?

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the nice work @scholz!

Regarding the error message, I don't immediately see where the difference comes from, but it might make sense to track it down and fix it for common/http... Any ideas? If so, it should be a new PR... Thanks again for your contribution!

@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 Jul 4, 2022
Comment on lines 182 to 183
// Disable logging
options.ClientLogMode = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like disabling logging is unrelated to this PR's goal of adding proxy http connections. Is this necessary or could we just accept NewFromConfig's default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing. When I did the first iteration, I used the proxy code from the cloudwatch input plugin. That's how that option landed there.

Looking at the docs (https://aws.github.io/aws-sdk-go-v2/docs/configuring-sdk/logging/ ;https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws#ClientLogMode) it seems that the logging is off by default. So its probably safe to remove it.

I will remove it from the code, rebuild and run another check tomorrow. And depending on the result updat the pull request.

@telegraf-tiger
Copy link
Contributor

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 decreases the Telegraf binary size by -2.53 % for linux amd64 (new size: 142.4 MB, nightly size 146.1 MB)

📦 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_i386.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz
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
static_linux_amd64.tar.gz

@powersj powersj merged commit 20acbf7 into influxdata:master Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloud Issues or requests around cloud environments feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin 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
Development

Successfully merging this pull request may close these issues.

[outputs.cloudwatch] proxy support
4 participants