From 85003d494b498d85dbc1bd49bbffa94c7e37d61e Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 4 Sep 2020 14:12:20 +0200 Subject: [PATCH] Review feedback Signed-off-by: Otto van der Schaaf --- source/common/common/logger.h | 43 ++++++++++++++++----------- test/common/common/log_macros_test.cc | 5 ++-- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/source/common/common/logger.h b/source/common/common/logger.h index 2390dad83786..cdeea32f8708 100644 --- a/source/common/common/logger.h +++ b/source/common/common/logger.h @@ -404,9 +404,11 @@ template class Loggable { #define ENVOY_LOG_FIRST_N(LEVEL, N, ...) \ do { \ - static auto* countdown = new std::atomic(); \ - 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(); \ + if (countdown->fetch_add(1) < N) { \ + ENVOY_LOG(LEVEL, ##__VA_ARGS__); \ + } \ } \ } while (0) @@ -417,33 +419,40 @@ template class Loggable { #define ENVOY_LOG_EVERY_NTH(LEVEL, N, ...) \ do { \ - static auto* count = new std::atomic(); \ - 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(); \ + 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(); \ - 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(); \ + 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(); \ - auto last = last_hit->load(); \ - const auto now = t_logclock::now().time_since_epoch().count(); \ - if ((now - last) > \ - std::chrono::duration_cast(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(); \ + auto last = last_hit->load(); \ + const auto now = t_logclock::now().time_since_epoch().count(); \ + if ((now - last) > \ + std::chrono::duration_cast(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 index e6742c5d4790..18642fef7a9e 100644 --- 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& evaluations() { MUTABLE_CONSTRUCT_ON_FIRST_USE(std::atomic); }; }; 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) {