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

fix: do not panic on evm thread because of tracing #1057

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Jun 10, 2024

Instead of passing the span ID and creating a new span in the EVM thread, we create the span in the client thread and only enter it inside the EVM thread.

@dinhani-cw dinhani-cw requested a review from a team as a code owner June 10, 2024 20:56
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]

2

🧪 Relevant tests

No

🔒 Security concerns

No

⚡ Key issues to review

Thread Safety:
The PR modifies how spans are handled in tracing by directly passing the Span object instead of the span ID. Ensure that this change does not affect thread safety or the correct propagation of the span context across threads.

Logging Consistency:
The removal of manual span entry and relying on automatic span handling using #[tracing::instrument] should be checked for consistency in logging practices across the application.

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Add error handling for the send operation to prevent silent failures

Consider adding error handling for the send operation on evm_tx. Currently, if the send
operation fails (e.g., if the receiver has been dropped), the error is not handled, which
could lead to unreported issues.

src/eth/executor.rs [361]

-self.evm_tx.send(EvmTask::new(evm_input, execution_tx))?;
+self.evm_tx.send(EvmTask::new(evm_input, execution_tx)).expect("Failed to send EvmTask");
 
Suggestion importance[1-10]: 9

Why: Adding error handling for the send operation is important to prevent silent failures, which could lead to unreported issues. This suggestion addresses a potential bug and improves the robustness of the code.

9
Possible issue
Improve span handling to ensure proper tracing context management

Ensure that the span is properly entered and exited to maintain accurate tracing context.
The current implementation may not correctly handle the span lifecycle, especially in
asynchronous contexts.

src/config.rs [236]

 let _span_enter = task.span.enter();
+// Ensure to exit the span at the end of the scope or handle it properly in asynchronous contexts
 
Suggestion importance[1-10]: 8

Why: This suggestion addresses a potential issue with span lifecycle management in asynchronous contexts, which is crucial for maintaining accurate tracing. However, the suggestion could be more specific about how to handle the span exit.

8
Best practice
Modify the tracing::instrument attribute to explicitly skip sensitive or verbose fields

The #[tracing::instrument] attribute is used without specifying fields to skip, which
might inadvertently capture sensitive or verbose data. Explicitly specify fields to skip
to protect sensitive information and reduce verbosity.

src/eth/executor.rs [358]

-#[tracing::instrument(name = "executor::evm", skip_all)]
+#[tracing::instrument(name = "executor::evm", skip(self, evm_input))]
 
Suggestion importance[1-10]: 8

Why: Modifying the tracing::instrument attribute to explicitly skip sensitive or verbose fields is a best practice that enhances security and performance by preventing the capture of unnecessary data.

8
Enhancement
Enhance the info_span! usage by adding relevant attributes for better traceability

The info_span! macro is used without specifying any attributes. Consider adding relevant
attributes to enhance the tracing output, such as including task-specific data or
identifiers.

src/eth/executor.rs [59]

-span: info_span!("evm::task"),
+span: info_span!("evm::task", task_id = %input.task_id, "Starting EVM task"),
 
Suggestion importance[1-10]: 7

Why: Enhancing the info_span! usage by adding relevant attributes can improve traceability and debugging. However, the suggestion assumes the presence of task_id in input, which may not always be the case.

7

@dinhani-cw dinhani-cw merged commit 81f3336 into main Jun 10, 2024
33 checks passed
@dinhani-cw dinhani-cw deleted the evm-tracing branch June 10, 2024 21:04
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