Skip to content

Extract logger setup into it's own module and implement ShortTargetFormatter #902

Open
luisschwab wants to merge 3 commits intogetfloresta:masterfrom
luisschwab:chore/extract-logger-into-a-module
Open

Extract logger setup into it's own module and implement ShortTargetFormatter #902
luisschwab wants to merge 3 commits intogetfloresta:masterfrom
luisschwab:chore/extract-logger-into-a-module

Conversation

@luisschwab
Copy link
Copy Markdown
Member

@luisschwab luisschwab commented Mar 18, 2026

Closes #561

This PR extracts the logger setup function into it's own module, does some minor logging-related cleanup on main.rs, and implements a custom log formatter that replaces the full module path with a custom alias when the log level is Level::INFO or higher.

Logs on the default log level go from

2026-04-07 16:24:38  INFO floresta_wire::p2p_wire::node::chain_selector_ctx: Downloading headers from peer=9 at height=1 hash=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048

to

2026-04-07 16:22:30  INFO wire: Downloading headers from peer=10 at height=1 hash=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048

Changelog

- Move the logger setup function into it's own module
- Rename it from `init_logging` to `start_logger`
- Minor logging-related cleanup on `main.rs`
- Implement a `ShortTargetFormatter` for formatting module paths
- Log milliseconds when level `>= Level::DEBUG`

@luisschwab luisschwab self-assigned this Mar 18, 2026
@luisschwab luisschwab added the code quality Generally improves code readability and maintainability label Mar 18, 2026
@luisschwab luisschwab added this to the Q2/2026 milestone Mar 18, 2026
@luisschwab luisschwab requested a review from csgui March 20, 2026 18:37
@luisschwab
Copy link
Copy Markdown
Member Author

luisschwab commented Mar 31, 2026

I'll update this so we only log a short module identifier when RUST_LOG >= debug.

- Move the logger setup function into it's own module.
- Rename it from `init_logging` to `start_logger`
@luisschwab luisschwab force-pushed the chore/extract-logger-into-a-module branch from 72cebb1 to eed6c95 Compare April 4, 2026 00:21
@luisschwab luisschwab changed the title Extract logger setup into it's own module Extract logger setup into it's own module and implement ShortTargetFormatter Apr 6, 2026
@luisschwab luisschwab force-pushed the chore/extract-logger-into-a-module branch from eed6c95 to a340696 Compare April 6, 2026 16:34
@luisschwab
Copy link
Copy Markdown
Member Author

luisschwab commented Apr 6, 2026

Pushed a340696 addressing @Davidson-Souza's reviews.

Also moves the exit on error on logger init from main.rs to logger.rs.

@luisschwab luisschwab force-pushed the chore/extract-logger-into-a-module branch from a340696 to 8a45586 Compare April 6, 2026 16:45
Copy link
Copy Markdown

@Vedd-Patel Vedd-Patel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also test for ShortTargetFormatter is not there
should i add a unit test for short_target mapping and formatter-level behavior under different verbosity\filter setups?

let file_path = format!("{}/{}", data_directory, LOG_FILE);

// Validate the log file path (`<data_directory>/<LOG_FILE>`).
let _ = fs::OpenOptions::new()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start_logger exits the process inside .map_err(...) instead of returning io::Error despite exposing a Result API
this mixes concerns and makes the signature misleading
not getting if its correct or not as iam not getting the correct way to handle this pls let me know about this one!

});
}
// The guard must stay alive until the end of `main` to flush file logs when dropped.
let _logger_guard = start_logger(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start_logger(...) returns Result<Option<WorkerGuard>, io::Error> but the result is not handled
assigned to _logger_guard and dropped
any future recoverable errors would be silently ignored!
handling the results of thes explicitly will help ig
something like -

  • let _logger_guard = start_logger(...).unwrap_or_else(|e| { eprintln!(...); exit(1); });
  • keep _logger_guard as Option<WorkerGuard>

@github-project-automation github-project-automation bot moved this to Backlog in Floresta Apr 7, 2026
@Davidson-Souza Davidson-Souza moved this from Backlog to Needs review in Floresta Apr 7, 2026
Copy link
Copy Markdown
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last nit

The `ShortTargetFormatter` will match and replace the full
module paths with short prefixes, when the log level is
`Level::INFO` or higher. Otherwise, the full module path is kept.
@luisschwab luisschwab force-pushed the chore/extract-logger-into-a-module branch from 8a45586 to 8d292ab Compare April 7, 2026 16:15
@luisschwab
Copy link
Copy Markdown
Member Author

@Davidson-Souza fixed!

Copy link
Copy Markdown
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8d292ab

@jaoleal
Copy link
Copy Markdown
Member

jaoleal commented Apr 7, 2026

ACK 8d292ab

@luisschwab
Copy link
Copy Markdown
Member Author

We should also log milliseconds when in debug

@luisschwab luisschwab requested a review from jaoleal April 8, 2026 14:28
@luisschwab luisschwab force-pushed the chore/extract-logger-into-a-module branch from a499b4a to 78692d3 Compare April 8, 2026 14:45
@Davidson-Souza
Copy link
Copy Markdown
Member

Since you're touching logs, what about making log to file default on? This is the standard between implementations afaik

@luisschwab
Copy link
Copy Markdown
Member Author

luisschwab commented Apr 8, 2026

Since you're touching logs, what about making log to file default on? This is the standard between implementations afaik

Agreed, but this can be a follow up. We should also allow specifying a log path, like Bitcoin Core:

-debuglogfile=<file>
    Specify location of debug log file (default: debug.log). Relative paths
    will be prefixed by a net-specific datadir location. Pass
    -nodebuglogfile to disable writing the log to a file.

Opened #947 for this

@luisschwab
Copy link
Copy Markdown
Member Author

@Davidson-Souza can you retrigger the no-std job? It's stuck

Copy link
Copy Markdown
Member

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 78692d3

@luisschwab luisschwab requested review from JoseSK999 and jaoleal and removed request for csgui, jaoleal and moisesPompilio April 10, 2026 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality Generally improves code readability and maintainability

Projects

Status: Needs review

Development

Successfully merging this pull request may close these issues.

[Enhancement] Logs: Only log the module name

4 participants