Refactor Stats::RawStatData into a StatsOptions struct (#3629)

This change allows us to deprecate the statics inside Stats::RawStatData. Some side effects of this change are: a) HeapRawStatDataAllocator no longer performs stat name truncation, b) we now construct BlockMemoryHashSet, HotRestartImpl, C/L/RdsSubscription, and ThreadLocalStoreImpl as functions of this Stats::StatsOptions struct, and c) Stats::RawStatData now looks more like a set of libraries for computing stat padding, as opposed to a source of truth for the maximum allowable name lengths. Finally, a chain of functions starting under server.cc (translateBootstrap, translateClusterManagerBootstrap, translateListener, translateCluster, translateVirtualHost) have had Stats::StatsOptions& added to their interfaces, so that Utility::checkObjNameLength() can be called with the necessary context.

Signed-off-by: James Buckland <jbuckland@google.com>

Author: ambuc

include/envoy/server/BUILD

@@ -111,6 +111,7 @@ envoy_cc_library(
     hdrs = ["options.h"],
     deps = [
         "//include/envoy/network:address_interface",
+        "//include/envoy/stats:stats_interface",
     ],
 )
 

include/envoy/server/options.h

@@ -6,6 +6,7 @@
 
 #include "envoy/common/pure.h"
 #include "envoy/network/address.h"
+#include "envoy/stats/stats.h"
 
 #include "spdlog/spdlog.h"
 
@@ -149,10 +150,10 @@ class Options {
   virtual uint64_t maxStats() const PURE;
 
   /**
-   * @return uint64_t the maximum name length of the name field in
+   * @return StatsOptions& the max stat name / suffix lengths for stats.
    * router/cluster/listener.
    */
-  virtual uint64_t maxObjNameLength() const PURE;
+  virtual const Stats::StatsOptions& statsOptions() const PURE;
 
   /**
    * @return bool indicating whether the hot restart functionality has been disabled via cli flags.

include/envoy/stats/stats.h

@@ -24,6 +24,43 @@ class Instance;
 
 namespace Stats {
 
+/**
+ * Struct stored under Server::Options to hold information about the maximum object name length and
+ * maximum stat suffix length of a stat. These have defaults in StatsOptionsImpl, and the maximum
+ * object name length can be overridden. The default initialization is used in IsolatedStatImpl, and
+ * the user-overridden struct is stored in Options.
+ *
+ * As noted in the comment above StatsOptionsImpl in source/common/stats/stats_impl.h, a stat name
+ * often contains both a string whose length is user-defined (cluster_name in the below example),
+ * and a specific statistic name generated by Envoy. To make room for growth on both fronts, we
+ * limit the max allowed length of each separately.
+ *
+ *                           name / stat name
+ *  |----------------------------------------------------------------|
+ *  cluster.<cluster_name>.outlier_detection.ejections_consecutive_5xx
+ *  |--------------------------------------| |-----------------------|
+ *                object name                         suffix
+ */
+class StatsOptions {
+public:
+  virtual ~StatsOptions() {}
+
+  /**
+   * The max allowed length of a complete stat name, including suffix.
+   */
+  virtual size_t maxNameLength() const PURE;
+
+  /**
+   * The max allowed length of the object part of a stat name.
+   */
+  virtual size_t maxObjNameLength() const PURE;
+
+  /**
+   * The max allowed length of a stat suffix.
+   */
+  virtual size_t maxStatSuffixLength() const PURE;
+};
+
 /**
  * General representation of a tag.
  */
@@ -329,6 +366,12 @@ class Scope {
    * @return a histogram within the scope's namespace with a particular value type.
    */
   virtual Histogram& histogram(const std::string& name) PURE;
+
+  /**
+   * @return a reference to the top-level StatsOptions struct, containing information about the
+   * maximum allowable object name length and stat suffix length.
+   */
+  virtual const Stats::StatsOptions& statsOptions() const PURE;
 };
 
 /**

source/common/common/BUILD

@@ -235,6 +235,8 @@ envoy_cc_library(
     deps = [
         ":assert_lib",
         ":logger_lib",
+        "//include/envoy/stats:stats_interface",
+        "//source/common/stats:stats_lib",
     ],
 )
 

source/common/common/block_memory_hash_set.h

@@ -5,10 +5,12 @@
 #include <utility>
 
 #include "envoy/common/exception.h"
+#include "envoy/stats/stats.h"
 
 #include "common/common/assert.h"
 #include "common/common/fmt.h"
 #include "common/common/logger.h"
+#include "common/stats/stats_impl.h"
 
 #include "absl/strings/string_view.h"
 
@@ -57,21 +59,24 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
 
   /**
    * Constructs a map control structure given a set of options, which cannot be changed.
-   * @param options describes the parameters comtrolling set layout.
+   * @param hash_set_options describes the parameters comtrolling set layout.
    * @param init true if the memory should be initialized on construction. If false,
    *             the data in the table will be sanity checked, and an exception thrown if
    *             it is incoherent or mismatches the passed-in options.
    * @param memory the memory buffer for the set data.
+   * @param stats_options a reference to the top-level StatsOptions struct containing
+   *                      information about max allowable stat name lengths.
    *
    * Note that no locking of any kind is done by this class; this must be done at the
    * call-site to support concurrent access.
    */
-  BlockMemoryHashSet(const BlockMemoryHashSetOptions& options, bool init, uint8_t* memory)
-      : cells_(nullptr), control_(nullptr), slots_(nullptr) {
-    mapMemorySegments(options, memory);
+  BlockMemoryHashSet(const BlockMemoryHashSetOptions& hash_set_options, bool init, uint8_t* memory,
+                     const Stats::StatsOptions& stats_options)
+      : cells_(nullptr), control_(nullptr), slots_(nullptr), stats_options_(stats_options) {
+    mapMemorySegments(hash_set_options, memory);
     if (init) {
-      initialize(options);
-    } else if (!attach(options)) {
+      initialize(hash_set_options);
+    } else if (!attach(hash_set_options)) {
       throw EnvoyException("BlockMemoryHashSet: Incompatible memory block");
     }
   }
@@ -82,22 +87,20 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
    * backing-store (eg) in memory, which we do after
    * constructing the object with the desired sizing.
    */
-  static uint64_t numBytes(const BlockMemoryHashSetOptions& options) {
-    uint64_t size =
-        cellOffset(options.capacity) + sizeof(Control) + options.num_slots * sizeof(uint32_t);
+  static uint64_t numBytes(const BlockMemoryHashSetOptions& hash_set_options,
+                           const Stats::StatsOptions& stats_options) {
+    uint64_t size = cellOffset(hash_set_options.capacity, stats_options) + sizeof(Control) +
+                    hash_set_options.num_slots * sizeof(uint32_t);
     return align(size);
   }
 
-  uint64_t numBytes() const { return numBytes(control_->options); }
-
-  /**
-   * Returns the options structure that was used to construct the set.
-   */
-  const BlockMemoryHashSetOptions& options() const { return control_->options; }
+  uint64_t numBytes(const Stats::StatsOptions& stats_options) const {
+    return numBytes(control_->hash_set_options, stats_options);
+  }
 
   /** Examines the data structures to see if they are sane, assert-failing on any trouble. */
   void sanityCheck() {
-    RELEASE_ASSERT(control_->size <= control_->options.capacity, "");
+    RELEASE_ASSERT(control_->size <= control_->hash_set_options.capacity, "");
 
     // As a sanity check, make sure there are control_->size values
     // reachable from the slots, each of which has a valid
@@ -107,11 +110,11 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
     // slot. Note that the num_values message will be emitted outside
     // the loop.
     uint32_t num_values = 0;
-    for (uint32_t slot = 0; slot < control_->options.num_slots; ++slot) {
+    for (uint32_t slot = 0; slot < control_->hash_set_options.num_slots; ++slot) {
       uint32_t next = 0; // initialized to silence compilers.
       for (uint32_t cell_index = slots_[slot];
            (cell_index != Sentinal) && (num_values <= control_->size); cell_index = next) {
-        RELEASE_ASSERT(cell_index < control_->options.capacity, "");
+        RELEASE_ASSERT(cell_index < control_->hash_set_options.capacity, "");
         Cell& cell = getCell(cell_index);
         absl::string_view key = cell.value.key();
         RELEASE_ASSERT(computeSlot(key) == slot, "");
@@ -122,7 +125,7 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
     RELEASE_ASSERT(num_values == control_->size, "");
 
     uint32_t num_free_entries = 0;
-    uint32_t expected_free_entries = control_->options.capacity - control_->size;
+    uint32_t expected_free_entries = control_->hash_set_options.capacity - control_->size;
 
     // Don't infinite-loop with a corruption; break when we see there's a problem.
     for (uint32_t cell_index = control_->free_cell_index;
@@ -152,7 +155,7 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
     if (value != nullptr) {
       return ValueCreatedPair(value, false);
     }
-    if (control_->size >= control_->options.capacity) {
+    if (control_->size >= control_->hash_set_options.capacity) {
       return ValueCreatedPair(nullptr, false);
     }
     const uint32_t slot = computeSlot(key);
@@ -162,7 +165,7 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
     cell.next_cell_index = slots_[slot];
     slots_[slot] = cell_index;
     value = &cell.value;
-    value->initialize(key);
+    value->truncateAndInit(key, stats_options_);
     ++control_->size;
     return ValueCreatedPair(value, true);
   }
@@ -215,9 +218,9 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
   /**
    * Computes a version signature based on the options and the hash function.
    */
-  std::string version() {
-    return fmt::format("options={} hash={} size={}", control_->options.toString(),
-                       control_->hash_signature, numBytes());
+  std::string version(const Stats::StatsOptions& stats_options) {
+    return fmt::format("options={} hash={} size={}", control_->hash_set_options.toString(),
+                       control_->hash_signature, numBytes(stats_options));
   }
 
 private:
@@ -228,20 +231,20 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
    * coming in.
    * @param memory
    */
-  void initialize(const BlockMemoryHashSetOptions& options) {
+  void initialize(const BlockMemoryHashSetOptions& hash_set_options) {
     control_->hash_signature = Value::hash(signatureStringToHash());
-    control_->num_bytes = numBytes(options);
-    control_->options = options;
+    control_->num_bytes = numBytes(hash_set_options, stats_options_);
+    control_->hash_set_options = hash_set_options;
     control_->size = 0;
     control_->free_cell_index = 0;
 
     // Initialize all the slots;
-    for (uint32_t slot = 0; slot < options.num_slots; ++slot) {
+    for (uint32_t slot = 0; slot < hash_set_options.num_slots; ++slot) {
       slots_[slot] = Sentinal;
     }
 
     // Initialize the free-cell list.
-    const uint32_t last_cell = options.capacity - 1;
+    const uint32_t last_cell = hash_set_options.capacity - 1;
     for (uint32_t cell_index = 0; cell_index < last_cell; ++cell_index) {
       Cell& cell = getCell(cell_index);
       cell.next_cell_index = cell_index + 1;
@@ -254,10 +257,10 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
    * sanity check to make sure the options copied to the provided memory match, and also
    * that the slot, cell, and key-string structures look sane.
    */
-  bool attach(const BlockMemoryHashSetOptions& options) {
-    if (numBytes(options) != control_->num_bytes) {
-      ENVOY_LOG(error, "BlockMemoryHashSet unexpected memory size {} != {}", numBytes(options),
-                control_->num_bytes);
+  bool attach(const BlockMemoryHashSetOptions& hash_set_options) {
+    if (numBytes(hash_set_options, stats_options_) != control_->num_bytes) {
+      ENVOY_LOG(error, "BlockMemoryHashSet unexpected memory size {} != {}",
+                numBytes(hash_set_options, stats_options_), control_->num_bytes);
       return false;
     }
     if (Value::hash(signatureStringToHash()) != control_->hash_signature) {
@@ -269,7 +272,7 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
   }
 
   uint32_t computeSlot(absl::string_view key) {
-    return Value::hash(key) % control_->options.num_slots;
+    return Value::hash(key) % control_->hash_set_options.num_slots;
   }
 
   /**
@@ -290,11 +293,11 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
    * Represents control-values for the hash-table.
    */
   struct Control {
-    BlockMemoryHashSetOptions options; // Options established at map construction time.
-    uint64_t hash_signature;           // Hash of a constant signature string.
-    uint64_t num_bytes;                // Bytes allocated on behalf of the map.
-    uint32_t size;                     // Number of values currently stored.
-    uint32_t free_cell_index;          // Offset of first free cell.
+    BlockMemoryHashSetOptions hash_set_options; // Options established at map construction time.
+    uint64_t hash_signature;                    // Hash of a constant signature string.
+    uint64_t num_bytes;                         // Bytes allocated on behalf of the map.
+    uint32_t size;                              // Number of values currently stored.
+    uint32_t free_cell_index;                   // Offset of first free cell.
   };
 
   /**
@@ -323,10 +326,11 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
    * simply an array index because we don't know the size of a key at
    * compile-time.
    */
-  static uint64_t cellOffset(uint32_t cell_index) {
+  static uint64_t cellOffset(uint32_t cell_index, const Stats::StatsOptions& stats_options) {
     // sizeof(Cell) includes 'sizeof Value' which may not be accurate. So we need to
     // subtract that off, and add the template method's view of the actual value-size.
-    uint64_t cell_size = align(sizeof(Cell) + Value::size() - sizeof(Value));
+    uint64_t cell_size =
+        align(sizeof(Cell) + Value::structSizeWithOptions(stats_options) - sizeof(Value));
     return cell_index * cell_size;
   }
 
@@ -335,17 +339,17 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
    */
   Cell& getCell(uint32_t cell_index) {
     // Because the key-size is parameteriziable, an array-lookup on sizeof(Cell) does not work.
-    char* ptr = reinterpret_cast<char*>(cells_) + cellOffset(cell_index);
+    char* ptr = reinterpret_cast<char*>(cells_) + cellOffset(cell_index, stats_options_);
     RELEASE_ASSERT((reinterpret_cast<uint64_t>(ptr) & (calculateAlignment() - 1)) == 0, "");
     return *reinterpret_cast<Cell*>(ptr);
   }
 
   /** Maps out the segments of memory for us to work with. */
-  void mapMemorySegments(const BlockMemoryHashSetOptions& options, uint8_t* memory) {
+  void mapMemorySegments(const BlockMemoryHashSetOptions& hash_set_options, uint8_t* memory) {
     // Note that we are not examining or mutating memory here, just looking at the pointer,
     // so we don't need to hold any locks.
     cells_ = reinterpret_cast<Cell*>(memory); // First because Value may need to be aligned.
-    memory += cellOffset(options.capacity);
+    memory += cellOffset(hash_set_options.capacity, stats_options_);
     control_ = reinterpret_cast<Control*>(memory);
     memory += sizeof(Control);
     slots_ = reinterpret_cast<uint32_t*>(memory);
@@ -356,6 +360,7 @@ template <class Value> class BlockMemoryHashSet : public Logger::Loggable<Logger
   Cell* cells_;
   Control* control_;
   uint32_t* slots_;
+  const Stats::StatsOptions& stats_options_;
 };
 
 } // namespace Envoy

source/common/config/BUILD

@@ -35,6 +35,7 @@ envoy_cc_library(
         "//source/common/common:assert_lib",
         "//source/common/json:config_schemas_lib",
         "//source/common/protobuf:utility_lib",
+        "//source/common/stats:stats_lib",
         "//source/extensions/stat_sinks:well_known_names",
         "@envoy_api//envoy/config/bootstrap/v2:bootstrap_cc",
     ],
@@ -66,6 +67,7 @@ envoy_cc_library(
         "//source/common/common:assert_lib",
         "//source/common/json:config_schemas_lib",
         "//source/common/network:utility_lib",
+        "//source/common/stats:stats_lib",
         "@envoy_api//envoy/api/v2:cds_cc",
         "@envoy_api//envoy/api/v2/cluster:circuit_breaker_cc",
     ],
@@ -106,6 +108,7 @@ envoy_cc_library(
         ":rds_json_lib",
         ":utility_lib",
         "//include/envoy/json:json_object_interface",
+        "//include/envoy/stats:stats_interface",
         "//source/common/common:assert_lib",
         "//source/common/common:utility_lib",
         "//source/common/json:config_schemas_lib",
@@ -218,6 +221,7 @@ envoy_cc_library(
         "//source/common/common:assert_lib",
         "//source/common/json:config_schemas_lib",
         "//source/common/network:utility_lib",
+        "//source/common/stats:stats_lib",
         "//source/extensions/filters/network:well_known_names",
         "@envoy_api//envoy/api/v2:lds_cc",
     ],
@@ -271,6 +275,7 @@ envoy_cc_library(
         "//source/common/common:assert_lib",
         "//source/common/config:utility_lib",
         "//source/common/json:config_schemas_lib",
+        "//source/common/stats:stats_lib",
         "//source/extensions/filters/http:well_known_names",
         "@envoy_api//envoy/api/v2:rds_cc",
     ],