-
Notifications
You must be signed in to change notification settings - Fork 25
refactor(logging): Cleaner logging setup #442
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
base: unstable
Are you sure you want to change the base?
Conversation
|
This pull request has merge conflicts. Could you please resolve them @dknopik? 🙏 |
# Conflicts: # anchor/logging/src/tracing_libp2p_discv5_layer.rs
| }; | ||
|
|
||
| let _guards = match enable_logging(file_logging_flags, &global_config) { | ||
| let _guards = match logging::enable_logging(file_logging_flags, &global_config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it not used?
|
This pull request has merge conflicts. Could you please resolve them @dknopik? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the logging setup by moving logging logic from the main application to the logging crate for better organization and consolidation.
- Consolidates logging setup code in the logging crate by moving
enable_loggingfunction from main.rs - Simplifies the module structure by defining items in the crate root instead of a separate logging module
- Improves guard management by keeping guards unified and separate from logging layers
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| anchor/src/main.rs | Removes logging setup code and imports, delegates to logging crate |
| anchor/logging/src/lib.rs | Adds consolidated logging setup functions and FileLoggingFlags definition |
| anchor/logging/src/logging.rs | File deleted - contents moved to lib.rs |
| anchor/logging/src/tracing_libp2p_discv5_layer.rs | Updates layer creation with compression support and better error handling |
| anchor/logging/Cargo.toml | Adds global_config dependency |
Comments suppressed due to low confidence (1)
anchor/logging/src/tracing_libp2p_discv5_layer.rs:93
- The parameter
confighas been renamed tologging_configin the function signature but the implementation still referencesconfig. This inconsistency could lead to confusion during maintenance.
.map_err(|e| format!("Failed to initialize discv5 rolling file appender: {e}"))?;
|
|
||
| pub fn init_file_logging( | ||
| logs_dir: &Path, | ||
| config: &FileLoggingFlags, |
Copilot
AI
Aug 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function signature change from config: FileLoggingFlags (by value) to config: &FileLoggingFlags (by reference) is a breaking change that could affect existing callers expecting to pass the struct by value.
| config: &FileLoggingFlags, | |
| config: FileLoggingFlags, |
| config: &FileLoggingFlags, | ||
| ) -> Result<Option<LoggingLayer>, String> { | ||
| if config.disabled_file_logging() { | ||
| return Ok(None); |
Copilot
AI
Aug 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type has changed from Option<LoggingLayer> to Result<Option<LoggingLayer>, String>, which is a breaking change. Existing callers that expect an Option will need to be updated to handle the Result.
| return Ok(None); | |
| ) -> Option<LoggingLayer> { | |
| if config.disabled_file_logging() { | |
| return None; |
|
Hi @dknopik, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time. |
|
rude |
# Conflicts: # anchor/logging/src/logging.rs # anchor/src/main.rs
Proposed Changes
loggingcrateAdditional Info
This was originally part of #347