stats: Shared scopes phase 1, changes scope APIs to use ScopeSharedPtr, leaving ScopePtr alias behind

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 parthers that might break from this change of 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 makeShared() { return shared_from_this(); }
+  /** @return a const shared_ptr for this */
+  ConstScopeSharedPtr makeConstShared() 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,27 @@ 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.makeConstShared()); });
+  }
+  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