Skip to content

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

Merged
powersj merged 4 commits intoinfluxdata:masterfrom
SebastianThorn:master
Dec 21, 2021
Merged

feat: Add timeout-setting to Graylog-plugin#10220
powersj merged 4 commits intoinfluxdata:masterfrom
SebastianThorn:master

Conversation

@SebastianThorn
Copy link
Copy Markdown
Contributor

@SebastianThorn SebastianThorn commented Dec 6, 2021

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

SebastianThorn commented Dec 6, 2021

!signed-cla

srebhan
srebhan reviewed Dec 6, 2021
View reviewed changes
Copy link
Copy Markdown
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/graylog.go Outdated
Comment on lines +34 to +38
Servers []string
Metrics []string
Username string
Password string
ResponseTimeout config.Duration
Copy link
Copy Markdown
Member

@srebhan srebhan Dec 6, 2021

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
Copy Markdown
Contributor Author

@SebastianThorn SebastianThorn Dec 6, 2021

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
Copy Markdown
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
Copy Markdown
Contributor Author

SebastianThorn commented Dec 8, 2021

@srebhan Does this look better?

srebhan
srebhan reviewed Dec 8, 2021
View reviewed changes
plugins/inputs/graylog/graylog.go
Metrics []string
Username string
Password string
Timeout config.Duration
Copy link
Copy Markdown
Member

@srebhan srebhan Dec 8, 2021

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

+1 more reviewer

@powersj powersj powersj approved these changes

Reviewers whose approvals may not affect merge requirements

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

4 participants

@SebastianThorn @srebhan @powersj @sma-hyns