AlertingNG: Correctly set StartsAt, EndsAt, UpdatedAt after alert reception (#33109)

Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>

pkg/services/ngalert/notifier/alert_reception.go

@@ -1,60 +0,0 @@
-package notifier
-
-import (
-	"context"
-	"time"
-
-	gokit_log "github.com/go-kit/kit/log"
-	apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
-	"github.com/prometheus/alertmanager/api/v2/models"
-	"github.com/prometheus/alertmanager/provider"
-	"github.com/prometheus/alertmanager/provider/mem"
-	"github.com/prometheus/alertmanager/types"
-	"github.com/prometheus/common/model"
-)
-
-type AlertProvider struct {
-	provider.Alerts
-}
-
-// NewAlertProvider returns AlertProvider that provides a method to translate
-// Grafana alerts to Prometheus Alertmanager alerts before passing it ahead.
-func NewAlertProvider(m types.Marker) (*AlertProvider, error) {
-	alerts, err := mem.NewAlerts(context.Background(), m, 30*time.Minute, gokit_log.NewNopLogger())
-	if err != nil {
-		return nil, err
-	}
-
-	return &AlertProvider{Alerts: alerts}, nil
-}
-
-func (ap *AlertProvider) PutPostableAlert(postableAlerts apimodels.PostableAlerts) error {
-	alerts := make([]*types.Alert, 0, len(postableAlerts.PostableAlerts))
-	for _, a := range postableAlerts.PostableAlerts {
-		alerts = append(alerts, alertForDelivery(a))
-	}
-	return ap.Alerts.Put(alerts...)
-}
-
-func alertForDelivery(a models.PostableAlert) *types.Alert {
-	lbls := model.LabelSet{}
-	annotations := model.LabelSet{}
-	for k, v := range a.Labels {
-		lbls[model.LabelName(k)] = model.LabelValue(v)
-	}
-	for k, v := range a.Annotations {
-		annotations[model.LabelName(k)] = model.LabelValue(v)
-	}
-
-	return &types.Alert{
-		Alert: model.Alert{
-			Labels:       lbls,
-			Annotations:  annotations,
-			StartsAt:     time.Time(a.StartsAt),
-			EndsAt:       time.Time(a.EndsAt),
-			GeneratorURL: a.GeneratorURL.String(),
-		},
-		UpdatedAt: time.Time{}, // TODO(codesome) what should this be?
-		Timeout:   false,       // TODO(codesome).
-	}
-}

pkg/services/ngalert/notifier/alert_reception_test.go

@@ -1,113 +0,0 @@
-package notifier
-
-import (
-	"testing"
-	"time"
-
-	"github.com/go-openapi/strfmt"
-	apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
-	"github.com/prometheus/alertmanager/api/v2/models"
-	"github.com/prometheus/alertmanager/provider"
-	"github.com/prometheus/alertmanager/types"
-	"github.com/prometheus/client_golang/prometheus"
-	"github.com/prometheus/common/model"
-	"github.com/stretchr/testify/require"
-)
-
-func TestAlertProvider(t *testing.T) {
-	marker := types.NewMarker(prometheus.DefaultRegisterer)
-	alertProvider := &mockAlertProvider{}
-
-	ap, err := NewAlertProvider(marker)
-	require.NoError(t, err)
-	ap.Alerts = alertProvider
-
-	startTime := time.Now()
-	endTime := startTime.Add(2 * time.Hour)
-	postableAlerts := apimodels.PostableAlerts{
-		PostableAlerts: []models.PostableAlert{
-			{ // Start and end set.
-				Annotations: models.LabelSet{"msg": "Alert1 annotation"},
-				Alert: models.Alert{
-					Labels:       models.LabelSet{"alertname": "Alert1"},
-					GeneratorURL: "http://localhost/url1",
-				},
-				StartsAt: strfmt.DateTime(startTime),
-				EndsAt:   strfmt.DateTime(endTime),
-			}, { // Only end is set.
-				Annotations: models.LabelSet{"msg": "Alert2 annotation"},
-				Alert: models.Alert{
-					Labels:       models.LabelSet{"alertname": "Alert2"},
-					GeneratorURL: "http://localhost/url2",
-				},
-				StartsAt: strfmt.DateTime{},
-				EndsAt:   strfmt.DateTime(endTime),
-			}, { // Only start is set.
-				Annotations: models.LabelSet{"msg": "Alert3 annotation"},
-				Alert: models.Alert{
-					Labels:       models.LabelSet{"alertname": "Alert3"},
-					GeneratorURL: "http://localhost/url3",
-				},
-				StartsAt: strfmt.DateTime(startTime),
-				EndsAt:   strfmt.DateTime{},
-			}, { // Both start and end are not set.
-				Annotations: models.LabelSet{"msg": "Alert4 annotation"},
-				Alert: models.Alert{
-					Labels:       models.LabelSet{"alertname": "Alert4"},
-					GeneratorURL: "http://localhost/url4",
-				},
-				StartsAt: strfmt.DateTime{},
-				EndsAt:   strfmt.DateTime{},
-			},
-		},
-	}
-
-	require.NoError(t, ap.PutPostableAlert(postableAlerts))
-
-	// Alerts that should be sent for routing.
-	expProviderAlerts := []*types.Alert{
-		{
-			Alert: model.Alert{
-				Annotations:  model.LabelSet{"msg": "Alert1 annotation"},
-				Labels:       model.LabelSet{"alertname": "Alert1"},
-				StartsAt:     startTime,
-				EndsAt:       endTime,
-				GeneratorURL: "http://localhost/url1",
-			},
-		}, {
-			Alert: model.Alert{
-				Annotations:  model.LabelSet{"msg": "Alert2 annotation"},
-				Labels:       model.LabelSet{"alertname": "Alert2"},
-				EndsAt:       endTime,
-				GeneratorURL: "http://localhost/url2",
-			},
-		}, {
-			Alert: model.Alert{
-				Annotations:  model.LabelSet{"msg": "Alert3 annotation"},
-				Labels:       model.LabelSet{"alertname": "Alert3"},
-				StartsAt:     startTime,
-				GeneratorURL: "http://localhost/url3",
-			},
-		}, {
-			Alert: model.Alert{
-				Annotations:  model.LabelSet{"msg": "Alert4 annotation"},
-				Labels:       model.LabelSet{"alertname": "Alert4"},
-				GeneratorURL: "http://localhost/url4",
-			},
-		},
-	}
-	require.Equal(t, expProviderAlerts, alertProvider.alerts)
-}
-
-type mockAlertProvider struct {
-	alerts []*types.Alert
-}
-
-func (a *mockAlertProvider) Subscribe() provider.AlertIterator           { return nil }
-func (a *mockAlertProvider) GetPending() provider.AlertIterator          { return nil }
-func (a *mockAlertProvider) Get(model.Fingerprint) (*types.Alert, error) { return nil, nil }
-
-func (a *mockAlertProvider) Put(alerts ...*types.Alert) error {
-	a.alerts = append(a.alerts, alerts...)
-	return nil
-}

pkg/services/ngalert/notifier/alertmanager.go

@@ -13,22 +13,26 @@ import (
 	"errors"
 
 	gokit_log "github.com/go-kit/kit/log"
-	apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
+	amv2 "github.com/prometheus/alertmanager/api/v2/models"
 	"github.com/prometheus/alertmanager/dispatch"
 	"github.com/prometheus/alertmanager/inhibit"
 	"github.com/prometheus/alertmanager/nflog"
 	"github.com/prometheus/alertmanager/nflog/nflogpb"
 	"github.com/prometheus/alertmanager/notify"
+	"github.com/prometheus/alertmanager/provider"
+	"github.com/prometheus/alertmanager/provider/mem"
 	"github.com/prometheus/alertmanager/silence"
 	"github.com/prometheus/alertmanager/template"
 	"github.com/prometheus/alertmanager/types"
 	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/common/model"
 
 	"github.com/grafana/grafana/pkg/components/securejsondata"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/registry"
 	"github.com/grafana/grafana/pkg/services/alerting"
+	apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
 	ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models"
 	"github.com/grafana/grafana/pkg/services/ngalert/notifier/channels"
 	"github.com/grafana/grafana/pkg/services/ngalert/store"
@@ -42,6 +46,11 @@ const (
 	workingDir   = "alerting"
 	// How long should we keep silences and notification entries on-disk after they've served their purpose.
 	retentionNotificationsAndSilences = 5 * 24 * time.Hour
+	// defaultResolveTimeout is the default timeout used for resolving an alert
+	// if the end time is not specified.
+	defaultResolveTimeout = 5 * time.Minute
+	// memoryAlertsGCInterval is the interval at which we'll remove resolved alerts from memory.
+	memoryAlertsGCInterval = 30 * time.Minute
 	// To start, the alertmanager needs at least one route defined.
 	// TODO: we should move this to Grafana settings and define this as the default.
 	alertmanagerDefaultConfiguration = `
@@ -76,10 +85,11 @@ type Alertmanager struct {
 	// notificationLog keeps tracks of which notifications we've fired already.
 	notificationLog *nflog.Log
 	// silences keeps the track of which notifications we should not fire due to user configuration.
+
 	silencer   *silence.Silencer
 	silences   *silence.Silences
 	marker     types.Marker
-	alerts     *AlertProvider
+	alerts     provider.Alerts
 	route      *dispatch.Route
 	dispatcher *dispatch.Dispatcher
 	inhibitor  *inhibit.Inhibitor
@@ -126,7 +136,7 @@ func (am *Alertmanager) Init() (err error) {
 		return fmt.Errorf("unable to initialize the silencing component of alerting: %w", err)
 	}
 
-	am.alerts, err = NewAlertProvider(am.marker)
+	am.alerts, err = mem.NewAlerts(context.Background(), am.marker, memoryAlertsGCInterval, gokit_log.NewNopLogger())
 	if err != nil {
 		return fmt.Errorf("unable to initialize the alert provider component of alerting: %w", err)
 	}
@@ -368,8 +378,88 @@ func (am *Alertmanager) buildReceiverIntegrations(receiver *apimodels.PostableAp
 }
 
 // PutAlerts receives the alerts and then sends them through the corresponding route based on whenever the alert has a receiver embedded or not
-func (am *Alertmanager) PutAlerts(alerts apimodels.PostableAlerts) error {
+func (am *Alertmanager) PutAlerts(postableAlerts apimodels.PostableAlerts) error {
-	return am.alerts.PutPostableAlert(alerts)
+	now := time.Now()
+	alerts := make([]*types.Alert, 0, len(postableAlerts.PostableAlerts))
+	var validationErr *AlertValidationError
+	for _, a := range postableAlerts.PostableAlerts {
+		alert := &types.Alert{
+			Alert: model.Alert{
+				Labels:       model.LabelSet{},
+				Annotations:  model.LabelSet{},
+				StartsAt:     time.Time(a.StartsAt),
+				EndsAt:       time.Time(a.EndsAt),
+				GeneratorURL: a.GeneratorURL.String(),
+			},
+			UpdatedAt: now,
+		}
+		for k, v := range a.Labels {
+			if len(v) == 0 { // Skip empty labels.
+				continue
+			}
+			alert.Alert.Labels[model.LabelName(k)] = model.LabelValue(v)
+		}
+		for k, v := range a.Annotations {
+			if len(v) == 0 { // Skip empty annotation.
+				continue
+			}
+			alert.Alert.Annotations[model.LabelName(k)] = model.LabelValue(v)
+		}
+
+		// Ensure StartsAt is set.
+		if alert.StartsAt.IsZero() {
+			if alert.EndsAt.IsZero() {
+				alert.StartsAt = now
+			} else {
+				alert.StartsAt = alert.EndsAt
+			}
+		}
+		// If no end time is defined, set a timeout after which an alert
+		// is marked resolved if it is not updated.
+		if alert.EndsAt.IsZero() {
+			alert.Timeout = true
+			alert.EndsAt = now.Add(defaultResolveTimeout)
+		}
+
+		if err := alert.Validate(); err != nil {
+			if validationErr == nil {
+				validationErr = &AlertValidationError{}
+			}
+			validationErr.Alerts = append(validationErr.Alerts, a)
+			validationErr.Errors = append(validationErr.Errors, err)
+			continue
+		}
+
+		alerts = append(alerts, alert)
+	}
+
+	if err := am.alerts.Put(alerts...); err != nil {
+		// Notification sending alert takes precedence over validation errors.
+		return err
+	}
+	if validationErr != nil {
+		// Even if validationErr is nil, the require.NoError fails on it.
+		return validationErr
+	}
+	return nil
+}
+
+// AlertValidationError is the error capturing the validation errors
+// faced on the alerts.
+type AlertValidationError struct {
+	Alerts []amv2.PostableAlert
+	Errors []error // Errors[i] refers to Alerts[i].
+}
+
+func (e AlertValidationError) Error() string {
+	errMsg := ""
+	if len(e.Errors) != 0 {
+		errMsg := e.Errors[0].Error()
+		for _, e := range e.Errors[1:] {
+			errMsg += ";" + e.Error()
+		}
+	}
+	return errMsg
 }
 
 // createReceiverStage creates a pipeline of stages for a receiver.

pkg/services/ngalert/notifier/alertmanager_test.go

@@ -1,10 +1,20 @@
 package notifier
 
 import (
+	"errors"
+	"io/ioutil"
+	"os"
 	"testing"
+	"time"
 
+	"github.com/go-openapi/strfmt"
+	"github.com/prometheus/alertmanager/api/v2/models"
+	"github.com/prometheus/alertmanager/provider"
+	"github.com/prometheus/alertmanager/types"
+	"github.com/prometheus/common/model"
 	"github.com/stretchr/testify/require"
 
+	apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
 	"github.com/grafana/grafana/pkg/setting"
 )
@@ -18,3 +28,265 @@ func TestAlertmanager_ShouldUseDefaultConfigurationWhenNoConfiguration(t *testin
 	require.NoError(t, am.SyncAndApplyConfigFromDatabase())
 	require.NotNil(t, am.config)
 }
+
+func TestPutAlert(t *testing.T) {
+	dir, err := ioutil.TempDir("", "")
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		require.NoError(t, os.RemoveAll(dir))
+	})
+
+	am := &Alertmanager{
+		Settings: &setting.Cfg{
+			DataPath: dir,
+		},
+	}
+	require.NoError(t, am.Init())
+
+	startTime := time.Now()
+	endTime := startTime.Add(2 * time.Hour)
+
+	cases := []struct {
+		title          string
+		postableAlerts apimodels.PostableAlerts
+		expAlerts      func(now time.Time) []*types.Alert
+		expError       *AlertValidationError
+	}{
+		{
+			title: "Valid alerts with different start/end set",
+			postableAlerts: apimodels.PostableAlerts{
+				PostableAlerts: []models.PostableAlert{
+					{ // Start and end set.
+						Annotations: models.LabelSet{"msg": "Alert1 annotation"},
+						Alert: models.Alert{
+							Labels:       models.LabelSet{"alertname": "Alert1"},
+							GeneratorURL: "http://localhost/url1",
+						},
+						StartsAt: strfmt.DateTime(startTime),
+						EndsAt:   strfmt.DateTime(endTime),
+					}, { // Only end is set.
+						Annotations: models.LabelSet{"msg": "Alert2 annotation"},
+						Alert: models.Alert{
+							Labels:       models.LabelSet{"alertname": "Alert2"},
+							GeneratorURL: "http://localhost/url2",
+						},
+						StartsAt: strfmt.DateTime{},
+						EndsAt:   strfmt.DateTime(endTime),
+					}, { // Only start is set.
+						Annotations: models.LabelSet{"msg": "Alert3 annotation"},
+						Alert: models.Alert{
+							Labels:       models.LabelSet{"alertname": "Alert3"},
+							GeneratorURL: "http://localhost/url3",
+						},
+						StartsAt: strfmt.DateTime(startTime),
+						EndsAt:   strfmt.DateTime{},
+					}, { // Both start and end are not set.
+						Annotations: models.LabelSet{"msg": "Alert4 annotation"},
+						Alert: models.Alert{
+							Labels:       models.LabelSet{"alertname": "Alert4"},
+							GeneratorURL: "http://localhost/url4",
+						},
+						StartsAt: strfmt.DateTime{},
+						EndsAt:   strfmt.DateTime{},
+					},
+				},
+			},
+			expAlerts: func(now time.Time) []*types.Alert {
+				return []*types.Alert{
+					{
+						Alert: model.Alert{
+							Annotations:  model.LabelSet{"msg": "Alert1 annotation"},
+							Labels:       model.LabelSet{"alertname": "Alert1"},
+							StartsAt:     startTime,
+							EndsAt:       endTime,
+							GeneratorURL: "http://localhost/url1",
+						},
+						UpdatedAt: now,
+					}, {
+						Alert: model.Alert{
+							Annotations:  model.LabelSet{"msg": "Alert2 annotation"},
+							Labels:       model.LabelSet{"alertname": "Alert2"},
+							StartsAt:     endTime,
+							EndsAt:       endTime,
+							GeneratorURL: "http://localhost/url2",
+						},
+						UpdatedAt: now,
+					}, {
+						Alert: model.Alert{
+							Annotations:  model.LabelSet{"msg": "Alert3 annotation"},
+							Labels:       model.LabelSet{"alertname": "Alert3"},
+							StartsAt:     startTime,
+							EndsAt:       now.Add(defaultResolveTimeout),
+							GeneratorURL: "http://localhost/url3",
+						},
+						UpdatedAt: now,
+						Timeout:   true,
+					}, {
+						Alert: model.Alert{
+							Annotations:  model.LabelSet{"msg": "Alert4 annotation"},
+							Labels:       model.LabelSet{"alertname": "Alert4"},
+							StartsAt:     now,
+							EndsAt:       now.Add(defaultResolveTimeout),
+							GeneratorURL: "http://localhost/url4",
+						},
+						UpdatedAt: now,
+						Timeout:   true,
+					},
+				}
+			},
+		}, {
+			title: "Removing empty labels and annotations",
+			postableAlerts: apimodels.PostableAlerts{
+				PostableAlerts: []models.PostableAlert{
+					{
+						Annotations: models.LabelSet{"msg": "Alert4 annotation", "empty": ""},
+						Alert: models.Alert{
+							Labels:       models.LabelSet{"alertname": "Alert4", "emptylabel": ""},
+							GeneratorURL: "http://localhost/url1",
+						},
+						StartsAt: strfmt.DateTime{},
+						EndsAt:   strfmt.DateTime{},
+					},
+				},
+			},
+			expAlerts: func(now time.Time) []*types.Alert {
+				return []*types.Alert{
+					{
+						Alert: model.Alert{
+							Annotations:  model.LabelSet{"msg": "Alert4 annotation"},
+							Labels:       model.LabelSet{"alertname": "Alert4"},
+							StartsAt:     now,
+							EndsAt:       now.Add(defaultResolveTimeout),
+							GeneratorURL: "http://localhost/url1",
+						},
+						UpdatedAt: now,
+						Timeout:   true,
+					},
+				}
+			},
+		}, {
+			title: "Invalid labels",
+			postableAlerts: apimodels.PostableAlerts{
+				PostableAlerts: []models.PostableAlert{
+					{
+						Alert: models.Alert{
+							Labels: models.LabelSet{"alertname$": "Alert1"},
+						},
+					},
+				},
+			},
+			expError: &AlertValidationError{
+				Alerts: []models.PostableAlert{
+					{
+						Alert: models.Alert{
+							Labels: models.LabelSet{"alertname$": "Alert1"},
+						},
+					},
+				},
+				Errors: []error{errors.New("invalid label set: invalid name \"alertname$\"")},
+			},
+		}, {
+			title: "Invalid annotation",
+			postableAlerts: apimodels.PostableAlerts{
+				PostableAlerts: []models.PostableAlert{
+					{
+						Annotations: models.LabelSet{"msg$": "Alert4 annotation"},
+						Alert: models.Alert{
+							Labels: models.LabelSet{"alertname": "Alert1"},
+						},
+					},
+				},
+			},
+			expError: &AlertValidationError{
+				Alerts: []models.PostableAlert{
+					{
+						Annotations: models.LabelSet{"msg$": "Alert4 annotation"},
+						Alert: models.Alert{
+							Labels: models.LabelSet{"alertname": "Alert1"},
+						},
+					},
+				},
+				Errors: []error{errors.New("invalid annotations: invalid name \"msg$\"")},
+			},
+		}, {
+			title: "No labels after removing empty",
+			postableAlerts: apimodels.PostableAlerts{
+				PostableAlerts: []models.PostableAlert{
+					{
+						Alert: models.Alert{
+							Labels: models.LabelSet{"alertname": ""},
+						},
+					},
+				},
+			},
+			expError: &AlertValidationError{
+				Alerts: []models.PostableAlert{
+					{
+						Alert: models.Alert{
+							Labels: models.LabelSet{"alertname": ""},
+						},
+					},
+				},
+				Errors: []error{errors.New("at least one label pair required")},
+			},
+		}, {
+			title: "Start should be before end",
+			postableAlerts: apimodels.PostableAlerts{
+				PostableAlerts: []models.PostableAlert{
+					{
+						Alert: models.Alert{
+							Labels: models.LabelSet{"alertname": ""},
+						},
+						StartsAt: strfmt.DateTime(endTime),
+						EndsAt:   strfmt.DateTime(startTime),
+					},
+				},
+			},
+			expError: &AlertValidationError{
+				Alerts: []models.PostableAlert{
+					{
+						Alert: models.Alert{
+							Labels: models.LabelSet{"alertname": ""},
+						},
+						StartsAt: strfmt.DateTime(endTime),
+						EndsAt:   strfmt.DateTime(startTime),
+					},
+				},
+				Errors: []error{errors.New("start time must be before end time")},
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.title, func(t *testing.T) {
+			alertProvider := &mockAlertProvider{}
+			am.alerts = alertProvider
+
+			err := am.PutAlerts(c.postableAlerts)
+			if c.expError != nil {
+				require.Error(t, err)
+				require.Equal(t, c.expError, err)
+				require.Equal(t, 0, len(alertProvider.alerts))
+				return
+			}
+			require.NoError(t, err)
+
+			// We take the "now" time from one of the UpdatedAt.
+			now := alertProvider.alerts[0].UpdatedAt
+			require.Equal(t, c.expAlerts(now), alertProvider.alerts)
+		})
+	}
+}
+
+type mockAlertProvider struct {
+	alerts []*types.Alert
+}
+
+func (a *mockAlertProvider) Subscribe() provider.AlertIterator           { return nil }
+func (a *mockAlertProvider) GetPending() provider.AlertIterator          { return nil }
+func (a *mockAlertProvider) Get(model.Fingerprint) (*types.Alert, error) { return nil, nil }
+
+func (a *mockAlertProvider) Put(alerts ...*types.Alert) error {
+	a.alerts = append(a.alerts, alerts...)
+	return nil
+}