diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 7f5114fb1b4a5..6cd1a853e8ba7 100644 --- a/include/envoy/stats/stats.h +++ b/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. diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index 7d5062a05de03..cde9e618511e0 100644 --- a/source/common/stats/BUILD +++ b/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", ], ) diff --git a/source/common/stats/heap_stat_data.h b/source/common/stats/heap_stat_data.h index d87a784673352..34a1fcd36e428 100644 --- a/source/common/stats/heap_stat_data.h +++ b/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 value_{0}; std::atomic pending_increment_{0}; std::atomic flags_{0}; diff --git a/source/common/stats/histogram_impl.h b/source/common/stats/histogram_impl.h index 28cb1e0931b71..9afd73d85cf04 100644 --- a/source/common/stats/histogram_impl.h +++ b/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&& 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 diff --git a/source/common/stats/metric_impl.h b/source/common/stats/metric_impl.h index 34ea7ed830374..eb3ef19f8f50c 100644 --- a/source/common/stats/metric_impl.h +++ b/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&& tags) - : name_(name), tag_extracted_name_(std::move(tag_extracted_name)), tags_(std::move(tags)) {} + MetricImpl(std::string&& tag_extracted_name, std::vector&& 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& 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 tags_; }; diff --git a/source/common/stats/raw_stat_data.h b/source/common/stats/raw_stat_data.h index da000fb277753..da7eaa99f2a77 100644 --- a/source/common/stats/raw_stat_data.h +++ b/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 value_; std::atomic pending_increment_; std::atomic flags_; diff --git a/source/common/stats/stat_data_allocator_impl.h b/source/common/stats/stat_data_allocator_impl.h index 7d4a96154a123..8c31af8d52811 100644 --- a/source/common/stats/stat_data_allocator_impl.h +++ b/source/common/stats/stat_data_allocator_impl.h @@ -63,10 +63,12 @@ template class CounterImpl : public Counter, public MetricImpl public: CounterImpl(StatData& data, StatDataAllocatorImpl& alloc, std::string&& tag_extracted_name, std::vector&& 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 GaugeImpl : public Gauge, public MetricImpl { public: GaugeImpl(StatData& data, StatDataAllocatorImpl& alloc, std::string&& tag_extracted_name, std::vector&& 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; diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index c7f71308ae3fb..dcf96f6b9239b 100644 --- a/source/common/stats/thread_local_store.cc +++ b/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&& 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&& 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_); diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index ab464672a1744..e5f00cdeeacad 100644 --- a/source/common/stats/thread_local_store.h +++ b/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 flags_; std::thread::id created_thread_id_; + const std::string name_; }; typedef std::shared_ptr 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 tls_histograms_ GUARDED_BY(merge_lock_); bool merged_; + const std::string name_; }; typedef std::shared_ptr ParentHistogramImplSharedPtr; diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index d2269d308f739..5b1dbf98df98c 100644 --- a/test/mocks/stats/mocks.cc +++ b/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_)); diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index af3f073db03e4..424a2194a27bb 100644 --- a/test/mocks/stats/mocks.h +++ b/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&()); 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&()); 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&()); @@ -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 ""; };