Remove errors.Cause() usage from store-gateway and its clients (#7213)

* Remove errors.Cause() usage from store-gateway and its clients Signed-off-by: Marco Pracucci <marco@pracucci.com> * Replace status.FromError() with grpcutil.ErrorToStatus() Signed-off-by: Marco Pracucci <marco@pracucci.com> --------- Signed-off-by: Marco Pracucci <marco@pracucci.com>
grafana · Jan 25, 2024 · fce7b38 · fce7b38
1 parent d609d3e
commit fce7b38
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 6 deletions.
diff --git a/pkg/querier/blocks_store_queryable.go b/pkg/querier/blocks_store_queryable.go
@@ -19,6 +19,7 @@ import (
 	"github.com/go-kit/log/level"
 	"github.com/gogo/protobuf/types"
 	"github.com/grafana/dskit/cancellation"
+	"github.com/grafana/dskit/grpcutil"
 	"github.com/grafana/dskit/kv"
 	"github.com/grafana/dskit/ring"
 	"github.com/grafana/dskit/services"
@@ -34,7 +35,6 @@ import (
 	"golang.org/x/exp/slices"
 	"golang.org/x/sync/errgroup"
 	grpc_metadata "google.golang.org/grpc/metadata"
-	"google.golang.org/grpc/status"
 
 	"github.com/grafana/mimir/pkg/querier/stats"
 	"github.com/grafana/mimir/pkg/storage/bucket"
@@ -927,7 +927,7 @@ func shouldStopQueryFunc(err error) bool {
 		return true
 	}
 
-	if st, ok := status.FromError(errors.Cause(err)); ok {
+	if st, ok := grpcutil.ErrorToStatus(err); ok {
 		if int(st.Code()) == http.StatusUnprocessableEntity {
 			return true
 		}

diff --git a/pkg/querier/blocks_store_queryable_test.go b/pkg/querier/blocks_store_queryable_test.go
@@ -13,12 +13,14 @@ import (
 	"math"
 	"math/rand"
 	"net"
+	"net/http"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/go-kit/log"
 	"github.com/gogo/protobuf/types"
+	gogoStatus "github.com/gogo/status"
 	"github.com/grafana/dskit/flagext"
 	"github.com/grafana/dskit/grpcclient"
 	"github.com/grafana/dskit/ring"
@@ -40,6 +42,7 @@ import (
 	"golang.org/x/exp/slices"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/metadata"
+	"google.golang.org/grpc/status"
 
 	"github.com/grafana/mimir/pkg/mimirpb"
 	"github.com/grafana/mimir/pkg/querier/stats"
@@ -2516,3 +2519,57 @@ func TestBlocksStoreQueryableErrMsgs(t *testing.T) {
 		})
 	}
 }
+
+func TestShouldStopQueryFunc(t *testing.T) {
+	tests := map[string]struct {
+		err      error
+		expected bool
+	}{
+		"should return true on context canceled": {
+			err:      context.Canceled,
+			expected: true,
+		},
+		"should return true on wrapped context canceled": {
+			err:      errors.Wrap(context.Canceled, "test"),
+			expected: true,
+		},
+		"should return true on deadline exceeded": {
+			err:      context.DeadlineExceeded,
+			expected: true,
+		},
+		"should return true on wrapped deadline exceeded": {
+			err:      errors.Wrap(context.DeadlineExceeded, "test"),
+			expected: true,
+		},
+		"should return true on gRPC error with status code = 422": {
+			err:      status.Error(http.StatusUnprocessableEntity, "test"),
+			expected: true,
+		},
+		"should return true on wrapped gRPC error with status code = 422": {
+			err:      errors.Wrap(status.Error(http.StatusUnprocessableEntity, "test"), "test"),
+			expected: true,
+		},
+		"should return true on gogo error with status code = 422": {
+			err:      gogoStatus.Error(http.StatusUnprocessableEntity, "test"),
+			expected: true,
+		},
+		"should return true on wrapped gogo error with status code = 422": {
+			err:      errors.Wrap(gogoStatus.Error(http.StatusUnprocessableEntity, "test"), "test"),
+			expected: true,
+		},
+		"should return false on gRPC error with status code != 422": {
+			err:      status.Error(http.StatusInternalServerError, "test"),
+			expected: false,
+		},
+		"should return false on generic error": {
+			err:      errors.New("test"),
+			expected: false,
+		},
+	}
+
+	for testName, testData := range tests {
+		t.Run(testName, func(t *testing.T) {
+			assert.Equal(t, testData.expected, shouldStopQueryFunc(testData.err))
+		})
+	}
+}
diff --git a/pkg/storegateway/bucket.go b/pkg/storegateway/bucket.go
@@ -22,6 +22,7 @@ import (
 	"github.com/go-kit/log/level"
 	"github.com/gogo/protobuf/types"
 	"github.com/grafana/dskit/gate"
+	"github.com/grafana/dskit/grpcutil"
 	"github.com/grafana/dskit/multierror"
 	"github.com/grafana/dskit/runutil"
 	"github.com/oklog/ulid"
@@ -548,7 +549,7 @@ func (s *BucketStore) Series(req *storepb.SeriesRequest, srv storegatewaypb.Stor
 			return
 		}
 		code := codes.Internal
-		if st, ok := status.FromError(errors.Cause(err)); ok {
+		if st, ok := grpcutil.ErrorToStatus(err); ok {
 			code = st.Code()
 		} else if errors.Is(err, context.Canceled) {
 			code = codes.Canceled

diff --git a/pkg/storegateway/gateway_test.go b/pkg/storegateway/gateway_test.go
@@ -29,7 +29,6 @@ import (
 	"github.com/grafana/dskit/services"
 	dstest "github.com/grafana/dskit/test"
 	"github.com/oklog/ulid"
-	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/prometheus/prometheus/model/histogram"
@@ -1481,9 +1480,9 @@ func TestStoreGateway_SeriesQueryingShouldEnforceMaxChunksPerQueryLimit(t *testi
 					if testData.expectedErr != nil {
 						require.Error(t, err)
 						assert.IsType(t, testData.expectedErr, err)
-						s1, ok := status.FromError(errors.Cause(err))
+						s1, ok := status.FromError(err)
 						assert.True(t, ok)
-						s2, ok := status.FromError(errors.Cause(testData.expectedErr))
+						s2, ok := status.FromError(testData.expectedErr)
 						assert.True(t, ok)
 						assert.Contains(t, s1.Message(), s2.Message())
 						assert.Equal(t, s1.Code(), s2.Code())