Skip to content

Include notifier type in retry logs and errors - #702

Merged
juliusv merged 1 commit into
masterfrom
log-failed-notifiers
Apr 11, 2017
Merged

Include notifier type in retry logs and errors#702
juliusv merged 1 commit into
masterfrom
log-failed-notifiers

Conversation

@juliusv

@juliusv juliusv commented Apr 10, 2017

Copy link
Copy Markdown
Member

No description provided.

@michaeljs1990

Copy link
Copy Markdown

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!

Comment thread notify/notify.go
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
Copy Markdown
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

brancz commented Apr 11, 2017

Copy link
Copy Markdown
Member

lgtm πŸ‘ , your call whether we want to keep the debug logging or not

@fabxc

fabxc commented Apr 11, 2017

Copy link
Copy Markdown
Contributor

πŸ‘

@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