fix: configuration parsing error mmap_log_buffer_size #2363

MorganaFuture · 2025-03-16T20:09:14Z

What
This PR introduces a JSON-based configuration system for the Tempesta FW logger daemon with:

A dedicated TfwLoggerConfig class for logger settings
JSON configuration file support via Boost's property_tree
Two operation modes: daemon (background) and handle (foreground for debugging)
Support for a logger_config attribute in the access_log directive
Updates to Tempesta scripts for compatibility
Backward compatibility with existing configuration

Why
The Tempesta logger configuration is currently scattered across multiple parameters, making it hard to manage. A dedicated JSON file provides a cleaner approach that separates logger settings from the main configuration. This structured format improves error handling and simplifies troubleshooting with the new foreground mode.

Links
2313

MorganaFuture · 2025-03-16T20:19:30Z

I have been starting work on 2313.

MorganaFuture · 2025-03-21T13:08:10Z

@krizhanovsky I slightly modified tfw_logger to make it work with JSON files. I also did some refactoring on it. Could you please assign someone to review these changes?

const-t · 2025-03-24T11:56:46Z

fw/access_log.c

-	TFW_CFG_CHECK_NO_ATTRS(cs, ce);
+
+	// Check for logger_config attribute
+	const char *logger_config = tfw_cfg_get_attr(ce, "logger_config", NULL);


Please delete this code, tfw_logger_config_path is not used now, there is no sense to keep it. If it become necessary in future it's not a big deal to add it.

const-t · 2025-03-24T11:58:31Z

fw/access_log.c

@@ -596,6 +611,19 @@ cfg_access_log_set(TfwCfgSpec *cs, TfwCfgEntry *ce)
 		return -EINVAL;
 	}

+	// Parse other attributes if using mmap
+	if (access_log_type & ACCESS_LOG_MMAP) {


Please remove this part. For tfw_logger we do validation only in tempesta.sh file, but now we must do it in tfw_logger.

const-t · 2025-03-24T12:11:05Z

utils/tfw_logger_config.hh

+    uint16_t port{9000};                     // ClickHouse server port
+    std::optional<std::string> user;         // Optional username
+    std::optional<std::string> password;     // Optional password
+    std::string table_name{"access_log"};    // Table name


There is no sense to add table name to config, anyway we create table using hardcoded name. Even if change this behavior, I think we not need to have this in the configuration file, at least now. The same for database name.

By the way, quite a good question: we supposed that Clickhouse is installed solely for us, but a user might want to use existing database instance for the logs. I.e. web frameworks typicallay provide not only database credentials and IP with port, but also database name and probably a table name.

const-t · 2025-03-24T12:52:15Z

utils/tfw_logger.cc

+    }
+    else
+    {
+        cpu_cnt = std::thread::hardware_concurrency();


I would prefer to not use std::thread::hardware_concurrency and always use sysconf as reliable source.

@const-t What's wrong with std::thread::hardware_concurrency?

@Kutumov I'm not sure that it works well with NUMA and with machines that has 100+ cpus. It may return invalid cpu count, but not zero and in this case we even can't fallback to sysconf. Even cppref describes it as: The value should be considered only a hint.

This must be commented in the source code - I was also thinking that std::thread::hardware_concurrency is good

const-t · 2025-03-24T13:35:49Z

utils/tfw_logger.cc

+	fs::path config_path = "/etc/tempesta/tfw_logger.json";
+
+	try
+	{		


Suggested change

{

{

const-t · 2025-03-24T13:52:17Z

utils/tfw_logger.cc

+	std::shared_ptr<spdlog::logger> logger = nullptr;
+	fs::path config_path = "/etc/tempesta/tfw_logger.json";
+
+	try


Suggested change

try

try {

Please have a look on our codingstyle, it not covers c++ completely, but we follow it where it's possible. For instance indentations.

const-t · 2025-03-24T14:08:47Z

utils/tfw_logger.cc

+{
+	int fd = -1, res = 0;
+	std::shared_ptr<spdlog::logger> logger = nullptr;
+	fs::path config_path = "/etc/tempesta/tfw_logger.json";


Hardcoding the config path doesn't look good. You've done a great job adding a configuration file, and I believe we should use its benefits. My suggestion:

Pass config_path as CLI argument into tfw_logger, but construct config_path in tempesta.sh using TFW_ROOT constant. Just as we do for tfw_cfg_path and tfw_cfg_temp.

Remove from tempesta.sh parsing of mmap_* directives, you moved them to tfw_logger let validate them there. Therefore do not pass mmap_* parameters from tempesta's config to tfw_logger, they are already there.

Give CLI arguments higher precedence over config file. To be able to override params defined in config using CLI.

@MorganaFuture If you need some clarifications, feel free to ask.

const-t · 2025-03-24T17:14:32Z

utils/tfw_logger_config.hh

+  size_t buffer_size{4 * 1024 * 1024};                   // 4MB default buffer size
+  size_t cpu_count{0};                                   // 0 means auto-detect
+  ClickHouseConfig clickhouse;                           // ClickHouse configuration
+  bool debug{false};                                     // Debug mode flag


Looks like unused flag, delete it please.

const-t · 2025-03-24T17:15:34Z

utils/tfw_logger.cc

+									 ch_cfg.password ? *ch_cfg.password : "",
+									 make_block());
+
+


Unnecessary blank line, please delete them in other places as well.

Kutumov · 2025-03-28T08:56:21Z

utils/tfw_logger.cc

@@ -266,6 +288,7 @@ run_thread(const int ncpu, const int fd, const std::string &host,
 			break;
 		}
 		catch (const Exception &e) {
+			std::cerr << "Exception in run_thread: " << e.what() << std::endl;


Why do we need the second error message?

Kutumov · 2025-03-28T09:06:59Z

utils/tfw_logger.cc

+std::shared_ptr<spdlog::logger> 
+setup_logger(const TfwLoggerConfig &config)
+{
+    try


Please reduce try area to the lines that may throw spdlog::spdlog_ex

Kutumov · 2025-03-28T09:11:19Z

utils/tfw_logger.cc

+    else
+    {
+     	// If config file not found or invalid, use command line args
+        std::cout << "Could not load configuration from file, using command line arguments" << std::endl;


In my opinion if expected configuration fails(or its part) it is better to exit in order not to execute the service with undesirable settings. @const-t FYI

Kutumov · 2025-03-28T09:24:11Z

utils/tfw_logger_config.cc

+        return std::nullopt;
+    }
+
+    try


Please reduce the try area to the lines that may throw

Kutumov · 2025-03-28T09:26:08Z

utils/tfw_logger_config.cc

+
+        TfwLoggerConfig config;
+        config.parse_from_ptree(tree);
+        return config;


Why not std::move(config)?

Kutumov · 2025-03-28T09:41:35Z

utils/tfw_logger.cc

+        std::cerr << "Error parsing command line: " << e.what() << std::endl;
+        std::cerr << "Use --help for usage information" << std::endl;
+        result = CommandResult::ERROR;
+        parse_error = true;


Maybe just return here and no need in parse_error?

Kutumov · 2025-03-28T10:02:02Z

utils/tfw_logger.cc

+
+	for (size_t i = 0; i < cpu_count; ++i)
+	{
+		std::packaged_task<void(int, int, TfwLoggerConfig)> task(run_thread);


packaged_task is redundant here. We can just wait when threads finish using std::thread::join

krizhanovsky · 2025-04-01T15:21:06Z

fw/access_log.c

@@ -576,7 +577,21 @@ cfg_access_log_set(TfwCfgSpec *cs, TfwCfgEntry *ce)
 	bool off = false;

 	TFW_CFG_CHECK_VAL_N(>, 0, cs, ce);
-	TFW_CFG_CHECK_NO_ATTRS(cs, ce);
+
+	// Check for logger_config attribute


We don't use C++ style comments in C, https://github.com/tempesta-tech/tempesta/blob/master/CodingStyle . In general https://tempesta-tech.com/knowledge-base/Development-guidelines/ is worth reading

krizhanovsky · 2025-04-01T18:41:02Z

utils/tfw_logger.cc

+	while (!stop_flag)
+	{


Suggested change

while (!stop_flag)

{

while (!stop_flag) {

krizhanovsky · 2025-04-01T18:41:23Z

utils/tfw_logger.cc

+		try
+		{


Suggested change

try

{

try {

krizhanovsky · 2025-04-01T18:41:40Z

utils/tfw_logger.cc

+			TfwClickhouse clickhouse(ch_cfg.host, ch_cfg.table_name,
+									 ch_cfg.user ? *ch_cfg.user : "",
+									 ch_cfg.password ? *ch_cfg.password : "",
+									 make_block());


too wide line and something is broken with identation

krizhanovsky · 2025-04-01T18:48:42Z

utils/tfw_logger.cc

+	if (fs::exists(config_path))
+	{
+		return;
+	}


Suggested change

if (fs::exists(config_path))

{

return;

}

if (fs::exists(config_path))

return;

krizhanovsky · 2025-04-01T19:11:12Z

utils/tfw_logger.cc

+	{
+		spdlog::info("Starting in handle (foreground) mode");
+	}
+}


I think we don't need daemonization. The modern Linux environments provide this out of the box and in the modern our projects we don't daemonize servers

krizhanovsky · 2025-04-01T19:20:26Z

utils/tfw_logger.cc

+		throw Except("Failed to open PID file");
+	pid_file << getpid();
+	pid_file.close();
+}


what's the purpose of the pid file? Typically in Linux we also should lock it and set the right permissions. @ttaym you recently worked with a nice small library for PID files, could you please reference it that we can borrow it's code here as well?

krizhanovsky · 2025-04-01T19:22:46Z

utils/tfw_logger.cc


-		spdlog::info("Daemon stopped");
+		// Open the device
+		fd = open_device();


Probably the comment is redundant - basically the function name is just the same as the comment

krizhanovsky · 2025-04-01T19:26:13Z

utils/tfw_logger_config.hh

+    uint16_t port{9000};                     // ClickHouse server port
+    std::optional<std::string> user;         // Optional username
+    std::optional<std::string> password;     // Optional password
+    std::string table_name{"access_log"};    // Table name


By the way, quite a good question: we supposed that Clickhouse is installed solely for us, but a user might want to use existing database instance for the logs. I.e. web frameworks typicallay provide not only database credentials and IP with port, but also database name and probably a table name.

krizhanovsky · 2025-04-01T19:27:13Z

utils/tfw_logger_config.hh

+  fs::path pid_file{"/var/run/tfw_logger.pid"};          // PID file path
+  size_t buffer_size{4 * 1024 * 1024};                   // 4MB default buffer size
+  size_t cpu_count{0};                                   // 0 means auto-detect
+  ClickHouseConfig clickhouse;                           // ClickHouse configuration


Issues with identation and usually we name members as clickhouse_, with _ suffix

MorganaFuture · 2025-05-23T09:17:34Z

The pull request is being moved here #2428

MorganaFuture force-pushed the morganaFuture/fix/configuration_parsing_error_mmap_log_buffer_size branch from c93a0ef to c6560d8 Compare March 16, 2025 20:20

fix: configuration parsing error mmap_log_buffer_size

117cd6d

MorganaFuture force-pushed the morganaFuture/fix/configuration_parsing_error_mmap_log_buffer_size branch from c6560d8 to 117cd6d Compare March 21, 2025 12:56

MorganaFuture marked this pull request as ready for review March 21, 2025 13:08

krizhanovsky requested review from krizhanovsky and const-t March 21, 2025 14:31

const-t requested changes Mar 24, 2025

View reviewed changes

Kutumov reviewed Mar 28, 2025

View reviewed changes

utils/tfw_logger_config.cc

TfwLoggerConfig config;

config.parse_from_ptree(tree);

return config;

Copy link

Contributor

Kutumov Mar 28, 2025

Choose a reason for hiding this comment

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

Why not std::move(config)?

Kutumov reviewed Mar 28, 2025

View reviewed changes

EvgeniiMekhanik self-requested a review March 29, 2025 02:42

krizhanovsky reviewed Apr 1, 2025

View reviewed changes

MorganaFuture closed this May 23, 2025

krizhanovsky mentioned this pull request May 23, 2025

Morgana future/fix/configuration parsing error mmap log buffer size #2428

Open

fix: configuration parsing error mmap_log_buffer_size #2363

fix: configuration parsing error mmap_log_buffer_size #2363

Uh oh!

Conversation

MorganaFuture commented Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MorganaFuture commented Mar 16, 2025

Uh oh!

MorganaFuture commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

const-t Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kutumov Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorganaFuture commented May 23, 2025

Uh oh!

Uh oh!

MorganaFuture commented Mar 16, 2025 •

edited

Loading

MorganaFuture commented Mar 21, 2025 •

edited

Loading

const-t Mar 28, 2025 •

edited

Loading

Kutumov Mar 28, 2025 •

edited

Loading