fix: ensure log crate output is shown with tracing - #2477
Conversation
WalkthroughOhayo, sensei! The recent changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SayaArgs
participant SozoArgs
participant NodeArgs
participant LogTracer
User->>SayaArgs: Initialize logging
SayaArgs->>LogTracer: LogTracer::init()
LogTracer-->>SayaArgs: Logging initialized
SayaArgs->>SayaArgs: Set logging filter
User->>SozoArgs: Initialize logging
SozoArgs->>LogTracer: LogTracer::init()
LogTracer-->>SozoArgs: Logging initialized
SozoArgs->>SozoArgs: Set logging filter
User->>NodeArgs: Initialize logging
NodeArgs->>LogTracer: LogTracer::init()
LogTracer-->>NodeArgs: Logging initialized
NodeArgs->>NodeArgs: Set logging filter
🪧 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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (2)
bin/sozo/src/args.rs (1)
55-64: Ohayo, sensei! Great improvements to logging initialization!The changes to the
init_loggingmethod are well-thought-out and improve the overall logging configuration. Here's a breakdown of the improvements:
- The addition of
salsa=offto the default log filter is a nice touch.- Using
LogTracer::init()ensures proper setup of logging before creating the subscriber.- The new implementation is more robust, using environment variables with a fallback to the default filter.
- The code is more concise and easier to read with the use of
FmtSubscriber.Consider extracting the
tracing_subscriber::EnvFilter::try_from_default_env()logic into a separate function for better readability. Here's a suggestion:fn get_env_filter() -> tracing_subscriber::EnvFilter { tracing_subscriber::EnvFilter::try_from_default_env() .unwrap_or_else(|_| tracing_subscriber::EnvFilter::new(DEFAULT_LOG_FILTER)) } // Then in init_logging: let subscriber = FmtSubscriber::builder() .with_env_filter(get_env_filter()) .finish();This change would make the
init_loggingmethod even more concise and easier to understand.bin/saya/src/args/mod.rs (1)
78-80: Excellent adjustments to the logging configuration, sensei!The changes in the
init_loggingmethod are well thought out:
Modifying the default log filter string to disable
blockchainandproviderlogging will help reduce noise in the logs, allowing us to focus on more relevant information.The addition of
LogTracer::init()?;is crucial for integratinglogcrate output withtracing. This ensures that all logs, regardless of their source, are processed uniformly through thetracinginfrastructure.These modifications align perfectly with our goal of streamlining the logging system and ensuring
logcrate output is shown withtracing.Ohayo, sensei! Just a small suggestion to consider:
You might want to add a comment explaining the purpose of
LogTracer::init()?;for future maintainers. Something like:// Initialize LogTracer to capture logs from the `log` crate and forward them to `tracing` LogTracer::init()?;This will help others understand the importance of this line at a glance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
🔇 Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
- Cargo.toml (1 hunks)
- bin/katana/Cargo.toml (1 hunks)
- bin/katana/src/cli/node.rs (2 hunks)
- bin/saya/Cargo.toml (1 hunks)
- bin/saya/src/args/mod.rs (2 hunks)
- bin/sozo/Cargo.toml (1 hunks)
- bin/sozo/src/args.rs (2 hunks)
🔇 Additional comments not posted (10)
bin/saya/Cargo.toml (1)
21-21: Ohayo, sensei! LGTM: Tracing-log addition looks great!The addition of
tracing-log.workspace = trueis a solid move! This change aligns perfectly with our goal of ensuring thelogcrate output plays nice withtracing. By bringingtracing-loginto the workspace, we're setting up a harmonious logging ecosystem across our project. Nicely done!bin/katana/Cargo.toml (1)
30-30: Ohayo, sensei! LGTM: Adding tracing-log dependencyThe addition of
tracing-logto the dependencies is a great move! This crate bridges the gap between thelogandtracingecosystems, allowinglogevents to be captured bytracingsubscribers. It aligns perfectly with the PR objective of ensuringlogcrate output is shown withtracing.Using
.workspace = trueis also a wise choice, as it ensures consistent versioning across your project workspace. Keep up the excellent work, sensei!bin/sozo/Cargo.toml (1)
59-59: Ohayo, sensei! This change looks great!The update to use
tracing-log.workspace = truealigns perfectly with the project's dependency management strategy. This change brings several benefits:
- Consistency: It matches the style used for other dependencies in this file.
- Simplified version management: By using the workspace-level dependency, it's easier to keep all parts of the project using the same version of
tracing-log.- Reduced maintenance: When updating
tracing-log, you'll only need to change it in one place for the entire workspace.bin/sozo/src/args.rs (2)
8-9: Ohayo, sensei! New imports look good!The new imports for
tracing_logandtracing_subscriber::FmtSubscriberare well-aligned with the changes in the logging initialization. They provide the necessary components for the updated implementation.
Line range hint
1-94: Ohayo, sensei! Overall, these changes are a solid improvement!The modifications to the logging initialization process in this file are well-implemented and integrate smoothly with the existing code. The new imports and the updated
init_loggingmethod work together to provide a more robust and flexible logging configuration.These changes align well with the PR objectives of ensuring the
logcrate output is shown withtracing. The introduction oftracing-logand the use ofLogTracershould help achieve this goal.Cargo.toml (2)
203-203: Ohayo, sensei! Excellent update to thetracingdependency!The changes to the
tracingdependency are well-thought-out:
- Updating to version 0.1.38 brings in the latest minor improvements.
- Adding the "log" feature enables seamless integration with the
logcrate.- Disabling default features helps optimize the dependency tree.
These modifications align perfectly with the PR objective of ensuring
logcrate output is shown withtracing. Great job, sensei!
204-204: Ohayo again, sensei! Brilliant addition of thetracing-logdependency!Adding the
tracing-logcrate (version 0.1.3) is a masterstroke:
- It provides the missing link between the
tracingandlogecosystems.- This addition perfectly complements the changes made to the
tracingdependency.- It directly supports the PR objective of ensuring
logcrate output is shown withtracing.Your choice of version 0.1.3 is spot-on, as it's recent enough to be compatible with the updated
tracingcrate. Excellent work, sensei!bin/saya/src/args/mod.rs (1)
15-15: Ohayo, sensei! Nice addition of the LogTracer.The import of
tracing_log::LogTraceris a great move to bridge the gap between thelogandtracingcrates. This will ensure that logs emitted via thelogcrate are captured and processed bytracing, aligning perfectly with our PR objectives.bin/katana/src/cli/node.rs (2)
38-38: Ohayo, sensei! LGTM on this import!The addition of
use tracing_log::LogTracer;is well-placed and necessary for the upcomingLogTracerusage. It's grouped nicely with other tracing-related imports.
254-255: Ohayo again, sensei! ThisLogTracerinitialization is sugoi!The addition of
LogTracer::init()?;is a crucial step in enhancing our logging capabilities. Here's why it's awesome:
- It routes log messages through the
tracingframework, unifying our logging approach.- Its placement before the
fmt::Subscriberbuilder setup is perfect.- The use of the
?operator ensures we handle any initialization errors gracefully.The empty line after the initialization is a nice touch for readability. Domo arigato for this improvement!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2477 +/- ##
==========================================
- Coverage 68.45% 68.45% -0.01%
==========================================
Files 368 368
Lines 48139 48153 +14
==========================================
+ Hits 32955 32961 +6
- Misses 15184 15192 +8 ☔ View full report in Codecov by Sentry. |
2 participants
Summary by CodeRabbit