Review feedback

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
envoyproxy · Sep 4, 2020 · 85003d4 · 85003d4
1 parent 42a4eba
commit 85003d4
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 20 deletions.
diff --git a/source/common/common/logger.h b/source/common/common/logger.h
@@ -404,9 +404,11 @@ template <Id id> class Loggable {
 
 #define ENVOY_LOG_FIRST_N(LEVEL, N, ...)                                                           \
   do {                                                                                             \
-    static auto* countdown = new std::atomic<uint64_t>();                                          \
-    if (countdown->fetch_add(1) < N) {                                                             \
-      ENVOY_LOG(LEVEL, ##__VA_ARGS__);                                                             \
+    if (ENVOY_LOG_COMP_LEVEL(ENVOY_LOGGER(), LEVEL)) {                                             \
+      static auto* countdown = new std::atomic<uint64_t>();                                        \
+      if (countdown->fetch_add(1) < N) {                                                           \
+        ENVOY_LOG(LEVEL, ##__VA_ARGS__);                                                           \
+      }                                                                                            \
     }                                                                                              \
   } while (0)
 
@@ -417,33 +419,40 @@ template <Id id> class Loggable {
 
 #define ENVOY_LOG_EVERY_NTH(LEVEL, N, ...)                                                         \
   do {                                                                                             \
-    static auto* count = new std::atomic<uint64_t>();                                              \
-    if ((count->fetch_add(1) % N) == 0) {                                                          \
-      ENVOY_LOG(LEVEL, ##__VA_ARGS__);                                                             \
+    if (ENVOY_LOG_COMP_LEVEL(ENVOY_LOGGER(), LEVEL)) {                                             \
+      static auto* count = new std::atomic<uint64_t>();                                            \
+      if ((count->fetch_add(1) % N) == 0) {                                                        \
+        ENVOY_LOG(LEVEL, ##__VA_ARGS__);                                                           \
+      }                                                                                            \
     }                                                                                              \
   } while (0)
 
 #define ENVOY_LOG_EVERY_POW_2(LEVEL, ...)                                                          \
   do {                                                                                             \
-    static auto* count = new std::atomic<uint64_t>();                                              \
-    if (std::bitset<64>(1 + count->fetch_add(1)).count() == 1) {                                   \
-      ENVOY_LOG(LEVEL, ##__VA_ARGS__);                                                             \
+    if (ENVOY_LOG_COMP_LEVEL(ENVOY_LOGGER(), LEVEL)) {                                             \
+      static auto* count = new std::atomic<uint64_t>();                                            \
+      if (std::bitset<64>(1 + count->fetch_add(1)).count() == 1) {                                 \
+        ENVOY_LOG(LEVEL, ##__VA_ARGS__);                                                           \
+      }                                                                                            \
     }                                                                                              \
   } while (0)
 
 // This is to get us to pass the format check. We reference a real-world time source here.
-// I think it's not easy to mock/simulate time here as it stands today anyway.
+// We'd have to introduce a singleton for a time source here, and consensus was that avoiding
+// that is preferrable.
 using t_logclock = std::chrono::steady_clock; // NOLINT
 
 #define ENVOY_LOG_PERIODIC(LEVEL, CHRONO_DURATION, ...)                                            \
   do {                                                                                             \
-    static auto* last_hit = new std::atomic<uint64_t>();                                           \
-    auto last = last_hit->load();                                                                  \
-    const auto now = t_logclock::now().time_since_epoch().count();                                 \
-    if ((now - last) >                                                                             \
-            std::chrono::duration_cast<std::chrono::nanoseconds>(CHRONO_DURATION).count() &&       \
-        last_hit->compare_exchange_strong(last, now)) {                                            \
-      ENVOY_LOG(LEVEL, ##__VA_ARGS__);                                                             \
+    if (ENVOY_LOG_COMP_LEVEL(ENVOY_LOGGER(), LEVEL)) {                                             \
+      static auto* last_hit = new std::atomic<uint64_t>();                                         \
+      auto last = last_hit->load();                                                                \
+      const auto now = t_logclock::now().time_since_epoch().count();                               \
+      if ((now - last) >                                                                           \
+              std::chrono::duration_cast<std::chrono::nanoseconds>(CHRONO_DURATION).count() &&     \
+          last_hit->compare_exchange_strong(last, now)) {                                          \
+        ENVOY_LOG(LEVEL, ##__VA_ARGS__);                                                           \
+      }                                                                                            \
     }                                                                                              \
   } while (0)
 

diff --git a/test/common/common/log_macros_test.cc b/test/common/common/log_macros_test.cc
@@ -151,7 +151,7 @@ TEST(Logger, SparseLogMacros) {
     void logSomethingThrice() { ENVOY_LOG_FIRST_N(error, 3, "foo4 '{}'", evaluations()++); }
     void logEverySeventh() { ENVOY_LOG_EVERY_NTH(error, 7, "foo5 '{}'", evaluations()++); }
     void logEveryPow2() { ENVOY_LOG_EVERY_POW_2(error, "foo6 '{}'", evaluations()++); }
-    void logEverySecond() { ENVOY_LOG_PERIODIC(error, 1s, "foo6 '{}'", evaluations()++); }
+    void logEverySecond() { ENVOY_LOG_PERIODIC(error, 1s, "foo7 '{}'", evaluations()++); }
     std::atomic<int32_t>& evaluations() { MUTABLE_CONSTRUCT_ON_FIRST_USE(std::atomic<int32_t>); };
   };
   constexpr uint32_t kNumThreads = 100;
@@ -203,8 +203,7 @@ TEST(Logger, SparseLogMacros) {
   spamCall([&helper]() { helper.logSomethingBelowLogLevelOnce(); }, kNumThreads);
   // Without fine-grained logging, we shouldn't observe additional argument evaluations
   // for log lines below the configured log level.
-  // TODO(#12885): fancy logger shouldn't always evaluate variadic macro arguments.
-  EXPECT_EQ(::Envoy::Logger::Context::useFancyLogger() ? 30 : 29, helper.evaluations());
+  EXPECT_EQ(29, helper.evaluations());
 }
 
 TEST(RegistryTest, LoggerWithName) {