-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Add ENVOY_LOG_ONCE #12830
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ENVOY_LOG_ONCE #12830
Changes from 10 commits
8f8b17b
6770737
72829d5
63b8a08
9e29f4a
2d47780
99e9686
d8a6304
aa67ebb
d8783b7
371fa5d
9480137
09ff427
de23f2a
53d5cc1
2ab747d
536300e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| #pragma once | ||
|
|
||
| #include <chrono> | ||
| #include <cstdint> | ||
| #include <memory> | ||
| #include <string> | ||
|
|
@@ -400,6 +401,43 @@ template <Id id> class Loggable { | |
| } \ | ||
| } while (0) | ||
|
|
||
| #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__); \ | ||
| } \ | ||
| } while (0) | ||
|
|
||
| #define ENVOY_LOG_ONCE(LEVEL, ...) \ | ||
| do { \ | ||
| ENVOY_LOG_FIRST_N(LEVEL, 1, ##__VA_ARGS__); \ | ||
| } while (0) | ||
|
|
||
| #define ENVOY_LOG_EVERY_NTH(LEVEL, N, ...) \ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per earlier discussion, every Pow2 could be good as well.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yes good catch, my intent was actually to add that. Will do.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, added
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /cc @ggreenway ^^ |
||
| do { \ | ||
| static auto* count = new std::atomic<uint64_t>(); \ | ||
| if ((count->fetch_add(1) % N) == 0) { \ | ||
| 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. | ||
| 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__); \ | ||
| } \ | ||
| } while (0) | ||
|
|
||
| #define ENVOY_FLUSH_LOG() \ | ||
| do { \ | ||
| if (Envoy::Logger::Context::useFancyLogger()) { \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,6 @@ | ||
| #include <chrono> | ||
| #include <functional> | ||
| #include <future> | ||
| #include <iostream> | ||
| #include <string> | ||
|
|
||
|
|
@@ -13,6 +16,8 @@ | |
|
|
||
| namespace Envoy { | ||
|
|
||
| using namespace std::chrono_literals; | ||
|
|
||
| class TestFilterLog : public Logger::Loggable<Logger::Id::filter> { | ||
| public: | ||
| void logMessage() { | ||
|
|
@@ -118,6 +123,80 @@ TEST(Logger, checkLoggerLevel) { | |
| EXPECT_THAT(test_obj.executeAtTraceLevel(), testing::Eq(2)); | ||
| } | ||
|
|
||
| void spamCall(std::function<void()>&& call_to_spam, const uint32_t num_threads) { | ||
| std::vector<std::thread> threads(num_threads); | ||
| std::promise<void> signal_all_threads_running; | ||
| std::shared_future<void> future(signal_all_threads_running.get_future()); | ||
|
|
||
| for (auto& thread : threads) { | ||
| thread = std::thread([future, &call_to_spam] { | ||
| future.wait(); | ||
| call_to_spam(); | ||
| }); | ||
| } | ||
| // Allow threads to accrue on future.wait() to maximize concurrency on the call | ||
| // we are testing. | ||
| sleep(1); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we want to avoid relying on real-time for tests. @jmarantz do you see a way to cleanly add an injected time source for the periodic macro that @oschaaf introduced in this PR? I'm thinking maybe we need a time source singleton for logging and do singleton injection, to avoid having to worry about time source at call site.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 371fa5d I rewrote the test to not rely on |
||
| signal_all_threads_running.set_value(); | ||
| for (std::thread& thread : threads) { | ||
| thread.join(); | ||
| } | ||
| } | ||
|
|
||
| TEST(Logger, SparseLogMacros) { | ||
| class SparseLogMacrosTestHelper : public Logger::Loggable<Logger::Id::filter> { | ||
| public: | ||
| SparseLogMacrosTestHelper() { ENVOY_LOGGER().set_level(spdlog::level::info); } | ||
| void logSomething() { ENVOY_LOG_ONCE(error, "foo1 '{}'", evaluations()++); } | ||
| void logSomethingElse() { ENVOY_LOG_ONCE(error, "foo2 '{}'", evaluations()++); } | ||
| void logSomethingBelowLogLevelOnce() { ENVOY_LOG_ONCE(debug, "foo3 '{}'", evaluations()++); } | ||
| void logSomethingThrice() { ENVOY_LOG_FIRST_N(error, 3, "foo4 '{}'", evaluations()++); } | ||
| void logEverySeventh() { ENVOY_LOG_EVERY_NTH(error, 7, "foo5 '{}'", evaluations()++); } | ||
| void logEverySecond() { ENVOY_LOG_PERIODIC(error, 1s, "foo6 '{}'", evaluations()++); } | ||
| std::atomic<int32_t>& evaluations() { MUTABLE_CONSTRUCT_ON_FIRST_USE(std::atomic<int32_t>); }; | ||
| }; | ||
| constexpr uint32_t kNumThreads = 100; | ||
| SparseLogMacrosTestHelper helper; | ||
| spamCall( | ||
| [&helper]() { | ||
| helper.logSomething(); | ||
| helper.logSomething(); | ||
| }, | ||
| kNumThreads); | ||
| EXPECT_EQ(1, helper.evaluations()); | ||
| spamCall( | ||
| [&helper]() { | ||
| helper.logSomethingElse(); | ||
| helper.logSomethingElse(); | ||
| }, | ||
| kNumThreads); | ||
| // Two distinct log lines ought to result in two evaluations, and no more. | ||
| EXPECT_EQ(2, helper.evaluations()); | ||
|
|
||
| spamCall([&helper]() { helper.logSomethingThrice(); }, kNumThreads); | ||
| // Single log line should be emitted 3 times. | ||
| EXPECT_EQ(5, helper.evaluations()); | ||
|
|
||
| spamCall([&helper]() { helper.logEverySeventh(); }, kNumThreads); | ||
| // (100 threads / log every 7th) + 1s = 15 more evaluations upon logging very 7th. | ||
| EXPECT_EQ(20, helper.evaluations()); | ||
|
|
||
| // So use our knowledge that spamCall will sleep 1 second in the following lines: | ||
| // We expect one log entry / second. Therefore each spamCall ought to result in one | ||
| // more evaluation. This depends on real time and not sim time, hopefully 1 second | ||
| // is enough to not introduce flakes in practice. | ||
| spamCall([&helper]() { helper.logEverySecond(); }, kNumThreads); | ||
| EXPECT_EQ(21, helper.evaluations()); | ||
| spamCall([&helper]() { helper.logEverySecond(); }, kNumThreads); | ||
| EXPECT_EQ(22, helper.evaluations()); | ||
|
|
||
| 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() ? 23 : 22, helper.evaluations()); | ||
| } | ||
|
|
||
| TEST(RegistryTest, LoggerWithName) { | ||
| EXPECT_EQ(nullptr, Logger::Registry::logger("blah")); | ||
| EXPECT_EQ("upstream", Logger::Registry::logger("upstream")->name()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't thread-safe. If two threads do this at the same time, there could be a leak. I think this ends up being better as
static std::atomic<uint64_t> countdown;. Either the compiler will see that it is initialized by zeroing, and do so at program start, or it will double-zero the same memory by initializing multiple times, but should avoid a leak at least.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that lead up to a potential deinitialization fiasco? I was following the lead of
envoy/source/common/common/macros.h
Line 32 in f2306b3
Reading up I am led to believe this would be thread safe since c++ 11 but I can't find a source that I would consider authorative,. Will dig some more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant on the subject of leaking and stack vs heap allocation in the construct on first use idiom: Why doesn’t the Construct On First Use Idiom use a static object instead of a static pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables has:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some reading, and you're correct. TIL something new about static initialization.