httpgrpc: correct handling of non-loggable errors #421

duricanikolic · 2023-10-27T15:23:49Z

What this PR does:
This PR allows correct handling of non-loggable errors.

First of all, it introduces middleware.DoNotLogError type, representing the errors that should not be logged. Additionally, it introduces httpgrpc.DoNotLogErrorHeaderKey, an HTTP header key with value X-DoNotLogError. In case of an erroneous HTTPResponse, the presence of this header means that the corresponding error should not be logged.

When httpgrpc's Server.Handle() receives a response from the HTTP server, an HTTPResponse object is created. HTTPResponses with a status 5xx are translated into gRPC errors, by using the httpgrpc package. Before this PR, all these errors were loggable. This PR allows Server.Handle() to return errors that should not be logged. More precisely, the presence of a header with key httpgrpc.DoNotLogHeader and any value in an erroneous HTTPResponse indicates to Servier.Handle() to wrap the gRPC error (created by the httpgrpc package) with a httpgrpc.DoNotLogError.

One more change introduced by this PR concerns httpgrpc.HTTPResponseFromError(). Namely, before this PR, errors built by httpgrpc package and wrapped with another error, were not recognized by httpgrpc.HTTPResponseFromError(). This PR fixes that behavior.

Which issue(s) this PR fixes:

Part of grafana/mimir#6008.

Checklist

Tests updated
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pstibrany

looks good overall, left some minor comments.

pstibrany · 2023-10-27T15:32:58Z

httpgrpc/server/server.go

@@ -234,3 +239,8 @@ func fromHeader(hs http.Header) []*httpgrpc.Header {
 	}
 	return result
 }
+
+func containsDoNotLogErrorKey(hs http.Header) bool {


This is too shallow. I understand it's that way for testing, but this functionality can be tested together with the rest of server handler too.

I have removed this method, as well as the corresponding test.

pstibrany · 2023-10-27T15:36:34Z

httpgrpc/server/server_test.go

+func TestContainsDoNotLogErrorKey(t *testing.T) {
+	testCases := map[string]struct {
+		header          http.Header
+		expectedOutcome bool
+	}{
+		"if headers do not contain X-DoNotLogError, return false": {
+			header: http.Header{
+				"X-First":  []string{"a", "b", "c"},
+				"X-Second": []string{"1", "2"},
+			},
+			expectedOutcome: false,
+		},
+		"if headers contain X-DoNotLogError with a value, return true": {
+			header: http.Header{
+				"X-First":         []string{"a", "b", "c"},
+				"X-DoNotLogError": []string{"true"},
+			},
+			expectedOutcome: true,
+		},
+		"if headers contain X-DoNotLogError without a value, return true": {
+			header: http.Header{
+				"X-First":         []string{"a", "b", "c"},
+				"X-DoNotLogError": nil,
+			},
+			expectedOutcome: true,
+		},
+	}
+	for testName, testData := range testCases {
+		t.Run(testName, func(t *testing.T) {
+			require.Equal(t, testData.expectedOutcome, containsDoNotLogErrorKey(testData.header))
+		})
+	}
+}


Honestly, this test seems bit too much for testing what's essentially a simple map key-check. I'd suggest removing this test, and including checks for DoNotLog error into already existing tests that handle 500 errors.

I have applied the required fix.

pstibrany · 2023-10-27T15:37:06Z

httpgrpc/server/server.go

-		return nil, httpgrpc.ErrorFromHTTPResponse(resp)
+		err := httpgrpc.ErrorFromHTTPResponse(resp)
+		if containsDoNotLogErrorKey(header) {
+			return nil, middleware.DoNotLogError{Err: err}


Suggested change

return nil, middleware.DoNotLogError{Err: err}

err = middleware.DoNotLogError{Err: err}

pstibrany · 2023-10-27T15:39:07Z

middleware/grpc_logging.go

+var (
+	DoNotLogErrorHeader = http.CanonicalHeaderKey("X-DoNotLogError")
+)


I think this belongs to httpgrpc package, where server is using it. grpc_logging.go has nothing to do with HTTP.

I have moved this header to httpgrpc/server.go.

56quarters · 2023-10-27T15:55:09Z

httpgrpc/httpgrpc.go

+// If the given error, or any error from its tree are a status.Status,
+// that status.Status and the outcome true are returned.
+// Otherwise, nil and the outcome false are returned.
+// This implementation differs from status.FromError() because the


It does check that. The implementation is nearly the same as this method:

https://github.com/grafana/mimir/blob/main/vendor/google.golang.org/grpc/status/status.go#L113

@56quarters you linked the grpc’s status, and not gogo’s status.

OK, but that's the one we should be using? That's the one that the circuit breakers in Mimir use to extract gRPC error codes. The gogo behavior sounds like something we absolutely should avoid.

Hi @56quarters,
Curently gogo is used both in dskit and in mimir. It is used even for the generation of Go structs out of .proto sources (e.g., here).
I agree that we should replace usages of gogo's status package with grpc's status package, but this should be done in a separate PR, because that change would be very big.

WDYT?

Update: apparently we already have an issue for removing the deprecated gogo dependency. But this would be a breaking change, plus we don't know which package to use instead. The grpc package has some big performance issues, that's why the gogo project was created.

That's the one that the circuit breakers in Mimir use to extract gRPC error codes

This is not a problem, since gRPC errors created by both grpc and gogo can be recognized by each other.

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

pstibrany

lgtm overall, but would like to see a better test for httpgrpc.Server.

httpgrpc/server/server.go

httpgrpc/server/server_test.go

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

pstibrany

Thank you.

pstibrany · 2023-10-31T12:07:40Z

httpgrpc/server/server.go

-		return nil, httpgrpc.ErrorFromHTTPResponse(resp)
+		err := httpgrpc.ErrorFromHTTPResponse(resp)
+		if _, ok := header[DoNotLogErrorHeaderKey]; ok {
+			recorder.Header().Del(DoNotLogErrorHeaderKey)


We don't need to remove header here, only headers in resp are important, and fromHeader is covering that already.

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

* httpgrpc: correct handling of non-loggable errors Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com> * Fixing review findings Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com> * Fixing review findings 2 Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com> * Fixing review findings 3 Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com> --------- Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

duricanikolic self-assigned this Oct 27, 2023

duricanikolic force-pushed the yuri/do-not-log-error branch from f09f3b4 to 7057a5d Compare October 27, 2023 15:24

duricanikolic requested review from pstibrany and 56quarters October 27, 2023 15:27

pstibrany reviewed Oct 27, 2023

View reviewed changes

56quarters reviewed Oct 27, 2023

View reviewed changes

duricanikolic requested review from 56quarters and pstibrany October 30, 2023 09:59

duricanikolic added 2 commits October 30, 2023 20:01

httpgrpc: correct handling of non-loggable errors

4520f0d

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Fixing review findings

00bb08e

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

duricanikolic force-pushed the yuri/do-not-log-error branch from b2488cc to 00bb08e Compare October 30, 2023 19:01

56quarters approved these changes Oct 30, 2023

View reviewed changes

pstibrany approved these changes Oct 31, 2023

View reviewed changes

httpgrpc/server/server.go Show resolved Hide resolved

httpgrpc/server/server.go Show resolved Hide resolved

httpgrpc/server/server_test.go Outdated Show resolved Hide resolved

Fixing review findings 2

3ec93e8

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

duricanikolic requested a review from pstibrany October 31, 2023 11:11

pstibrany approved these changes Oct 31, 2023

View reviewed changes

Fixing review findings 3

9145a20

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

duricanikolic merged commit 52f4e8d into main Oct 31, 2023
3 checks passed

duricanikolic deleted the yuri/do-not-log-error branch October 31, 2023 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

httpgrpc: correct handling of non-loggable errors #421

httpgrpc: correct handling of non-loggable errors #421

duricanikolic commented Oct 27, 2023 •

edited

pstibrany left a comment

pstibrany Oct 27, 2023

duricanikolic Oct 29, 2023

pstibrany Oct 27, 2023

duricanikolic Oct 29, 2023

pstibrany Oct 27, 2023

pstibrany Oct 27, 2023

duricanikolic Oct 29, 2023

56quarters Oct 27, 2023

duricanikolic Oct 27, 2023

56quarters Oct 27, 2023 •

edited

duricanikolic Oct 29, 2023 •

edited

duricanikolic Oct 30, 2023 •

edited

pstibrany left a comment

pstibrany left a comment

pstibrany Oct 31, 2023

	return nil, middleware.DoNotLogError{Err: err}
	err = middleware.DoNotLogError{Err: err}

httpgrpc: correct handling of non-loggable errors #421

httpgrpc: correct handling of non-loggable errors #421

Conversation

duricanikolic commented Oct 27, 2023 • edited

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

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

56quarters Oct 27, 2023 • edited

Choose a reason for hiding this comment

duricanikolic Oct 29, 2023 • edited

Choose a reason for hiding this comment

duricanikolic Oct 30, 2023 • edited

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duricanikolic commented Oct 27, 2023 •

edited

56quarters Oct 27, 2023 •

edited

duricanikolic Oct 29, 2023 •

edited

duricanikolic Oct 30, 2023 •

edited