fix: node gateway error handling improvements #124

brianluong · 2023-12-19T02:16:53Z

Description

This task to essentially fix the response_code tag on node_gateway_router_upstream_rpc_request_errors.count ended up just a tad bit larger. I ended up making some improvements to how we handle errors, and some small misc things.

Type of change

🐛 Bug fix (non-breaking change which fixes an issue)
😎 New feature (non-breaking change which adds functionality)
⁉️ Breaking change (fix or feature that would cause existing functionality to not work as expected)
⚒️ Refactor (no functional changes)
📖 Documentation (updating or adding docs)

How Has This Been Tested?

Testing local and staging before prod.

brianluong · 2023-12-19T02:18:38Z

internal/metrics/metrics.go

@@ -94,7 +94,7 @@ var (
 				"Batches are deconstructed to single JSON RPC requests for this metric.",
 		},
 		// jsonrpc_method is "batch" for batch requests
-		[]string{"chain_name", "client", "upstream_id", "url", "jsonrpc_method", "response_code", "jsonrpc_error_code"},
+		[]string{"chain_name", "client", "upstream_id", "url", "jsonrpc_method", "jsonrpc_error_code"},


Didn't feel HTTP code on a JSON RPC metric was necessary.

brianluong · 2023-12-19T02:20:30Z

internal/route/request_executor.go

 	)

 	originFunc := func() (*jsonrpc.SingleResponseBody, error) {
 		var err error

 		// Any errors will result in respBody and resp being nil.
-		respBody, resp, err = r.getResponseBody(httpReq, &requestBody, configToRoute) //nolint:bodyclose // linter bug
+		jsonRPCRespBody, httpResp, err = r.getResponseBody(httpReq, &requestBody, configToRoute) //nolint:bodyclose // linter bug


A lil bit of renaming to jsonRPCRespBody and httpResp in a few places. Thought this naming would be more clear.

brianluong · 2023-12-19T02:23:28Z

internal/route/request_executor.go

 func (r *RequestExecutor) routeToConfig(
 	ctx context.Context,
 	requestBody jsonrpc.RequestBody,
 	configToRoute *config.UpstreamConfig,
-) (jsonrpc.ResponseBody, *http.Response, bool, error) {
+) (jsonrpc.ResponseBody, *HttpResponse, bool, error) {


Didn't think it was necessary to plumb the full *http.Response object all the way back to the front. We can close it and pass back some specific pieces of metadata we're interested in (e.g. httpStatusCode).

The impetus for this change was that I was getting confused when I was first looking at the code (I haven't looked at it in months!).

brianluong · 2023-12-19T02:25:10Z

internal/route/request_executor.go

+	respBodyString := util.ReadAndCopyBackResponseBody(resp)
+	httpResp := &HttpResponse{resp.StatusCode}
+
+	if resp.StatusCode >= http.StatusBadRequest {


We weren't doing a good job of handling HTTP responses w/ 4xx and 5xxs in them.

We were trying to deserialize their bodies and failing (most of the time, unless body was empty).

This caused our error metrics to have the wrong HTTP response code attached to them. Ex: I saw a 429 response in logs but it was a 500 in metrics.

HTTP responses with non 2xx//3xx codes should count as errors IMO.

brianluong · 2023-12-19T02:27:29Z

internal/route/request_executor.go


 	if err != nil {
 		r.logger.Warn("Could not deserialize response.", zap.Any("request", requestBody),
 			zap.String("upstreamID", configToRoute.ID), zap.String("response", fmt.Sprintf("%v", resp)), zap.Error(err))
-		return nil, nil, &OriginError{err}
+		return nil, httpResp, err


When we see DecodeError from responses, we should return the response to the customer even though we can't deserialize it.

Passing the DecodeError back all the way back to the front so that we can do that.

As far as I'm aware, this isn't affecting anything right now. Just thought it was something nice to fix while I'm meddling around here.

brianluong · 2023-12-19T02:30:27Z

internal/route/router.go

@@ -155,7 +142,7 @@ func (r *SimpleRouter) Route(
 		strconv.FormatBool(cached),
 	).Inc()

-	if err != nil {
+	if err != nil || httpResponse.StatusCode > http.StatusBadRequest {


This fixes an issue where the HTTP response codes aren't added to the node_gateway_router_upstream_rpc_request_errors.count metric

nice find, do we want 400 as well? aka >= vs >?

Good catch!

kevin-satsuma

looks good overall, just some small questions

kevin-satsuma · 2023-12-19T16:38:57Z

internal/metrics/metrics.go

@@ -94,7 +94,7 @@ var (
 				"Batches are deconstructed to single JSON RPC requests for this metric.",
 		},
 		// jsonrpc_method is "batch" for batch requests
-		[]string{"chain_name", "client", "upstream_id", "url", "jsonrpc_method", "response_code", "jsonrpc_error_code"},
+		[]string{"chain_name", "client", "upstream_id", "url", "jsonrpc_method", "jsonrpc_error_code"},


kevin-satsuma · 2023-12-19T16:45:31Z

internal/route/router.go

@@ -155,7 +142,7 @@ func (r *SimpleRouter) Route(
 		strconv.FormatBool(cached),
 	).Inc()

-	if err != nil {
+	if err != nil || httpResponse.StatusCode > http.StatusBadRequest {


nice find, do we want 400 as well? aka >= vs >?

kevin-satsuma · 2023-12-19T16:46:36Z

internal/route/request_executor.go

+type HTTPResponse struct {
+	StatusCode int


just curious, what was the reason behind using this type instead of http.Response?

oh sorry, i missed your comment : #124 (comment)

My intuition says that the response body, aka HTTP.Response.Body, should be read and discarded (body.close()) as soon as possible. Otherwise, the connection is held until body.close() is called.

And since http.Response.body is a field on http.Response, it seems to appropriate to apply the "read and discard ASAP" model to the entire object. If we need some data from the object, it's best to read it out into an user-defined structure.

Besides that, the body is a read-once object. How will other functions know if it has been read yet?

kevin-satsuma · 2023-12-19T16:49:14Z

internal/server/rpc_handler.go

+		// Still pass the response to client if we're not able to decode response from upstream.
+		case *jsonrpc.DecodeError:


i wonder how this changes the behavior on the indexers. i'm assuming current decode errors get swallowed up into a generic 5XX or something, and the indexer will retry on the request.

once we add this change the indexer will potentially receive a poorly formatted JSON response. hopefully it gracefully handles this scenario and retries. then again this case is probably pretty rare.

Yeah, I'm not totally sure of the consequences atm, but you're right, it's probably rare.

brianluong · 2023-12-19T19:24:19Z

internal/jsonrpc/jsonrpc.go

-
-	var body *SingleRequestBody
-
+func DecodeRequestBody(requestBodyRawBytes []byte) (RequestBody, error) {


Moving the io.ReadAll call out of this function because errors reading data out of the stream were being conflated with real decoding errors.

Example from logs: "error":"decode error: http2: server sent GOAWAY and closed the connection; LastStreamID=19999, ErrCode=NO_ERROR, debug=\"\",

This is an error from io.ReadAll. It's an error on the connection level, not with the contents of the response.

brianluong · 2023-12-19T19:25:21Z

internal/route/request_executor.go

+	respBodyBytes, err := util.ReadAndCopyBackResponseBody(resp)
+	respBodyString := string(respBodyBytes)
+
+	if err != nil {


Here we're handling connection-level errors like
"error":"decode error: http2: server sent GOAWAY and closed the connection; LastStreamID=19999, ErrCode=NO_ERROR, debug=\"\",

brianluong commented Dec 19, 2023

View reviewed changes

brianluong force-pushed the misc_ng_improvements branch from 6ecb206 to e5325f9 Compare December 19, 2023 02:37

brianluong changed the title ~~node gateway visbility improvements~~ fix: node gateway visbility improvements Dec 19, 2023

brianluong force-pushed the misc_ng_improvements branch from e5325f9 to 6c05817 Compare December 19, 2023 02:42

brianluong changed the title ~~fix: node gateway visbility improvements~~ fix: node gateway error handling improvements Dec 19, 2023

brianluong force-pushed the misc_ng_improvements branch from 6c05817 to 3104450 Compare December 19, 2023 02:44

brianluong requested a review from kevin-satsuma December 19, 2023 03:13

kevin-satsuma reviewed Dec 19, 2023

View reviewed changes

kevin-satsuma previously approved these changes Dec 19, 2023

View reviewed changes

brianluong dismissed kevin-satsuma’s stale review via 15bb16d December 19, 2023 18:35

brianluong force-pushed the misc_ng_improvements branch from da8fcaa to 15bb16d Compare December 19, 2023 18:35

brianluong commented Dec 19, 2023

View reviewed changes

brianluong force-pushed the misc_ng_improvements branch from df53485 to 46451c0 Compare December 19, 2023 21:02

brianluong added 4 commits December 19, 2023 13:17

fix: node gateway error handling improvements

240b9e0

fix: remove depguard from linter

77cd3c3

fix: remove varcheck and deadcode linters since they're deprecated

1a7a578

fix: satsify linter

b3d6480

brianluong force-pushed the misc_ng_improvements branch from 46451c0 to b3d6480 Compare December 19, 2023 21:17

brianluong added 2 commits December 19, 2023 14:56

fix: small fixes and logging improvements

1da3476

fix: bump version of golangci-lint in ci

c791c0e

brianluong merged commit b7f20aa into main Dec 19, 2023
3 checks passed

brianluong deleted the misc_ng_improvements branch December 19, 2023 23:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: node gateway error handling improvements #124

fix: node gateway error handling improvements #124

brianluong commented Dec 19, 2023 •

edited

Loading

brianluong Dec 19, 2023 •

edited

Loading

kevin-satsuma Dec 19, 2023

brianluong Dec 19, 2023

brianluong Dec 19, 2023

brianluong Dec 19, 2023 •

edited

Loading

brianluong Dec 19, 2023

brianluong Dec 19, 2023

brianluong Dec 19, 2023

kevin-satsuma Dec 19, 2023

brianluong Dec 19, 2023

kevin-satsuma left a comment

kevin-satsuma Dec 19, 2023

kevin-satsuma Dec 19, 2023

kevin-satsuma Dec 19, 2023

kevin-satsuma Dec 19, 2023

brianluong Dec 19, 2023 •

edited

Loading

kevin-satsuma Dec 19, 2023

brianluong Dec 19, 2023

brianluong Dec 19, 2023

brianluong Dec 19, 2023

		// Still pass the response to client if we're not able to decode response from upstream.
		case *jsonrpc.DecodeError:


		var body *SingleRequestBody

		func DecodeRequestBody(requestBodyRawBytes []byte) (RequestBody, error) {

fix: node gateway error handling improvements #124

fix: node gateway error handling improvements #124

Conversation

brianluong commented Dec 19, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

brianluong Dec 19, 2023 • edited Loading

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

brianluong Dec 19, 2023 • edited Loading

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

kevin-satsuma 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

brianluong Dec 19, 2023 • edited Loading

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

brianluong commented Dec 19, 2023 •

edited

Loading

brianluong Dec 19, 2023 •

edited

Loading

brianluong Dec 19, 2023 •

edited

Loading

brianluong Dec 19, 2023 •

edited

Loading