This PR adds a new class Color to logging.h which is used to wrap strings easily in ANSI color codes.
I think this makes the logs much easier to read.
I tested this on NixOS with the terminal Kitty and $TERM set to xterm-kitty.
I don’t have a Windows lying around so I have not tested on Windows.
can also be disabled, like before
445 }
446
447 if (m_log_threadnames && m_started_new_line) {
448 const auto& threadname = util::ThreadGetInternalName();
449- str_prefixed.insert(0, "[" + (threadname.empty() ? "unknown" : threadname) + "] ");
450+ static auto c{m_colorman({Color::Attribute::FgBlack, Color::Attribute::BgBlue})};
At the very least, we shouldn’t do any background coloring. This can easily degrade visibility of the information depending on what theme the user has set. This change shouldn’t make any information harder to see.
| master | pr |
|---|---|
Thanks for the feedback, I had not thought of this. I’ve just pushed a change, the thread name is now red without background.
I’m open to suggestions for the colors.
Also, if there are issues with the colors, the user can disable them with the env variable shown in the PR description. Maybe adding a bitcoind parameter may be more intuitive for the user though.
I like colors in my terminals, and in general i would be Concept ACK on this change.
However, the glaring issue here is compatibility with users terminals and their color schemes. Right now, without colorizing, we assure that that the information is easy to read with human eyes. This change can cause problems for a lot of themes, and make the information harder to read. It would suck if core forces you to change your theme because you can no longer see some information. At the very least, not setting any background coloring should be an improvement over the current state of the PR.
Adds a new class to the logging system, Color, which allows for
easy colorization of strings. Also adds a new class, ColorMan, to
the logger which is used to manage the colorization.
155+ class ColorMan
156+ {
157+ public:
158+ ColorMan()
159+ {
160+ m_enable_colors = getenv("TERM") != nullptr && getenv("TERM") != std::string("dumb") && (getenv("ANSI_COLORS_DISABLED") == nullptr || getenv("ANSI_COLORS_DISABLED") == std::string("0"));
isatty() check for stdout should be added. Or changed that colouring is always disabled by default and can be enabled manually.
Concept NACK - this seems out of scope for Bitcoin Core to maintain, and is never going to work perfectly everywhere, for everyone, leading to endless bikeshedding / “improvements”.
The Bitcoin Core logging code almost warrants being it’s own externally maintained library at this point, and I don’t think we should be adding to it/expanding scope by introducing more nice-to-have functionality.
./src/bitcoind -printtoconsole=1 | ./color.py
What about an external script that colors the logs? Something like
./src/bitcoind -printtoconsole=1 | ./color.py
Agree this would be better approach.
What about an external script that colors the logs? Something like ./src/bitcoind -printtoconsole=1 | ./color.py
IMO that might be more difficult to achieve the same result as this PR because this require parsing the logs and identifying each part of the string (date, thread name, file, function, etc.), also they can be enabled/disabled with -logthreadnames -logsourcelocations which imo doesn’t help either.
How about an opt-in bitcoind parameter instead? Another solution would be to allow users to define their own colors if the default ones don’t work. But this seems like too much work for what it is.
The Bitcoin Core logging code almost warrants being it’s own externally maintained library at this point, and I don’t think we should be adding to it/expanding scope by introducing more nice-to-have functionality.
In this case, should we discuss a logging library to use?
If colors are not welcome, it would be nice to have at least indented logs to make logs more readable, such as:
02022-09-09T09:24:18Z [init] [init/common.cpp:149] [LogPackageVersion] Bitcoin Core version v23.99.0-19585eeb778a-dirty
12022-09-09T09:24:18Z [init] [kernel/context.cpp:21] [Context] Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
22022-09-09T09:24:18Z [init] [random.cpp:100] [ReportHardwareRand] Using RdSeed as an additional entropy source
32022-09-09T09:24:18Z [init] [random.cpp:103] [ReportHardwareRand] Using RdRand as an additional entropy source
42022-09-09T09:24:18Z [init] [init/common.cpp:120] [StartLogging] Default data directory /home/aureleoules/.bitcoin
52022-09-09T09:24:18Z [init] [init/common.cpp:121] [StartLogging] Using data directory /home/aureleoules/.bitcoin/testnet3
instead of
02022-09-09T09:24:18Z [init] [init/common.cpp:149] [LogPackageVersion] Bitcoin Core version v23.99.0-19585eeb778a-dirty (release build)
12022-09-09T09:24:18Z [init] [kernel/context.cpp:21] [Context] Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
22022-09-09T09:24:18Z [init] [random.cpp:100] [ReportHardwareRand] Using RdSeed as an additional entropy source
32022-09-09T09:24:18Z [init] [random.cpp:103] [ReportHardwareRand] Using RdRand as an additional entropy source
42022-09-09T09:24:18Z [init] [init/common.cpp:120] [StartLogging] Default data directory /home/aureleoules/.bitcoin
52022-09-09T09:24:18Z [init] [init/common.cpp:121] [StartLogging] Using data directory /home/aureleoules/.bitcoin/testnet3
Is it important that each [...] type is coloured the same? You could just define 5 colors and then color each [...] in order, regardless of their type.
Padding doesn’t work because you don’t know the pad size.
I wish there was a function to get the color palette from the system (if there is one), so that there would be no need to bikeshed colors, as they are picked by the system user.
In this case, should we discuss a logging library to use?
I’m definitely not suggesting we take on a new (external) dependency for logging. Adding colours to logs is not a problem we need to solve. Especially not when there is already an internal migration happening within our logging code, and finishing that would be a much better allocation of engineering time/resources, as opposed to bikeshedding log colouring / adding options to bitcoind to let people colour logs / starting a migration towards another dependency.
If colors are not welcome…
Rather maintenance costs they come with.
68@@ -68,6 +69,121 @@ namespace BCLog {
69 BLOCKSTORE = (1 << 26),
70 ALL = ~(uint32_t)0,
71 };
72+
73+
74+ constexpr auto escape_sequence{"\033["};
75+
76+ class Color
logging.h is widely included, so it may be best to move as much new code into the logging.cpp implementation file instead.
Also, there are refactorings to come in #25203 to de-duplicate and maybe speed up some of the logging.{h,cpp} code, along with benchmarks for it. Could be a good idea to test this rebased on that pull once it is updated and see how the new code performs in the benchmarks. (We are past feature freeze for v24, so this change would target v25 at the earliest.)
Tested on macOS 12.3 (ARM M1 Max). The colorization seemed noisy to me at first, but I would probably end up using it. No strong opinion whether this should be done, or if it should be on or off by default.
It might be useful to refer to the existing use of colorization in src/bitcoin-cli.cpp for -getinfo (and the -color={always, auto, never} config option) and how it was handled. There hasn’t been bikeshedding or maintainance since it was added IIRC; that experience may or may not transfer to logging.
Is it important that each […] type is coloured the same? You could just define 5 colors and then color each […] in order, regardless of their type.
I’ll try to draft a script.
Adding colours to logs is not a problem we need to solve
Sure, but it’s a means to make the logs more readable.
If colors are not welcome…
Rather maintenance costs they come with.
Yes I understand, I meant this as well. (didn’t mean it in an aggressive way)