alerting: no notification when going from nodata -> pending (#16905)

ref #16496

pkg/services/alerting/eval_context_test.go

@@ -6,6 +6,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/grafana/grafana/pkg/models"
 )
 
@@ -195,12 +197,10 @@ func TestGetStateFromEvalContext(t *testing.T) {
 	}
 
 	for _, tc := range tcs {
-		ctx := NewEvalContext(context.TODO(), &Rule{Conditions: []Condition{&conditionStub{firing: true}}})
+		evalContext := NewEvalContext(context.Background(), &Rule{Conditions: []Condition{&conditionStub{firing: true}}})
 
-		tc.applyFn(ctx)
-		have := ctx.GetNewState()
-		if have != tc.expected {
-			t.Errorf("failed: %s \n expected '%s' have '%s'\n", tc.name, tc.expected, string(have))
-		}
+		tc.applyFn(evalContext)
+		newState := evalContext.GetNewState()
+		assert.Equal(t, tc.expected, newState, "failed: %s \n expected '%s' have '%s'\n", tc.name, tc.expected, string(newState))
 	}
 }

pkg/services/alerting/notifiers/base.go

@@ -48,12 +48,15 @@ func NewNotifierBase(model *models.AlertNotification) NotifierBase {
 
 // ShouldNotify checks this evaluation should send an alert notification
 func (n *NotifierBase) ShouldNotify(ctx context.Context, context *alerting.EvalContext, notiferState *models.AlertNotificationState) bool {
+	prevState := context.PrevAlertState
+	newState := context.Rule.State
+
 	// Only notify on state change.
-	if context.PrevAlertState == context.Rule.State && !n.SendReminder {
+	if prevState == newState && !n.SendReminder {
 		return false
 	}
 
-	if context.PrevAlertState == context.Rule.State && n.SendReminder {
+	if prevState == newState && n.SendReminder {
 		// Do not notify if interval has not elapsed
 		lastNotify := time.Unix(notiferState.UpdatedAt, 0)
 		if notiferState.UpdatedAt != 0 && lastNotify.Add(n.Frequency).After(time.Now()) {
@@ -61,32 +64,35 @@ func (n *NotifierBase) ShouldNotify(ctx context.Context, context *alerting.EvalC
 		}
 
 		// Do not notify if alert state is OK or pending even on repeated notify
-		if context.Rule.State == models.AlertStateOK || context.Rule.State == models.AlertStatePending {
+		if newState == models.AlertStateOK || newState == models.AlertStatePending {
 			return false
 		}
 	}
 
-	// Do not notify when we become OK for the first time.
-	if context.PrevAlertState == models.AlertStateUnknown && context.Rule.State == models.AlertStateOK {
+	unknownOrNoData := prevState == models.AlertStateUnknown || prevState == models.AlertStateNoData
+	okOrPending := newState == models.AlertStatePending || newState == models.AlertStateOK
+
+	// Do not notify when new state is ok/pending when previous is unknown or no_data
+	if unknownOrNoData && okOrPending {
 		return false
 	}
 
-	// Do not notify when we become OK for the first time.
-	if context.PrevAlertState == models.AlertStateUnknown && context.Rule.State == models.AlertStatePending {
+	// Do not notify when we become Pending for the first
+	if prevState == models.AlertStateNoData && newState == models.AlertStatePending {
 		return false
 	}
 
 	// Do not notify when we become OK from pending
-	if context.PrevAlertState == models.AlertStatePending && context.Rule.State == models.AlertStateOK {
+	if prevState == models.AlertStatePending && newState == models.AlertStateOK {
 		return false
 	}
 
 	// Do not notify when we OK -> Pending
-	if context.PrevAlertState == models.AlertStateOK && context.Rule.State == models.AlertStatePending {
+	if prevState == models.AlertStateOK && newState == models.AlertStatePending {
 		return false
 	}
 
-	// Do not notifu if state pending and it have been updated last minute
+	// Do not notify if state pending and it have been updated last minute
 	if notiferState.State == models.AlertNotificationStatePending {
 		lastUpdated := time.Unix(notiferState.UpdatedAt, 0)
 		if lastUpdated.Add(1 * time.Minute).After(time.Now()) {
@@ -95,7 +101,7 @@ func (n *NotifierBase) ShouldNotify(ctx context.Context, context *alerting.EvalC
 	}
 
 	// Do not notify when state is OK if DisableResolveMessage is set to true
-	if context.Rule.State == models.AlertStateOK && n.DisableResolveMessage {
+	if newState == models.AlertStateOK && n.DisableResolveMessage {
 		return false
 	}
 

pkg/services/alerting/notifiers/base_test.go

@@ -5,8 +5,10 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/grafana/grafana/pkg/components/simplejson"
-	m "github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/alerting"
 	. "github.com/smartystreets/goconvey/convey"
 )
@@ -16,76 +18,76 @@ func TestShouldSendAlertNotification(t *testing.T) {
 
 	tcs := []struct {
 		name         string
-		prevState    m.AlertStateType
-		newState     m.AlertStateType
+		prevState    models.AlertStateType
+		newState     models.AlertStateType
 		sendReminder bool
 		frequency    time.Duration
-		state        *m.AlertNotificationState
+		state        *models.AlertNotificationState
 
 		expect bool
 	}{
 		{
 			name:         "pending -> ok should not trigger an notification",
-			newState:     m.AlertStateOK,
-			prevState:    m.AlertStatePending,
+			newState:     models.AlertStateOK,
+			prevState:    models.AlertStatePending,
 			sendReminder: false,
 
 			expect: false,
 		},
 		{
 			name:         "ok -> alerting should trigger an notification",
-			newState:     m.AlertStateAlerting,
-			prevState:    m.AlertStateOK,
+			newState:     models.AlertStateAlerting,
+			prevState:    models.AlertStateOK,
 			sendReminder: false,
 
 			expect: true,
 		},
 		{
 			name:         "ok -> pending should not trigger an notification",
-			newState:     m.AlertStatePending,
-			prevState:    m.AlertStateOK,
+			newState:     models.AlertStatePending,
+			prevState:    models.AlertStateOK,
 			sendReminder: false,
 
 			expect: false,
 		},
 		{
 			name:         "ok -> ok should not trigger an notification",
-			newState:     m.AlertStateOK,
-			prevState:    m.AlertStateOK,
+			newState:     models.AlertStateOK,
+			prevState:    models.AlertStateOK,
 			sendReminder: false,
 
 			expect: false,
 		},
 		{
 			name:         "ok -> ok with reminder should not trigger an notification",
-			newState:     m.AlertStateOK,
-			prevState:    m.AlertStateOK,
+			newState:     models.AlertStateOK,
+			prevState:    models.AlertStateOK,
 			sendReminder: true,
 
 			expect: false,
 		},
 		{
 			name:         "alerting -> ok should trigger an notification",
-			newState:     m.AlertStateOK,
-			prevState:    m.AlertStateAlerting,
+			newState:     models.AlertStateOK,
+			prevState:    models.AlertStateAlerting,
 			sendReminder: false,
 
 			expect: true,
 		},
 		{
 			name:         "alerting -> ok should trigger an notification when reminders enabled",
-			newState:     m.AlertStateOK,
-			prevState:    m.AlertStateAlerting,
+			newState:     models.AlertStateOK,
+			prevState:    models.AlertStateAlerting,
 			frequency:    time.Minute * 10,
 			sendReminder: true,
-			state:        &m.AlertNotificationState{UpdatedAt: tnow.Add(-time.Minute).Unix()},
+			state:        &models.AlertNotificationState{UpdatedAt: tnow.Add(-time.Minute).Unix()},
 
 			expect: true,
 		},
 		{
 			name:         "alerting -> alerting with reminder and no state should trigger",
-			newState:     m.AlertStateAlerting,
-			prevState:    m.AlertStateAlerting,
+			newState:     models.AlertStateAlerting,
+			prevState:    models.AlertStateAlerting,
 			frequency:    time.Minute * 10,
 			sendReminder: true,
 
@@ -93,78 +95,84 @@ func TestShouldSendAlertNotification(t *testing.T) {
 		},
 		{
 			name:         "alerting -> alerting with reminder and last notification sent 1 minute ago should not trigger",
-			newState:     m.AlertStateAlerting,
-			prevState:    m.AlertStateAlerting,
+			newState:     models.AlertStateAlerting,
+			prevState:    models.AlertStateAlerting,
 			frequency:    time.Minute * 10,
 			sendReminder: true,
-			state:        &m.AlertNotificationState{UpdatedAt: tnow.Add(-time.Minute).Unix()},
+			state:        &models.AlertNotificationState{UpdatedAt: tnow.Add(-time.Minute).Unix()},
 
 			expect: false,
 		},
 		{
 			name:         "alerting -> alerting with reminder and last notifciation sent 11 minutes ago should trigger",
-			newState:     m.AlertStateAlerting,
-			prevState:    m.AlertStateAlerting,
+			newState:     models.AlertStateAlerting,
+			prevState:    models.AlertStateAlerting,
 			frequency:    time.Minute * 10,
 			sendReminder: true,
-			state:        &m.AlertNotificationState{UpdatedAt: tnow.Add(-11 * time.Minute).Unix()},
+			state:        &models.AlertNotificationState{UpdatedAt: tnow.Add(-11 * time.Minute).Unix()},
 
 			expect: true,
 		},
 		{
 			name:      "OK -> alerting with notifciation state pending and updated 30 seconds ago should not trigger",
-			newState:  m.AlertStateAlerting,
-			prevState: m.AlertStateOK,
-			state:     &m.AlertNotificationState{State: m.AlertNotificationStatePending, UpdatedAt: tnow.Add(-30 * time.Second).Unix()},
+			newState:  models.AlertStateAlerting,
+			prevState: models.AlertStateOK,
+			state:     &models.AlertNotificationState{State: models.AlertNotificationStatePending, UpdatedAt: tnow.Add(-30 * time.Second).Unix()},
 
 			expect: false,
 		},
 		{
 			name:      "OK -> alerting with notifciation state pending and updated 2 minutes ago should trigger",
-			newState:  m.AlertStateAlerting,
-			prevState: m.AlertStateOK,
-			state:     &m.AlertNotificationState{State: m.AlertNotificationStatePending, UpdatedAt: tnow.Add(-2 * time.Minute).Unix()},
+			newState:  models.AlertStateAlerting,
+			prevState: models.AlertStateOK,
+			state:     &models.AlertNotificationState{State: models.AlertNotificationStatePending, UpdatedAt: tnow.Add(-2 * time.Minute).Unix()},
 
 			expect: true,
 		},
 		{
 			name:      "unknown -> ok",
-			prevState: m.AlertStateUnknown,
-			newState:  m.AlertStateOK,
+			prevState: models.AlertStateUnknown,
+			newState:  models.AlertStateOK,
 
 			expect: false,
 		},
 		{
 			name:      "unknown -> pending",
-			prevState: m.AlertStateUnknown,
-			newState:  m.AlertStatePending,
+			prevState: models.AlertStateUnknown,
+			newState:  models.AlertStatePending,
 
 			expect: false,
 		},
 		{
 			name:      "unknown -> alerting",
-			prevState: m.AlertStateUnknown,
-			newState:  m.AlertStateAlerting,
+			prevState: models.AlertStateUnknown,
+			newState:  models.AlertStateAlerting,
 
 			expect: true,
 		},
+		{
+			name:      "no_data -> pending",
+			prevState: models.AlertStateNoData,
+			newState:  models.AlertStatePending,
+
+			expect: false,
+		},
 	}
 
 	for _, tc := range tcs {
-		evalContext := alerting.NewEvalContext(context.TODO(), &alerting.Rule{
+		evalContext := alerting.NewEvalContext(context.Background(), &alerting.Rule{
 			State: tc.prevState,
 		})
 
 		if tc.state == nil {
-			tc.state = &m.AlertNotificationState{}
+			tc.state = &models.AlertNotificationState{}
 		}
 
 		evalContext.Rule.State = tc.newState
 		nb := &NotifierBase{SendReminder: tc.sendReminder, Frequency: tc.frequency}
 
-		if nb.ShouldNotify(evalContext.Ctx, evalContext, tc.state) != tc.expect {
-			t.Errorf("failed test %s.\n expected \n%+v \nto return: %v", tc.name, tc, tc.expect)
-		}
+		r := nb.ShouldNotify(evalContext.Ctx, evalContext, tc.state)
+		assert.Equal(t, r, tc.expect, "failed test %s. expected %+v to return: %v", tc.name, tc, tc.expect)
 	}
 }
 
@@ -172,7 +180,7 @@ func TestBaseNotifier(t *testing.T) {
 	Convey("default constructor for notifiers", t, func() {
 		bJson := simplejson.New()
 
-		model := &m.AlertNotification{
+		model := &models.AlertNotification{
 			Uid:      "1",
 			Name:     "name",
 			Type:     "email",