From aa7a6633b8c3c63ed06d8dc4042e18ed6c90960e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A1bor=20Farkas?= Date: Tue, 31 Aug 2021 14:49:30 +0200 Subject: [PATCH] old-alert: set interval and max-data-points (#38434) * alerting: old-alerting: handle interval + maxdatapoints better * fixed failing test * refactor: use interval instead of tsdb * fix typo * added unit-test * added comment * refactor --- pkg/services/alerting/conditions/query.go | 48 +++++- .../conditions/query_interval_test.go | 162 ++++++++++++++++++ pkg/tsdb/interval/interval.go | 4 +- 3 files changed, 205 insertions(+), 9 deletions(-) create mode 100644 pkg/services/alerting/conditions/query_interval_test.go diff --git a/pkg/services/alerting/conditions/query.go b/pkg/services/alerting/conditions/query.go index 68797b8582b..e7efbae16fb 100644 --- a/pkg/services/alerting/conditions/query.go +++ b/pkg/services/alerting/conditions/query.go @@ -7,6 +7,7 @@ import ( "time" "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/tsdb/interval" "github.com/grafana/grafana/pkg/tsdb/prometheus" gocontext "context" @@ -108,6 +109,28 @@ func (c *QueryCondition) Eval(context *alerting.EvalContext, requestHandler plug }, nil } +func calculateInterval(timeRange plugins.DataTimeRange, model *simplejson.Json, dsInfo *models.DataSource) (time.Duration, error) { + // interval.GetIntervalFrom has two problems (but they do not affect us here): + // - it returns the min-interval, so it should be called interval.GetMinIntervalFrom + // - it falls back to model.intervalMs. it should not, because that one is the real final + // interval-value calculated by the browser. but, in this specific case (old-alert), + // that value is not set, so the fallback never happens. + minInterval, err := interval.GetIntervalFrom(dsInfo, model, time.Duration(0)) + + if err != nil { + return time.Duration(0), err + } + + calc := interval.NewCalculator() + + interval, err := calc.Calculate(timeRange, minInterval, "min") + if err != nil { + return time.Duration(0), err + } + + return interval.Value, nil +} + func (c *QueryCondition) executeQuery(context *alerting.EvalContext, timeRange plugins.DataTimeRange, requestHandler plugins.DataRequestHandler) (plugins.DataTimeSeriesSlice, error) { getDsInfo := &models.GetDataSourceQuery{ @@ -124,7 +147,10 @@ func (c *QueryCondition) executeQuery(context *alerting.EvalContext, timeRange p return nil, fmt.Errorf("access denied: %w", err) } - req := c.getRequestForAlertRule(getDsInfo.Result, timeRange, context.IsDebug) + req, err := c.getRequestForAlertRule(getDsInfo.Result, timeRange, context.IsDebug) + if err != nil { + return nil, fmt.Errorf("interval calculation failed: %w", err) + } result := make(plugins.DataTimeSeriesSlice, 0) if context.IsDebug { @@ -220,16 +246,24 @@ func (c *QueryCondition) executeQuery(context *alerting.EvalContext, timeRange p } func (c *QueryCondition) getRequestForAlertRule(datasource *models.DataSource, timeRange plugins.DataTimeRange, - debug bool) plugins.DataQuery { + debug bool) (plugins.DataQuery, error) { queryModel := c.Query.Model + + calculatedInterval, err := calculateInterval(timeRange, queryModel, datasource) + if err != nil { + return plugins.DataQuery{}, err + } + req := plugins.DataQuery{ TimeRange: &timeRange, Queries: []plugins.DataSubQuery{ { - RefID: "A", - Model: queryModel, - DataSource: datasource, - QueryType: queryModel.Get("queryType").MustString(""), + RefID: "A", + Model: queryModel, + DataSource: datasource, + QueryType: queryModel.Get("queryType").MustString(""), + MaxDataPoints: interval.DefaultRes, + IntervalMS: calculatedInterval.Milliseconds(), }, }, Headers: map[string]string{ @@ -238,7 +272,7 @@ func (c *QueryCondition) getRequestForAlertRule(datasource *models.DataSource, t Debug: debug, } - return req + return req, nil } func newQueryCondition(model *simplejson.Json, index int) (*QueryCondition, error) { diff --git a/pkg/services/alerting/conditions/query_interval_test.go b/pkg/services/alerting/conditions/query_interval_test.go new file mode 100644 index 00000000000..72b9d3e76dd --- /dev/null +++ b/pkg/services/alerting/conditions/query_interval_test.go @@ -0,0 +1,162 @@ +package conditions + +import ( + "context" + "testing" + + "github.com/grafana/grafana/pkg/services/validations" + "github.com/grafana/grafana/pkg/tsdb/interval" + + "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/services/alerting" + . "github.com/smartystreets/goconvey/convey" +) + +// the time-range is 5m (300seconds) for every test-case, +// maxDataPoints is 1500 for every test-case, +// so the interval for this simple case should be 300s/1500 = 200ms, +// but in some cases this is overridden by dashboard-panel-min-interval +// or by datasource-min-interval + +func TestQueryInterval(t *testing.T) { + Convey("When evaluating query condition, regarding the interval value", t, func() { + Convey("Can handle interval-calculation with no panel-min-interval and no datasource-min-interval", func() { + // no panel-min-interval in the queryModel + queryModel := `{"target": "aliasByNode(statsd.fakesite.counters.session_start.mobile.count, 4)"}` + + // no datasource-min-interval + var dataSourceJson *simplejson.Json = nil + + verifier := func(query plugins.DataSubQuery) { + // 5minutes timerange = 300000milliseconds; default-resolution is 1500pixels, + // so we should have 300000/1500 = 200milliseconds here + So(query.IntervalMS, ShouldEqual, 200) + So(query.MaxDataPoints, ShouldEqual, interval.DefaultRes) + } + + applyScenario(dataSourceJson, queryModel, verifier) + }) + Convey("Can handle interval-calculation with panel-min-interval and no datasource-min-interval", func() { + // panel-min-interval in the queryModel + queryModel := `{"interval":"123s", "target": "aliasByNode(statsd.fakesite.counters.session_start.mobile.count, 4)"}` + + // no datasource-min-interval + var dataSourceJson *simplejson.Json = nil + + verifier := func(query plugins.DataSubQuery) { + So(query.IntervalMS, ShouldEqual, 123000) + So(query.MaxDataPoints, ShouldEqual, interval.DefaultRes) + } + + applyScenario(dataSourceJson, queryModel, verifier) + }) + Convey("Can handle interval-calculation with no panel-min-interval and datasource-min-interval", func() { + // no panel-min-interval in the queryModel + queryModel := `{"target": "aliasByNode(statsd.fakesite.counters.session_start.mobile.count, 4)"}` + + // min-interval in datasource-json + dataSourceJson, err := simplejson.NewJson([]byte(`{ + "timeInterval": "71s" + }`)) + So(err, ShouldBeNil) + + verifier := func(query plugins.DataSubQuery) { + So(query.IntervalMS, ShouldEqual, 71000) + So(query.MaxDataPoints, ShouldEqual, interval.DefaultRes) + } + + applyScenario(dataSourceJson, queryModel, verifier) + }) + Convey("Can handle interval-calculation with both panel-min-interval and datasource-min-interval", func() { + // panel-min-interval in the queryModel + queryModel := `{"interval":"19s", "target": "aliasByNode(statsd.fakesite.counters.session_start.mobile.count, 4)"}` + + // min-interval in datasource-json + dataSourceJson, err := simplejson.NewJson([]byte(`{ + "timeInterval": "71s" + }`)) + So(err, ShouldBeNil) + + verifier := func(query plugins.DataSubQuery) { + // when both panel-min-interval and datasource-min-interval exists, + // panel-min-interval is used + So(query.IntervalMS, ShouldEqual, 19000) + So(query.MaxDataPoints, ShouldEqual, interval.DefaultRes) + } + + applyScenario(dataSourceJson, queryModel, verifier) + }) + }) +} + +type queryIntervalTestContext struct { + result *alerting.EvalContext + condition *QueryCondition +} + +type queryIntervalVerifier func(query plugins.DataSubQuery) + +type fakeIntervalTestReqHandler struct { + //nolint: staticcheck // plugins.DataResponse deprecated + response plugins.DataResponse + verifier queryIntervalVerifier +} + +//nolint: staticcheck // plugins.DataResponse deprecated +func (rh fakeIntervalTestReqHandler) HandleRequest(ctx context.Context, dsInfo *models.DataSource, query plugins.DataQuery) ( + plugins.DataResponse, error) { + q := query.Queries[0] + rh.verifier(q) + return rh.response, nil +} + +//nolint: staticcheck // plugins.DataResponse deprecated +func applyScenario(dataSourceJsonData *simplejson.Json, queryModel string, verifier func(query plugins.DataSubQuery)) { + Convey("desc", func() { + bus.AddHandler("test", func(query *models.GetDataSourceQuery) error { + query.Result = &models.DataSource{Id: 1, Type: "graphite", JsonData: dataSourceJsonData} + return nil + }) + + ctx := &queryIntervalTestContext{} + ctx.result = &alerting.EvalContext{ + Rule: &alerting.Rule{}, + RequestValidator: &validations.OSSPluginRequestValidator{}, + } + + jsonModel, err := simplejson.NewJson([]byte(`{ + "type": "query", + "query": { + "params": ["A", "5m", "now"], + "datasourceId": 1, + "model": ` + queryModel + ` + }, + "reducer":{"type": "avg"}, + "evaluator":{"type": "gt", "params": [100]} + }`)) + So(err, ShouldBeNil) + + condition, err := newQueryCondition(jsonModel, 0) + So(err, ShouldBeNil) + + ctx.condition = condition + + qr := plugins.DataQueryResult{} + + reqHandler := fakeIntervalTestReqHandler{ + response: plugins.DataResponse{ + Results: map[string]plugins.DataQueryResult{ + "A": qr, + }, + }, + verifier: verifier, + } + + _, err = condition.Eval(ctx.result, reqHandler) + + So(err, ShouldBeNil) + }) +} diff --git a/pkg/tsdb/interval/interval.go b/pkg/tsdb/interval/interval.go index c3a3cd057a6..71d97af68cf 100644 --- a/pkg/tsdb/interval/interval.go +++ b/pkg/tsdb/interval/interval.go @@ -13,7 +13,7 @@ import ( ) var ( - defaultRes int64 = 1500 + DefaultRes int64 = 1500 defaultMinInterval = time.Millisecond * 1 year = time.Hour * 24 * 365 day = time.Hour * 24 @@ -58,7 +58,7 @@ func (i *Interval) Milliseconds() int64 { func (ic *intervalCalculator) Calculate(timerange plugins.DataTimeRange, interval time.Duration, intervalMode string) (Interval, error) { to := timerange.MustGetTo().UnixNano() from := timerange.MustGetFrom().UnixNano() - calculatedInterval := time.Duration((to - from) / defaultRes) + calculatedInterval := time.Duration((to - from) / DefaultRes) switch intervalMode { case "min":