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

Try to reconnect to Riemann if metrics upload failed. #1013

Closed
wants to merge 2 commits into from

Conversation

echupriyanov
Copy link
Contributor

Signed-off-by: Eugene Chupriyanov tchu@tchu.ru

@@ -68,8 +68,13 @@ func (r *Riemann) Write(metrics []telegraf.Metric) error {

var senderr = r.client.SendMulti(events)
if senderr != nil {
return errors.New(fmt.Sprintf("FAILED to send riemann message: %s\n",
senderr))
r.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

you shouldn't ignore errors from the Close and Connect functions

@echupriyanov echupriyanov force-pushed the riemann_improved branch 2 times, most recently from e3b3cbb to 95b104f Compare April 12, 2016 21:23
@echupriyanov
Copy link
Contributor Author

@sparrc Added better (I hope) error checks.

Signed-off-by: Eugene Chupriyanov <tchu@tchu.ru>

Error checks added

Don't Close() nil client

Signed-off-by: Eugene Chupriyanov <e.chupriyanov@cpm.ru>
@@ -68,8 +79,16 @@ func (r *Riemann) Write(metrics []telegraf.Metric) error {

var senderr = r.client.SendMulti(events)
if senderr != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

in here I would say just do:

r.Close()
return fmt.Errorf("FAILED to send riemann message (will try to reconnect). Error: %s\n",
                senderr)

because r.Close will set the connection to nil, and then a connection will be attempted on the next write.

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. But will Telegraf try to re-attempt to send metrics again? Or they would be dropped if send fails?

Anyway, I removed immediate reconnection attempt on send failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

the metrics will be cached, telegraf will re-attempt to send

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great, thanks for clarification.
should I squash PR into one commit, if it will be accepted for merge?

Copy link
Contributor

Choose a reason for hiding this comment

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

2 commits is fine, I'll merge it

Signed-off-by: Eugene Chupriyanov <e.chupriyanov@cpm.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants