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

feat: improving tracing configuration and adding tokio-console #881

Merged
merged 3 commits into from
May 21, 2024

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented May 21, 2024

  • Improved tracing initialization.
  • Added tokio-console support with console-subscriber.
  • spawn_named function for spawning tasks with an associated name.
  • warn_task_cancellation and warn_task_tx_closed functions to be used in common task exiting scenarios.

@dinhani-cw dinhani-cw requested a review from a team as a code owner May 21, 2024 05:09
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

4, due to the complexity and breadth of the changes across multiple files and systems, including task spawning, tracing, and logging configurations. The PR modifies critical infrastructure components which require careful review to ensure system stability and performance.

🧪 Relevant tests

No

⚡ Possible issues

Possible Bug: The use of not(sub.is_closed()) in rpc_subscriptions.rs might be incorrect if not is intended to be a logical NOT operation. Typically, Rust uses ! for logical negation. This could lead to logical errors in subscription handling.

Error Handling: The .expect("message") used after spawning tasks could be replaced with proper error handling to avoid potential panics in production.

🔒 Security concerns

No

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

Replace not(sub.is_closed()) with !sub.is_closed() to correctly use Rust's logical negation, ensuring that the logic for retaining subscriptions works as intended. [important]

relevant linesubs.new_pending_txs.write().await.retain(|_, sub| not(sub.is_closed()));

relevant filesrc/eth/rpc/rpc_subscriptions.rs
suggestion      

Consider handling errors from task spawning gracefully rather than using .expect(). Implement a logging mechanism for the error and continue without panicking. [important]

relevant line.expect("spawning rpc subscription cleaner notifier should not fail")

relevant filesrc/infra/tracing.rs
suggestion      

Ensure that the console_server.serve().await is properly handled by logging the error without stopping the execution of the program, which could improve the robustness of the tracing initialization. [medium]

relevant linetracing::error!(reason = ?e, "failed to start tokio-console server");

relevant filesrc/infra/tracing.rs
suggestion      

Use structured logging for the initialization steps in init_tracing to provide more context in logs, which can be helpful for debugging and monitoring. Replace println! with tracing::info! or similar. [medium]

relevant lineprintln!("tracing enabling json logs");

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Correct the logical negation to use the Rust '!' operator instead of a non-existent 'not' function

Replace the incorrect usage of not function with the correct Rust syntax for negation. The
not function is not a built-in Rust function for logical negation. Instead, use the !
operator for negating boolean expressions.

src/eth/rpc/rpc_subscriptions.rs [71-73]

-subs.new_pending_txs.write().await.retain(|_, sub| not(sub.is_closed()));
-subs.new_heads.write().await.retain(|_, sub| not(sub.is_closed()));
-subs.logs.write().await.retain(|_, (sub, _)| not(sub.is_closed()));
+subs.new_pending_txs.write().await.retain(|_, sub| !sub.is_closed());
+subs.new_heads.write().await.retain(|_, sub| !sub.is_closed());
+subs.logs.write().await.retain(|_, (sub, _)| !sub.is_closed());
 
Suggestion importance[1-10]: 10

Why: The suggestion correctly identifies a potential bug due to the use of a non-existent not function and provides the correct Rust syntax for logical negation, which is crucial for the proper functioning of the code.

10
Error handling
Improve error handling in the task spawning to prevent application panic and ensure graceful shutdown on errors

Handle the potential error from the expect method more gracefully by logging the error and
returning a controlled error response instead of panicking.

src/eth/rpc/rpc_subscriptions.rs [84]

-.expect("spawning rpc subscription cleaner notifier should not fail")
+.unwrap_or_else(|e| {
+    tracing::error!("Failed to spawn rpc subscription cleaner notifier: {:?}", e);
+    std::process::exit(1);
+})
 
Suggestion importance[1-10]: 8

Why: The suggestion improves error handling by preventing the application from panicking and ensuring a graceful shutdown, which is important for robustness and maintainability.

8
Best practice
Use expect instead of unwrap for better error messaging and debugging

Replace the unwrap method with expect to provide a clearer error message in case of
failure when installing the OpenTelemetry pipeline.

src/infra/tracing.rs [57]

 .install_batch(runtime::Tokio)
-.unwrap();
+.expect("Failed to install OpenTelemetry pipeline");
 
Suggestion importance[1-10]: 7

Why: Using expect instead of unwrap provides better error messaging, which aids in debugging and improves code reliability. However, this is a minor improvement.

7
Maintainability
Improve variable naming for clarity and maintainability

Consider using a more descriptive variable name than url for the OpenTelemetry endpoint
configuration to enhance code readability and maintainability.

src/infra/tracing.rs [49-61]

 let opentelemetry_layer = match url {
-    Some(url) => {
+    Some(endpoint) => {
         println!("tracing enabling opentelemetry");
         let tracer = opentelemetry_otlp::new_pipeline()
             .tracing()
-            .with_exporter(opentelemetry_otlp::new_exporter().tonic().with_endpoint(url))
+            .with_exporter(opentelemetry_otlp::new_exporter().tonic().with_endpoint(endpoint))
             .with_trace_config(trace::config().with_resource(Resource::new(vec![KeyValue::new("service.name", "stratus")])))
             .install_batch(runtime::Tokio)
             .unwrap();
         Some(tracing_opentelemetry::layer().with_tracked_inactivity(false).with_tracer(tracer))
     }
     None => None,
 };
 
Suggestion importance[1-10]: 6

Why: The suggestion enhances code readability and maintainability by using a more descriptive variable name, but it is a minor improvement and not critical to the functionality.

6

@dinhani-cw dinhani-cw enabled auto-merge (squash) May 21, 2024 06:04
@dinhani-cw dinhani-cw merged commit 842ef05 into main May 21, 2024
23 checks passed
@dinhani-cw dinhani-cw deleted the tracing branch May 21, 2024 06:46
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