[deprecated] stats: do not canonicalize StatNames when storing in allocator, using a special syntax in tests for identifying dynamic stat names.

include/envoy/stats/symbol_table.h

@@ -120,8 +120,7 @@ class SymbolTable {
    * @param num_names The number of names.
    * @param symbol_table The symbol table in which to encode the names.
    */
-  virtual void populateList(const absl::string_view* names, uint32_t num_names,
-                            StatNameList& list) PURE;
+  virtual void populateList(const StatName* names, uint32_t num_names, StatNameList& list) PURE;
 
 #ifndef ENVOY_CONFIG_COVERAGE
   virtual void debugPrint() const PURE;

source/common/stats/allocator_impl.cc

@@ -51,7 +51,10 @@ template <class BaseClass> class StatsSharedImpl : public MetricImpl<BaseClass>
 public:
   StatsSharedImpl(StatName name, AllocatorImpl& alloc, absl::string_view tag_extracted_name,
                   const std::vector<Tag>& tags)
-      : MetricImpl<BaseClass>(name, tag_extracted_name, tags, alloc.symbolTable()), alloc_(alloc) {}
+      : MetricImpl<BaseClass>(
+            name, StatNameManagedStorage(tag_extracted_name, alloc.symbolTable()).statName(), tags,
+            alloc.symbolTable()),
+        alloc_(alloc) {}
 
   ~StatsSharedImpl() override {
     // MetricImpl must be explicitly cleared() before destruction, otherwise it

source/common/stats/fake_symbol_table_impl.h

@@ -50,8 +50,7 @@ namespace Stats {
 class FakeSymbolTableImpl : public SymbolTable {
 public:
   // SymbolTable
-  void populateList(const absl::string_view* names, uint32_t num_names,
-                    StatNameList& list) override {
+  void populateList(const StatName* names, uint32_t num_names, StatNameList& list) override {
     // This implementation of populateList is similar to
     // SymbolTableImpl::populateList. This variant is more efficient for
     // FakeSymbolTableImpl, because it avoid "encoding" each name in names. The
@@ -66,21 +65,16 @@ class FakeSymbolTableImpl : public SymbolTable {
 
     size_t total_size_bytes = 1; /* one byte for holding the number of names */
     for (uint32_t i = 0; i < num_names; ++i) {
-      size_t name_size = names[i].size();
-      total_size_bytes += SymbolTableImpl::Encoding::totalSizeBytes(name_size);
+      total_size_bytes += names[i].size();
     }
 
     // Now allocate the exact number of bytes required and move the encodings
     // into storage.
     MemBlockBuilder<uint8_t> mem_block(total_size_bytes);
     mem_block.appendOne(num_names);
     for (uint32_t i = 0; i < num_names; ++i) {
-      absl::string_view name = names[i];
-      const size_t sz = name.size();
-      SymbolTableImpl::Encoding::appendEncoding(sz, mem_block);
-      if (!name.empty()) {
-        mem_block.appendData(absl::MakeSpan(reinterpret_cast<const uint8_t*>(name.data()), sz));
-      }
+      const StatName name = names[i];
+      mem_block.appendData(absl::MakeSpan(name.dataIncludingSize(), name.size()));
     }
 
     // This assertion double-checks the arithmetic where we computed

source/common/stats/histogram_impl.h

@@ -51,7 +51,9 @@ class HistogramImplHelper : public MetricImpl<Histogram> {
 public:
   HistogramImplHelper(StatName name, const std::string& tag_extracted_name,
                       const std::vector<Tag>& tags, SymbolTable& symbol_table)
-      : MetricImpl<Histogram>(name, tag_extracted_name, tags, symbol_table) {}
+      : MetricImpl<Histogram>(name,
+                              StatNameManagedStorage(tag_extracted_name, symbol_table).statName(),
+                              tags, symbol_table) {}
   HistogramImplHelper(SymbolTable& symbol_table) : MetricImpl<Histogram>(symbol_table) {}
 
   // RefcountInterface

source/common/stats/metric_impl.cc

@@ -14,20 +14,21 @@ MetricHelper::~MetricHelper() {
   ASSERT(!stat_names_.populated());
 }
 
-MetricHelper::MetricHelper(absl::string_view name, absl::string_view tag_extracted_name,
-                           const std::vector<Tag>& tags, SymbolTable& symbol_table) {
+MetricHelper::MetricHelper(StatName name, StatName tag_extracted_name, const std::vector<Tag>& 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,
   // and we multiply the number of tags by 2 to account for the name and value
   // of each tag.
   const uint32_t num_names = 2 + 2 * tags.size();
-  STACK_ARRAY(names, absl::string_view, num_names);
+  STACK_ARRAY(names, StatName, num_names);
   names[0] = name;
   names[1] = tag_extracted_name;
   int index = 1;
+  StatNamePool pool(symbol_table);
   for (auto& tag : tags) {
-    names[++index] = tag.name_;
-    names[++index] = tag.value_;
+    names[++index] = pool.add(tag.name_);
+    names[++index] = pool.add(tag.value_);
   }
   symbol_table.populateList(names.begin(), num_names, stat_names_);
 }

source/common/stats/metric_impl.h

@@ -21,8 +21,8 @@ namespace Stats {
  */
 class MetricHelper {
 public:
-  MetricHelper(absl::string_view name, absl::string_view tag_extracted_name,
-               const std::vector<Tag>& tags, SymbolTable& symbol_table);
+  MetricHelper(StatName name, StatName tag_extracted_name, const std::vector<Tag>& tags,
+               SymbolTable& symbol_table);
   ~MetricHelper();
 
   StatName statName() const;
@@ -57,15 +57,17 @@ template <class BaseClass> class MetricImpl : public BaseClass {
 public:
   MetricImpl(absl::string_view name, absl::string_view tag_extracted_name,
              const std::vector<Tag>& tags, SymbolTable& symbol_table)
-      : helper_(name, tag_extracted_name, tags, symbol_table) {}
+      : MetricImpl(StatNameManagedStorage(name, symbol_table).statName(),
+                   StatNameManagedStorage(tag_extracted_name, symbol_table).statName(), tags,
+                   symbol_table) {}
 
   // Alternate API to take the name as a StatName, which is needed at most call-sites.
   // TODO(jmarantz): refactor impl to either be able to pass string_view at call-sites
   // always, or to make it more efficient to populate a StatNameList with a mixture of
   // StatName and string_view.
-  MetricImpl(StatName name, absl::string_view tag_extracted_name, const std::vector<Tag>& tags,
+  MetricImpl(StatName name, StatName tag_extracted_name, const std::vector<Tag>& tags,
              SymbolTable& symbol_table)
-      : MetricImpl(symbol_table.toString(name), tag_extracted_name, tags, symbol_table) {}
+      : helper_(name, tag_extracted_name, tags, symbol_table) {}
 
   explicit MetricImpl(SymbolTable& symbol_table)
       : MetricImpl("", "", std::vector<Tag>(), symbol_table) {}

source/common/stats/symbol_table_creator.cc

@@ -4,7 +4,7 @@ namespace Envoy {
 namespace Stats {
 
 bool SymbolTableCreator::initialized_ = false;
-bool SymbolTableCreator::use_fake_symbol_tables_ = true;
+bool SymbolTableCreator::use_fake_symbol_tables_ = false;
 
 SymbolTablePtr SymbolTableCreator::initAndMakeSymbolTable(bool use_fake) {
   ASSERT(!initialized_ || (use_fake_symbol_tables_ == use_fake));

source/common/stats/symbol_table_impl.cc

@@ -1,6 +1,7 @@
 #include "common/stats/symbol_table_impl.h"
 
 #include <algorithm>
+#include <iostream>
 #include <memory>
 #include <unordered_map>
 #include <vector>
@@ -41,19 +42,19 @@ uint64_t StatName::dataSize() const {
 #ifndef ENVOY_CONFIG_COVERAGE
 void StatName::debugPrint() {
   if (size_and_data_ == nullptr) {
-    ENVOY_LOG_MISC(info, "Null StatName");
+    std::cerr << "Null StatName" << std::endl;
   } else {
     const uint64_t nbytes = dataSize();
-    std::string msg = absl::StrCat("dataSize=", nbytes, ":");
+    std::cerr << "dataSize=" << nbytes << ":";
     for (uint64_t i = 0; i < nbytes; ++i) {
-      absl::StrAppend(&msg, " ", static_cast<uint64_t>(data()[i]));
+      std::cerr << " " << static_cast<uint64_t>(data()[i]);
     }
     const SymbolVec encoding = SymbolTableImpl::Encoding::decodeSymbols(data(), dataSize());
-    absl::StrAppend(&msg, ", numSymbols=", encoding.size(), ":");
+    std::cerr << ", numSymbols=" << encoding.size() << ":";
     for (Symbol symbol : encoding) {
-      absl::StrAppend(&msg, " ", symbol);
+      std::cerr << " " << symbol;
     }
-    ENVOY_LOG_MISC(info, "{}", msg);
+    std::cerr << std::endl;
   }
 }
 #endif
@@ -524,26 +525,24 @@ SymbolTable::StoragePtr SymbolTableImpl::join(const StatNameVec& stat_names) con
   return mem_block.release();
 }
 
-void SymbolTableImpl::populateList(const absl::string_view* names, uint32_t num_names,
-                                   StatNameList& list) {
+void SymbolTableImpl::populateList(const StatName* names, uint32_t num_names, StatNameList& list) {
   RELEASE_ASSERT(num_names < 256, "Maximum number elements in a StatNameList exceeded");
 
   // First encode all the names.
   size_t total_size_bytes = 1; /* one byte for holding the number of names */
 
-  STACK_ARRAY(encodings, Encoding, num_names);
   for (uint32_t i = 0; i < num_names; ++i) {
-    Encoding& encoding = encodings[i];
-    addTokensToEncoding(names[i], encoding);
-    total_size_bytes += encoding.bytesRequired();
+    total_size_bytes += names[i].size();
   }
 
   // Now allocate the exact number of bytes required and move the encodings
   // into storage.
   MemBlockBuilder<uint8_t> mem_block(total_size_bytes);
   mem_block.appendOne(num_names);
-  for (auto& encoding : encodings) {
-    encoding.moveToMemBlock(mem_block);
+  for (uint32_t i = 0; i < num_names; ++i) {
+    const StatName stat_name = names[i];
+    incRefCount(stat_name);
+    mem_block.appendData(absl::MakeSpan(stat_name.dataIncludingSize(), stat_name.size()));
   }
 
   // This assertion double-checks the arithmetic where we computed

source/common/stats/symbol_table_impl.h

@@ -182,8 +182,7 @@ class SymbolTableImpl : public SymbolTable {
   void free(const StatName& stat_name) override;
   void incRefCount(const StatName& stat_name) override;
   StoragePtr join(const StatNameVec& stat_names) const override;
-  void populateList(const absl::string_view* names, uint32_t num_names,
-                    StatNameList& list) override;
+  void populateList(const StatName* names, uint32_t num_names, StatNameList& list) override;
   StoragePtr encode(absl::string_view name) override;
   StoragePtr makeDynamicStorage(absl::string_view name) override;
   void callWithStringView(StatName stat_name,
@@ -389,7 +388,8 @@ class StatName {
     // hasher.
     const char* cdata = reinterpret_cast<const char*>(stat_name.data());
     absl::string_view data_as_string_view = absl::string_view(cdata, stat_name.dataSize());
-    return H::combine(std::move(h), data_as_string_view);
+    auto hh = H::combine(std::move(h), data_as_string_view);
+    return hh;
   }
 
   /**
@@ -453,6 +453,8 @@ class StatName {
     return size_and_data_ + SymbolTableImpl::Encoding::encodingSizeBytes(dataSize());
   }
 
+  const uint8_t* dataIncludingSize() const { return size_and_data_; }
+
   /**
    * @return whether this is empty.
    */

source/common/stats/thread_local_store.cc

@@ -593,8 +593,9 @@ void ThreadLocalHistogramImpl::merge(histogram_t* target) {
 ParentHistogramImpl::ParentHistogramImpl(StatName name, Histogram::Unit unit, Store& parent,
                                          TlsScope& tls_scope, absl::string_view tag_extracted_name,
                                          const std::vector<Tag>& tags)
-    : MetricImpl(name, tag_extracted_name, tags, parent.symbolTable()), unit_(unit),
-      parent_(parent), tls_scope_(tls_scope), interval_histogram_(hist_alloc()),
+    : MetricImpl(name, StatNameManagedStorage(tag_extracted_name, parent.symbolTable()).statName(),
+                 tags, parent.symbolTable()),
+      unit_(unit), parent_(parent), tls_scope_(tls_scope), interval_histogram_(hist_alloc()),
       cumulative_histogram_(hist_alloc()), interval_statistics_(interval_histogram_),
       cumulative_statistics_(cumulative_histogram_), merged_(false) {}
 

test/common/router/BUILD

@@ -257,6 +257,7 @@ envoy_cc_test(
         "//source/common/upstream:upstream_includes",
         "//source/common/upstream:upstream_lib",
         "//test/common/http:common_lib",
+        "//test/common/stats:stat_test_utility_lib",
         "//test/mocks/http:http_mocks",
         "//test/mocks/local_info:local_info_mocks",
         "//test/mocks/network:network_mocks",

test/common/router/router_test.cc

@@ -23,6 +23,7 @@
 #include "common/upstream/upstream_impl.h"
 
 #include "test/common/http/common.h"
+#include "test/common/stats/stat_test_utility.h"
 #include "test/mocks/http/mocks.h"
 #include "test/mocks/local_info/mocks.h"
 #include "test/mocks/network/mocks.h"
@@ -3671,15 +3672,13 @@ TEST_F(RouterTest, AltStatName) {
   EXPECT_EQ(1U,
             cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("canary.upstream_rq_200")
                 .value());
-  EXPECT_EQ(
-      1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_.counter("alt_stat.upstream_rq_200")
-              .value());
-  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
-                    .counter("alt_stat.zone.zone_name.to_az.upstream_rq_200")
-                    .value());
-  EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_
-                    .counter("alt_stat.zone.zone_name.to_az.upstream_rq_200")
-                    .value());
+
+  Stats::TestUtil::StatNames stat_names(cm_.thread_local_cluster_.cluster_.info_->stats_store_);
+  Stats::StatName alt_stat = stat_names.dynamic("alt_stat");
+  EXPECT_EQ(1U, stat_names.counterValue({alt_stat, stat_names.symbolic("upstream_rq_200")}));
+  EXPECT_EQ(1U, stat_names.counterValue(
+                    {alt_stat, stat_names.symbolic("zone.zone_name.to_az.upstream_rq_200")}));
+  EXPECT_EQ(1U, stat_names.counterValue({alt_stat, stat_names.symbolic("upstream_rq_200")}));
 }
 
 TEST_F(RouterTest, Redirect) {

test/common/stats/BUILD

@@ -85,6 +85,7 @@ envoy_cc_test_library(
         "abseil_strings",
     ],
     deps = [
+        "//include/envoy/stats:stats_interface",
         "//source/common/common:assert_lib",
         "//source/common/memory:stats_lib",
         "//source/common/stats:symbol_table_creator_lib",