stats: cleanup APIs and avoid extra conversions between StatName and string.

docs/root/faq/configuration/sni.rst

@@ -70,9 +70,9 @@ The following is a YAML example of the above requirement.
 How do I configure SNI for clusters?
 ====================================
 
-For clusters, a fixed SNI can be set in `UpstreamTlsContext <envoy_api_field_auth.UpstreamTlsContext.sni>`_.
+For clusters, a fixed SNI can be set in :ref:`UpstreamTlsContext <envoy_api_field_auth.UpstreamTlsContext.sni>`.
 To derive SNI from HTTP `host` or `:authority` header, turn on
-`auto_sni <envoy_api_field_core.UpstreamHttpProtocolOptions.auto_sni>`_ to override the fixed SNI in
+:ref:`auto_sni <envoy_api_field_core.UpstreamHttpProtocolOptions.auto_sni>` to override the fixed SNI in
 `UpstreamTlsContext`. If upstream will present certificates with the hostname in SAN, turn on
-`auto_san_validation <envoy_api_field_core.UpstreamHttpProtocolOptions.auto_san_validation>`_ too.
+:ref:`auto_san_validation <envoy_api_field_core.UpstreamHttpProtocolOptions.auto_san_validation>` too.
 It still needs a trust CA in validation context in `UpstreamTlsContext` for trust anchor.

include/envoy/stats/allocator.h

@@ -35,7 +35,7 @@ class Allocator {
    * @return CounterSharedPtr a counter, or nullptr if allocation failed, in which case
    *     tag_extracted_name and tags are not moved.
    */
-  virtual CounterSharedPtr makeCounter(StatName name, absl::string_view tag_extracted_name,
+  virtual CounterSharedPtr makeCounter(StatName name, StatName tag_extracted_name,
                                        const StatNameTagVector& stat_name_tags) PURE;
 
   /**
@@ -45,7 +45,7 @@ class Allocator {
    * @return GaugeSharedPtr a gauge, or nullptr if allocation failed, in which case
    *     tag_extracted_name and tags are not moved.
    */
-  virtual GaugeSharedPtr makeGauge(StatName name, absl::string_view tag_extracted_name,
+  virtual GaugeSharedPtr makeGauge(StatName name, StatName tag_extracted_name,
                                    const StatNameTagVector& stat_name_tags,
                                    Gauge::ImportMode import_mode) PURE;
 

source/common/stats/allocator_impl.cc

@@ -49,7 +49,7 @@ void AllocatorImpl::debugPrint() {
 // wasted in the alignment padding next to flags_.
 template <class BaseClass> class StatsSharedImpl : public MetricImpl<BaseClass> {
 public:
-  StatsSharedImpl(StatName name, AllocatorImpl& alloc, absl::string_view tag_extracted_name,
+  StatsSharedImpl(StatName name, AllocatorImpl& alloc, StatName tag_extracted_name,
                   const StatNameTagVector& stat_name_tags)
       : MetricImpl<BaseClass>(name, tag_extracted_name, stat_name_tags, alloc.symbolTable()),
         alloc_(alloc) {}
@@ -121,7 +121,7 @@ template <class BaseClass> class StatsSharedImpl : public MetricImpl<BaseClass>
 
 class CounterImpl : public StatsSharedImpl<Counter> {
 public:
-  CounterImpl(StatName name, AllocatorImpl& alloc, absl::string_view tag_extracted_name,
+  CounterImpl(StatName name, AllocatorImpl& alloc, StatName tag_extracted_name,
               const StatNameTagVector& stat_name_tags)
       : StatsSharedImpl(name, alloc, tag_extracted_name, stat_name_tags) {}
 
@@ -149,7 +149,7 @@ class CounterImpl : public StatsSharedImpl<Counter> {
 
 class GaugeImpl : public StatsSharedImpl<Gauge> {
 public:
-  GaugeImpl(StatName name, AllocatorImpl& alloc, absl::string_view tag_extracted_name,
+  GaugeImpl(StatName name, AllocatorImpl& alloc, StatName tag_extracted_name,
             const StatNameTagVector& stat_name_tags, ImportMode import_mode)
       : StatsSharedImpl(name, alloc, tag_extracted_name, stat_name_tags) {
     switch (import_mode) {
@@ -228,7 +228,7 @@ class GaugeImpl : public StatsSharedImpl<Gauge> {
   }
 };
 
-CounterSharedPtr AllocatorImpl::makeCounter(StatName name, absl::string_view tag_extracted_name,
+CounterSharedPtr AllocatorImpl::makeCounter(StatName name, StatName tag_extracted_name,
                                             const StatNameTagVector& stat_name_tags) {
   Thread::LockGuard lock(mutex_);
   ASSERT(gauges_.find(name) == gauges_.end());
@@ -241,7 +241,7 @@ CounterSharedPtr AllocatorImpl::makeCounter(StatName name, absl::string_view tag
   return counter;
 }
 
-GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, absl::string_view tag_extracted_name,
+GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, StatName tag_extracted_name,
                                         const StatNameTagVector& stat_name_tags,
                                         Gauge::ImportMode import_mode) {
   Thread::LockGuard lock(mutex_);

source/common/stats/allocator_impl.h

@@ -23,9 +23,9 @@ class AllocatorImpl : public Allocator {
   ~AllocatorImpl() override;
 
   // Allocator
-  CounterSharedPtr makeCounter(StatName name, absl::string_view tag_extracted_name,
+  CounterSharedPtr makeCounter(StatName name, StatName tag_extracted_name,
                                const StatNameTagVector& stat_name_tags) override;
-  GaugeSharedPtr makeGauge(StatName name, absl::string_view tag_extracted_name,
+  GaugeSharedPtr makeGauge(StatName name, StatName tag_extracted_name,
                            const StatNameTagVector& stat_name_tags,
                            Gauge::ImportMode import_mode) override;
   SymbolTable& symbolTable() override { return symbol_table_; }

source/common/stats/fake_symbol_table_impl.h

@@ -73,8 +73,7 @@ class FakeSymbolTableImpl : public SymbolTable {
     MemBlockBuilder<uint8_t> mem_block(total_size_bytes);
     mem_block.appendOne(num_names);
     for (uint32_t i = 0; i < num_names; ++i) {
-      const StatName name = names[i];
-      mem_block.appendData(absl::MakeSpan(name.sizeAndData(), name.size()));
+      SymbolTableImpl::Encoding::appendToMemBlock(names[i], mem_block);
     }
 
     // This assertion double-checks the arithmetic where we computed

source/common/stats/histogram_impl.h

@@ -49,7 +49,7 @@ class HistogramStatisticsImpl : public HistogramStatistics, NonCopyable {
 
 class HistogramImplHelper : public MetricImpl<Histogram> {
 public:
-  HistogramImplHelper(StatName name, const std::string& tag_extracted_name,
+  HistogramImplHelper(StatName name, StatName tag_extracted_name,
                       const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table)
       : MetricImpl<Histogram>(name, tag_extracted_name, stat_name_tags, symbol_table) {}
   HistogramImplHelper(SymbolTable& symbol_table) : MetricImpl<Histogram>(symbol_table) {}
@@ -68,7 +68,7 @@ class HistogramImplHelper : public MetricImpl<Histogram> {
  */
 class HistogramImpl : public HistogramImplHelper {
 public:
-  HistogramImpl(StatName name, Unit unit, Store& parent, const std::string& tag_extracted_name,
+  HistogramImpl(StatName name, Unit unit, Store& parent, StatName tag_extracted_name,
                 const StatNameTagVector& stat_name_tags)
       : HistogramImplHelper(name, tag_extracted_name, stat_name_tags, parent.symbolTable()),
         unit_(unit), parent_(parent) {}

source/common/stats/isolated_store_impl.cc

@@ -24,14 +24,13 @@ IsolatedStoreImpl::IsolatedStoreImpl(std::unique_ptr<SymbolTable>&& symbol_table
 IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table)
     : StoreImpl(symbol_table), alloc_(symbol_table),
       counters_([this](StatName name) -> CounterSharedPtr {
-        return alloc_.makeCounter(name, toString(name), StatNameTagVector{});
+        return alloc_.makeCounter(name, name, StatNameTagVector{});
       }),
       gauges_([this](StatName name, Gauge::ImportMode import_mode) -> GaugeSharedPtr {
-        return alloc_.makeGauge(name, toString(name), StatNameTagVector{}, import_mode);
+        return alloc_.makeGauge(name, name, StatNameTagVector{}, import_mode);
       }),
       histograms_([this](StatName name, Histogram::Unit unit) -> HistogramSharedPtr {
-        return HistogramSharedPtr(
-            new HistogramImpl(name, unit, *this, toString(name), StatNameTagVector{}));
+        return HistogramSharedPtr(new HistogramImpl(name, unit, *this, name, StatNameTagVector{}));
       }),
       null_counter_(new NullCounterImpl(symbol_table)),
       null_gauge_(new NullGaugeImpl(symbol_table)) {}

source/common/stats/isolated_store_impl.h

@@ -160,8 +160,6 @@ class IsolatedStoreImpl : public StoreImpl {
 private:
   IsolatedStoreImpl(std::unique_ptr<SymbolTable>&& symbol_table);
 
-  std::string toString(StatName stat_name) { return alloc_.symbolTable().toString(stat_name); }
-
   SymbolTablePtr symbol_table_storage_;
   AllocatorImpl alloc_;
   IsolatedStatsCache<Counter> counters_;

source/common/stats/metric_impl.cc

@@ -14,7 +14,7 @@ MetricHelper::~MetricHelper() {
   ASSERT(!stat_names_.populated());
 }
 
-MetricHelper::MetricHelper(StatName name, absl::string_view tag_extracted_name,
+MetricHelper::MetricHelper(StatName name, StatName tag_extracted_name,
                            const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table) {
   // Encode all the names and tags into transient storage so we can count the
   // required bytes. 2 is added to account for the name and tag_extracted_name,
@@ -23,8 +23,7 @@ MetricHelper::MetricHelper(StatName name, absl::string_view tag_extracted_name,
   const uint32_t num_names = 2 + 2 * stat_name_tags.size();
   absl::FixedArray<StatName> names(num_names);
   names[0] = name;
-  StatNameManagedStorage storage(tag_extracted_name, symbol_table);
-  names[1] = storage.statName();
+  names[1] = tag_extracted_name;
   int index = 1;
   for (auto& stat_name_tag : stat_name_tags) {
     names[++index] = stat_name_tag.first;

source/common/stats/metric_impl.h

@@ -21,8 +21,8 @@ namespace Stats {
  */
 class MetricHelper {
 public:
-  MetricHelper(StatName name, absl::string_view tag_extracted_name,
-               const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table);
+  MetricHelper(StatName name, StatName tag_extracted_name, const StatNameTagVector& stat_name_tags,
+               SymbolTable& symbol_table);
   ~MetricHelper();
 
   StatName statName() const;
@@ -55,15 +55,14 @@ class MetricHelper {
  */
 template <class BaseClass> class MetricImpl : public BaseClass {
 public:
-  // TODO(jmarantz): Use StatName for tag_extracted_name.
-  MetricImpl(StatName name, absl::string_view tag_extracted_name,
-             const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table)
+  MetricImpl(StatName name, StatName tag_extracted_name, const StatNameTagVector& stat_name_tags,
+             SymbolTable& symbol_table)
       : helper_(name, tag_extracted_name, stat_name_tags, symbol_table) {}
 
   // Empty construction of a MetricImpl; used for null stats.
   explicit MetricImpl(SymbolTable& symbol_table)
-      : MetricImpl(StatNameManagedStorage("", symbol_table).statName(), "", StatNameTagVector{},
-                   symbol_table) {}
+      : MetricImpl(StatName(), StatName(), StatNameTagVector(), symbol_table) {}
+
   TagVector tags() const override { return helper_.tags(constSymbolTable()); }
   StatName statName() const override { return helper_.statName(); }
   StatName tagExtractedStatName() const override { return helper_.tagExtractedStatName(); }

source/common/stats/symbol_table_impl.cc

@@ -173,6 +173,16 @@ void SymbolTableImpl::Encoding::moveToMemBlock(MemBlockBuilder<uint8_t>& mem_blo
   mem_block_.reset(); // Logically transfer ownership, enabling empty assert on destruct.
 }
 
+void SymbolTableImpl::Encoding::appendToMemBlock(StatName stat_name,
+                                                 MemBlockBuilder<uint8_t>& mem_block) {
+  const uint8_t* data = stat_name.dataIncludingSize();
+  if (data == nullptr) {
+    mem_block.appendOne(0);
+  } else {
+    mem_block.appendData(absl::MakeSpan(data, stat_name.size()));
+  }
+}
+
 SymbolTableImpl::SymbolTableImpl()
     // Have to be explicitly initialized, if we want to use the GUARDED_BY macro.
     : next_symbol_(FirstValidSymbol), monotonic_counter_(FirstValidSymbol) {}
@@ -516,7 +526,9 @@ void StatNameStorageSet::free(SymbolTable& symbol_table) {
 SymbolTable::StoragePtr SymbolTableImpl::join(const StatNameVec& stat_names) const {
   uint64_t num_bytes = 0;
   for (StatName stat_name : stat_names) {
-    num_bytes += stat_name.dataSize();
+    if (!stat_name.empty()) {
+      num_bytes += stat_name.dataSize();
+    }
   }
   MemBlockBuilder<uint8_t> mem_block(Encoding::totalSizeBytes(num_bytes));
   Encoding::appendEncoding(num_bytes, mem_block);
@@ -543,8 +555,8 @@ void SymbolTableImpl::populateList(const StatName* names, uint32_t num_names, St
   mem_block.appendOne(num_names);
   for (uint32_t i = 0; i < num_names; ++i) {
     const StatName stat_name = names[i];
+    Encoding::appendToMemBlock(stat_name, mem_block);
     incRefCount(stat_name);
-    mem_block.appendData(absl::MakeSpan(stat_name.sizeAndData(), stat_name.size()));
   }
 
   // This assertion double-checks the arithmetic where we computed

source/common/stats/symbol_table_impl.h

@@ -157,6 +157,15 @@ class SymbolTableImpl : public SymbolTable {
      */
     static void appendEncoding(uint64_t number, MemBlockBuilder<uint8_t>& mem_block);
 
+    /**
+     * Appends stat_name's bytes into mem_block, which must have been allocated to
+     * allow for stat_name.size() bytes.
+     *
+     * @param stat_name the stat_name to append.
+     * @param mem_block the block of memory to append to.
+     */
+    static void appendToMemBlock(StatName stat_name, MemBlockBuilder<uint8_t>& mem_block);
+
     /**
      * Decodes a byte-array containing a variable-length number.
      *
@@ -452,6 +461,8 @@ class StatName {
     return size_and_data_ + SymbolTableImpl::Encoding::encodingSizeBytes(dataSize());
   }
 
+  const uint8_t* dataIncludingSize() const { return size_and_data_; }
+
   /**
    * @return A pointer to the buffer, including the size bytes.
    */

source/common/stats/thread_local_store.cc

@@ -280,23 +280,23 @@ class StatNameTagHelper {
     if (!stat_name_tags) {
       TagVector tags;
       tls.symbolTable().callWithStringView(name, [&tags, &tls, this](absl::string_view name_str) {
-        tag_extracted_name_ = tls.tagProducer().produceTags(name_str, tags);
+        tag_extracted_name_ = pool_.add(tls.tagProducer().produceTags(name_str, tags));
       });
       for (const auto& tag : tags) {
         stat_name_tags_.emplace_back(pool_.add(tag.name_), pool_.add(tag.value_));
       }
     } else {
-      tag_extracted_name_ = tls.symbolTable().toString(name);
+      tag_extracted_name_ = name;
     }
   }
 
   const StatNameTagVector& statNameTags() const { return stat_name_tags_; }
-  const std::string& tagExtractedName() { return tag_extracted_name_; }
+  StatName tagExtractedName() const { return tag_extracted_name_; }
 
 private:
   StatNamePool pool_;
   StatNameTagVector stat_name_tags_;
-  std::string tag_extracted_name_;
+  StatName tag_extracted_name_;
 };
 
 bool ThreadLocalStoreImpl::checkAndRememberRejection(StatName name,
@@ -420,9 +420,9 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatNameWithTags(
   return safeMakeStat<Counter>(
       final_stat_name, joiner.tagExtractedName(), stat_name_tags, central_cache_->counters_,
       central_cache_->rejected_stats_,
-      [](Allocator& allocator, StatName name, absl::string_view final_name,
+      [](Allocator& allocator, StatName name, StatName tag_extracted_name,
          const StatNameTagVector& tags) -> CounterSharedPtr {
-        return allocator.makeCounter(name, final_name, tags);
+        return allocator.makeCounter(name, tag_extracted_name, tags);
       },
       tls_cache, tls_rejected_stats, parent_.null_counter_);
 }
@@ -471,7 +471,7 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatNameWithTags(
   Gauge& gauge = safeMakeStat<Gauge>(
       final_stat_name, joiner.tagExtractedName(), stat_name_tags, central_cache_->gauges_,
       central_cache_->rejected_stats_,
-      [import_mode](Allocator& allocator, StatName name, absl::string_view tag_extracted_name,
+      [import_mode](Allocator& allocator, StatName name, StatName tag_extracted_name,
                     const StatNameTagVector& tags) -> GaugeSharedPtr {
         return allocator.makeGauge(name, tag_extracted_name, tags, import_mode);
       },
@@ -587,7 +587,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(StatName name,
 }
 
 ThreadLocalHistogramImpl::ThreadLocalHistogramImpl(StatName name, Histogram::Unit unit,
-                                                   const std::string& tag_extracted_name,
+                                                   StatName tag_extracted_name,
                                                    const StatNameTagVector& stat_name_tags,
                                                    SymbolTable& symbol_table)
     : HistogramImplHelper(name, tag_extracted_name, stat_name_tags, symbol_table), unit_(unit),
@@ -616,7 +616,7 @@ void ThreadLocalHistogramImpl::merge(histogram_t* target) {
 }
 
 ParentHistogramImpl::ParentHistogramImpl(StatName name, Histogram::Unit unit, Store& parent,
-                                         TlsScope& tls_scope, absl::string_view tag_extracted_name,
+                                         TlsScope& tls_scope, StatName tag_extracted_name,
                                          const StatNameTagVector& stat_name_tags)
     : MetricImpl(name, tag_extracted_name, stat_name_tags, parent.symbolTable()), unit_(unit),
       parent_(parent), tls_scope_(tls_scope), interval_histogram_(hist_alloc()),

source/common/stats/thread_local_store.h

@@ -32,8 +32,7 @@ namespace Stats {
  */
 class ThreadLocalHistogramImpl : public HistogramImplHelper {
 public:
-  ThreadLocalHistogramImpl(StatName name, Histogram::Unit unit,
-                           const std::string& tag_extracted_name,
+  ThreadLocalHistogramImpl(StatName name, Histogram::Unit unit, StatName tag_extracted_name,
                            const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table);
   ~ThreadLocalHistogramImpl() override;
 
@@ -81,8 +80,7 @@ class TlsScope;
 class ParentHistogramImpl : public MetricImpl<ParentHistogram> {
 public:
   ParentHistogramImpl(StatName name, Histogram::Unit unit, Store& parent, TlsScope& tls_scope,
-                      absl::string_view tag_extracted_name,
-                      const StatNameTagVector& stat_name_tags);
+                      StatName tag_extracted_name, const StatNameTagVector& stat_name_tags);
   ~ParentHistogramImpl() override;
 
   void addTlsHistogram(const TlsHistogramSharedPtr& hist_ptr);
@@ -320,9 +318,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo
     HistogramOptConstRef findHistogram(StatName name) const override;
 
     template <class StatType>
-    using MakeStatFn = std::function<RefcountPtr<StatType>(Allocator&, StatName name,
-                                                           absl::string_view tag_extracted_name,
-                                                           const StatNameTagVector& tags)>;
+    using MakeStatFn = std::function<RefcountPtr<StatType>(
+        Allocator&, StatName name, StatName tag_extracted_name, const StatNameTagVector& tags)>;
 
     /**
      * Makes a stat either by looking it up in the central cache,

test/common/stats/BUILD

@@ -27,6 +27,7 @@ envoy_cc_test(
     srcs = ["isolated_store_impl_test.cc"],
     deps = [
         "//source/common/stats:isolated_store_lib",
+        "//source/common/stats:symbol_table_creator_lib",
     ],
 )