From b4eb3a0637f80a56efbbf77d24d90eb7305f3697 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Mon, 22 Jan 2024 19:22:00 +0100 Subject: [PATCH] Fixes & comments --- nano/lib/config.cpp | 5 +++-- nano/lib/config.hpp | 8 ++++++-- nano/lib/logging.cpp | 41 +++++++++++++++++++++---------------- nano/lib/logging.hpp | 21 ++++++++++--------- nano/test_common/system.hpp | 2 +- 5 files changed, 44 insertions(+), 33 deletions(-) diff --git a/nano/lib/config.cpp b/nano/lib/config.cpp index d8b3a60d..7f40e3b0 100644 --- a/nano/lib/config.cpp +++ b/nano/lib/config.cpp @@ -377,6 +377,7 @@ std::string_view nano::to_string (nano::networks network) return "n/a"; } +// Using std::cerr here, since logging may not be initialized yet nano::tomlconfig nano::load_toml_file (const std::filesystem::path & config_filename, const std::filesystem::path & data_path, const std::vector & config_overrides) { std::stringstream config_overrides_stream; @@ -396,7 +397,7 @@ nano::tomlconfig nano::load_toml_file (const std::filesystem::path & config_file { throw std::runtime_error (error.get_message ()); } - nano::default_logger ().info (nano::log::type::config, "Config for `{}` loaded from node data directory: ", config_filename.string (), toml_config_path.string ()); + std::cerr << "Config file `" << config_filename.string () << "` loaded from node data directory: " << toml_config_path.string () << std::endl; return toml; } else @@ -408,7 +409,7 @@ nano::tomlconfig nano::load_toml_file (const std::filesystem::path & config_file { throw std::runtime_error (error.get_message ()); } - nano::default_logger ().info (nano::log::type::config, "Config for `{}` not found, using default configuration", config_filename.string ()); + std::cerr << "Config file `" << config_filename.string () << "` not found, using default configuration" << std::endl; return toml; } } diff --git a/nano/lib/config.hpp b/nano/lib/config.hpp index d319d0af..5db3296c 100644 --- a/nano/lib/config.hpp +++ b/nano/lib/config.hpp @@ -410,11 +410,15 @@ bool is_sanitizer_build (); void force_nano_dev_network (); /** - * Attempt to read a configuration file from current working directory, or if not found, the nano root directory. - * Returns empty tomlconfig if nothing is found. + * Attempt to read a configuration file from specified directory. Returns empty tomlconfig if nothing is found. + * @throws std::runtime_error with error code if the file or overrides are not valid toml */ nano::tomlconfig load_toml_file (const std::filesystem::path & config_filename, const std::filesystem::path & data_path, const std::vector & config_overrides); +/** + * Attempt to read a configuration file from specified directory. Returns fallback config if nothing is found. + * @throws std::runtime_error with error code if the file or overrides are not valid toml or deserialization fails + */ template T load_config_file (T fallback, const std::filesystem::path & config_filename, const std::filesystem::path & data_path, const std::vector & config_overrides) { diff --git a/nano/lib/logging.cpp b/nano/lib/logging.cpp index ce87050e..8e906228 100644 --- a/nano/lib/logging.cpp +++ b/nano/lib/logging.cpp @@ -3,18 +3,12 @@ #include #include -#include #include #include #include #include #include -namespace -{ -std::atomic logging_initialized{ false }; -} - nano::logger & nano::default_logger () { static nano::logger logger{ "default" }; @@ -34,9 +28,10 @@ std::function nano::l return std::string{ to_string (tag) }; } }; -void nano::logger::initialize (nano::log_config fallback, std::filesystem::path data_path, std::vector const & config_overrides) +void nano::logger::initialize (nano::log_config fallback, std::optional data_path, std::vector const & config_overrides) { - auto config = nano::load_log_config (std::move (fallback), data_path, config_overrides); + // Only load log config from file if data_path is available (i.e. not running in cli mode) + nano::log_config config = data_path ? nano::load_log_config (fallback, *data_path, config_overrides) : fallback; initialize_common (config, data_path); global_initialized = true; } @@ -97,8 +92,8 @@ public: void nano::logger::initialize_for_tests (nano::log_config fallback) { - auto config = nano::load_log_config (std::move (fallback), /* load log config from current workdir */ {}); - initialize_common (config, /* store log file in current workdir */ {}); + auto config = nano::load_log_config (std::move (fallback), /* load log config from current workdir */ std::filesystem::current_path ()); + initialize_common (config, /* store log file in current workdir */ std::filesystem::current_path ()); // Use tag and identifier as the logger name, since multiple nodes may be running in the same process global_name_formatter = [] (nano::log::type tag, std::string identifier) { @@ -119,13 +114,13 @@ void nano::logger::initialize_for_tests (nano::log_config fallback) global_initialized = true; } -void nano::logger::initialize_common (nano::log_config const & config, std::filesystem::path data_path) +// Using std::cerr here, since logging may not be initialized yet +void nano::logger::initialize_common (nano::log_config const & config, std::optional data_path) { global_config = config; spdlog::set_automatic_registration (false); spdlog::set_level (to_spdlog_level (config.default_level)); - spdlog::cfg::load_env_levels (); global_sinks.clear (); @@ -156,13 +151,16 @@ void nano::logger::initialize_common (nano::log_config const & config, std::file // File setup if (config.file.enable) { + // In cases where data_path is not available, file logging should always be disabled + release_assert (data_path); + auto now = std::chrono::system_clock::now (); auto time = std::chrono::system_clock::to_time_t (now); auto filename = fmt::format ("log_{:%Y-%m-%d_%H-%M}-{:%S}", fmt::localtime (time), now.time_since_epoch ()); std::replace (filename.begin (), filename.end (), '.', '_'); // Replace millisecond dot separator with underscore - std::filesystem::path log_path{ data_path / "log" / (filename + ".log") }; + std::filesystem::path log_path{ data_path.value () / "log" / (filename + ".log") }; log_path = std::filesystem::absolute (log_path); std::cerr << "Logging to file: " << log_path.string () << std::endl; @@ -170,7 +168,7 @@ void nano::logger::initialize_common (nano::log_config const & config, std::file // If either max_size or rotation_count is 0, then disable file rotation if (config.file.max_size == 0 || config.file.rotation_count == 0) { - // TODO: Maybe show a warning to the user about possibly unlimited log file size + std::cerr << "WARNING: Log file rotation is disabled, log file size may grow without bound" << std::endl; auto file_sink = std::make_shared (log_path.string (), true); global_sinks.push_back (file_sink); @@ -198,13 +196,19 @@ void nano::logger::flush () nano::logger::logger (std::string identifier) : identifier{ std::move (identifier) } { + release_assert (global_initialized, "logging should be initialized before creating a logger"); +} + +nano::logger::~logger () +{ + flush (); } spdlog::logger & nano::logger::get_logger (nano::log::type tag) { // This is a two-step process to avoid exclusively locking the mutex in the common case { - std::shared_lock slock{ mutex }; + std::shared_lock lock{ mutex }; if (auto it = spd_loggers.find (tag); it != spd_loggers.end ()) { @@ -215,8 +219,8 @@ spdlog::logger & nano::logger::get_logger (nano::log::type tag) { std::unique_lock lock{ mutex }; - auto [it2, inserted] = spd_loggers.emplace (tag, make_logger (tag)); - return *it2->second; + auto [it, inserted] = spd_loggers.emplace (tag, make_logger (tag)); + return *it->second; } } @@ -242,7 +246,7 @@ std::shared_ptr nano::logger::make_logger (nano::log::type tag) return spd_logger; } -spdlog::level::level_enum nano::to_spdlog_level (nano::log::level level) +spdlog::level::level_enum nano::logger::to_spdlog_level (nano::log::level level) { switch (level) { @@ -437,6 +441,7 @@ std::map nano::log_config::defa * config loading */ +// Using std::cerr here, since logging may not be initialized yet nano::log_config nano::load_log_config (nano::log_config fallback, const std::filesystem::path & data_path, const std::vector & config_overrides) { const std::string config_filename = "config-log.toml"; diff --git a/nano/lib/logging.hpp b/nano/lib/logging.hpp index 4844cda4..55f08ad6 100644 --- a/nano/lib/logging.hpp +++ b/nano/lib/logging.hpp @@ -59,24 +59,19 @@ private: static std::map default_levels (nano::log::level); }; -/// @throws std::runtime_error if the log config file is malformed -nano::log_config load_log_config (nano::log_config fallback, std::filesystem::path const & data_path = {}, std::vector const & config_overrides = std::vector ()); -} - -namespace nano -{ -spdlog::level::level_enum to_spdlog_level (nano::log::level); +nano::log_config load_log_config (nano::log_config fallback, std::filesystem::path const & data_path, std::vector const & config_overrides = {}); class logger final { public: - logger (std::string identifier = ""); + explicit logger (std::string identifier = ""); + ~logger (); // Disallow copies logger (logger const &) = delete; public: - static void initialize (nano::log_config fallback, std::filesystem::path data_path = {}, std::vector const & config_overrides = std::vector ()); + static void initialize (nano::log_config fallback, std::optional data_path = std::nullopt, std::vector const & config_overrides = std::vector ()); static void initialize_for_tests (nano::log_config fallback); static void flush (); @@ -86,7 +81,7 @@ private: static std::vector global_sinks; static std::function global_name_formatter; - static void initialize_common (nano::log_config const &, std::filesystem::path data_path); + static void initialize_common (nano::log_config const &, std::optional data_path); public: template @@ -134,7 +129,13 @@ private: private: spdlog::logger & get_logger (nano::log::type tag); std::shared_ptr make_logger (nano::log::type tag); + + static spdlog::level::level_enum to_spdlog_level (nano::log::level); }; +/** + * Returns a logger instance that can be used before node specific logging is available. + * Should only be used for logging that happens during startup and initialization, since it won't contain node specific identifier. + */ nano::logger & default_logger (); } \ No newline at end of file diff --git a/nano/test_common/system.hpp b/nano/test_common/system.hpp index 8bff4088..8e0cf19b 100644 --- a/nano/test_common/system.hpp +++ b/nano/test_common/system.hpp @@ -67,7 +67,7 @@ namespace test boost::asio::io_context io_ctx; std::vector> nodes; nano::stats stats; - nano::logger logger; + nano::logger logger{ "tests" }; nano::work_pool work{ nano::dev::network_params.network, std::max (nano::hardware_concurrency (), 1u) }; std::chrono::time_point> deadline{ std::chrono::steady_clock::time_point::max () }; double deadline_scaling_factor{ 1.0 };