From 4732f832f71aeef3a92ab68b84d2eb6687fe1404 Mon Sep 17 00:00:00 2001 From: David Parrott Date: Thu, 17 Jun 2021 10:01:46 -0700 Subject: [PATCH] Alerting: recalculate EndsAt (#35830) * setEndsAt * one more test case * add should clause to tests --- pkg/services/ngalert/state/manager.go | 8 +-- pkg/services/ngalert/state/state.go | 57 ++++++--------- pkg/services/ngalert/state/state_test.go | 92 ++++++++++++++++++++++++ 3 files changed, 118 insertions(+), 39 deletions(-) diff --git a/pkg/services/ngalert/state/manager.go b/pkg/services/ngalert/state/manager.go index 0e4ab699c6e..a71f2ee778a 100644 --- a/pkg/services/ngalert/state/manager.go +++ b/pkg/services/ngalert/state/manager.go @@ -83,13 +83,13 @@ func (st *Manager) setNextState(alertRule *ngModels.AlertRule, result eval.Resul st.Log.Debug("setting alert state", "uid", alertRule.UID) switch result.State { case eval.Normal: - currentState = currentState.resultNormal(result) + currentState.resultNormal(result) case eval.Alerting: - currentState = currentState.resultAlerting(alertRule, result) + currentState.resultAlerting(alertRule, result) case eval.Error: - currentState = currentState.resultError(alertRule, result) + currentState.resultError(alertRule, result) case eval.NoData: - currentState = currentState.resultNoData(alertRule, result) + currentState.resultNoData(alertRule, result) case eval.Pending: // we do not emit results with this state } diff --git a/pkg/services/ngalert/state/state.go b/pkg/services/ngalert/state/state.go index 08c913e2db3..121741bbe11 100644 --- a/pkg/services/ngalert/state/state.go +++ b/pkg/services/ngalert/state/state.go @@ -30,76 +30,54 @@ type Evaluation struct { EvaluationString string } -func (a *State) resultNormal(result eval.Result) *State { +func (a *State) resultNormal(result eval.Result) { if a.State != eval.Normal { a.EndsAt = result.EvaluatedAt a.StartsAt = result.EvaluatedAt } a.Error = result.Error // should be nil since state is not error a.State = eval.Normal - return a } -func (a *State) resultAlerting(alertRule *ngModels.AlertRule, result eval.Result) *State { +func (a *State) resultAlerting(alertRule *ngModels.AlertRule, result eval.Result) { switch a.State { case eval.Alerting: - if !(alertRule.For > 0) { - // If there is not For set, we will set EndsAt to be twice the evaluation interval - // to avoid flapping with every evaluation - a.EndsAt = result.EvaluatedAt.Add(time.Duration(alertRule.IntervalSeconds*2) * time.Second) - return a - } - a.EndsAt = result.EvaluatedAt.Add(alertRule.For) + a.setEndsAt(alertRule, result) case eval.Pending: if result.EvaluatedAt.Sub(a.StartsAt) > alertRule.For { a.State = eval.Alerting a.StartsAt = result.EvaluatedAt - a.EndsAt = result.EvaluatedAt.Add(alertRule.For) + a.setEndsAt(alertRule, result) } default: a.StartsAt = result.EvaluatedAt + a.setEndsAt(alertRule, result) if !(alertRule.For > 0) { - a.EndsAt = result.EvaluatedAt.Add(time.Duration(alertRule.IntervalSeconds*2) * time.Second) + // If For is 0, immediately set Alerting a.State = eval.Alerting } else { - a.EndsAt = result.EvaluatedAt.Add(alertRule.For) - if result.EvaluatedAt.Sub(a.StartsAt) > alertRule.For { - a.State = eval.Alerting - } else { - a.State = eval.Pending - } + a.State = eval.Pending } } - return a } -func (a *State) resultError(alertRule *ngModels.AlertRule, result eval.Result) *State { +func (a *State) resultError(alertRule *ngModels.AlertRule, result eval.Result) { a.Error = result.Error if a.StartsAt.IsZero() { a.StartsAt = result.EvaluatedAt } - if !(alertRule.For > 0) { - a.EndsAt = result.EvaluatedAt.Add(time.Duration(alertRule.IntervalSeconds*2) * time.Second) - } else { - a.EndsAt = result.EvaluatedAt.Add(alertRule.For) - } + a.setEndsAt(alertRule, result) - switch alertRule.ExecErrState { - case ngModels.AlertingErrState: + if alertRule.ExecErrState == ngModels.AlertingErrState { a.State = eval.Alerting } - return a } -func (a *State) resultNoData(alertRule *ngModels.AlertRule, result eval.Result) *State { +func (a *State) resultNoData(alertRule *ngModels.AlertRule, result eval.Result) { if a.StartsAt.IsZero() { a.StartsAt = result.EvaluatedAt } - if !(alertRule.For > 0) { - a.EndsAt = result.EvaluatedAt.Add(time.Duration(alertRule.IntervalSeconds*2) * time.Second) - } else { - a.EndsAt = result.EvaluatedAt.Add(alertRule.For) - } + a.setEndsAt(alertRule, result) switch alertRule.NoDataState { case ngModels.Alerting: @@ -109,7 +87,6 @@ func (a *State) resultNoData(alertRule *ngModels.AlertRule, result eval.Result) case ngModels.OK: a.State = eval.Normal } - return a } func (a *State) NeedsSending(resendDelay time.Duration) bool { @@ -147,3 +124,13 @@ func (a *State) TrimResults(alertRule *ngModels.AlertRule) { copy(newResults, a.Results[len(a.Results)-int(numBuckets):]) a.Results = newResults } + +func (a *State) setEndsAt(alertRule *ngModels.AlertRule, result eval.Result) { + if int64(alertRule.For.Seconds()) > alertRule.IntervalSeconds { + // For is set and longer than IntervalSeconds + a.EndsAt = result.EvaluatedAt.Add(alertRule.For) + } else { + // For is not set or is less than or equal to IntervalSeconds + a.EndsAt = result.EvaluatedAt.Add(time.Duration(alertRule.IntervalSeconds*2) * time.Second) + } +} diff --git a/pkg/services/ngalert/state/state_test.go b/pkg/services/ngalert/state/state_test.go index 46dcf517290..cf87f062457 100644 --- a/pkg/services/ngalert/state/state_test.go +++ b/pkg/services/ngalert/state/state_test.go @@ -4,6 +4,8 @@ import ( "testing" "time" + ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" + "github.com/grafana/grafana/pkg/services/ngalert/eval" "github.com/stretchr/testify/assert" @@ -73,3 +75,93 @@ func TestNeedsSending(t *testing.T) { }) } } + +func TestSetEndsAt(t *testing.T) { + evaluationTime, _ := time.Parse("2006-01-02", "2021-03-25") + testCases := []struct { + name string + expected time.Time + testState *State + testRule *ngmodels.AlertRule + testResult eval.Result + }{ + { + name: "For: unset Interval: 10s EndsAt should be evaluation time + 2X IntervalSeconds", + expected: evaluationTime.Add(20 * time.Second), + testState: &State{}, + testRule: &ngmodels.AlertRule{ + IntervalSeconds: 10, + }, + testResult: eval.Result{ + EvaluatedAt: evaluationTime, + }, + }, + { + name: "For: 0s Interval: 10s EndsAt should be evaluation time + 2X IntervalSeconds", + expected: evaluationTime.Add(20 * time.Second), + testState: &State{}, + testRule: &ngmodels.AlertRule{ + For: 0 * time.Second, + IntervalSeconds: 10, + }, + testResult: eval.Result{ + EvaluatedAt: evaluationTime, + }, + }, + { + name: "For: 1s Interval: 10s EndsAt should be evaluation time + 2X IntervalSeconds", + expected: evaluationTime.Add(20 * time.Second), + testState: &State{}, + testRule: &ngmodels.AlertRule{ + For: 0 * time.Second, + IntervalSeconds: 10, + }, + testResult: eval.Result{ + EvaluatedAt: evaluationTime, + }, + }, + { + name: "For: 10s Interval: 10s EndsAt should be evaluation time + 2X IntervalSeconds", + expected: evaluationTime.Add(20 * time.Second), + testState: &State{}, + testRule: &ngmodels.AlertRule{ + For: 10 * time.Second, + IntervalSeconds: 10, + }, + testResult: eval.Result{ + EvaluatedAt: evaluationTime, + }, + }, + { + name: "For: 11s Interval: 10s EndsAt should be evaluation time + For duration", + expected: evaluationTime.Add(11 * time.Second), + testState: &State{}, + testRule: &ngmodels.AlertRule{ + For: 11 * time.Second, + IntervalSeconds: 10, + }, + testResult: eval.Result{ + EvaluatedAt: evaluationTime, + }, + }, + { + name: "For: 20s Interval: 10s EndsAt should be evaluation time + For duration", + expected: evaluationTime.Add(20 * time.Second), + testState: &State{}, + testRule: &ngmodels.AlertRule{ + For: 20 * time.Second, + IntervalSeconds: 10, + }, + testResult: eval.Result{ + EvaluatedAt: evaluationTime, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tc.testState.setEndsAt(tc.testRule, tc.testResult) + assert.Equal(t, tc.expected, tc.testState.EndsAt) + }) + } +}