Allow reporting gRPC codes in "status_code" label of request duration metrics #424

duricanikolic · 2023-11-01T12:29:02Z

What this PR does:
Before this PR grpc_instrumentation.go could recognize only the errors built by the httpgrpc package and use their error codes in the status_code label of the request duration metrics. All other gRPC errors were treated as unknown, and used to be labeled as error. This PR fixes that erroneous behaviour, and allows all gRPC status codes to be used as the status_code label.

Types middleware.InstrumentationLabel and middleware.InstrumentationOption, as well as a special value of the latter, middleware.ReportGRPCStatusOption have been introduced. This special value can be used to enable reporting of gRPC status codes in status_code label (instead of simplified "error", "cancel" or "success" values). middleware.ReportGRPCStatusOption can be passed as an optional argument to the following methods:

middleware.UnaryServerInstrumentInterceptor
middleware.StreamServerInstrumentInterceptor
middleware.UnaryClientInstrumentInterceptor
middleware.StreamClientInstrumentInterceptor
middleware.Instrument.

In order to guarantee backwards compatibility, reporting of gRPC codes as labels is disabled by default and could be enabled as follows:

on server side by setting the new CLI experimental flag -server.report-grpc-codes-in-instrumentation-label to true, or by setting server.Config.ReportGRPCCodesInInstrumentationLabel to true.
on client side by passing middleware.ReportGRPCStatusOption to middleware.Instrument.
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]

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

middleware/grpc_instrumentation.go

middleware/grpc_instrumentation_test.go

middleware/grpc_instrumentation.go

duricanikolic · 2023-11-01T22:27:10Z

grpcutil/status.go

+// This implementation differs from status.FromError() because the
+// latter checks only if the given error can be cast to status.Status,
+// and doesn't check other errors in the given error's tree.
+func ErrorToStatus(err error) (*status.Status, bool) {


This current httpgrpc.statusFromError() function has been renamed to ErrorToStatus(), exported and moved here.

duricanikolic · 2023-11-01T22:27:39Z

grpcutil/status_test.go

+	grpcstatus "google.golang.org/grpc/status"
+)
+
+func TestErrorToStatus(t *testing.T) {


The current httpgrpc.TestStatusFromError() has been moved here.

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

duricanikolic · 2023-11-01T22:37:57Z

grpcutil/metadata_test.go

 )

 func TestAppendMessageSizeToOutgoingContext(t *testing.T) {
 	ctx := context.Background()

-	req := &httpgrpc.HTTPRequest{


This has been replaced with fakeSizer to avoid a circular dependency. The parameter of AppendMessageSizeToOutgoingContext should be any implementation of the Sizer interface, and httpgrpc.HTTPRequest was just one of them.

middleware/grpc_instrumentation.go

pstibrany · 2023-11-02T09:18:46Z

grpcutil/cancel.go

 )

 // IsCanceled checks whether an error comes from an operation being canceled
 func IsCanceled(err error) bool {
 	if errors.Is(err, context.Canceled) {
 		return true
 	}
-	s, ok := status.FromError(err)
+	s, ok := ErrorToStatus(err)


Doing full conversion to gogo's status.Status seems unnecessary here, if all we care about is code from grpc.Status. Perhaps we can have ErrorToStatusCode function too?

It is true that we need only the status code here, but in order to get it, we anyway need to call a gogostatus.FromError() or grpcstatus.FromError() (the current implementation).
My idea here was to unify the way we extract statuses. Because in most of the places it is done by gogostatus, and in a couple of places it is done by grpcstatus.

The idea is, anyway, to get rid of the gogo-related things, but once we find a valid alternative. If we now start mixing gogo and grpc usages, it will be more difficult to do the change one day.

If we now start mixing gogo and grpc usages, it will be more difficult to do the change one day.

I don't see how my proposal would do that?

I don't see how my proposal would do that?

I was referring to the current implementation: it added a grpcstatus usage to dskit. And my motivation with the proposed change was to get rid of it. I meant that.

I was referring to the current implementation: it added a grpcstatus usage to dskit. And my motivation with the proposed change was to get rid of it. I meant that.

We need to check for GRPCStatus() *grpcstatus.Status method, is this what you're referring to? I don't see a way around it.

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

middleware/grpc_instrumentation.go

grpcutil/status.go

grpcutil/status_test.go

middleware/grpc_instrumentation_test.go

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

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

pstibrany

This PR is getting bigger and bigger :/

server/server.go

middleware/grpc_instrumentation.go

pstibrany · 2023-11-03T11:39:36Z

middleware/grpc_instrumentation.go

+	statusCode := grpcutil.ErrorToStatusCode(err)
+
+	if statusCode == codes.Canceled {
+		return statusCode
+	}
+
+	if isHTTPStatusCode(statusCode) {
+		return statusCode
+	}
+	if i.acceptGRPCStatuses {
+		return statusCode
+	}
+	return codes.Unknown


I think this function is doing too much -- it should just return grpcutil.ErrorToStatusCode(err) for non-nil and not-canceled errors. (It also doesn't need to be a method of InstrumentationLabel.)

This function is the only place where we actually take into account whether i.acceptGRPCStatues is set to true or not. If we move it out from here, we need to put it in statusCodeToString() function. I can try to see how it would look like, but it will for sure complicate the test.

I can try to see how it would look like, but it will for sure complicate the test.

I'm not sure if test is too complicated or not, but I think this function looks good now. Thank you for giving it a try.

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

pstibrany

Great work 👏

pstibrany · 2023-11-03T16:25:24Z

middleware/grpc_instrumentation_test.go

+		instrumentationOptions     []InstrumentationOption
+		expectedAcceptGRPCStatuses bool
+	}{
+		"Applying no InstrimentationOption sets acceptGRPCStatus to false": {


nit: typo in InstrimentationOption

pstibrany · 2023-11-03T16:25:48Z

middleware/grpc_instrumentation_test.go

+func TestApplyInstrumentationLabelOptions(t *testing.T) {
+	testCases := map[string]struct {
+		instrumentationOptions     []InstrumentationOption
+		expectedAcceptGRPCStatuses bool


it's now called "reportGRPCStatus".

pstibrany · 2023-11-03T16:29:33Z

CHANGELOG.md

@@ -155,6 +155,13 @@
 * [ENHANCEMENT] Memcached: allow to configure write and read buffer size (in bytes). #414
 * [ENHANCEMENT] Server: Add `-server.http-read-header-timeout` option to specify timeout for reading HTTP request header. It defaults to 0, in which case reading of headers can take up to `-server.http-read-timeout`, leaving no time for reading body, if there's any. #423
 * [ENHANCEMENT] Make httpgrpc.Server produce non-loggable errors when a header with key `httpgrpc.DoNotLogErrorHeaderKey` and any value is present in the HTTP response. #421
+* [ENHANCEMENT] Server: Add `-server.report-grpc-codes-in-instrumentation-label` CLI flag to specify whether gRPC status codes should be used in instrumentation labels. It defaults to false, meaning that gRPC status codes are represented with `error` value. #424
+* [ENHANCEMENT] Instrumentation: `middleware.InstrumentationOption` struct, and a special value `middleware.ReportGRPCStatusOption` have been added to allow both server and clients to configure gRPC status code usages in instrumentation labels. This can be optionally used in the following functions: #424


Suggested change

* [ENHANCEMENT] Instrumentation: `middleware.InstrumentationOption` struct, and a special value `middleware.ReportGRPCStatusOption` have been added to allow both server and clients to configure gRPC status code usages in instrumentation labels. This can be optionally used in the following functions: #424

* [ENHANCEMENT] Instrumentation: Added `middleware.ReportGRPCStatusOption` that can be passed to the following functions to enable reporting of gRPC status codes in "status_code" label (instead of simplified "error", "cancel" or "success" values): #424

pstibrany · 2023-11-03T16:31:05Z

CHANGELOG.md

@@ -155,6 +155,13 @@
 * [ENHANCEMENT] Memcached: allow to configure write and read buffer size (in bytes). #414
 * [ENHANCEMENT] Server: Add `-server.http-read-header-timeout` option to specify timeout for reading HTTP request header. It defaults to 0, in which case reading of headers can take up to `-server.http-read-timeout`, leaving no time for reading body, if there's any. #423
 * [ENHANCEMENT] Make httpgrpc.Server produce non-loggable errors when a header with key `httpgrpc.DoNotLogErrorHeaderKey` and any value is present in the HTTP response. #421
+* [ENHANCEMENT] Server: Add `-server.report-grpc-codes-in-instrumentation-label` CLI flag to specify whether gRPC status codes should be used in instrumentation labels. It defaults to false, meaning that gRPC status codes are represented with `error` value. #424


Suggested change

* [ENHANCEMENT] Server: Add `-server.report-grpc-codes-in-instrumentation-label` CLI flag to specify whether gRPC status codes should be used in instrumentation labels. It defaults to false, meaning that gRPC status codes are represented with `error` value. #424

* [ENHANCEMENT] Server: Add `-server.report-grpc-codes-in-instrumentation-label` CLI flag to specify whether gRPC status codes should be used in `status_code` label of request duration metric. It defaults to false, meaning that gRPC status codes are represented with `error` value. #424

pstibrany · 2023-11-03T16:35:49Z

middleware/grpc_instrumentation.go

+// errorToStatusCode extracts a status code from the given error, and does the following:
+//
+//   - If the error is nil, codes.OK is returned.
+//   - If the error corresponds to context.Canceled, codes.Canceled is returned.
+//   - Otherwise, the actual status code of the error is returned.
+func errorToStatusCode(err error) codes.Code {


nit: This description of implementation seems unnecessary. Same for next function. (I'd only comment public functions, of functions with tricky behaviour. But this is pretty straightforward)

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

grpc_instrumentation: recognize both HTTP and gRPC status codes

32642d0

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

duricanikolic self-assigned this Nov 1, 2023

duricanikolic requested a review from pstibrany November 1, 2023 12:30

pstibrany reviewed Nov 1, 2023

View reviewed changes

middleware/grpc_instrumentation.go Outdated Show resolved Hide resolved

pstibrany reviewed Nov 1, 2023

View reviewed changes

middleware/grpc_instrumentation_test.go Outdated Show resolved Hide resolved

middleware/grpc_instrumentation.go Outdated Show resolved Hide resolved

duricanikolic commented Nov 1, 2023

View reviewed changes

duricanikolic requested a review from pstibrany November 1, 2023 22:29

Fixing review findings

d9ac52a

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

duricanikolic force-pushed the yuri/status-code branch from 741ad95 to d9ac52a Compare November 1, 2023 22:35

duricanikolic commented Nov 1, 2023

View reviewed changes