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

feat: more logs #938

Merged
merged 4 commits into from
May 28, 2024
Merged

feat: more logs #938

merged 4 commits into from
May 28, 2024

Conversation

dinhani-cw
Copy link
Contributor

No description provided.

@dinhani-cw dinhani-cw requested a review from a team as a code owner May 28, 2024 01:44
@dinhani-cw dinhani-cw enabled auto-merge (squash) May 28, 2024 01:45
Copy link

PR Review 🔍

⏱️ Estimated effort to review [1-5]

2, because the changes are straightforward and involve only log message updates and a comment change. The PR is small and the logic remains unchanged, making it relatively easy to review.

🧪 Relevant tests

No

⚡ Possible issues

No

🔒 Security concerns

No

Code feedback:
relevant filesrc/eth/storage/inmemory/inmemory_temporary.rs
suggestion      

Consider adding more detailed context to the log message for better traceability. For example, include initialization parameters or state if applicable. [medium]

relevant linetracing::info!("creating inmemory temporary storage");

relevant filesrc/eth/storage/rocks/rocks_permanent.rs
suggestion      

It might be beneficial to handle potential errors from state.sync_data().await before proceeding with other operations to ensure the storage state is ready and consistent. [important]

relevant linetracing::info!("creating rocksdb storage");

relevant filesrc/infra/blockchain_client/blockchain_client.rs
suggestion      

Since the method name was changed to fetch_block_number, consider updating the log message to reflect this new terminology consistently throughout the method's implementation. [medium]

relevant line/// Fetches the current block number.

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Enhancement
Improve the log message for clarity and specificity

Consider using a more descriptive log message for the creation of in-memory temporary
storage. The current message does not specify what is being created or its purpose.

src/eth/storage/inmemory/inmemory_temporary.rs [55]

-tracing::info!("creating inmemory temporary storage");
+tracing::info!("Initializing new InMemoryTemporaryStorage instance to manage temporary states in memory.");
 
Suggestion importance[1-10]: 8

Why: The suggestion improves the clarity and specificity of the log message, making it more informative about what is being created and its purpose. This enhances the readability and maintainability of the code.

8
Enhance the log message to provide more detailed context about the RocksDB storage initialization

Update the log message to include more details about the RocksDB storage initialization,
such as the configuration or the expected operations.

src/eth/storage/rocks/rocks_permanent.rs [43]

-tracing::info!("creating rocksdb storage");
+tracing::info!("Creating new RocksDB storage instance with default configuration for persistent data management.");
 
Suggestion importance[1-10]: 8

Why: The suggested log message provides more context about the initialization process, including the configuration and purpose of the RocksDB storage. This makes the logs more useful for debugging and understanding the system's behavior.

8
Maintainability
Update the log message to match the updated method name and provide more clarity

The method name and the documentation comment have been updated to 'fetch_block_number'
but the log message still uses 'fetching block number'. It would be more consistent to
update the log message to reflect the new method name for clarity.

src/infra/blockchain_client/blockchain_client.rs [129]

-tracing::debug!("fetching block number");
+tracing::debug!("Fetching the current block number from the blockchain.");
 
Suggestion importance[1-10]: 7

Why: The suggestion improves consistency between the method name, documentation comment, and log message. This enhances code readability and maintainability, although the improvement is relatively minor.

7

@dinhani-cw dinhani-cw disabled auto-merge May 28, 2024 01:46
@dinhani-cw dinhani-cw enabled auto-merge (squash) May 28, 2024 01:59
@dinhani-cw dinhani-cw merged commit 5480425 into main May 28, 2024
31 checks passed
@dinhani-cw dinhani-cw deleted the tracing-2 branch May 28, 2024 02:06
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