Cleaned up the logging level configuration.

Review: https://reviews.apache.org/r/20850
gabrtv · May 7, 2014 · 964acc5 · 964acc5
1 parent 1de238f
commit 964acc5
Show file tree

Hide file tree

Showing 6 changed files with 39 additions and 79 deletions.
diff --git a/src/logging/flags.hpp b/src/logging/flags.hpp
@@ -38,11 +38,11 @@ class Flags : public virtual flags::FlagsBase
         "Disable logging to stderr",
         false);
 
-    add(&Flags::minloglevel,
-        "minloglevel",
-        "Log message at or above this level; if quiet flag\n"
-        "is used, this will affect just the logs from log_dir\n"
-        "(if specified)",
+    add(&Flags::logging_level,
+        "logging_level",
+        "Log message at or above this level; possible values: \n"
+        "'INFO', 'WARNING', 'ERROR'; if quiet flag is used, this \n"
+        "will affect just the logs from log_dir (if specified)",
         "INFO");
 
     add(&Flags::log_dir,
@@ -58,7 +58,7 @@ class Flags : public virtual flags::FlagsBase
   }
 
   bool quiet;
-  std::string minloglevel;
+  std::string logging_level;
   Option<std::string> log_dir;
   int logbufsecs;
 };

diff --git a/src/logging/logging.cpp b/src/logging/logging.cpp
@@ -91,48 +91,18 @@ void handler(int signal)
 }
 
 
-void createLogFile(google::LogSeverity severity)
+google::LogSeverity getLogSeverity(const string& logging_level)
 {
-  if (severity < google::INFO || severity > google::FATAL) {
-    severity = google::INFO;
-  }
-
-  // Log this message in order to create the log file; also
-  // recreate the file if it has been created on a previous run.
-  string msg = "Logging " + string(google::GetLogSeverityName(severity)) +
-               " level started!";
-  switch(severity) {
-    case 0:
-      LOG(INFO) << msg;
-      break;
-    case 1:
-      LOG(WARNING) << msg;
-      break;
-    case 2:
-      LOG(ERROR) << msg;
-      break;
-  }
-}
-
-
-google::LogSeverity getMinLogLevel(const string& minloglevel)
-{
-  if (minloglevel == "INFO") {
+  if (logging_level == "INFO") {
     return google::INFO;
-  }
-  if (minloglevel == "WARNING") {
+  } else if (logging_level == "WARNING") {
     return google::WARNING;
-  }
-  if (minloglevel == "ERROR") {
+  } else if (logging_level == "ERROR") {
     return google::ERROR;
+  } else {
+    // TODO(bmahler): Consider an error here.
+    return google::INFO;
   }
-  if (minloglevel == "FATAL") {
-    return google::FATAL;
-  }
-
-  // Return the default value of logging (INFO) if an invalid
-  // value is passed to minloglevel flag.
-  return google::INFO;
 }
 
 
@@ -149,8 +119,15 @@ void initialize(
 
   argv0 = _argv0;
 
-  // Set glog's parameters through Google Flags variables.
-  FLAGS_minloglevel = getMinLogLevel(flags.minloglevel);
+  if (flags.logging_level != "INFO" &&
+      flags.logging_level != "WARNING" &&
+      flags.logging_level != "ERROR") {
+    EXIT(1) << "'" << flags.logging_level << "' is not a valid logging level."
+               " Possible values for 'logging_level' flag are: "
+               " 'INFO', 'WARNING', 'ERROR'.";
+  }
+
+  FLAGS_minloglevel = getLogSeverity(flags.logging_level);
 
   if (flags.log_dir.isSome()) {
     Try<Nothing> mkdir = os::mkdir(flags.log_dir.get());
@@ -183,8 +160,13 @@ void initialize(
   FLAGS_logbufsecs = flags.logbufsecs;
 
   google::InitGoogleLogging(argv0.c_str());
-  if (flags.log_dir.isSome() && FLAGS_minloglevel != google::FATAL) {
-    createLogFile(FLAGS_minloglevel);
+  if (flags.log_dir.isSome()) {
+    // Log this message in order to create the log file; this is because GLOG
+    // creates the log file once the first log message occurs; also recreate
+    // the file if it has been created on a previous run.
+    LOG_AT_LEVEL(FLAGS_minloglevel)
+      << google::GetLogSeverityName(FLAGS_minloglevel)
+      << " level logging started!";
   }
 
   VLOG(1) << "Logging to " <<

diff --git a/src/logging/logging.hpp b/src/logging/logging.hpp
@@ -40,12 +40,8 @@ void initialize(
 Try<std::string> getLogFile(google::LogSeverity severity);
 
 
-// Creates the log file for the provided severity.
-void createLogFile(google::LogSeverity severity);
-
-
-// Returns the minimum level of logging as a number.
-google::LogSeverity getMinLogLevel(const std::string& minloglevel);
+// Returns the provided logging level as a LogSeverity type.
+google::LogSeverity getLogSeverity(const std::string& logging_level);
 
 } // namespace logging {
 } // namespace internal {

diff --git a/src/master/master.cpp b/src/master/master.cpp
@@ -522,12 +522,10 @@ void Master::initialize()
   provide("", path::join(flags.webui_dir, "master/static/index.html"));
   provide("static", path::join(flags.webui_dir, "master/static"));
 
-  // No need to access FATAL log file; if the program
-  // is still running, there definitely haven't been any
-  // FATAL logs yet; a FATAL log will cause the program to crash.
-  google::LogSeverity minloglevel = logging::getMinLogLevel(flags.minloglevel);
-  if (flags.log_dir.isSome() && minloglevel != google::FATAL) {
-    Try<string> log = logging::getLogFile(minloglevel);
+  if (flags.log_dir.isSome()) {
+    Try<string> log = logging::getLogFile(
+        logging::getLogSeverity(flags.logging_level));
+
     if (log.isError()) {
       LOG(ERROR) << "Master log file cannot be found: " << log.error();
     } else {

diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp
@@ -382,12 +382,10 @@ void Slave::initialize()
   route("/stats.json", None(), lambda::bind(&Http::stats, http, lambda::_1));
   route("/state.json", None(), lambda::bind(&Http::state, http, lambda::_1));
 
-  // No need to access FATAL log file; if the program
-  // is still running, there definitely haven't been any
-  // FATAL logs yet; a FATAL log will cause the program to crash.
-  google::LogSeverity minloglevel = logging::getMinLogLevel(flags.minloglevel);
-  if (flags.log_dir.isSome() && minloglevel != google::FATAL) {
-    Try<string> log = logging::getLogFile(minloglevel);
+  if (flags.log_dir.isSome()) {
+    Try<string> log = logging::getLogFile(
+        logging::getLogSeverity(flags.logging_level));
+
     if (log.isError()) {
       LOG(ERROR) << "Slave log file cannot be found: " << log.error();
     } else {

diff --git a/src/webui/master/static/js/controllers.js b/src/webui/master/static/js/controllers.js
@@ -340,13 +340,6 @@
           "Set the 'log_dir' option if you wish to access the logs.",
           [{label: 'Continue'}]
         ).open();
-      } else if ($scope.state.flags.minloglevel == "FATAL") {
-        $dialog.messageBox(
-          'No FATAL logs yet',
-          "The configured log level is FATAL. For more meaningful logs,\
-          please set 'minloglevel' flag to INFO, WARNING or ERROR.",
-          [{label: 'Continue'}]
-        ).open();
       } else {
         pailer(
             $scope.$location.host() + ':' + $scope.$location.port(),
@@ -412,13 +405,6 @@
             "Set the 'log_dir' option if you wish to access the logs.",
             [{label: 'Continue'}]
           ).open();
-        } else if ($scope.state.flags.minloglevel == "FATAL") {
-          $dialog.messageBox(
-            'No FATAL logs yet',
-            "The configured log level is FATAL. For more meaningful logs, please set \
-            'minloglevel' flag to INFO, WARNING or ERROR.",
-            [{label: 'Continue'}]
-          ).open();
         } else {
           pailer(host, '/slave/log', 'Mesos Slave');
         }