Skip to content
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

Use fmt for logging and allow to log to system daemon directly #606

Closed
wants to merge 9 commits into from

Conversation

imwints
Copy link
Contributor

@imwints imwints commented Aug 29, 2023

This PR improves the logger in a few ways:

  1. The logger now lives in it's own file, which avoids unnecessary recompilation
  2. The logger uses fmt, See [REQUEST] Continued implementation of fmtlib #535
    • The logging functions now have the same syntax as fmt::format
    • The logging backend logs with fmt::print instead of std::ofstream
  3. FMT_HEADER_ONLY is now defined by Make and passed to the compiler. Other files including <fmt/*.h> are now independent of the definition in btop_tools.hpp
  4. By default, if <syslog.h> is present, btop will log it's messages there
  5. When compiled with make ENABLE_JOURNALD=true btop can directly log to journald. This can log interesting metadata like the source file location for each message
  6. Parse BTOP_LOG_LEVEL environment variable and set the log level accordingly

On systemd systems, with either syslog or journald logging, btops logs can be read with journalctl -t btop

If this is not a good default, it's an easy fix to put this behind a ENABLE_... flag!
I'm curious if this is a nice feature or not. Looking forward for feedback :)

@imwints
Copy link
Contributor Author

imwints commented Aug 29, 2023

Apples shell command within make has some kind of issue with line 168, but I'm not sure why

@imwints
Copy link
Contributor Author

imwints commented Sep 10, 2023

Fixed the shell issue. You need to escape the # in Apples make, which on the other hand doesn't work on Linux... # omitted :)

Copy link
Owner

@aristocratos aristocratos left a comment

Choose a reason for hiding this comment

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

@nobounce
Nice work!
I like the idea, but would prefer the defaults to keep the code contained (with only simple logfile logging).
But having the option to easily enable syslog or journald options would be great.
Don't forget to add documentation in the compile sections of the README.md

@imwints
Copy link
Contributor Author

imwints commented Sep 13, 2023

@aristocratos
I'm on it

@imwints imwints marked this pull request as draft September 15, 2023 12:44
@imwints imwints marked this pull request as ready for review September 18, 2023 18:23
@imwints
Copy link
Contributor Author

imwints commented Sep 18, 2023

@aristocratos
Rebased, added the same logic to CMakeLists.txt, changed default back to plain log file and added documentation

@imwints
Copy link
Contributor Author

imwints commented Sep 19, 2023

@aristocratos
In theory this is good to go

Avoids unnecessary recompilation
See: aristocratos#535

The logger now takes a formatting string and a number of variable
arguments according to the `fmt` spec
Moving the definition of the FMT_HEADER_ONLY macro from a file to the
compiler makes all code relying on this macro independent of a single
file

Also cleanup some unused fmt includes
If the BTOP_LOG_LEVEL is found in the environment it is parsed and
overwrites the current configured log level. The --debug switch still
has precedence

The value of BTOP_LOG_LEVEL must match one of `Logger::log_levels`
fields, being case insensitive
This commit allows for btop to log to either syslog(3) or systemd's
journal
Using std::call_once instead of boolean

Define some log file specific variables only if syslog or journald are
not used

Rename the privilege RAII wrapper and delete all operators and
constructors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants