notify: improve logs on notification errors #2273
Merged
+32
−25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Alertmanager can experience occasional failures when sending notifications to an external service. If the operation succeeds after some retry, the 'alertmanager_notifications_failed_total' metric increases but nothing is logged (unless running with log.level=debug). Hence an operator might receive an alert about notification failures but wouldn't know which integration was failing. With this change, notification failures are logged at the warning level. To avoid log flooding, similar failures on retries aren't logged. Additional information on the failing integration has also been added. Signed-off-by: Simon Pasquier <spasquie@redhat.com>
roidelapluie
reviewed
Jun 3, 2020
roidelapluie
reviewed
Jun 3, 2020
notify/notify.go
Outdated
| } | ||
|
|
||
| // Save this error to be able to return the last seen error by an | ||
| // integration upon context timeout. | ||
| iErr = err | ||
| } else { | ||
| level.Debug(l).Log("msg", "Notify success", "attempts", i) |
There was a problem hiding this comment.
Should we log to info if i > 1 ?
|
LGTM, added a few suggestions that you are free to pick up |
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alertmanager can experience occasional failures when sending
notifications to an external service. If the operation succeeds after
some retry, the 'alertmanager_notifications_failed_total' metric
increases but nothing is logged (unless running with log.level=debug).
Hence an operator might receive an alert about notification failures but
wouldn't know which integration was failing.
With this change, notification failures are logged at the warning level.
To avoid log flooding, similar failures on retries aren't logged.
Additional information on the failing integration has also been added.
cc @roidelapluie for review. Based on user's feedback from #2147 (comment).