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

@@ -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) {

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)
+	})
+}

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":