stats: cache the results of the stats-matcher class.

source/common/stats/thread_local_store.cc

@@ -234,12 +234,38 @@ std::atomic<uint64_t> ThreadLocalStoreImpl::ScopeImpl::next_scope_id_;
 
 ThreadLocalStoreImpl::ScopeImpl::~ScopeImpl() { parent_.releaseScopeCrossThread(this); }
 
+bool ThreadLocalStoreImpl::checkAndRememberRejection(const std::string& name,
+                                                     CharStarHashSet* tls_rejected_stats) {
+  auto reject_iter = rejected_stats_.find(name);
+  if (reject_iter == rejected_stats_.end()) {
+    if (rejects(name)) {
+      auto insertion = rejected_stats_.insert(name);
+      reject_iter = insertion.first;
+    }
+  }
+  if (reject_iter != rejected_stats_.end()) {
+    if (tls_rejected_stats != nullptr) {
+      tls_rejected_stats->insert(reject_iter->c_str());
+    }
+    return true;
+  }
+  return false;
+}
+
 template <class StatType>
 StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
     const std::string& name, StatMap<std::shared_ptr<StatType>>& central_cache_map,
-    MakeStatFn<StatType> make_stat, StatMap<std::shared_ptr<StatType>>* tls_cache) {
+    MakeStatFn<StatType> make_stat, StatMap<std::shared_ptr<StatType>>* tls_cache,
+    CharStarHashSet* 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<std::string> truncation_buffer;
   absl::string_view truncated_name = parent_.truncateStatNameIfNeeded(name);
   if (truncated_name.size() < name.size()) {
@@ -262,6 +288,9 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat(
   std::shared_ptr<StatType>* central_ref = nullptr;
   if (p != central_cache_map.end()) {
     central_ref = &(p->second);
+  } else if (parent_.checkAndRememberRejection(name, 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) {
@@ -302,6 +331,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
@@ -312,15 +345,15 @@ 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<CounterSharedPtr>* tls_cache = nullptr;
+  CharStarHashSet* tls_rejected_stats = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_) {
-    tls_cache = &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].counters_;
+    TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
+    tls_cache = &entry.counters_;
+    tls_rejected_stats = &entry.rejected_stats_;
   }
 
   return safeMakeStat<Counter>(
@@ -329,7 +362,7 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) {
          std::vector<Tag>&& 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,
@@ -349,6 +382,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.
   //
@@ -358,13 +395,13 @@ 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<GaugeSharedPtr>* tls_cache = nullptr;
+  CharStarHashSet* tls_rejected_stats = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_) {
-    tls_cache = &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].gauges_;
+    TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
+    tls_cache = &entry.gauges_;
+    tls_rejected_stats = &entry.rejected_stats_;
   }
 
   return safeMakeStat<Gauge>(
@@ -373,10 +410,14 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) {
          std::vector<Tag>&& tags) -> GaugeSharedPtr {
         return allocator.makeGauge(name, std::move(tag_extracted_name), std::move(tags));
       },
-      tls_cache);
+      tls_cache, tls_rejected_stats, null_gauge_);
 }
 
 BoolIndicator& ThreadLocalStoreImpl::ScopeImpl::boolIndicator(const std::string& name) {
+  if (parent_.rejectsAll()) {
+    return null_bool_;
+  }
+
   // 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.
   //
@@ -386,13 +427,13 @@ BoolIndicator& ThreadLocalStoreImpl::ScopeImpl::boolIndicator(const std::string&
   // 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_bool_;
-  }
 
   StatMap<BoolIndicatorSharedPtr>* tls_cache = nullptr;
+  CharStarHashSet* tls_rejected_stats = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_) {
-    tls_cache = &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].bool_indicators_;
+    TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
+    tls_cache = &entry.bool_indicators_;
+    tls_rejected_stats = &entry.rejected_stats_;
   }
 
   return safeMakeStat<BoolIndicator>(
@@ -401,10 +442,14 @@ BoolIndicator& ThreadLocalStoreImpl::ScopeImpl::boolIndicator(const std::string&
          std::vector<Tag>&& tags) -> BoolIndicatorSharedPtr {
         return allocator.makeBoolIndicator(name, std::move(tag_extracted_name), std::move(tags));
       },
-      tls_cache);
+      tls_cache, tls_rejected_stats, null_bool_);
 }
 
 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.
   //
@@ -414,25 +459,29 @@ 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<ParentHistogramSharedPtr>* tls_cache = nullptr;
+  CharStarHashSet* tls_rejected_stats = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_) {
-    tls_cache =
-        &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].parent_histograms_;
+    TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_];
+    tls_cache = &entry.parent_histograms_;
     auto p = tls_cache->find(final_name.c_str());
     if (p != tls_cache->end()) {
       return *p->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());
   ParentHistogramImplSharedPtr* central_ref = nullptr;
   if (p != central_cache_.histograms_.end()) {
     central_ref = &p->second;
+  } else if (parent_.checkAndRememberRejection(final_name, tls_rejected_stats)) {
+    return null_histogram_;
   } else {
     std::vector<Tag> tags;
     std::string tag_extracted_name = parent_.getTagsForName(final_name, tags);
@@ -450,12 +499,11 @@ 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<TlsHistogramSharedPtr>* tls_cache = nullptr;
   if (!parent_.shutting_down_ && parent_.tls_) {
     tls_cache = &parent_.tls_->getTyped<TlsCache>().scope_cache_[this->scope_id_].histograms_;

source/common/stats/thread_local_store.h

@@ -175,14 +175,18 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   const Stats::StatsOptions& statsOptions() const override { return stats_options_; }
 
 private:
+  friend class ThreadLocalStoreTestScope;
+
   template <class Stat> using StatMap = CharStarHashMap<Stat>;
+  using StringSet = absl::flat_hash_set<std::string>;
 
   struct TlsCacheEntry {
     StatMap<CounterSharedPtr> counters_;
     StatMap<GaugeSharedPtr> gauges_;
     StatMap<BoolIndicatorSharedPtr> bool_indicators_;
     StatMap<TlsHistogramSharedPtr> histograms_;
     StatMap<ParentHistogramSharedPtr> parent_histograms_;
+    CharStarHashSet rejected_stats_; // References set entries from CentralCacheEntry.
   };
 
   struct CentralCacheEntry {
@@ -230,7 +234,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     template <class StatType>
     StatType&
     safeMakeStat(const std::string& name, StatMap<std::shared_ptr<StatType>>& central_cache_map,
-                 MakeStatFn<StatType> make_stat, StatMap<std::shared_ptr<StatType>>* tls_cache);
+                 MakeStatFn<StatType> make_stat, StatMap<std::shared_ptr<StatType>>* tls_cache,
+                 CharStarHashSet* tls_rejected_stats, StatType& null_stat);
 
     static std::atomic<uint64_t> next_scope_id_;
 
@@ -261,9 +266,13 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   void releaseScopeCrossThread(ScopeImpl* scope);
   void mergeInternal(PostMergeCb mergeCb);
   absl::string_view truncateStatNameIfNeeded(absl::string_view name);
+  bool rejectsAll() const { return stats_matcher_->rejectsAll(); }
+  StringSet rejected_stats_ GUARDED_BY(lock_);
   bool rejects(const std::string& name) const;
   template <class StatMapClass, class StatListClass>
   void removeRejectedStats(StatMapClass& map, StatListClass& list);
+  bool checkAndRememberRejection(const std::string& name, CharStarHashSet* tls_rejected_stats)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
   const Stats::StatsOptions& stats_options_;
   StatDataAllocator& alloc_;

test/common/stats/thread_local_store_test.cc

@@ -548,7 +548,34 @@ TEST_F(StatsThreadLocalStoreTest, HotRestartTruncation) {
 
 class StatsMatcherTLSTest : public StatsThreadLocalStoreTest {
 public:
-  StatsMatcherTLSTest() : StatsThreadLocalStoreTest() {}
+  using LookupStatFn = std::function<std::string(Scope&, const std::string&)>;
+
+  // 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, bool is_allocated) {
+    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));
+    if (is_allocated) {
+      EXPECT_CALL(*alloc_, alloc(_)).Times(1);
+      EXPECT_CALL(*alloc_, free(_)).Times(1);
+    }
+    ScopePtr scope = store_->createScope("scope.");
+
+    for (int j = 0; j < 5; ++j) {
+      EXPECT_EQ("", lookup_stat(*scope, "reject"));
+      EXPECT_EQ("scope.ok", lookup_stat(*scope, "ok"));
+    }
+  }
+
   envoy::config::metrics::v2::StatsConfig stats_config_;
 };
 
@@ -625,6 +652,8 @@ TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) {
 TEST_F(StatsMatcherTLSTest, TestExclusionRegex) {
   InSequence s;
 
+  store_->initializeThreading(main_thread_dispatcher_, tls_);
+
   // Expected to alloc lowercase_counter, lowercase_gauge, lowercase_bool,
   //                   valid_counter, valid_gauge, valid_bool
   EXPECT_CALL(*alloc_, alloc(_)).Times(6);
@@ -729,6 +758,89 @@ 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<bool> {
+public:
+  RememberStatsMatcherTest() : store_(options_, heap_alloc_) {
+    if (GetParam()) {
+      store_.initializeThreading(main_thread_dispatcher_, tls_);
+    }
+  }
+
+  ~RememberStatsMatcherTest() override {
+    store_.shutdownThreading();
+    tls_.shutdownThread();
+  }
+
+  using LookupStatFn = std::function<std::string(Scope&, const std::string&)>;
+
+  // 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));
+    ScopePtr scope = store_.createScope("scope.");
+
+    for (int j = 0; j < 5; ++j) {
+      EXPECT_EQ("", lookup_stat(*scope, "reject"));
+      EXPECT_EQ("scope.ok", lookup_stat(*scope, "ok"));
+    }
+  }
+
+  NiceMock<Event::MockDispatcher> main_thread_dispatcher_;
+  NiceMock<ThreadLocal::MockInstance> tls_;
+  StatsOptionsImpl options_;
+  HeapStatDataAllocator heap_alloc_;
+  ThreadLocalStoreImpl store_;
+};
+
+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, Counter) {
+  auto make_counter = [](Scope& scope, const std::string& stat_name) -> std::string {
+    return scope.counter(stat_name).name();
+  };
+  testRememberMatcher(make_counter);
+}
+
+TEST_P(RememberStatsMatcherTest, Gauge) {
+  auto make_gauge = [](Scope& scope, const std::string& stat_name) -> std::string {
+    return scope.gauge(stat_name).name();
+  };
+  testRememberMatcher(make_gauge);
+}
+
+TEST_P(RememberStatsMatcherTest, BoolIndicator) {
+  auto make_bool_indicator = [](Scope& scope, const std::string& stat_name) -> std::string {
+    return scope.boolIndicator(stat_name).name();
+  };
+  testRememberMatcher(make_bool_indicator);
+}
+
+TEST_P(RememberStatsMatcherTest, Histogram) {
+  auto make_histogram = [](Scope& scope, const std::string& stat_name) -> std::string {
+    return scope.histogram(stat_name).name();
+  };
+  testRememberMatcher(make_histogram);
+}
+
 class HeapStatsThreadLocalStoreTest : public StatsThreadLocalStoreTest {
 public:
   void SetUp() override {