LogLevel off has the wrong enum value

[bartgol]

Since off=6, calling log (off, "msg") will always print the message, since off is greater than whatever the logger log level is. I changed logger::should_log impl as

return msg_level!=level::level_enum::off and msg_level >= level_.load(std::memory_order_relaxed);

and I get the expected behavior, namely that logger.log(level::off, "blah"); does not log.

I think the log_level "off" should have value 6 when it comes to the logger log level, but should have the value -1 when it comes to the message log level, since it must be lower than the log level of the logger (regardless of what the logger verbosity is).

Some context: I have a logger with log level "info" (say). There is certain category of messages that in some cases I want to disable completely, regardless of the logger log level. I suppose I could use 'trace' for these messages, but the level "off" seemed so appropriate. And yet, it does exactly the opposite, since it bumps the msg level to the max verbosity possible.

[tt4g]

I think the log level is designed to achieve log filtering by comparison, and the behavior of logger.log(level::off, "blah"); to output logs is just not taken into account.
Note that you seem to have a desire to call the logging function but never output logs, but conversely you also have a desire to force logs to be output regardless of the logging level (#2640).

Since importing the suggested changes will break the existing behavior (logging forced by off), @gabime needs to decide if the changes can be imported.

[bartgol]

I'm not sure I follow you. I do want my logger to output logs, but there's a category of messages that I want to turn off in certain cases. Something like

logger.trace("fine grain blah");
logger.info("more general blah");
logger.log(special_feature_log_level,"I don't want to see this sometimes, regardless of log level");

So the logger will log some stuff, like the first line should always log if I want logging at all. But sometimes I want to set special_feature_log_level to off, so that it doesn't appear in the logs, regardless of the logger level. I think there's no way to achieve that, as things stand.

I don't see how the change would break things. I'm only checking if the message log level is off before comparing it to the logger log level. I assume, given how things work, that nobody is calling logger.log(off,"blah") with the intent of always output the message. In fact, the fact that logger.log(off,"blah") currently always outputs the message (regardless of the logger log level), seems very counter-intuitive, and seeing that in a code would puzzle me (in fact, I was puzzled when messages were being outputed when I set special_feature_log_level to off).

[tt4g]

It means that off is designed to be passed to spdlog::set_level(spdlog::level::off);. It is not part of the design to pass the log level to logger::log()

If you just don't want to output logs under certain conditions, why not just not call the logging function?

if (<COND>) {
    logger.info("I don't want to see this sometimes, regardless of log level");
}

[bartgol]

I see what you mean. However, since off is part of the log level enum, and log accepts a log level enum, the logger should either error out if it's used, or handle it in a reasonable way. The fact that logger.log(off,"blah") prints with any logger log level seems quite counter intuitive.

I think the enum log level is then abused a bit, being used for both message and logger. I suppose conceptually there should be two enums then, one for the message level and one for the logger level. Then, off should be a valid value for the logger log level, but not for the message log level. Alternatively, off should not be a value at all, and users should do something like logger.turn_off() rather than logger.set_level(off). The should_log method should then do something like if (is_off or msg_lvl<level_) return.

Anyhow, I think what you suggest is probably the best solution I currently have to avoid printing that kind of messages.

[tt4g]

Yes. I understand the opinion that the use of the log level is confusing to users.
However, the log level API change would be a breaking change, so we would have to ask them to use a workaround for now.

[tt4g]

Another way to avoid logging when logging level is off is to create a custom sink: https://github.com/gabime/spdlog/wiki/4.-Sinks#implementing-your-own-sink
This sink can be implemented simply by adding a log-level filter to dist_sink.

Example:

#include "spdlog/sinks/base_sink.h"
#include "spdlog/sinks/dist_sink.h"
#include "spdlog/details/null_mutex.h"

#include <memory>
#include <mutex>
#include <vector>
#include <utility>

template<typename Mutex>
class filter_off_sink : public spdlog::sinks::dist_sink<Mutex>
{
public:
    filter_off_sink() = default;
    explicit filter_off_sink(std::vector<std::shared_ptr<sink>> sinks)
        : spdlog::sinks::dist_sink(std::move(sinks)) {}

protected:
    void sink_it_(const spdlog::details::log_msg& msg) override
    {
        if (msg.level == spdlog::level::off) {
            return;
        }
        spdlog::sinks::dist_sink<Mutex>::sink_it_(msg);
    }
};

using filter_off_sink_mt = filter_off_sink<std::mutex>;
using filter_off_sink_st = filter_off_sink<spdlog::details::null_mutex>;