From f7aa17f2e47958a777926a8e489f8c93c20c44f3 Mon Sep 17 00:00:00 2001 From: Alexander Akhmetov Date: Wed, 26 Mar 2025 11:46:49 +0100 Subject: [PATCH] Alerting: Add default values to AlertRule.Data queries in Prometheus conversion (#102843) What is this feature? Prometheus conversion: ensures that AlertRule.Data queries always have default parameters set (intervalMs, maxDataPoints). Without this, updates of the same rule can cause version increments. Why do we need this feature? Currently, when converting Prometheus rules to Grafana alerts, some default parameters are not explicitly set in the query model. This creates a problem during rule updates: When a user updates a rule that hasn't changed, we still detect differences in the AlertQuery.Model because the newly converted rules are missing the default fields, such as intervalMs and maxDataPoints. This causes unnecessary version increments of alert rules. --- pkg/services/ngalert/models/alert_query.go | 17 ++++-- pkg/services/ngalert/prom/convert_test.go | 37 ++++++++++++ pkg/services/ngalert/prom/query.go | 70 +++++++++++----------- 3 files changed, 84 insertions(+), 40 deletions(-) diff --git a/pkg/services/ngalert/models/alert_query.go b/pkg/services/ngalert/models/alert_query.go index c5d83c90bb8..e38c1610584 100644 --- a/pkg/services/ngalert/models/alert_query.go +++ b/pkg/services/ngalert/models/alert_query.go @@ -290,12 +290,10 @@ func (aq *AlertQuery) PreSave() error { return fmt.Errorf("failed to set query type to query model: %w", err) } - // override model - model, err := aq.GetModel() - if err != nil { + // Initialize defaults, which also overrides the model + if err := aq.InitDefaults(); err != nil { return err } - aq.Model = model isExpression, err := aq.IsExpression() if err != nil { @@ -307,3 +305,14 @@ func (aq *AlertQuery) PreSave() error { } return nil } + +// InitDefaults ensures all default parameters are set in the query model. +// This helps maintain consistent query models for comparisons. +func (aq *AlertQuery) InitDefaults() error { + model, err := aq.GetModel() + if err != nil { + return err + } + aq.Model = model + return nil +} diff --git a/pkg/services/ngalert/prom/convert_test.go b/pkg/services/ngalert/prom/convert_test.go index 479417bc6b8..d5e7aae559b 100644 --- a/pkg/services/ngalert/prom/convert_test.go +++ b/pkg/services/ngalert/prom/convert_test.go @@ -746,3 +746,40 @@ func TestPrometheusRulesToGrafana_KeepOriginalRuleDefinition(t *testing.T) { }) } } + +func TestQueryModelContainsRequiredParameters(t *testing.T) { + cfg := Config{ + DatasourceUID: "datasource-uid", + DatasourceType: datasources.DS_PROMETHEUS, + DefaultInterval: 1 * time.Minute, + } + converter, err := NewConverter(cfg) + require.NoError(t, err) + + promRule := PrometheusRule{ + Alert: "test-alert", + Expr: "up == 0", + } + + queries, err := converter.createQuery(promRule.Expr, false, PrometheusRuleGroup{}) + require.NoError(t, err) + require.Len(t, queries, 3) + + for _, query := range queries { + var model map[string]any + err = json.Unmarshal(query.Model, &model) + require.NoError(t, err) + + // Check intervalMs + intervalMs, exists := model["intervalMs"] + require.True(t, exists) + _, isNumber := intervalMs.(float64) + require.True(t, isNumber, "intervalMs should be a number") + + // Check maxDataPoints + maxDataPoints, exists := model["maxDataPoints"] + require.True(t, exists) + _, isNumber = maxDataPoints.(float64) + require.True(t, isNumber, "maxDataPoints should be a number") + } +} diff --git a/pkg/services/ngalert/prom/query.go b/pkg/services/ngalert/prom/query.go index 8e28b0ac3e7..7d57469246b 100644 --- a/pkg/services/ngalert/prom/query.go +++ b/pkg/services/ngalert/prom/query.go @@ -16,9 +16,35 @@ type CommonQueryModel struct { Type expr.QueryType `json:"type"` } +// createAlertQueryWithDefaults creates an AlertQuery with default values initialized. +func createAlertQueryWithDefaults(datasourceUID string, modelData any, refID string, relTimeRange *models.RelativeTimeRange, queryType string) (models.AlertQuery, error) { + modelJSON, err := json.Marshal(modelData) + if err != nil { + return models.AlertQuery{}, fmt.Errorf("failed to marshal query model: %w", err) + } + + query := models.AlertQuery{ + DatasourceUID: datasourceUID, + Model: modelJSON, + RefID: refID, + QueryType: queryType, + } + + // Set relative time range if provided + if relTimeRange != nil { + query.RelativeTimeRange = *relTimeRange + } + + if err := query.InitDefaults(); err != nil { + return models.AlertQuery{}, err + } + + return query, nil +} + func createQueryNode(datasourceUID, datasourceType, expr string, fromTimeRange, evaluationOffset time.Duration) (models.AlertQuery, error) { - modelData := map[string]interface{}{ - "datasource": map[string]interface{}{ + modelData := map[string]any{ + "datasource": map[string]any{ "type": datasourceType, "uid": datasourceUID, }, @@ -32,20 +58,12 @@ func createQueryNode(datasourceUID, datasourceType, expr string, fromTimeRange, modelData["queryType"] = "instant" } - modelJSON, err := json.Marshal(modelData) - if err != nil { - return models.AlertQuery{}, err + relTimeRange := models.RelativeTimeRange{ + From: models.Duration(fromTimeRange + evaluationOffset), + To: models.Duration(evaluationOffset), } - return models.AlertQuery{ - DatasourceUID: datasourceUID, - Model: modelJSON, - RefID: queryRefID, - RelativeTimeRange: models.RelativeTimeRange{ - From: models.Duration(fromTimeRange + evaluationOffset), - To: models.Duration(evaluationOffset), - }, - }, nil + return createAlertQueryWithDefaults(datasourceUID, modelData, queryRefID, &relTimeRange, datasourceType) } type MathQueryModel struct { @@ -70,17 +88,7 @@ func createMathNode() (models.AlertQuery, error) { }, } - modelJSON, err := json.Marshal(model) - if err != nil { - return models.AlertQuery{}, err - } - - return models.AlertQuery{ - DatasourceUID: expr.DatasourceUID, - Model: modelJSON, - RefID: prometheusMathRefID, - QueryType: string(model.Type), - }, nil + return createAlertQueryWithDefaults(expr.DatasourceUID, model, prometheusMathRefID, nil, string(expr.QueryTypeMath)) } type ThresholdQueryModel struct { @@ -113,15 +121,5 @@ func createThresholdNode() (models.AlertQuery, error) { }, } - modelJSON, err := json.Marshal(model) - if err != nil { - return models.AlertQuery{}, err - } - - return models.AlertQuery{ - DatasourceUID: expr.DatasourceUID, - Model: modelJSON, - RefID: thresholdRefID, - QueryType: string(model.Type), - }, nil + return createAlertQueryWithDefaults(expr.DatasourceUID, model, thresholdRefID, nil, string(expr.QueryTypeThreshold)) }