Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 91 additions & 27 deletions include/seastar/util/log.hh
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ class logger {
#endif

public:
/// Tag used to request that a log call bypass the configured level
/// gate. Passed as the first argument to any force-suffixed overload
/// or to the per-level helpers. A distinct type (not bool) avoids
/// ambiguity with the format_info-first overloads: a string literal
/// converts to format_info but not to force_tag.
Comment on lines +75 to +78
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

The doc comment for force_tag says it is "Passed as the first argument" to force overloads, but for logger::log(log_level, force_tag, ...) the tag is the second argument (after log_level). Consider rewording to avoid misleading API consumers (e.g., "passed immediately after the log level" for log(), and as the first arg for per-level helpers).

Suggested change
/// gate. Passed as the first argument to any force-suffixed overload
/// or to the per-level helpers. A distinct type (not bool) avoids
/// ambiguity with the format_info-first overloads: a string literal
/// converts to format_info but not to force_tag.
/// gate. Passed immediately after the log level to log() overloads,
/// and as the first argument to the per-level helpers. A distinct
/// type (not bool) avoids ambiguity with the format_info-first
/// overloads: a string literal converts to format_info but not to
/// force_tag.

Copilot uses AI. Check for mistakes.
struct force_tag {};
static constexpr force_tag force{};

class log_writer {
public:
virtual ~log_writer() = default;
Expand Down Expand Up @@ -251,20 +259,13 @@ public:
///
template <typename... Args>
void log(log_level level, format_info_t<Args...> fmt, Args&&... args) noexcept {
if (is_enabled(level)) {
try {
lambda_log_writer writer([&] (internal::log_buf::inserter_iterator it) {
#ifdef SEASTAR_LOGGER_COMPILE_TIME_FMT
return fmt::format_to(it, fmt.format, std::forward<Args>(args)...);
#else
return fmt::format_to(it, fmt::runtime(fmt.format), std::forward<Args>(args)...);
#endif
});
do_log(level, writer);
} catch (...) {
failed_to_log(std::current_exception(), fmt::string_view(fmt.format), fmt.loc);
}
}
do_log_checked(level, false, std::move(fmt), std::forward<Args>(args)...);
}

/// Force-emit variant: bypass the level gate. See \ref force_tag.
template <typename... Args>
void log(log_level level, force_tag, format_info_t<Args...> fmt, Args&&... args) noexcept {
do_log_checked(level, true, std::move(fmt), std::forward<Args>(args)...);
}

/// logs with a rate limit to desired level if enabled, otherwise we ignore the log line
Expand All @@ -283,19 +284,14 @@ public:
///
template <typename... Args>
void log(log_level level, rate_limit& rl, format_info_t<Args...> fmt, Args&&... args) noexcept {
if (is_enabled(level) && rl.check()) {
try {
lambda_log_writer writer([&] (internal::log_buf::inserter_iterator it) {
if (rl.has_dropped_messages()) {
it = fmt::format_to(it, "(rate limiting dropped {} similar messages) ", rl.get_and_reset_dropped_messages());
}
return fmt::format_to(it, fmt::runtime(fmt.format), std::forward<Args>(args)...);
});
do_log(level, writer);
} catch (...) {
failed_to_log(std::current_exception(), fmt::string_view(fmt.format), fmt.loc);
}
}
do_log_checked_rl(level, false, rl, std::move(fmt), std::forward<Args>(args)...);
}

/// Force-emit rate-limited variant: bypass the level gate (the rate
/// limit itself still applies).
template <typename... Args>
void log(log_level level, force_tag, rate_limit& rl, format_info_t<Args...> fmt, Args&&... args) noexcept {
do_log_checked_rl(level, true, rl, std::move(fmt), std::forward<Args>(args)...);
}

/// \cond internal
Expand Down Expand Up @@ -355,6 +351,11 @@ public:
void error(format_info_t<Args...> fmt, Args&&... args) noexcept {
log(log_level::error, std::move(fmt), std::forward<Args>(args)...);
}
/// Force-emit variant: bypass the level gate. See \ref force_tag.
template <typename... Args>
void error(force_tag, format_info_t<Args...> fmt, Args&&... args) noexcept {
log(log_level::error, force, std::move(fmt), std::forward<Args>(args)...);
}
/// Log with warning tag:
/// WARN %Y-%m-%d %T,%03d [shard 0] - "your msg" \n
///
Expand All @@ -366,6 +367,11 @@ public:
void warn(format_info_t<Args...> fmt, Args&&... args) noexcept {
log(log_level::warn, std::move(fmt), std::forward<Args>(args)...);
}
/// Force-emit variant: bypass the level gate. See \ref force_tag.
template <typename... Args>
void warn(force_tag, format_info_t<Args...> fmt, Args&&... args) noexcept {
log(log_level::warn, force, std::move(fmt), std::forward<Args>(args)...);
}
/// Log with info tag:
/// INFO %Y-%m-%d %T,%03d [shard 0] - "your msg" \n
///
Expand All @@ -377,6 +383,11 @@ public:
void info(format_info_t<Args...> fmt, Args&&... args) noexcept {
log(log_level::info, std::move(fmt), std::forward<Args>(args)...);
}
/// Force-emit variant: bypass the level gate. See \ref force_tag.
template <typename... Args>
void info(force_tag, format_info_t<Args...> fmt, Args&&... args) noexcept {
log(log_level::info, force, std::move(fmt), std::forward<Args>(args)...);
}
/// Log with info tag on shard zero only:
/// INFO %Y-%m-%d %T,%03d [shard 0] - "your msg" \n
///
Expand All @@ -390,6 +401,13 @@ public:
log(log_level::info, std::move(fmt), std::forward<Args>(args)...);
}
}
/// Force-emit variant: bypass the level gate. See \ref force_tag.
template <typename... Args>
void info0(force_tag, format_info_t<Args...> fmt, Args&&... args) noexcept {
if (is_shard_zero()) {
log(log_level::info, force, std::move(fmt), std::forward<Args>(args)...);
}
}
/// Log with debug tag:
/// DEBUG %Y-%m-%d %T,%03d [shard 0] - "your msg" \n
///
Expand All @@ -401,6 +419,11 @@ public:
void debug(format_info_t<Args...> fmt, Args&&... args) noexcept {
log(log_level::debug, std::move(fmt), std::forward<Args>(args)...);
}
/// Force-emit variant: bypass the level gate. See \ref force_tag.
template <typename... Args>
void debug(force_tag, format_info_t<Args...> fmt, Args&&... args) noexcept {
log(log_level::debug, force, std::move(fmt), std::forward<Args>(args)...);
}
/// Log with trace tag:
/// TRACE %Y-%m-%d %T,%03d [shard 0] - "your msg" \n
///
Expand All @@ -412,6 +435,11 @@ public:
void trace(format_info_t<Args...> fmt, Args&&... args) noexcept {
log(log_level::trace, std::move(fmt), std::forward<Args>(args)...);
}
/// Force-emit variant: bypass the level gate. See \ref force_tag.
template <typename... Args>
void trace(force_tag, format_info_t<Args...> fmt, Args&&... args) noexcept {
log(log_level::trace, force, std::move(fmt), std::forward<Args>(args)...);
}

/// \return name of the logger. Usually one logger per module
///
Expand Down Expand Up @@ -457,6 +485,42 @@ public:
///
/// \note this is a noop if fmtlib's version is less than 6.0
static void set_with_color(bool enabled) noexcept;

private:
template <typename... Args>
void do_log_checked(log_level level, bool bypass, format_info_t<Args...> fmt, Args&&... args) noexcept {
if (bypass || is_enabled(level)) {
try {
lambda_log_writer writer([&] (internal::log_buf::inserter_iterator it) {
#ifdef SEASTAR_LOGGER_COMPILE_TIME_FMT
return fmt::format_to(it, fmt.format, std::forward<Args>(args)...);
#else
return fmt::format_to(it, fmt::runtime(fmt.format), std::forward<Args>(args)...);
#endif
});
do_log(level, writer);
} catch (...) {
failed_to_log(std::current_exception(), fmt::string_view(fmt.format), fmt.loc);
}
}
}

template <typename... Args>
void do_log_checked_rl(log_level level, bool bypass, rate_limit& rl, format_info_t<Args...> fmt, Args&&... args) noexcept {
if ((bypass || is_enabled(level)) && rl.check()) {
try {
lambda_log_writer writer([&] (internal::log_buf::inserter_iterator it) {
if (rl.has_dropped_messages()) {
it = fmt::format_to(it, "(rate limiting dropped {} similar messages) ", rl.get_and_reset_dropped_messages());
}
return fmt::format_to(it, fmt::runtime(fmt.format), std::forward<Args>(args)...);
Comment on lines +515 to +516
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

do_log_checked_rl() always uses fmt::runtime(fmt.format) even when SEASTAR_LOGGER_COMPILE_TIME_FMT is enabled, while do_log_checked() preserves compile-time formatting via #ifdef. This is an inconsistency and likely a performance regression (and can reduce compile-time format checking). Consider adding the same compile-time branch here (fmt::format_to(it, fmt.format, ...) when enabled).

Suggested change
}
return fmt::format_to(it, fmt::runtime(fmt.format), std::forward<Args>(args)...);
}
#ifdef SEASTAR_LOGGER_COMPILE_TIME_FMT
return fmt::format_to(it, fmt.format, std::forward<Args>(args)...);
#else
return fmt::format_to(it, fmt::runtime(fmt.format), std::forward<Args>(args)...);
#endif

Copilot uses AI. Check for mistakes.
});
do_log(level, writer);
} catch (...) {
failed_to_log(std::current_exception(), fmt::string_view(fmt.format), fmt.loc);
}
}
}
};

/// \brief used to keep a static registry of loggers
Expand Down
3 changes: 3 additions & 0 deletions tests/unit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,9 @@ seastar_add_test (weak_ptr
seastar_add_test (log_buf
SOURCES log_buf_test.cc)

seastar_add_test (logger_force
SOURCES logger_force_test.cc)

seastar_add_test (exception_logging
KIND BOOST
SOURCES exception_logging_test.cc)
Expand Down
68 changes: 68 additions & 0 deletions tests/unit/logger_force_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* This file is open source software, licensed to you under the terms
* of the Apache License, Version 2.0 (the "License"). See the NOTICE file
* distributed with this work for additional information regarding copyright
* ownership. You may not use this file except in compliance with the License.
*
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

#include <seastar/testing/thread_test_case.hh>
#include <seastar/util/log.hh>

#include <sstream>

namespace {
seastar::logger test_log("test");
}

SEASTAR_THREAD_TEST_CASE(force_bypasses_level_gate_log_overload) {
std::ostringstream captured;
seastar::logger::set_ostream(captured);
seastar::logger::set_ostream_enabled(true);

Comment on lines +29 to +32
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

logger::set_ostream(captured) stores a raw pointer in a global static. Because captured is a local std::ostringstream, _out will dangle after the test case returns (and even between test cases in the same binary), which can cause UB if any logging happens after captured is destroyed. Reset the ostream to a long-lived stream (e.g., std::cerr) before returning, or use a long-lived/static capture stream for the whole test executable.

Copilot uses AI. Check for mistakes.
test_log.set_level(seastar::log_level::warn);

// Without force: dropped.
test_log.log(seastar::log_level::info, "dropped_line");
BOOST_REQUIRE(captured.str().find("dropped_line") == std::string::npos);

// With force: emitted.
test_log.log(seastar::log_level::info, seastar::logger::force, "forced_line");
BOOST_REQUIRE(captured.str().find("forced_line") != std::string::npos);
}

SEASTAR_THREAD_TEST_CASE(force_bypasses_level_gate_convenience_methods) {
std::ostringstream captured;
seastar::logger::set_ostream(captured);
seastar::logger::set_ostream_enabled(true);

Comment on lines +45 to +48
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

Same issue as above: set_ostream(captured) points the global logger output at a stack-allocated stream that will be destroyed when the test exits. Ensure the global ostream is reset to a valid long-lived stream (or make the capture stream long-lived) to avoid dangling pointer use.

Copilot uses AI. Check for mistakes.
test_log.set_level(seastar::log_level::warn);

test_log.info(seastar::logger::force, "info_forced");
test_log.debug(seastar::logger::force, "debug_forced");
test_log.trace(seastar::logger::force, "trace_forced");
test_log.warn(seastar::logger::force, "warn_forced");
test_log.error(seastar::logger::force, "error_forced");

const auto out = captured.str();
BOOST_REQUIRE(out.find("info_forced") != std::string::npos);
BOOST_REQUIRE(out.find("debug_forced") != std::string::npos);
BOOST_REQUIRE(out.find("trace_forced") != std::string::npos);
BOOST_REQUIRE(out.find("warn_forced") != std::string::npos);
BOOST_REQUIRE(out.find("error_forced") != std::string::npos);

// Without force, info/debug/trace dropped under warn level.
captured.str("");
test_log.info("info_dropped");
BOOST_REQUIRE(captured.str().find("info_dropped") == std::string::npos);
}
Loading