Avoid logging debug info for invalid JSON-RPC methods #611
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue,
zhangchiqing and
ramtinms
as code owners
WalkthroughThe changes introduce a new variable Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
api/api.go (2)
35-87: LGTM! Consider using a constant for the empty struct value.The
validMethodsmap is a comprehensive and memory-efficient implementation of a set containing all valid Ethereum JSON-RPC API methods. This is a good addition that will help in method validation.Consider defining a constant for the empty struct value to improve readability:
+const void struct{} var validMethods = map[string]struct{}{ - "eth_blockNumber": {}, + "eth_blockNumber": void, // ... (apply to all entries) }
Line range hint
1-1191: Consider improving error handling and logging consistency.While reviewing the existing API endpoint implementations, I noticed a few areas where improvements could be made:
Error handling could be more consistent across different methods. Some methods use the
handleErrorfunction, while others directly return errors.Some methods could benefit from additional logging, especially for debugging purposes.
The
handleErrorfunction (lines 1146-1166) could be simplified by using a switch statement instead of multiple if-else conditions.Consider implementing these improvements to enhance the overall code quality and maintainability. Here's an example of how the
handleErrorfunction could be refactored:func handleError[T any](err error, log zerolog.Logger, collector metrics.Collector) (T, error) { var zero T var revertedErr *errs.RevertError switch { case errors.Is(err, errs.ErrEntityNotFound): return zero, nil case errors.Is(err, errs.ErrInvalid), errors.Is(err, errs.ErrFailedTransaction): return zero, err case errors.As(err, &revertedErr): return zero, revertedErr default: collector.ApiErrorOccurred() log.Error().Err(err).Msg("api error") return zero, errs.ErrInternal } }Additionally, consider adding more consistent logging across methods, especially for debugging purposes. For example:
func (b *BlockChainAPI) GetTransactionCount( ctx context.Context, address common.Address, blockNumberOrHash rpc.BlockNumberOrHash, ) (*hexutil.Uint64, error) { l := b.logger.With(). Str("endpoint", "getTransactionCount"). Str("address", address.String()). Logger() l.Debug().Msg("Processing getTransactionCount request") // Add debug logging // ... (rest of the method implementation) l.Debug().Uint64("nonce", nonce).Msg("Transaction count retrieved") // Add debug logging for the result return (*hexutil.Uint64)(&networkNonce), nil }Implementing these suggestions will improve code consistency, error handling, and debugging capabilities across the API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- api/api.go (1 hunks)
- api/server.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
api/api.go (2)
89-94: LGTM! The IsValidMethod function is well-implemented.The
IsValidMethodfunction is a simple and efficient way to check if a given method name is valid. It correctly utilizes thevalidMethodsmap for this purpose.
Line range hint
1-1191: Overall, the changes and existing code are well-implemented with room for minor improvements.The introduction of the
validMethodsmap andIsValidMethodfunction enhances the API's capability to validate incoming requests. The existing code is generally well-structured and follows consistent patterns.Some suggestions for further improvement include:
- Enhancing error handling consistency across methods.
- Adding more debug logging for better traceability.
- Simplifying the
handleErrorfunction.These changes would further improve the code's maintainability and debugging capabilities.
api/server.go (1)
489-492: Logging adjustments for error responses look goodThe updates to the error logging correctly prevent known errors, such as method not found, from generating unnecessary debug log entries. This helps reduce noise in the logs while still providing visibility into unexpected issues.
api/server.go
Outdated
| method, ok := requestBody["method"] | ||
| // Do not log any debug info for methods that are not valid | ||
| // JSON-RPC methods. | ||
| if ok && IsValidMethod(method.(string)) { | ||
| h.logger.Debug(). | ||
| Str("IP", r.RemoteAddr). | ||
| Str("url", r.URL.String()). | ||
| Fields(requestBody). | ||
| Bool("is-ws", isWebSocket(r)). | ||
| Msg("API request") | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix potential panic due to unsafe type assertion
The type assertion method.(string) can cause a panic if method is not of type string. To prevent this, you should perform a safe type assertion to ensure the program does not crash if method is not a string.
Apply the following fix to safely handle the type assertion:
-method, ok := requestBody["method"]
-// Do not log any debug info for methods that are not valid
-// JSON-RPC methods.
-if ok && IsValidMethod(method.(string)) {
+if methodValue, ok := requestBody["method"]; ok {
+ if methodStr, ok := methodValue.(string); ok && IsValidMethod(methodStr) {
h.logger.Debug().
Str("IP", r.RemoteAddr).
Str("url", r.URL.String()).
Fields(requestBody).
Bool("is-ws", isWebSocket(r)).
Msg("API request")
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| method, ok := requestBody["method"] | |
| // Do not log any debug info for methods that are not valid | |
| // JSON-RPC methods. | |
| if ok && IsValidMethod(method.(string)) { | |
| h.logger.Debug(). | |
| Str("IP", r.RemoteAddr). | |
| Str("url", r.URL.String()). | |
| Fields(requestBody). | |
| Bool("is-ws", isWebSocket(r)). | |
| Msg("API request") | |
| } | |
| if methodValue, ok := requestBody["method"]; ok { | |
| if methodStr, ok := methodValue.(string); ok && IsValidMethod(methodStr) { | |
| h.logger.Debug(). | |
| Str("IP", r.RemoteAddr). | |
| Str("url", r.URL.String()). | |
| Fields(requestBody). | |
| Bool("is-ws", isWebSocket(r)). | |
| Msg("API request") | |
| } | |
| } |
m-Peter
force-pushed
the
avoid-logs-for-unknown-json-rpc-methods
branch
from
f2b539d to
0324b75
Compare
@franklywatson I have done that in this line: 0324b75#diff-bf98d5fab5bcfabded9769069e7318683f549ec91cff84c5f87a9ae3a3ca3d8dR37 |
Description
When the EVM Gateway receives requests with unknown JSON-RPC methods, we still log the following two lines:
{ "level": "debug", "version": "development", "component": "API", "IP": "[::1]:60022", "url": "/", "id": 1, "jsonrpc": "2.0", "method": "eth_etc/passwd", "params": [], "is-ws": false, "time": "2024-10-08T17:08:57+03:00", "message": "API request" } { "level": "debug", "version": "development", "component": "API", "id": 1, "jsonrpc": "2.0", "method": "eth_etc/passwd", "params": [], "error": "the method eth_etc/passwd does not exist/is not available", "code": -32601, "data": null, "time": "2024-10-08T17:08:57+03:00", "message": "API error response" }This creates a lot of noise on the logged data, and in tools such as Grafana, so we completely avoid logging anything in this case.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Improvements
Code Cleanup