SQL Expressions / Alerting: Do not allow duplicates (#103394)

3 months ago · b1490a10e8
parent 3766deed34
commit b1490a10e8
3 changed files with 164 additions and 12 deletions
--- 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
+}
--- 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
--- 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")
+	})
+}