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 relayer client with retries and metrics #1059

Merged
merged 1 commit into from
Jun 10, 2024

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 21:53
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]

3

🧪 Relevant tests

No

🔒 Security concerns

No

⚡ Key issues to review

Retry Logic:
The retry logic in send_to_relayer does not include a delay between retries, which might be necessary to handle transient issues effectively.

Logging and Monitoring:
While logging and metrics have been added, it's important to ensure that metrics are correctly capturing failures and retries. Consider adding metrics for retry attempts.

Blocking Task:
The send_to_relayer function is performing database operations which are I/O bound and should ideally be executed in a separate thread to avoid blocking the async executor.

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Add specific error handling for JSON serialization to improve robustness

To ensure that the block_json variable is correctly formatted and does not cause
serialization issues, consider adding error handling specifically for the JSON
serialization process.

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

-let block_json = serde_json::to_value(block)?;
+let block_json = serde_json::to_value(block).map_err(|e| anyhow!("Failed to serialize block: {}", e))?;
 
Suggestion importance[1-10]: 10

Why: Adding specific error handling for JSON serialization improves robustness by ensuring that serialization issues are caught and handled appropriately, preventing potential runtime errors.

10
Performance
Use exponential backoff for retry delays to improve handling of transient failures

Implement exponential backoff for retry delays to handle failures more efficiently. This
approach increases the delay between retries progressively, which can be more effective in
handling temporary failures that need more time to recover.

src/eth/relayer/external.rs [329-338]

+let mut delay = 1;
 while remaining_tries > 0 {
     if let Err(err) = sqlx::query!("INSERT INTO relayer_blocks (number, payload) VALUES ($1, $2)", block_number as _, &block_json)
         .execute(&self.pool)
         .await
     {
         remaining_tries -= 1;
         tracing::warn!(?err, ?remaining_tries, "failed to insert into relayer_blocks");
+        tokio::time::sleep(tokio::time::Duration::from_secs(delay)).await;
+        delay *= 2;
     } else {
         break;
     }
 }
 
Suggestion importance[1-10]: 9

Why: Exponential backoff is a well-known strategy for handling transient failures more efficiently. It progressively increases the delay between retries, which can be more effective in scenarios where temporary failures need more time to resolve.

9
Enhancement
Add a delay between retries to handle transient failures more effectively

Consider adding a delay between retries when attempting to insert into relayer_blocks.
This can help in scenarios where the failure is due to transient issues that may resolve
in a short time. Using a delay can increase the likelihood of a successful retry.

src/eth/relayer/external.rs [329-338]

 while remaining_tries > 0 {
     if let Err(err) = sqlx::query!("INSERT INTO relayer_blocks (number, payload) VALUES ($1, $2)", block_number as _, &block_json)
         .execute(&self.pool)
         .await
     {
         remaining_tries -= 1;
         tracing::warn!(?err, ?remaining_tries, "failed to insert into relayer_blocks");
+        tokio::time::sleep(tokio::time::Duration::from_secs(1)).await;
     } else {
         break;
     }
 }
 
Suggestion importance[1-10]: 8

Why: Adding a delay between retries can help handle transient issues more effectively, increasing the likelihood of a successful retry. This is a practical enhancement for robustness.

8
Best practice
Add a log statement for successful database insertions for better traceability

It's a good practice to log the successful insertion into the database for better
traceability and debugging. Consider adding a success log statement when the insertion is
successful.

src/eth/relayer/external.rs [330-338]

 if let Err(err) = sqlx::query!("INSERT INTO relayer_blocks (number, payload) VALUES ($1, $2)", block_number as _, &block_json)
     .execute(&self.pool)
     .await
 {
     remaining_tries -= 1;
     tracing::warn!(?err, ?remaining_tries, "failed to insert into relayer_blocks");
 } else {
+    tracing::info!("Successfully inserted block number {} into relayer_blocks", block_number);
     break;
 }
 
Suggestion importance[1-10]: 7

Why: Adding a log statement for successful insertions improves traceability and debugging, which is beneficial for monitoring and maintaining the system.

7

@carneiro-cw carneiro-cw merged commit b6df5b9 into main Jun 10, 2024
33 checks passed
@carneiro-cw carneiro-cw deleted the improve_relayer_client branch June 10, 2024 22:01
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