feat: better grouping alert & pagerduty receiver fix (#172)

* fix(alert): better grouping when sending notification

* fix(pagerduty): proper incident key to resolve and silent when acked

core/notification/builder.go

@@ -28,62 +28,70 @@ import (
 // - alertname
 // - (others labels defined in rules)
 func BuildFromAlerts(
-	as []alert.Alert,
+	alerts []alert.Alert,
 	firingLen int,
 	createdTime time.Time,
-) Notification {
-	if len(as) == 0 {
-		return Notification{}
+) ([]Notification, error) {
+	if len(alerts) == 0 {
+		return nil, errors.New("empty alerts")
 	}
 
-	sampleAlert := as[0]
+	alertsMap, err := groupByLabels(alerts)
+	if err != nil {
+		return nil, err
+	}
+
+	var notifications []Notification
 
-	data := map[string]interface{}{}
+	for hashKey, groupedAlerts := range alertsMap {
+		sampleAlert := groupedAlerts[0]
 
-	mergedAnnotations := map[string][]string{}
-	for _, a := range as {
-		for k, v := range a.Annotations {
-			mergedAnnotations[k] = append(mergedAnnotations[k], v)
+		data := map[string]interface{}{}
+
+		mergedAnnotations := map[string][]string{}
+		for _, a := range groupedAlerts {
+			for k, v := range a.Annotations {
+				mergedAnnotations[k] = append(mergedAnnotations[k], v)
+			}
 		}
-	}
-	// make unique
-	for k, v := range mergedAnnotations {
-		mergedAnnotations[k] = removeDuplicateStringValues(v)
-	}
-	// render annotations
-	for k, vSlice := range mergedAnnotations {
-		for _, v := range vSlice {
-			if _, ok := data[k]; ok {
-				data[k] = fmt.Sprintf("%s\n%s", data[k], v)
-			} else {
-				data[k] = v
+		// make unique
+		for k, v := range mergedAnnotations {
+			mergedAnnotations[k] = removeDuplicateStringValues(v)
+		}
+		// render annotations
+		for k, vSlice := range mergedAnnotations {
+			for _, v := range vSlice {
+				if _, ok := data[k]; ok {
+					data[k] = fmt.Sprintf("%s\n%s", data[k], v)
+				} else {
+					data[k] = v
+				}
 			}
 		}
-	}
 
-	data["status"] = sampleAlert.Status
-	data["generator_url"] = sampleAlert.GeneratorURL
-	data["num_alerts_firing"] = firingLen
+		data["status"] = sampleAlert.Status
+		data["generator_url"] = sampleAlert.GeneratorURL
+		data["num_alerts_firing"] = firingLen
 
-	labels := map[string]string{}
-	alertIDs := []int64{}
+		alertIDs := []int64{}
 
-	for _, a := range as {
-		alertIDs = append(alertIDs, int64(a.ID))
-		for k, v := range a.Labels {
-			labels[k] = v
+		for _, a := range groupedAlerts {
+			alertIDs = append(alertIDs, int64(a.ID))
 		}
-	}
 
-	return Notification{
-		NamespaceID: sampleAlert.NamespaceID,
-		Type:        TypeSubscriber,
-		Data:        data,
-		Labels:      labels,
-		Template:    template.ReservedName_SystemDefault,
-		CreatedAt:   createdTime,
-		AlertIDs:    alertIDs,
+		notifications = append(notifications, Notification{
+			NamespaceID: sampleAlert.NamespaceID,
+			Type:        TypeSubscriber,
+			Data:        data,
+			Labels:      sampleAlert.Labels,
+			Template:    template.ReservedName_SystemDefault,
+			UniqueKey:   hashGroupKey(sampleAlert.GroupKey, hashKey),
+			CreatedAt:   createdTime,
+			AlertIDs:    alertIDs,
+		})
 	}
+
+	return notifications, nil
 }
 
 // BuildTypeReceiver builds a notification struct with receiver type flow
@@ -115,16 +123,3 @@ func BuildTypeReceiver(receiverID uint64, payloadMap map[string]interface{}) (No
 
 	return n, nil
 }
-
-func removeDuplicateStringValues(strSlice []string) []string {
-	keys := make(map[string]bool)
-	list := []string{}
-
-	for _, v := range strSlice {
-		if _, value := keys[v]; !value {
-			keys[v] = true
-			list = append(list, v)
-		}
-	}
-	return list
-}

core/notification/builder_test.go

@@ -5,6 +5,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/odpf/siren/core/alert"
 	"github.com/odpf/siren/core/notification"
 	"github.com/odpf/siren/core/template"
@@ -86,17 +87,18 @@ func TestBuildFromAlerts(t *testing.T) {
 		name      string
 		alerts    []alert.Alert
 		firingLen int
-		want      notification.Notification
+		want      []notification.Notification
+		errString string
 	}{
 
 		{
-			name: "should return empty notification if alerts slice is empty",
-			want: notification.Notification{},
+			name:      "should return empty notification if alerts slice is empty",
+			errString: "empty alerts",
 		},
 		{
 			name: `should properly return notification
 				- same annotations are joined by newline
-				- labels are merged
+				- different labels are splitted into two notifications
 			`,
 			alerts: []alert.Alert{
 				{
@@ -121,38 +123,72 @@ func TestBuildFromAlerts(t *testing.T) {
 					MetricValue:  "16",
 					Severity:     "WARNING",
 					Rule:         "test-alert-template",
-					Labels:       map[string]string{"lk1": "lv11", "lk2": "lv2"},
+					Labels:       map[string]string{"lk1": "lv1", "lk2": "lv2"},
+					Annotations:  map[string]string{"ak1": "akv1"},
+					Status:       "FIRING",
+				},
+				{
+					ID:           16,
+					ProviderID:   1,
+					NamespaceID:  1,
+					ResourceName: "test-alert-host-2",
+					MetricName:   "test-alert",
+					MetricValue:  "16",
+					Severity:     "WARNING",
+					Rule:         "test-alert-template",
+					Labels:       map[string]string{"lk1": "lv1", "lk2": "lv2"},
 					Annotations:  map[string]string{"ak1": "akv11", "ak2": "akv2"},
 					Status:       "FIRING",
 				},
 			},
 			firingLen: 2,
-			want: notification.Notification{
-				NamespaceID: 1,
-				Type:        notification.TypeSubscriber,
-
-				Data: map[string]interface{}{
-					"generator_url":     "",
-					"num_alerts_firing": 2,
-					"status":            "FIRING",
-					"ak1":               "akv1\nakv11",
-					"ak2":               "akv2",
+			want: []notification.Notification{
+				{
+					NamespaceID: 1,
+					Type:        notification.TypeSubscriber,
+					Data: map[string]interface{}{
+						"generator_url":     "",
+						"num_alerts_firing": 2,
+						"status":            "FIRING",
+						"ak1":               "akv1",
+					},
+					Labels: map[string]string{
+						"lk1": "lv1",
+					},
+					UniqueKey: "ignored",
+					Template:  template.ReservedName_SystemDefault,
+					AlertIDs:  []int64{14},
 				},
-				Labels: map[string]string{
-					"lk1": "lv11",
-					"lk2": "lv2",
+				{
+					NamespaceID: 1,
+					Type:        notification.TypeSubscriber,
+
+					Data: map[string]interface{}{
+						"generator_url":     "",
+						"num_alerts_firing": 2,
+						"status":            "FIRING",
+						"ak1":               "akv1\nakv11",
+						"ak2":               "akv2",
+					},
+					Labels: map[string]string{
+						"lk1": "lv1",
+						"lk2": "lv2",
+					},
+					UniqueKey: "ignored",
+					Template:  template.ReservedName_SystemDefault,
+					AlertIDs:  []int64{15, 16},
 				},
-				Template: template.ReservedName_SystemDefault,
-				AlertIDs: []int64{14, 15},
 			},
 		},
-		{},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := notification.BuildFromAlerts(tt.alerts, tt.firingLen, time.Time{})
-
-			if diff := cmp.Diff(got, tt.want); diff != "" {
+			got, err := notification.BuildFromAlerts(tt.alerts, tt.firingLen, time.Time{})
+			if (err != nil) && (err.Error() != tt.errString) {
+				t.Errorf("BuildTypeReceiver() error = %v, wantErr %s", err, tt.errString)
+				return
+			}
+			if diff := cmp.Diff(got, tt.want, cmpopts.IgnoreFields(notification.Notification{}, "ID", "UniqueKey")); diff != "" {
 				t.Errorf("BuildFromAlerts() got diff = %v", diff)
 			}
 		})

core/notification/notification.go

@@ -32,6 +32,7 @@ type Notification struct {
 	Labels        map[string]string      `json:"labels"`
 	ValidDuration time.Duration          `json:"valid_duration"`
 	Template      string                 `json:"template"`
+	UniqueKey     string                 `json:"unique_key"`
 	CreatedAt     time.Time              `json:"created_at"`
 
 	// won't be stored in notification table, only to propaget this to notification_subscriber

core/notification/utils.go

@@ -0,0 +1,45 @@
+package notification
+
+import (
+	"crypto/sha256"
+	"fmt"
+
+	"github.com/mitchellh/hashstructure/v2"
+	"github.com/odpf/siren/core/alert"
+)
+
+func removeDuplicateStringValues(strSlice []string) []string {
+	keys := make(map[string]bool)
+	list := []string{}
+
+	for _, v := range strSlice {
+		if _, value := keys[v]; !value {
+			keys[v] = true
+			list = append(list, v)
+		}
+	}
+	return list
+}
+
+func groupByLabels(alerts []alert.Alert) (map[uint64][]alert.Alert, error) {
+	var alertsMap = map[uint64][]alert.Alert{}
+
+	for _, a := range alerts {
+		hash, err := hashstructure.Hash(a.Labels, hashstructure.FormatV2, nil)
+		if err != nil {
+			return nil, fmt.Errorf("cannot get hash from alert %v", a)
+		}
+		alertsMap[hash] = append(alertsMap[hash], a)
+	}
+
+	return alertsMap, nil
+}
+
+// hashGroupKey hash groupKey from alert and hashKey from labels
+func hashGroupKey(groupKey string, hashKey uint64) string {
+	h := sha256.New()
+	// hash.Hash.Write never returns an error.
+	//nolint: errcheck
+	h.Write([]byte(fmt.Sprintf("%s%d", groupKey, hashKey)))
+	return fmt.Sprintf("%x", h.Sum(nil))
+}