Fix log-facade logging and clarify max level
#454
Conversation
tnull
force-pushed
the
2025-01-fix-env-logger
branch
from
869bfc7 to
90dba9e
Compare
There was a problem hiding this comment.
Thanks for catching this @tnull.
Part of the conversation we had when reviewing #407 was to add a follow-up that addressed testing the writers in ways that didn't clutter the existing tests. I will be adding those tests shortly but I have tested this with a log facade writer locally and it logs entries correctly.
.. for clarity
Previously, the log facade logger would always use a `logger` target but then *also* add the correct module path and line as part of the normal log messages. Here, we override the log target correctly. Moreover, we previously would log all messages at the configured *maximum* level, not the level in the log record.
tnull
force-pushed
the
2025-01-fix-env-logger
branch
from
90dba9e to
9f2714c
Compare
|
Rebased on |
| Some(LogWriterConfig::Log(log_level)) => Logger::new_log_facade(*log_level), | ||
| Some(LogWriterConfig::Log { max_log_level }) => { | ||
| let max_log_level = max_log_level.unwrap_or_else(|| DEFAULT_LOG_LEVEL); | ||
| Logger::new_log_facade(max_log_level) |
There was a problem hiding this comment.
One thing that I noticed about the log facade is that you can set log level GOSSIP, but that isn't visible anymore after this PR. The standard log only knows TRACE of course. Not a problem I think?
And perhaps some explanation on how the RUST_LOG env var interacts with this log level would be helpful too. It wasn't very clear to me how that works. Maybe there is a way to set RUST_LOG programmatically?
There was a problem hiding this comment.
One thing that I noticed about the log facade is that you can set log level GOSSIP, but that isn't visible anymore after this PR. The standard log only knows TRACE of course. Not a problem I think?
Yeah, I think it's not super important. We could consider adding something special in the target or message itself, but I'd wait for a user to complain about it. So far, I assume it's fine to squash TRACE and GOSSIP together in this case.
And perhaps some explanation on how the RUST_LOG env var interacts with this log level would be helpful too. It wasn't very clear to me how that works. Maybe there is a way to set RUST_LOG programmatically?
Ah, RUST_LOG is entirely out-of-scope here, as it's a special behavior of env_logger, which is just one of the backends for the log facade. env_logger users simply need to read their docs and discover that it's mandatory to set RUST_LOG (see https://docs.rs/env_logger/latest/env_logger/#enabling-logging).
This is another small follow-up to #407.
Previously, the log facade logger would always use a
loggertarget but then also add the correct module paht and line as part of the normal log messages. Here, we override the log target correctly.Moreover, we previously would log all messages at the configured maximum level, not the level in the log record. We therefore also fix this and rename
leveltomax_log_leveleverywhere to clear up any confusions regarding what the variable does.(cc @enigbe)