Improve Orchestrator error logs #575
Conversation
WalkthroughThis pull request focuses on enhancing error logging and error handling across multiple Rust files in the orchestrator and relayer components. The changes involve updating error messages to include specific error details, improving the context provided during failures. Modifications span several functions, targeting operations such as retrieving block numbers, event logs, and chain IDs, as well as handling logic call submissions. Additionally, the error handling in the retrieval of blockchain event logs has been improved with a match-based approach, allowing for better recovery from transient errors. Changes
Sequence DiagramsequenceDiagram
participant Client
participant EthNode
participant OracleResync
Client->>EthNode: Request Event Logs
alt Log Retrieval Successful
EthNode-->>OracleResync: Return Event Logs
OracleResync->>OracleResync: Process Logs
else Log Retrieval Failed
EthNode--xOracleResync: Error
OracleResync->>OracleResync: Log Error
OracleResync->>OracleResync: Delay and Retry
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
orchestrator/orchestrator/src/oracle_resync.rs (1)
80-123: LGTM! Comprehensive error handling for event logs.The implementation properly handles errors for each event type and includes retry mechanism for transient failures.
Consider reducing code duplication.
The error handling pattern is repeated for each event type. Consider extracting this into a helper function to improve maintainability.
async fn get_logs_with_retry<T>( eth_client: &EthClient, filter: &Filter, event_type: &str, ) -> Result<Vec<Log>, ethers::providers::ProviderError> { match eth_client.get_logs(filter).await { Ok(events) => Ok(events), Err(e) => { error!("Failed to get {} events (may be transient): {:?}", event_type, e); delay_for(RETRY_TIME).await; Err(e) } } }Usage example:
let erc20_deployed_events = match get_logs_with_retry( ð_client, &erc20_deployed_filter, "ERC20 deployed" ).await { Ok(events) => events, Err(_) => continue, };orchestrator/orchestrator/src/main_loop.rs (1)
260-260: LGTM! Consider adding more context to the error message.The addition of the error value improves debugging capabilities. However, consider adding more context about what the GravityID is used for to help operators understand the impact of this failure.
- error!("Failed to get GravityID, check your Eth node: {:?}", gravity_id); + error!("Failed to get GravityID (required for message signing), check your Eth node: {:?}", gravity_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
orchestrator/Cargo.lockis excluded by!**/*.lock,!**/*.lockorchestrator/Cargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (5)
orchestrator/orchestrator/src/get_with_retry.rs(3 hunks)orchestrator/orchestrator/src/main_loop.rs(1 hunks)orchestrator/orchestrator/src/oracle_resync.rs(1 hunks)orchestrator/relayer/src/logic_call_relaying.rs(1 hunks)orchestrator/relayer/src/main_loop.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: solidity-build
- GitHub Check: go-build
- GitHub Check: rust-test
- GitHub Check: hardhat-build
- GitHub Check: go-test
- GitHub Check: rust-build
- GitHub Check: local-tests (16.x)
- GitHub Check: node-build (16.x)
- GitHub Check: Clippy
🔇 Additional comments (5)
orchestrator/orchestrator/src/get_with_retry.rs (3)
17-17: LGTM! Improved error message clarity.The error message now includes the actual error result, which will help with debugging Ethereum node issues.
32-32: LGTM! Enhanced error context.Good addition of "(may be transient)" to clarify that GRPC connectivity issues could be temporary.
45-45: LGTM! Consistent error message structure.The error message follows the same pattern as other retry functions, maintaining consistency in error reporting.
orchestrator/relayer/src/main_loop.rs (1)
27-27: LGTM! Enhanced GravityID error logging.The error message now includes the specific error details, which will help diagnose Ethereum node connectivity issues.
orchestrator/relayer/src/logic_call_relaying.rs (1)
172-172: LGTM! Improved error visibility for logic call failures.The error message now includes the specific error result, which will help diagnose submission failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
orchestrator/relayer/src/batch_relaying.rs (1)
Line range hint
71-75: Enhance error logging to provide more context.To align with the PR's objective of improving error logs, consider enhancing these error messages:
let latest_batches = if let Ok(lb) = get_latest_transaction_batches(grpc_client).await { lb } else { + error!("Failed to get latest transaction batches from Cosmos: {:?}", latest_batches); return HashMap::new(); }; // ... later in the code ... } else { error!( - "could not get signatures for {}:{} with {:?}", + "Failed to retrieve batch signatures from Cosmos - Token: {}, Nonce: {}, Error: {:?}", batch.token_contract, batch.nonce, sigs ); }Also applies to: 89-93
🧹 Nitpick comments (8)
orchestrator/relayer/src/batch_relaying.rs (2)
109-109: LGTM! Explicit return improves code clarity.The explicit return statement makes the function's output clearer, which is especially important for this complex function that handles batch processing and signature validation.
Consider adding a brief comment above the return statement to document what the returned HashMap represents, e.g.:
+ // Return mapping of token contracts to their submittable batches with valid signatures possible_batches
Line range hint
71-109: Consider implementing structured error handling.The function could benefit from more structured error handling to improve error reporting and maintainability:
- Define a custom error type for batch processing:
#[derive(Debug)] pub enum BatchProcessingError { FetchBatchesFailed(String), FetchSignaturesFailed { token: EthAddress, nonce: u64, error: String }, InvalidSignatures { token: EthAddress, nonce: u64 }, }
- Update the function signature to use Result:
async fn get_batches_and_signatures( current_valset: Valset, grpc_client: &mut GravityQueryClient<Channel>, gravity_id: String, ) -> Result<HashMap<EthAddress, Vec<SubmittableBatch>>, BatchProcessingError>This would provide:
- More structured error reporting
- Better error handling in calling code
- Clearer error propagation paths
orchestrator/orchestrator/src/oracle_resync.rs (6)
80-90: Enhance error context for ERC20 deployed events.While the error handling is good, consider including additional context such as the block range being searched to help with debugging.
let erc20_deployed_events = match eth_client.get_logs(&erc20_deployed_filter).await { Ok(events) => events, Err(e) => { error!( - "Failed to get ERC20 deployed events (may be transient): {:?}", + "Failed to get ERC20 deployed events in block range {:?}-{:?} (may be transient): {:?}", + start_search_block, end_search_block, e ); delay_for(RETRY_TIME).await; continue; } };
91-101: Enhance error context for logic call events.Similar to the ERC20 events, include the block range in the error message for better debugging context.
let logic_call_events = match eth_client.get_logs(&logic_call_filter).await { Ok(events) => events, Err(e) => { error!( - "Failed to get logic call events (may be transient): {:?}", + "Failed to get logic call events in block range {:?}-{:?} (may be transient): {:?}", + start_search_block, end_search_block, e ); delay_for(RETRY_TIME).await; continue; } };
102-112: Enhance error context for send to cosmos events.Include the block range in the error message to maintain consistency and improve debugging capabilities.
let send_to_cosmos_events = match eth_client.get_logs(&send_to_cosmos_filter).await { Ok(events) => events, Err(e) => { error!( - "Failed to get send to cosmos events (may be transient): {:?}", + "Failed to get send to cosmos events in block range {:?}-{:?} (may be transient): {:?}", + start_search_block, end_search_block, e ); delay_for(RETRY_TIME).await; continue; } };
113-123: Enhance error context for transaction batch events.Include the block range in the error message for consistency with other event handlers.
let transaction_batch_events = match eth_client.get_logs(&transaction_batch_filter).await { Ok(events) => events, Err(e) => { error!( - "Failed to get transaction batch events (may be transient): {:?}", + "Failed to get transaction batch events in block range {:?}-{:?} (may be transient): {:?}", + start_search_block, end_search_block, e ); delay_for(RETRY_TIME).await; continue; } };
128-138: Enhance error context for valset updated events.Given the special significance of valset events (especially nonce 0 for contract initialization), include both block range and additional context in the error message.
let mut valset_updated_events = match eth_client.get_logs(&valset_updated_filter).await { Ok(events) => events, Err(e) => { error!( - "Failed to get valset updated events (may be transient): {:?}", + "Failed to get valset updated events in block range {:?}-{:?} (including potential contract initialization events) (may be transient): {:?}", + start_search_block, end_search_block, e ); delay_for(RETRY_TIME).await; continue; } };
80-138: Consider implementing exponential backoff for retries.While the current retry mechanism is functional, consider implementing an exponential backoff strategy for transient errors. This would help prevent overwhelming the Ethereum node during periods of instability.
You could create a helper function like this:
async fn retry_with_backoff<T, E, F: Future<Output = Result<T, E>>>( operation: impl Fn() -> F, max_retries: u32, initial_delay: Duration, ) -> Result<T, E> { let mut delay = initial_delay; let mut attempts = 0; loop { match operation().await { Ok(result) => return Ok(result), Err(e) => { attempts += 1; if attempts >= max_retries { return Err(e); } delay_for(delay).await; delay *= 2; // Exponential backoff } } } }This would provide more robust handling of transient network issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
orchestrator/cosmos_gravity/src/query.rs(3 hunks)orchestrator/orchestrator/src/get_with_retry.rs(3 hunks)orchestrator/orchestrator/src/main_loop.rs(1 hunks)orchestrator/orchestrator/src/oracle_resync.rs(1 hunks)orchestrator/relayer/src/batch_relaying.rs(1 hunks)orchestrator/relayer/src/main_loop.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- orchestrator/cosmos_gravity/src/query.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- orchestrator/relayer/src/main_loop.rs
- orchestrator/orchestrator/src/main_loop.rs
- orchestrator/orchestrator/src/get_with_retry.rs
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: solidity-build
- GitHub Check: rust-test
- GitHub Check: go-build
- GitHub Check: rust-build
- GitHub Check: go-test
- GitHub Check: Clippy
- GitHub Check: local-tests (16.x)
- GitHub Check: node-build (16.x)
- GitHub Check: hardhat-build
Summary by CodeRabbit
Bug Fixes
Chores