fix: missing notification on error (argoproj#3076)

* fix: missing notification on error Signed-off-by: zachaller <[email protected]> * fix tests Signed-off-by: zachaller <[email protected]> * aggregate errors Signed-off-by: zachaller <[email protected]> * fix bad stat counts Signed-off-by: zachaller <[email protected]> * return errors Signed-off-by: zachaller <[email protected]> * fix tests on return of errors Signed-off-by: zachaller <[email protected]> * change case on logs Signed-off-by: zachaller <[email protected]> * missed a case Signed-off-by: zachaller <[email protected]> --------- Signed-off-by: zachaller <[email protected]>
meeech · Oct 5, 2023 · ba7c9a5 · ba7c9a5
1 parent 83fe979
commit ba7c9a5
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 21 deletions.
diff --git a/utils/record/record.go b/utils/record/record.go
@@ -218,9 +218,7 @@ func (e *EventRecorderAdapter) defaultEventf(object runtime.Object, warn bool, o
 			err := e.sendNotifications(api, object, opts)
 			if err != nil {
 				logCtx.Errorf("Notifications failed to send for eventReason %s with error: %s", opts.EventReason, err)
-				e.NotificationFailedCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc()
 			}
-			e.NotificationSuccessCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc()
 		}
 	}
 
@@ -248,7 +246,7 @@ func NewAPIFactorySettings() api.Settings {
 }
 
 // Send notifications for triggered event if user is subscribed
-func (e *EventRecorderAdapter) sendNotifications(notificationsAPI api.API, object runtime.Object, opts EventOptions) error {
+func (e *EventRecorderAdapter) sendNotifications(notificationsAPI api.API, object runtime.Object, opts EventOptions) []error {
 	logCtx := logutil.WithObject(object)
 	_, namespace, name := logutil.KindNamespaceName(logCtx)
 	startTime := timeutil.Now()
@@ -259,7 +257,7 @@ func (e *EventRecorderAdapter) sendNotifications(notificationsAPI api.API, objec
 	}()
 
 	if notificationsAPI == nil {
-		return fmt.Errorf("notificationsAPI is nil")
+		return []error{fmt.Errorf("NotificationsAPI is nil")}
 	}
 
 	cfg := notificationsAPI.GetConfig()
@@ -274,39 +272,53 @@ func (e *EventRecorderAdapter) sendNotifications(notificationsAPI api.API, objec
 
 	objMap, err := toObjectMap(object)
 	if err != nil {
-		return err
+		return []error{err}
 	}
 
 	emptyCondition := hash("")
 
+	// We should not return in these loops because we want other configured notifications to still send if they can.
+	errors := []error{}
 	for _, destination := range destinations {
 		res, err := notificationsAPI.RunTrigger(trigger, objMap)
 		if err != nil {
-			log.Errorf("Failed to execute condition of trigger %s: %v", trigger, err)
-			return err
+			log.Errorf("Failed to run trigger, trigger: %s, destination: %s, namespace config: %s : %v",
+				trigger, destination, notificationsAPI.GetConfig().Namespace, err)
+			errors = append(errors, err)
+			continue
 		}
 		log.Infof("Trigger %s result: %v", trigger, res)
 
 		for _, c := range res {
-			log.Infof("Res When Condition hash: %s, Templates: %s", c.Key, c.Templates)
+			log.Infof("Result when condition hash: %s, templates: %s", c.Key, c.Templates)
 			s := strings.Split(c.Key, ".")[1]
 			if s != emptyCondition && c.Triggered == true {
 				err = notificationsAPI.Send(objMap, c.Templates, destination)
 				if err != nil {
-					log.Errorf("notification error: %s", err.Error())
-					return err
+					log.Errorf("Failed to execute the sending of notification on not empty condition, trigger: %s, destination: %s, namespace config: %s : %v",
+						trigger, destination, notificationsAPI.GetConfig().Namespace, err)
+					e.NotificationFailedCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc()
+					errors = append(errors, err)
+					continue
 				}
+				e.NotificationSuccessCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc()
 			} else if s == emptyCondition {
 				err = notificationsAPI.Send(objMap, c.Templates, destination)
 				if err != nil {
-					log.Errorf("notification error: %s", err.Error())
-					return err
+					log.Errorf("Failed to execute the sending of notification on empty condition, trigger: %s, destination: %s, namespace config: %s : %v",
+						trigger, destination, notificationsAPI.GetConfig().Namespace, err)
+					e.NotificationFailedCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc()
+					errors = append(errors, err)
+					continue
 				}
+				e.NotificationSuccessCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc()
 			}
 		}
 	}
-
-	return nil
+	if len(errors) == 0 {
+		return nil
+	}
+	return errors
 }
 
 // This function is copied over from notification engine to make sure we honour emptyCondition

diff --git a/utils/record/record_test.go b/utils/record/record_test.go
@@ -113,7 +113,7 @@ func TestSendNotifications(t *testing.T) {
 	rec.EventRecorderAdapter.apiFactory = apiFactory
 	//ch := make(chan prometheus.HistogramVec, 1)
 	err := rec.sendNotifications(mockAPI, &r, EventOptions{EventReason: "FooReason"})
-	assert.NoError(t, err)
+	assert.Nil(t, err)
 }
 
 func TestSendNotificationsWhenCondition(t *testing.T) {
@@ -140,7 +140,7 @@ func TestSendNotificationsWhenCondition(t *testing.T) {
 	rec.EventRecorderAdapter.apiFactory = apiFactory
 	//ch := make(chan prometheus.HistogramVec, 1)
 	err := rec.sendNotifications(mockAPI, &r, EventOptions{EventReason: "FooReason"})
-	assert.NoError(t, err)
+	assert.Nil(t, err)
 }
 
 func TestSendNotificationsWhenConditionTime(t *testing.T) {
@@ -340,7 +340,7 @@ func TestSendNotificationsFails(t *testing.T) {
 		rec.EventRecorderAdapter.apiFactory = apiFactory
 
 		err := rec.sendNotifications(mockAPI, &r, EventOptions{EventReason: "FooReason"})
-		assert.Error(t, err)
+		assert.Len(t, err, 1)
 	})
 
 	t.Run("GetAPIError", func(t *testing.T) {
@@ -349,7 +349,7 @@ func TestSendNotificationsFails(t *testing.T) {
 		rec.EventRecorderAdapter.apiFactory = apiFactory
 
 		err := rec.sendNotifications(nil, &r, EventOptions{EventReason: "FooReason"})
-		assert.Error(t, err)
+		assert.NotNil(t, err)
 	})
 
 }
@@ -380,7 +380,7 @@ func TestSendNotificationsFailsWithRunTriggerError(t *testing.T) {
 		rec.EventRecorderAdapter.apiFactory = apiFactory
 
 		err := rec.sendNotifications(mockAPI, &r, EventOptions{EventReason: "FooReason"})
-		assert.Error(t, err)
+		assert.Len(t, err, 1)
 	})
 
 	t.Run("GetAPIError", func(t *testing.T) {
@@ -389,7 +389,7 @@ func TestSendNotificationsFailsWithRunTriggerError(t *testing.T) {
 		rec.EventRecorderAdapter.apiFactory = apiFactory
 
 		err := rec.sendNotifications(nil, &r, EventOptions{EventReason: "FooReason"})
-		assert.Error(t, err)
+		assert.NotNil(t, err)
 	})
 
 }
@@ -419,7 +419,7 @@ func TestSendNotificationsNoTrigger(t *testing.T) {
 	rec.EventRecorderAdapter.apiFactory = apiFactory
 
 	err := rec.sendNotifications(mockAPI, &r, EventOptions{EventReason: "MissingReason"})
-	assert.Error(t, err)
+	assert.Len(t, err, 1)
 }
 
 func TestNewAPIFactorySettings(t *testing.T) {