Skip to content
Draft
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
2 changes: 2 additions & 0 deletions src/util/config/ConfigDefinition.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,8 @@ getClioConfig()
{"log.directory_max_files",
ConfigValue{ConfigType::Integer}.defaultValue(25).withConstraint(gValidateUint32)},

{"log.rotate", ConfigValue{ConfigType::Boolean}.defaultValue(true)},

{"log.tag_style",
ConfigValue{ConfigType::String}.defaultValue("none").withConstraint(gValidateLogTag)},

Expand Down
4 changes: 4 additions & 0 deletions src/util/config/ConfigDescription.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,10 @@ Documentation can be found at: <https://github.com/gabime/spdlog/wiki/Custom-for
"file starts."},
KV{.key = "log.directory_max_files",
.value = "The maximum number of log files in the directory."},
KV{.key = "log.rotate",
.value = "Enables or disables log file rotation. When disabled, a single log file is "
"used without size-based rotation. Useful when rotation is managed externally "
"(e.g., via logrotate)."},
KV{.key = "log.tag_style",
.value = "Log tags are unique identifiers for log messages. `uint`/`int` starts logging "
"from 0 and increments, "
Expand Down
30 changes: 23 additions & 7 deletions src/util/log/Logger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <spdlog/formatter.h>
#include <spdlog/logger.h>
#include <spdlog/pattern_formatter.h>
#include <spdlog/sinks/basic_file_sink.h>
#include <spdlog/sinks/rotating_file_sink.h>
#include <spdlog/sinks/stdout_color_sinks.h>
#include <spdlog/spdlog.h>
Expand Down Expand Up @@ -184,12 +185,20 @@ 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);
std::shared_ptr<spdlog::sinks::sink> fileSink;

if (params.rotation.has_value()) {
// rotation sizes are taken from user in MB, but spdlog needs bytes
auto const rotationSize = mbToBytes(params.rotation->sizeMB);
fileSink = std::make_shared<spdlog::sinks::rotating_file_sink_mt>(
(dirPath / "clio.log").string(), rotationSize, params.rotation->maxFiles
);
} else {
fileSink = std::make_shared<spdlog::sinks::basic_file_sink_mt>(
(dirPath / "clio.log").string(), /*truncate=*/false
);
}

auto fileSink = std::make_shared<spdlog::sinks::rotating_file_sink_mt>(
(dirPath / "clio.log").string(), rotationSize, params.dirMaxFiles
);
fileSink->set_level(spdlog::level::trace);
fileSink->set_formatter(std::make_unique<spdlog::pattern_formatter>(format));

Expand Down Expand Up @@ -336,10 +345,17 @@ LogService::getSinks(config::ClioConfigDefinition const& config)
}
}

std::optional<RotationParams> rotation = std::nullopt;
if (config.get<bool>("log.rotate")) {
rotation = RotationParams{
.sizeMB = config.get<uint32_t>("log.rotation_size"),
.maxFiles = config.get<uint32_t>("log.directory_max_files"),
};
}

FileLoggingParams const params{
.logDir = logDir.value(),
.rotationSizeMB = config.get<uint32_t>("log.rotation_size"),
.dirMaxFiles = config.get<uint32_t>("log.directory_max_files"),
.rotation = rotation,
};
allSinks.push_back(createFileSink(params, format));
}
Expand Down
13 changes: 9 additions & 4 deletions src/util/log/Logger.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class sink; // NOLINT(readability-identifier-naming)
struct BenchmarkLoggingInitializer;
class LoggerFixture;
struct LogServiceInitTests;
struct LogFileRotationTests;

namespace util {

Expand Down Expand Up @@ -229,6 +230,7 @@ class LogServiceState {
protected:
friend struct ::LogServiceInitTests;
friend class ::LoggerFixture;
friend struct ::LogFileRotationTests;
friend class Logger;
friend class ::util::impl::OnAssert;

Expand Down Expand Up @@ -386,13 +388,16 @@ class LogService : public LogServiceState {
*/
[[nodiscard]] static std::
expected<std::vector<std::shared_ptr<spdlog::sinks::sink>>, std::string>
getSinks(config::ClioConfigDefinition const& config);
tSinks(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<RotationParams> rotation; ///< nullopt when rotation is disabled
};

friend struct ::BenchmarkLoggingInitializer;
Expand Down
21 changes: 21 additions & 0 deletions tests/unit/util/log/LogServiceInitTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ struct LogServiceInitTests : virtual public LoggerFixture {
{"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")},
};

Expand Down Expand Up @@ -219,3 +221,22 @@ 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<bool>("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<bool>("log.rotate"));
}

144 changes: 144 additions & 0 deletions tests/unit/util/log/LoggerTests.cpp
Original file line number Diff line number Diff line change
@@ -1,14 +1,26 @@
#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 <boost/json/object.hpp>
#include <gtest/gtest.h>
#include <spdlog/logger.h>
#include <spdlog/spdlog.h>

#include <cstddef>
#include <filesystem>
#include <memory>
#include <string>
using namespace util;
using util::config::Array;
using util::config::ConfigFileJson;
using util::config::ConfigType;
using util::config::ConfigValue;

namespace {
size_t
Expand Down Expand Up @@ -91,3 +103,135 @@ 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() / "clio_log_rotation_tests";

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)
{
// rotation_size=1 (MB) is the minimum allowed value; writing > 1 MB triggers rotation.
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);
}
Loading