diff --git a/source/common/common/hash.h b/source/common/common/hash.h index d4f637bbbe59b..b6c50e63ac74f 100644 --- a/source/common/common/hash.h +++ b/source/common/common/hash.h @@ -77,16 +77,49 @@ class MurmurHash { static inline uint64_t shift_mix(uint64_t v) { return v ^ (v >> 47); } }; -struct CharStarHash { +struct ConstCharStarHash { size_t operator()(const char* a) const { return HashUtil::xxHash64(a); } }; -struct CharStarEqual { +struct ConstCharStarEqual { size_t operator()(const char* a, const char* b) const { return strcmp(a, b) == 0; } }; template -using CharStarHashMap = absl::flat_hash_map; -using CharStarHashSet = absl::flat_hash_set; +using ConstCharStarHashMap = + absl::flat_hash_map; +using ConstCharStarHashSet = + absl::flat_hash_set; + +using SharedString = std::shared_ptr; + +struct HeterogeneousStringHash { + // Specifying is_transparent indicates to the library infrastructure that + // type-conversions should not be applied when calling find(), but instead + // pass the actual types of the contained and searched-for objects directly to + // these functors. See + // https://en.cppreference.com/w/cpp/utility/functional/less_void for an + // official reference, and https://abseil.io/tips/144 for a description of + // using it in the context of absl. + using is_transparent = void; + + size_t operator()(absl::string_view a) const { return HashUtil::xxHash64(a); } + size_t operator()(const SharedString& a) const { return HashUtil::xxHash64(*a); } +}; + +struct HeterogeneousStringEqual { + // See description for HeterogeneousStringHash::is_transparent. + using is_transparent = void; + + size_t operator()(absl::string_view a, absl::string_view b) const { return a == b; } + size_t operator()(const SharedString& a, const SharedString& b) const { return *a == *b; } + size_t operator()(absl::string_view a, const SharedString& b) const { return a == *b; } + size_t operator()(const SharedString& a, absl::string_view b) const { return *a == b; } +}; + +// We use heterogeneous hash/equal functors to do a find() without constructing +// a shared_string, which would entail making a full copy of the stat name. +using SharedStringSet = + absl::flat_hash_set; } // namespace Envoy diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 2c72dbf94b983..4e58aec9e1446 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -58,10 +58,10 @@ void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass& } } for (const char* stat_name : remove_list) { - auto p = map.find(stat_name); - ASSERT(p != map.end()); - list.push_back(p->second); // Save SharedPtr to the list to avoid invalidating refs to stat. - map.erase(p); + auto iter = map.find(stat_name); + ASSERT(iter != map.end()); + list.push_back(iter->second); // Save SharedPtr to the list to avoid invalidating refs to stat. + map.erase(iter); } } @@ -75,7 +75,7 @@ bool ThreadLocalStoreImpl::rejects(const std::string& name) const { std::vector ThreadLocalStoreImpl::counters() const { // Handle de-dup due to overlapping scopes. std::vector ret; - CharStarHashSet names; + ConstCharStarHashSet names; Thread::LockGuard lock(lock_); for (ScopeImpl* scope : scopes_) { for (auto& counter : scope->central_cache_.counters_) { @@ -98,7 +98,7 @@ ScopePtr ThreadLocalStoreImpl::createScope(const std::string& name) { std::vector ThreadLocalStoreImpl::gauges() const { // Handle de-dup due to overlapping scopes. std::vector ret; - CharStarHashSet names; + ConstCharStarHashSet names; Thread::LockGuard lock(lock_); for (ScopeImpl* scope : scopes_) { for (auto& gauge : scope->central_cache_.gauges_) { @@ -217,12 +217,47 @@ std::atomic ThreadLocalStoreImpl::ScopeImpl::next_scope_id_; ThreadLocalStoreImpl::ScopeImpl::~ScopeImpl() { parent_.releaseScopeCrossThread(this); } +bool ThreadLocalStoreImpl::checkAndRememberRejection(const std::string& name, + SharedStringSet& central_rejected_stats, + SharedStringSet* tls_rejected_stats) { + if (stats_matcher_->acceptsAll()) { + return false; + } + + auto iter = central_rejected_stats.find(name); + SharedString rejected_name; + if (iter != central_rejected_stats.end()) { + rejected_name = *iter; + } else { + if (rejects(name)) { + rejected_name = std::make_shared(name); + central_rejected_stats.insert(rejected_name); + } + } + if (rejected_name != nullptr) { + if (tls_rejected_stats != nullptr) { + tls_rejected_stats->insert(rejected_name); + } + return true; + } + return false; +} + template StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( const std::string& name, StatMap>& central_cache_map, - MakeStatFn make_stat, StatMap>* tls_cache) { + SharedStringSet& central_rejected_stats, MakeStatFn make_stat, + StatMap>* tls_cache, SharedStringSet* tls_rejected_stats, + StatType& null_stat) { const char* stat_key = name.c_str(); + + // We do name-rejections on the full name, prior to truncation. + if (tls_rejected_stats != nullptr && + tls_rejected_stats->find(stat_key) != tls_rejected_stats->end()) { + return null_stat; + } + std::unique_ptr truncation_buffer; absl::string_view truncated_name = parent_.truncateStatNameIfNeeded(name); if (truncated_name.size() < name.size()) { @@ -241,10 +276,13 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( // We must now look in the central store so we must be locked. We grab a reference to the // central store location. It might contain nothing. In this case, we allocate a new stat. Thread::LockGuard lock(parent_.lock_); - auto p = central_cache_map.find(stat_key); + auto iter = central_cache_map.find(stat_key); std::shared_ptr* central_ref = nullptr; - if (p != central_cache_map.end()) { - central_ref = &(p->second); + if (iter != central_cache_map.end()) { + central_ref = &(iter->second); + } else if (parent_.checkAndRememberRejection(name, central_rejected_stats, tls_rejected_stats)) { + // Note that again we do the name-rejection lookup on the untruncated name. + return null_stat; } else { // If we had to truncate, warn now that we've missed all caches. if (truncation_buffer != nullptr) { @@ -285,6 +323,10 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( } Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) { + if (parent_.rejectsAll()) { + return null_counter_; + } + // Determine the final name based on the prefix and the passed name. // // Note that we can do map.find(final_name.c_str()), but we cannot do @@ -295,24 +337,24 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) { // strategy costs an extra hash lookup for each miss, but saves time // re-copying the string and significant memory overhead. std::string final_name = prefix_ + name; - if (parent_.rejects(final_name)) { - return null_counter_; - } // We now find the TLS cache. This might remain null if we don't have TLS // initialized currently. StatMap* tls_cache = nullptr; + SharedStringSet* tls_rejected_stats = nullptr; if (!parent_.shutting_down_ && parent_.tls_) { - tls_cache = &parent_.tls_->getTyped().scope_cache_[this->scope_id_].counters_; + TlsCacheEntry& entry = parent_.tls_->getTyped().scope_cache_[this->scope_id_]; + tls_cache = &entry.counters_; + tls_rejected_stats = &entry.rejected_stats_; } return safeMakeStat( - final_name, central_cache_.counters_, + final_name, central_cache_.counters_, central_cache_.rejected_stats_, [](StatDataAllocator& allocator, absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) -> CounterSharedPtr { return allocator.makeCounter(name, std::move(tag_extracted_name), std::move(tags)); }, - tls_cache); + tls_cache, tls_rejected_stats, null_counter_); } void ThreadLocalStoreImpl::ScopeImpl::deliverHistogramToSinks(const Histogram& histogram, @@ -332,6 +374,10 @@ void ThreadLocalStoreImpl::ScopeImpl::deliverHistogramToSinks(const Histogram& h } Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) { + if (parent_.rejectsAll()) { + return null_gauge_; + } + // See comments in counter(). There is no super clean way (via templates or otherwise) to // share this code so I'm leaving it largely duplicated for now. // @@ -341,25 +387,29 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) { // do a find() first, using that if it succeeds. If it fails, then after we // construct the stat we can insert it into the required maps. std::string final_name = prefix_ + name; - if (parent_.rejects(final_name)) { - return null_gauge_; - } StatMap* tls_cache = nullptr; + SharedStringSet* tls_rejected_stats = nullptr; if (!parent_.shutting_down_ && parent_.tls_) { - tls_cache = &parent_.tls_->getTyped().scope_cache_[this->scope_id_].gauges_; + TlsCacheEntry& entry = parent_.tls_->getTyped().scope_cache_[this->scope_id_]; + tls_cache = &entry.gauges_; + tls_rejected_stats = &entry.rejected_stats_; } return safeMakeStat( - final_name, central_cache_.gauges_, + final_name, central_cache_.gauges_, central_cache_.rejected_stats_, [](StatDataAllocator& allocator, absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) -> GaugeSharedPtr { return allocator.makeGauge(name, std::move(tag_extracted_name), std::move(tags)); }, - tls_cache); + tls_cache, tls_rejected_stats, null_gauge_); } Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) { + if (parent_.rejectsAll()) { + return null_histogram_; + } + // See comments in counter(). There is no super clean way (via templates or otherwise) to // share this code so I'm leaving it largely duplicated for now. // @@ -369,25 +419,30 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) { // do a find() first, using that if it succeeds. If it fails, then after we // construct the stat we can insert it into the required maps. std::string final_name = prefix_ + name; - if (parent_.rejects(final_name)) { - return null_histogram_; - } StatMap* tls_cache = nullptr; + SharedStringSet* tls_rejected_stats = nullptr; if (!parent_.shutting_down_ && parent_.tls_) { - tls_cache = - &parent_.tls_->getTyped().scope_cache_[this->scope_id_].parent_histograms_; - auto p = tls_cache->find(final_name.c_str()); - if (p != tls_cache->end()) { - return *p->second; + TlsCacheEntry& entry = parent_.tls_->getTyped().scope_cache_[this->scope_id_]; + tls_cache = &entry.parent_histograms_; + auto iter = tls_cache->find(final_name.c_str()); + if (iter != tls_cache->end()) { + return *iter->second; + } + tls_rejected_stats = &entry.rejected_stats_; + if (tls_rejected_stats->find(final_name.c_str()) != tls_rejected_stats->end()) { + return null_histogram_; } } Thread::LockGuard lock(parent_.lock_); - auto p = central_cache_.histograms_.find(final_name.c_str()); + auto iter = central_cache_.histograms_.find(final_name.c_str()); ParentHistogramImplSharedPtr* central_ref = nullptr; - if (p != central_cache_.histograms_.end()) { - central_ref = &p->second; + if (iter != central_cache_.histograms_.end()) { + central_ref = &iter->second; + } else if (parent_.checkAndRememberRejection(final_name, central_cache_.rejected_stats_, + tls_rejected_stats)) { + return null_histogram_; } else { std::vector tags; std::string tag_extracted_name = parent_.getTagsForName(final_name, tags); @@ -405,18 +460,17 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) { Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(const std::string& name, ParentHistogramImpl& parent) { - if (parent_.rejects(name)) { - return null_histogram_; - } + // tlsHistogram() is generally not called for a histogram that is rejected by + // the matcher, so no further rejection-checking is needed at this level. + // TlsHistogram inherits its reject/accept status from ParentHistogram. // See comments in counter() which explains the logic here. - StatMap* tls_cache = nullptr; if (!parent_.shutting_down_ && parent_.tls_) { tls_cache = &parent_.tls_->getTyped().scope_cache_[this->scope_id_].histograms_; - auto p = tls_cache->find(name.c_str()); - if (p != tls_cache->end()) { - return *p->second; + auto iter = tls_cache->find(name.c_str()); + if (iter != tls_cache->end()) { + return *iter->second; } } diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 4f85c763567c2..adbfe7d184a72 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -171,19 +171,28 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo const Stats::StatsOptions& statsOptions() const override { return stats_options_; } private: - template using StatMap = CharStarHashMap; + template using StatMap = ConstCharStarHashMap; struct TlsCacheEntry { StatMap counters_; StatMap gauges_; StatMap histograms_; StatMap parent_histograms_; + + // We keep a TLS cache of rejected stat names. This costs memory, but + // reduces runtime overhead running the matcher. Moreover, once symbol + // tables are integrated, rejection will need the fully elaborated string, + // and it we need to take a global symbol-table lock to run. We keep + // this char* map here in the TLS cache to avoid taking a lock to compute + // rejection. + SharedStringSet rejected_stats_; }; struct CentralCacheEntry { StatMap counters_; StatMap gauges_; StatMap histograms_; + SharedStringSet rejected_stats_; }; struct ScopeImpl : public TlsScope { @@ -221,9 +230,11 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo * used if non-empty, or filled in if empty (and non-null). */ template - StatType& - safeMakeStat(const std::string& name, StatMap>& central_cache_map, - MakeStatFn make_stat, StatMap>* tls_cache); + StatType& safeMakeStat(const std::string& name, + StatMap>& central_cache_map, + SharedStringSet& central_rejected_stats_, MakeStatFn make_stat, + StatMap>* tls_cache, + SharedStringSet* tls_rejected_stats, StatType& null_stat); static std::atomic next_scope_id_; @@ -254,8 +265,11 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void mergeInternal(PostMergeCb mergeCb); absl::string_view truncateStatNameIfNeeded(absl::string_view name); bool rejects(const std::string& name) const; + bool rejectsAll() const { return stats_matcher_->rejectsAll(); } template void removeRejectedStats(StatMapClass& map, StatListClass& list); + bool checkAndRememberRejection(const std::string& name, SharedStringSet& central_rejected_stats, + SharedStringSet* tls_rejected_stats); const Stats::StatsOptions& stats_options_; StatDataAllocator& alloc_; diff --git a/test/common/common/hash_test.cc b/test/common/common/hash_test.cc index e1fcafc5ea971..4112b67d59c56 100644 --- a/test/common/common/hash_test.cc +++ b/test/common/common/hash_test.cc @@ -38,4 +38,12 @@ TEST(Hash, stdhash) { } #endif +TEST(Hash, sharedStringSet) { + SharedStringSet set; + auto foo = std::make_shared("foo"); + set.insert(foo); + auto pos = set.find("foo"); + EXPECT_EQ(pos->get(), foo.get()); +} + } // namespace Envoy diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index dddfea3e6146e..5c7c70be1b74c 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -476,7 +476,6 @@ TEST_F(StatsThreadLocalStoreTest, HotRestartTruncation) { class StatsMatcherTLSTest : public StatsThreadLocalStoreTest { public: - StatsMatcherTLSTest() : StatsThreadLocalStoreTest() {} envoy::config::metrics::v2::StatsConfig stats_config_; }; @@ -622,6 +621,150 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { store_->shutdownThreading(); } +// Tests the logic for caching the stats-matcher results, and in particular the +// private impl method checkAndRememberRejection(). That method behaves +// differently depending on whether TLS is enabled or not, so we parameterize +// the test accordingly; GetParam()==true means we want a TLS cache. In either +// case, we should never be calling the stats-matcher rejection logic more than +// once on given stat name. +class RememberStatsMatcherTest : public testing::TestWithParam { +public: + RememberStatsMatcherTest() : store_(options_, heap_alloc_), scope_(store_.createScope("scope.")) { + if (GetParam()) { + store_.initializeThreading(main_thread_dispatcher_, tls_); + } + } + + ~RememberStatsMatcherTest() override { + store_.shutdownThreading(); + tls_.shutdownThread(); + } + + using LookupStatFn = std::function; + + // Helper function to test the rejection cache. The goal here is to use + // mocks to ensure that we don't call rejects() more than once on any of the + // stats, even with 5 name-based lookups. + void testRememberMatcher(const LookupStatFn lookup_stat) { + InSequence s; + + MockStatsMatcher* matcher = new MockStatsMatcher; + EXPECT_CALL(*matcher, rejects("stats.overflow")).WillRepeatedly(Return(false)); + + StatsMatcherPtr matcher_ptr(matcher); + store_.setStatsMatcher(std::move(matcher_ptr)); + + EXPECT_CALL(*matcher, rejects("scope.reject")).WillOnce(Return(true)); + EXPECT_CALL(*matcher, rejects("scope.ok")).WillOnce(Return(false)); + + for (int j = 0; j < 5; ++j) { + EXPECT_EQ("", lookup_stat("reject")); + EXPECT_EQ("scope.ok", lookup_stat("ok")); + } + } + + void testRejectsAll(const LookupStatFn lookup_stat) { + InSequence s; + + MockStatsMatcher* matcher = new MockStatsMatcher; + EXPECT_CALL(*matcher, rejects("stats.overflow")).WillRepeatedly(Return(false)); + matcher->rejects_all_ = true; + StatsMatcherPtr matcher_ptr(matcher); + store_.setStatsMatcher(std::move(matcher_ptr)); + + ScopePtr scope = store_.createScope("scope."); + + for (int j = 0; j < 5; ++j) { + // Note: zero calls to reject() are made, as reject-all should short-circuit. + EXPECT_EQ("", lookup_stat("reject")); + } + } + + void testAcceptsAll(const LookupStatFn lookup_stat) { + InSequence s; + + MockStatsMatcher* matcher = new MockStatsMatcher; + EXPECT_CALL(*matcher, rejects("stats.overflow")).WillRepeatedly(Return(false)); + matcher->accepts_all_ = true; + StatsMatcherPtr matcher_ptr(matcher); + store_.setStatsMatcher(std::move(matcher_ptr)); + + for (int j = 0; j < 5; ++j) { + // Note: zero calls to reject() are made, as accept-all should short-circuit. + EXPECT_EQ("scope.ok", lookup_stat("ok")); + } + } + + LookupStatFn lookupCounterFn() { + return [this](const std::string& stat_name) -> std::string { + return scope_->counter(stat_name).name(); + }; + } + + LookupStatFn lookupGaugeFn() { + return [this](const std::string& stat_name) -> std::string { + return scope_->gauge(stat_name).name(); + }; + } + +// TODO(jmarantz): restore BoolIndicator tests when https://github.com/envoyproxy/envoy/pull/6280 +// is reverted. +#define HAS_BOOL_INDICATOR 0 +#if HAS_BOOL_INDICATOR + LookupStatFn lookupBoolIndicator() { + return [this](const std::string& stat_name) -> std::string { + return scope_->boolIndicator(stat_name).name(); + }; + } +#endif + + LookupStatFn lookupHistogramFn() { + return [this](const std::string& stat_name) -> std::string { + return scope_->histogram(stat_name).name(); + }; + } + + NiceMock main_thread_dispatcher_; + NiceMock tls_; + StatsOptionsImpl options_; + HeapStatDataAllocator heap_alloc_; + ThreadLocalStoreImpl store_; + ScopePtr scope_; +}; + +INSTANTIATE_TEST_CASE_P(RememberStatsMatcherTest, RememberStatsMatcherTest, + testing::ValuesIn({false, true})); + +// Tests that the logic for remembering rejected stats works properly, both +// with and without threading. +TEST_P(RememberStatsMatcherTest, CounterRejectOne) { testRememberMatcher(lookupCounterFn()); } + +TEST_P(RememberStatsMatcherTest, CounterRejectsAll) { testRejectsAll(lookupCounterFn()); } + +TEST_P(RememberStatsMatcherTest, CounterAcceptsAll) { testAcceptsAll(lookupCounterFn()); } + +TEST_P(RememberStatsMatcherTest, GaugeRejectOne) { testRememberMatcher(lookupGaugeFn()); } + +TEST_P(RememberStatsMatcherTest, GaugeRejectsAll) { testRejectsAll(lookupGaugeFn()); } + +TEST_P(RememberStatsMatcherTest, GaugeAcceptsAll) { testAcceptsAll(lookupGaugeFn()); } + +#if HAS_BOOL_INDICATOR +TEST_P(RememberStatsMatcherTest, BoolIndicatorRejectOne) { + testRememberMatcher(lookupBoolIndicator()); +} + +TEST_P(RememberStatsMatcherTest, BoolIndicatorRejectsAll) { testRejectsAll(lookupBoolIndicator()); } + +TEST_P(RememberStatsMatcherTest, BoolIndicatorAcceptsAll) { testAcceptsAll(lookupBoolIndicator()); } +#endif + +TEST_P(RememberStatsMatcherTest, HistogramRejectOne) { testRememberMatcher(lookupHistogramFn()); } + +TEST_P(RememberStatsMatcherTest, HistogramRejectsAll) { testRejectsAll(lookupHistogramFn()); } + +TEST_P(RememberStatsMatcherTest, HistogramAcceptsAll) { testAcceptsAll(lookupHistogramFn()); } + class HeapStatsThreadLocalStoreTest : public StatsThreadLocalStoreTest { public: void SetUp() override { diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index 5b1dbf98df98c..92dcd6a2ef0e3 100644 --- a/test/mocks/stats/mocks.cc +++ b/test/mocks/stats/mocks.cc @@ -84,5 +84,8 @@ MockStore::~MockStore() {} MockIsolatedStatsStore::MockIsolatedStatsStore() {} MockIsolatedStatsStore::~MockIsolatedStatsStore() {} +MockStatsMatcher::MockStatsMatcher() {} +MockStatsMatcher::~MockStatsMatcher() {} + } // namespace Stats } // namespace Envoy diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index c079e785be16b..21aade4b5b1de 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -9,6 +9,7 @@ #include "envoy/stats/sink.h" #include "envoy/stats/source.h" #include "envoy/stats/stats.h" +#include "envoy/stats/stats_matcher.h" #include "envoy/stats/store.h" #include "envoy/stats/timespan.h" #include "envoy/thread_local/thread_local.h" @@ -180,5 +181,17 @@ class MockIsolatedStatsStore : public IsolatedStoreImpl { MOCK_METHOD2(deliverHistogramToSinks, void(const Histogram& histogram, uint64_t value)); }; +class MockStatsMatcher : public StatsMatcher { +public: + MockStatsMatcher(); + ~MockStatsMatcher(); + MOCK_CONST_METHOD1(rejects, bool(const std::string& name)); + bool acceptsAll() const override { return accepts_all_; } + bool rejectsAll() const override { return rejects_all_; } + + bool accepts_all_{false}; + bool rejects_all_{false}; +}; + } // namespace Stats } // namespace Envoy