stats: improve test-coverage for a few stats-related functions.

include/envoy/stats/scope.h

@@ -111,7 +111,7 @@ class Scope {
   /**
    * @return a reference to the symbol table.
    */
-  virtual const SymbolTable& symbolTable() const PURE;
+  virtual const SymbolTable& constSymbolTable() const PURE;
   virtual SymbolTable& symbolTable() PURE;
 };
 

include/envoy/stats/stat_data_allocator.h

@@ -49,7 +49,7 @@ class StatDataAllocator {
   virtual GaugeSharedPtr makeGauge(StatName name, absl::string_view tag_extracted_name,
                                    const std::vector<Tag>& tags) PURE;
 
-  virtual const SymbolTable& symbolTable() const PURE;
+  virtual const SymbolTable& constSymbolTable() const PURE;
   virtual SymbolTable& symbolTable() PURE;
 
   // TODO(jmarantz): create a parallel mechanism to instantiate histograms. At

include/envoy/stats/stats.h

@@ -105,7 +105,7 @@ class Metric {
     static const uint8_t LogicCached = LogicAccumulate | LogicNeverImport;
   };
   virtual SymbolTable& symbolTable() PURE;
-  virtual const SymbolTable& symbolTable() const PURE;
+  virtual const SymbolTable& constSymbolTable() const PURE;
 };
 
 /**

source/common/stats/histogram_impl.h

@@ -76,7 +76,6 @@ class HistogramImpl : public Histogram, public MetricImpl {
 
   bool used() const override { return true; }
   StatName statName() const override { return name_.statName(); }
-  const SymbolTable& symbolTable() const override { return parent_.symbolTable(); }
   SymbolTable& symbolTable() override { return parent_.symbolTable(); }
 
 private:

source/common/stats/metric_impl.cc

@@ -34,7 +34,7 @@ MetricImpl::MetricImpl(absl::string_view tag_extracted_name, const std::vector<T
 void MetricImpl::clear() { stat_names_.clear(symbolTable()); }
 
 std::string MetricImpl::tagExtractedName() const {
-  return symbolTable().toString(tagExtractedStatName());
+  return constSymbolTable().toString(tagExtractedStatName());
 }
 
 StatName MetricImpl::tagExtractedStatName() const {
@@ -76,7 +76,7 @@ void MetricImpl::iterateTagStatNames(const TagStatNameIterFn& fn) const {
 }
 
 void MetricImpl::iterateTags(const TagIterFn& fn) const {
-  const SymbolTable& symbol_table = symbolTable();
+  const SymbolTable& symbol_table = constSymbolTable();
   iterateTagStatNames([&fn, &symbol_table](StatName name, StatName value) -> bool {
     return fn(Tag{symbol_table.toString(name), symbol_table.toString(value)});
   });

source/common/stats/metric_impl.h

@@ -26,13 +26,26 @@ class MetricImpl : public virtual Metric {
              SymbolTable& symbol_table);
   ~MetricImpl();
 
-  std::string name() const override { return symbolTable().toString(statName()); }
+  std::string name() const override { return constSymbolTable().toString(statName()); }
   std::string tagExtractedName() const override;
   std::vector<Tag> tags() const override;
   StatName tagExtractedStatName() const override;
   void iterateTagStatNames(const TagStatNameIterFn& fn) const override;
   void iterateTags(const TagIterFn& fn) const override;
 
+  // Metric implementations must each implement Metric::symbolTable(). However,
+  // they can inherit the const version of that accessor from MetricImpl.
+  const SymbolTable& constSymbolTable() const override {
+    // Cast our 'this', which is of type `const MetricImpl*` to a non-const
+    // pointer, so we can use it to call the subclass implementation of
+    // symbolTable(). That will be returned as a non-const SymbolTable&,
+    // which will become const on return.
+    //
+    // This pattern is used to share a single non-trivial implementation to
+    // provide const and non-const variants of a method.
+    return const_cast<MetricImpl*>(this)->symbolTable();
+  }
+
 protected:
   void clear();
 
@@ -45,7 +58,6 @@ class NullMetricImpl : public MetricImpl {
   explicit NullMetricImpl(SymbolTable& symbol_table)
       : MetricImpl("", std::vector<Tag>(), symbol_table), stat_name_storage_("", symbol_table) {}
 
-  const SymbolTable& symbolTable() const override { return stat_name_storage_.symbolTable(); }
   SymbolTable& symbolTable() override { return stat_name_storage_.symbolTable(); }
   bool used() const override { return false; }
   StatName statName() const override { return stat_name_storage_.statName(); }

source/common/stats/scope_prefixer.h

@@ -40,7 +40,7 @@ class ScopePrefixer : public Scope {
   absl::optional<std::reference_wrapper<const Histogram>>
   findHistogram(StatName name) const override;
 
-  const SymbolTable& symbolTable() const override { return scope_.symbolTable(); }
+  const SymbolTable& constSymbolTable() const override { return scope_.constSymbolTable(); }
   virtual SymbolTable& symbolTable() override { return scope_.symbolTable(); }
 
   NullGaugeImpl& nullGauge(const std::string& str) override { return scope_.nullGauge(str); }

source/common/stats/stat_data_allocator_impl.h

@@ -37,7 +37,7 @@ template <class StatData> class StatDataAllocatorImpl : public StatDataAllocator
   virtual void free(StatData& data) PURE;
 
   SymbolTable& symbolTable() override { return symbol_table_; }
-  const SymbolTable& symbolTable() const override { return symbol_table_; }
+  const SymbolTable& constSymbolTable() const override { return symbol_table_; }
 
 private:
   // SymbolTable encodes encodes stat names as back into strings. This does not
@@ -81,7 +81,6 @@ template <class StatData> class CounterImpl : public Counter, public MetricImpl
   bool used() const override { return data_.flags_ & Flags::Used; }
   uint64_t value() const override { return data_.value_; }
 
-  const SymbolTable& symbolTable() const override { return alloc_.symbolTable(); }
   SymbolTable& symbolTable() override { return alloc_.symbolTable(); }
 
 protected:
@@ -164,8 +163,6 @@ template <class StatData> class GaugeImpl : public Gauge, public MetricImpl {
     }
   }
 
-private:
-  const SymbolTable& symbolTable() const override { return alloc_.symbolTable(); }
   SymbolTable& symbolTable() override { return alloc_.symbolTable(); }
 
 protected:

source/common/stats/store_impl.h

@@ -15,18 +15,8 @@ class StoreImpl : public Store {
 public:
   explicit StoreImpl(SymbolTable& symbol_table) : symbol_table_(symbol_table) {}
 
-  Counter& counterFromStatName(StatName name) override {
-    return counter(symbol_table_.toString(name));
-  }
-
-  Gauge& gaugeFromStatName(StatName name) override { return gauge(symbol_table_.toString(name)); }
-
-  Histogram& histogramFromStatName(StatName name) override {
-    return histogram(symbol_table_.toString(name));
-  }
-
   SymbolTable& symbolTable() override { return symbol_table_; }
-  const SymbolTable& symbolTable() const override { return symbol_table_; }
+  const SymbolTable& constSymbolTable() const override { return symbol_table_; }
 
 private:
   SymbolTable& symbol_table_;

source/common/stats/symbol_table_impl.h

@@ -401,7 +401,7 @@ class StatNameManagedStorage : public StatNameStorage {
   ~StatNameManagedStorage() { free(symbol_table_); }
 
   SymbolTable& symbolTable() { return symbol_table_; }
-  const SymbolTable& symbolTable() const { return symbol_table_; }
+  const SymbolTable& constSymbolTable() const { return symbol_table_; }
 
 private:
   SymbolTable& symbol_table_;

source/common/stats/thread_local_store.cc

@@ -12,7 +12,6 @@
 #include "envoy/stats/stats.h"
 
 #include "common/common/lock_guard.h"
-#include "common/stats/scope_prefixer.h"
 #include "common/stats/stats_matcher_impl.h"
 #include "common/stats/tag_producer_impl.h"
 
@@ -86,7 +85,8 @@ bool ThreadLocalStoreImpl::rejects(StatName stat_name) const {
   // Also note that the elaboration of the stat-name into a string is expensive,
   // so I think it might be better to move the matcher test until after caching,
   // unless its acceptsAll/rejectsAll.
-  return stats_matcher_->rejectsAll() || stats_matcher_->rejects(symbolTable().toString(stat_name));
+  return stats_matcher_->rejectsAll() ||
+         stats_matcher_->rejects(constSymbolTable().toString(stat_name));
 }
 
 std::vector<CounterSharedPtr> ThreadLocalStoreImpl::counters() const {

source/common/stats/thread_local_store.h

@@ -51,7 +51,6 @@ class ThreadLocalHistogramImpl : public Histogram, public MetricImpl {
   // Stats::Metric
   StatName statName() const override { return name_.statName(); }
   SymbolTable& symbolTable() override { return symbol_table_; }
-  const SymbolTable& symbolTable() const override { return symbol_table_; }
 
 private:
   uint64_t otherHistogramIndex() const { return 1 - current_active_; }
@@ -98,7 +97,6 @@ class ParentHistogramImpl : public ParentHistogram, public MetricImpl {
   // Stats::Metric
   StatName statName() const override { return name_.statName(); }
   SymbolTable& symbolTable() override { return parent_.symbolTable(); }
-  const SymbolTable& symbolTable() const override { return parent_.symbolTable(); }
 
 private:
   bool usedLockHeld() const EXCLUSIVE_LOCKS_REQUIRED(merge_lock_);
@@ -159,7 +157,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
   }
   Histogram& histogram(const std::string& name) override { return default_scope_->histogram(name); }
   NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; }
-  const SymbolTable& symbolTable() const override { return alloc_.symbolTable(); }
+  const SymbolTable& constSymbolTable() const override { return alloc_.constSymbolTable(); }
   SymbolTable& symbolTable() override { return alloc_.symbolTable(); }
   const TagProducer& tagProducer() const { return *tag_producer_; }
   absl::optional<std::reference_wrapper<const Counter>> findCounter(StatName name) const override {
@@ -250,7 +248,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     ScopePtr createScope(const std::string& name) override {
       return parent_.createScope(symbolTable().toString(prefix_.statName()) + "." + name);
     }
-    const SymbolTable& symbolTable() const override { return parent_.symbolTable(); }
+    const SymbolTable& constSymbolTable() const override { return parent_.constSymbolTable(); }
     SymbolTable& symbolTable() override { return parent_.symbolTable(); }
 
     Counter& counter(const std::string& name) override {

test/common/stats/isolated_store_impl_test.cc

@@ -46,6 +46,7 @@ TEST_F(StatsIsolatedStoreImplTest, All) {
   EXPECT_EQ(0, g2.tags().size());
 
   Histogram& h1 = store_.histogram("h1");
+  EXPECT_TRUE(h1.used()); // hardcoded in impl to be true always.
   Histogram& h2 = scope1->histogram("h2");
   scope1->deliverHistogramToSinks(h2, 0);
   EXPECT_EQ("h1", h1.name());
@@ -111,6 +112,14 @@ TEST_F(StatsIsolatedStoreImplTest, AllWithSymbolTable) {
   EXPECT_EQ(2UL, store_.gauges().size());
 }
 
+TEST_F(StatsIsolatedStoreImplTest, ConstSymtabAccessor) {
+  ScopePtr scope = store_.createScope("scope.");
+  const Scope& cscope = *scope;
+  const SymbolTable& const_symbol_table = cscope.constSymbolTable();
+  SymbolTable& symbol_table = scope->symbolTable();
+  EXPECT_EQ(&const_symbol_table, &symbol_table);
+}
+
 TEST_F(StatsIsolatedStoreImplTest, LongStatName) {
   const std::string long_string(128, 'A');
 

test/common/stats/thread_local_store_test.cc

@@ -342,6 +342,14 @@ TEST_F(StatsThreadLocalStoreTest, SanitizePrefix) {
   tls_.shutdownThread();
 }
 
+TEST_F(StatsThreadLocalStoreTest, ConstSymtabAccessor) {
+  ScopePtr scope = store_->createScope("scope.");
+  const Scope& cscope = *scope;
+  const SymbolTable& const_symbol_table = cscope.constSymbolTable();
+  SymbolTable& symbol_table = scope->symbolTable();
+  EXPECT_EQ(&const_symbol_table, &symbol_table);
+}
+
 TEST_F(StatsThreadLocalStoreTest, ScopeDelete) {
   InSequence s;
   store_->initializeThreading(main_thread_dispatcher_, tls_);
@@ -501,6 +509,13 @@ TEST_F(LookupWithStatNameTest, All) {
   EXPECT_EQ(2UL, store_.gauges().size());
 }
 
+TEST_F(LookupWithStatNameTest, NotFound) {
+  StatName not_found(makeStatName("not_found"));
+  EXPECT_FALSE(store_.findCounter(not_found));
+  EXPECT_FALSE(store_.findGauge(not_found));
+  EXPECT_FALSE(store_.findHistogram(not_found));
+}
+
 class StatsMatcherTLSTest : public StatsThreadLocalStoreTest {
 public:
   envoy::config::metrics::v2::StatsConfig stats_config_;
@@ -527,6 +542,7 @@ TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) {
   EXPECT_EQ(noop_counter.value(), 0);
   Counter& noop_counter_2 = store_->counter("noop_counter_2");
   EXPECT_EQ(&noop_counter, &noop_counter_2);
+  EXPECT_FALSE(noop_counter.used()); // hardcoded to return false in NullMetricImpl.
 
   // Gauge
   Gauge& noop_gauge = store_->gauge("noop_gauge");

test/integration/filters/eds_ready_filter.cc

@@ -20,7 +20,7 @@ class EdsReadyFilter : public Http::PassThroughFilter {
   EdsReadyFilter(const Stats::Scope& root_scope)
       : root_scope_(root_scope),
         stat_name_("cluster.cluster_0.membership_healthy",
-                   const_cast<Stats::SymbolTable&>(root_scope_.symbolTable())) {}
+                   const_cast<Stats::SymbolTable&>(root_scope_.constSymbolTable())) {}
   Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap&, bool) override {
     absl::optional<std::reference_wrapper<const Stats::Gauge>> gauge =
         root_scope_.findGauge(stat_name_.statName());

test/integration/server.h

@@ -119,7 +119,9 @@ class TestScopeWrapper : public Scope {
     return wrapped_scope_->findHistogram(name);
   }
 
-  const SymbolTable& symbolTable() const override { return wrapped_scope_->symbolTable(); }
+  const SymbolTable& constSymbolTable() const override {
+    return wrapped_scope_->constSymbolTable();
+  }
   SymbolTable& symbolTable() override { return wrapped_scope_->symbolTable(); }
 
 private:
@@ -177,7 +179,7 @@ class TestIsolatedStoreImpl : public StoreRoot {
     Thread::LockGuard lock(lock_);
     return store_.findHistogram(name);
   }
-  const SymbolTable& symbolTable() const override { return store_.symbolTable(); }
+  const SymbolTable& constSymbolTable() const override { return store_.constSymbolTable(); }
   SymbolTable& symbolTable() override { return store_.symbolTable(); }
 
   // Stats::Store

test/mocks/stats/mocks.h

@@ -51,7 +51,7 @@ class MockMetric : public virtual Metric {
   };
 
   SymbolTable& symbolTable() override { return symbol_table_.get(); }
-  const SymbolTable& symbolTable() const override { return symbol_table_.get(); }
+  const SymbolTable& constSymbolTable() const override { return symbol_table_.get(); }
 
   // Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This
   // creates a deadlock in gmock and is an unintended use of mock functions.
@@ -205,9 +205,6 @@ class MockStore : public SymbolTableProvider, public StoreImpl {
     return histogram(symbol_table_->toString(name));
   }
 
-  SymbolTable& symbolTable() override { return symbol_table_.get(); }
-  const SymbolTable& symbolTable() const override { return symbol_table_.get(); }
-
   Test::Global<FakeSymbolTableImpl> symbol_table_;
   testing::NiceMock<MockCounter> counter_;
   std::vector<std::unique_ptr<MockHistogram>> histograms_;