diff --git a/pkg/expr/graph.go b/pkg/expr/graph.go index d04ea82fe47..b241c534f0e 100644 --- a/pkg/expr/graph.go +++ b/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) diff --git a/pkg/expr/service_sql_test.go b/pkg/expr/service_sql_test.go index 55eb8db6524..5249737f4db 100644 --- a/pkg/expr/service_sql_test.go +++ b/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, - newABSQLQueries(`SELECT * FROM nonExisting`), + reg := prometheus.NewPedanticRegistry() + + 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)) }) } diff --git a/pkg/expr/service_test.go b/pkg/expr/service_test.go index 0fb46e18ddf..9edc98416d0 100644 --- a/pkg/expr/service_test.go +++ b/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) +}