chore: importer-offline: add context-id to logs

[marcospb19-cw]

#806 reopened.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves multiple changes across different files including the addition of new functionality (context ID generation and instrumentation), and modifications to existing logic. The changes are spread across Rust source files and the Cargo.toml, requiring a thorough understanding of the existing codebase and the new tracing and instrumentation logic.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The use of new_context_id() in multiple spans might lead to different context IDs for related operations within the same logical block of work, potentially making it harder to trace all operations under a single context. Consider using a single context ID per logical operation.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/bin/importer_offline.rs
suggestion	Consider initializing context_id once at the start of each logical operation (e.g., block import or RPC call) and reusing it for all spans within that operation to ensure all related logs are easily traceable under the same context. This change enhances traceability and debugging efficiency. [important]
relevant line	let span = info_span!("re-executing block", context_id = new_context_id());

---

relevant file	src/bin/importer_offline.rs
suggestion	To avoid potential deadlocks or performance issues with asynchronous tasks, ensure that the .await inside the instrumented block does not lead to unintended blocking or delays, especially since tracing and logging might introduce slight overhead. Consider profiling the async block to ensure performance is not adversely affected. [important]
relevant line	.await?;

---

relevant file	src/utils.rs
suggestion	Since new_context_id() generates a new UUID for each call, consider adding caching or a context management strategy if the application's logic requires frequent ID generation, to improve performance and reduce overhead. [medium]
relevant line	pub fn new_context_id() -> String {

---

relevant file	src/bin/importer_offline.rs
suggestion	When adding new tracing spans, ensure that the span names and fields are consistent and follow a naming convention that aligns with the rest of the application. This consistency helps in maintaining readable and manageable logs. [medium]
relevant line	#[tracing::instrument(name = "[Importer]", skip_all)]

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestions
Enhancement	Add error handling for asynchronous block execution to improve robustness. Consider handling the error from the await operation inside the async block. Currently, the ? operator is used, which will propagate the error, but it might be beneficial to handle specific errors gracefully to maintain the flow of block processing. src/bin/importer_offline.rs [190] -executor.reexecute_external(&block, &receipts).await?; +match executor.reexecute_external(&block, &receipts).await { + Ok(_) => {}, + Err(e) => { + // Handle specific errors or log them + tracing::error!("Error re-executing block: {:?}", e); + continue; // Skip this block or handle it differently + } +}
	Add error handling for the commit operation to enhance application stability. Consider adding error handling for the await operation when committing the mined block. This operation could fail, and handling this error could prevent the application from crashing or behaving unexpectedly. src/bin/importer_offline.rs [199] -miner.commit(mined_block.clone()).await?; +if let Err(e) = miner.commit(mined_block.clone()).await { + tracing::error!("Failed to commit mined block: {:?}", e); + // Additional error handling logic here +}
Best practice	Modify the tracing instrument attribute to skip specific parameters instead of all. To avoid potential data races or unexpected behavior, consider using a more specific skip directive in the #[tracing::instrument] attribute. Instead of skip_all, explicitly list the parameters to skip, ensuring that important context is still logged. src/bin/importer_offline.rs [149] -#[tracing::instrument(name = "[Importer]", skip_all)] +#[tracing::instrument(name = "[Importer]", skip(executor, miner, storage, csv))]
	Move the instrument call to the task creation line to correctly associate the span. The instrument call on the task should be moved to where the task is actually created (load_blocks_and_receipts) to ensure that the span is correctly associated with the entire task's execution context. src/bin/importer_offline.rs [245-246] -let task = load_blocks_and_receipts(Arc::clone(&rpc_storage), cancellation.clone(), start, end); -let task = task.instrument(span); +let task = load_blocks_and_receipts(Arc::clone(&rpc_storage), cancellation.clone(), start, end).instrument(span);
Maintainability	Refactor the large async block into a separate asynchronous function for better maintainability. To improve the performance and readability of the loop that processes blocks, consider refactoring the large async block into a separate asynchronous function. This will make the code cleaner and potentially easier to maintain and test. src/bin/importer_offline.rs [185-211] -async { - // large block of code +async fn process_block(block: Block, receipts: Receipts, miner: &Miner, storage: &Storage) -> Result<(), Error> { + // Refactored code here }

[marcospb19-cw]

Usability is considerably worse, due to limitations with tracing, the custom formatter will allow us to create a better API.

But this one works for now.