fix log and change rpc in response to json rpc error #474
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ian-shim
reviewed
Apr 11, 2024
common/geth/handle_error.go
Outdated
| @@ -25,7 +25,7 @@ const ( | |||
| func (f *FailoverController) handleHttpError(httpRespError rpc.HTTPError) (NextEndpoint, ImmediateAction) { | |||
| sc := httpRespError.StatusCode | |||
| // Default to rotation the current RPC, because it allows a higher chance to get the query completed. | |||
| f.Logger.Info("[HTTP Response Error]", "Status Code", sc, "Error", httpRespError) | |||
| f.Logger.Info("[HTTP Response Error]", "Curr Rpc Fault", f.numberRpcFault, "Status Code", sc, "Error", httpRespError) | |||
There was a problem hiding this comment.
Let's not use spaces as attribute labels
f.Logger.Info("[HTTP Response Error]", "numRPCFfault", f.numberRpcFault, "statusCode", sc, "err", httpRespError)
pschork
reviewed
Apr 11, 2024
common/geth/handle_error.go
Outdated
| f.Logger.Info("[JSON RPC Response Error]", "Error Code", ec, "Error", rpcError) | ||
| // we always attribute JSON RPC error as sender's fault, i.e no connection rotation | ||
| return CurrentRPC, Return | ||
| f.Logger.Warn("[JSON RPC Response Error]", "numRPCFfault", f.numberRpcFault, "errorCode", ec, "err", rpcError) |
There was a problem hiding this comment.
Is it possible to extract top level domain (ie strip api token) from url and include it in message so that oncall knows exactly which provider failed?
There was a problem hiding this comment.
addressed, please take a look
ian-shim
approved these changes
Apr 12, 2024
jianoaix
approved these changes
Apr 12, 2024
common/geth/handle_error.go
Outdated
| } | ||
|
|
||
| // If no http response or no rpc response is returned, it is a connection issue, | ||
| // since we can't accurately attribute the network issue to neither sender nor receiver | ||
| // side. Optimistically, switch rpc client | ||
| f.Logger.Info("[Default Response Error]", err) | ||
| f.Logger.Warn("[Default Response Error]", "numRPCFfault", f.numberRpcFault, "err", err) |
There was a problem hiding this comment.
Is it logged anywhere when it switches to a different RPC endpoint? That'd be a useful information to see from logs
common/geth/handle_error.go
Outdated
| @@ -51,7 +51,8 @@ func (f *FailoverController) handleHttpError(httpRespError rpc.HTTPError) (NextE | |||
| // If the error is http, non2xx error would generate HTTP error, https://github.com/ethereum/go-ethereum/blob/master/rpc/http.go#L233 | |||
| // but a 2xx http response could contain JSON RPC error, https://github.com/ethereum/go-ethereum/blob/master/rpc/http.go#L181 | |||
| // If the error is Websocket or IPC, we only look for JSON error, https://github.com/ethereum/go-ethereum/blob/master/rpc/json.go#L67 | |||
|
|
|||
| // | |||
| // This function is protected by mutex from the outside | |||
There was a problem hiding this comment.
nit: "This function assumes the mutex protection from caller to ensure thread safety"
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Checks