clone of envoyproxy#9774

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

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/common/utility.cc

@@ -241,8 +241,8 @@ bool StringUtil::atoull(const char* str, uint64_t& out, int base) {
   }
 }
 
-absl::string_view StringUtil::ltrim(absl::string_view source) {
-  const absl::string_view::size_type pos = source.find_first_not_of(WhitespaceChars);
+absl::string_view StringUtil::ltrim(absl::string_view source, absl::string_view delims) {
+  const absl::string_view::size_type pos = source.find_first_not_of(delims);
   if (pos != absl::string_view::npos) {
     source.remove_prefix(pos);
   } else {
@@ -251,8 +251,8 @@ absl::string_view StringUtil::ltrim(absl::string_view source) {
   return source;
 }
 
-absl::string_view StringUtil::rtrim(absl::string_view source) {
-  const absl::string_view::size_type pos = source.find_last_not_of(WhitespaceChars);
+absl::string_view StringUtil::rtrim(absl::string_view source, absl::string_view delims) {
+  const absl::string_view::size_type pos = source.find_last_not_of(delims);
   if (pos != absl::string_view::npos) {
     source.remove_suffix(source.size() - pos - 1);
   } else {
@@ -261,7 +261,9 @@ absl::string_view StringUtil::rtrim(absl::string_view source) {
   return source;
 }
 
-absl::string_view StringUtil::trim(absl::string_view source) { return ltrim(rtrim(source)); }
+absl::string_view StringUtil::trim(absl::string_view source, absl::string_view delims) {
+  return ltrim(rtrim(source, delims), delims);
+}
 
 absl::string_view StringUtil::removeTrailingCharacters(absl::string_view source, char ch) {
   const absl::string_view::size_type pos = source.find_last_not_of(ch);

source/common/common/utility.h

@@ -203,23 +203,29 @@ class StringUtil {
   /**
    * Trim leading whitespace from a string view.
    * @param source supplies the string view to be trimmed.
+   * @param delims the set of characters to trim.
    * @return trimmed string view.
    */
-  static absl::string_view ltrim(absl::string_view source);
+  static absl::string_view ltrim(absl::string_view source,
+                                 absl::string_view delims = WhitespaceChars);
 
   /**
    * Trim trailing whitespaces from a string view.
    * @param source supplies the string view to be trimmed.
+   * @param delims the set of characters to trim.
    * @return trimmed string view.
    */
-  static absl::string_view rtrim(absl::string_view source);
+  static absl::string_view rtrim(absl::string_view source,
+                                 absl::string_view delims = WhitespaceChars);
 
   /**
    * Trim leading and trailing whitespaces from a string view.
    * @param source supplies the string view to be trimmed.
+   * @param delims the set of characters to trim.
    * @return trimmed string view.
    */
-  static absl::string_view trim(absl::string_view source);
+  static absl::string_view trim(absl::string_view source,
+                                absl::string_view delims = WhitespaceChars);
 
   /**
    * Removes any specific trailing characters from the end of a string_view.

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.sizeAndData(), name.size()));
     }
 
     // This assertion double-checks the arithmetic where we computed

source/common/stats/isolated_store_impl.cc

@@ -24,15 +24,14 @@ 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, alloc_.symbolTable().toString(name), std::vector<Tag>());
+        return alloc_.makeCounter(name, toString(name), std::vector<Tag>());
       }),
       gauges_([this](StatName name, Gauge::ImportMode import_mode) -> GaugeSharedPtr {
-        return alloc_.makeGauge(name, alloc_.symbolTable().toString(name), std::vector<Tag>(),
-                                import_mode);
+        return alloc_.makeGauge(name, toString(name), std::vector<Tag>(), import_mode);
       }),
       histograms_([this](StatName name, Histogram::Unit unit) -> HistogramSharedPtr {
-        return HistogramSharedPtr(new HistogramImpl(
-            name, unit, *this, alloc_.symbolTable().toString(name), std::vector<Tag>()));
+        return HistogramSharedPtr(
+            new HistogramImpl(name, unit, *this, toString(name), std::vector<Tag>()));
       }),
       null_counter_(new NullCounterImpl(symbol_table)),
       null_gauge_(new NullGaugeImpl(symbol_table)) {}

source/common/stats/isolated_store_impl.h

@@ -147,6 +147,8 @@ 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,20 +14,21 @@ MetricHelper::~MetricHelper() {
   ASSERT(!stat_names_.populated());
 }
 
-MetricHelper::MetricHelper(absl::string_view name, absl::string_view tag_extracted_name,
+MetricHelper::MetricHelper(StatName name, absl::string_view 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();
-  absl::FixedArray<absl::string_view> names(num_names);
+  absl::FixedArray<StatName> names(num_names);
   names[0] = name;
-  names[1] = tag_extracted_name;
+  StatNamePool pool(symbol_table);
+  names[1] = pool.add(tag_extracted_name);
   int index = 1;
   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, absl::string_view tag_extracted_name, const std::vector<Tag>& tags,
+               SymbolTable& symbol_table);
   ~MetricHelper();
 
   StatName statName() const;
@@ -55,21 +55,15 @@ class MetricHelper {
  */
 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) {}
-
-  // 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.
+  // TODO(jmarantz): Use StatName for tag_extracted_name.
   MetricImpl(StatName name, absl::string_view 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) {}
 
+  // Empty construction of a MetricImpl; used for null stats.
   explicit MetricImpl(SymbolTable& symbol_table)
-      : MetricImpl("", "", std::vector<Tag>(), symbol_table) {}
-
+      : MetricImpl(StatNameManagedStorage("", symbol_table).statName(), "", std::vector<Tag>(),
+                   symbol_table) {}
   std::vector<Tag> 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

@@ -1,8 +1,8 @@
 #include "common/stats/symbol_table_impl.h"
 
 #include <algorithm>
+#include <iostream>
 #include <memory>
-#include <unordered_map>
 #include <vector>
 
 #include "common/common/assert.h"
@@ -40,20 +40,23 @@ uint64_t StatName::dataSize() const {
 
 #ifndef ENVOY_CONFIG_COVERAGE
 void StatName::debugPrint() {
+  // TODO(jmarantz): capture this functionality (always prints regardless of
+  // loglevel) in an ENVOY_LOG macro variant or similar, perhaps
+  // ENVOY_LOG_MISC(stderr, ...);
   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 +527,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 */
 
-  absl::FixedArray<Encoding> encodings(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.sizeAndData(), 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,
@@ -453,6 +452,11 @@ class StatName {
     return size_and_data_ + SymbolTableImpl::Encoding::encodingSizeBytes(dataSize());
   }
 
+  /**
+   * @return A pointer to the buffer, including the size bytes.
+   */
+  const uint8_t* sizeAndData() const { return size_and_data_; }
+
   /**
    * @return whether this is empty.
    */

source/server/options_impl.cc

@@ -157,18 +157,8 @@ OptionsImpl::OptionsImpl(std::vector<std::string> args,
   }
 
   hot_restart_disabled_ = disable_hot_restart.getValue();
-
   mutex_tracing_enabled_ = enable_mutex_tracing.getValue();
-
-  // TODO(#9768); When This bug is fixed, we can remove this hack. Until then
-  // is not safe to use real symbol tables.
-  //   fake_symbol_table_enabled_ = use_fake_symbol_table.getValue();
-  fake_symbol_table_enabled_ = true;
-  if (!use_fake_symbol_table.getValue()) {
-    ENVOY_LOG(warn, "Real symbol tables are temporarily disabled due to #9768. "
-                    "Using fake symbol tables");
-  }
-
+  fake_symbol_table_enabled_ = use_fake_symbol_table.getValue();
   cpuset_threads_ = cpuset_threads.getValue();
 
   log_level_ = default_log_level;

test/common/common/utility_test.cc

@@ -223,6 +223,22 @@ TEST(StringUtil, StringViewRtrim) {
   EXPECT_EQ("", StringUtil::rtrim(""));
 }
 
+TEST(StringUtil, StringViewLtrimPunctuation) {
+  static const char Punctuation[] = ".:,;";
+  EXPECT_EQ("", StringUtil::ltrim(".....", Punctuation));
+  EXPECT_EQ("hello:;", StringUtil::ltrim(",.hello:;", Punctuation));
+  EXPECT_EQ("", StringUtil::ltrim(".;.;,,,:", Punctuation));
+  EXPECT_EQ("", StringUtil::ltrim("", Punctuation));
+}
+
+TEST(StringUtil, StringViewRtrimPunctuation) {
+  static const char Punctuation[] = ".:,;";
+  EXPECT_EQ("", StringUtil::rtrim(".....", Punctuation));
+  EXPECT_EQ(",.hello", StringUtil::rtrim(",.hello:;", Punctuation));
+  EXPECT_EQ("", StringUtil::rtrim(".;.;,,,:", Punctuation));
+  EXPECT_EQ("", StringUtil::rtrim("", Punctuation));
+}
+
 TEST(StringUtil, RemoveTrailingCharacters) {
   EXPECT_EQ("", StringUtil::removeTrailingCharacters("......", '.'));
   EXPECT_EQ("\t\f\v\n\rhello ", StringUtil::removeTrailingCharacters("\t\f\v\n\rhello ", '.'));

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,11 @@ 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::MixedStatNames stat_names(
+      cm_.thread_local_cluster_.cluster_.info_->stats_store_);
+  EXPECT_EQ(1U, stat_names.counterValue("`alt_stat`.upstream_rq_200"));
+  EXPECT_EQ(1U, stat_names.counterValue("`alt_stat`.zone.zone_name.to_az.upstream_rq_200"));
 }
 
 TEST_F(RouterTest, Redirect) {