-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat(notify): add notification logging for successfully sent notifications #4906
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
base: main
Are you sure you want to change the base?
Conversation
…tions This adds a new --notification-log.file flag that enables logging of successfully sent notifications to a JSON lines file. This feature addresses the need for auditing and analytics of notification delivery. When enabled, each successful notification is logged with metadata including timestamp, integration type, receiver name, group key, alert counts, and group labels. The implementation follows the pattern of Prometheus query logging as suggested by maintainers. The log format is JSON lines, making it easy to parse and integrate with log aggregation systems. Log rotation is left to external tools like logrotate, following Prometheus conventions. Signed-off-by: Willian Paixao <willian@ufpa.br>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements notification logging for Alertmanager, enabling users to audit and analyze successfully sent notifications. The implementation adds a new CLI flag --notification-log.file that, when set, writes JSON-formatted log entries containing notification metadata (receiver, integration, group key, alert counts, and group labels) to a file.
Changes:
- Added notification logging infrastructure with a file-based logger implementation that supports concurrent writes
- Integrated notification logger into the RetryStage of the notification pipeline to log successful notifications
- Added CLI flag for enabling notification logging with file path configuration
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| notify/notification_log.go | New file implementing NotificationLogger interface with FileNotificationLogger and NoopNotificationLogger |
| notify/notification_log_test.go | Comprehensive test suite covering JSON serialization, file operations, concurrency, and edge cases |
| notify/notify.go | Integration of NotificationLogger into PipelineBuilder and RetryStage with helper function labelSetToMap |
| notify/notify_test.go | Updated existing tests to accommodate new NotificationLogger parameter in NewRetryStage function signature |
| cmd/alertmanager/main.go | Added CLI flag and initialization logic for notification logger |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| require.Equal(t, entry.Integration, decoded.Integration) | ||
| require.Equal(t, entry.IntegrationIdx, decoded.IntegrationIdx) | ||
| require.Equal(t, entry.Receiver, decoded.Receiver) | ||
| require.Equal(t, entry.GroupKey, decoded.GroupKey) | ||
| require.Equal(t, entry.AlertsCount, decoded.AlertsCount) | ||
| require.Equal(t, entry.FiringCount, decoded.FiringCount) | ||
| require.Equal(t, entry.ResolvedCount, decoded.ResolvedCount) | ||
| require.Equal(t, entry.GroupLabels, decoded.GroupLabels) |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timestamp field is not verified in the test assertion. While the other fields are checked for equality after JSON serialization/deserialization, the Timestamp field should also be verified to ensure proper JSON marshaling and unmarshaling of the time.Time type.
| // Log writes a notification entry to the log file as a JSON line. | ||
| // The data is buffered by the OS and synced to disk when Close is called. | ||
| // Returns ErrNotificationLogClosed if the logger has been closed. | ||
| func (l *FileNotificationLogger) Log(entry *NotificationLogEntry) error { | ||
| l.mu.Lock() | ||
| defer l.mu.Unlock() | ||
|
|
||
| if l.closed { | ||
| return ErrNotificationLogClosed | ||
| } | ||
|
|
||
| data, err := json.Marshal(entry) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| data = append(data, '\n') | ||
| _, err = l.file.Write(data) | ||
| return err |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Log method writes to the file without calling Sync. If the application crashes or the system loses power before the OS flushes the buffered data, notification logs could be lost. Consider adding a configuration option to sync after each write for users who need guaranteed durability, or document this behavior in the function comment since it currently states "The data is buffered by the OS and synced to disk when Close is called."
| // Log writes a notification entry to the log file as a JSON line. | ||
| // The data is buffered by the OS and synced to disk when Close is called. | ||
| // Returns ErrNotificationLogClosed if the logger has been closed. | ||
| func (l *FileNotificationLogger) Log(entry *NotificationLogEntry) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we should add new locks for this, or a custom notification format...
Would it make sense to use slog for example, and standardize to the standard library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also implementation wise, would sending the entries to a channel, and having a goroutine do the marshalling and logging be better, and hold locks for less? I am a bit worried about json.Marshal(...) while holding a lock
This PR implements notification logging for Alertmanager.
--notification-log.fileCLI flag to enable JSON-formatted notification loggingMotivation
Users need visibility into which notifications were successfully sent for auditing and analytics purposes. The current workaround of using a webhook receiver with
continue: truehas limitations around destination visibility and routing behavior.Implementation Details
New Files
notify/notification_log.go- Notification logger implementationnotify/notification_log_test.go- Unit testsModified Files
notify/notify.go- Integrate NotificationLogger into the pipelinecmd/alertmanager/main.go- Add CLI flag and initializationnotify/notify_test.go- Update existing tests for new function signatureLog Entry Format
Each line in the notification log file is a JSON object:
{ "timestamp": "2024-01-15T10:30:00.123Z", "integration": "slack", "integrationIdx": 0, "receiver": "team-alerts", "groupKey": "{}:{alertname=\"HighMemory\"}", "alertsCount": 3, "firingCount": 2, "resolvedCount": 1, "groupLabels": { "alertname": "HighMemory" } }Design Decisions
Usage
When the flag is not set or empty, notification logging is disabled (default behavior).
Test Plan
Future Considerations (Out of Scope)
status: "failure"in future PR)Fixes #2304