Improve env var handling in create_logger

- catch an exception thrown by env var parsing function
- check for unknown parameters set in an env var
- fix wrong logger level when creating logger from
  an environment variable with an "error" level set
- handle Windows absolute path to logfile properly
- extend failure messages
- strict check for standard output logger 'output' option

Author: PatKamin

source/common/logger/ur_level.hpp

@@ -35,7 +35,7 @@ inline auto str_to_level(std::string name) {
     const lvl_name lvl_names[] = {{"debug", Level::DEBUG},
                                   {"info", Level::INFO},
                                   {"warning", Level::WARN},
-                                  {"error", Level::WARN}};
+                                  {"error", Level::ERR}};
 
     for (auto const &item : lvl_names) {
         if (item.name.compare(name) == 0) {

source/common/logger/ur_logger.hpp

@@ -78,39 +78,50 @@ inline Logger create_logger(std::string logger_name, bool skip_prefix) {
     std::unique_ptr<logger::Sink> sink;
 
     env_var_name << "UR_LOG_" << logger_name;
-    auto map = getenv_to_map(env_var_name.str().c_str());
-    if (!map.has_value()) {
-        return Logger(
-            std::make_unique<logger::StderrSink>(logger_name, skip_prefix));
-    }
-
     try {
+        auto map = getenv_to_map(env_var_name.str().c_str());
+        if (!map.has_value()) {
+            return Logger(
+                std::make_unique<logger::StderrSink>(logger_name, skip_prefix));
+        }
+
         auto kv = map->find("level");
         if (kv != map->end()) {
             auto value = kv->second.front();
             level = str_to_level(value);
+            map->erase(kv);
         }
 
         kv = map->find("flush");
         if (kv != map->end()) {
             auto value = kv->second.front();
             flush_level = str_to_level(value);
+            map->erase(kv);
         }
 
         std::vector<std::string> values = {default_output};
         kv = map->find("output");
         if (kv != map->end()) {
             values = kv->second;
+            map->erase(kv);
+        }
+
+        if (!map->empty()) {
+            std::cerr << "Wrong logger environment variable parameter: '"
+                      << map->begin()->first
+                      << "'. Default logger options are set.";
+            return Logger(
+                std::make_unique<logger::StderrSink>(logger_name, skip_prefix));
         }
 
         sink =
             values.size() == 2
                 ? sink_from_str(logger_name, values[0], values[1], skip_prefix)
                 : sink_from_str(logger_name, values[0], "", skip_prefix);
     } catch (const std::invalid_argument &e) {
-        std::cerr
-            << "Error when creating a logger instance from environment variable"
-            << e.what();
+        std::cerr << "Error when creating a logger instance from the '"
+                  << env_var_name.str() << "' environment variable:\n"
+                  << e.what() << std::endl;
         return Logger(
             std::make_unique<logger::StderrSink>(logger_name, skip_prefix));
     }

source/common/logger/ur_sinks.hpp

@@ -6,6 +6,7 @@
 
 #include <fstream>
 #include <iostream>
+#include <sstream>
 
 #if __has_include(<filesystem>)
 #include <filesystem>
@@ -142,12 +143,12 @@ class FileSink : public Sink {
     FileSink(std::string logger_name, filesystem::path file_path,
              bool skip_prefix = false)
         : Sink(logger_name, skip_prefix) {
-        ofstream = std::ofstream(file_path, std::ofstream::out);
+        ofstream = std::ofstream(file_path);
         if (!ofstream.good()) {
-            throw std::invalid_argument(
-                std::string("Failure while opening log file: ") +
-                file_path.string() +
-                std::string(" Check if given path exists."));
+            std::stringstream ss;
+            ss << "Failure while opening log file " << file_path.string()
+               << ". Check if given path exists.";
+            throw std::invalid_argument(ss.str());
         }
         this->ostream = &ofstream;
     }
@@ -164,20 +165,20 @@ class FileSink : public Sink {
 
 inline std::unique_ptr<Sink> sink_from_str(std::string logger_name,
                                            std::string name,
-                                           std::string file_path = "",
+                                           std::filesystem::path file_path = "",
                                            bool skip_prefix = false) {
-    if (name == "stdout") {
+    if (name == "stdout" && file_path.empty()) {
         return std::make_unique<logger::StdoutSink>(logger_name, skip_prefix);
-    } else if (name == "stderr") {
+    } else if (name == "stderr" && file_path.empty()) {
         return std::make_unique<logger::StderrSink>(logger_name, skip_prefix);
     } else if (name == "file" && !file_path.empty()) {
-        return std::make_unique<logger::FileSink>(
-            logger_name, file_path.c_str(), skip_prefix);
+        return std::make_unique<logger::FileSink>(logger_name, file_path,
+                                                  skip_prefix);
     }
 
     throw std::invalid_argument(
         std::string("Parsing error: no valid sink for string '") + name +
-        std::string("' with path '") + file_path + std::string("'.") +
+        std::string("' with path '") + file_path.string() + std::string("'.") +
         std::string("\nValid sink names are: stdout, stderr, file"));
 }
 

source/common/ur_util.hpp

@@ -107,11 +107,13 @@ static void throw_wrong_format_vec(const char *env_var_name) {
     throw std::invalid_argument(ex_ss.str());
 }
 
-static void throw_wrong_format_map(const char *env_var_name) {
+static void throw_wrong_format_map(const char *env_var_name,
+                                   std::string env_var_value) {
     std::stringstream ex_ss;
-    ex_ss << "Wrong format of the " << env_var_name << " environment variable!"
-          << " Proper format is: "
-             "ENV_VAR=\"param_1:value_1,value_2;param_2:value_1";
+    ex_ss << "Wrong format of the " << env_var_name
+          << " environment variable value: '" << env_var_value << "'\n"
+          << "Proper format is: "
+             "ENV_VAR=\"param_1:value_1,value_2;param_2:value_1\"";
     throw std::invalid_argument(ex_ss.str());
 }
 
@@ -188,31 +190,42 @@ inline std::optional<EnvVarMap> getenv_to_map(const char *env_var_name,
         return std::nullopt;
     }
 
+    auto is_quoted = [](std::string &str) {
+        return (str.front() == '\'' && str.back() == '\'') ||
+               (str.front() == '"' && str.back() == '"');
+    };
+    auto has_colon = [](std::string &str) {
+        return str.find(':') != std::string::npos;
+    };
+
     std::stringstream ss(*env_var);
     std::string key_value;
     while (std::getline(ss, key_value, main_delim)) {
         std::string key;
         std::string values;
         std::stringstream kv_ss(key_value);
 
-        if (reject_empty && key_value.find(':') == std::string::npos) {
-            throw_wrong_format_map(env_var_name);
+        if (reject_empty && !has_colon(key_value)) {
+            throw_wrong_format_map(env_var_name, *env_var);
         }
 
         std::getline(kv_ss, key, key_value_delim);
         std::getline(kv_ss, values);
         if (key.empty() || (reject_empty && values.empty()) ||
-            values.find(':') != std::string::npos ||
             map.find(key) != map.end()) {
-            throw_wrong_format_map(env_var_name);
+            throw_wrong_format_map(env_var_name, *env_var);
         }
 
         std::vector<std::string> values_vec;
         std::stringstream values_ss(values);
         std::string value;
         while (std::getline(values_ss, value, values_delim)) {
-            if (value.empty()) {
-                throw_wrong_format_map(env_var_name);
+            if (value.empty() || (has_colon(value) && !is_quoted(value))) {
+                throw_wrong_format_map(env_var_name, *env_var);
+            }
+            if (is_quoted(value)) {
+                value.erase(value.cbegin());
+                value.pop_back();
             }
             values_vec.push_back(value);
         }