stats: refactoring MetricImpl without strings (envoyproxy#4190)

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

include/envoy/stats/stats.h

@@ -23,7 +23,7 @@ class Metric {
   /**
    * Returns the full name of the Metric.
    */
-  virtual const std::string& name() const PURE;
+  virtual const std::string name() const PURE;
 
   /**
    * Returns a vector of configurable tags to identify this Metric.

source/common/stats/BUILD

@@ -54,6 +54,7 @@ envoy_cc_library(
     hdrs = ["metric_impl.h"],
     deps = [
         "//include/envoy/stats:stats_interface",
+        "//source/common/common:assert_lib",
     ],
 )
 

source/common/stats/heap_stat_data.h

@@ -24,6 +24,11 @@ struct HeapStatData {
    */
   absl::string_view key() const { return name_; }
 
+  /**
+   * @returns std::string the name as a std::string.
+   */
+  std::string name() const { return name_; }
+
   std::atomic<uint64_t> value_{0};
   std::atomic<uint64_t> pending_increment_{0};
   std::atomic<uint16_t> flags_{0};

source/common/stats/histogram_impl.h

@@ -46,7 +46,10 @@ class HistogramImpl : public Histogram, public MetricImpl {
 public:
   HistogramImpl(const std::string& name, Store& parent, std::string&& tag_extracted_name,
                 std::vector<Tag>&& tags)
-      : MetricImpl(name, std::move(tag_extracted_name), std::move(tags)), parent_(parent) {}
+      : MetricImpl(std::move(tag_extracted_name), std::move(tags)), parent_(parent), name_(name) {}
+
+  // Stats:;Metric
+  const std::string name() const override { return name_; }
 
   // Stats::Histogram
   void recordValue(uint64_t value) override { parent_.deliverHistogramToSinks(*this, value); }
@@ -56,6 +59,8 @@ class HistogramImpl : public Histogram, public MetricImpl {
 private:
   // This is used for delivering the histogram data to sinks.
   Store& parent_;
+
+  const std::string name_;
 };
 
 } // namespace Stats

source/common/stats/metric_impl.h

@@ -5,19 +5,23 @@
 
 #include "envoy/stats/stats.h"
 
+#include "common/common/assert.h"
+
 namespace Envoy {
 namespace Stats {
 
 /**
  * Implementation of the Metric interface. Virtual inheritance is used because the interfaces that
  * will inherit from Metric will have other base classes that will also inherit from Metric.
+ *
+ * MetricImpl is not meant to be instantiated as-is. For performance reasons we keep name() virtual
+ * and expect child classes to implement it.
  */
 class MetricImpl : public virtual Metric {
 public:
-  MetricImpl(const std::string& name, std::string&& tag_extracted_name, std::vector<Tag>&& tags)
-      : name_(name), tag_extracted_name_(std::move(tag_extracted_name)), tags_(std::move(tags)) {}
+  MetricImpl(std::string&& tag_extracted_name, std::vector<Tag>&& tags)
+      : tag_extracted_name_(std::move(tag_extracted_name)), tags_(std::move(tags)) {}
 
-  const std::string& name() const override { return name_; }
   const std::string& tagExtractedName() const override { return tag_extracted_name_; }
   const std::vector<Tag>& tags() const override { return tags_; }
 
@@ -30,7 +34,6 @@ class MetricImpl : public virtual Metric {
   };
 
 private:
-  const std::string name_;
   const std::string tag_extracted_name_;
   const std::vector<Tag> tags_;
 };

source/common/stats/raw_stat_data.h

@@ -74,6 +74,11 @@ struct RawStatData {
    */
   absl::string_view key() const { return absl::string_view(name_); }
 
+  /**
+   * Returns the name as a std::string.
+   */
+  std::string name() const { return std::string(name_); }
+
   std::atomic<uint64_t> value_;
   std::atomic<uint64_t> pending_increment_;
   std::atomic<uint16_t> flags_;

source/common/stats/stat_data_allocator_impl.h

@@ -63,10 +63,12 @@ template <class StatData> class CounterImpl : public Counter, public MetricImpl
 public:
   CounterImpl(StatData& data, StatDataAllocatorImpl<StatData>& alloc,
               std::string&& tag_extracted_name, std::vector<Tag>&& tags)
-      : MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data),
-        alloc_(alloc) {}
+      : MetricImpl(std::move(tag_extracted_name), std::move(tags)), data_(data), alloc_(alloc) {}
   ~CounterImpl() { alloc_.free(data_); }
 
+  // Stats::Metric
+  const std::string name() const override { return data_.name(); }
+
   // Stats::Counter
   void add(uint64_t amount) override {
     data_.value_ += amount;
@@ -92,10 +94,12 @@ template <class StatData> class GaugeImpl : public Gauge, public MetricImpl {
 public:
   GaugeImpl(StatData& data, StatDataAllocatorImpl<StatData>& alloc,
             std::string&& tag_extracted_name, std::vector<Tag>&& tags)
-      : MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data),
-        alloc_(alloc) {}
+      : MetricImpl(std::move(tag_extracted_name), std::move(tags)), data_(data), alloc_(alloc) {}
   ~GaugeImpl() { alloc_.free(data_); }
 
+  // Stats::Metric
+  const std::string name() const override { return data_.name(); }
+
   // Stats::Gauge
   virtual void add(uint64_t amount) override {
     data_.value_ += amount;

source/common/stats/thread_local_store.cc

@@ -347,8 +347,8 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(const std::string& name
 ThreadLocalHistogramImpl::ThreadLocalHistogramImpl(const std::string& name,
                                                    std::string&& tag_extracted_name,
                                                    std::vector<Tag>&& tags)
-    : MetricImpl(name, std::move(tag_extracted_name), std::move(tags)), current_active_(0),
-      flags_(0), created_thread_id_(std::this_thread::get_id()) {
+    : MetricImpl(std::move(tag_extracted_name), std::move(tags)), current_active_(0), flags_(0),
+      created_thread_id_(std::this_thread::get_id()), name_(name) {
   histograms_[0] = hist_alloc();
   histograms_[1] = hist_alloc();
 }
@@ -373,10 +373,10 @@ void ThreadLocalHistogramImpl::merge(histogram_t* target) {
 ParentHistogramImpl::ParentHistogramImpl(const std::string& name, Store& parent,
                                          TlsScope& tls_scope, std::string&& tag_extracted_name,
                                          std::vector<Tag>&& tags)
-    : MetricImpl(name, std::move(tag_extracted_name), std::move(tags)), parent_(parent),
+    : MetricImpl(std::move(tag_extracted_name), std::move(tags)), parent_(parent),
       tls_scope_(tls_scope), interval_histogram_(hist_alloc()), cumulative_histogram_(hist_alloc()),
       interval_statistics_(interval_histogram_), cumulative_statistics_(cumulative_histogram_),
-      merged_(false) {}
+      merged_(false), name_(name) {}
 
 ParentHistogramImpl::~ParentHistogramImpl() {
   hist_free(interval_histogram_);

source/common/stats/thread_local_store.h

@@ -47,12 +47,16 @@ class ThreadLocalHistogramImpl : public Histogram, public MetricImpl {
   void recordValue(uint64_t value) override;
   bool used() const override { return flags_ & Flags::Used; }
 
+  // Stats::Metric
+  const std::string name() const override { return name_; }
+
 private:
   uint64_t otherHistogramIndex() const { return 1 - current_active_; }
   uint64_t current_active_;
   histogram_t* histograms_[2];
   std::atomic<uint16_t> flags_;
   std::thread::id created_thread_id_;
+  const std::string name_;
 };
 
 typedef std::shared_ptr<ThreadLocalHistogramImpl> TlsHistogramSharedPtr;
@@ -86,6 +90,9 @@ class ParentHistogramImpl : public ParentHistogram, public MetricImpl {
   }
   const std::string summary() const override;
 
+  // Stats::Metric
+  const std::string name() const override { return name_; }
+
 private:
   bool usedLockHeld() const EXCLUSIVE_LOCKS_REQUIRED(merge_lock_);
 
@@ -98,6 +105,7 @@ class ParentHistogramImpl : public ParentHistogram, public MetricImpl {
   mutable Thread::MutexBasicLockable merge_lock_;
   std::list<TlsHistogramSharedPtr> tls_histograms_ GUARDED_BY(merge_lock_);
   bool merged_;
+  const std::string name_;
 };
 
 typedef std::shared_ptr<ParentHistogramImpl> ParentHistogramImplSharedPtr;

test/mocks/stats/mocks.cc

@@ -14,7 +14,6 @@ namespace Envoy {
 namespace Stats {
 
 MockCounter::MockCounter() {
-  ON_CALL(*this, name()).WillByDefault(ReturnRef(name_));
   ON_CALL(*this, tagExtractedName()).WillByDefault(ReturnRef(name_));
   ON_CALL(*this, tags()).WillByDefault(ReturnRef(tags_));
   ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_));
@@ -24,7 +23,6 @@ MockCounter::MockCounter() {
 MockCounter::~MockCounter() {}
 
 MockGauge::MockGauge() {
-  ON_CALL(*this, name()).WillByDefault(ReturnRef(name_));
   ON_CALL(*this, tagExtractedName()).WillByDefault(ReturnRef(name_));
   ON_CALL(*this, tags()).WillByDefault(ReturnRef(tags_));
   ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_));

test/mocks/stats/mocks.h

@@ -27,10 +27,13 @@ class MockCounter : public Counter {
   MockCounter();
   ~MockCounter();
 
+  // Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This
+  // creates a deadlock in gmock and is an unintended use of mock functions.
+  const std::string name() const override { return name_; };
+
   MOCK_METHOD1(add, void(uint64_t amount));
   MOCK_METHOD0(inc, void());
   MOCK_METHOD0(latch, uint64_t());
-  MOCK_CONST_METHOD0(name, const std::string&());
   MOCK_CONST_METHOD0(tagExtractedName, const std::string&());
   MOCK_CONST_METHOD0(tags, const std::vector<Tag>&());
   MOCK_METHOD0(reset, void());
@@ -49,10 +52,13 @@ class MockGauge : public Gauge {
   MockGauge();
   ~MockGauge();
 
+  // Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This
+  // creates a deadlock in gmock and is an unintended use of mock functions.
+  const std::string name() const override { return name_; };
+
   MOCK_METHOD1(add, void(uint64_t amount));
   MOCK_METHOD0(dec, void());
   MOCK_METHOD0(inc, void());
-  MOCK_CONST_METHOD0(name, const std::string&());
   MOCK_CONST_METHOD0(tagExtractedName, const std::string&());
   MOCK_CONST_METHOD0(tags, const std::vector<Tag>&());
   MOCK_METHOD1(set, void(uint64_t value));
@@ -73,7 +79,7 @@ class MockHistogram : public Histogram {
 
   // Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This
   // creates a deadlock in gmock and is an unintended use of mock functions.
-  const std::string& name() const override { return name_; };
+  const std::string name() const override { return name_; };
 
   MOCK_CONST_METHOD0(tagExtractedName, const std::string&());
   MOCK_CONST_METHOD0(tags, const std::vector<Tag>&());
@@ -92,7 +98,7 @@ class MockParentHistogram : public ParentHistogram {
 
   // Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This
   // creates a deadlock in gmock and is an unintended use of mock functions.
-  const std::string& name() const override { return name_; };
+  const std::string name() const override { return name_; };
   void merge() override {}
   const std::string summary() const override { return ""; };