From f1460de5d398cc4360b3c3aa6a90de191dfd704e Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Mon, 27 Apr 2026 15:30:53 +0100 Subject: [PATCH] feat: Optional log rotation (#3016) This PR adds an option to disable log rotation. --- benchmarks/util/log/LoggerBenchmark.cpp | 17 ++- docs/config-description.md | 8 ++ src/util/config/ConfigDefinition.cpp | 2 + src/util/config/ConfigDescription.hpp | 4 + src/util/log/Logger.cpp | 28 +++- src/util/log/Logger.hpp | 11 +- tests/unit/util/log/LogServiceInitTests.cpp | 20 +++ tests/unit/util/log/LoggerTests.cpp | 151 ++++++++++++++++++++ 8 files changed, 224 insertions(+), 17 deletions(-) diff --git a/benchmarks/util/log/LoggerBenchmark.cpp b/benchmarks/util/log/LoggerBenchmark.cpp index 90749092f..e175bf7ce 100644 --- a/benchmarks/util/log/LoggerBenchmark.cpp +++ b/benchmarks/util/log/LoggerBenchmark.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -24,9 +25,15 @@ static constexpr auto kLOG_FORMAT = "%Y-%m-%d %H:%M:%S.%f %^%3!l:%n%$ - %v"; struct BenchmarkLoggingInitializer { [[nodiscard]] static std::shared_ptr - createFileSink(LogService::FileLoggingParams const& params) + createFileSink(std::string const& logDir, uint32_t sizeMB, uint32_t maxFiles) { - return LogService::createFileSink(params, kLOG_FORMAT); + return LogService::createFileSink( + LogService::FileLoggingParams{ + .logDir = logDir, + .rotation = LogService::RotationParams{.sizeMB = sizeMB, .maxFiles = maxFiles}, + }, + kLOG_FORMAT + ); } static Logger @@ -68,11 +75,7 @@ benchmarkConcurrentFileLogging(benchmark::State& state) static constexpr size_t kTHREAD_COUNT = 1; spdlog::init_thread_pool(kQUEUE_SIZE, kTHREAD_COUNT); - auto fileSink = BenchmarkLoggingInitializer::createFileSink({ - .logDir = logDir, - .rotationSizeMB = 5, - .dirMaxFiles = 25, - }); + auto fileSink = BenchmarkLoggingInitializer::createFileSink(logDir, 5, 25); std::vector threads; threads.reserve(numThreads); diff --git a/docs/config-description.md b/docs/config-description.md index 6a5a15f88..be20dbdd6 100644 --- a/docs/config-description.md +++ b/docs/config-description.md @@ -561,6 +561,14 @@ Documentation can be found at: #include #include +#include #include #include #include @@ -184,12 +185,18 @@ spdlog::sink_ptr LogService::createFileSink(FileLoggingParams const& params, std::string const& format) { std::filesystem::path const dirPath(params.logDir); - // the below are taken from user in MB, but spdlog needs it to be in bytes - auto const rotationSize = mbToBytes(params.rotationSizeMB); + auto fileSink = [&]() -> std::shared_ptr { + auto const logPath = (dirPath / "clio.log").string(); + if (params.rotation.has_value()) { + // rotation sizes are taken from user in MB, but spdlog needs bytes + auto const rotationSize = mbToBytes(params.rotation->sizeMB); + return std::make_shared( + logPath, rotationSize, params.rotation->maxFiles + ); + } + return std::make_shared(logPath, /*truncate=*/false); + }(); - auto fileSink = std::make_shared( - (dirPath / "clio.log").string(), rotationSize, params.dirMaxFiles - ); fileSink->set_level(spdlog::level::trace); fileSink->set_formatter(std::make_unique(format)); @@ -336,10 +343,17 @@ LogService::getSinks(config::ClioConfigDefinition const& config) } } + std::optional rotation = std::nullopt; + if (config.get("log.rotate")) { + rotation = RotationParams{ + .sizeMB = config.get("log.rotation_size"), + .maxFiles = config.get("log.directory_max_files"), + }; + } + FileLoggingParams const params{ .logDir = logDir.value(), - .rotationSizeMB = config.get("log.rotation_size"), - .dirMaxFiles = config.get("log.directory_max_files"), + .rotation = rotation, }; allSinks.push_back(createFileSink(params, format)); } diff --git a/src/util/log/Logger.hpp b/src/util/log/Logger.hpp index 7cb177e7d..4dbe04715 100644 --- a/src/util/log/Logger.hpp +++ b/src/util/log/Logger.hpp @@ -28,6 +28,7 @@ class sink; // NOLINT(readability-identifier-naming) struct BenchmarkLoggingInitializer; class LoggerFixture; struct LogServiceInitTests; +struct LogFileRotationTests; namespace util { @@ -229,6 +230,7 @@ class LogServiceState { protected: friend struct ::LogServiceInitTests; friend class ::LoggerFixture; + friend struct ::LogFileRotationTests; friend class Logger; friend class ::util::impl::OnAssert; @@ -388,11 +390,14 @@ private: expected>, std::string> getSinks(config::ClioConfigDefinition const& config); + struct RotationParams { + uint32_t sizeMB; + uint32_t maxFiles; + }; + struct FileLoggingParams { std::string logDir; - - uint32_t rotationSizeMB; - uint32_t dirMaxFiles; + std::optional rotation; ///< nullopt when rotation is disabled }; friend struct ::BenchmarkLoggingInitializer; diff --git a/tests/unit/util/log/LogServiceInitTests.cpp b/tests/unit/util/log/LogServiceInitTests.cpp index 23af73e82..ac3c0d55a 100644 --- a/tests/unit/util/log/LogServiceInitTests.cpp +++ b/tests/unit/util/log/LogServiceInitTests.cpp @@ -66,6 +66,8 @@ protected: {"log.directory_max_files", ConfigValue{ConfigType::Integer}.defaultValue(25).withConstraint(config::gValidateUint32)}, + {"log.rotate", ConfigValue{ConfigType::Boolean}.defaultValue(true)}, + {"log.tag_style", ConfigValue{ConfigType::String}.defaultValue("none")}, }; @@ -219,3 +221,21 @@ TEST_F(LogServiceInitTests, LogSizeAndHourRotationCannotBeZero) ); } } + +TEST_F(LogServiceInitTests, RotateDefaultsToTrue) +{ + auto const parsingErrors = config_.parse(ConfigFileJson{boost::json::object{}}); + ASSERT_FALSE(parsingErrors.has_value()); + + EXPECT_TRUE(config_.get("log.rotate")); +} + +TEST_F(LogServiceInitTests, RotationDisabledConfigParsesSuccessfully) +{ + auto const parsingErrors = config_.parse( + ConfigFileJson{boost::json::object{{"log", boost::json::object{{"rotate", false}}}}} + ); + ASSERT_FALSE(parsingErrors.has_value()); + + EXPECT_FALSE(config_.get("log.rotate")); +} diff --git a/tests/unit/util/log/LoggerTests.cpp b/tests/unit/util/log/LoggerTests.cpp index 658fc1d82..8f523f4a7 100644 --- a/tests/unit/util/log/LoggerTests.cpp +++ b/tests/unit/util/log/LoggerTests.cpp @@ -1,14 +1,30 @@ #include "util/LoggerFixtures.hpp" +#include "util/config/Array.hpp" +#include "util/config/ConfigConstraints.hpp" +#include "util/config/ConfigDefinition.hpp" +#include "util/config/ConfigFileJson.hpp" +#include "util/config/ConfigValue.hpp" +#include "util/config/Types.hpp" #include "util/log/Logger.hpp" +#include +#include +#include +#include +#include #include #include #include #include +#include #include #include using namespace util; +using util::config::Array; +using util::config::ConfigFileJson; +using util::config::ConfigType; +using util::config::ConfigValue; namespace { size_t @@ -91,3 +107,138 @@ TEST_F(LoggerTest, ManyDynamicLoggers) ASSERT_EQ(loggersNum(), initialLoggers); } + +/** + * @brief Fixture for testing real log-file rotation behaviour. + * + * Unlike LoggerTest (which uses LoggerFixture's in-memory buffer), this fixture + * initialises the LogService with a real file sink and redirects all spdlog + * loggers to that sink so that written messages actually land on disk. + */ +struct LogFileRotationTests : ::testing::Test { + std::filesystem::path const tmpDir = std::filesystem::temp_directory_path() / + fmt::format("clio_log_rotation_tests_{}", + boost::uuids::to_string(boost::uuids::random_generator{}())); + + util::config::ClioConfigDefinition config{ + {"log.channels.[].channel", Array{ConfigValue{ConfigType::String}}}, + {"log.channels.[].level", Array{ConfigValue{ConfigType::String}}}, + + {"log.level", ConfigValue{ConfigType::String}.defaultValue("info")}, + + {"log.format", + ConfigValue{ConfigType::String}.defaultValue(R"(%Y-%m-%d %H:%M:%S.%f %^%3!l:%n%$ - %v)")}, + {"log.is_async", ConfigValue{ConfigType::Boolean}.defaultValue(false)}, + + {"log.enable_console", ConfigValue{ConfigType::Boolean}.defaultValue(false)}, + + {"log.directory", ConfigValue{ConfigType::String}.optional()}, + + {"log.rotation_size", + ConfigValue{ConfigType::Integer}.defaultValue(2048).withConstraint( + util::config::gValidateUint32 + )}, + + {"log.directory_max_files", + ConfigValue{ConfigType::Integer}.defaultValue(25).withConstraint( + util::config::gValidateUint32 + )}, + + {"log.rotate", ConfigValue{ConfigType::Boolean}.defaultValue(true)}, + + {"log.tag_style", ConfigValue{ConfigType::String}.defaultValue("none")}, + }; + + LogFileRotationTests() + { + std::filesystem::remove_all(tmpDir); + if (LogServiceState::initialized()) + LogServiceState::reset(); + } + + ~LogFileRotationTests() override + { + if (LogService::initialized()) + LogService::reset(); + // Leave state initialised so that subsequent tests can call reset(). + LogServiceState::init(false, Severity::FTL, {}); + std::filesystem::remove_all(tmpDir); + } + + /** + * @brief Initialises LogService with the current config and redirects all + * existing spdlog loggers to the newly created file sink. + * + * LogService::init() skips updating sinks on loggers that already exist in + * the spdlog registry. Calling replaceSinks() here ensures every logger + * writes to the file sink regardless of prior test state. + */ + void + initFileLogging() const + { + ASSERT_TRUE(LogService::init(config)); + LogServiceState::replaceSinks(LogServiceState::sinks_); + } + + /** @brief Returns the number of regular files in tmpDir_. */ + [[nodiscard]] std::size_t + countLogFiles() const + { + std::size_t count = 0; + for (auto const& entry : std::filesystem::directory_iterator(tmpDir)) { + if (entry.is_regular_file()) + ++count; + } + return count; + } +}; + +TEST_F(LogFileRotationTests, RotationDisabledProducesSingleLogFile) +{ + auto const parsingErrors = config.parse( + ConfigFileJson{boost::json::object{ + {"log", + boost::json::object{ + {"directory", tmpDir.string()}, + {"rotate", false}, + }} + }} + ); + ASSERT_FALSE(parsingErrors.has_value()); + + initFileLogging(); + + // Write enough data to trigger rotation if it were enabled (> 1 MB). + // Writing at error level flushes immediately because flush_on(err) is set. + Logger const log{"General"}; + std::string const bigMessage(1000, 'x'); + for (int i = 0; i < 1100; ++i) + log.error() << bigMessage; + + EXPECT_EQ(countLogFiles(), 1u); +} + +TEST_F(LogFileRotationTests, RotationEnabledProducesMultipleLogFiles) +{ + auto const parsingErrors = config.parse( + ConfigFileJson{boost::json::object{ + {"log", + boost::json::object{ + {"directory", tmpDir.string()}, + {"rotate", true}, + {"rotation_size", 1}, + {"directory_max_files", 2}, + }} + }} + ); + ASSERT_FALSE(parsingErrors.has_value()); + + initFileLogging(); + + Logger const log{"General"}; + std::string const bigMessage(1000, 'x'); + for (int i = 0; i < 1100; ++i) + log.error() << bigMessage; + + EXPECT_GT(countLogFiles(), 1u); +}