Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add distinct ways to deal with probes #824

Merged
merged 2 commits into from
May 11, 2024
Merged

Conversation

renancloudwalk
Copy link
Contributor

No description provided.

@renancloudwalk renancloudwalk requested a review from a team as a code owner May 11, 2024 17:23
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

3, because the PR involves changes to the RPC server functionality, including the addition of new structures and handling different types of probes. Understanding the context and ensuring the changes are correct requires a moderate level of effort.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The net_listening function now returns a JSON value wrapped in an anyhow::Result, but the error handling is not explicitly shown in the PR. This could lead to unhandled errors if not properly implemented.

Code Clarity: The TODO comment in the net_listening function suggests a future feature that affects the readiness probe but does not implement it now. This could lead to confusion or forgotten updates.

🔒 Security concerns

No

Code feedback:
relevant filesrc/eth/rpc/rpc_server.rs
suggestion      

Consider implementing error handling for the net_listening function to manage potential failures in probe handling. This could involve catching errors from the next_rpc_param_or_default function and responding appropriately. [important]

relevant lineasync fn net_listening(params: Params<'_>, _: Arc) -> anyhow::Result {

relevant filesrc/eth/rpc/rpc_server.rs
suggestion      

It's recommended to handle the None case explicitly in the probe type matching to ensure clarity and maintainability. This could involve adding a specific log message or handling for when probe_type is None. [medium]

relevant line_ => tracing::info!("General check or unspecified probe type"),

Copy link

PR Code Suggestions ✨

CategorySuggestions                                                                                                                                                       
Best practice
Replace unwrap() with error propagation to enhance error handling.

Replace the unwrap() call with a more robust error handling mechanism to prevent potential
panics if serialization fails. Use ? to propagate the error.

src/eth/rpc/rpc_server.rs [201]

-Ok(serde_json::to_value(timestamp).unwrap())
+Ok(serde_json::to_value(timestamp)?)
 
Use specific error types for clearer error handling.

Use a more specific error type or custom error handling instead of using anyhow::Result to
provide clearer error information and handling.

src/eth/rpc/rpc_server.rs [210]

-async fn net_listening(params: Params<'_>, _: Arc<RpcContext>) -> anyhow::Result<JsonValue, RpcError> {
+async fn net_listening(params: Params<'_>, _: Arc<RpcContext>) -> Result<JsonValue, CustomError> {
 
Enhancement
Add detailed logging for unspecified probe types.

Consider adding more specific logging for the error case in the probe type matching, which
will help in debugging and maintaining the code.

src/eth/rpc/rpc_server.rs [218]

+None(probe_type) => tracing::warn!("Unspecified probe type: {:?}", probe_type),
 _ => tracing::info!("General check or unspecified probe type"),
 
Use an enum for probe_type to enhance type safety and clarity.

Consider using a structured enum for probe_type instead of a string to enforce
compile-time checks and avoid runtime errors.

src/eth/rpc/rpc_server.rs [206]

-probe_type: Option<String>,  // This will be optional
+enum ProbeType {
+    Liveness,
+    Startup,
+    Readiness,
+    Unspecified
+}
+probe_type: Option<ProbeType>,
 
Maintainability
Explicitly handle the None case in the match statement for probe types.

Refactor the match statement to handle the None case explicitly for better clarity and
maintainability.

src/eth/rpc/rpc_server.rs [214-218]

 match probe_params.probe_type.as_deref() {
     Some("liveness") => tracing::info!("Performing liveness check"),
     Some("startup") => tracing::info!("Performing startup check"),
     Some("readiness") => tracing::info!("Performing readiness check"),
-    _ => tracing::info!("General check or unspecified probe type"),
+    None => tracing::info!("No probe type specified"),
+    _ => tracing::info!("Unrecognized probe type"),
 }
 

@renancloudwalk renancloudwalk merged commit 640fb52 into main May 11, 2024
25 checks passed
@renancloudwalk renancloudwalk deleted the rpc-server branch May 11, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant