diff --git a/pkg/expr/errors.go b/pkg/expr/errors.go index 9835f51184c..6e2d3e3e89b 100644 --- a/pkg/expr/errors.go +++ b/pkg/expr/errors.go @@ -3,6 +3,8 @@ package expr import ( "errors" "fmt" + "sort" + "strings" "github.com/grafana/grafana/pkg/apimachinery/errutil" ) @@ -93,3 +95,31 @@ func makeUnexpectedNodeTypeError(refID, nodeType string) error { return UnexpectedNodeTypeError.Build(data) } + +var DuplicateStringColumnError = errutil.NewBase( + errutil.StatusBadRequest, "sse.duplicateStringColumns").MustTemplate( + "your SQL query returned {{ .Public.count }} rows with duplicate values across the string columns, which is not allowed for alerting. Examples: ({{ .Public.examples }}). Hint: use GROUP BY or aggregation (e.g. MAX(), AVG()) to return one row per unique combination.", + errutil.WithPublic("SQL query returned duplicate combinations of string column values. Use GROUP BY or aggregation to return one row per combination."), +) + +func makeDuplicateStringColumnError(examples []string) error { + const limit = 5 + sort.Strings(examples) + exampleStr := strings.Join(truncateExamples(examples, limit), ", ") + + return DuplicateStringColumnError.Build(errutil.TemplateData{ + Public: map[string]any{ + "examples": exampleStr, + "count": len(examples), + }, + }) +} + +func truncateExamples(examples []string, limit int) []string { + if len(examples) <= limit { + return examples + } + truncated := examples[:limit] + truncated = append(truncated, fmt.Sprintf("... and %d more", len(examples)-limit)) + return truncated +} diff --git a/pkg/expr/sql_command.go b/pkg/expr/sql_command.go index 138630534a8..270ccc3530c 100644 --- a/pkg/expr/sql_command.go +++ b/pkg/expr/sql_command.go @@ -182,6 +182,20 @@ func totalCells(frames []*data.Frame) (total int64) { return } +// extractNumberSetFromSQLForAlerting converts a data frame produced by a SQL expression +// into a slice of mathexp.Number values for use in alerting. +// +// This function enforces strict semantics: each row must have exactly one numeric value +// and a unique label set. If any label set appears more than once, an error is returned. +// +// It is the responsibility of the SQL query to ensure uniqueness — for example, by +// applying GROUP BY or aggregation clauses. This function will not deduplicate rows; +// it will reject the entire input if any duplicates are present. +// +// Returns an error if: +// - No numeric field is found. +// - More than one numeric field exists. +// - Any label set appears more than once. func extractNumberSetFromSQLForAlerting(frame *data.Frame) ([]mathexp.Number, error) { var ( numericField *data.Field @@ -202,7 +216,13 @@ func extractNumberSetFromSQLForAlerting(frame *data.Frame) ([]mathexp.Number, er return nil, fmt.Errorf("no numeric field found in frame") } - numbers := make([]mathexp.Number, frame.Rows()) + type row struct { + value float64 + labels data.Labels + } + rows := make([]row, 0, frame.Rows()) + counts := map[data.Fingerprint]int{} + labelMap := map[data.Fingerprint]string{} for i := 0; i < frame.Rows(); i++ { val, err := numericField.FloatAt(i) @@ -227,10 +247,32 @@ func extractNumberSetFromSQLForAlerting(frame *data.Frame) ([]mathexp.Number, er } } - n := mathexp.NewNumber(numericField.Name, labels) + fp := labels.Fingerprint() + counts[fp]++ + labelMap[fp] = labels.String() + + rows = append(rows, row{value: val, labels: labels}) + } + + // Check for any duplicates + duplicates := make([]string, 0) + for fp, count := range counts { + if count > 1 { + duplicates = append(duplicates, labelMap[fp]) + } + } + + if len(duplicates) > 0 { + return nil, makeDuplicateStringColumnError(duplicates) + } + + // Build final result + numbers := make([]mathexp.Number, 0, len(rows)) + for _, r := range rows { + n := mathexp.NewNumber(numericField.Name, r.labels) n.Frame.Fields[0].Config = numericField.Config - n.SetValue(&val) - numbers[i] = n + n.SetValue(&r.value) + numbers = append(numbers, n) } return numbers, nil diff --git a/pkg/expr/sql_command_alert_test.go b/pkg/expr/sql_command_alert_test.go index 179a203ec21..54fbef24085 100644 --- a/pkg/expr/sql_command_alert_test.go +++ b/pkg/expr/sql_command_alert_test.go @@ -1,6 +1,7 @@ package expr import ( + "fmt" "testing" "github.com/grafana/grafana-plugin-sdk-go/data" @@ -10,7 +11,6 @@ import ( func TestExtractNumberSetFromSQLForAlerting(t *testing.T) { t.Run("SingleRowNoLabels", func(t *testing.T) { input := data.NewFrame("", - data.NewField(SQLMetricFieldName, nil, []string{"cpu"}), // will be treated as a label data.NewField(SQLValueFieldName, nil, []*float64{fp(3.14)}), ) @@ -20,17 +20,15 @@ func TestExtractNumberSetFromSQLForAlerting(t *testing.T) { got := numbers[0] require.Equal(t, fp(3.14), got.GetFloat64Value()) - require.Equal(t, data.Labels{ - SQLMetricFieldName: "cpu", - }, got.GetLabels()) + require.Equal(t, data.Labels{}, got.GetLabels()) }) t.Run("TwoRowsWithLabelsAndDisplay", func(t *testing.T) { input := data.NewFrame("", data.NewField(SQLMetricFieldName, nil, []string{"cpu", "cpu"}), data.NewField(SQLValueFieldName, nil, []*float64{fp(1.0), fp(2.0)}), - data.NewField(SQLDisplayFieldName, nil, []*string{sp("CPU A"), sp("CPU A")}), - data.NewField("host", nil, []*string{sp("a"), sp("a")}), + data.NewField(SQLDisplayFieldName, nil, []*string{sp("CPU A"), sp("CPU B")}), + data.NewField("host", nil, []*string{sp("a"), sp("b")}), ) numbers, err := extractNumberSetFromSQLForAlerting(input) @@ -47,8 +45,8 @@ func TestExtractNumberSetFromSQLForAlerting(t *testing.T) { require.Equal(t, fp(2.0), numbers[1].GetFloat64Value()) require.Equal(t, data.Labels{ SQLMetricFieldName: "cpu", - SQLDisplayFieldName: "CPU A", - "host": "a", + SQLDisplayFieldName: "CPU B", + "host": "b", }, numbers[1].GetLabels()) }) @@ -78,3 +76,85 @@ func TestExtractNumberSetFromSQLForAlerting(t *testing.T) { }, numbers[1].GetLabels()) }) } + +func TestExtractNumberSetFromSQLForAlerting_Duplicates(t *testing.T) { + t.Run("AllDuplicates_ReturnsError", func(t *testing.T) { + input := data.NewFrame("", + data.NewField(SQLMetricFieldName, nil, []string{"cpu", "cpu"}), + data.NewField(SQLValueFieldName, nil, []*float64{fp(1.0), fp(2.0)}), + data.NewField("host", nil, []*string{sp("a"), sp("a")}), + ) + + numbers, err := extractNumberSetFromSQLForAlerting(input) + require.Error(t, err) + require.Nil(t, numbers) + require.Contains(t, err.Error(), "duplicate values across the string columns") + require.Contains(t, err.Error(), "host=a") + require.Contains(t, err.Error(), "GROUP BY or aggregation") + }) + + t.Run("SomeDuplicates_ReturnsError", func(t *testing.T) { + input := data.NewFrame("", + data.NewField(SQLMetricFieldName, nil, []string{"cpu", "cpu", "cpu"}), + data.NewField(SQLValueFieldName, nil, []*float64{fp(1.0), fp(2.0), fp(3.0)}), + data.NewField("host", nil, []*string{sp("a"), sp("a"), sp("b")}), + ) + + numbers, err := extractNumberSetFromSQLForAlerting(input) + require.Error(t, err) + require.Nil(t, numbers) + require.Contains(t, err.Error(), "duplicate values across the string columns") + require.Contains(t, err.Error(), "host=a") + require.Contains(t, err.Error(), "GROUP BY or aggregation") + }) + + t.Run("NoDuplicates_Succeeds", func(t *testing.T) { + input := data.NewFrame("", + data.NewField(SQLMetricFieldName, nil, []string{"cpu", "cpu"}), + data.NewField(SQLValueFieldName, nil, []*float64{fp(1.0), fp(2.0)}), + data.NewField("host", nil, []*string{sp("a"), sp("b")}), + ) + + numbers, err := extractNumberSetFromSQLForAlerting(input) + require.NoError(t, err) + require.Len(t, numbers, 2) + + require.Equal(t, data.Labels{ + SQLMetricFieldName: "cpu", + "host": "a", + }, numbers[0].GetLabels()) + require.Equal(t, data.Labels{ + SQLMetricFieldName: "cpu", + "host": "b", + }, numbers[1].GetLabels()) + }) + + t.Run("MoreThan10DuplicateSets_TruncatesErrorList", func(t *testing.T) { + const totalRows = 30 + labels := make([]string, totalRows) + values := make([]*float64, totalRows) + hosts := make([]*string, totalRows) + + for i := 0; i < totalRows; i++ { + labels[i] = "cpu" + values[i] = fp(float64(i + 1)) + h := fmt.Sprintf("host%d", i%15) // 15 distinct combos, each duplicated + hosts[i] = &h + } + + input := data.NewFrame("", + data.NewField(SQLMetricFieldName, nil, labels), + data.NewField(SQLValueFieldName, nil, values), + data.NewField("host", nil, hosts), + ) + + numbers, err := extractNumberSetFromSQLForAlerting(input) + require.Error(t, err) + require.Nil(t, numbers) + + require.Contains(t, err.Error(), "duplicate values across the string columns") + require.Contains(t, err.Error(), "Examples:") + require.Contains(t, err.Error(), "... and 10 more") + require.Contains(t, err.Error(), "GROUP BY or aggregation") + }) +}