SQL Expressions: Instrumentation fix and extra testing (#110778)

pkg/expr/graph.go

@@ -106,7 +106,7 @@ func (dp *DataPipeline) execute(c context.Context, now time.Time, s *Service) (m
 							// If it is already SQL error with type (e.g. limit exceeded, input conversion, capture the type as that)
 							eType = errWithType.Category()
 						}
-						s.metrics.SqlCommandCount.WithLabelValues("error", eType)
+						s.metrics.SqlCommandCount.WithLabelValues("error", eType).Inc()
 						depErr = e
 					} else { // general SSE dependency error
 						depErr = MakeDependencyError(node.RefID(), neededVar)
@@ -377,7 +377,6 @@ func (s *Service) buildGraphEdges(dp *simple.DirectedGraph, registry map[string]
 					// this missing dependency. But we collection the metric as there was an
 					// attempt to execute a SQL expression.
 					e := sql.MakeTableNotFoundError(cmdNode.refID, neededVar)
-					s.metrics.SqlCommandCount.WithLabelValues("error", e.Category()).Inc()
 					return e
 				}
 				return fmt.Errorf("unable to find dependent node '%v'", neededVar)

pkg/expr/service_sql_test.go

@@ -10,8 +10,10 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/grafana/grafana-plugin-sdk-go/backend"
 	"github.com/grafana/grafana-plugin-sdk-go/data"
+	"github.com/grafana/grafana/pkg/expr/sql"
 	"github.com/grafana/grafana/pkg/services/datasources"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
+	"github.com/prometheus/client_golang/prometheus"
 	"github.com/stretchr/testify/require"
 )
 
@@ -91,6 +93,9 @@ func TestSQLService(t *testing.T) {
 
 		require.Error(t, rsp.Responses["B"].Error, "should return invalid sql error")
 		require.ErrorContains(t, rsp.Responses["B"].Error, "not in the allowed list of")
+		var sqlErr *sql.ErrorWithCategory
+		require.ErrorAs(t, rsp.Responses["B"].Error, &sqlErr)
+		require.Equal(t, sql.ErrCategoryBlockedNodeOrFunc, sqlErr.Category())
 	})
 
 	t.Run("parse error should be returned", func(t *testing.T) {
@@ -108,6 +113,9 @@ func TestSQLService(t *testing.T) {
 
 		require.Error(t, rsp.Responses["B"].Error, "should return sql error on parsing")
 		require.ErrorContains(t, rsp.Responses["B"].Error, "limit expression expected to be numeric")
+		var sqlErr *sql.ErrorWithCategory
+		require.ErrorAs(t, rsp.Responses["B"].Error, &sqlErr)
+		require.Equal(t, sql.ErrCategoryGeneralGMSError, sqlErr.Category())
 	})
 }
 
@@ -170,8 +178,11 @@ func TestSQLServiceErrors(t *testing.T) {
 	}
 
 	t.Run("conversion failure (and therefore dependency error)", func(t *testing.T) {
-		s, req := newMockQueryService(resp,
+		reg := prometheus.NewPedanticRegistry()
+
+		s, req := newMockQueryServiceWithMetricsRegistry(resp,
 			newABSQLQueries(`SELECT * FROM tsMultiNoType`),
+			reg,
 		)
 
 		s.features = featuremgmt.WithFeatures(featuremgmt.FlagSqlExpressions)
@@ -184,20 +195,38 @@ func TestSQLServiceErrors(t *testing.T) {
 
 		require.Error(t, rsp.Responses["tsMultiNoType"].Error, "should return conversion error on DS response")
 		require.ErrorContains(t, rsp.Responses["tsMultiNoType"].Error, "missing the data type")
+		var sqlErr *sql.ErrorWithCategory
+		require.ErrorAs(t, rsp.Responses["tsMultiNoType"].Error, &sqlErr)
+		require.Equal(t, sql.ErrCategoryInputConversion, sqlErr.Category())
 
 		require.Error(t, rsp.Responses["sqlExpression"].Error, "should return dependency error")
 		require.ErrorContains(t, rsp.Responses["sqlExpression"].Error, "dependency")
+		require.ErrorAs(t, rsp.Responses["sqlExpression"].Error, &sqlErr)
+		require.Equal(t, sql.ErrCategoryDependency, sqlErr.Category())
+
+		require.Equal(t, 0.0, counterVal(t, s.metrics.SqlCommandCount, "ok", "none"))
+		require.Equal(t, 1.0, counterVal(t, s.metrics.SqlCommandCount, "error", sql.ErrCategoryInputConversion))
+		require.Equal(t, 1.0, counterVal(t, s.metrics.SqlCommandInputCount, "error", "false", "test", "missing"))
 	})
 
 	t.Run("pipeline (expressions and DS queries) will fail if the table is not found, before execution of the sql expression", func(t *testing.T) {
-		s, req := newMockQueryService(resp,
+		reg := prometheus.NewPedanticRegistry()
-			newABSQLQueries(`SELECT * FROM nonExisting`),
+
+		s, req := newMockQueryServiceWithMetricsRegistry(resp,
+			newABSQLQueries(`SELECT * FROM nonExisting`), reg,
 		)
 
 		s.features = featuremgmt.WithFeatures(featuremgmt.FlagSqlExpressions)
 
 		_, err := s.BuildPipeline(t.Context(), req)
+		var sqlErr *sql.ErrorWithCategory
+		require.ErrorAs(t, err, &sqlErr)
+		require.Equal(t, sql.ErrCategoryTableNotFound, sqlErr.Category())
 		require.Error(t, err, "whole pipeline fails when selecting a dependency that does not exist")
+
+		// Metrics
+		require.Equal(t, 0.0, counterVal(t, s.metrics.SqlCommandCount, "ok", "none"))
+		require.Equal(t, 1.0, counterVal(t, s.metrics.SqlCommandCount, "error", sql.ErrCategoryTableNotFound))
 	})
 
 	t.Run("pipeline will fail if query is longer than the configured limit", func(t *testing.T) {
@@ -209,5 +238,10 @@ func TestSQLServiceErrors(t *testing.T) {
 
 		_, err := s.BuildPipeline(t.Context(), req)
 		require.ErrorContains(t, err, "exceeded the configured limit of 5 characters")
+		var sqlErr *sql.ErrorWithCategory
+		require.ErrorAs(t, err, &sqlErr)
+		require.Equal(t, sql.ErrCategoryQueryTooLong, sqlErr.Category())
+
+		require.Equal(t, 1.0, counterVal(t, s.metrics.SqlCommandCount, "error", sql.ErrCategoryQueryTooLong))
 	})
 }

pkg/expr/service_test.go

@@ -11,6 +11,8 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/grafana/grafana-plugin-sdk-go/backend"
 	"github.com/grafana/grafana-plugin-sdk-go/data"
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/stretchr/testify/require"
 
 	"github.com/grafana/grafana/pkg/apimachinery/errutil"
@@ -279,3 +281,22 @@ func newMockQueryService(responses map[string]backend.DataResponse, queries []Qu
 		qsDatasourceClientBuilder: dsquerierclient.NewNullQSDatasourceClientBuilder(),
 	}, &Request{Queries: queries, User: &user.SignedInUser{}}
 }
+
+func newMockQueryServiceWithMetricsRegistry(
+	responses map[string]backend.DataResponse,
+	queries []Query,
+	reg *prometheus.Registry,
+) (*Service, *Request) {
+	s, req := newMockQueryService(responses, queries)
+	// Replace the default metrics with a set bound to our private registry.
+	s.metrics = metrics.NewSSEMetrics(reg)
+	return s, req
+}
+
+// Return the value of a prometheus counter with the given labels to test if it has been incremented, if the labels don't exist 0 will still be returned.
+func counterVal(t *testing.T, cv *prometheus.CounterVec, labels ...string) float64 {
+	t.Helper()
+	ch, err := cv.GetMetricWithLabelValues(labels...)
+	require.NoError(t, err)
+	return testutil.ToFloat64(ch)
+}