stats: Filter out rejected stats from scope caches when the stat-matcher is applied.

source/common/stats/thread_local_store.cc

@@ -34,6 +34,44 @@ ThreadLocalStoreImpl::~ThreadLocalStoreImpl() {
   ASSERT(scopes_.empty());
 }
 
+void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) {
+  stats_matcher_ = std::move(stats_matcher);
+
+  // The Filesystem and potentially other stat-registering objects are
+  // constructed prior to the stat-matcher, and those add stats
+  // in the default_scope. There should be no requests, so there will
+  // be no copies in TLS caches.
+  Thread::LockGuard lock(lock_);
+  for (ScopeImpl* scope : scopes_) {
+    removeRejectedStats(scope->central_cache_.counters_, deleted_counters_);
+    removeRejectedStats(scope->central_cache_.gauges_, deleted_gauges_);
+    removeRejectedStats(scope->central_cache_.histograms_, deleted_histograms_);
+  }
+}
+
+template <class StatMapClass, class StatListClass>
+void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass& list) {
+  std::vector<const char*> remove_list;
+  for (auto& stat : map) {
+    if (rejects(stat.first)) {
+      remove_list.push_back(stat.first);
+    }
+  }
+  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);
+  }
+}
+
+bool ThreadLocalStoreImpl::rejects(const std::string& name) const {
+  // TODO(ambuc): If stats_matcher_ depends on regexes, this operation (on the
+  // hot path) could become prohibitively expensive. Revisit this usage in the
+  // future.
+  return stats_matcher_->rejects(name);
+}
+
 std::vector<CounterSharedPtr> ThreadLocalStoreImpl::counters() const {
   // Handle de-dup due to overlapping scopes.
   std::vector<CounterSharedPtr> ret;
@@ -257,10 +295,7 @@ 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;
-
-  // TODO(ambuc): If stats_matcher_ depends on regexes, this operation (on the hot path) could
-  // become prohibitively expensive. Revisit this usage in the future.
-  if (parent_.stats_matcher_->rejects(final_name)) {
+  if (parent_.rejects(final_name)) {
     return null_counter_;
   }
 
@@ -306,9 +341,7 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) {
   // do a find() first, using tha 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;
-
-  // See warning/comments in counter().
-  if (parent_.stats_matcher_->rejects(final_name)) {
+  if (parent_.rejects(final_name)) {
     return null_gauge_;
   }
 
@@ -336,9 +369,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) {
   // do a find() first, using tha 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;
-
-  // See warning/comments in counter().
-  if (parent_.stats_matcher_->rejects(final_name)) {
+  if (parent_.rejects(final_name)) {
     return null_histogram_;
   }
 
@@ -374,7 +405,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) {
 
 Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(const std::string& name,
                                                          ParentHistogramImpl& parent) {
-  if (parent_.stats_matcher_->rejects(name)) {
+  if (parent_.rejects(name)) {
     return null_histogram_;
   }
 

source/common/stats/thread_local_store.h

@@ -158,9 +158,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   void setTagProducer(TagProducerPtr&& tag_producer) override {
     tag_producer_ = std::move(tag_producer);
   }
-  void setStatsMatcher(StatsMatcherPtr&& stats_matcher) override {
-    stats_matcher_ = std::move(stats_matcher);
-  }
+  void setStatsMatcher(StatsMatcherPtr&& stats_matcher) override;
   void initializeThreading(Event::Dispatcher& main_thread_dispatcher,
                            ThreadLocal::Instance& tls) override;
   void shutdownThreading() override;
@@ -254,6 +252,9 @@ 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 rejects(const std::string& name) const;
+  template <class StatMapClass, class StatListClass>
+  void removeRejectedStats(StatMapClass& map, StatListClass& list);
 
   const Stats::StatsOptions& stats_options_;
   StatDataAllocator& alloc_;
@@ -270,6 +271,18 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   Counter& num_last_resort_stats_;
   HeapStatDataAllocator heap_allocator_;
   SourceImpl source_;
+
+  // Retain storage for deleted stats; these are no longer in maps because the
+  // matcher-pattern was established after they were created. Since the stats
+  // are held by reference in code that expects them to be there, we can't
+  // actually delete the stats.
+  //
+  // It seems like it would be better to have each client that expects a stat
+  // to exist to hold it as (e.g.) a CounterSharedPtr rather than a Counter&
+  // but that would be fairly complex to change.
+  std::vector<CounterSharedPtr> deleted_counters_;
+  std::vector<GaugeSharedPtr> deleted_gauges_;
+  std::vector<HistogramSharedPtr> deleted_histograms_;
 };
 
 } // namespace Stats

test/common/stats/thread_local_store_test.cc

@@ -630,6 +630,36 @@ class HeapStatsThreadLocalStoreTest : public StatsThreadLocalStoreTest {
   HeapStatDataAllocator heap_alloc_;
 };
 
+TEST_F(HeapStatsThreadLocalStoreTest, RemoveRejectedStats) {
+  Counter& counter = store_->counter("c1");
+  Gauge& gauge = store_->gauge("g1");
+  Histogram& histogram = store_->histogram("h1");
+  ASSERT_EQ(2, store_->counters().size()); // "stats.overflow" and "c1".
+  EXPECT_TRUE(&counter == store_->counters()[0].get() ||
+              &counter == store_->counters()[1].get()); // counters() order is non-deterministic.
+  ASSERT_EQ(1, store_->gauges().size());
+  EXPECT_EQ("g1", store_->gauges()[0]->name());
+  ASSERT_EQ(1, store_->histograms().size());
+  EXPECT_EQ("h1", store_->histograms()[0]->name());
+
+  // Will effectively block all stats, and remove all the non-matching stats.
+  envoy::config::metrics::v2::StatsConfig stats_config;
+  stats_config.mutable_stats_matcher()->mutable_inclusion_list()->add_patterns()->set_exact(
+      "no-such-stat");
+  store_->setStatsMatcher(std::make_unique<StatsMatcherImpl>(stats_config));
+
+  // They can no longer be found.
+  EXPECT_EQ(0, store_->counters().size());
+  EXPECT_EQ(0, store_->gauges().size());
+  EXPECT_EQ(0, store_->histograms().size());
+
+  // However, referencing the previously allocated stats will not crash.
+  counter.inc();
+  gauge.inc();
+  EXPECT_CALL(sink_, onHistogramComplete(Ref(histogram), 42));
+  histogram.recordValue(42);
+}
+
 TEST_F(HeapStatsThreadLocalStoreTest, NonHotRestartNoTruncation) {
   InSequence s;
   store_->initializeThreading(main_thread_dispatcher_, tls_);

test/integration/integration_admin_test.cc

@@ -519,8 +519,7 @@ TEST_P(StatsMatcherIntegrationTest, IncludeExact) {
       "listener_manager.listener_create_success");
   initialize();
   makeRequest();
-  EXPECT_THAT(response_->body(),
-              testing::Eq("listener_manager.listener_create_success: 1\nstats.overflow: 0\n"));
+  EXPECT_EQ(response_->body(), "listener_manager.listener_create_success: 1\n");
 }
 
 } // namespace Envoy