logging: don't clamp the log level to DFATAL in debug builds
authorAdam Simpkins <simpkins@fb.com>
Thu, 30 Nov 2017 01:35:11 +0000 (17:35 -0800)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Thu, 30 Nov 2017 01:51:07 +0000 (17:51 -0800)
Summary:
Remove the logic that clamps the log level to DFATAL in debug builds.

This behavior logically makes some sense, but results in subtle and potentially
confusing behavior changes.  It seems confusing that after calling
`setLevel(LogLevel::MAX_LEVEL)` on a log category, calling `getLevel()` on that
object would return `DFATAL` rather than `MAX_LEVEL`.  We also weren't clamping
the level to DFATAL consistently: `setLevel()` would clamp the value, but on
construction the initial level was still set to MAX_LEVEL rather than DFATAL.
This resulted in some issues when implementing `LoggerDB::getConfig()` since we
could not consistently detect if a log category was using the default level
settings.  Rather than fix this inconsistency it seems better to simply remove
this clamping behavior.

This means that it is possible for users to disable DFATAL log messages even in
debug builds if they really want to.  Previously this was only allowed in
release builds.  This protection doesn't really seem all that
valuable--presumably most developers won't really want to do this, and if they
really do request this configuration it doesn't seem all that bad to honor it.

Reviewed By: bolinfest, yfeldblum

Differential Revision: D6200569

fbshipit-source-id: 83ef8e2e4d3b61bc5b105038cbe3132979e9ac67

folly/experimental/logging/LogCategory.cpp

index 66640df..15f8a0d 100644 (file)
@@ -18,6 +18,7 @@
 #include <cstdio>
 #include <cstdlib>
 
+#include <folly/ConstexprMath.h>
 #include <folly/ExceptionString.h>
 #include <folly/FileUtil.h>
 #include <folly/experimental/logging/LogHandler.h>
@@ -155,16 +156,7 @@ void LogCategory::setLevelLocked(LogLevel level, bool inherit) {
   //
   // This makes sure that UNINITIALIZED is always less than any valid level
   // value, and that level values cannot conflict with our flag bits.
-  //
-  // In debug builds we clamp the maximum to DFATAL rather than MAX_LEVEL
-  // (FATAL), to ensure that fatal log messages can never be disabled.
-  constexpr LogLevel maxLogLevel =
-      kIsDebug ? LogLevel::DFATAL : LogLevel::MAX_LEVEL;
-  if (level > maxLogLevel) {
-    level = maxLogLevel;
-  } else if (level < LogLevel::MIN_LEVEL) {
-    level = LogLevel::MIN_LEVEL;
-  }
+  level = constexpr_clamp(level, LogLevel::MIN_LEVEL, LogLevel::MAX_LEVEL);
 
   // Make sure the inherit flag is always off for the root logger.
   if (!parent_) {