stats: prevent unused counters from leaking across hot restart

include/envoy/stats/store.h

@@ -43,6 +43,18 @@ class Store : public Scope {
    * @return a list of all known histograms.
    */
   virtual std::vector<ParentHistogramSharedPtr> histograms() const PURE;
+
+  // TODO(fredlas) can be removed once #6712 is merged.
+  /**
+   * @return whether any known counter exists with this name.
+   */
+  virtual bool counterExists(const std::string& counter_name) PURE;
+
+  // TODO(fredlas) can be removed once #6712 is merged.
+  /**
+   * @return whether any known gauge exists with this name.
+   */
+  virtual bool gaugeExists(const std::string& gauge_name) PURE;
 };
 
 typedef std::unique_ptr<Store> StorePtr;

source/common/stats/isolated_store_impl.h

@@ -49,6 +49,8 @@ template <class Base> class IsolatedStatsCache {
     return vec;
   }
 
+  bool statExists(StatName name) { return stats_.find(name) != stats_.end(); }
+
 private:
   StatNameHashMap<std::shared_ptr<Base>> stats_;
   Allocator alloc_;
@@ -77,6 +79,15 @@ class IsolatedStoreImpl : public StoreImpl {
     return std::vector<ParentHistogramSharedPtr>{};
   }
 
+  bool counterExists(const std::string& counter_name) override {
+    StatNameManagedStorage storage(counter_name, symbolTable());
+    return counters_.statExists(storage.statName());
+  }
+  bool gaugeExists(const std::string& gauge_name) override {
+    StatNameManagedStorage storage(gauge_name, symbolTable());
+    return gauges_.statExists(storage.statName());
+  }
+
   Counter& counter(const std::string& name) override {
     StatNameManagedStorage storage(name, symbolTable());
     return counterFromStatName(storage.statName());

source/common/stats/stat_merger.cc

@@ -5,7 +5,7 @@
 namespace Envoy {
 namespace Stats {
 
-StatMerger::StatMerger(Stats::Store& target_store) : target_store_(target_store) {}
+StatMerger::StatMerger(Stats::Store& target_store) : temp_scope_(target_store.createScope("")) {}
 
 bool StatMerger::shouldImport(Gauge& gauge, const std::string& gauge_name) {
   absl::optional<bool> should_import = gauge.cachedShouldImport();
@@ -56,13 +56,13 @@ bool StatMerger::shouldImport(Gauge& gauge, const std::string& gauge_name) {
 
 void StatMerger::mergeCounters(const Protobuf::Map<std::string, uint64_t>& counter_deltas) {
   for (const auto& counter : counter_deltas) {
-    target_store_.counter(counter.first).add(counter.second);
+    temp_scope_->counter(counter.first).add(counter.second);
   }
 }
 
 void StatMerger::mergeGauges(const Protobuf::Map<std::string, uint64_t>& gauges) {
   for (const auto& gauge : gauges) {
-    auto& gauge_ref = target_store_.gauge(gauge.first);
+    auto& gauge_ref = temp_scope_->gauge(gauge.first);
     uint64_t& parent_value_ref = parent_gauge_values_[gauge_ref.statName()];
     uint64_t old_parent_value = parent_value_ref;
     uint64_t new_parent_value = gauge.second;
@@ -79,12 +79,6 @@ void StatMerger::mergeGauges(const Protobuf::Map<std::string, uint64_t>& gauges)
   }
 }
 
-// TODO(fredlas) the current implementation can "leak" obsolete parent stats into the child.
-// That is, the parent had stat "foo", the child doesn't care about "foo" and back in the
-// shared memory implementation would have dropped it, but the import causes it to be made into
-// a real stat that stays around forever. The initial mini-consensus approach will be to
-// track which stats are actually getting used by the child, and drop those that aren't when
-// the hot restart completes.
 void StatMerger::mergeStats(const Protobuf::Map<std::string, uint64_t>& counter_deltas,
                             const Protobuf::Map<std::string, uint64_t>& gauges) {
   mergeCounters(counter_deltas);

source/common/stats/stat_merger.h

@@ -36,7 +36,25 @@ class StatMerger {
   void mergeCounters(const Protobuf::Map<std::string, uint64_t>& counter_deltas);
   void mergeGauges(const Protobuf::Map<std::string, uint64_t>& gauges);
   StatNameHashMap<uint64_t> parent_gauge_values_;
-  Stats::Store& target_store_;
+  // A stats Scope for our in-the-merging-process counters to live in. Scopes conceptually hold
+  // shared_ptrs to the stats that live in them, with the question of which stats are living in a
+  // given scope determined by which stat names have been accessed via that scope. E.g., if you
+  // access a stat named "some.shared" directly through the ordinary store, and then access a
+  // stat named "shared" in a scope configured with the prefix "some.", there is now a single
+  // stat named some.shared pointed to by both. As another example, if you access the stat
+  // "single" in the "some" scope, there will be a stat named "some.single" pointed to by just
+  // that scope. Now, if you delete the scope, some.shared will stick around, but some.single
+  // will be destroyed.
+  //
+  // All of that is relevant here because it is used to get a certain desired behavior.
+  // Specifically, stats must be kept up to date with values from the parent throughout hot
+  // restart, but once the restart completes, they must be dropped without a trace if the child has
+  // not taken action (independent of the hot restart stat merging) that would lead to them getting
+  // created in the store. By storing these stats in a scope (with an empty prefix), we can
+  // preserve all stats throughout the hot restart. Then, when the restart completes, dropping
+  // the scope will drop exactly those stats whose names have not already been accessed through
+  // another store/scope.
+  ScopePtr temp_scope_;
 };
 
 } // namespace Stats

source/common/stats/thread_local_store.cc

@@ -146,6 +146,30 @@ std::vector<ParentHistogramSharedPtr> ThreadLocalStoreImpl::histograms() const {
   return ret;
 }
 
+bool ThreadLocalStoreImpl::counterExists(const std::string& counter_name) {
+  Thread::LockGuard lock(lock_);
+  for (ScopeImpl* scope : scopes_) {
+    if (scope->central_cache_.counters_.find(
+            StatNameManagedStorage(counter_name, symbolTable()).statName()) !=
+        scope->central_cache_.counters_.end()) {
+      return true;
+    }
+  }
+  return false;
+}
+
+bool ThreadLocalStoreImpl::gaugeExists(const std::string& gauge_name) {
+  Thread::LockGuard lock(lock_);
+  for (ScopeImpl* scope : scopes_) {
+    if (scope->central_cache_.gauges_.find(
+            StatNameManagedStorage(gauge_name, symbolTable()).statName()) !=
+        scope->central_cache_.gauges_.end()) {
+      return true;
+    }
+  }
+  return false;
+}
+
 void ThreadLocalStoreImpl::initializeThreading(Event::Dispatcher& main_thread_dispatcher,
                                                ThreadLocal::Instance& tls) {
   main_thread_dispatcher_ = &main_thread_dispatcher;

source/common/stats/thread_local_store.h

@@ -168,6 +168,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   std::vector<CounterSharedPtr> counters() const override;
   std::vector<GaugeSharedPtr> gauges() const override;
   std::vector<ParentHistogramSharedPtr> histograms() const override;
+  bool counterExists(const std::string& counter_name) override;
+  bool gaugeExists(const std::string& gauge_name) override;
 
   // Stats::StoreRoot
   void addSink(Sink& sink) override { timer_sinks_.push_back(sink); }

source/server/hot_restarting_child.cc

@@ -82,6 +82,10 @@ void HotRestartingChild::sendParentTerminateRequest() {
   parent_terminated_ = true;
   // Once setting parent_terminated_ == true, we can send no more hot restart RPCs, and therefore
   // receive no more responses, including stats. So, now safe to forget our stat transferral state.
+  //
+  // This destruction is actually important far beyond memory efficiency. The scope-based temporary
+  // counter logic relies on the StatMerger getting destroyed once hot restart's stat merging is
+  // all done. (See stat_merger.h for details).
   stat_merger_.reset();
 }
 

test/common/stats/BUILD

@@ -51,8 +51,12 @@ envoy_cc_test(
     name = "stat_merger_test",
     srcs = ["stat_merger_test.cc"],
     deps = [
+        "//source/common/stats:fake_symbol_table_lib",
         "//source/common/stats:isolated_store_lib",
         "//source/common/stats:stat_merger_lib",
+        "//source/common/stats:thread_local_store_lib",
+        "//test/mocks/event:event_mocks",
+        "//test/mocks/thread_local:thread_local_mocks",
     ],
 )
 

test/common/stats/stat_merger_test.cc

@@ -1,10 +1,17 @@
 #include <memory>
 
+#include "common/stats/fake_symbol_table_impl.h"
 #include "common/stats/isolated_store_impl.h"
 #include "common/stats/stat_merger.h"
+#include "common/stats/thread_local_store.h"
+
+#include "test/mocks/event/mocks.h"
+#include "test/mocks/thread_local/mocks.h"
 
 #include "gtest/gtest.h"
 
+using testing::NiceMock;
+
 namespace Envoy {
 namespace Stats {
 namespace {
@@ -52,23 +59,6 @@ TEST_F(StatMergerTest, counterMerge) {
   EXPECT_EQ(4, store_.counter("draculaer").latch());
 }
 
-// It should be fine for the parent to send us stats we haven't ourselves instantiated.
-// TODO(6756) This is how things currently work, but this is actually what 6756 is looking to avoid.
-TEST_F(StatMergerTest, newStatFromParent) {
-  Protobuf::Map<std::string, uint64_t> counter_values;
-  Protobuf::Map<std::string, uint64_t> counter_deltas;
-  Protobuf::Map<std::string, uint64_t> gauges;
-  counter_deltas["newcounter0"] = 0;
-  counter_deltas["newcounter1"] = 1;
-  gauges["newgauge"] = 5;
-  stat_merger_.mergeStats(counter_deltas, gauges);
-  EXPECT_EQ(0, store_.counter("newcounter0").value());
-  EXPECT_EQ(0, store_.counter("newcounter0").latch());
-  EXPECT_EQ(1, store_.counter("newcounter1").value());
-  EXPECT_EQ(1, store_.counter("newcounter1").latch());
-  EXPECT_EQ(5, store_.gauge("newgauge").value());
-}
-
 TEST_F(StatMergerTest, basicDefaultAccumulationImport) {
   Protobuf::Map<std::string, uint64_t> gauges;
   gauges["whywassixafraidofseven"] = 111;
@@ -164,6 +154,46 @@ TEST_F(StatMergerTest, exclusionsNotImported) {
   EXPECT_FALSE(store_.gauge("overload.something.pressure").used());
 }
 
+// When the parent sends us counters we haven't ourselves instantiated, they should be stored
+// temporarily, but then uninstantiated if hot restart ends without the child accessing them.
+TEST(StatMergerNonFixtureTest, newStatFromParent) {
+  FakeSymbolTableImpl symbol_table;
+  HeapStatDataAllocator alloc(symbol_table);
+  ThreadLocalStoreImpl store(alloc);
+  {
+    StatMerger stat_merger(store);
+
+    Protobuf::Map<std::string, uint64_t> counter_values;
+    Protobuf::Map<std::string, uint64_t> counter_deltas;
+    Protobuf::Map<std::string, uint64_t> gauges;
+    counter_deltas["newcounter0"] = 0;
+    counter_deltas["newcounter1"] = 1;
+    counter_deltas["newcounter2"] = 2;
+    gauges["newgauge1"] = 1;
+    gauges["newgauge2"] = 2;
+    stat_merger.mergeStats(counter_deltas, gauges);
+    EXPECT_EQ(0, store.counter("newcounter0").value());
+    EXPECT_EQ(0, store.counter("newcounter0").latch());
+    EXPECT_EQ(1, store.counter("newcounter1").value());
+    EXPECT_EQ(1, store.counter("newcounter1").latch());
+    EXPECT_EQ(1, store.gauge("newgauge1").value());
+  }
+  // We accessed 0 and 1 above, but not 2. Now that StatMerger has been destroyed,
+  // 2 should be gone.
+  // TODO(fredlas) replace counterExists with Scope::getCounter once #6712 is merged.
+  EXPECT_TRUE(store.counterExists("newcounter0"));
+  EXPECT_TRUE(store.counterExists("newcounter1"));
+  EXPECT_FALSE(store.counterExists("newcounter2"));
+  EXPECT_TRUE(store.gaugeExists("newgauge1"));
+  EXPECT_FALSE(store.gaugeExists("newgauge2"));
+  // HACK: doesn't like shutting down without threading having started.
+  NiceMock<Event::MockDispatcher> main_thread_dispatcher;
+  NiceMock<ThreadLocal::MockInstance> tls;
+  store.initializeThreading(main_thread_dispatcher, tls);
+  store.shutdownThreading();
+  tls.shutdownThread();
+}
+
 } // namespace
 } // namespace Stats
 } // namespace Envoy

test/integration/server.h

@@ -170,6 +170,13 @@ class TestIsolatedStoreImpl : public StoreRoot {
     return store_.histograms();
   }
 
+  bool counterExists(const std::string& counter_name) override {
+    return store_.counterExists(counter_name);
+  }
+  bool gaugeExists(const std::string& gauge_name) override {
+    return store_.gaugeExists(gauge_name);
+  }
+
   // Stats::StoreRoot
   void addSink(Sink&) override {}
   void setTagProducer(TagProducerPtr&&) override {}

test/mocks/stats/mocks.h

@@ -188,6 +188,8 @@ class MockStore : public SymbolTableProvider, public StoreImpl {
   MOCK_CONST_METHOD0(gauges, std::vector<GaugeSharedPtr>());
   MOCK_METHOD1(histogram, Histogram&(const std::string& name));
   MOCK_CONST_METHOD0(histograms, std::vector<ParentHistogramSharedPtr>());
+  MOCK_METHOD1(counterExists, bool(const std::string& counter_name));
+  MOCK_METHOD1(gaugeExists, bool(const std::string& gauge_name));
 
   Counter& counterFromStatName(StatName name) override {
     return counter(symbol_table_->toString(name));