feat: improve alert system performance, UX, and edge case handling

Implement 5 medium/low priority improvements identified in systematic review:

UX IMPROVEMENTS:
- Notify existing critical alerts when activating from pending_review state
  Previously: critical alerts during observation window would never notify
  Now: users receive notifications for active critical alerts after activation
  Implementation: Added NotifyExistingAlert() method and logic in ActivateAlerts()

PERFORMANCE OPTIMIZATIONS:
- Replace per-alert cleanup goroutines with periodic batch cleanup
  Prevents spawning 1000s of goroutines during alert flapping
  recentlyResolved entries now cleaned up once per minute instead of 1 goroutine per alert
- Simplify GetActiveAlerts() implementation
  Removed intermediate map copy, holds lock slightly longer but operation is fast
  Cleaner code with reduced memory allocation

CONFIGURATION VALIDATION:
- Validate timezone in quiet hours configuration
  Invalid timezones now disable quiet hours with error log instead of silent fallback
  Prevents unexpected behavior when timezone is typo'd or invalid

GRACEFUL SHUTDOWN:
- Add 100ms delay in Stop() for background goroutine cleanup
  Reduces risk of state corruption during shutdown
  Allows escalation checker and periodic save to exit cleanly

Technical details:
- internal/alerts/alerts.go: Added NotifyExistingAlert(), optimized cleanup patterns
- internal/api/alerts.go: Enhanced ActivateAlerts() to notify existing critical alerts
- Removed ~20 lines of goroutine spawning code
- Added periodic cleanup for recentlyResolved map
- All changes preserve backward compatibility

Testing: Verified compilation with 'go build -o /dev/null ./...'
This commit is contained in:
rcourtman 2025-10-21 11:05:45 +00:00
parent 06b5d5153b
commit ad371bf412
2 changed files with 64 additions and 21 deletions

View file

@ -925,6 +925,21 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
ensureValidHysteresis(config.NodeDefaults.Temperature, "node.temperature")
ensureValidHysteresis(&config.StorageDefault, "storage")
// Validate timezone if quiet hours are enabled
if config.Schedule.QuietHours.Enabled {
if config.Schedule.QuietHours.Timezone != "" {
_, err := time.LoadLocation(config.Schedule.QuietHours.Timezone)
if err != nil {
log.Error().
Err(err).
Str("timezone", config.Schedule.QuietHours.Timezone).
Msg("Invalid timezone in quiet hours config, disabling quiet hours")
// Disable quiet hours rather than silently using wrong timezone
config.Schedule.QuietHours.Enabled = false
}
}
}
m.config = config
for id, override := range m.config.Overrides {
override.PoweredOffSeverity = normalizePoweredOffSeverity(override.PoweredOffSeverity)
@ -4153,19 +4168,6 @@ func (m *Manager) checkMetric(resourceID, resourceName, node, instance, resource
Str("alertID", alertID).
Msg("Added alert to recently resolved")
// Schedule cleanup after 5 minutes
go func() {
defer func() {
if r := recover(); r != nil {
log.Error().Interface("panic", r).Str("alertID", alertID).Msg("Panic in cleanup goroutine")
}
}()
time.Sleep(5 * time.Minute)
m.resolvedMutex.Lock()
delete(m.recentlyResolved, alertID)
m.resolvedMutex.Unlock()
}()
log.Info().
Str("resource", resourceName).
Str("metric", metricType).
@ -4343,20 +4345,30 @@ func (m *Manager) removeActiveAlertNoLock(alertID string) {
// GetActiveAlerts returns all active alerts
func (m *Manager) GetActiveAlerts() []Alert {
m.mu.RLock()
// Make a quick copy of the map to avoid holding the lock too long
alertsCopy := make(map[string]*Alert, len(m.activeAlerts))
for k, v := range m.activeAlerts {
alertsCopy[k] = v
}
m.mu.RUnlock()
defer m.mu.RUnlock()
alerts := make([]Alert, 0, len(alertsCopy))
for _, alert := range alertsCopy {
alerts := make([]Alert, 0, len(m.activeAlerts))
for _, alert := range m.activeAlerts {
alerts = append(alerts, *alert)
}
return alerts
}
// NotifyExistingAlert re-dispatches a notification for an existing active alert
// Used when activation state changes from pending to active
func (m *Manager) NotifyExistingAlert(alertID string) {
m.mu.RLock()
alert, exists := m.activeAlerts[alertID]
m.mu.RUnlock()
if !exists {
return
}
// Dispatch notification for existing alert
m.dispatchAlert(alert, true)
}
// GetRecentlyResolved returns recently resolved alerts
func (m *Manager) GetRecentlyResolved() []models.ResolvedAlert {
m.resolvedMutex.RLock()
@ -6063,6 +6075,16 @@ func (m *Manager) Cleanup(maxAge time.Duration) {
}
}
// Clean up old recently resolved alerts (older than 5 minutes)
fiveMinutesAgo := now.Add(-5 * time.Minute)
m.resolvedMutex.Lock()
for alertID, resolved := range m.recentlyResolved {
if resolved.ResolvedTime.Before(fiveMinutesAgo) {
delete(m.recentlyResolved, alertID)
}
}
m.resolvedMutex.Unlock()
// Clean up stale pending alerts (older than max time threshold window)
// This prevents memory leak from deleted resources that never triggered alerts
maxPendingAge := 10 * time.Minute // Longest time threshold + safety buffer
@ -6588,6 +6610,10 @@ func (m *Manager) checkEscalations() {
func (m *Manager) Stop() {
close(m.escalationStop)
m.historyManager.Stop()
// Give background goroutines time to exit cleanly
time.Sleep(100 * time.Millisecond)
// Save active alerts before stopping
if err := m.SaveActiveAlerts(); err != nil {
log.Error().Err(err).Msg("Failed to save active alerts on stop")

View file

@ -135,6 +135,23 @@ func (h *AlertHandlers) ActivateAlerts(w http.ResponseWriter, r *http.Request) {
return
}
// Notify about existing critical alerts after activation
activeAlerts := h.monitor.GetAlertManager().GetActiveAlerts()
criticalCount := 0
for _, alert := range activeAlerts {
if alert.Level == alerts.AlertLevelCritical && !alert.Acknowledged {
// Re-dispatch critical alerts to trigger notifications
h.monitor.GetAlertManager().NotifyExistingAlert(alert.ID)
criticalCount++
}
}
if criticalCount > 0 {
log.Info().
Int("criticalAlerts", criticalCount).
Msg("Sent notifications for existing critical alerts after activation")
}
log.Info().Msg("Alert notifications activated")
if err := utils.WriteJSONResponse(w, map[string]interface{}{