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(outputs.graphite): Retry connecting to servers with failed send attempts #11439

Merged
merged 5 commits into from
Jul 25, 2022

Conversation

sspaink
Copy link
Contributor

@sspaink sspaink commented Jun 30, 2022

resolve: #11429

The Graphite output plugin would only retry a server connection if it failed to send on all connections, this behavior wasn't obvious and if one server always remains up the other servers would never reconnect. This pull requests updates the retry logic to track the failed servers and reconnect to them after each send. I could also add a retry_delay configuration option similar to the inputs.cloud_pubsub and inputs.jti_openconfig_telemetry plugins, but from the issue the author mentioned this might not be necessary so I thought maybe we could add the config option if someone does require it?

I also updated the log.Errorf to log.Debugf for connection related errors, seeing as these servers will retry anyway this would help remove some noise in the logs which was also a concern in reported in #11429. If all servers fail it should print an error to the log though.

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Jun 30, 2022
@sspaink sspaink marked this pull request as draft June 30, 2022 20:24
@sspaink sspaink changed the title fix: Improve logging in outputs.Graphite fix(outputs.graphite): Improve logging when setting deadline Jun 30, 2022
@sspaink sspaink changed the title fix(outputs.graphite): Improve logging when setting deadline feat(outputs.graphite): Retry connecting to servers with failed send attempts Jul 12, 2022
@sspaink sspaink added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin and removed fix pr to fix corresponding bug labels Jul 12, 2022
@sspaink sspaink marked this pull request as ready for review July 12, 2022 20:44
Copy link
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.

One small log change, thank you!

Co-authored-by: Joshua Powers <powersj@fastmail.com>
@telegraf-tiger
Copy link
Contributor

@sspaink sspaink 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 21, 2022
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.

Looks good to me. Thanks @sspaink!

@powersj powersj merged commit beb18d9 into master Jul 25, 2022
@powersj powersj deleted the logerrgraphite branch July 25, 2022 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Graphite Error: Couldn't set read deadline for connection / use of closed network connection
3 participants