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: improve spans and logs #1054

Merged
merged 2 commits into from
Jun 10, 2024
Merged

Conversation

carneiro-cw
Copy link
Contributor

No description provided.

@carneiro-cw carneiro-cw requested a review from a team as a code owner June 10, 2024 18:23
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]

2

🧪 Relevant tests

No

🔒 Security concerns

No

⚡ Key issues to review

Logging Level Change:
The logging level has been changed from debug to info. Ensure this change aligns with the intended verbosity and does not expose sensitive information in production logs.

Span Handling:
The PR replaces direct span manipulation with Span::with(...). Verify that this change correctly maintains the span context across asynchronous tasks.

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Enhancement
Implement a retry mechanism for transaction sending to enhance robustness

Consider adding a retry mechanism or a circuit breaker to handle repeated failures in the
send_raw_transaction method, enhancing the robustness of the transaction sending process.

src/eth/relayer/external.rs [248]

-tracing::warn!(?tx_hash, ?err, "failed to send raw transaction, retrying...");
+if retry_count < MAX_RETRY_LIMIT {
+    tracing::warn!(?tx_hash, ?err, "failed to send raw transaction, retrying...");
+    retry_count += 1;
+} else {
+    tracing::error!(?tx_hash, ?err, "exceeded maximum retry limit for sending transaction");
+    return Err(err);
+}
 
Suggestion importance[1-10]: 10

Why: Implementing a retry mechanism significantly enhances the robustness and reliability of the transaction sending process, addressing potential repeated failures effectively.

10
Enhance the structure and clarity of logging for transaction failures

Use structured logging for the error context in the tracing::warn call to improve the
readability and utility of log messages, especially when debugging issues related to
transaction failures.

src/eth/relayer/external.rs [248]

-tracing::warn!(?tx_hash, ?err, "failed to send raw transaction, retrying...");
+tracing::warn!(error = ?err, tx_hash = ?tx_hash, "failed to send raw transaction, retrying...");
 
Suggestion importance[1-10]: 8

Why: Structured logging improves the clarity and utility of log messages, which is beneficial for debugging and maintaining the code.

8
Possible bug
Improve error handling in the transaction fetch operation

Consider handling the unwrap_or(None) more safely by explicitly handling the error case
instead of using unwrap_or. This can prevent potential runtime panics if the
fetch_transaction method fails unexpectedly.

src/eth/relayer/external.rs [244]

-if self.substrate_chain.fetch_transaction(tx_hash).await.unwrap_or(None).is_some() {
+if let Ok(Some(_)) = self.substrate_chain.fetch_transaction(tx_hash).await {
 
Suggestion importance[1-10]: 9

Why: This suggestion enhances error handling, reducing the risk of runtime panics, which is crucial for the robustness and reliability of the code.

9
Maintainability
Replace Span::with with Span::current() to ensure consistent span handling

Replace the Span::with closure with direct usage of Span::current() to maintain
consistency with the existing span handling pattern in the codebase. This ensures that the
span context is preserved correctly across asynchronous boundaries.

src/eth/relayer/external.rs [90]

-Span::with(|s| s.rec_str("block_number", &block_number));
+let span = Span::current();
+span.rec_str("block_number", &block_number);
 
Suggestion importance[1-10]: 7

Why: The suggestion improves code consistency and maintainability by aligning with the existing span handling pattern. However, it does not address a critical issue or bug.

7

@carneiro-cw carneiro-cw merged commit c15a782 into main Jun 10, 2024
32 checks passed
@carneiro-cw carneiro-cw deleted the improve_relayer_logging_and_tracing branch June 10, 2024 18:29
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