From 4af420f759a47305f87f6eee067195b2ec3a7bfc Mon Sep 17 00:00:00 2001 From: bergquist Date: Thu, 3 Nov 2016 15:26:17 +0100 Subject: [PATCH] tech(alerting): refactor how evalhandler uses conditions --- pkg/services/alerting/conditions/query.go | 17 +++++--- .../alerting/conditions/query_test.go | 40 +++++++++---------- pkg/services/alerting/eval_handler.go | 12 +++++- pkg/services/alerting/eval_handler_test.go | 11 ++--- pkg/services/alerting/interfaces.go | 10 ++++- pkg/services/alerting/rule_test.go | 4 +- 6 files changed, 58 insertions(+), 36 deletions(-) diff --git a/pkg/services/alerting/conditions/query.go b/pkg/services/alerting/conditions/query.go index 7cf255c6af8..b73db9d590e 100644 --- a/pkg/services/alerting/conditions/query.go +++ b/pkg/services/alerting/conditions/query.go @@ -33,16 +33,17 @@ type AlertQuery struct { To string } -func (c *QueryCondition) Eval(context *alerting.EvalContext) { +func (c *QueryCondition) Eval(context *alerting.EvalContext) (*alerting.ConditionResult, error) { timeRange := tsdb.NewTimeRange(c.Query.From, c.Query.To) + seriesList, err := c.executeQuery(context, timeRange) if err != nil { - context.Error = err - return + return nil, err } emptySerieCount := 0 evalMatchCount := 0 + var matches []*alerting.EvalMatch for _, series := range seriesList { reducedValue := c.Reducer.Reduce(series) evalMatch := c.Evaluator.Eval(reducedValue) @@ -60,15 +61,19 @@ func (c *QueryCondition) Eval(context *alerting.EvalContext) { if evalMatch { evalMatchCount++ - context.EvalMatches = append(context.EvalMatches, &alerting.EvalMatch{ + + matches = append(matches, &alerting.EvalMatch{ Metric: series.Name, Value: reducedValue.Float64, }) } } - context.NoDataFound = emptySerieCount == len(seriesList) - context.Firing = evalMatchCount > 0 + return &alerting.ConditionResult{ + Firing: evalMatchCount > 0, + NoDataFound: emptySerieCount == len(seriesList), + EvalMatches: matches, + }, nil } func (c *QueryCondition) executeQuery(context *alerting.EvalContext, timeRange *tsdb.TimeRange) (tsdb.TimeSeriesSlice, error) { diff --git a/pkg/services/alerting/conditions/query_test.go b/pkg/services/alerting/conditions/query_test.go index 43e0381a80c..c3797beaf37 100644 --- a/pkg/services/alerting/conditions/query_test.go +++ b/pkg/services/alerting/conditions/query_test.go @@ -46,19 +46,19 @@ func TestQueryCondition(t *testing.T) { Convey("should fire when avg is above 100", func() { points := tsdb.NewTimeSeriesPointsFromArgs(120, 0) ctx.series = tsdb.TimeSeriesSlice{tsdb.NewTimeSeries("test1", points)} - ctx.exec() + cr, err := ctx.exec() - So(ctx.result.Error, ShouldBeNil) - So(ctx.result.Firing, ShouldBeTrue) + So(err, ShouldBeNil) + So(cr.Firing, ShouldBeTrue) }) Convey("Should not fire when avg is below 100", func() { points := tsdb.NewTimeSeriesPointsFromArgs(90, 0) ctx.series = tsdb.TimeSeriesSlice{tsdb.NewTimeSeries("test1", points)} - ctx.exec() + cr, err := ctx.exec() - So(ctx.result.Error, ShouldBeNil) - So(ctx.result.Firing, ShouldBeFalse) + So(err, ShouldBeNil) + So(cr.Firing, ShouldBeFalse) }) Convey("Should fire if only first serie matches", func() { @@ -66,10 +66,10 @@ func TestQueryCondition(t *testing.T) { tsdb.NewTimeSeries("test1", tsdb.NewTimeSeriesPointsFromArgs(120, 0)), tsdb.NewTimeSeries("test2", tsdb.NewTimeSeriesPointsFromArgs(0, 0)), } - ctx.exec() + cr, err := ctx.exec() - So(ctx.result.Error, ShouldBeNil) - So(ctx.result.Firing, ShouldBeTrue) + So(err, ShouldBeNil) + So(cr.Firing, ShouldBeTrue) }) Convey("Empty series", func() { @@ -78,10 +78,10 @@ func TestQueryCondition(t *testing.T) { tsdb.NewTimeSeries("test1", tsdb.NewTimeSeriesPointsFromArgs()), tsdb.NewTimeSeries("test2", tsdb.NewTimeSeriesPointsFromArgs()), } - ctx.exec() + cr, err := ctx.exec() - So(ctx.result.Error, ShouldBeNil) - So(ctx.result.NoDataFound, ShouldBeTrue) + So(err, ShouldBeNil) + So(cr.NoDataFound, ShouldBeTrue) }) Convey("Should set NoDataFound both series contains null", func() { @@ -89,10 +89,10 @@ func TestQueryCondition(t *testing.T) { tsdb.NewTimeSeries("test1", tsdb.TimeSeriesPoints{tsdb.TimePoint{null.FloatFromPtr(nil), null.FloatFrom(0)}}), tsdb.NewTimeSeries("test2", tsdb.TimeSeriesPoints{tsdb.TimePoint{null.FloatFromPtr(nil), null.FloatFrom(0)}}), } - ctx.exec() + cr, err := ctx.exec() - So(ctx.result.Error, ShouldBeNil) - So(ctx.result.NoDataFound, ShouldBeTrue) + So(err, ShouldBeNil) + So(cr.NoDataFound, ShouldBeTrue) }) Convey("Should not set NoDataFound if one serie is empty", func() { @@ -100,10 +100,10 @@ func TestQueryCondition(t *testing.T) { tsdb.NewTimeSeries("test1", tsdb.NewTimeSeriesPointsFromArgs()), tsdb.NewTimeSeries("test2", tsdb.NewTimeSeriesPointsFromArgs(120, 0)), } - ctx.exec() + cr, err := ctx.exec() - So(ctx.result.Error, ShouldBeNil) - So(ctx.result.NoDataFound, ShouldBeFalse) + So(err, ShouldBeNil) + So(cr.NoDataFound, ShouldBeFalse) }) }) }) @@ -120,7 +120,7 @@ type queryConditionTestContext struct { type queryConditionScenarioFunc func(c *queryConditionTestContext) -func (ctx *queryConditionTestContext) exec() { +func (ctx *queryConditionTestContext) exec() (*alerting.ConditionResult, error) { jsonModel, err := simplejson.NewJson([]byte(`{ "type": "query", "query": { @@ -146,7 +146,7 @@ func (ctx *queryConditionTestContext) exec() { }, nil } - condition.Eval(ctx.result) + return condition.Eval(ctx.result) } func queryConditionScenario(desc string, fn queryConditionScenarioFunc) { diff --git a/pkg/services/alerting/eval_handler.go b/pkg/services/alerting/eval_handler.go index 74054ba8191..538c639abb8 100644 --- a/pkg/services/alerting/eval_handler.go +++ b/pkg/services/alerting/eval_handler.go @@ -20,8 +20,12 @@ func NewEvalHandler() *DefaultEvalHandler { } func (e *DefaultEvalHandler) Eval(context *EvalContext) { + firing := true for _, condition := range context.Rule.Conditions { - condition.Eval(context) + cr, err := condition.Eval(context) + if err != nil { + context.Error = err + } // break if condition could not be evaluated if context.Error != nil { @@ -29,11 +33,15 @@ func (e *DefaultEvalHandler) Eval(context *EvalContext) { } // break if result has not triggered yet - if context.Firing == false { + if cr.Firing == false { + firing = false break } + + context.EvalMatches = append(context.EvalMatches, cr.EvalMatches...) } + context.Firing = firing context.EndTime = time.Now() elapsedTime := context.EndTime.Sub(context.StartTime) / time.Millisecond metrics.M_Alerting_Exeuction_Time.Update(elapsedTime) diff --git a/pkg/services/alerting/eval_handler_test.go b/pkg/services/alerting/eval_handler_test.go index b69e62f9622..4c2ec24b506 100644 --- a/pkg/services/alerting/eval_handler_test.go +++ b/pkg/services/alerting/eval_handler_test.go @@ -8,11 +8,12 @@ import ( ) type conditionStub struct { - firing bool + firing bool + matches []*EvalMatch } -func (c *conditionStub) Eval(context *EvalContext) { - context.Firing = c.firing +func (c *conditionStub) Eval(context *EvalContext) (*ConditionResult, error) { + return &ConditionResult{Firing: c.firing, EvalMatches: c.matches}, nil } func TestAlertingExecutor(t *testing.T) { @@ -30,10 +31,10 @@ func TestAlertingExecutor(t *testing.T) { So(context.Firing, ShouldEqual, true) }) - Convey("Show return false with not passing condition", func() { + Convey("Show return false with not passing asdf", func() { context := NewEvalContext(context.TODO(), &Rule{ Conditions: []Condition{ - &conditionStub{firing: true}, + &conditionStub{firing: true, matches: []*EvalMatch{&EvalMatch{}, &EvalMatch{}}}, &conditionStub{firing: false}, }, }) diff --git a/pkg/services/alerting/interfaces.go b/pkg/services/alerting/interfaces.go index 583e12a120d..cc2561473e3 100644 --- a/pkg/services/alerting/interfaces.go +++ b/pkg/services/alerting/interfaces.go @@ -21,6 +21,12 @@ type Notifier interface { GetIsDefault() bool } -type Condition interface { - Eval(result *EvalContext) +type ConditionResult struct { + Firing bool + NoDataFound bool + EvalMatches []*EvalMatch +} + +type Condition interface { + Eval(result *EvalContext) (*ConditionResult, error) } diff --git a/pkg/services/alerting/rule_test.go b/pkg/services/alerting/rule_test.go index 6144c01d54d..f8761efcd90 100644 --- a/pkg/services/alerting/rule_test.go +++ b/pkg/services/alerting/rule_test.go @@ -10,7 +10,9 @@ import ( type FakeCondition struct{} -func (f *FakeCondition) Eval(context *EvalContext) {} +func (f *FakeCondition) Eval(context *EvalContext) (*ConditionResult, error) { + return &ConditionResult{}, nil +} func TestAlertRuleModel(t *testing.T) { Convey("Testing alert rule", t, func() {