From 94e709fdcb7b6fb063f7fa84e6fa09b8375db5a7 Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Mon, 27 Jun 2022 17:40:44 -0400 Subject: [PATCH] Alerting: Simplify eval.Evaluator interface (#51463) * remove ExpressionService from argument list of Evaluator's methods --- pkg/services/ngalert/api/api.go | 11 ++--- pkg/services/ngalert/api/api_testing.go | 14 +++--- pkg/services/ngalert/api/api_testing_test.go | 8 ++-- pkg/services/ngalert/eval/eval.go | 33 +++++++------ pkg/services/ngalert/eval/evaluator_mock.go | 48 +++++++++---------- pkg/services/ngalert/ngalert.go | 4 +- pkg/services/ngalert/schedule/schedule.go | 15 +++--- .../ngalert/schedule/schedule_test.go | 2 +- .../ngalert/schedule/schedule_unit_test.go | 4 +- 9 files changed, 66 insertions(+), 73 deletions(-) diff --git a/pkg/services/ngalert/api/api.go b/pkg/services/ngalert/api/api.go index 57cb59e470b..d86dc462160 100644 --- a/pkg/services/ngalert/api/api.go +++ b/pkg/services/ngalert/api/api.go @@ -120,12 +120,11 @@ func (api *API) RegisterAPIEndpoints(m *metrics.API) { ), m) api.RegisterTestingApiEndpoints(NewForkedTestingApi( &TestingApiSrv{ - AlertingProxy: proxy, - ExpressionService: api.ExpressionService, - DatasourceCache: api.DatasourceCache, - log: logger, - accessControl: api.AccessControl, - evaluator: eval.NewEvaluator(api.Cfg, log.New("ngalert.eval"), api.DatasourceCache, api.SecretsService), + AlertingProxy: proxy, + DatasourceCache: api.DatasourceCache, + log: logger, + accessControl: api.AccessControl, + evaluator: eval.NewEvaluator(api.Cfg, log.New("ngalert.eval"), api.DatasourceCache, api.SecretsService, api.ExpressionService), }), m) api.RegisterConfigurationApiEndpoints(NewForkedConfiguration( &AdminSrv{ diff --git a/pkg/services/ngalert/api/api_testing.go b/pkg/services/ngalert/api/api_testing.go index b5fc1234424..32fd5e4e1a9 100644 --- a/pkg/services/ngalert/api/api_testing.go +++ b/pkg/services/ngalert/api/api_testing.go @@ -11,7 +11,6 @@ import ( "github.com/grafana/grafana-plugin-sdk-go/data" "github.com/grafana/grafana/pkg/api/response" - "github.com/grafana/grafana/pkg/expr" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" @@ -24,11 +23,10 @@ import ( type TestingApiSrv struct { *AlertingProxy - ExpressionService *expr.Service - DatasourceCache datasources.CacheService - log log.Logger - accessControl accesscontrol.AccessControl - evaluator eval.Evaluator + DatasourceCache datasources.CacheService + log log.Logger + accessControl accesscontrol.AccessControl + evaluator eval.Evaluator } func (srv TestingApiSrv) RouteTestGrafanaRuleConfig(c *models.ReqContext, body apimodels.TestRulePayload) response.Response { @@ -57,7 +55,7 @@ func (srv TestingApiSrv) RouteTestGrafanaRuleConfig(c *models.ReqContext, body a now = timeNow() } - evalResults, err := srv.evaluator.ConditionEval(&evalCond, now, srv.ExpressionService) + evalResults, err := srv.evaluator.ConditionEval(&evalCond, now) if err != nil { return ErrResp(http.StatusBadRequest, err, "Failed to evaluate conditions") } @@ -123,7 +121,7 @@ func (srv TestingApiSrv) RouteEvalQueries(c *models.ReqContext, cmd apimodels.Ev return ErrResp(http.StatusBadRequest, err, "invalid queries or expressions") } - evalResults, err := srv.evaluator.QueriesAndExpressionsEval(c.SignedInUser.OrgId, cmd.Data, now, srv.ExpressionService) + evalResults, err := srv.evaluator.QueriesAndExpressionsEval(c.SignedInUser.OrgId, cmd.Data, now) if err != nil { return ErrResp(http.StatusBadRequest, err, "Failed to evaluate queries and expressions") } diff --git a/pkg/services/ngalert/api/api_testing_test.go b/pkg/services/ngalert/api/api_testing_test.go index 8e8826afeef..ecebe2eedab 100644 --- a/pkg/services/ngalert/api/api_testing_test.go +++ b/pkg/services/ngalert/api/api_testing_test.go @@ -69,7 +69,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) { evaluator := &eval.FakeEvaluator{} var result []eval.Result - evaluator.EXPECT().ConditionEval(mock.Anything, mock.Anything, mock.Anything).Return(result, nil) + evaluator.EXPECT().ConditionEval(mock.Anything, mock.Anything).Return(result, nil) srv := createTestingApiSrv(ds, ac, evaluator) @@ -109,7 +109,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) { evaluator := &eval.FakeEvaluator{} var result []eval.Result - evaluator.EXPECT().ConditionEval(mock.Anything, mock.Anything, mock.Anything).Return(result, nil) + evaluator.EXPECT().ConditionEval(mock.Anything, mock.Anything).Return(result, nil) srv := createTestingApiSrv(ds, ac, evaluator) @@ -197,7 +197,7 @@ func TestRouteEvalQueries(t *testing.T) { }, }, } - evaluator.EXPECT().QueriesAndExpressionsEval(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(result, nil) + evaluator.EXPECT().QueriesAndExpressionsEval(mock.Anything, mock.Anything, mock.Anything).Return(result, nil) srv := createTestingApiSrv(ds, ac, evaluator) @@ -240,7 +240,7 @@ func TestRouteEvalQueries(t *testing.T) { }, }, } - evaluator.EXPECT().QueriesAndExpressionsEval(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(result, nil) + evaluator.EXPECT().QueriesAndExpressionsEval(mock.Anything, mock.Anything, mock.Anything).Return(result, nil) srv := createTestingApiSrv(ds, ac, evaluator) diff --git a/pkg/services/ngalert/eval/eval.go b/pkg/services/ngalert/eval/eval.go index e14ab7b4a84..c98abf8ff09 100644 --- a/pkg/services/ngalert/eval/eval.go +++ b/pkg/services/ngalert/eval/eval.go @@ -28,28 +28,31 @@ import ( //go:generate mockery --name Evaluator --structname FakeEvaluator --inpackage --filename evaluator_mock.go --with-expecter type Evaluator interface { // ConditionEval executes conditions and evaluates the result. - ConditionEval(condition *models.Condition, now time.Time, expressionService *expr.Service) (Results, error) + ConditionEval(condition *models.Condition, now time.Time) (Results, error) // QueriesAndExpressionsEval executes queries and expressions and returns the result. - QueriesAndExpressionsEval(orgID int64, data []models.AlertQuery, now time.Time, expressionService *expr.Service) (*backend.QueryDataResponse, error) + QueriesAndExpressionsEval(orgID int64, data []models.AlertQuery, now time.Time) (*backend.QueryDataResponse, error) } type evaluatorImpl struct { - cfg *setting.Cfg - log log.Logger - dataSourceCache datasources.CacheService - secretsService secrets.Service + cfg *setting.Cfg + log log.Logger + dataSourceCache datasources.CacheService + secretsService secrets.Service + expressionService *expr.Service } func NewEvaluator( cfg *setting.Cfg, log log.Logger, datasourceCache datasources.CacheService, - secretsService secrets.Service) Evaluator { + secretsService secrets.Service, + expressionService *expr.Service) Evaluator { return &evaluatorImpl{ - cfg: cfg, - log: log, - dataSourceCache: datasourceCache, - secretsService: secretsService, + cfg: cfg, + log: log, + dataSourceCache: datasourceCache, + secretsService: secretsService, + expressionService: expressionService, } } @@ -588,26 +591,26 @@ func (evalResults Results) AsDataFrame() data.Frame { } // ConditionEval executes conditions and evaluates the result. -func (e *evaluatorImpl) ConditionEval(condition *models.Condition, now time.Time, expressionService *expr.Service) (Results, error) { +func (e *evaluatorImpl) ConditionEval(condition *models.Condition, now time.Time) (Results, error) { alertCtx, cancelFn := context.WithTimeout(context.Background(), e.cfg.UnifiedAlerting.EvaluationTimeout) defer cancelFn() alertExecCtx := AlertExecCtx{OrgID: condition.OrgID, Ctx: alertCtx, ExpressionsEnabled: e.cfg.ExpressionsEnabled, Log: e.log} - execResult := executeCondition(alertExecCtx, condition, now, expressionService, e.dataSourceCache, e.secretsService) + execResult := executeCondition(alertExecCtx, condition, now, e.expressionService, e.dataSourceCache, e.secretsService) evalResults := evaluateExecutionResult(execResult, now) return evalResults, nil } // QueriesAndExpressionsEval executes queries and expressions and returns the result. -func (e *evaluatorImpl) QueriesAndExpressionsEval(orgID int64, data []models.AlertQuery, now time.Time, expressionService *expr.Service) (*backend.QueryDataResponse, error) { +func (e *evaluatorImpl) QueriesAndExpressionsEval(orgID int64, data []models.AlertQuery, now time.Time) (*backend.QueryDataResponse, error) { alertCtx, cancelFn := context.WithTimeout(context.Background(), e.cfg.UnifiedAlerting.EvaluationTimeout) defer cancelFn() alertExecCtx := AlertExecCtx{OrgID: orgID, Ctx: alertCtx, ExpressionsEnabled: e.cfg.ExpressionsEnabled, Log: e.log} - execResult, err := executeQueriesAndExpressions(alertExecCtx, data, now, expressionService, e.dataSourceCache, e.secretsService) + execResult, err := executeQueriesAndExpressions(alertExecCtx, data, now, e.expressionService, e.dataSourceCache, e.secretsService) if err != nil { return nil, fmt.Errorf("failed to execute conditions: %w", err) } diff --git a/pkg/services/ngalert/eval/evaluator_mock.go b/pkg/services/ngalert/eval/evaluator_mock.go index 70ff9b14871..ed3f46b7f2a 100644 --- a/pkg/services/ngalert/eval/evaluator_mock.go +++ b/pkg/services/ngalert/eval/evaluator_mock.go @@ -4,8 +4,6 @@ package eval import ( backend "github.com/grafana/grafana-plugin-sdk-go/backend" - expr "github.com/grafana/grafana/pkg/expr" - mock "github.com/stretchr/testify/mock" models "github.com/grafana/grafana/pkg/services/ngalert/models" @@ -26,13 +24,13 @@ func (_m *FakeEvaluator) EXPECT() *FakeEvaluator_Expecter { return &FakeEvaluator_Expecter{mock: &_m.Mock} } -// ConditionEval provides a mock function with given fields: condition, now, expressionService -func (_m *FakeEvaluator) ConditionEval(condition *models.Condition, now time.Time, expressionService *expr.Service) (Results, error) { - ret := _m.Called(condition, now, expressionService) +// ConditionEval provides a mock function with given fields: condition, now +func (_m *FakeEvaluator) ConditionEval(condition *models.Condition, now time.Time) (Results, error) { + ret := _m.Called(condition, now) var r0 Results - if rf, ok := ret.Get(0).(func(*models.Condition, time.Time, *expr.Service) Results); ok { - r0 = rf(condition, now, expressionService) + if rf, ok := ret.Get(0).(func(*models.Condition, time.Time) Results); ok { + r0 = rf(condition, now) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(Results) @@ -40,8 +38,8 @@ func (_m *FakeEvaluator) ConditionEval(condition *models.Condition, now time.Tim } var r1 error - if rf, ok := ret.Get(1).(func(*models.Condition, time.Time, *expr.Service) error); ok { - r1 = rf(condition, now, expressionService) + if rf, ok := ret.Get(1).(func(*models.Condition, time.Time) error); ok { + r1 = rf(condition, now) } else { r1 = ret.Error(1) } @@ -57,14 +55,13 @@ type FakeEvaluator_ConditionEval_Call struct { // ConditionEval is a helper method to define mock.On call // - condition *models.Condition // - now time.Time -// - expressionService *expr.Service -func (_e *FakeEvaluator_Expecter) ConditionEval(condition interface{}, now interface{}, expressionService interface{}) *FakeEvaluator_ConditionEval_Call { - return &FakeEvaluator_ConditionEval_Call{Call: _e.mock.On("ConditionEval", condition, now, expressionService)} +func (_e *FakeEvaluator_Expecter) ConditionEval(condition interface{}, now interface{}) *FakeEvaluator_ConditionEval_Call { + return &FakeEvaluator_ConditionEval_Call{Call: _e.mock.On("ConditionEval", condition, now)} } -func (_c *FakeEvaluator_ConditionEval_Call) Run(run func(condition *models.Condition, now time.Time, expressionService *expr.Service)) *FakeEvaluator_ConditionEval_Call { +func (_c *FakeEvaluator_ConditionEval_Call) Run(run func(condition *models.Condition, now time.Time)) *FakeEvaluator_ConditionEval_Call { _c.Call.Run(func(args mock.Arguments) { - run(args[0].(*models.Condition), args[1].(time.Time), args[2].(*expr.Service)) + run(args[0].(*models.Condition), args[1].(time.Time)) }) return _c } @@ -74,13 +71,13 @@ func (_c *FakeEvaluator_ConditionEval_Call) Return(_a0 Results, _a1 error) *Fake return _c } -// QueriesAndExpressionsEval provides a mock function with given fields: orgID, data, now, expressionService -func (_m *FakeEvaluator) QueriesAndExpressionsEval(orgID int64, data []models.AlertQuery, now time.Time, expressionService *expr.Service) (*backend.QueryDataResponse, error) { - ret := _m.Called(orgID, data, now, expressionService) +// QueriesAndExpressionsEval provides a mock function with given fields: orgID, data, now +func (_m *FakeEvaluator) QueriesAndExpressionsEval(orgID int64, data []models.AlertQuery, now time.Time) (*backend.QueryDataResponse, error) { + ret := _m.Called(orgID, data, now) var r0 *backend.QueryDataResponse - if rf, ok := ret.Get(0).(func(int64, []models.AlertQuery, time.Time, *expr.Service) *backend.QueryDataResponse); ok { - r0 = rf(orgID, data, now, expressionService) + if rf, ok := ret.Get(0).(func(int64, []models.AlertQuery, time.Time) *backend.QueryDataResponse); ok { + r0 = rf(orgID, data, now) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*backend.QueryDataResponse) @@ -88,8 +85,8 @@ func (_m *FakeEvaluator) QueriesAndExpressionsEval(orgID int64, data []models.Al } var r1 error - if rf, ok := ret.Get(1).(func(int64, []models.AlertQuery, time.Time, *expr.Service) error); ok { - r1 = rf(orgID, data, now, expressionService) + if rf, ok := ret.Get(1).(func(int64, []models.AlertQuery, time.Time) error); ok { + r1 = rf(orgID, data, now) } else { r1 = ret.Error(1) } @@ -106,14 +103,13 @@ type FakeEvaluator_QueriesAndExpressionsEval_Call struct { // - orgID int64 // - data []models.AlertQuery // - now time.Time -// - expressionService *expr.Service -func (_e *FakeEvaluator_Expecter) QueriesAndExpressionsEval(orgID interface{}, data interface{}, now interface{}, expressionService interface{}) *FakeEvaluator_QueriesAndExpressionsEval_Call { - return &FakeEvaluator_QueriesAndExpressionsEval_Call{Call: _e.mock.On("QueriesAndExpressionsEval", orgID, data, now, expressionService)} +func (_e *FakeEvaluator_Expecter) QueriesAndExpressionsEval(orgID interface{}, data interface{}, now interface{}) *FakeEvaluator_QueriesAndExpressionsEval_Call { + return &FakeEvaluator_QueriesAndExpressionsEval_Call{Call: _e.mock.On("QueriesAndExpressionsEval", orgID, data, now)} } -func (_c *FakeEvaluator_QueriesAndExpressionsEval_Call) Run(run func(orgID int64, data []models.AlertQuery, now time.Time, expressionService *expr.Service)) *FakeEvaluator_QueriesAndExpressionsEval_Call { +func (_c *FakeEvaluator_QueriesAndExpressionsEval_Call) Run(run func(orgID int64, data []models.AlertQuery, now time.Time)) *FakeEvaluator_QueriesAndExpressionsEval_Call { _c.Call.Run(func(args mock.Arguments) { - run(args[0].(int64), args[1].([]models.AlertQuery), args[2].(time.Time), args[3].(*expr.Service)) + run(args[0].(int64), args[1].([]models.AlertQuery), args[2].(time.Time)) }) return _c } diff --git a/pkg/services/ngalert/ngalert.go b/pkg/services/ngalert/ngalert.go index ae953825a4f..1ad04d91cbb 100644 --- a/pkg/services/ngalert/ngalert.go +++ b/pkg/services/ngalert/ngalert.go @@ -133,7 +133,7 @@ func (ng *AlertNG) init() error { BaseInterval: ng.Cfg.UnifiedAlerting.BaseInterval, Logger: ng.Log, MaxAttempts: ng.Cfg.UnifiedAlerting.MaxAttempts, - Evaluator: eval.NewEvaluator(ng.Cfg, ng.Log, ng.DataSourceCache, ng.SecretsService), + Evaluator: eval.NewEvaluator(ng.Cfg, ng.Log, ng.DataSourceCache, ng.SecretsService, ng.ExpressionService), InstanceStore: store, RuleStore: store, AdminConfigStore: store, @@ -152,7 +152,7 @@ func (ng *AlertNG) init() error { } stateManager := state.NewManager(ng.Log, ng.Metrics.GetStateMetrics(), appUrl, store, store, ng.dashboardService, ng.imageService, clock.New()) - scheduler := schedule.NewScheduler(schedCfg, ng.ExpressionService, appUrl, stateManager, ng.bus) + scheduler := schedule.NewScheduler(schedCfg, appUrl, stateManager, ng.bus) ng.stateManager = stateManager ng.schedule = scheduler diff --git a/pkg/services/ngalert/schedule/schedule.go b/pkg/services/ngalert/schedule/schedule.go index 0f88ea73cf3..9483cc4c72a 100644 --- a/pkg/services/ngalert/schedule/schedule.go +++ b/pkg/services/ngalert/schedule/schedule.go @@ -10,7 +10,6 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/events" - "github.com/grafana/grafana/pkg/expr" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/alerting" @@ -83,11 +82,10 @@ type schedule struct { evaluator eval.Evaluator - ruleStore store.RuleStore - instanceStore store.InstanceStore - adminConfigStore store.AdminConfigurationStore - orgStore store.OrgStore - expressionService *expr.Service + ruleStore store.RuleStore + instanceStore store.InstanceStore + adminConfigStore store.AdminConfigurationStore + orgStore store.OrgStore stateManager *state.Manager @@ -136,7 +134,7 @@ type SchedulerCfg struct { } // NewScheduler returns a new schedule. -func NewScheduler(cfg SchedulerCfg, expressionService *expr.Service, appURL *url.URL, stateManager *state.Manager, bus bus.Bus) *schedule { +func NewScheduler(cfg SchedulerCfg, appURL *url.URL, stateManager *state.Manager, bus bus.Bus) *schedule { ticker := alerting.NewTicker(cfg.C, cfg.BaseInterval, cfg.Metrics.Ticker) sch := schedule{ @@ -152,7 +150,6 @@ func NewScheduler(cfg SchedulerCfg, expressionService *expr.Service, appURL *url ruleStore: cfg.RuleStore, instanceStore: cfg.InstanceStore, orgStore: cfg.OrgStore, - expressionService: expressionService, adminConfigStore: cfg.AdminConfigStore, multiOrgNotifier: cfg.MultiOrgNotifier, metrics: cfg.Metrics, @@ -628,7 +625,7 @@ func (sch *schedule) ruleRoutine(grafanaCtx context.Context, key ngmodels.AlertR OrgID: r.OrgID, Data: r.Data, } - results, err := sch.evaluator.ConditionEval(&condition, e.scheduledAt, sch.expressionService) + results, err := sch.evaluator.ConditionEval(&condition, e.scheduledAt) dur := sch.clock.Now().Sub(start) evalTotal.Inc() evalDuration.Observe(dur.Seconds()) diff --git a/pkg/services/ngalert/schedule/schedule_test.go b/pkg/services/ngalert/schedule/schedule_test.go index 4b40331be7d..23842932326 100644 --- a/pkg/services/ngalert/schedule/schedule_test.go +++ b/pkg/services/ngalert/schedule/schedule_test.go @@ -165,7 +165,7 @@ func TestAlertingTicker(t *testing.T) { Scheme: "http", Host: "localhost", } - sched := schedule.NewScheduler(schedCfg, nil, appUrl, st, busmock.New()) + sched := schedule.NewScheduler(schedCfg, appUrl, st, busmock.New()) go func() { err := sched.Run(ctx) diff --git a/pkg/services/ngalert/schedule/schedule_unit_test.go b/pkg/services/ngalert/schedule/schedule_unit_test.go index 8a9708cb635..b7ba1d64799 100644 --- a/pkg/services/ngalert/schedule/schedule_unit_test.go +++ b/pkg/services/ngalert/schedule/schedule_unit_test.go @@ -949,7 +949,7 @@ func setupScheduler(t *testing.T, rs store.RuleStore, is store.InstanceStore, ac C: mockedClock, BaseInterval: time.Second, MaxAttempts: 1, - Evaluator: eval.NewEvaluator(&setting.Cfg{ExpressionsEnabled: true}, logger, nil, secretsService), + Evaluator: eval.NewEvaluator(&setting.Cfg{ExpressionsEnabled: true}, logger, nil, secretsService, expr.ProvideService(&setting.Cfg{ExpressionsEnabled: true}, nil, nil)), RuleStore: rs, InstanceStore: is, AdminConfigStore: acs, @@ -963,7 +963,7 @@ func setupScheduler(t *testing.T, rs store.RuleStore, is store.InstanceStore, ac Scheme: "http", Host: "localhost", } - return NewScheduler(schedCfg, expr.ProvideService(&setting.Cfg{ExpressionsEnabled: true}, nil, nil), appUrl, st, busmock.New()), mockedClock + return NewScheduler(schedCfg, appUrl, st, busmock.New()), mockedClock } // createTestAlertRule creates a dummy alert definition to be used by the tests.