-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Port logging to log
#3042
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: next
Are you sure you want to change the base?
Port logging to log
#3042
Conversation
318a6fc to
9f8e045
Compare
Keats
left a comment
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.
I think this is a bit too much. A lot of it that we always want to show should stay as println. I think logs is going to be used mostly for perf/what's happening at the info level and warnings like in your PR. I don't have a clear view of what it would look like yet but I'm fairly sure everything we're printing during a normal run (such as link checking, timing etc) should stay println
| }); | ||
|
|
||
| println!( | ||
| log::info!( |
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.
i think all that functions should stay println? it's stuff we always want the end user to see, logs should be reserved to things like debugging or warnings
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.
As long as the logging framework is configured correctly, it’s very easy to always show these messages. Possibly using a special target for log.
| let local_addr = listener.local_addr().unwrap(); | ||
|
|
||
| println!( | ||
| log::info!( |
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.
anything in src should stay println/eprintln
|
I take issue with the current printing approach because it is sometimes hard to guess which subsystem is doing what unless the messages are very clear, and whether certain messages are even related. mdbook, which has a very similar CLI to zola, also uses |
I don't want the classic |
Me neither!
I’m fairly certain you can still inject ANSI codes into the log message, AFAIK there’s quite a bit of support around that (color macros, detecting terminal color support, etc.). I’m not sure where we do that right now though, didn’t see it when going through the print statements. If I missed something please point it out and I’ll definitely add it back.
Okay, so I will add an |
Cargo.toml
Outdated
| # For essence_str() function, see https://github.com/getzola/zola/issues/1845 | ||
| mime = "0.3.16" | ||
| env_logger = "0.11" | ||
| anstyle = "1" |
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.
we are already using termcolor in components/console
It's fine to replace it but let's not have both
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.
Yes we can remove the anstyle dependency, at least when disabling the color feature of env_logger. (That might require us to detect terminals manually tho, I will see.)
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.
After looking into this more, we should use anstyle instead of termcolor. It also has a utility crate (anstream) which provides auto-color-detection, pretty much matching the manual logic in console but more accurate (and compatible with old Windows terminals). So I now also ported console to anstyle which was simple enough.
|
I'll have a look at the output tomorrow |
|
Looking at the output:
|
7f9c5f7 to
7555bbd
Compare
I noticed this too earlier when testing, so now we fall back to
I personally disagree, but will do |
d781f82 to
433e75f
Compare
This change ports all println and console::* users to use log::* with the appropriate level. Caveats: - Some commands have slightly changed output (e.g. removed “warning” text for log::warn), but a lot of them might still contain formatting symbols like “->” and “>”. - Init command (and its tooling) is unchanged, as it should be a nice interactive command. - There is no `log::success`, so the success-info distinction has been lost in some places. - Separate prints close to each other have been transformed into one log call each. They might look less related than they did before. - No new logging is added. Many components might benefit from extra logging within them.
As per discussion, this only prints the log level with basic coloring as well as the message. Additionally, replace termcolor by anstyle+anstream which provides accurate terminal detection and more formatting capabilities. (anstyle is already a dependency, and termcolor is mainly present in some older package versions.)
433e75f to
083ded1
Compare
|
Sorry can you rebase? The giallo branch merge created a bunch of conflicts |
|
This doesn't make sense, if you're using Axum and Tokio why aren't you tracing for logging? |
|
We wouldn't use anything from tracing other than the logs so it wouldn't make sense to use it |
|
Tracing would allow you to run debugging information on a span covering thinks like shortcode expansion or calls to cmark, and get comprehensive reporting -- when desired -- about what took time in generating the site. It would also allow you to get comprehensive timings for requests and it easily flows with axum/tokio (it's the support mechanism of doing this). Not to mention, I think it's probably already in the req stack somewhere by way of If you want a patch to show it off and see, accept my axum patch and I'll provide an alternative for you to A-B. |
|
We dont care about the dev server at all for logging. I don't care
particularly about spans or instrumenting or multiple sinks either, zola is
a basic CLI we can easily handle it with basic logging.
…On Wed, 24 Dec 2025, 21:13 Evan Carroll, ***@***.***> wrote:
*EvanCarroll* left a comment (getzola/zola#3042)
<#3042 (comment)>
Tracing would allow you to run debugging information on a span covering
thinks like shortcode expansion or calls to cmark, and get comprehensive
reporting -- when desired -- about what took time in generating the site.
It would also allow you to get comprehensive timings for requests and it
easily flows with axum/tokio (it's the support mechanism of doing this).
Not to mention, I think it's probably already in the req stack somewhere by
way of hyper-util and axum. So it's basically more power and free.
If you want a patch to show it off and see, accept my axum patch and I'll
provide an alternative for you to A-B.
—
Reply to this email directly, view it on GitHub
<#3042 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFGDI4IQLCQ3BVPC6XVDUL4DLXV3AVCNFSM6AAAAACOZMQKWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMOJQGQ2TCOJVGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I'm not trying to be standoffish and it's not just about the dev server either (though I don't know why you wouldn't want the things you don't care about).
It's literally switching |
|
I'll well aware of tracing, but since we won't use any of it right now, there's no point adding it. As you said, it's easy to swap later if we want to |
As per discussion in #3038 (comment), we want to move to using log for all of our logging needs. This PR does both the backend work, and also adds an env_logger configured with:
zola=info, so that only Zola’s output is visible by default (and no debugging)<Level> <Message>matchingmdBook, except forINFOwhich only prints the messageAlso migrate to
anstylewhich is more flexible thantermcolorand also provides a utility for color mode auto-detection.Caveats:
removed “warning” text for log::warn), but a lot
of them might still contain formatting symbols
like “->” and “>”.
it should be a nice interactive command.
log::success, so the success-infodistinction has been lost in some places.
transformed into one log call each. They might
look less related than they did before.
benefit from extra logging within them.
Sanity check:
Code changes
(Delete or ignore this section for documentation changes)
nextbranch?If the change is a new feature or adding to/changing an existing one: