[RFC] Introducing level_filter concept

[SpriteOvO]

Summary

This issue proposes a new concept level_filter to distinguish from the existing enum level_enum. It is inspired by my implementing spdlog-rs https://github.com/SpriteOvO/spdlog-rs/blob/f076b7d08fc0fe067389b27461c8bbade6144d3f/spdlog/src/level.rs#L185-L223, which is designed to hide the numerical relationships between levels.

Motivation

Currently, we only use enum level_enum to represent levels of records and levels of filters. It causes some confusion and inconveniences, for example:

• level_a < level_b and sink.set_level(level::error) is non-intuitive for newbies unfamiliar with this library. Who is smaller, info or err?

• write error and critical logs to stderr #345 is impossible in the current design unless users implement a custom sink.

With this new concept added, users can be flexible manipulate the levels of filters for complex conditions to implement something like the issue #345 mentioned above and write intuitive code in this way:

auto setup_logger()
{
    std::vector<spdlog::sink_ptr> sinks;

    auto stdout_sink = spdlog::sinks::stdout_sink_mt::instance();
    auto stderr_sink = spdlog::sinks::stderr_sink_mt::instance();

    stdout_sink->set_level(spdlog::level_filter::more_verbose_equal(spdlog::level::info));
    stderr_sink->set_level(spdlog::level_filter::more_severe(spdlog::level::info));

    sinks.push_back(stdout_sink);
    sinks.push_back(color_stderr_sink);

    auto logger = std::make_shared<spdlog::logger>("main", std::begin(sinks), std::end(sinks));

    return logger;
}

Guide-level explanation

Basically, the new concept is supposed to use like:

spdlog::level_filter variable = spdlog::level_filter::more_severe(spdlog::level::info);

// For backward compatibility, we do not rename the member function
sink_a.set_level(variable);
sink_b.set_level(spdlog::level_filter::all);
sink_c.set_level(spdlog::level_filter::equal(spdlog::level::warn));

// And also for backward compatibility, there will be an implicit constructor for `enum filter_enum`
sink_d.set_level(spdlog::level::info);
// It equals to
sink_e.set_level(spdlog::level_filter::more_verbose_equal(spdlog::level::info));

This proposal expects not to break something for most use cases. So that users can still use the old way to set levels. When should users use the new way?

1. They want a more readable and intuitive style.
2. They need to implement some filters with a little more complex condition, like write error and critical logs to stderr #345.

Reference-level explanation

There are 2 possible implementations I can imagine.

Pseudo-code:

class level_filter {
public:
    // ... other ctors

    // For backward-compatibility
    level_filter(level_enum level) : level_filter{more_severe_equal(level)} {}

    // Filter conditions
    static level_filter off()  {
        return level_filter{[](level_enum arg) { return false; }};
    }
    static level_filter equal(level_enum level) {
        return level_filter{[level](level_enum arg) { return arg == level; }};
    }
    static level_filter not_equal(level_enum level) {
        return level_filter{[level](level_enum arg) { return arg != level; }};
    }
    static level_filter more_severe(level_enum level) {
        return level_filter{[level](level_enum arg) { return arg > level; }};
    }
    static level_filter more_severe_equal(level_enum level) {
        return level_filter{[level](level_enum arg) { return arg >= level; }};
    }
    static level_filter more_verbose(level_enum level) {
        return level_filter{[level](level_enum arg) { return arg < level; }};
    }
    static level_filter more_verbose_equal(level_enum level) {
        return level_filter{[level](level_enum arg) { return arg <= level; }};
    }
    static level_filter all() {
        return level_filter{[](level_enum arg) { return true; }};
    }
    // Or furthermore, make `predicate_t` public, then
    static level_filter custom(predicate_t predicate) { // (Should we use `std::function` for lambda with captures here?)
        return level_filter{predicate};
    }

    // Returns `true` if the given level is enabled for this filter
    bool compare(level_enum level) const {
        return predicate_(level);
    }

private:
    using predicate_t = bool(*)(level_enum);

    level_filter(predicate_t predicate) : predicate_{predicate} {}

    predicate_t predicate_;
};

Or

class level_filter {
public:
    // ... other ctors

    // For backward-compatibility
    level_filter(level_enum level) : level_filter{condition::more_severe_equal, level} {}

    // Filter conditions
    static level_filter off()  {
        return level_filter{condition::off};
    }
    static level_filter equal(level_enum level) {
        return level_filter{condition::equal, level};
    }
    static level_filter not_equal(level_enum level) {
        return level_filter{condition::not_equal, level};
    }
    static level_filter more_severe(level_enum level) {
        return level_filter{condition::more_severe, level};
    }
    static level_filter more_severe_equal(level_enum level) {
        return level_filter{condition::more_severe_equal, level};
    }
    static level_filter more_verbose(level_enum level) {
        return level_filter{condition::more_verbose, level};
    }
    static level_filter more_verbose_equal(level_enum level) {
        return level_filter{condition::more_verbose_equal, level};
    }
    static level_filter all() {
        return level_filter{condition::all};
    }
    // Or furthermore
    using predicate_t = std::function<bool(level_enum)>;
    static level_filter custom(predicate_t predicate) {
        return level_filter{predicate};
    }

    // Returns `true` if the given level is enabled for this filter
    bool compare(level_enum level) const {
        switch (cond_) {
            case condition::off:
                return false;
            case condition::equal:
                return level == level_;
            case condition::not_equal:
                return level != level_;
            case condition::more_severe:
                return level > level_;
            case condition::more_severe_equal:
                return level >= level_;
            case condition::more_verbose:
                return level < level_;
            case condition::more_verbose_equal:
                return level <= level_;
            case condition::all:
                return true;
            case condition::custom:
                return predicate_(level);
            default:
                unreachable();
        }
    }

private:
    enum class condition : uint32_t {
        off,
        equal,
        not_equal,
        more_severe,
        more_severe_equal,
        more_verbose,
        more_verbose_equal,
        all,
        custom,
    }

    explicit level_filter(condition cond) : cond_{cond}, level_{level::off} {}
    explicit level_filter(condition cond, level_enum level) : cond_{cond}, level_{level} {}
    explicit level_filter(predicate_t predicate) : cond_{condition::custom}, level_{level::off}, predicate_{predicate} {}

    condition cond_;
    level_enum level_;
    predicate_t predicate_;
};

Then replace level_enum with level_filter where they have filter semantics. e.g.

void sink::set_level(level::level_enum log_level);     -> void sink::set_level(level_filter filter);
void logger::set_level(level::level_enum log_level);   -> void logger::set_level(level_filter filter);
void registry::set_level(level::level_enum log_level); -> void registry::set_level(level_filter filter);
...

Drawbacks

• Implementing this proposal contains breaking changes eventually (e.g. for sinks are implemented by users manually, since the type of parameter has changed), however, we could add it in v2.x.

• It also brings more overhead compared to the old way, but it is not expected to be significant. The more overhead of the first possible implementation may just be that the functions cannot be inline.

Future possibilities

If we decide to add it in v2.x, we could also consider if it's necessary to remove the old way completely (rename set_level to set_level_filter to avoid confusion, removing the backward-compatibility tricks and enumerator level_enum::off, etc.).

[tt4g]

The level_filter constructor for backward compatibility should be modified as follows:

     // For backward-compatibility
-     level_filter(level_enum level) {
-         return level_filter::more_severe_equal(level);
-     }
+     level_filter(level_enum level) : level_filter(condition::more_severe_equal, level) {}

[SpriteOvO]

@tt4g Good catch! Code updated.

[tt4g]

IMO.

If this feature is added, someone will want equivalent features in the following APIs:

• spdlog::logger::set_level()
• spdlog::logger::flush_on()

---

spdlog::sink::level() API returns the log level, so we need to be able to access the log level of level_filter.

spdlog/include/spdlog/sinks/sink.h

Line 22 in c65aa4e

level::level_enum level() const;

---

Remember that one of the reasons ABI breaks is that the type of spdlog::sink::level_ changes.

spdlog/include/spdlog/sinks/sink.h

Lines 25 to 28 in c65aa4e

	protected:
	// sink log level - default is all
	level_t level_{level::trace};
	};

[gabime]

Thanks for the RFC. Might be useful to avoid confusions regarding log levels, however it might be an overkill with too many options. Most users just want to set the log level and forget about it.

It also brings more overhead compared to the old way, but it is not expected to be significant. The more overhead of the first possible implementation may just be that the functions cannot be inline.

Right, checking log level is the absolute hottest path in spdlog, extra care should be taken.
Another approach would be to create a 2 dimensinal lookup table where each row is the logger's (or sink's) log level and each column the log level of acutall message.
The lookup table would be created when set_level() is called either on logger or sink.

For example a table of produced when the condition::equal() is requested:

+---------+----------+----------+----------+----------+----------+----------+
|         |  trace   |  debug   |  info    |  warn    |  err     | critical |
+---------+----------+----------+----------+----------+----------+----------+
|  trace  | *true*   | false    | false    | false    | false    | false    |
+---------+----------+----------+----------+----------+----------+----------+
|  debug  | false    | *true*   | false    | false    | false    | false    |
+---------+----------+----------+----------+----------+----------+----------+
|  info   | false    | false    | *true*   | false    | false    | false    |
+---------+----------+----------+----------+----------+----------+----------+
|  warn   | false    | false    | false    | *true*   | false    | false    |
+---------+----------+----------+----------+----------+----------+----------+
|  err    | false    | false    | false    | false    | *true*   | false    |
+---------+----------+----------+----------+----------+----------+----------+
| critical| false    | false    | false    | false    | false    | *true*   |
+---------+----------+----------+----------+----------+----------+----------+

and the should_log(level) would be:

bool should_log(level::level_enum msg_level) const
{
  auto my_level = level_.load(std::memory_order_relaxed);
  return lookup_table[my_level][msg_level];
}

Of course, there is a good chance it is not any faster or even slower than the switch (cond_) you offered. Any approach must be benched thoroughly.

[cppcooper]

Was there a pull request introducing these features? I can't find it, or the features mentioned.

[tt4g]

@cppcooper No PR has implemented this proposal.