Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for request batching over HTTP and websockets by introducing new batch serialization and decoding functions as well as updating downstream handlers and tests.
- Updated ws and HTTP handlers to work with batched requests/responses.
- Added new batch processing logic in the rpc package with accompanying test cases.
- Updated metrics recording to differentiate between successful and failed batched requests.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ws.go | Updated websocket pump to record metrics using new batch request IDs and error handling. |
| internal/rpc/rpc.go | Introduced a BatchIDCounter and updated ID retrieval for responses. |
| internal/rpc/response.go | Added a GetID() method and adjusted error code extraction in GetError(). |
| internal/rpc/request.go | Re-ordered SerializeRequest and added GetID() for requests. |
| internal/rpc/batch.go | Implemented JSON marshalling/unmarshalling for batch requests/responses. |
| internal/provider_test.go | Updated tests to use batch requests instead of single requests. |
| internal/provider.go | Changed Healthcheck to wrap single requests into a batch and adjusted error extraction. |
| internal/metrics/metrics.go | Separated success and failure metrics calls. |
| internal/http.go | Modified HTTP proxy and request handling to support batching, including header propagation and metrics recording. |
| internal/endpoint.go | Added support for custom headers in the endpoint configuration. |
| for _, res := range req.Requests { | ||
| if req.IsBatch { | ||
| log.Debug("request", "rpc_id", res.GetID(), "method", res.Method) |
There was a problem hiding this comment.
The loop variable name 'res' in this iteration over req.Requests may be confused with the BatchResponse variable 'res'. Consider renaming it (e.g., to 'reqItem') to improve readability and reduce shadowing.
| for _, res := range req.Requests { | |
| if req.IsBatch { | |
| log.Debug("request", "rpc_id", res.GetID(), "method", res.Method) | |
| for _, reqItem := range req.Requests { | |
| if req.IsBatch { | |
| log.Debug("request", "rpc_id", reqItem.GetID(), "method", reqItem.Method) |
| for i, res := range res.Responses { | ||
| method := req.Requests[i].Method | ||
|
|
||
| if res.IsError() { |
There was a problem hiding this comment.
The inner loop reuses the variable name 'res', which shadows the outer BatchResponse 'res'. Renaming the loop variable (e.g., to 'response') would clarify the intent and prevent potential confusion.
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Accessing res.Responses[0] without verifying that the Responses slice is non-empty might lead to a panic in edge cases. Consider adding a check to ensure that res.Responses has at least one element before usage.
| if len(res.Responses) == 0 { | |
| return nil, fmt.Errorf("no responses received from RPC request") | |
| } |
| errorCode, _ := err["code"].(float64) | ||
|
|
There was a problem hiding this comment.
[nitpick] Silently ignoring the failure of the type assertion when extracting the error code may mask unexpected data formats. Consider handling the case where the type assertion fails to avoid potential misinterpretation of error codes.
| errorCode, _ := err["code"].(float64) | |
| errorCodeValue, ok := err["code"] | |
| if !ok { | |
| return 0, fmt.Errorf("error code not found in error object: %v", err) | |
| } | |
| errorCode, ok := errorCodeValue.(float64) | |
| if !ok { | |
| return 0, fmt.Errorf("error code is not a float64: %v", errorCodeValue) | |
| } |
Fully support request batching for HTTP
For websockets, the incoming batch will be sent as individual messages to the upstream