Do not use grpc/status.FromError() and gogo/status.FromError() #7224

pracucci · 2024-01-25T13:48:29Z

What this PR does

In the review of #7213 I've learned from @duricanikolic that:

I have worked on the error handling improvement on the write path, and I ensured that most of the usages of the status packages there were from gogo instead of grpc.
What is the difference?
On one side, gogo's status package allows you to easily add custom error details, which are crucial for the error handling on the write path. On another side, status.FromError() doesn't use errors.As() internally.
Related to grpc's status package, it is not easy to add custom error details, but status.FromError() uses errors.As() internally.
In order to minimizes differences between different packages used, I then implemented [grpcutil.ErrorToStatus()](https://github.com/grafana/dskit/blob/main/grpcutil/status.go#L12-L36) in dskit that, independently from whether the status is created by gogo or grpc uses errors.As().
I'd recommend using grpcutil.ErrorToStatus() instead of status.FromError().

Since we want support for errors wrapping everywhere, to avoid subtle bugs depending on which status package is imported, I propose to never allow .FromError(), replace it with grpcutil.ErrorToStatus() and add a faillint rule to enforce it.

Which issue(s) this PR fixes or relates to

N/A

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

pstibrany

Makes sense to me.

Signed-off-by: Marco Pracucci <[email protected]>

duricanikolic · 2024-01-25T14:29:51Z

Makefile

@@ -306,6 +306,10 @@ lint: check-makefiles
 	# Ensure all errors are report as APIError
 	faillint -paths "github.com/weaveworks/common/httpgrpc.{Errorf}=github.com/grafana/mimir/pkg/api/error.Newf" ./pkg/frontend/querymiddleware/...

+	# Do not use grpc/status.FromError() because doesn't support wrapped errors.


This is actually not true:

grpc/status.FromError() support wrapped errors (it uses errors.As() internally)

gogo/status.FromError() doesn't support wrapped errors (it doesn't use errors.As() internally)

So, I'd suggest to decommission both grpc/status.FromError() and gogo/status.FromError() and use grpcutil.ErrorToStatus() instead.

I think I was completely drunk when I worked on this PR 🤦

@duricanikolic I should have fix it.

Signed-off-by: Marco Pracucci <[email protected]>

duricanikolic

LGTM
I have added some non-blocking suggestions.

duricanikolic · 2024-01-25T15:03:00Z

pkg/ingester/client/circuitbreaker_test.go

@@ -78,7 +79,7 @@ func TestNewCircuitBreaker(t *testing.T) {

 	// Failed request that should put the circuit breaker into "open"
 	err = breaker(context.Background(), "/cortex.Ingester/Push", "", "", &conn, failure)
-	s, ok := status.FromError(err)
+	s, ok := grpcutil.ErrorToStatus(err)


[nit]
Another possibility would be:

Suggested change

s, ok := grpcutil.ErrorToStatus(err)

code := grpcutil.ErrorToStatusCode(err)

and then to check

require.Equal(t, codes.Unavailable, code)

duricanikolic · 2024-01-25T15:04:40Z

pkg/ingester/client/mimir_util_test.go

@@ -79,7 +78,7 @@ func TestSendQueryStream(t *testing.T) {
 	// Try to receive the response and assert the error we get is the context.Canceled
 	// wrapped within a gRPC error.
 	_, err = stream.Recv()
-	s, ok := status.FromError(err)
+	s, ok := grpcutil.ErrorToStatus(err)


[nit]
Another possibility would be:

Suggested change

s, ok := grpcutil.ErrorToStatus(err)

code := grpcutil.ErrorToStatusCode(err)

and then to check

require.Equal(t, codes.Canceled, code)

duricanikolic · 2024-01-25T15:06:17Z

pkg/ingester/ingester_test.go

@@ -6504,7 +6504,7 @@ func TestIngester_PushInstanceLimits(t *testing.T) {
 							assert.ErrorIs(t, err, testData.expectedErr)
 							var optional middleware.OptionalLogging
 							assert.ErrorAs(t, err, &optional)
-							s, ok := status.FromError(err)
+							s, ok := grpcutil.ErrorToStatus(err)


[nit]
Another possibility would be:

Suggested change

s, ok := grpcutil.ErrorToStatus(err)

code := grpcutil.ErrorToStatusCode(err)

and then to check

require.Equal(t, codes.Unavailable, code)

duricanikolic · 2024-01-25T15:06:50Z

pkg/ingester/ingester_test.go

@@ -6824,7 +6824,7 @@ func TestIngester_inflightPushRequests(t *testing.T) {
 		require.ErrorAs(t, err, &optional)
 		require.False(t, optional.ShouldLog(ctx, time.Duration(0)), "expected not to log via .ShouldLog()")

-		s, ok := status.FromError(err)
+		s, ok := grpcutil.ErrorToStatus(err)


[nit]
Another possibility would be:

Suggested change

s, ok := grpcutil.ErrorToStatus(err)

code := grpcutil.ErrorToStatusCode(err)

and then to check

require.Equal(t, codes.Unavailable, code)

duricanikolic · 2024-01-25T15:07:00Z

pkg/ingester/ingester_test.go

@@ -6931,7 +6931,7 @@ func TestIngester_inflightPushRequestsBytes(t *testing.T) {
 		require.ErrorAs(t, err, &optional)
 		require.False(t, optional.ShouldLog(ctx, time.Duration(0)), "expected not to log via .ShouldLog()")

-		s, ok := status.FromError(err)
+		s, ok := grpcutil.ErrorToStatus(err)


[nit]
Another possibility would be:

Suggested change

s, ok := grpcutil.ErrorToStatus(err)

code := grpcutil.ErrorToStatusCode(err)

and then to check

require.Equal(t, codes.Unavailable, code)

duricanikolic · 2024-01-25T15:17:12Z

pkg/storegateway/bucket_test.go

@@ -2033,14 +2033,14 @@ func TestBucketStore_Series_CanceledRequest(t *testing.T) {
 	srv := newStoreGatewayTestServer(t, store)
 	_, _, _, _, err = srv.Series(ctx, req)
 	assert.Error(t, err)
-	s, ok := status.FromError(err)
+	s, ok := grpcutil.ErrorToStatus(err)


[nit]
Another possibility would be:

Suggested change

s, ok := grpcutil.ErrorToStatus(err)

code := grpcutil.ErrorToStatusCode(err)

and then to check

require.Equal(t, codes.Canceled, code)

duricanikolic · 2024-01-25T15:17:24Z

pkg/storegateway/bucket_test.go

 	assert.True(t, ok)
 	assert.Equal(t, codes.Canceled, s.Code())

 	req.StreamingChunksBatchSize = 10
 	_, _, _, _, err = srv.Series(ctx, req)
 	assert.Error(t, err)
-	s, ok = status.FromError(err)
+	s, ok = grpcutil.ErrorToStatus(err)


[nit]
Another possibility would be:

Suggested change

s, ok = grpcutil.ErrorToStatus(err)

code := grpcutil.ErrorToStatusCode(err)

and then to check

require.Equal(t, codes.Canceled, code)

duricanikolic · 2024-01-25T15:17:43Z

pkg/storegateway/bucket_test.go

@@ -2747,7 +2747,7 @@ func TestLabelNames_Cancelled(t *testing.T) {

 	_, err := store.LabelNames(ctx, req)
 	assert.Error(t, err)
-	s, ok := status.FromError(err)
+	s, ok := grpcutil.ErrorToStatus(err)


[nit]
Another possibility would be:

Suggested change

s, ok := grpcutil.ErrorToStatus(err)

code := grpcutil.ErrorToStatusCode(err)

and then to check

require.Equal(t, codes.Canceled, code)

duricanikolic · 2024-01-25T15:17:58Z

pkg/storegateway/bucket_test.go

@@ -2774,7 +2774,7 @@ func TestLabelValues_Cancelled(t *testing.T) {

 	_, err := store.LabelValues(ctx, req)
 	assert.Error(t, err)
-	s, ok := status.FromError(err)
+	s, ok := grpcutil.ErrorToStatus(err)


[nit]
Another possibility would be:

Suggested change

s, ok := grpcutil.ErrorToStatus(err)

code := grpcutil.ErrorToStatusCode(err)

and then to check

require.Equal(t, codes.Canceled, code)

duricanikolic · 2024-01-25T15:18:46Z

pkg/storegateway/limiter_test.go

@@ -43,7 +43,7 @@ func TestLimiter(t *testing.T) {
 }

 func checkErrorStatusCode(t *testing.T, err error) {
-	st, ok := status.FromError(err)
+	st, ok := grpcutil.ErrorToStatus(err)


[nit]
Another possibility would be:

Suggested change

st, ok := grpcutil.ErrorToStatus(err)

code := grpcutil.ErrorToStatusCode(err)

and then to check

require.Equal(t, uint32(http.StatusUnprocessableEntity), code)

pracucci · 2024-01-25T15:35:46Z

Thanks @duricanikolic for the review. I checked at the nits you've left about using ErrorToStatusCode() instead of ErrorToStatus(), and I'm not going to do the replace. I still see a value asserting on ok returned by ErrorToStatus(), and in most of non-test places there're just a couple of places where we could use ErrorToStatusCode() but doesn't make a significant difference, so I'm keeping the code as is to keep the diff as smallest as possible.

pracucci requested a review from duricanikolic January 25, 2024 13:48

pracucci mentioned this pull request Jan 25, 2024

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

Merged

4 tasks

pstibrany approved these changes Jan 25, 2024

View reviewed changes

Do not use grpc/status.FromError()

4ed52da

Signed-off-by: Marco Pracucci <[email protected]>

pracucci force-pushed the do-not-use-grpc-fromerror branch from 3b857e5 to 4ed52da Compare January 25, 2024 14:11

pracucci marked this pull request as ready for review January 25, 2024 14:17

pracucci requested review from wilfriedroset, vaxvms, bubu11e and a team as code owners January 25, 2024 14:17

duricanikolic reviewed Jan 25, 2024

View reviewed changes

pracucci marked this pull request as draft January 25, 2024 14:40

Do not use gogo/status.FromError()

b99879f

Signed-off-by: Marco Pracucci <[email protected]>

pracucci changed the title ~~Do not use grpc/status.FromError()~~ Do not use grpc/status.FromError() and gogo/status.FromError() Jan 25, 2024

pracucci marked this pull request as ready for review January 25, 2024 15:01

pracucci requested a review from a team as a code owner January 25, 2024 15:01

duricanikolic approved these changes Jan 25, 2024

View reviewed changes

pracucci merged commit a9282f0 into main Jan 25, 2024
28 checks passed

pracucci deleted the do-not-use-grpc-fromerror branch January 25, 2024 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not use grpc/status.FromError() and gogo/status.FromError() #7224

Do not use grpc/status.FromError() and gogo/status.FromError() #7224

pracucci commented Jan 25, 2024 •

edited

Loading

pstibrany left a comment

duricanikolic Jan 25, 2024

pracucci Jan 25, 2024

pracucci Jan 25, 2024

duricanikolic left a comment

duricanikolic Jan 25, 2024

duricanikolic Jan 25, 2024

duricanikolic Jan 25, 2024

duricanikolic Jan 25, 2024

duricanikolic Jan 25, 2024

duricanikolic Jan 25, 2024

duricanikolic Jan 25, 2024

duricanikolic Jan 25, 2024

duricanikolic Jan 25, 2024

duricanikolic Jan 25, 2024

pracucci commented Jan 25, 2024

	s, ok := grpcutil.ErrorToStatus(err)
	code := grpcutil.ErrorToStatusCode(err)

	s, ok = grpcutil.ErrorToStatus(err)
	code := grpcutil.ErrorToStatusCode(err)

	st, ok := grpcutil.ErrorToStatus(err)
	code := grpcutil.ErrorToStatusCode(err)

Do not use grpc/status.FromError() and gogo/status.FromError() #7224

Do not use grpc/status.FromError() and gogo/status.FromError() #7224

Conversation

pracucci commented Jan 25, 2024 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duricanikolic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci commented Jan 25, 2024

pracucci commented Jan 25, 2024 •

edited

Loading