stats: Don't use real symbol's decodeTokens function to extract fake-symbol dynamics

include/envoy/stats/symbol_table.h

@@ -2,6 +2,7 @@
 
 #include <functional>
 #include <memory>
+#include <utility>
 #include <vector>
 
 #include "envoy/common/pure.h"
@@ -26,6 +27,14 @@ class StatNameSet;
 
 using StatNameSetPtr = std::unique_ptr<StatNameSet>;
 
+/**
+ * Holds a range of indexes indicating which parts of a stat-name are
+ * dynamic. This is used to transfer stats from hot-restart parent to child,
+ * retaining the same name structure.
+ */
+using DynamicSpan = std::pair<uint32_t, uint32_t>;
+using DynamicSpans = std::vector<DynamicSpan>;
+
 /**
  * SymbolTable manages a namespace optimized for stat names, exploiting their
  * typical composition from "."-separated tokens, with a significant overlap
@@ -179,6 +188,19 @@ class SymbolTable {
    */
   virtual StatNameSetPtr makeSet(absl::string_view name) PURE;
 
+  /**
+   * Identifies the dynamic components of a stat_name into an array of integer
+   * pairs, indicating the begin/end of spans of tokens in the stat-name that
+   * are created from StatNameDynamicStore or StatNameDynamicPool.
+   *
+   * This can be used to reconstruct the same exact StatNames in
+   * StatNames::mergeStats(), to enable stat continuity across hot-restart.
+   *
+   * @param stat_name the input stat name.
+   * @return the array of pairs indicating the bounds.
+   */
+  virtual DynamicSpans getDynamicSpans(StatName stat_name) const PURE;
+
 private:
   friend struct HeapStatData;
   friend class StatNameDynamicStorage;

source/common/stats/fake_symbol_table_impl.h

@@ -122,6 +122,7 @@ class FakeSymbolTableImpl : public SymbolTable {
   void clearRecentLookups() override {}
   void setRecentLookupCapacity(uint64_t) override {}
   uint64_t recentLookupCapacity() const override { return 0; }
+  DynamicSpans getDynamicSpans(StatName) const override { return DynamicSpans(); }
 
 private:
   absl::string_view toStringView(const StatName& stat_name) const {

source/common/stats/stat_merger.cc

@@ -7,31 +7,6 @@ namespace Stats {
 
 StatMerger::StatMerger(Store& target_store) : temp_scope_(target_store.createScope("")) {}
 
-StatMerger::DynamicSpans StatMerger::DynamicContext::encodeSegments(StatName stat_name) {
-  DynamicSpans dynamic_spans;
-  uint32_t index = 0;
-  auto record_dynamic = [&dynamic_spans, &index](absl::string_view str) {
-    DynamicSpan span;
-    span.first = index;
-    index += std::count(str.begin(), str.end(), '.');
-    span.second = index;
-    ++index;
-    dynamic_spans.push_back(span);
-  };
-
-  // Use decodeTokens to suss out which components of stat_name are
-  // symbolic vs dynamic. The lambda that takes a Symbol is called
-  // for symbolic components. The lambda called with a string_view
-  // is called for dynamic components.
-  //
-  // Note that with fake symbol tables, the Symbol lambda is called
-  // once for each character in the string, and no dynamics will
-  // be recorded.
-  SymbolTableImpl::Encoding::decodeTokens(
-      stat_name.data(), stat_name.dataSize(), [&index](Symbol) { ++index; }, record_dynamic);
-  return dynamic_spans;
-}
-
 StatName StatMerger::DynamicContext::makeDynamicStatName(const std::string& name,
                                                          const DynamicsMap& map) {
   auto iter = map.find(name);

source/common/stats/stat_merger.h

@@ -14,8 +14,6 @@ namespace Stats {
 // (typically hot restart parent+child) Envoy processes.
 class StatMerger {
 public:
-  using DynamicSpan = std::pair<uint32_t, uint32_t>;
-  using DynamicSpans = std::vector<DynamicSpan>;
   using DynamicsMap = absl::flat_hash_map<std::string, DynamicSpans>;
 
   // Holds state needed to construct StatName with mixed dynamic/symbolic
@@ -25,19 +23,6 @@ class StatMerger {
     DynamicContext(SymbolTable& symbol_table)
         : symbol_table_(symbol_table), symbolic_pool_(symbol_table), dynamic_pool_(symbol_table) {}
 
-    /**
-     * Identifies the dynamic components of a stat_name into an array of integer
-     * pairs, indicating the begin/end of spans of tokens in the stat-name that
-     * are created from StatNameDynamicStore or StatNameDynamicPool.
-     *
-     * This can be used to reconstruct the same exact StatNames in mergeStats(),
-     * to enable stat continuity across hot-restart.
-     *
-     * @param stat_name the input stat name.
-     * @return the array pair indicating the bounds.
-     */
-    static DynamicSpans encodeSegments(StatName stat_name);
-
     /**
      * Generates a StatName with mixed dynamic/symbolic components based on
      * the string and the dynamic_map obtained from encodeSegments.

source/common/stats/symbol_table_impl.cc

@@ -314,6 +314,32 @@ uint64_t SymbolTableImpl::getRecentLookups(const RecentLookupsFn& iter) const {
   return total;
 }
 
+DynamicSpans SymbolTableImpl::getDynamicSpans(StatName stat_name) const {
+  DynamicSpans dynamic_spans;
+
+  uint32_t index = 0;
+  auto record_dynamic = [&dynamic_spans, &index](absl::string_view str) {
+    DynamicSpan span;
+    span.first = index;
+    index += std::count(str.begin(), str.end(), '.');
+    span.second = index;
+    ++index;
+    dynamic_spans.push_back(span);
+  };
+
+  // Use decodeTokens to suss out which components of stat_name are
+  // symbolic vs dynamic. The lambda that takes a Symbol is called
+  // for symbolic components. The lambda called with a string_view
+  // is called for dynamic components.
+  //
+  // Note that with fake symbol tables, the Symbol lambda is called
+  // once for each character in the string, and no dynamics will
+  // be recorded.
+  Encoding::decodeTokens(
+      stat_name.data(), stat_name.dataSize(), [&index](Symbol) { ++index; }, record_dynamic);
+  return dynamic_spans;
+}
+
 void SymbolTableImpl::setRecentLookupCapacity(uint64_t capacity) {
   Thread::LockGuard lock(lock_);
   recent_lookups_.setCapacity(capacity);

source/common/stats/symbol_table_impl.h

@@ -206,6 +206,7 @@ class SymbolTableImpl : public SymbolTable {
   void clearRecentLookups() override;
   void setRecentLookupCapacity(uint64_t capacity) override;
   uint64_t recentLookupCapacity() const override;
+  DynamicSpans getDynamicSpans(StatName stat_name) const override;
 
 private:
   friend class StatName;

source/server/hot_restarting_child.cc

@@ -91,19 +91,18 @@ void HotRestartingChild::sendParentTerminateRequest() {
 
 void HotRestartingChild::mergeParentStats(Stats::Store& stats_store,
                                           const HotRestartMessage::Reply::Stats& stats_proto) {
-  using StatMerger = Stats::StatMerger;
   if (!stat_merger_) {
-    stat_merger_ = std::make_unique<StatMerger>(stats_store);
+    stat_merger_ = std::make_unique<Stats::StatMerger>(stats_store);
   }
 
   // Convert the protobuf for serialized dynamic spans into the structure
   // required by StatMerger.
-  StatMerger::DynamicsMap dynamics;
+  Stats::StatMerger::DynamicsMap dynamics;
   for (auto iter : stats_proto.dynamics()) {
-    StatMerger::DynamicSpans& spans = dynamics[iter.first];
+    Stats::DynamicSpans& spans = dynamics[iter.first];
     for (int i = 0; i < iter.second.spans_size(); ++i) {
       const HotRestartMessage::Reply::Span& span_proto = iter.second.spans(i);
-      spans.push_back(StatMerger::DynamicSpan(span_proto.first(), span_proto.last()));
+      spans.push_back(Stats::DynamicSpan(span_proto.first(), span_proto.last()));
     }
   }
   stat_merger_->mergeStats(stats_proto.counter_deltas(), stats_proto.gauges(), dynamics);

source/server/hot_restarting_parent.cc

@@ -163,14 +163,13 @@ void HotRestartingParent::Internal::recordDynamics(HotRestartMessage::Reply::Sta
   // components using a dynamic representation.
   //
   // See https://github.com/envoyproxy/envoy/issues/9874 for more details.
-  using Stats::StatMerger;
-  StatMerger::DynamicSpans spans = StatMerger::DynamicContext::encodeSegments(stat_name);
+  Stats::DynamicSpans spans = server_->stats().symbolTable().getDynamicSpans(stat_name);
 
   // Convert that C++ structure (controlled by stat_merger.cc) into a protobuf
   // for serialization.
   if (!spans.empty()) {
     HotRestartMessage::Reply::RepeatedSpan spans_proto;
-    for (const StatMerger::DynamicSpan& span : spans) {
+    for (const Stats::DynamicSpan& span : spans) {
       HotRestartMessage::Reply::Span* span_proto = spans_proto.add_spans();
       span_proto->set_first(span.first);
       span_proto->set_last(span.second);

test/common/stats/stat_merger_fuzz_test.cc

@@ -6,6 +6,8 @@
 #include "test/fuzz/fuzz_runner.h"
 #include "test/fuzz/utility.h"
 
+#include "absl/strings/str_replace.h"
+
 namespace Envoy {
 namespace Stats {
 namespace Fuzz {
@@ -15,6 +17,13 @@ void testDynamicEncoding(absl::string_view data, SymbolTable& symbol_table) {
   StatNamePool symbolic_pool(symbol_table);
   std::vector<StatName> stat_names;
 
+  // This local string is write-only; it's used to help when debugging
+  // a crash. If a crash is found, you can print the unit_test_encoding
+  // in the debugger and then add that as a test-case in stat_merger_text.cc,
+  // in StatMergerDynamicTest.DynamicsWithFakeSymbolTable and
+  // StatMergerDynamicTest.DynamicsWithRealSymbolTable.
+  std::string unit_test_encoding;
+
   for (uint32_t index = 0; index < data.size();) {
     // Select component lengths between 0 and 7 bytes inclusive, and ensure it
     // doesn't overrun our buffer. It's OK to get very small or empty segments.
@@ -26,10 +35,15 @@ void testDynamicEncoding(absl::string_view data, SymbolTable& symbol_table) {
     // determine whether to treat this segment symbolic or not.
     absl::string_view segment = data.substr(index, num_bytes);
     bool is_symbolic = (data[index] & 0x8) == 0x0;
+    if (index != 0) {
+      unit_test_encoding += ".";
+    }
     index += num_bytes + 1;
     if (is_symbolic) {
+      absl::StrAppend(&unit_test_encoding, segment);
       stat_names.push_back(symbolic_pool.add(segment));
     } else {
+      absl::StrAppend(&unit_test_encoding, "D:", absl::StrReplaceAll(segment, {{".", ","}}));
       stat_names.push_back(dynamic_pool.add(segment));
     }
   }
@@ -40,7 +54,7 @@ void testDynamicEncoding(absl::string_view data, SymbolTable& symbol_table) {
   StatMerger::DynamicContext dynamic_context(symbol_table);
   std::string name = symbol_table.toString(stat_name);
   StatMerger::DynamicsMap dynamic_map;
-  dynamic_map[name] = StatMerger::DynamicContext::encodeSegments(stat_name);
+  dynamic_map[name] = symbol_table.getDynamicSpans(stat_name);
 
   StatName decoded = dynamic_context.makeDynamicStatName(name, dynamic_map);
   FUZZ_ASSERT(name == symbol_table.toString(decoded));

test/common/stats/stat_merger_test.cc

@@ -51,7 +51,7 @@ class StatMergerTest : public testing::Test {
 
     std::string name = symbol_table.toString(stat_name);
     StatMerger::DynamicsMap dynamic_map;
-    dynamic_map[name] = StatMerger::DynamicContext::encodeSegments(stat_name);
+    dynamic_map[name] = symbol_table.getDynamicSpans(stat_name);
     StatMerger::DynamicContext dynamic_context(symbol_table);
     StatName decoded = dynamic_context.makeDynamicStatName(name, dynamic_map);
     EXPECT_EQ(stat_name, decoded) << name;
@@ -256,7 +256,7 @@ class StatMergerDynamicTest : public testing::Test {
 
     std::string name = symbol_table_->toString(stat_name);
     StatMerger::DynamicsMap dynamic_map;
-    StatMerger::DynamicSpans spans = StatMerger::DynamicContext::encodeSegments(stat_name);
+    DynamicSpans spans = symbol_table_->getDynamicSpans(stat_name);
     uint32_t size = 0;
     if (!spans.empty()) {
       dynamic_map[name] = spans;
@@ -276,6 +276,12 @@ class StatMergerDynamicTest : public testing::Test {
 TEST_F(StatMergerDynamicTest, DynamicsWithRealSymbolTable) {
   init(std::make_unique<SymbolTableImpl>());
 
+  for (uint32_t i = 1; i < 256; ++i) {
+    char ch = static_cast<char>(i);
+    absl::string_view one_char(&ch, 1);
+    EXPECT_EQ(1, dynamicEncodeDecodeTest(absl::StrCat("D:", one_char))) << "dynamic=" << one_char;
+    EXPECT_EQ(0, dynamicEncodeDecodeTest(one_char)) << "symbolic=" << one_char;
+  }
   EXPECT_EQ(0, dynamicEncodeDecodeTest("normal"));
   EXPECT_EQ(1, dynamicEncodeDecodeTest("D:dynamic"));
   EXPECT_EQ(0, dynamicEncodeDecodeTest("hello.world"));
@@ -299,6 +305,12 @@ TEST_F(StatMergerDynamicTest, DynamicsWithRealSymbolTable) {
 TEST_F(StatMergerDynamicTest, DynamicsWithFakeSymbolTable) {
   init(std::make_unique<FakeSymbolTableImpl>());
 
+  for (uint32_t i = 1; i < 256; ++i) {
+    char ch = static_cast<char>(i);
+    absl::string_view one_char(&ch, 1);
+    EXPECT_EQ(0, dynamicEncodeDecodeTest(absl::StrCat("D:", one_char))) << "dynamic=" << one_char;
+    EXPECT_EQ(0, dynamicEncodeDecodeTest(one_char)) << "symbolic=" << one_char;
+  }
   EXPECT_EQ(0, dynamicEncodeDecodeTest("normal"));
   EXPECT_EQ(0, dynamicEncodeDecodeTest("D:dynamic"));
   EXPECT_EQ(0, dynamicEncodeDecodeTest("hello.world"));