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

Include notifier type in retry logs and errors #702

Merged
merged 1 commit into from
Apr 11, 2017
Merged

Conversation

juliusv
Copy link
Member

@juliusv juliusv commented Apr 10, 2017

No description provided.

@michaeljs1990
Copy link

Thanks for publishing this. I tested it out on one of the servers having an issue with too many alerts coming in from prometheus and was able to get a more detailed output!

@@ -543,9 +543,9 @@ func (r RetryStage) Exec(ctx context.Context, alerts ...*types.Alert) (context.C
case <-tick.C:
if retry, err := r.integration.Notify(ctx, alerts...); err != nil {
numFailedNotifications.WithLabelValues(r.integration.name).Inc()
log.Debugf("Notify attempt %d failed: %s", i, err)
log.Debugf("Notify attempt %d for %q failed: %s", i, r.integration.name, err)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this debug logging actually has any value as we ultimately always log errors happening in the notification pipeline, but I also don't mind keeping it as it's closer to the source here without many moving parts where it may get lost.

@brancz
Copy link
Member

brancz commented Apr 11, 2017

lgtm 👍 , your call whether we want to keep the debug logging or not

@fabxc
Copy link
Contributor

fabxc commented Apr 11, 2017

👍

@juliusv juliusv merged commit 0ce2de5 into master Apr 11, 2017
@juliusv juliusv deleted the log-failed-notifiers branch April 11, 2017 11:00
hh pushed a commit to ii/alertmanager that referenced this pull request Nov 6, 2017
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.

5 participants