From 1df4d332c982dc5e394201bb2ef35b442727ce63 Mon Sep 17 00:00:00 2001 From: Kyle Brandt Date: Fri, 21 Jul 2023 06:38:29 -0400 Subject: [PATCH] SSE: Use errutil to show better error messages in prod (#71658) - include public message - propagate data source query errors so they are shown as well to which fixes #70026 --- pkg/expr/errors.go | 58 ++++++++++++++++++++++ pkg/expr/errors_test.go | 20 ++++++++ pkg/expr/ml.go | 11 +--- pkg/expr/ml_test.go | 2 +- pkg/expr/nodes.go | 31 +++--------- pkg/expr/nodes_test.go | 17 ++----- pkg/services/ngalert/state/manager_test.go | 28 +++-------- pkg/services/ngalert/state/state.go | 8 +-- 8 files changed, 104 insertions(+), 71 deletions(-) create mode 100644 pkg/expr/errors.go create mode 100644 pkg/expr/errors_test.go diff --git a/pkg/expr/errors.go b/pkg/expr/errors.go new file mode 100644 index 00000000000..6440ab3d64a --- /dev/null +++ b/pkg/expr/errors.go @@ -0,0 +1,58 @@ +package expr + +import ( + "errors" + + "github.com/grafana/grafana/pkg/util/errutil" +) + +var ConversionError = errutil.NewBase( + errutil.StatusBadRequest, + "sse.readDataError", +).MustTemplate( + "[{{ .Public.refId }}] got error: {{ .Error }}", + errutil.WithPublic( + "failed to read data from from query {{ .Public.refId }}: {{ .Public.error }}", + ), +) + +func MakeConversionError(refID string, err error) error { + data := errutil.TemplateData{ + // Conversion errors should only have meta information in errors + Public: map[string]interface{}{ + "refId": refID, + "error": err.Error(), + }, + Error: err, + } + return ConversionError.Build(data) +} + +var QueryError = errutil.NewBase( + errutil.StatusBadRequest, "sse.dataQueryError").MustTemplate( + "failed to execute query [{{ .Public.refId }}]: {{ .Error }}", + errutil.WithPublic( + "failed to execute query [{{ .Public.refId }}]: {{ .Public.error }}", + )) + +func MakeQueryError(refID, datasourceUID string, err error) error { + var pErr error + var utilErr errutil.Error + // See if this is grafana error, if so, grab public message + if errors.As(err, &utilErr) { + pErr = utilErr.Public() + } else { + pErr = err + } + + data := errutil.TemplateData{ + Public: map[string]interface{}{ + "refId": refID, + "datasourceUID": datasourceUID, + "error": pErr.Error(), + }, + Error: err, + } + + return QueryError.Build(data) +} diff --git a/pkg/expr/errors_test.go b/pkg/expr/errors_test.go new file mode 100644 index 00000000000..718d5383fcf --- /dev/null +++ b/pkg/expr/errors_test.go @@ -0,0 +1,20 @@ +package expr_test + +import ( + "errors" + "fmt" + "testing" + + "github.com/grafana/grafana/pkg/expr" + "github.com/grafana/grafana/pkg/util/errutil" + "github.com/stretchr/testify/require" +) + +func TestQueryErrorType(t *testing.T) { + qet := expr.QueryError + utilError := errutil.Error{} + qe := expr.MakeQueryError("A", "", fmt.Errorf("not work")) + + require.True(t, errors.Is(qe, qet)) + require.True(t, errors.As(qe, &utilError)) +} diff --git a/pkg/expr/ml.go b/pkg/expr/ml.go index 902a64aef88..33c9e653ea8 100644 --- a/pkg/expr/ml.go +++ b/pkg/expr/ml.go @@ -111,10 +111,7 @@ func (m *MLNode) Execute(ctx context.Context, now time.Time, _ mathexp.Vars, s * }) if err != nil { - return result, QueryError{ - RefID: m.refID, - Err: err, - } + return result, MakeQueryError(m.refID, "ml", err) } // data is not guaranteed to be specified. In this case simulate NoData scenario @@ -124,11 +121,7 @@ func (m *MLNode) Execute(ctx context.Context, now time.Time, _ mathexp.Vars, s * dataFrames, err := getResponseFrame(data, m.refID) if err != nil { - return mathexp.Results{}, QueryError{ - RefID: m.refID, - DatasourceUID: mlPluginID, - Err: err, - } + return mathexp.Results{}, MakeQueryError(m.refID, "ml", err) } // process the response the same way DSNode does. Use plugin ID as data source type. Semantically, they are the same. diff --git a/pkg/expr/ml_test.go b/pkg/expr/ml_test.go index b7f7771668c..272f57bddd6 100644 --- a/pkg/expr/ml_test.go +++ b/pkg/expr/ml_test.go @@ -213,7 +213,7 @@ func TestMLNodeExecute(t *testing.T) { } _, err := node.Execute(context.Background(), timeNow, nil, s) - require.IsType(t, err, QueryError{}) + require.IsType(t, err, MakeQueryError("A", "", expectedError{})) require.ErrorIs(t, err, cmd.Error) }) } diff --git a/pkg/expr/nodes.go b/pkg/expr/nodes.go index de664501944..eab6ff830a0 100644 --- a/pkg/expr/nodes.go +++ b/pkg/expr/nodes.go @@ -27,20 +27,6 @@ var ( logger = log.New("expr") ) -type QueryError struct { - RefID string - DatasourceUID string - Err error -} - -func (e QueryError) Error() string { - return fmt.Sprintf("failed to execute query %s: %s", e.RefID, e.Err) -} - -func (e QueryError) Unwrap() error { - return e.Err -} - // baseNode includes common properties used across DPNodes. type baseNode struct { id int64 @@ -254,24 +240,19 @@ func (dn *DSNode) Execute(ctx context.Context, now time.Time, _ mathexp.Vars, s resp, err := s.dataService.QueryData(ctx, req) if err != nil { - return mathexp.Results{}, QueryError{ - RefID: dn.refID, - DatasourceUID: dn.datasource.UID, - Err: err, - } + return mathexp.Results{}, MakeQueryError(dn.refID, dn.datasource.UID, err) } dataFrames, err := getResponseFrame(resp, dn.refID) if err != nil { - return mathexp.Results{}, QueryError{ - RefID: dn.refID, - DatasourceUID: dn.datasource.UID, - Err: err, - } + return mathexp.Results{}, MakeQueryError(dn.refID, dn.datasource.UID, err) } var result mathexp.Results responseType, result, err = convertDataFramesToResults(ctx, dataFrames, dn.datasource.Type, s, logger) + if err != nil { + err = MakeConversionError(dn.refID, err) + } return result, err } @@ -492,7 +473,7 @@ func extractNumberSet(frame *data.Frame) ([]mathexp.Number, error) { func WideToMany(frame *data.Frame, fixSeries func(series mathexp.Series, valueField *data.Field)) ([]mathexp.Series, error) { tsSchema := frame.TimeSeriesSchema() if tsSchema.Type != data.TimeSeriesTypeWide { - return nil, fmt.Errorf("input data must be a wide series but got type %s (input refid)", tsSchema.Type) + return nil, fmt.Errorf("input data must be a wide series but got type %s", tsSchema.Type) } if len(tsSchema.ValueIndices) == 1 { diff --git a/pkg/expr/nodes_test.go b/pkg/expr/nodes_test.go index 58ea03b60bb..f889e817c2e 100644 --- a/pkg/expr/nodes_test.go +++ b/pkg/expr/nodes_test.go @@ -26,28 +26,19 @@ func (e expectedError) Error() string { } func TestQueryError_Error(t *testing.T) { - e := QueryError{ - RefID: "A", - Err: errors.New("this is an error message"), - } - assert.EqualError(t, e, "failed to execute query A: this is an error message") + e := MakeQueryError("A", "", errors.New("this is an error message")) + assert.EqualError(t, e, "[sse.dataQueryError] failed to execute query [A]: this is an error message") } func TestQueryError_Unwrap(t *testing.T) { t.Run("errors.Is", func(t *testing.T) { expectedIsErr := errors.New("expected") - e := QueryError{ - RefID: "A", - Err: expectedIsErr, - } + e := MakeQueryError("A", "", expectedIsErr) assert.True(t, errors.Is(e, expectedIsErr)) }) t.Run("errors.As", func(t *testing.T) { - e := QueryError{ - RefID: "A", - Err: expectedError{}, - } + e := MakeQueryError("A", "", expectedError{}) var expectedAsError expectedError assert.True(t, errors.As(e, &expectedAsError)) }) diff --git a/pkg/services/ngalert/state/manager_test.go b/pkg/services/ngalert/state/manager_test.go index ec9081954c0..b58894506f0 100644 --- a/pkg/services/ngalert/state/manager_test.go +++ b/pkg/services/ngalert/state/manager_test.go @@ -1722,11 +1722,8 @@ func TestProcessEvalResults(t *testing.T) { }, { eval.Result{ - Instance: data.Labels{"instance_label": "test"}, - Error: expr.QueryError{ - RefID: "A", - Err: errors.New("this is an error"), - }, + Instance: data.Labels{"instance_label": "test"}, + Error: expr.MakeQueryError("A", "", errors.New("this is an error")), State: eval.Error, EvaluatedAt: evaluationTime.Add(10 * time.Second), EvaluationDuration: evaluationDuration, @@ -1750,10 +1747,7 @@ func TestProcessEvalResults(t *testing.T) { }, Values: make(map[string]float64), State: eval.Error, - Error: expr.QueryError{ - RefID: "A", - Err: errors.New("this is an error"), - }, + Error: expr.MakeQueryError("A", "", errors.New("this is an error")), Results: []state.Evaluation{ { EvaluationTime: evaluationTime, @@ -1770,7 +1764,7 @@ func TestProcessEvalResults(t *testing.T) { EndsAt: evaluationTime.Add(10 * time.Second).Add(state.ResendDelay * 3), LastEvaluationTime: evaluationTime.Add(10 * time.Second), EvaluationDuration: evaluationDuration, - Annotations: map[string]string{"annotation": "test", "Error": "failed to execute query A: this is an error"}, + Annotations: map[string]string{"annotation": "test", "Error": "[sse.dataQueryError] failed to execute query [A]: this is an error"}, }, }, }, @@ -1802,11 +1796,8 @@ func TestProcessEvalResults(t *testing.T) { }, { eval.Result{ - Instance: data.Labels{"instance_label": "test"}, - Error: expr.QueryError{ - RefID: "A", - Err: errors.New("this is an error"), - }, + Instance: data.Labels{"instance_label": "test"}, + Error: expr.MakeQueryError("A", "", errors.New("this is an error")), State: eval.Error, EvaluatedAt: evaluationTime.Add(10 * time.Second), EvaluationDuration: evaluationDuration, @@ -1878,11 +1869,8 @@ func TestProcessEvalResults(t *testing.T) { }, { eval.Result{ - Instance: data.Labels{"instance_label": "test"}, - Error: expr.QueryError{ - RefID: "A", - Err: errors.New("this is an error"), - }, + Instance: data.Labels{"instance_label": "test"}, + Error: expr.MakeQueryError("A", "", errors.New("this is an error")), State: eval.Error, EvaluatedAt: evaluationTime.Add(10 * time.Second), EvaluationDuration: evaluationDuration, diff --git a/pkg/services/ngalert/state/state.go b/pkg/services/ngalert/state/state.go index c11dab29f0d..6d8b9a2e9d0 100644 --- a/pkg/services/ngalert/state/state.go +++ b/pkg/services/ngalert/state/state.go @@ -17,6 +17,7 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/eval" "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/screenshot" + "github.com/grafana/grafana/pkg/util/errutil" ) type State struct { @@ -297,10 +298,11 @@ func resultError(state *State, rule *models.AlertRule, result eval.Result, logge state.Annotations["Error"] = result.Error.Error() // If the evaluation failed because a query returned an error then add the Ref ID and // Datasource UID as labels - var queryError expr.QueryError - if errors.As(state.Error, &queryError) { + var utilError errutil.Error + if errors.As(state.Error, &utilError) && + (errors.Is(state.Error, expr.QueryError) || errors.Is(state.Error, expr.ConversionError)) { for _, next := range rule.Data { - if next.RefID == queryError.RefID { + if next.RefID == utilError.PublicPayload["refId"].(string) { state.Labels["ref_id"] = next.RefID state.Labels["datasource_uid"] = next.DatasourceUID break