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

Libraries logging callbacks / json output #2288

Open
AntoinePrv opened this issue Feb 13, 2023 · 6 comments
Open

Libraries logging callbacks / json output #2288

AntoinePrv opened this issue Feb 13, 2023 · 6 comments

Comments

@AntoinePrv
Copy link
Member

For libmamba to be free of side effects (such as printing), it is best to provide dependency-free callback functions for logging purposes.
Users, including micromamba (or a verbose libmambacli), can then set these callbacks to forward the output to a log, a stdout/stderr...

Goals

Yes-goals

It would useful if the callback functions handle the following inputs:

  • Source file and line of call (this will be mostly useful for debugging/development)
  • Some levels (Info, Debug, Warning, Error)
  • Some namespace (e.g. libmamba, curl, libsolv) so that we can use these as callbacks in libmamba private dependencies as well.

No-goals

Re-writing a logging library is not a goal here. Everything that can be done by the user (eventually though a 3rd party) should be excluded, for example:

  • Any IO
  • Time and date (they can be computed in the user callback)
  • Dynamic behavior such as silencing through an environment variable

Maybe-goals

  • Thread safety: should it be the responsibility of the user callback to be thread safe, or should libmamba only call the user callbacks in a thread safe fashion? Customizable?
  • std::ostream (or similar way to provide a message by parts) support for avoiding allocating temporary strings
  • Lazy fmt formatting and concatenating on lib mamba side, for instance
    template<typename Args
    log_error_fmt(const char* fmt, Args const& args...){
      if(user_log_error){
          user_log_error(fmt::format(fmt, args));
      }
    }

Implementation

Source

In C++20, we'de have std::source_location. Otherwise we can use macros with __FILE__ and the like (no portable function name though).

Enums vs mulitple callbacks

Should users provide one callback per log level

log_info(std::string_view msg, /* etc */);
log_debug(std::string_view msg, /* etc */);
...

Or a single function with a log level

log(mamba::LogLevel lvl, std::string_view msg)

The former is mostly useful for if there is no way to pass the message by parts (e.g. std::ostream) to the user so that temporary allocations can be deactivated as needed.

@Klaim
Copy link
Member

Klaim commented Feb 15, 2023

Here is my current opinion summarized:

  • there should be one unique callback stored for all logging handling. No need to handle thread-safety on our side, just let the callback handle it;
  • the callback should take an object, a log record, and that's it - all the informations are in there;
  • informations coming from our library should be minimal, so message, "channel" and some kind of level and location seems adequate;
  • No formatting should happen in this system, if the callback wants to format they can, or not, that happens there.
  • a function setting up specifically á callback to setup spdlog (or whatever we use in micromamba) would be helpful for users (that use spdlog).

Some Notes:

No-Goal: Time and date (they can be computed in the user callback)

I agree, though this means that the callback must be called in-place as soon as the record is built - no other latency introduced.

In C++20, we'de have std::source_location

We could model a similar type (reuse the implementation from boost or other sources?) and replace it by std::source_location as soon as we have it available?

@AntoinePrv
Copy link
Member Author

No formatting should happen in this system, if the callback wants to format they can, or not, that happens there.

I meant formatting in with fmt::format for utility (not layout). I feel a lot of message might look like "Error got {} but input should be smaller than {}", val, max.

@Hind-M
Copy link
Member

Hind-M commented Mar 1, 2023

I am writing a comment here because it's related to logging, and we should consider this when thinking about the different possible solutions:
In micromamba python tests (maybe in mamba as well, I didn't check), we use json format to get the results of micromamba commands.

This leads to several issues:

  • Some important info may not be printed in the case of a requested json format.
  • The info that we want to print may be existing in different places in the code base (in order to include it in the json structure message for example), and so, any changes in one place wouldn't be automatically considered elsewhere, and one should be aware of all the involved places.
  • There are several ways the printing is done Console::stream(), std::cout, std::stringstream... This should be more consistent.
  • We use if...else to check whether we should print the info or not depending on the format, which is cumbersome...

@Klaim
Copy link
Member

Klaim commented Mar 2, 2023

Good points @Hind-M and I would also add that the Powerloader integration PR is also "fixing" the behavior of --json and other visibility/log-level behavior because the current code is inconsistant.
Basically, at the library level we will not need to handle any output- but at the executable cli level we have to handle:

  • json output, which needs to be exclusive when requested for tools to be able to output the results and also must not be considered as logging;
  • logging which is just logging;
  • output which is not logging but the program's output (as in always displayed to the user);

@JohanMabille
Copy link
Member

Additional requirement in #3415

@Hind-M Hind-M added the --json Problems with --json label Jan 27, 2025
@Hind-M Hind-M changed the title Libraries logging callbacks Libraries logging callbacks / json output Jan 27, 2025
@Hind-M
Copy link
Member

Hind-M commented Jan 27, 2025

This issue ended up tracking both json output and logging problems.
When starting to tackle this, we should probably think of having a separate issue for each problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants