From ad371bf412444408230292407df73e359343f7bb Mon Sep 17 00:00:00 2001 From: rcourtman Date: Tue, 21 Oct 2025 11:05:45 +0000 Subject: [PATCH] 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 ./...' --- internal/alerts/alerts.go | 68 +++++++++++++++++++++++++++------------ internal/api/alerts.go | 17 ++++++++++ 2 files changed, 64 insertions(+), 21 deletions(-) diff --git a/internal/alerts/alerts.go b/internal/alerts/alerts.go index 2ce1a2a..5df87a7 100644 --- a/internal/alerts/alerts.go +++ b/internal/alerts/alerts.go @@ -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") diff --git a/internal/api/alerts.go b/internal/api/alerts.go index ff32b9a..15da79c 100644 --- a/internal/api/alerts.go +++ b/internal/api/alerts.go @@ -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{}{