From 2115e286f923ebc57e52c113b7bd03afb98e52f8 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Wed, 4 Sep 2024 15:43:34 +0200 Subject: [PATCH] Autoinstrumentation improvements (#1066) Clean up code/logic for error source and request status. Log partial data response errors. Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> Co-authored-by: Will Browne --------- Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> Co-authored-by: Will Browne --- backend/data_adapter.go | 69 +++++++------------ backend/data_adapter_test.go | 35 +++++++++- backend/error_source.go | 6 +- backend/error_source_test.go | 41 +++++++++++ .../apis/data/v0alpha1/openapi_test.go | 2 +- 5 files changed, 106 insertions(+), 47 deletions(-) diff --git a/backend/data_adapter.go b/backend/data_adapter.go index bd8078ecb..5c2d63508 100644 --- a/backend/data_adapter.go +++ b/backend/data_adapter.go @@ -29,34 +29,29 @@ func (a *dataSDKAdapter) QueryData(ctx context.Context, req *pluginv2.QueryDataR var innerErr error resp, innerErr = a.queryDataHandler.QueryData(ctx, parsedReq) - if resp == nil || len(resp.Responses) == 0 { - return RequestStatusFromError(innerErr), innerErr - } - - if isCancelledError(innerErr) { - return RequestStatusCancelled, nil - } - - if isHTTPTimeoutError(innerErr) { - return RequestStatusError, nil + status := RequestStatusFromQueryDataResponse(resp, innerErr) + if innerErr != nil { + return status, innerErr + } else if resp == nil { + return RequestStatusError, errors.New("both response and error are nil, but one must be provided") } + ctxLogger := Logger.FromContext(ctx) // Set downstream status source in the context if there's at least one response with downstream status source, // and if there's no plugin error - var hasPluginError bool - var hasDownstreamError bool - var hasCancelledError bool - var hasHTTPTimeoutError bool - for _, r := range resp.Responses { + var hasPluginError, hasDownstreamError bool + for refID, r := range resp.Responses { if r.Error == nil { continue } - if isCancelledError(r.Error) { - hasCancelledError = true + // if error source not set and the error is a downstream error, set error source to downstream. + if !r.ErrorSource.IsValid() && IsDownstreamError(r.Error) { + r.ErrorSource = ErrorSourceDownstream } - if isHTTPTimeoutError(r.Error) { - hasHTTPTimeoutError = true + + if !r.Status.IsValid() { + r.Status = statusFromError(r.Error) } if r.ErrorSource == ErrorSourceDownstream { @@ -64,43 +59,29 @@ func (a *dataSDKAdapter) QueryData(ctx context.Context, req *pluginv2.QueryDataR } else { hasPluginError = true } - } - if hasCancelledError { - if err := WithDownstreamErrorSource(ctx); err != nil { - return RequestStatusError, fmt.Errorf("failed to set downstream status source: %w", errors.Join(innerErr, err)) + logParams := []any{ + "refID", refID, + "status", int(r.Status), + "error", r.Error, + "statusSource", string(r.ErrorSource), } - return RequestStatusCancelled, nil - } - - if hasHTTPTimeoutError { - if err := WithDownstreamErrorSource(ctx); err != nil { - return RequestStatusError, fmt.Errorf("failed to set downstream status source: %w", errors.Join(innerErr, err)) - } - return RequestStatusError, nil + ctxLogger.Error("Partial data response error", logParams...) } // A plugin error has higher priority than a downstream error, // so set to downstream only if there's no plugin error - if hasDownstreamError && !hasPluginError { - if err := WithDownstreamErrorSource(ctx); err != nil { - return RequestStatusError, fmt.Errorf("failed to set downstream status source: %w", errors.Join(innerErr, err)) - } - return RequestStatusError, nil - } - if hasPluginError { if err := WithErrorSource(ctx, ErrorSourcePlugin); err != nil { return RequestStatusError, fmt.Errorf("failed to set plugin status source: %w", errors.Join(innerErr, err)) } - return RequestStatusError, nil - } - - if innerErr != nil { - return RequestStatusFromError(innerErr), innerErr + } else if hasDownstreamError { + if err := WithDownstreamErrorSource(ctx); err != nil { + return RequestStatusError, fmt.Errorf("failed to set downstream status source: %w", errors.Join(innerErr, err)) + } } - return RequestStatusOK, nil + return status, nil }) if err != nil { return nil, err diff --git a/backend/data_adapter_test.go b/backend/data_adapter_test.go index 57fd76152..f20ef284a 100644 --- a/backend/data_adapter_test.go +++ b/backend/data_adapter_test.go @@ -140,6 +140,7 @@ func TestQueryData(t *testing.T) { name string queryDataResponse *QueryDataResponse expErrorSource ErrorSource + expError bool }{ { name: `single downstream error should be "downstream" error source`, @@ -180,6 +181,32 @@ func TestQueryData(t *testing.T) { }, expErrorSource: ErrorSourcePlugin, }, + { + name: `single downstream error without error source should be "downstream" error source`, + queryDataResponse: &QueryDataResponse{ + Responses: map[string]DataResponse{ + "A": {Error: DownstreamErrorf("boom")}, + }, + }, + expErrorSource: ErrorSourceDownstream, + }, + { + name: `multiple downstream error without error source and single plugin error should be "plugin" error source`, + queryDataResponse: &QueryDataResponse{ + Responses: map[string]DataResponse{ + "A": {Error: DownstreamErrorf("boom")}, + "B": {Error: someErr}, + "C": {Error: DownstreamErrorf("boom")}, + }, + }, + expErrorSource: ErrorSourcePlugin, + }, + { + name: "nil queryDataResponse and nil error should throw error", + queryDataResponse: nil, + expErrorSource: ErrorSourcePlugin, + expError: true, + }, } { t.Run(tc.name, func(t *testing.T) { var actualCtx context.Context @@ -190,7 +217,13 @@ func TestQueryData(t *testing.T) { _, err := a.QueryData(context.Background(), &pluginv2.QueryDataRequest{ PluginContext: &pluginv2.PluginContext{}, }) - require.NoError(t, err) + + if tc.expError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + ss := errorSourceFromContext(actualCtx) require.Equal(t, tc.expErrorSource, ss) }) diff --git a/backend/error_source.go b/backend/error_source.go index 1df6a0dd2..8c157cf30 100644 --- a/backend/error_source.go +++ b/backend/error_source.go @@ -21,6 +21,10 @@ const ( DefaultErrorSource ErrorSource = ErrorSourcePlugin ) +func (es ErrorSource) IsValid() bool { + return es == ErrorSourceDownstream || es == ErrorSourcePlugin +} + // ErrorSourceFromStatus returns an [ErrorSource] based on provided HTTP status code. func ErrorSourceFromHTTPStatus(statusCode int) ErrorSource { switch statusCode { @@ -62,7 +66,7 @@ func IsDownstreamError(err error) bool { return true } - if isHTTPTimeoutError(err) { + if isHTTPTimeoutError(err) || isCancelledError(err) { return true } diff --git a/backend/error_source_test.go b/backend/error_source_test.go index 8724b5c33..94ebe1354 100644 --- a/backend/error_source_test.go +++ b/backend/error_source_test.go @@ -1,6 +1,7 @@ package backend import ( + "context" "errors" "fmt" "net" @@ -8,8 +9,18 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) +func TestErrorSource(t *testing.T) { + var es ErrorSource + require.False(t, es.IsValid()) + require.True(t, ErrorSourceDownstream.IsValid()) + require.True(t, ErrorSourcePlugin.IsValid()) +} + func TestIsDownstreamError(t *testing.T) { tcs := []struct { name string @@ -66,6 +77,36 @@ func TestIsDownstreamError(t *testing.T) { err: fmt.Errorf("other error"), expected: false, }, + { + name: "context.Canceled", + err: context.Canceled, + expected: true, + }, + { + name: "wrapped context.Canceled", + err: fmt.Errorf("error: %w", context.Canceled), + expected: true, + }, + { + name: "joined context.Canceled", + err: errors.Join(fmt.Errorf("oh no"), context.Canceled), + expected: true, + }, + { + name: "gRPC canceled error", + err: status.Error(codes.Canceled, "canceled"), + expected: true, + }, + { + name: "wrapped gRPC canceled error", + err: fmt.Errorf("error: %w", status.Error(codes.Canceled, "canceled")), + expected: true, + }, + { + name: "joined gRPC canceled error", + err: errors.Join(fmt.Errorf("oh no"), status.Error(codes.Canceled, "canceled")), + expected: true, + }, } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { diff --git a/experimental/apis/data/v0alpha1/openapi_test.go b/experimental/apis/data/v0alpha1/openapi_test.go index f219be0ea..39bb46088 100644 --- a/experimental/apis/data/v0alpha1/openapi_test.go +++ b/experimental/apis/data/v0alpha1/openapi_test.go @@ -42,5 +42,5 @@ func TestOpenAPI(t *testing.T) { // Ensure DataSourceRef exists and has three properties def, ok = defs["github.com/grafana/grafana-plugin-sdk-go/experimental/apis/data/v0alpha1.DataSourceRef"] require.True(t, ok) - require.Equal(t, []string{"type", "uid", "apiVersion"}, maps.Keys(def.Schema.Properties)) + require.ElementsMatch(t, []string{"type", "uid", "apiVersion"}, maps.Keys(def.Schema.Properties)) }