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.

Sign up for GitHub

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

Jump to bottom

feat: Add timeout-setting to Graylog-plugin #10220

Merged
merged 4 commits into from
Dec 21, 2021
Merged

feat: Add timeout-setting to Graylog-plugin #10220

merged 4 commits into from
Dec 21, 2021

Conversation

SebastianThorn
Copy link
Contributor

Required for all PRs:

resolves #10185

I added timeout, but not sure if I did it correctly in regards to a default-vaule. I have not dealt with go before.

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Dec 6, 2021

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

1 similar comment
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Dec 6, 2021

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@SebastianThorn SebastianThorn changed the title Add timeout-setting to Graylog-plugin feat: Add timeout-setting to Graylog-plugin Dec 6, 2021
@SebastianThorn
Copy link
Contributor Author

!signed-cla

srebhan
srebhan reviewed Dec 6, 2021
View reviewed changes
Copy link
Member

@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.

@SebastianThorn thanks for the submission. The code looks good. I just have two very minor comments about the option name and ask for your help to add toml-tags to the config-option fields...

plugins/inputs/graylog/README.md Outdated Show resolved Hide resolved
plugins/inputs/graylog/graylog.go Outdated
Comment on lines 34 to 38
Servers []string
Metrics []string
Username string
Password string
ResponseTimeout config.Duration
Copy link
Member

Choose a reason for hiding this comment

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

When you are touching this anyway, can you please add the corresponding toml tag annotations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@srebhan srebhan self-assigned this Dec 6, 2021
@srebhan srebhan added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Dec 6, 2021
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Dec 6, 2021

👍 This pull request doesn't change the Telegraf binary size

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here ! 🐯

Artifact URLs

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

@SebastianThorn
Copy link
Contributor Author

@srebhan Does this look better?

srebhan
srebhan reviewed Dec 8, 2021
View reviewed changes
plugins/inputs/graylog/graylog.go
@@ -34,6 +35,7 @@ type GrayLog struct {
Metrics []string
Username string
Password string
Timeout config.Duration
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add the toml tags to the struct fields that receive user-options like

Suggested change
Timeout config.Duration
Timeout config.Duration `toml:"timeout"`

This makes it more clear which fields are configured by the user...

@srebhan
Copy link
Member

srebhan commented Dec 8, 2021 •
edited
Loading

The option looks good, I was waiting for the toml tags. Do you mind to add those also for the other struct fields?

srebhan
srebhan approved these changes Dec 21, 2021
View reviewed changes
Copy link
Member

@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.

Ok looks like we should add the TOML tags later and take what we have.
Looks good to me. Thanks @SebastianThorn 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 Dec 21, 2021
@powersj
Copy link
Contributor

powersj commented Dec 21, 2021

Ok looks like we should add the TOML tags later and take what we have. Looks good to me. Thanks @SebastianThorn for your contribution!

Can one of you please file a bug so we do not forget to go back and do this please?

Thanks!

@powersj powersj merged commit a024203 into influxdata:master Dec 21, 2021
@srebhan srebhan mentioned this pull request Dec 21, 2021
Closed
@srebhan
Copy link
Member

srebhan commented Dec 21, 2021

@powersj done in #10320.

powersj pushed a commit to powersj/telegraf that referenced this pull request Jan 21, 2022
@SebastianThorn @powersj
feat: add timeout-setting to Graylog-plugin (influxdata#10220)
9b92ef6
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

@powersj powersj powersj approved these changes

Assignees

@srebhan srebhan

Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input 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
No milestone
Development

Successfully merging this pull request may close these issues.

Timeout parameter for inputs.graylog
3 participants
@SebastianThorn @srebhan @powersj