stats: Shared scopes phase 1, changes scope APIs to use ScopeSharedPt…

…r, leaving ScopePtr alias behind (#19791)

Commit Message: To support an algorithmic solution to a long burst of CPU on admin /stats with a large # clusters, we need to hold scopes in shared pointers. This PR makes that change and updates the usage within stats to reference scopes as ScopeSharedPtr rather than ScopePtr, and adds enable_shared_from_this as a Scope super-class so existing APIs that pass Scope& don't need to change.

This leaves behind a ScopePtr alias to ScopeSharedPtr temporarily to (a) make this PR be reviewable and (b) avoid breaking other repos that may reference ScopePtr. It will be easier to submit this PR first, then a external repos can rename their references, and we can do a follow-up PR that removes ScopePtr to avoid longer term confusion. That PR would like like #19790 which has 90 changed files in addition to the semantic change. Once this is merged, that PR will just be a strict rename and will be a lot easier to review.

This PR blocks #19693 which implements the admin change requiring holding onto a shared scope ptr.

Additional Description:
Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Joshua Marantz <[email protected]>

envoy/stats/scope.h

@@ -24,19 +24,48 @@ using CounterOptConstRef = absl::optional<std::reference_wrapper<const Counter>>
 using GaugeOptConstRef = absl::optional<std::reference_wrapper<const Gauge>>;
 using HistogramOptConstRef = absl::optional<std::reference_wrapper<const Histogram>>;
 using TextReadoutOptConstRef = absl::optional<std::reference_wrapper<const TextReadout>>;
-using ScopePtr = std::unique_ptr<Scope>;
+using ConstScopeSharedPtr = std::shared_ptr<const Scope>;
 using ScopeSharedPtr = std::shared_ptr<Scope>;
 
+// TODO(jmarantz): In the initial transformation to store Scope as shared_ptr,
+// we don't change all the references, as that would result in an unreviewable
+// PR that combines a small number of semantic changes and a large number of
+// files with trivial changes. Furthermore, code that depends on the Envoy stats
+// infrastructure that's not in this repo will stop compiling when we remove
+// ScopePtr. We should fully remove this alias in a future PR and change all the
+// references, once known consumers that might break from this change have a
+// chance to do the global replace in their own repos.
+using ScopePtr = ScopeSharedPtr;
+
 template <class StatType> using IterateFn = std::function<bool(const RefcountPtr<StatType>&)>;
 
 /**
  * A named scope for stats. Scopes are a grouping of stats that can be acted on as a unit if needed
  * (for example to free/delete all of them).
+ *
+ * We enable use of shared pointers for Scopes to make it possible for the admin
+ * stats handler to safely capture all the scope references and remain robust to
+ * other threads deleting those scopes while rendering an admin stats page.
+ *
+ * We use std::shared_ptr rather than Stats::RefcountPtr, which we use for other
+ * stats, because:
+ *  * existing uses of shared_ptr<Scope> exist in the Wasm extension and would need
+ *    to be rewritten to allow for RefcountPtr<Scope>.
+ *  * the main advantage of RefcountPtr is it's smaller per instance by (IIRC) 16
+ *    bytes, but there are not typically enough scopes that the extra per-scope
+ *    overhead would matter.
+ *  * It's a little less coding to use enable_shared_from_this compared to adding
+ *    a ref_count to the scope object, for each of its implementations.
  */
-class Scope {
+class Scope : public std::enable_shared_from_this<Scope> {
 public:
   virtual ~Scope() = default;
 
+  /** @return a shared_ptr for this */
+  ScopeSharedPtr getShared() { return shared_from_this(); }
+  /** @return a const shared_ptr for this */
+  ConstScopeSharedPtr getConstShared() const { return shared_from_this(); }
+
   /**
    * Allocate a new scope. NOTE: The implementation should correctly handle overlapping scopes
    * that point to the same reference counted backing stats. This allows a new scope to be
@@ -46,7 +75,7 @@ class Scope {
    *
    * @param name supplies the scope's namespace prefix.
    */
-  virtual ScopePtr createScope(const std::string& name) PURE;
+  virtual ScopeSharedPtr createScope(const std::string& name) PURE;
 
   /**
    * Allocate a new scope. NOTE: The implementation should correctly handle overlapping scopes
@@ -55,7 +84,7 @@ class Scope {
    *
    * @param name supplies the scope's namespace prefix.
    */
-  virtual ScopePtr scopeFromStatName(StatName name) PURE;
+  virtual ScopeSharedPtr scopeFromStatName(StatName name) PURE;
 
   /**
    * Deliver an individual histogram value to all registered sinks.

source/common/stats/isolated_store_impl.cc

@@ -34,14 +34,20 @@ IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table)
         return alloc_.makeTextReadout(name, name, StatNameTagVector{});
       }),
       null_counter_(new NullCounterImpl(symbol_table)),
-      null_gauge_(new NullGaugeImpl(symbol_table)) {}
+      null_gauge_(new NullGaugeImpl(symbol_table)),
+      default_scope_(std::make_shared<ScopePrefixer>("", *this)) {}
 
-ScopePtr IsolatedStoreImpl::createScope(const std::string& name) {
-  return std::make_unique<ScopePrefixer>(name, *this);
+IsolatedStoreImpl::~IsolatedStoreImpl() = default;
+
+ScopeSharedPtr IsolatedStoreImpl::createScope(const std::string& name) {
+  StatNameManagedStorage stat_name_storage(Utility::sanitizeStatsName(name), alloc_.symbolTable());
+  return scopeFromStatName(stat_name_storage.statName());
 }
 
-ScopePtr IsolatedStoreImpl::scopeFromStatName(StatName name) {
-  return std::make_unique<ScopePrefixer>(name, *this);
+ScopeSharedPtr IsolatedStoreImpl::scopeFromStatName(StatName name) {
+  ScopeSharedPtr scope = std::make_shared<ScopePrefixer>(name, *this);
+  scopes_.push_back(scope);
+  return scope;
 }
 
 } // namespace Stats

source/common/stats/isolated_store_impl.h

@@ -132,6 +132,7 @@ class IsolatedStoreImpl : public StoreImpl {
 public:
   IsolatedStoreImpl();
   explicit IsolatedStoreImpl(SymbolTable& symbol_table);
+  ~IsolatedStoreImpl() override;
 
   // Stats::Scope
   Counter& counterFromStatNameWithTags(const StatName& name,
@@ -140,8 +141,8 @@ class IsolatedStoreImpl : public StoreImpl {
     Counter& counter = counters_.get(joiner.nameWithTags());
     return counter;
   }
-  ScopePtr createScope(const std::string& name) override;
-  ScopePtr scopeFromStatName(StatName name) override;
+  ScopeSharedPtr createScope(const std::string& name) override;
+  ScopeSharedPtr scopeFromStatName(StatName name) override;
   void deliverHistogramToSinks(const Histogram&, uint64_t) override {}
   Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef tags,
                                    Gauge::ImportMode import_mode) override {
@@ -234,10 +235,12 @@ class IsolatedStoreImpl : public StoreImpl {
 
   void forEachScope(SizeFn f_size, StatFn<const Scope> f_stat) const override {
     if (f_size != nullptr) {
-      f_size(1);
+      f_size(scopes_.size() + 1);
+    }
+    f_stat(*default_scope_);
+    for (const ScopeSharedPtr& scope : scopes_) {
+      f_stat(*scope);
     }
-    const Scope& scope = *this;
-    f_stat(scope);
   }
 
   Stats::StatName prefix() const override { return StatName(); }
@@ -265,6 +268,8 @@ class IsolatedStoreImpl : public StoreImpl {
   IsolatedStatsCache<TextReadout> text_readouts_;
   RefcountPtr<NullCounterImpl> null_counter_;
   RefcountPtr<NullGaugeImpl> null_gauge_;
+  ScopeSharedPtr default_scope_;
+  std::vector<ScopeSharedPtr> scopes_;
 };
 
 } // namespace Stats

source/common/stats/scope_prefixer.cc

@@ -16,12 +16,12 @@ ScopePrefixer::ScopePrefixer(StatName prefix, Scope& scope)
 
 ScopePrefixer::~ScopePrefixer() { prefix_.free(symbolTable()); }
 
-ScopePtr ScopePrefixer::scopeFromStatName(StatName name) {
+ScopeSharedPtr ScopePrefixer::scopeFromStatName(StatName name) {
   SymbolTable::StoragePtr joined = symbolTable().join({prefix_.statName(), name});
-  return std::make_unique<ScopePrefixer>(StatName(joined.get()), scope_);
+  return std::make_shared<ScopePrefixer>(StatName(joined.get()), scope_);
 }
 
-ScopePtr ScopePrefixer::createScope(const std::string& name) {
+ScopeSharedPtr ScopePrefixer::createScope(const std::string& name) {
   StatNameManagedStorage stat_name_storage(Utility::sanitizeStatsName(name), symbolTable());
   return scopeFromStatName(stat_name_storage.statName());
 }

source/common/stats/scope_prefixer.h

@@ -16,8 +16,8 @@ class ScopePrefixer : public Scope {
   ~ScopePrefixer() override;
 
   // Scope
-  ScopePtr createScope(const std::string& name) override;
-  ScopePtr scopeFromStatName(StatName name) override;
+  ScopeSharedPtr createScope(const std::string& name) override;
+  ScopeSharedPtr scopeFromStatName(StatName name) override;
   Counter& counterFromStatNameWithTags(const StatName& name,
                                        StatNameTagVectorOptConstRef tags) override;
   Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef tags,

source/common/stats/stat_merger.h

@@ -96,7 +96,7 @@ class StatMerger {
   // 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_;
+  ScopeSharedPtr temp_scope_;
 };
 
 } // namespace Stats

source/common/stats/thread_local_store.cc

@@ -144,12 +144,12 @@ std::vector<CounterSharedPtr> ThreadLocalStoreImpl::counters() const {
   return ret;
 }
 
-ScopePtr ThreadLocalStoreImpl::createScope(const std::string& name) {
+ScopeSharedPtr ThreadLocalStoreImpl::createScope(const std::string& name) {
   StatNameManagedStorage stat_name_storage(Utility::sanitizeStatsName(name), alloc_.symbolTable());
   return scopeFromStatName(stat_name_storage.statName());
 }
 
-ScopePtr ThreadLocalStoreImpl::scopeFromStatName(StatName name) {
+ScopeSharedPtr ThreadLocalStoreImpl::scopeFromStatName(StatName name) {
   auto new_scope = std::make_unique<ScopeImpl>(*this, name);
   Thread::LockGuard lock(lock_);
   scopes_.emplace(new_scope.get());
@@ -975,9 +975,8 @@ void ThreadLocalStoreImpl::forEachScope(std::function<void(std::size_t)> f_size,
                                         StatFn<const Scope> f_scope) const {
   Thread::LockGuard lock(lock_);
   if (f_size != nullptr) {
-    f_size(scopes_.size() + 1 /* for default_scope_ */);
+    f_size(scopes_.size());
   }
-  f_scope(*default_scope_);
   for (ScopeImpl* scope : scopes_) {
     f_scope(*scope);
   }

source/common/stats/thread_local_store.h

@@ -158,8 +158,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   Counter& counterFromString(const std::string& name) override {
     return default_scope_->counterFromString(name);
   }
-  ScopePtr createScope(const std::string& name) override;
-  ScopePtr scopeFromStatName(StatName name) override;
+  ScopeSharedPtr createScope(const std::string& name) override;
+  ScopeSharedPtr scopeFromStatName(StatName name) override;
   void deliverHistogramToSinks(const Histogram& histogram, uint64_t value) override {
     return default_scope_->deliverHistogramToSinks(histogram, value);
   }
@@ -348,10 +348,10 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
                                              Histogram::Unit unit) override;
     TextReadout& textReadoutFromStatNameWithTags(const StatName& name,
                                                  StatNameTagVectorOptConstRef tags) override;
-    ScopePtr createScope(const std::string& name) override {
+    ScopeSharedPtr createScope(const std::string& name) override {
       return parent_.createScope(symbolTable().toString(prefix_.statName()) + "." + name);
     }
-    ScopePtr scopeFromStatName(StatName name) override {
+    ScopeSharedPtr scopeFromStatName(StatName name) override {
       SymbolTable::StoragePtr joined = symbolTable().join({prefix_.statName(), name});
       return parent_.scopeFromStatName(StatName(joined.get()));
     }
@@ -520,7 +520,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   ThreadLocal::TypedSlotPtr<TlsCache> tls_cache_;
   mutable Thread::MutexBasicLockable lock_;
   absl::flat_hash_set<ScopeImpl*> scopes_ ABSL_GUARDED_BY(lock_);
-  ScopePtr default_scope_;
+  ScopeSharedPtr default_scope_;
   std::list<std::reference_wrapper<Sink>> timer_sinks_;
   TagProducerPtr tag_producer_;
   StatsMatcherPtr stats_matcher_;

source/common/stats/utility.cc

@@ -68,7 +68,7 @@ struct ElementVisitor {
 
 namespace Utility {
 
-ScopePtr scopeFromStatNames(Scope& scope, const StatNameVec& elements) {
+ScopeSharedPtr scopeFromStatNames(Scope& scope, const StatNameVec& elements) {
   SymbolTable::StoragePtr joined = scope.symbolTable().join(elements);
   return scope.scopeFromStatName(StatName(joined.get()));
 }

source/common/stats/utility.h

@@ -92,7 +92,7 @@ absl::optional<StatName> findTag(const Metric& metric, StatName find_tag_name);
  * @param elements The vector of mixed DynamicName and StatName
  * @return A scope named using the joined elements.
  */
-ScopePtr scopeFromStatNames(Scope& scope, const StatNameVec& names);
+ScopeSharedPtr scopeFromStatNames(Scope& scope, const StatNameVec& names);
 
 /**
  * Creates a counter from a vector of tokens which are used to create the

test/common/stats/isolated_store_impl_test.cc

@@ -31,7 +31,7 @@ class StatsIsolatedStoreImplTest : public testing::Test {
 };
 
 TEST_F(StatsIsolatedStoreImplTest, All) {
-  ScopePtr scope1 = store_->createScope("scope1.");
+  ScopeSharedPtr scope1 = store_->createScope("scope1.");
   Counter& c1 = store_->counterFromString("c1");
   Counter& c2 = scope1->counterFromString("c2");
   EXPECT_EQ("c1", c1.name());
@@ -108,11 +108,11 @@ TEST_F(StatsIsolatedStoreImplTest, All) {
   ASSERT_TRUE(found_histogram.has_value());
   EXPECT_EQ(&h1, &found_histogram->get());
 
-  ScopePtr scope2 = scope1->scopeFromStatName(makeStatName("foo."));
+  ScopeSharedPtr scope2 = scope1->scopeFromStatName(makeStatName("foo."));
   EXPECT_EQ("scope1.foo.bar", scope2->counterFromString("bar").name());
 
   // Validate that we sanitize away bad characters in the stats prefix.
-  ScopePtr scope3 = scope1->createScope(std::string("foo:\0:.", 7));
+  ScopeSharedPtr scope3 = scope1->createScope(std::string("foo:\0:.", 7));
   EXPECT_EQ("scope1.foo___.bar", scope3->counterFromString("bar").name());
 
   EXPECT_EQ(4UL, store_->counters().size());
@@ -125,14 +125,14 @@ TEST_F(StatsIsolatedStoreImplTest, All) {
 }
 
 TEST_F(StatsIsolatedStoreImplTest, PrefixIsStatName) {
-  ScopePtr scope1 = store_->createScope("scope1");
-  ScopePtr scope2 = scope1->scopeFromStatName(makeStatName("scope2"));
+  ScopeSharedPtr scope1 = store_->createScope("scope1");
+  ScopeSharedPtr scope2 = scope1->scopeFromStatName(makeStatName("scope2"));
   Counter& c1 = scope2->counterFromString("c1");
   EXPECT_EQ("scope1.scope2.c1", c1.name());
 }
 
 TEST_F(StatsIsolatedStoreImplTest, AllWithSymbolTable) {
-  ScopePtr scope1 = store_->createScope("scope1.");
+  ScopeSharedPtr scope1 = store_->createScope("scope1.");
   Counter& c1 = store_->counterFromStatName(makeStatName("c1"));
   Counter& c2 = scope1->counterFromStatName(makeStatName("c2"));
   EXPECT_EQ("c1", c1.name());
@@ -174,11 +174,11 @@ TEST_F(StatsIsolatedStoreImplTest, AllWithSymbolTable) {
   h1.recordValue(200);
   h2.recordValue(200);
 
-  ScopePtr scope2 = scope1->createScope("foo.");
+  ScopeSharedPtr scope2 = scope1->createScope("foo.");
   EXPECT_EQ("scope1.foo.bar", scope2->counterFromStatName(makeStatName("bar")).name());
 
   // Validate that we sanitize away bad characters in the stats prefix.
-  ScopePtr scope3 = scope1->createScope(std::string("foo:\0:.", 7));
+  ScopeSharedPtr scope3 = scope1->createScope(std::string("foo:\0:.", 7));
   EXPECT_EQ("scope1.foo___.bar", scope3->counterFromString("bar").name());
 
   EXPECT_EQ(4UL, store_->counters().size());
@@ -187,7 +187,7 @@ TEST_F(StatsIsolatedStoreImplTest, AllWithSymbolTable) {
 }
 
 TEST_F(StatsIsolatedStoreImplTest, ConstSymtabAccessor) {
-  ScopePtr scope = store_->createScope("scope.");
+  ScopeSharedPtr scope = store_->createScope("scope.");
   const Scope& cscope = *scope;
   const SymbolTable& const_symbol_table = cscope.constSymbolTable();
   SymbolTable& symbol_table = scope->symbolTable();
@@ -197,7 +197,7 @@ TEST_F(StatsIsolatedStoreImplTest, ConstSymtabAccessor) {
 TEST_F(StatsIsolatedStoreImplTest, LongStatName) {
   const std::string long_string(128, 'A');
 
-  ScopePtr scope = store_->createScope("scope.");
+  ScopeSharedPtr scope = store_->createScope("scope.");
   Counter& counter = scope->counterFromString(long_string);
   EXPECT_EQ(absl::StrCat("scope.", long_string), counter.name());
 }
@@ -250,8 +250,8 @@ TEST_F(StatsIsolatedStoreImplTest, StatNamesStruct) {
   MAKE_STAT_NAMES_STRUCT(StatNames, ALL_TEST_STATS);
   StatNames stat_names(store_->symbolTable());
   EXPECT_EQ("prefix", store_->symbolTable().toString(stat_names.prefix_));
-  ScopePtr scope1 = store_->createScope("scope1.");
-  ScopePtr scope2 = store_->createScope("scope2.");
+  ScopeSharedPtr scope1 = store_->createScope("scope1.");
+  ScopeSharedPtr scope2 = store_->createScope("scope2.");
   MAKE_STATS_STRUCT(Stats, StatNames, ALL_TEST_STATS);
   Stats stats1(stat_names, *scope1);
   EXPECT_EQ("scope1.test_counter", stats1.test_counter_.name());
@@ -265,5 +265,26 @@ TEST_F(StatsIsolatedStoreImplTest, StatNamesStruct) {
   EXPECT_EQ("scope2.prefix.test_text_readout", stats2.test_text_readout_.name());
 }
 
+TEST_F(StatsIsolatedStoreImplTest, SharedScopes) {
+  std::vector<ConstScopeSharedPtr> scopes;
+
+  // Verifies shared_ptr functionality by creating some scopes, iterating
+  // through them from the store and saving them in a vector, dropping the
+  // references, and then referencing the scopes, verifying their names.
+  {
+    ScopeSharedPtr scope1 = store_->createScope("scope1.");
+    ScopeSharedPtr scope2 = store_->createScope("scope2.");
+    store_->forEachScope(
+        [](size_t) {}, [&scopes](const Scope& scope) { scopes.push_back(scope.getConstShared()); });
+  }
+  ASSERT_EQ(3, scopes.size());
+  store_->symbolTable().sortByStatNames<ConstScopeSharedPtr>(
+      scopes.begin(), scopes.end(),
+      [](const ConstScopeSharedPtr& scope) -> StatName { return scope->prefix(); });
+  EXPECT_EQ("", store_->symbolTable().toString(scopes[0]->prefix())); // default scope
+  EXPECT_EQ("scope1", store_->symbolTable().toString(scopes[1]->prefix()));
+  EXPECT_EQ("scope2", store_->symbolTable().toString(scopes[2]->prefix()));
+}
+
 } // namespace Stats
 } // namespace Envoy