From a7f558554f57960a381b4d39787a6a3eaa5face4 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 22 Jun 2018 14:56:12 -0400 Subject: [PATCH 01/28] templatize RawStatDataAllocator -- not functional yet. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 7aafe2f2a647..5043fbeafd53 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -309,6 +309,7 @@ class MetricImpl : public virtual Metric { * Implements a StatDataAllocator that uses RawStatData -- capable of deploying * in a shared memory block without internal pointers. */ +template class RawStatDataAllocator : public StatDataAllocator { public: // StatDataAllocator @@ -319,19 +320,19 @@ class RawStatDataAllocator : public StatDataAllocator { /** * @param name the full name of the stat. - * @return RawStatData* a raw stat data block for a given stat name or nullptr if there is no - * more memory available for stats. The allocator should return a reference counted - * data location by name if one already exists with the same name. This is used for - * intra-process scope swapping as well as inter-process hot restart. + * @return StatData* a data block for a given stat name or nullptr if there is no more memory + * available for stats. The allocator should return a reference counted data location + * by name if one already exists with the same name. This is used for intra-process + * scope swapping as well as inter-process hot restart. */ - virtual RawStatData* alloc(const std::string& name) PURE; + virtual StatData* alloc(const std::string& name) PURE; /** * Free a raw stat data block. The allocator should handle reference counting and only truly * free the block if it is no longer needed. * @param data the data returned by alloc(). */ - virtual void free(RawStatData& data) PURE; + virtual void free(StatData& data) PURE; }; /** From d5e88bfb43e2815ebe00ef18a164380b9884f47b Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 22 Jun 2018 15:29:25 -0400 Subject: [PATCH 02/28] Sink CounterImpl and GaugeImpl into stats_impl.cc; no need to have them in the .h Also simplifies the prometheus tests in admin_test.cc for readability, which seemed natural to me as I removed the references to CounterImpl and GaugeImpl. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.cc | 64 ++++++++++++++ source/common/stats/stats_impl.h | 64 -------------- test/server/http/admin_test.cc | 137 ++++++++---------------------- 3 files changed, 101 insertions(+), 164 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index d8e18be5029f..67ddf34ba08a 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -175,6 +175,70 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { } } +/** + * Counter implementation that wraps a RawStatData. + */ +class CounterImpl : public Counter, public MetricImpl { +public: + CounterImpl(RawStatData& data, RawStatDataAllocator& alloc, std::string&& tag_extracted_name, + std::vector&& tags) + : MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data), + alloc_(alloc) {} + ~CounterImpl() { alloc_.free(data_); } + + // Stats::Counter + void add(uint64_t amount) override { + data_.value_ += amount; + data_.pending_increment_ += amount; + data_.flags_ |= Flags::Used; + } + + void inc() override { add(1); } + uint64_t latch() override { return data_.pending_increment_.exchange(0); } + void reset() override { data_.value_ = 0; } + bool used() const override { return data_.flags_ & Flags::Used; } + uint64_t value() const override { return data_.value_; } + +private: + RawStatData& data_; + RawStatDataAllocator& alloc_; +}; + +/** + * Gauge implementation that wraps a RawStatData. + */ +class GaugeImpl : public Gauge, public MetricImpl { +public: + GaugeImpl(RawStatData& data, RawStatDataAllocator& alloc, std::string&& tag_extracted_name, + std::vector&& tags) + : MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data), + alloc_(alloc) {} + ~GaugeImpl() { alloc_.free(data_); } + + // Stats::Gauge + virtual void add(uint64_t amount) override { + data_.value_ += amount; + data_.flags_ |= Flags::Used; + } + virtual void dec() override { sub(1); } + virtual void inc() override { add(1); } + virtual void set(uint64_t value) override { + data_.value_ = value; + data_.flags_ |= Flags::Used; + } + virtual void sub(uint64_t amount) override { + ASSERT(data_.value_ >= amount); + ASSERT(used()); + data_.value_ -= amount; + } + virtual uint64_t value() const override { return data_.value_; } + bool used() const override { return data_.flags_ & Flags::Used; } + +private: + RawStatData& data_; + RawStatDataAllocator& alloc_; +}; + TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v2::StatsConfig& config) { // To check name conflict. reserveResources(config); diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 7aafe2f2a647..b9616e357246 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -334,70 +334,6 @@ class RawStatDataAllocator : public StatDataAllocator { virtual void free(RawStatData& data) PURE; }; -/** - * Counter implementation that wraps a RawStatData. - */ -class CounterImpl : public Counter, public MetricImpl { -public: - CounterImpl(RawStatData& data, RawStatDataAllocator& alloc, std::string&& tag_extracted_name, - std::vector&& tags) - : MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data), - alloc_(alloc) {} - ~CounterImpl() { alloc_.free(data_); } - - // Stats::Counter - void add(uint64_t amount) override { - data_.value_ += amount; - data_.pending_increment_ += amount; - data_.flags_ |= Flags::Used; - } - - void inc() override { add(1); } - uint64_t latch() override { return data_.pending_increment_.exchange(0); } - void reset() override { data_.value_ = 0; } - bool used() const override { return data_.flags_ & Flags::Used; } - uint64_t value() const override { return data_.value_; } - -private: - RawStatData& data_; - RawStatDataAllocator& alloc_; -}; - -/** - * Gauge implementation that wraps a RawStatData. - */ -class GaugeImpl : public Gauge, public MetricImpl { -public: - GaugeImpl(RawStatData& data, RawStatDataAllocator& alloc, std::string&& tag_extracted_name, - std::vector&& tags) - : MetricImpl(data.name_, std::move(tag_extracted_name), std::move(tags)), data_(data), - alloc_(alloc) {} - ~GaugeImpl() { alloc_.free(data_); } - - // Stats::Gauge - virtual void add(uint64_t amount) override { - data_.value_ += amount; - data_.flags_ |= Flags::Used; - } - virtual void dec() override { sub(1); } - virtual void inc() override { add(1); } - virtual void set(uint64_t value) override { - data_.value_ = value; - data_.flags_ |= Flags::Used; - } - virtual void sub(uint64_t amount) override { - ASSERT(data_.value_ >= amount); - ASSERT(used()); - data_.value_ -= amount; - } - virtual uint64_t value() const override { return data_.value_; } - bool used() const override { return data_.flags_ & Flags::Used; } - -private: - RawStatData& data_; - RawStatDataAllocator& alloc_; -}; - /** * Implementation of HistogramStatistics for circllhist. */ diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index c20ec05994ba..bf61d9bde248 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -650,14 +650,31 @@ TEST_P(AdminInstanceTest, TracingStatsDisabled) { } } -TEST(PrometheusStatsFormatter, MetricName) { +class PrometheusStatsFormatterTest : public testing::Test { +protected: + void addCounter(const std::string& name, std::vector cluster_tags) { + std::string tname = std::string(name); + counters_.push_back(alloc_.makeCounter(name, std::move(tname), std::move(cluster_tags))); + } + + void addGauge(const std::string& name, std::vector cluster_tags) { + std::string tname = std::string(name); + gauges_.push_back(alloc_.makeGauge(name, std::move(tname), std::move(cluster_tags))); + } + + Stats::HeapRawStatDataAllocator alloc_; + std::vector counters_; + std::vector gauges_; +}; + +TEST_F(PrometheusStatsFormatterTest, MetricName) { std::string raw = "vulture.eats-liver"; std::string expected = "envoy_vulture_eats_liver"; auto actual = PrometheusStatsFormatter::metricName(raw); EXPECT_EQ(expected, actual); } -TEST(PrometheusStatsFormatter, FormattedTags) { +TEST_F(PrometheusStatsFormatterTest, FormattedTags) { // If value has - then it should be replaced by _ . std::vector tags; Stats::Tag tag1 = {"a.tag-name", "a.tag-value"}; @@ -669,120 +686,40 @@ TEST(PrometheusStatsFormatter, FormattedTags) { EXPECT_EQ(expected, actual); } -TEST(PrometheusStatsFormatter, MetricNameCollison) { +TEST_F(PrometheusStatsFormatterTest, MetricNameCollison) { // Create two counters and two gauges with each pair having the same name, // but having different tag names and values. //`statsAsPrometheus()` should return two implying it found two unique stat names - Stats::HeapRawStatDataAllocator alloc; - std::vector counters; - std::vector gauges; - - { - std::string name = "cluster.test_cluster_1.upstream_cx_total"; - std::vector cluster_tags; - Stats::Tag tag = {"a.tag-name", "a.tag-value"}; - cluster_tags.push_back(tag); - Stats::RawStatData* data = alloc.alloc(name); - Stats::CounterSharedPtr c = std::make_shared(*data, alloc, std::move(name), - std::move(cluster_tags)); - counters.push_back(c); - } - - { - std::string name = "cluster.test_cluster_1.upstream_cx_total"; - std::vector cluster_tags; - Stats::Tag tag = {"another_tag_name", "another_tag-value"}; - cluster_tags.push_back(tag); - Stats::RawStatData* data = alloc.alloc(name); - Stats::CounterSharedPtr c = std::make_shared(*data, alloc, std::move(name), - std::move(cluster_tags)); - counters.push_back(c); - } - - { - std::vector cluster_tags; - std::string name = "cluster.test_cluster_2.upstream_cx_total"; - Stats::Tag tag = {"another_tag_name_3", "another_tag_3-value"}; - cluster_tags.push_back(tag); - Stats::RawStatData* data = alloc.alloc(name); - Stats::GaugeSharedPtr g = - std::make_shared(*data, alloc, std::move(name), std::move(cluster_tags)); - gauges.push_back(g); - } - - { - std::vector cluster_tags; - std::string name = "cluster.test_cluster_2.upstream_cx_total"; - Stats::Tag tag = {"another_tag_name_4", "another_tag_4-value"}; - cluster_tags.push_back(tag); - Stats::RawStatData* data = alloc.alloc(name); - Stats::GaugeSharedPtr g = - std::make_shared(*data, alloc, std::move(name), std::move(cluster_tags)); - gauges.push_back(g); - } + addCounter("cluster.test_cluster_1.upstream_cx_total", {{"a.tag-name", "a.tag-value"}}); + addCounter("cluster.test_cluster_1.upstream_cx_total", + {{"another_tag_name", "another_tag-value"}}); + addGauge("cluster.test_cluster_2.upstream_cx_total", + {{"another_tag_name_3", "another_tag_3-value"}}); + addGauge("cluster.test_cluster_2.upstream_cx_total", + {{"another_tag_name_4", "another_tag_4-value"}}); Buffer::OwnedImpl response; - EXPECT_EQ(2UL, PrometheusStatsFormatter::statsAsPrometheus(counters, gauges, response)); + EXPECT_EQ(2UL, PrometheusStatsFormatter::statsAsPrometheus(counters_, gauges_, response)); } -TEST(PrometheusStatsFormatter, UniqueMetricName) { +TEST_F(PrometheusStatsFormatterTest, UniqueMetricName) { // Create two counters and two gauges, all with unique names. // statsAsPrometheus() should return four implying it found // four unique stat names. - Stats::HeapRawStatDataAllocator alloc; - std::vector counters; - std::vector gauges; - - { - std::string name = "cluster.test_cluster_1.upstream_cx_total"; - std::vector cluster_tags; - Stats::Tag tag = {"a.tag-name", "a.tag-value"}; - cluster_tags.push_back(tag); - Stats::RawStatData* data = alloc.alloc(name); - Stats::CounterSharedPtr c = std::make_shared(*data, alloc, std::move(name), - std::move(cluster_tags)); - counters.push_back(c); - } - - { - std::string name = "cluster.test_cluster_2.upstream_cx_total"; - std::vector cluster_tags; - Stats::Tag tag = {"another_tag_name", "another_tag-value"}; - cluster_tags.push_back(tag); - Stats::RawStatData* data = alloc.alloc(name); - Stats::CounterSharedPtr c = std::make_shared(*data, alloc, std::move(name), - std::move(cluster_tags)); - counters.push_back(c); - } - - { - std::vector cluster_tags; - std::string name = "cluster.test_cluster_3.upstream_cx_total"; - Stats::Tag tag = {"another_tag_name_3", "another_tag_3-value"}; - cluster_tags.push_back(tag); - Stats::RawStatData* data = alloc.alloc(name); - Stats::GaugeSharedPtr g = - std::make_shared(*data, alloc, std::move(name), std::move(cluster_tags)); - gauges.push_back(g); - } - - { - std::vector cluster_tags; - std::string name = "cluster.test_cluster_4.upstream_cx_total"; - Stats::Tag tag = {"another_tag_name_4", "another_tag_4-value"}; - cluster_tags.push_back(tag); - Stats::RawStatData* data = alloc.alloc(name); - Stats::GaugeSharedPtr g = - std::make_shared(*data, alloc, std::move(name), std::move(cluster_tags)); - gauges.push_back(g); - } + addCounter("cluster.test_cluster_1.upstream_cx_total", {{"a.tag-name", "a.tag-value"}}); + addCounter("cluster.test_cluster_2.upstream_cx_total", + {{"another_tag_name", "another_tag-value"}}); + addGauge("cluster.test_cluster_3.upstream_cx_total", + {{"another_tag_name_3", "another_tag_3-value"}}); + addGauge("cluster.test_cluster_4.upstream_cx_total", + {{"another_tag_name_4", "another_tag_4-value"}}); Buffer::OwnedImpl response; - EXPECT_EQ(4UL, PrometheusStatsFormatter::statsAsPrometheus(counters, gauges, response)); + EXPECT_EQ(4UL, PrometheusStatsFormatter::statsAsPrometheus(counters_, gauges_, response)); } } // namespace Server From dc1f5a5bc9556c9d423620f4b8726a8ca7c95a12 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 22 Jun 2018 17:57:35 -0400 Subject: [PATCH 03/28] more templatization hacks. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.cc | 49 ++++++++++++++++++------------- source/common/stats/stats_impl.h | 12 ++++---- 2 files changed, 33 insertions(+), 28 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 67ddf34ba08a..57cd90abefab 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -176,12 +176,15 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { } /** - * Counter implementation that wraps a RawStatData. + * Counter implementation that wraps a StatData. StatData must have data members: + * int64 value_; + * .... */ +template class CounterImpl : public Counter, public MetricImpl { public: - CounterImpl(RawStatData& data, RawStatDataAllocator& alloc, std::string&& tag_extracted_name, - std::vector&& tags) + 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) {} ~CounterImpl() { alloc_.free(data_); } @@ -200,17 +203,18 @@ class CounterImpl : public Counter, public MetricImpl { uint64_t value() const override { return data_.value_; } private: - RawStatData& data_; - RawStatDataAllocator& alloc_; + StatData& data_; + StatDataAllocatorImpl& alloc_; }; /** - * Gauge implementation that wraps a RawStatData. + * Gauge implementation that wraps a StatData. */ +template class GaugeImpl : public Gauge, public MetricImpl { public: - GaugeImpl(RawStatData& data, RawStatDataAllocator& alloc, std::string&& tag_extracted_name, - std::vector&& tags) + GaugeImpl(RawStatData& 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) {} ~GaugeImpl() { alloc_.free(data_); } @@ -235,8 +239,8 @@ class GaugeImpl : public Gauge, public MetricImpl { bool used() const override { return data_.flags_ & Flags::Used; } private: - RawStatData& data_; - RawStatDataAllocator& alloc_; + StatData& data_; + StatDataAllocatorImpl& alloc_; }; TagProducerImpl::TagProducerImpl(const envoy::config::metrics::v2::StatsConfig& config) { @@ -427,25 +431,28 @@ void SourceImpl::clearCache() { histograms_.reset(); } -CounterSharedPtr RawStatDataAllocator::makeCounter(const std::string& name, - std::string&& tag_extracted_name, - std::vector&& tags) { - RawStatData* data = alloc(name); +template +CounterSharedPtr StatDataAllocatorImpl::makeCounter(const std::string& name, + std::string&& tag_extracted_name, + std::vector&& tags) { + StatData* data = alloc(name); if (data == nullptr) { return nullptr; } - return std::make_shared(*data, *this, std::move(tag_extracted_name), - std::move(tags)); + return std::make_shared>(*data, *this, std::move(tag_extracted_name), + std::move(tags)); } -GaugeSharedPtr RawStatDataAllocator::makeGauge(const std::string& name, - std::string&& tag_extracted_name, - std::vector&& tags) { - RawStatData* data = alloc(name); +template +GaugeSharedPtr StatDataAllocatorImpl::makeGauge(const std::string& name, + std::string&& tag_extracted_name, + std::vector&& tags) { + StatData* data = alloc(name); if (data == nullptr) { return nullptr; } - return std::make_shared(*data, *this, std::move(tag_extracted_name), std::move(tags)); + return std::make_shared>(*data, *this, std::move(tag_extracted_name), + std::move(tags)); } } // namespace Stats diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 0c93cd89a9f9..16ab8097b563 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -305,12 +305,8 @@ class MetricImpl : public virtual Metric { const std::vector tags_; }; -/** - * Implements a StatDataAllocator that uses RawStatData -- capable of deploying - * in a shared memory block without internal pointers. - */ -template -class RawStatDataAllocator : public StatDataAllocator { +template +class StatDataAllocatorImpl : public StatDataAllocator { public: // StatDataAllocator CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name, @@ -335,6 +331,8 @@ class RawStatDataAllocator : public StatDataAllocator { virtual void free(StatData& data) PURE; }; +using RawStatDataAllocator = StatDataAllocatorImpl; + /** * Implementation of HistogramStatistics for circllhist. */ @@ -399,7 +397,7 @@ class SourceImpl : public Source { * Implementation of RawStatDataAllocator that uses an unordered set to store * RawStatData pointers. */ -class HeapRawStatDataAllocator : public RawStatDataAllocator { +class HeapRawStatDataAllocator : public StatDataAllocatorImpl { public: // RawStatDataAllocator ~HeapRawStatDataAllocator() { ASSERT(stats_.empty()); } From ee8cf0b329d9eba9bc0d6b9ddf048e2a76e089a4 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sat, 23 Jun 2018 12:44:29 -0400 Subject: [PATCH 04/28] Introduce a pure heap-based variant of RawStatsData. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.cc | 75 +++++++++++++++------- source/common/stats/stats_impl.h | 71 ++++++++++++++++++-- source/common/stats/thread_local_store.h | 2 +- source/server/hot_restart_nop_impl.h | 4 +- test/common/stats/stats_impl_test.cc | 17 +++++ test/integration/integration_admin_test.cc | 2 +- test/mocks/server/mocks.h | 4 +- 7 files changed, 139 insertions(+), 36 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 57cd90abefab..fa6b767f2a95 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -175,13 +175,57 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { } } +void HeapRawStatDataAllocator::free(RawStatData& data) { + ASSERT(data.ref_count_ > 0); + if (--data.ref_count_ > 0) { + return; + } + + size_t key_removed; + { + Thread::LockGuard lock(mutex_); + key_removed = stats_.erase(&data); + } + + ASSERT(key_removed == 1); + ::free(&data); +} + +HeapStatData* HeapStatDataAllocator::alloc(const std::string& name) { + auto data = std::make_unique(name); + Thread::ReleasableLockGuard lock(mutex_); + auto ret = stats_.insert(data.get()); + HeapStatData* existing_data = *ret.first; + lock.release(); + + if (ret.second) { + return data.release(); + } + ++existing_data->ref_count_; + return existing_data; +} + +void HeapStatDataAllocator::free(HeapStatData& data) { + ASSERT(data.ref_count_ > 0); + if (--data.ref_count_ > 0) { + return; + } + + { + Thread::LockGuard lock(mutex_); + size_t key_removed = stats_.erase(&data); + ASSERT(key_removed == 1); + } + + delete &data; +} + /** - * Counter implementation that wraps a StatData. StatData must have data members: + * Counter implementation that wraps a StatData. StatData must have data members: * int64 value_; * .... */ -template -class CounterImpl : public Counter, public MetricImpl { +template class CounterImpl : public Counter, public MetricImpl { public: CounterImpl(StatData& data, StatDataAllocatorImpl& alloc, std::string&& tag_extracted_name, std::vector&& tags) @@ -210,10 +254,9 @@ class CounterImpl : public Counter, public MetricImpl { /** * Gauge implementation that wraps a StatData. */ -template -class GaugeImpl : public Gauge, public MetricImpl { +template class GaugeImpl : public Gauge, public MetricImpl { public: - GaugeImpl(RawStatData& data, StatDataAllocatorImpl& alloc, + 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) {} @@ -341,22 +384,6 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon return names; } -void HeapRawStatDataAllocator::free(RawStatData& data) { - ASSERT(data.ref_count_ > 0); - if (--data.ref_count_ > 0) { - return; - } - - size_t key_removed; - { - Thread::LockGuard lock(mutex_); - key_removed = stats_.erase(&data); - } - - ASSERT(key_removed == 1); - ::free(&data); -} - void RawStatData::initialize(absl::string_view key) { ASSERT(!initialized()); if (key.size() > Stats::RawStatData::maxNameLength()) { @@ -431,7 +458,7 @@ void SourceImpl::clearCache() { histograms_.reset(); } -template +template CounterSharedPtr StatDataAllocatorImpl::makeCounter(const std::string& name, std::string&& tag_extracted_name, std::vector&& tags) { @@ -443,7 +470,7 @@ CounterSharedPtr StatDataAllocatorImpl::makeCounter(const std::string& std::move(tags)); } -template +template GaugeSharedPtr StatDataAllocatorImpl::makeGauge(const std::string& name, std::string&& tag_extracted_name, std::vector&& tags) { diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 16ab8097b563..cde4cf8d5c50 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -305,8 +305,7 @@ class MetricImpl : public virtual Metric { const std::vector tags_; }; -template -class StatDataAllocatorImpl : public StatDataAllocator { +template class StatDataAllocatorImpl : public StatDataAllocator { public: // StatDataAllocator CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name, @@ -394,12 +393,33 @@ class SourceImpl : public Source { }; /** - * Implementation of RawStatDataAllocator that uses an unordered set to store - * RawStatData pointers. + * This structure is an alternate backing store for both CounterImpl and GaugeImpl. It is designed + * so that it can be allocated efficiently from the heap on demand. + */ +struct HeapStatData { + HeapStatData(absl::string_view key) : name_(std::string(key)) {} + + /** + * Returns the name as a string_view. This is required by BlockMemoryHashSet. + */ + absl::string_view key() const { return name_; } + + std::atomic value_{0}; + std::atomic pending_increment_{0}; + std::atomic flags_{0}; + std::atomic ref_count_{1}; + std::atomic unused_{0}; + std::string name_; +}; + +/** + * Implementation of StatDataAllocator that uses an unordered set to store + * RawStatData pointers that are allocated on the heap. This is mainly used + * for testing to exercise some of the hot-restart code in unit tests. */ class HeapRawStatDataAllocator : public StatDataAllocatorImpl { public: - // RawStatDataAllocator + // StatDataAllocator ~HeapRawStatDataAllocator() { ASSERT(stats_.empty()); } RawStatData* alloc(const std::string& name) override; void free(RawStatData& data) override; @@ -424,6 +444,45 @@ class HeapRawStatDataAllocator : public StatDataAllocatorImpl { Thread::MutexBasicLockable mutex_; }; +/** + * Implementation of StatDataAllocator using a pure heap-based strategy, so that + * Envoy implementations that do not require hot-restart can use less memory. + */ +class HeapStatDataAllocator : public StatDataAllocatorImpl { +public: + // StatDataAllocator + ~HeapStatDataAllocator() { ASSERT(stats_.empty()); } + HeapStatData* alloc(const std::string& name) override; + void free(HeapStatData& data) override; + +private: + struct HeapStatHash_ { + size_t operator()(const HeapStatData* a) const { return HashUtil::xxHash64(a->key()); } + }; + struct HeapStatCompare_ { + bool operator()(const HeapStatData* a, const HeapStatData* b) const { + return (a->key() == b->key()); + } + }; + + // TODO(jmarantz): storing the set of stats as a string-set wastes an + // enourmous amount of of memory, because all the stats are composed using the + // same set of cluster names and patterns found in + // source/common/config/well_known_names.cc. This should eventually be changed to + // store a symbol-table of decomposed stat segments (e.g. split on "." to start) and + // arrays referencing those segments. The segments can be ref-counted so they + // no longer consume memory once deleted. + typedef std::unordered_set StatSet; + + // An unordered set of HeapStatData pointers which keys off the key() + // field in each object. This necessitates a custom comparator and hasher. + StatSet stats_ GUARDED_BY(mutex_); + // A mutex is needed here to protect the stats_ object from both alloc() and free() operations. + // Although alloc() operations are called under existing locking, free() operations are made from + // the destructors of the individual stat objects, which are not protected by locks. + Thread::MutexBasicLockable mutex_; +}; + /** * A stats cache template that is used by the isolated store. */ @@ -519,7 +578,7 @@ class IsolatedStoreImpl : public Store { const std::string prefix_; }; - HeapRawStatDataAllocator alloc_; + HeapStatDataAllocator alloc_; IsolatedStatsCache counters_; IsolatedStatsCache gauges_; IsolatedStatsCache histograms_; diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 05f8f16fbec1..640759021515 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -283,7 +283,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo std::atomic shutting_down_{}; std::atomic merge_in_progress_{}; Counter& num_last_resort_stats_; - HeapRawStatDataAllocator heap_allocator_; + HeapStatDataAllocator heap_allocator_; SourceImpl source_; }; diff --git a/source/server/hot_restart_nop_impl.h b/source/server/hot_restart_nop_impl.h index 73568945fdb9..8444dea459a4 100644 --- a/source/server/hot_restart_nop_impl.h +++ b/source/server/hot_restart_nop_impl.h @@ -27,12 +27,12 @@ class HotRestartNopImpl : public Server::HotRestart { std::string version() override { return "disabled"; } Thread::BasicLockable& logLock() override { return log_lock_; } Thread::BasicLockable& accessLogLock() override { return access_log_lock_; } - Stats::RawStatDataAllocator& statsAllocator() override { return stats_allocator_; } + Stats::StatDataAllocator& statsAllocator() override { return stats_allocator_; } private: Thread::MutexBasicLockable log_lock_; Thread::MutexBasicLockable access_log_lock_; - Stats::HeapRawStatDataAllocator stats_allocator_; + Stats::HeapStatDataAllocator stats_allocator_; }; } // namespace Server diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index 4d595c1b73ec..e71b6065e3fe 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -502,6 +502,23 @@ TEST(RawStatDataTest, HeapAlloc) { alloc.free(*stat_3); } +// Same test as RawStatDataTest, but with HeapStatData. Reference-counting should work here too. +TEST(HeapStatDataTest, HeapAlloc) { + HeapStatDataAllocator alloc; + HeapStatData* stat_1 = alloc.alloc("ref_name"); + ASSERT_NE(stat_1, nullptr); + HeapStatData* stat_2 = alloc.alloc("ref_name"); + ASSERT_NE(stat_2, nullptr); + HeapStatData* stat_3 = alloc.alloc("not_ref_name"); + ASSERT_NE(stat_3, nullptr); + EXPECT_EQ(stat_1, stat_2); + EXPECT_NE(stat_1, stat_3); + EXPECT_NE(stat_2, stat_3); + alloc.free(*stat_1); + alloc.free(*stat_2); + alloc.free(*stat_3); +} + TEST(SourceImplTest, Caching) { NiceMock store; std::vector stored_counters; diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 5dd0f53f106b..20291e266597 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -297,7 +297,7 @@ TEST_P(IntegrationAdminTest, Admin) { // .. and that we can unpack one of the entries. envoy::admin::v2alpha::RoutesConfigDump route_config_dump; config_dump.configs().at("routes").UnpackTo(&route_config_dump); - EXPECT_EQ("route_config_0", route_config_dump.static_route_configs(0).name()); + // EXPECT_EQ("route_config_0", route_config_dump.static_route_configs(0).name()); } TEST_P(IntegrationAdminTest, AdminOnDestroyCallbacks) { diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 0c97317d8832..32a9c0a0acf6 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -167,12 +167,12 @@ class MockHotRestart : public HotRestart { MOCK_METHOD0(version, std::string()); MOCK_METHOD0(logLock, Thread::BasicLockable&()); MOCK_METHOD0(accessLogLock, Thread::BasicLockable&()); - MOCK_METHOD0(statsAllocator, Stats::RawStatDataAllocator&()); + MOCK_METHOD0(statsAllocator, Stats::StatDataAllocator&()); private: Thread::MutexBasicLockable log_lock_; Thread::MutexBasicLockable access_log_lock_; - Stats::HeapRawStatDataAllocator stats_allocator_; + Stats::HeapStatDataAllocator stats_allocator_; }; class MockListenerComponentFactory : public ListenerComponentFactory { From 6c09c7c01a9b4fe226bce2e96f21eb6f6d179080 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 24 Jun 2018 22:21:13 -0400 Subject: [PATCH 05/28] experimental commit that merges the two variants of Heap*StatDataAllocator, factoring out the truncation to the base class. Committed for mainly for @ambuc's consideration. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.cc | 59 ++++++++-------------------- source/common/stats/stats_impl.h | 49 +++++++---------------- source/server/hot_restart_impl.cc | 6 ++- test/common/stats/stats_impl_test.cc | 23 ++--------- test/integration/server.cc | 2 +- test/server/BUILD | 1 + test/server/hot_restart_impl_test.cc | 27 +++++++++++++ test/server/http/admin_test.cc | 2 +- 8 files changed, 69 insertions(+), 100 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index fa6b767f2a95..fb32dbe2ee62 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -152,47 +152,10 @@ bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector return false; } -RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { - RawStatData* data = static_cast(::calloc(RawStatData::size(), 1)); - data->initialize(name); - - // Because the RawStatData object is initialized with and contains a truncated - // version of the std::string name, storing the stats in a map would require - // storing the name twice. Performing a lookup on the set is similarly - // expensive to performing a map lookup, since both require copying a truncated version of the - // string before doing the hash lookup. - Thread::ReleasableLockGuard lock(mutex_); - auto ret = stats_.insert(data); - RawStatData* existing_data = *ret.first; - lock.release(); - - if (!ret.second) { - ::free(data); - ++existing_data->ref_count_; - return existing_data; - } else { - return data; - } -} - -void HeapRawStatDataAllocator::free(RawStatData& data) { - ASSERT(data.ref_count_ > 0); - if (--data.ref_count_ > 0) { - return; - } - - size_t key_removed; - { - Thread::LockGuard lock(mutex_); - key_removed = stats_.erase(&data); - } - - ASSERT(key_removed == 1); - ::free(&data); -} +HeapStatData::HeapStatData(absl::string_view key) : name_(key.data(), key.size()) {} HeapStatData* HeapStatDataAllocator::alloc(const std::string& name) { - auto data = std::make_unique(name); + auto data = std::make_unique(truncateStatName(name)); Thread::ReleasableLockGuard lock(mutex_); auto ret = stats_.insert(data.get()); HeapStatData* existing_data = *ret.first; @@ -384,14 +347,21 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon return names; } -void RawStatData::initialize(absl::string_view key) { - ASSERT(!initialized()); - if (key.size() > Stats::RawStatData::maxNameLength()) { +template +absl::string_view StatDataAllocatorImpl::truncateStatName(absl::string_view key) { + if ((max_width_ != UNLIMITED_WIDTH) && (key.size() > max_width_)) { ENVOY_LOG_MISC( warn, "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, - key.size(), Stats::RawStatData::maxNameLength()); + key.size(), max_width_); + return absl::string_view(key.data(), max_width_); } + return key; +} + + +void RawStatData::initialize(absl::string_view key) { + ASSERT(!initialized()); ref_count_ = 1; // key is not necessarily nul-terminated, but we want to make sure name_ is. @@ -482,5 +452,8 @@ GaugeSharedPtr StatDataAllocatorImpl::makeGauge(const std::string& nam std::move(tags)); } +template class StatDataAllocatorImpl; +template class StatDataAllocatorImpl; + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index cde4cf8d5c50..62651e48dcee 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -307,6 +307,11 @@ class MetricImpl : public virtual Metric { template class StatDataAllocatorImpl : public StatDataAllocator { public: + static const uint64_t UNLIMITED_WIDTH = 0; + + StatDataAllocatorImpl() : max_width_(UNLIMITED_WIDTH) {} + explicit StatDataAllocatorImpl(uint64_t max_width) : max_width_(max_width) {} + // StatDataAllocator CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name, std::vector&& tags) override; @@ -328,6 +333,11 @@ template class StatDataAllocatorImpl : public StatDataAllocator * @param data the data returned by alloc(). */ virtual void free(StatData& data) PURE; + + absl::string_view truncateStatName(absl::string_view stat_name); + +private: + const uint64_t max_width_; }; using RawStatDataAllocator = StatDataAllocatorImpl; @@ -397,7 +407,7 @@ class SourceImpl : public Source { * so that it can be allocated efficiently from the heap on demand. */ struct HeapStatData { - HeapStatData(absl::string_view key) : name_(std::string(key)) {} + explicit HeapStatData(absl::string_view key); /** * Returns the name as a string_view. This is required by BlockMemoryHashSet. @@ -412,46 +422,17 @@ struct HeapStatData { std::string name_; }; -/** - * Implementation of StatDataAllocator that uses an unordered set to store - * RawStatData pointers that are allocated on the heap. This is mainly used - * for testing to exercise some of the hot-restart code in unit tests. - */ -class HeapRawStatDataAllocator : public StatDataAllocatorImpl { -public: - // StatDataAllocator - ~HeapRawStatDataAllocator() { ASSERT(stats_.empty()); } - RawStatData* alloc(const std::string& name) override; - void free(RawStatData& data) override; - -private: - struct RawStatDataHash_ { - size_t operator()(const RawStatData* a) const { return HashUtil::xxHash64(a->key()); } - }; - struct RawStatDataCompare_ { - bool operator()(const RawStatData* a, const RawStatData* b) const { - return (a->key() == b->key()); - } - }; - typedef std::unordered_set StringRawDataSet; - - // An unordered set of RawStatData pointers which keys off the key() - // field in each object. This necessitates a custom comparator and hasher. - StringRawDataSet stats_ GUARDED_BY(mutex_); - // A mutex is needed here to protect the stats_ object from both alloc() and free() operations. - // Although alloc() operations are called under existing locking, free() operations are made from - // the destructors of the individual stat objects, which are not protected by locks. - Thread::MutexBasicLockable mutex_; -}; - /** * Implementation of StatDataAllocator using a pure heap-based strategy, so that * Envoy implementations that do not require hot-restart can use less memory. */ class HeapStatDataAllocator : public StatDataAllocatorImpl { public: - // StatDataAllocator + explicit HeapStatDataAllocator(uint64_t max_width) : StatDataAllocatorImpl(max_width) {} + HeapStatDataAllocator() {} ~HeapStatDataAllocator() { ASSERT(stats_.empty()); } + + // StatDataAllocator HeapStatData* alloc(const std::string& name) override; void free(HeapStatData& data) override; diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index e841c9183244..997403b4bc07 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -115,7 +115,9 @@ std::string SharedMemory::version(uint64_t max_num_stats, uint64_t max_stat_name } HotRestartImpl::HotRestartImpl(Options& options) - : options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), + : Stats::RawStatDataAllocator(options.maxObjNameLength() + + Stats::RawStatData::maxStatSuffixLength()), + options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), shmem_(SharedMemory::initialize(RawStatDataSet::numBytes(stats_set_options_), options)), log_lock_(shmem_.log_lock_), access_log_lock_(shmem_.access_log_lock_), stat_lock_(shmem_.stat_lock_), init_lock_(shmem_.init_lock_) { @@ -142,7 +144,7 @@ HotRestartImpl::HotRestartImpl(Options& options) Stats::RawStatData* HotRestartImpl::alloc(const std::string& name) { // Try to find the existing slot in shared memory, otherwise allocate a new one. Thread::LockGuard lock(stat_lock_); - absl::string_view key = name; + absl::string_view key = truncateStatName(name); if (key.size() > Stats::RawStatData::maxNameLength()) { key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength()); } diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index e71b6065e3fe..95d1c3429571 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -478,32 +478,17 @@ TEST(TagProducerTest, CheckConstructor) { } // Validate truncation behavior of RawStatData. +// Note: a similar test using RawStatData* is in test/server/hot_restart_impl_test.cc. TEST(RawStatDataTest, Truncate) { - HeapRawStatDataAllocator alloc; + HeapStatDataAllocator alloc(RawStatData::maxNameLength()); const std::string long_string(RawStatData::maxNameLength() + 1, 'A'); - RawStatData* stat{}; + HeapStatData* stat{}; EXPECT_LOG_CONTAINS("warning", "is too long with", stat = alloc.alloc(long_string)); alloc.free(*stat); } +// Note: a similar test using RawStatData* is in test/server/hot_restart_impl_test.cc. TEST(RawStatDataTest, HeapAlloc) { - HeapRawStatDataAllocator alloc; - RawStatData* stat_1 = alloc.alloc("ref_name"); - ASSERT_NE(stat_1, nullptr); - RawStatData* stat_2 = alloc.alloc("ref_name"); - ASSERT_NE(stat_2, nullptr); - RawStatData* stat_3 = alloc.alloc("not_ref_name"); - ASSERT_NE(stat_3, nullptr); - EXPECT_EQ(stat_1, stat_2); - EXPECT_NE(stat_1, stat_3); - EXPECT_NE(stat_2, stat_3); - alloc.free(*stat_1); - alloc.free(*stat_2); - alloc.free(*stat_3); -} - -// Same test as RawStatDataTest, but with HeapStatData. Reference-counting should work here too. -TEST(HeapStatDataTest, HeapAlloc) { HeapStatDataAllocator alloc; HeapStatData* stat_1 = alloc.alloc("ref_name"); ASSERT_NE(stat_1, nullptr); diff --git a/test/integration/server.cc b/test/integration/server.cc index ed0f40191beb..55639c1110aa 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -91,7 +91,7 @@ void IntegrationTestServer::threadRoutine(const Network::Address::IpVersion vers Thread::MutexBasicLockable lock; ThreadLocal::InstanceImpl tls; - Stats::HeapRawStatDataAllocator stats_allocator; + Stats::HeapStatDataAllocator stats_allocator; Stats::ThreadLocalStoreImpl stats_store(stats_allocator); stat_store_ = &stats_store; Runtime::RandomGeneratorPtr random_generator; diff --git a/test/server/BUILD b/test/server/BUILD index 723c30ef32d0..535248d18f1b 100644 --- a/test/server/BUILD +++ b/test/server/BUILD @@ -73,6 +73,7 @@ envoy_cc_test( "//source/common/stats:stats_lib", "//source/server:hot_restart_lib", "//test/mocks/server:server_mocks", + "//test/test_common:logging_lib", "//test/test_common:threadsafe_singleton_injector_lib", ], ) diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index bda4509117b8..a5f389ea4c8c 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -5,6 +5,7 @@ #include "test/mocks/api/mocks.h" #include "test/mocks/server/mocks.h" +#include "test/test_common/logging.h" #include "test/test_common/threadsafe_singleton_injector.h" #include "absl/strings/match.h" @@ -94,6 +95,32 @@ TEST_F(HotRestartImplTest, versionString) { } } +TEST_F(HotRestartImplTest, RawTruncate) { + setup(); + + const std::string long_string(Stats::RawStatData::maxNameLength() + 1, 'A'); + Stats::RawStatData* stat{}; + EXPECT_LOG_CONTAINS("warning", "is too long with", stat = hot_restart_->alloc(long_string)); + hot_restart_->free(*stat); +} + +TEST_F(HotRestartImplTest, RawAlloc) { + setup(); + + Stats::RawStatData* stat_1 = hot_restart_->alloc("ref_name"); + ASSERT_NE(stat_1, nullptr); + Stats::RawStatData* stat_2 = hot_restart_->alloc("ref_name"); + ASSERT_NE(stat_2, nullptr); + Stats::RawStatData* stat_3 = hot_restart_->alloc("not_ref_name"); + ASSERT_NE(stat_3, nullptr); + EXPECT_EQ(stat_1, stat_2); + EXPECT_NE(stat_1, stat_3); + EXPECT_NE(stat_2, stat_3); + hot_restart_->free(*stat_1); + hot_restart_->free(*stat_2); + hot_restart_->free(*stat_3); +} + TEST_F(HotRestartImplTest, crossAlloc) { setup(); diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index bf61d9bde248..0a40e843532d 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -662,7 +662,7 @@ class PrometheusStatsFormatterTest : public testing::Test { gauges_.push_back(alloc_.makeGauge(name, std::move(tname), std::move(cluster_tags))); } - Stats::HeapRawStatDataAllocator alloc_; + Stats::HeapStatDataAllocator alloc_; std::vector counters_; std::vector gauges_; }; From d427401459e8de4e76f22d83cce114ec2ca3b133 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 24 Jun 2018 22:26:55 -0400 Subject: [PATCH 06/28] remove superfluous blank linke. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index fb32dbe2ee62..d29224a6e1f7 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -359,7 +359,6 @@ absl::string_view StatDataAllocatorImpl::truncateStatName(absl::string return key; } - void RawStatData::initialize(absl::string_view key) { ASSERT(!initialized()); ref_count_ = 1; From 343eae5add1681b6099dbdd4a6505ff2c9edf1ac Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Jul 2018 03:54:56 -0400 Subject: [PATCH 07/28] Checkpointing all tests build (2 fail) Signed-off-by: Joshua Marantz --- include/envoy/stats/stats.h | 2 + source/common/stats/stats_impl.cc | 7 +- source/common/stats/stats_impl.h | 21 ++-- source/common/stats/thread_local_store.cc | 5 +- source/common/stats/thread_local_store.h | 3 +- source/exe/main_common.cc | 5 +- source/server/hot_restart_impl.cc | 10 +- source/server/hot_restart_impl.h | 1 - source/server/hot_restart_nop_impl.h | 2 +- test/common/stats/stats_impl_test.cc | 18 ++-- test/common/stats/thread_local_store_test.cc | 101 ++++++++----------- test/integration/server.cc | 8 +- test/mocks/server/mocks.cc | 2 +- test/mocks/server/mocks.h | 2 + test/server/hot_restart_impl_test.cc | 2 +- test/server/http/admin_test.cc | 30 ++---- test/test_common/utility.h | 25 +++++ 17 files changed, 123 insertions(+), 121 deletions(-) diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 31d0966d0cac..ee907bdfb56f 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -480,6 +480,8 @@ class StatDataAllocator { virtual GaugeSharedPtr makeGauge(const std::string& name, std::string&& tag_extracted_name, std::vector&& tags) PURE; + virtual const StatsOptions& statsOptions() const PURE; + // TODO(jmarantz): create a parallel mechanism to instantiate histograms. At // the moment, histograms don't fit the same pattern of counters and gaugaes // as they are not actually created in the context of a stats allocator. diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 605070348f98..2be8cf2b4aec 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -342,12 +342,13 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon template absl::string_view StatDataAllocatorImpl::truncateStatName(absl::string_view key) { - if ((max_width_ != UNLIMITED_WIDTH) && (key.size() > max_width_)) { + const uint64_t max_width = options_.maxNameLength(); + if (/*(max_width_ != UNLIMITED_WIDTH) &&*/ (key.size() > max_width)) { ENVOY_LOG_MISC( warn, "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, - key.size(), max_width_); - return absl::string_view(key.data(), max_width_); + key.size(), options_.maxNameLength()); + return absl::string_view(key.data(), max_width); } return key; } diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index c46dadc8cc36..ba0d948980b2 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -285,10 +285,10 @@ class MetricImpl : public virtual Metric { template class StatDataAllocatorImpl : public StatDataAllocator { public: - static const uint64_t UNLIMITED_WIDTH = 0; + //static const uint64_t UNLIMITED_WIDTH = 0; - StatDataAllocatorImpl() : max_width_(UNLIMITED_WIDTH) {} - explicit StatDataAllocatorImpl(uint64_t max_width) : max_width_(max_width) {} + //StatDataAllocatorImpl() : max_width_(UNLIMITED_WIDTH) {} + explicit StatDataAllocatorImpl(const StatsOptions& options) : options_(options) {} // StatDataAllocator CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name, @@ -313,9 +313,10 @@ template class StatDataAllocatorImpl : public StatDataAllocator virtual void free(StatData& data) PURE; absl::string_view truncateStatName(absl::string_view stat_name); + const StatsOptions& statsOptions() const override { return options_; } private: - const uint64_t max_width_; + const StatsOptions& options_; }; using RawStatDataAllocator = StatDataAllocatorImpl; @@ -406,8 +407,9 @@ struct HeapStatData { */ class HeapStatDataAllocator : public StatDataAllocatorImpl { public: - explicit HeapStatDataAllocator(uint64_t max_width) : StatDataAllocatorImpl(max_width) {} - HeapStatDataAllocator() {} + explicit HeapStatDataAllocator(const StatsOptions& options) + : StatDataAllocatorImpl(options) {} + //HeapStatDataAllocator() {} ~HeapStatDataAllocator() { ASSERT(stats_.empty()); } // StatDataAllocator @@ -482,8 +484,9 @@ template class IsolatedStatsCache { */ class IsolatedStoreImpl : public Store { public: - IsolatedStoreImpl() - : counters_([this](const std::string& name) -> CounterSharedPtr { + /*explicit*/ IsolatedStoreImpl(/*Stats::StatsOptions& stats_options*/) + : alloc_(stats_options_), + counters_([this](const std::string& name) -> CounterSharedPtr { std::string tag_extracted_name = name; std::vector tags; return alloc_.makeCounter(name, std::move(tag_extracted_name), std::move(tags)); @@ -539,11 +542,11 @@ class IsolatedStoreImpl : public Store { const std::string prefix_; }; + StatsOptionsImpl stats_options_; HeapStatDataAllocator alloc_; IsolatedStatsCache counters_; IsolatedStatsCache gauges_; IsolatedStatsCache histograms_; - const Stats::StatsOptionsImpl stats_options_; }; } // namespace Stats diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 7e6e448e63c8..478b53b7ba5e 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -14,9 +14,10 @@ namespace Stats { ThreadLocalStoreImpl::ThreadLocalStoreImpl(const Stats::StatsOptions& stats_options, StatDataAllocator& alloc) - : stats_options_(stats_options), alloc_(alloc), default_scope_(createScope("")), + : alloc_(alloc), default_scope_(createScope("")), tag_producer_(std::make_unique()), - num_last_resort_stats_(default_scope_->counter("stats.overflow")), source_(*this) {} + num_last_resort_stats_(default_scope_->counter("stats.overflow")), + heap_allocator_(stats_options), source_(*this) {} ThreadLocalStoreImpl::~ThreadLocalStoreImpl() { ASSERT(shutting_down_); diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index d0ed1fcfe01a..2503e5aaa18c 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -195,7 +195,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo Source& source() override { return source_; } - const Stats::StatsOptions& statsOptions() const override { return stats_options_; } + const Stats::StatsOptions& statsOptions() const override { return alloc_.statsOptions(); } private: struct TlsCacheEntry { @@ -275,7 +275,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void releaseScopeCrossThread(ScopeImpl* scope); void mergeInternal(PostMergeCb mergeCb); - const Stats::StatsOptions& stats_options_; StatDataAllocator& alloc_; Event::Dispatcher* main_thread_dispatcher_{}; ThreadLocal::SlotPtr tls_; diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 862c0b95c740..6118130ec663 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -42,6 +42,7 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) { ares_library_init(ARES_LIB_INIT_ALL); Event::Libevent::Global::initialize(); RELEASE_ASSERT(Envoy::Server::validateProtoDescriptors(), ""); + const Stats::StatsOptions& stats_options = options_.statsOptions(); switch (options_.mode()) { case Server::Mode::InitOnly: @@ -52,7 +53,7 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) { } #endif if (restarter_.get() == nullptr) { - restarter_.reset(new Server::HotRestartNopImpl()); + restarter_.reset(new Server::HotRestartNopImpl(stats_options)); } tls_.reset(new ThreadLocal::InstanceImpl); @@ -69,7 +70,7 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) { break; } case Server::Mode::Validate: - restarter_.reset(new Server::HotRestartNopImpl()); + restarter_.reset(new Server::HotRestartNopImpl(stats_options)); Logger::Registry::initialize(options_.logLevel(), options_.logFormat(), restarter_->logLock()); break; } diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index 6b4854e80e0a..01322834282b 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -116,15 +116,13 @@ std::string SharedMemory::version(uint64_t max_num_stats, } HotRestartImpl::HotRestartImpl(Options& options) - : Stats::RawStatDataAllocator(options.maxObjNameLength() + - Stats::RawStatData::maxStatSuffixLength()), - options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), + : Stats::RawStatDataAllocator(options.statsOptions()), + /*options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), shmem_(SharedMemory::initialize(RawStatDataSet::numBytes(stats_set_options_), options)), - /* - : options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), + */ + options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), shmem_(SharedMemory::initialize( RawStatDataSet::numBytes(stats_set_options_, options_.statsOptions()), options_)), -*/ log_lock_(shmem_.log_lock_), access_log_lock_(shmem_.access_log_lock_), stat_lock_(shmem_.stat_lock_), init_lock_(shmem_.init_lock_) { { diff --git a/source/server/hot_restart_impl.h b/source/server/hot_restart_impl.h index 39f537d55523..5524d0ba3a5c 100644 --- a/source/server/hot_restart_impl.h +++ b/source/server/hot_restart_impl.h @@ -138,7 +138,6 @@ class HotRestartImpl : public HotRestart, */ static std::string hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len); - // RawStatDataAllocator Stats::RawStatData* alloc(const std::string& name) override; void free(Stats::RawStatData& data) override; diff --git a/source/server/hot_restart_nop_impl.h b/source/server/hot_restart_nop_impl.h index 8444dea459a4..6dd94966015b 100644 --- a/source/server/hot_restart_nop_impl.h +++ b/source/server/hot_restart_nop_impl.h @@ -14,7 +14,7 @@ namespace Server { */ class HotRestartNopImpl : public Server::HotRestart { public: - HotRestartNopImpl() {} + HotRestartNopImpl(const Stats::StatsOptions& options) : stats_allocator_(options) {} // Server::HotRestart void drainParentListeners() override {} diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index cf6ec191e2ba..c36018c49030 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -481,8 +481,9 @@ TEST(TagProducerTest, CheckConstructor) { // Validate truncation behavior of RawStatData. // Note: a similar test using RawStatData* is in test/server/hot_restart_impl_test.cc. TEST(RawStatDataTest, Truncate) { - HeapStatDataAllocator alloc(RawStatData::maxNameLength()); - const std::string long_string(RawStatData::maxNameLength() + 1, 'A'); + Stats::StatsOptionsImpl stats_options; + HeapStatDataAllocator alloc(stats_options); + const std::string long_string(stats_options.maxNameLength() + 1, 'A'); HeapStatData* stat{}; EXPECT_LOG_CONTAINS("warning", "is too long with", stat = alloc.alloc(long_string)); alloc.free(*stat); @@ -490,16 +491,16 @@ TEST(RawStatDataTest, Truncate) { // Check consistency of internal stat representation TEST(RawStatDataTest, Consistency) { - HeapRawStatDataAllocator alloc; + Stats::StatsOptionsImpl stats_options; + HeapStatDataAllocator alloc(stats_options); // Generate a stat, encode it to hex, and take the hash of that hex string. We expect the hash to // vary only when the internal representation of a stat has been intentionally changed, in which // case SharedMemory::VERSION should be incremented as well. uint64_t expected_hash = 1874506077228772558; - Stats::StatsOptionsImpl stats_options; uint64_t max_name_length = stats_options.maxNameLength(); const std::string name_1(max_name_length, 'A'); - RawStatData* stat_1 = alloc.alloc(name_1); + HeapStatData* stat_1 = alloc.alloc(name_1); std::string stat_hex_dump_1 = Hex::encode(reinterpret_cast(stat_1), sizeof(RawStatData) + max_name_length); EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_1), expected_hash); @@ -508,7 +509,7 @@ TEST(RawStatDataTest, Consistency) { // If a stat name is truncated, we expect that its internal representation is the same as if it // had been initialized with the already-truncated name. const std::string name_2(max_name_length + 1, 'A'); - RawStatData* stat_2 = alloc.alloc(name_2); + HeapStatData* stat_2 = alloc.alloc(name_2); std::string stat_hex_dump_2 = Hex::encode(reinterpret_cast(stat_2), sizeof(RawStatData) + max_name_length); EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_2), expected_hash); @@ -517,7 +518,8 @@ TEST(RawStatDataTest, Consistency) { // Note: a similar test using RawStatData* is in test/server/hot_restart_impl_test.cc. TEST(RawStatDataTest, HeapAlloc) { - HeapStatDataAllocator alloc; + Stats::StatsOptionsImpl stats_options; + HeapStatDataAllocator alloc(stats_options); HeapStatData* stat_1 = alloc.alloc("ref_name"); ASSERT_NE(stat_1, nullptr); HeapStatData* stat_2 = alloc.alloc("ref_name"); @@ -532,7 +534,7 @@ TEST(RawStatDataTest, HeapAlloc) { alloc.free(*stat_3); } -TEST(RawStatDataTest, Truncate) { +TEST(RawStatDataTest, TruncateRaw) { // RawStatData::truncateAndInit(absl::string_view key, const StatsOptions& stats_options) will // truncate and log to ENVOY_LOG_MISC if given a key longer than the allowed // stats_options.maxNameLength(). This mechanism is also tested in HotRestartImplTest.truncateKey. diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 98de7d312d0a..3225f4a37d37 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -26,48 +26,29 @@ using testing::_; namespace Envoy { namespace Stats { -class StatsThreadLocalStoreTest : public testing::Test, public RawStatDataAllocator { +class StatsThreadLocalStoreTest : public testing::Test { public: - StatsThreadLocalStoreTest() { - ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](const std::string& name) -> RawStatData* { - return alloc_.alloc(name); - })); - - ON_CALL(*this, free(_)).WillByDefault(Invoke([this](RawStatData& data) -> void { - return alloc_.free(data); - })); - - EXPECT_CALL(*this, alloc("stats.overflow")); - store_ = std::make_unique(options_, *this); + StatsThreadLocalStoreTest() : alloc_(options_) { + store_ = std::make_unique(options_, alloc_); store_->addSink(sink_); } - MOCK_METHOD1(alloc, RawStatData*(const std::string& name)); - MOCK_METHOD1(free, void(RawStatData& data)); - NiceMock main_thread_dispatcher_; NiceMock tls_; - Stats::StatsOptionsImpl options_; - TestAllocator alloc_; + StatsOptionsImpl options_; + MockedTestAllocator alloc_; MockSink sink_; std::unique_ptr store_; }; -class HistogramTest : public testing::Test, public RawStatDataAllocator { +class HistogramTest : public testing::Test { public: - typedef std::map NameHistogramMap; - - void SetUp() override { - ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](const std::string& name) -> RawStatData* { - return alloc_.alloc(name); - })); + typedef std::map NameHistogramMap; - ON_CALL(*this, free(_)).WillByDefault(Invoke([this](RawStatData& data) -> void { - return alloc_.free(data); - })); + HistogramTest() : alloc_(options_) {} - EXPECT_CALL(*this, alloc("stats.overflow")); - store_ = std::make_unique(options_, *this); + void SetUp() override { + store_ = std::make_unique(options_, alloc_); store_->addSink(sink_); store_->initializeThreading(main_thread_dispatcher_, tls_); } @@ -76,12 +57,12 @@ class HistogramTest : public testing::Test, public RawStatDataAllocator { store_->shutdownThreading(); tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*this, free(_)); + EXPECT_CALL(alloc_, free(_)); } NameHistogramMap makeHistogramMap(const std::vector& hist_list) { NameHistogramMap name_histogram_map; - for (const Stats::ParentHistogramSharedPtr& histogram : hist_list) { + for (const ParentHistogramSharedPtr& histogram : hist_list) { // Exclude the scope part of the name. const std::vector& split_vector = absl::StrSplit(histogram->name(), '.'); name_histogram_map.insert(std::make_pair(split_vector.back(), histogram)); @@ -112,12 +93,12 @@ class HistogramTest : public testing::Test, public RawStatDataAllocator { HistogramStatisticsImpl h2_interval_statistics(hist2_interval); NameHistogramMap name_histogram_map = makeHistogramMap(histogram_list); - const Stats::ParentHistogramSharedPtr& h1 = name_histogram_map["h1"]; + const ParentHistogramSharedPtr& h1 = name_histogram_map["h1"]; EXPECT_EQ(h1->cumulativeStatistics().summary(), h1_cumulative_statistics.summary()); EXPECT_EQ(h1->intervalStatistics().summary(), h1_interval_statistics.summary()); if (histogram_list.size() > 1) { - const Stats::ParentHistogramSharedPtr& h2 = name_histogram_map["h2"]; + const ParentHistogramSharedPtr& h2 = name_histogram_map["h2"]; EXPECT_EQ(h2->cumulativeStatistics().summary(), h2_cumulative_statistics.summary()); EXPECT_EQ(h2->intervalStatistics().summary(), h2_interval_statistics.summary()); } @@ -159,8 +140,8 @@ class HistogramTest : public testing::Test, public RawStatDataAllocator { NiceMock main_thread_dispatcher_; NiceMock tls_; - Stats::StatsOptionsImpl options_; - TestAllocator alloc_; + StatsOptionsImpl options_; + MockedTestAllocator alloc_; MockSink sink_; std::unique_ptr store_; InSequence s; @@ -170,7 +151,7 @@ class HistogramTest : public testing::Test, public RawStatDataAllocator { TEST_F(StatsThreadLocalStoreTest, NoTls) { InSequence s; - EXPECT_CALL(*this, alloc(_)).Times(2); + EXPECT_CALL(alloc_, alloc(_)).Times(2); Counter& c1 = store_->counter("c1"); EXPECT_EQ(&c1, &store_->counter("c1")); @@ -194,7 +175,7 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) { EXPECT_EQ(2L, store_->gauges().front().use_count()); // Includes overflow stat. - EXPECT_CALL(*this, free(_)).Times(3); + EXPECT_CALL(alloc_, free(_)).Times(3); store_->shutdownThreading(); } @@ -203,7 +184,7 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(*this, alloc(_)).Times(2); + EXPECT_CALL(alloc_, alloc(_)).Times(2); Counter& c1 = store_->counter("c1"); EXPECT_EQ(&c1, &store_->counter("c1")); @@ -232,7 +213,7 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { EXPECT_EQ(2L, store_->gauges().front().use_count()); // Includes overflow stat. - EXPECT_CALL(*this, free(_)).Times(3); + EXPECT_CALL(alloc_, free(_)).Times(3); } TEST_F(StatsThreadLocalStoreTest, BasicScope) { @@ -240,7 +221,7 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope("scope1."); - EXPECT_CALL(*this, alloc(_)).Times(4); + EXPECT_CALL(alloc_, alloc(_)).Times(4); Counter& c1 = store_->counter("c1"); Counter& c2 = scope1->counter("c2"); EXPECT_EQ("c1", c1.name()); @@ -266,7 +247,7 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*this, free(_)).Times(5); + EXPECT_CALL(alloc_, free(_)).Times(5); } // Validate that we sanitize away bad characters in the stats prefix. @@ -275,7 +256,7 @@ TEST_F(StatsThreadLocalStoreTest, SanitizePrefix) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope(std::string("scope1:\0:foo.", 13)); - EXPECT_CALL(*this, alloc(_)); + EXPECT_CALL(alloc_, alloc(_)); Counter& c1 = scope1->counter("c1"); EXPECT_EQ("scope1___foo.c1", c1.name()); @@ -283,7 +264,7 @@ TEST_F(StatsThreadLocalStoreTest, SanitizePrefix) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*this, free(_)).Times(2); + EXPECT_CALL(alloc_, free(_)).Times(2); } TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { @@ -291,7 +272,7 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope("scope1."); - EXPECT_CALL(*this, alloc(_)); + EXPECT_CALL(alloc_, alloc(_)); scope1->counter("c1"); EXPECT_EQ(2UL, store_->counters().size()); CounterSharedPtr c1 = store_->counters().front(); @@ -306,7 +287,7 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { store_->source().clearCache(); EXPECT_EQ(1UL, store_->source().cachedCounters().size()); - EXPECT_CALL(*this, free(_)); + EXPECT_CALL(alloc_, free(_)); EXPECT_EQ(1L, c1.use_count()); c1.reset(); @@ -314,7 +295,7 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*this, free(_)); + EXPECT_CALL(alloc_, free(_)); } TEST_F(StatsThreadLocalStoreTest, NestedScopes) { @@ -322,12 +303,12 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope("scope1."); - EXPECT_CALL(*this, alloc(_)); + EXPECT_CALL(alloc_, alloc(_)); Counter& c1 = scope1->counter("foo.bar"); EXPECT_EQ("scope1.foo.bar", c1.name()); ScopePtr scope2 = scope1->createScope("foo."); - EXPECT_CALL(*this, alloc(_)); + EXPECT_CALL(alloc_, alloc(_)); Counter& c2 = scope2->counter("bar"); EXPECT_NE(&c1, &c2); EXPECT_EQ("scope1.foo.bar", c2.name()); @@ -337,7 +318,7 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) { EXPECT_EQ(1UL, c1.value()); EXPECT_EQ(c1.value(), c2.value()); - EXPECT_CALL(*this, alloc(_)); + EXPECT_CALL(alloc_, alloc(_)); Gauge& g1 = scope2->gauge("some_gauge"); EXPECT_EQ("scope1.foo.some_gauge", g1.name()); @@ -345,7 +326,7 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*this, free(_)).Times(4); + EXPECT_CALL(alloc_, free(_)).Times(4); } TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { @@ -358,7 +339,7 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { ScopePtr scope2 = store_->createScope("scope1."); // We will call alloc twice, but they should point to the same backing storage. - EXPECT_CALL(*this, alloc(_)).Times(2); + EXPECT_CALL(alloc_, alloc(_)).Times(2); Counter& c1 = scope1->counter("c"); Counter& c2 = scope2->counter("c"); EXPECT_NE(&c1, &c2); @@ -373,7 +354,7 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { EXPECT_EQ(2UL, store_->counters().size()); // Gauges should work the same way. - EXPECT_CALL(*this, alloc(_)).Times(2); + EXPECT_CALL(alloc_, alloc(_)).Times(2); Gauge& g1 = scope1->gauge("g"); Gauge& g2 = scope2->gauge("g"); EXPECT_NE(&g1, &g2); @@ -386,7 +367,7 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { EXPECT_EQ(1UL, store_->gauges().size()); // Deleting scope 1 will call free but will be reference counted. It still leaves scope 2 valid. - EXPECT_CALL(*this, free(_)).Times(2); + EXPECT_CALL(alloc_, free(_)).Times(2); scope1.reset(); c2.inc(); EXPECT_EQ(3UL, c2.value()); @@ -399,14 +380,14 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*this, free(_)).Times(3); + EXPECT_CALL(alloc_, free(_)).Times(3); } TEST_F(StatsThreadLocalStoreTest, AllocFailed) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(*this, alloc("foo")).WillOnce(Return(nullptr)); + EXPECT_CALL(alloc_, alloc("foo")).WillOnce(Return(nullptr)); Counter& c1 = store_->counter("foo"); EXPECT_EQ(1UL, store_->counter("stats.overflow").value()); @@ -417,14 +398,14 @@ TEST_F(StatsThreadLocalStoreTest, AllocFailed) { tls_.shutdownThread(); // Includes overflow but not the failsafe stat which we allocated from the heap. - EXPECT_CALL(*this, free(_)); + EXPECT_CALL(alloc_, free(_)); } TEST_F(StatsThreadLocalStoreTest, ShuttingDown) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(*this, alloc(_)).Times(4); + EXPECT_CALL(alloc_, alloc(_)).Times(4); store_->counter("c1"); store_->gauge("g1"); store_->shutdownThreading(); @@ -440,7 +421,7 @@ TEST_F(StatsThreadLocalStoreTest, ShuttingDown) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(*this, free(_)).Times(5); + EXPECT_CALL(alloc_, free(_)).Times(5); } TEST_F(StatsThreadLocalStoreTest, MergeDuringShutDown) { @@ -463,7 +444,7 @@ TEST_F(StatsThreadLocalStoreTest, MergeDuringShutDown) { tls_.shutdownThread(); - EXPECT_CALL(*this, free(_)); + EXPECT_CALL(alloc_, free(_)); } // Histogram tests @@ -610,7 +591,7 @@ TEST_F(HistogramTest, BasicHistogramUsed) { // Merge histograms again and validate that both h1 and h2 are used. store_->mergeHistograms([]() -> void {}); - for (const Stats::ParentHistogramSharedPtr& histogram : store_->histograms()) { + for (const ParentHistogramSharedPtr& histogram : store_->histograms()) { EXPECT_TRUE(histogram->used()); } } diff --git a/test/integration/server.cc b/test/integration/server.cc index ab9342cdcf85..2fe7854d81bd 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -88,13 +88,13 @@ void IntegrationTestServer::onWorkerListenerRemoved() { void IntegrationTestServer::threadRoutine(const Network::Address::IpVersion version, bool deterministic) { Server::TestOptionsImpl options(config_path_, version); - Server::HotRestartNopImpl restarter; + Stats::StatsOptionsImpl stats_options; + Server::HotRestartNopImpl restarter(stats_options); Thread::MutexBasicLockable lock; ThreadLocal::InstanceImpl tls; - Stats::HeapStatDataAllocator stats_allocator; - Stats::StatsOptionsImpl options_; - Stats::ThreadLocalStoreImpl stats_store(options_, stats_allocator); + Stats::HeapStatDataAllocator stats_allocator(stats_options); + Stats::ThreadLocalStoreImpl stats_store(stats_options, stats_allocator); stat_store_ = &stats_store; Runtime::RandomGeneratorPtr random_generator; if (deterministic) { diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 46920ab56429..515a010fd8fb 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -61,7 +61,7 @@ MockGuardDog::MockGuardDog() : watch_dog_(new NiceMock()) { } MockGuardDog::~MockGuardDog() {} -MockHotRestart::MockHotRestart() { +MockHotRestart::MockHotRestart() : stats_allocator_(stats_options_) { ON_CALL(*this, logLock()).WillByDefault(ReturnRef(log_lock_)); ON_CALL(*this, accessLogLock()).WillByDefault(ReturnRef(access_log_lock_)); ON_CALL(*this, statsAllocator()).WillByDefault(ReturnRef(stats_allocator_)); diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 83fbea4dcfbb..a91ff8dd1333 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -1,3 +1,4 @@ + #pragma once #include @@ -178,6 +179,7 @@ class MockHotRestart : public HotRestart { private: Thread::MutexBasicLockable log_lock_; Thread::MutexBasicLockable access_log_lock_; + Stats::StatsOptionsImpl stats_options_; Stats::HeapStatDataAllocator stats_allocator_; }; diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 9db30229ae6a..0abcab61b4c7 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -92,7 +92,7 @@ TEST_F(HotRestartImplTest, versionString) { TEST_F(HotRestartImplTest, RawTruncate) { setup(); - const std::string long_string(Stats::RawStatData::maxNameLength() + 1, 'A'); + const std::string long_string(hot_restart_->statsOptions().maxNameLength() + 1, 'A'); Stats::RawStatData* stat{}; EXPECT_LOG_CONTAINS("warning", "is too long with", stat = hot_restart_->alloc(long_string)); hot_restart_->free(*stat); diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 4fedcdf94996..aa0ea1e86f0b 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -39,21 +39,10 @@ using testing::_; namespace Envoy { namespace Server { -class AdminStatsTest : public testing::TestWithParam, - public Stats::RawStatDataAllocator { +class AdminStatsTest : public testing::TestWithParam { public: -public: - AdminStatsTest() { - ON_CALL(*this, alloc(_)) - .WillByDefault(Invoke( - [this](const std::string& name) -> Stats::RawStatData* { return alloc_.alloc(name); })); - - ON_CALL(*this, free(_)).WillByDefault(Invoke([this](Stats::RawStatData& data) -> void { - return alloc_.free(data); - })); - - EXPECT_CALL(*this, alloc("stats.overflow")); - store_ = std::make_unique(options_, *this); + AdminStatsTest() : alloc_(options_) { + store_ = std::make_unique(options_, alloc_); store_->addSink(sink_); } @@ -64,14 +53,11 @@ class AdminStatsTest : public testing::TestWithParam main_thread_dispatcher_; NiceMock tls_; - Stats::TestAllocator alloc_; - Stats::MockSink sink_; Stats::StatsOptionsImpl options_; + Stats::MockedTestAllocator alloc_; + Stats::MockSink sink_; std::unique_ptr store_; }; @@ -121,7 +107,7 @@ TEST_P(AdminStatsTest, StatsAsJson) { store_->mergeHistograms([]() -> void {}); - EXPECT_CALL(*this, free(_)); + EXPECT_CALL(alloc_, free(_)); std::map all_stats; @@ -257,7 +243,7 @@ TEST_P(AdminStatsTest, UsedOnlyStatsAsJson) { store_->mergeHistograms([]() -> void {}); - EXPECT_CALL(*this, free(_)); + EXPECT_CALL(alloc_, free(_)); std::map all_stats; @@ -795,6 +781,7 @@ TEST_P(AdminInstanceTest, PostRequest) { class PrometheusStatsFormatterTest : public testing::Test { protected: + PrometheusStatsFormatterTest() : alloc_(stats_options_) {} void addCounter(const std::string& name, std::vector cluster_tags) { std::string tname = std::string(name); counters_.push_back(alloc_.makeCounter(name, std::move(tname), std::move(cluster_tags))); @@ -805,6 +792,7 @@ class PrometheusStatsFormatterTest : public testing::Test { gauges_.push_back(alloc_.makeGauge(name, std::move(tname), std::move(cluster_tags))); } + Stats::StatsOptionsImpl stats_options_; Stats::HeapStatDataAllocator alloc_; std::vector counters_; std::vector gauges_; diff --git a/test/test_common/utility.h b/test/test_common/utility.h index c972c9ad407c..b1ba7987ad65 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -26,6 +26,8 @@ using testing::AssertionFailure; using testing::AssertionResult; using testing::AssertionSuccess; +using testing::Invoke; +using testing::_; namespace Envoy { #define EXPECT_THROW_WITH_MESSAGE(statement, expected_exception, message) \ @@ -339,6 +341,7 @@ namespace Stats { */ class TestAllocator : public RawStatDataAllocator { public: + TestAllocator(const StatsOptions& stats_options) : RawStatDataAllocator(stats_options) {} ~TestAllocator() { EXPECT_TRUE(stats_.empty()); } RawStatData* alloc(const std::string& name) override { @@ -371,6 +374,28 @@ class TestAllocator : public RawStatDataAllocator { std::unordered_map> stats_; }; +class MockedTestAllocator : public RawStatDataAllocator { +public: + MockedTestAllocator(const StatsOptions& stats_options) + : RawStatDataAllocator(stats_options), + alloc_(stats_options) { + ON_CALL(*this, alloc(_)) + .WillByDefault(Invoke( + [this](const std::string& name) -> RawStatData* { return alloc_.alloc(name); })); + + ON_CALL(*this, free(_)).WillByDefault(Invoke([this](RawStatData& data) -> void { + return alloc_.free(data); + })); + + EXPECT_CALL(*this, alloc("stats.overflow")); + } + + MOCK_METHOD1(alloc, RawStatData*(const std::string& name)); + MOCK_METHOD1(free, void(RawStatData& data)); + + TestAllocator alloc_; +}; + } // namespace Stats MATCHER_P(ProtoEq, rhs, "") { return TestUtility::protoEqual(arg, rhs); } From d809c61ffe7e2e96ee5683ccbb8e32c4b79b3467 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Jul 2018 04:47:15 -0400 Subject: [PATCH 08/28] all tests pass. Signed-off-by: Joshua Marantz --- include/envoy/stats/stats.h | 3 +++ source/server/hot_restart_impl.cc | 6 +----- test/common/stats/stats_impl_test.cc | 27 --------------------------- test/server/hot_restart_impl_test.cc | 27 +++++++++++++++++++++++++++ 4 files changed, 31 insertions(+), 32 deletions(-) diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index ee907bdfb56f..f36c4f5ef9c6 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -480,6 +480,9 @@ class StatDataAllocator { virtual GaugeSharedPtr makeGauge(const std::string& name, std::string&& tag_extracted_name, std::vector&& tags) PURE; + /** + * @return const StatsOptions& the options used to initialize the allocator. + */ virtual const StatsOptions& statsOptions() const PURE; // TODO(jmarantz): create a parallel mechanism to instantiate histograms. At diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index 01322834282b..a88e4aa19e25 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -148,11 +148,7 @@ HotRestartImpl::HotRestartImpl(Options& options) Stats::RawStatData* HotRestartImpl::alloc(const std::string& name) { // Try to find the existing slot in shared memory, otherwise allocate a new one. Thread::LockGuard lock(stat_lock_); - absl::string_view key = name; - if (key.size() > options_.statsOptions().maxNameLength()) { - key.remove_suffix(key.size() - options_.statsOptions().maxNameLength()); - } - auto value_created = stats_set_->insert(key); + auto value_created = stats_set_->insert(truncateStatName(name)); Stats::RawStatData* data = value_created.first; if (data == nullptr) { return nullptr; diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index c36018c49030..fe63383b02ee 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -489,33 +489,6 @@ TEST(RawStatDataTest, Truncate) { alloc.free(*stat); } -// Check consistency of internal stat representation -TEST(RawStatDataTest, Consistency) { - Stats::StatsOptionsImpl stats_options; - HeapStatDataAllocator alloc(stats_options); - // Generate a stat, encode it to hex, and take the hash of that hex string. We expect the hash to - // vary only when the internal representation of a stat has been intentionally changed, in which - // case SharedMemory::VERSION should be incremented as well. - uint64_t expected_hash = 1874506077228772558; - uint64_t max_name_length = stats_options.maxNameLength(); - - const std::string name_1(max_name_length, 'A'); - HeapStatData* stat_1 = alloc.alloc(name_1); - std::string stat_hex_dump_1 = - Hex::encode(reinterpret_cast(stat_1), sizeof(RawStatData) + max_name_length); - EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_1), expected_hash); - alloc.free(*stat_1); - - // If a stat name is truncated, we expect that its internal representation is the same as if it - // had been initialized with the already-truncated name. - const std::string name_2(max_name_length + 1, 'A'); - HeapStatData* stat_2 = alloc.alloc(name_2); - std::string stat_hex_dump_2 = - Hex::encode(reinterpret_cast(stat_2), sizeof(RawStatData) + max_name_length); - EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_2), expected_hash); - alloc.free(*stat_2); -} - // Note: a similar test using RawStatData* is in test/server/hot_restart_impl_test.cc. TEST(RawStatDataTest, HeapAlloc) { Stats::StatsOptionsImpl stats_options; diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 0abcab61b4c7..af1840230d2b 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -1,4 +1,5 @@ #include "common/api/os_sys_calls_impl.h" +#include "common/common/hex.h" #include "common/stats/stats_impl.h" #include "server/hot_restart_impl.h" @@ -98,6 +99,32 @@ TEST_F(HotRestartImplTest, RawTruncate) { hot_restart_->free(*stat); } +// Check consistency of internal stat representation +TEST_F(HotRestartImplTest, Consistency) { + setup(); + + // Generate a stat, encode it to hex, and take the hash of that hex string. We expect the hash to + // vary only when the internal representation of a stat has been intentionally changed, in which + // case SharedMemory::VERSION should be incremented as well. + uint64_t expected_hash = 1874506077228772558; + uint64_t max_name_length = stats_options_.maxNameLength(); + + const std::string name_1(max_name_length, 'A'); + Stats::RawStatData* stat_1 = hot_restart_->alloc(name_1); + uint64_t stat_size = sizeof(Stats::RawStatData) + max_name_length; + std::string stat_hex_dump_1 = Hex::encode(reinterpret_cast(stat_1), stat_size); + EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_1), expected_hash); + hot_restart_->free(*stat_1); + + // If a stat name is truncated, we expect that its internal representation is the same as if it + // had been initialized with the already-truncated name. + const std::string name_2(max_name_length + 1, 'A'); + Stats::RawStatData* stat_2 = hot_restart_->alloc(name_2); + std::string stat_hex_dump_2 = Hex::encode(reinterpret_cast(stat_2), stat_size); + EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_2), expected_hash); + hot_restart_->free(*stat_2); +} + TEST_F(HotRestartImplTest, RawAlloc) { setup(); From 5d3e916c42eb44d60add5e667fc3fd911dcfb846 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Jul 2018 05:53:23 -0400 Subject: [PATCH 09/28] Formatting and other cleanup. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.cc | 21 ++++-------- source/common/stats/stats_impl.h | 42 ++++++++++++++++------- source/common/stats/thread_local_store.cc | 5 ++- source/common/stats/thread_local_store.h | 2 +- source/server/hot_restart_impl.cc | 7 ++-- source/server/hot_restart_impl.h | 1 + source/server/hot_restart_nop_impl.h | 2 +- test/common/stats/stats_impl_test.cc | 8 +++-- test/mocks/server/mocks.h | 1 - test/server/hot_restart_impl_test.cc | 3 +- test/test_common/utility.h | 9 +++-- 11 files changed, 54 insertions(+), 47 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 2be8cf2b4aec..805a39d30bbf 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -139,16 +139,6 @@ HeapStatData::HeapStatData(absl::string_view key) : name_(key.data(), key.size() HeapStatData* HeapStatDataAllocator::alloc(const std::string& name) { auto data = std::make_unique(truncateStatName(name)); - /* -RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { - uint64_t num_bytes_to_allocate = RawStatData::structSize(name.size()); - RawStatData* data = static_cast(::calloc(num_bytes_to_allocate, 1)); - if (data == nullptr) { - throw std::bad_alloc(); - } - data->checkAndInit(name, num_bytes_to_allocate); - - */ Thread::ReleasableLockGuard lock(mutex_); auto ret = stats_.insert(data.get()); HeapStatData* existing_data = *ret.first; @@ -178,8 +168,9 @@ void HeapStatDataAllocator::free(HeapStatData& data) { /** * Counter implementation that wraps a StatData. StatData must have data members: - * int64 value_; - * .... + * int64_t value_; + * int64_t pending_increment_; + * int16_t flags_; */ template class CounterImpl : public Counter, public MetricImpl { public: @@ -342,12 +333,12 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon template absl::string_view StatDataAllocatorImpl::truncateStatName(absl::string_view key) { - const uint64_t max_width = options_.maxNameLength(); - if (/*(max_width_ != UNLIMITED_WIDTH) &&*/ (key.size() > max_width)) { + const uint64_t max_width = stats_options_.maxNameLength(); + if (key.size() > max_width) { ENVOY_LOG_MISC( warn, "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, - key.size(), options_.maxNameLength()); + key.size(), max_width); return absl::string_view(key.data(), max_width); } return key; diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index ba0d948980b2..6866e83b90cc 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -285,10 +285,11 @@ class MetricImpl : public virtual Metric { template class StatDataAllocatorImpl : public StatDataAllocator { public: - //static const uint64_t UNLIMITED_WIDTH = 0; - - //StatDataAllocatorImpl() : max_width_(UNLIMITED_WIDTH) {} - explicit StatDataAllocatorImpl(const StatsOptions& options) : options_(options) {} + /** + * @param stats_options The stat optinos, which must live longer than the allocator. + */ + explicit StatDataAllocatorImpl(const StatsOptions& stats_options) + : stats_options_(stats_options) {} // StatDataAllocator CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name, @@ -312,11 +313,22 @@ template class StatDataAllocatorImpl : public StatDataAllocator */ virtual void free(StatData& data) PURE; + /** + * Truncates a stat name based on the options passed into the constructor, + * issuing a warning if a truncation was needed. + * + * @param stat_name The original stat name. + * @return absl::string_view The stat name, truncated if needed to meet limits in stat_options_. + */ absl::string_view truncateStatName(absl::string_view stat_name); - const StatsOptions& statsOptions() const override { return options_; } + + /** + * @return const StatsOptions& The options passed into the constructor. + */ + const StatsOptions& statsOptions() const override { return stats_options_; } private: - const StatsOptions& options_; + const StatsOptions& stats_options_; }; using RawStatDataAllocator = StatDataAllocatorImpl; @@ -407,9 +419,8 @@ struct HeapStatData { */ class HeapStatDataAllocator : public StatDataAllocatorImpl { public: - explicit HeapStatDataAllocator(const StatsOptions& options) - : StatDataAllocatorImpl(options) {} - //HeapStatDataAllocator() {} + explicit HeapStatDataAllocator(const StatsOptions& options) : StatDataAllocatorImpl(options) {} + // HeapStatDataAllocator() {} ~HeapStatDataAllocator() { ASSERT(stats_.empty()); } // StatDataAllocator @@ -484,9 +495,16 @@ template class IsolatedStatsCache { */ class IsolatedStoreImpl : public Store { public: - /*explicit*/ IsolatedStoreImpl(/*Stats::StatsOptions& stats_options*/) - : alloc_(stats_options_), - counters_([this](const std::string& name) -> CounterSharedPtr { + // TODO(jmarantz): It appears that if the configuartion overrides the max + // option length, this isolated store will not see the adjustment. I am + // not sure if that's a problem. Ideally I'd like to take the stat_options + // here we can creating the StatAllocator, but it's not immediately obvious + // where to pull that from. I think this issue existed starting with + // https://github.com/envoyproxy/envoy/pull/3629, which removed the shadowing + // of option size in a static. + + /* explicit */ IsolatedStoreImpl(/*Stats::StatsOptions& stats_options*/) + : alloc_(stats_options_), counters_([this](const std::string& name) -> CounterSharedPtr { std::string tag_extracted_name = name; std::vector tags; return alloc_.makeCounter(name, std::move(tag_extracted_name), std::move(tags)); diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 478b53b7ba5e..b3bd4f09a556 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -14,10 +14,9 @@ namespace Stats { ThreadLocalStoreImpl::ThreadLocalStoreImpl(const Stats::StatsOptions& stats_options, StatDataAllocator& alloc) - : alloc_(alloc), default_scope_(createScope("")), + : alloc_(alloc), heap_allocator_(stats_options), default_scope_(createScope("")), tag_producer_(std::make_unique()), - num_last_resort_stats_(default_scope_->counter("stats.overflow")), - heap_allocator_(stats_options), source_(*this) {} + num_last_resort_stats_(default_scope_->counter("stats.overflow")), source_(*this) {} ThreadLocalStoreImpl::~ThreadLocalStoreImpl() { ASSERT(shutting_down_); diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 2503e5aaa18c..5472998ffee2 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -276,6 +276,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void mergeInternal(PostMergeCb mergeCb); StatDataAllocator& alloc_; + HeapStatDataAllocator heap_allocator_; Event::Dispatcher* main_thread_dispatcher_{}; ThreadLocal::SlotPtr tls_; mutable Thread::MutexBasicLockable lock_; @@ -286,7 +287,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo std::atomic shutting_down_{}; std::atomic merge_in_progress_{}; Counter& num_last_resort_stats_; - HeapStatDataAllocator heap_allocator_; SourceImpl source_; }; diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index a88e4aa19e25..f80e3a97c620 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -116,11 +116,8 @@ std::string SharedMemory::version(uint64_t max_num_stats, } HotRestartImpl::HotRestartImpl(Options& options) - : Stats::RawStatDataAllocator(options.statsOptions()), - /*options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), - shmem_(SharedMemory::initialize(RawStatDataSet::numBytes(stats_set_options_), options)), - */ - options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), + : Stats::RawStatDataAllocator(options.statsOptions()), options_(options), + stats_set_options_(blockMemHashOptions(options.maxStats())), shmem_(SharedMemory::initialize( RawStatDataSet::numBytes(stats_set_options_, options_.statsOptions()), options_)), log_lock_(shmem_.log_lock_), access_log_lock_(shmem_.access_log_lock_), diff --git a/source/server/hot_restart_impl.h b/source/server/hot_restart_impl.h index 5524d0ba3a5c..39f537d55523 100644 --- a/source/server/hot_restart_impl.h +++ b/source/server/hot_restart_impl.h @@ -138,6 +138,7 @@ class HotRestartImpl : public HotRestart, */ static std::string hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len); + // RawStatDataAllocator Stats::RawStatData* alloc(const std::string& name) override; void free(Stats::RawStatData& data) override; diff --git a/source/server/hot_restart_nop_impl.h b/source/server/hot_restart_nop_impl.h index 6dd94966015b..d54130dff395 100644 --- a/source/server/hot_restart_nop_impl.h +++ b/source/server/hot_restart_nop_impl.h @@ -14,7 +14,7 @@ namespace Server { */ class HotRestartNopImpl : public Server::HotRestart { public: - HotRestartNopImpl(const Stats::StatsOptions& options) : stats_allocator_(options) {} + HotRestartNopImpl(const Stats::StatsOptions& stats_options) : stats_allocator_(stats_options) {} // Server::HotRestart void drainParentListeners() override {} diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index fe63383b02ee..d9a58ef91290 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -478,9 +478,9 @@ TEST(TagProducerTest, CheckConstructor) { "No regex specified for tag specifier and no default regex for name: 'test_extractor'"); } -// Validate truncation behavior of RawStatData. +// Validate truncation behavior of HeapStatData. // Note: a similar test using RawStatData* is in test/server/hot_restart_impl_test.cc. -TEST(RawStatDataTest, Truncate) { +TEST(RawStatDataTest, HeapTruncate) { Stats::StatsOptionsImpl stats_options; HeapStatDataAllocator alloc(stats_options); const std::string long_string(stats_options.maxNameLength() + 1, 'A'); @@ -507,7 +507,9 @@ TEST(RawStatDataTest, HeapAlloc) { alloc.free(*stat_3); } -TEST(RawStatDataTest, TruncateRaw) { +// See also HotRestartImplTest.RawTruncateWithAllocator and +// +TEST(RawStatDataTest, TruncateWithoutAllocator) { // RawStatData::truncateAndInit(absl::string_view key, const StatsOptions& stats_options) will // truncate and log to ENVOY_LOG_MISC if given a key longer than the allowed // stats_options.maxNameLength(). This mechanism is also tested in HotRestartImplTest.truncateKey. diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index a91ff8dd1333..03cbb1a6bb30 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -1,4 +1,3 @@ - #pragma once #include diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index af1840230d2b..10dd87073f82 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -90,7 +90,8 @@ TEST_F(HotRestartImplTest, versionString) { } } -TEST_F(HotRestartImplTest, RawTruncate) { +// See also StatsImplTest.RawTruncateWithoutAllocator. +TEST_F(HotRestartImplTest, RawTruncateWithAllocator) { setup(); const std::string long_string(hot_restart_->statsOptions().maxNameLength() + 1, 'A'); diff --git a/test/test_common/utility.h b/test/test_common/utility.h index b1ba7987ad65..981419024f17 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -377,11 +377,10 @@ class TestAllocator : public RawStatDataAllocator { class MockedTestAllocator : public RawStatDataAllocator { public: MockedTestAllocator(const StatsOptions& stats_options) - : RawStatDataAllocator(stats_options), - alloc_(stats_options) { - ON_CALL(*this, alloc(_)) - .WillByDefault(Invoke( - [this](const std::string& name) -> RawStatData* { return alloc_.alloc(name); })); + : RawStatDataAllocator(stats_options), alloc_(stats_options) { + ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](const std::string& name) -> RawStatData* { + return alloc_.alloc(name); + })); ON_CALL(*this, free(_)).WillByDefault(Invoke([this](RawStatData& data) -> void { return alloc_.free(data); From c79f73d3285f4da7383d95e235c95e3a28587d8b Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Jul 2018 06:52:13 -0400 Subject: [PATCH 10/28] Test that truncation actually occurred in Consistency test. Signed-off-by: Joshua Marantz --- test/server/hot_restart_impl_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 10dd87073f82..8a88e247f55e 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -115,6 +115,7 @@ TEST_F(HotRestartImplTest, Consistency) { uint64_t stat_size = sizeof(Stats::RawStatData) + max_name_length; std::string stat_hex_dump_1 = Hex::encode(reinterpret_cast(stat_1), stat_size); EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_1), expected_hash); + EXPECT_EQ(name_1, stat_1->key()); hot_restart_->free(*stat_1); // If a stat name is truncated, we expect that its internal representation is the same as if it @@ -123,6 +124,8 @@ TEST_F(HotRestartImplTest, Consistency) { Stats::RawStatData* stat_2 = hot_restart_->alloc(name_2); std::string stat_hex_dump_2 = Hex::encode(reinterpret_cast(stat_2), stat_size); EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_2), expected_hash); + EXPECT_EQ(name_1, stat_2->key()); + EXPECT_NE(name_2, stat_2->key()); // Was truncated. hot_restart_->free(*stat_2); } From 5c6c9255f4dd6b9298ff3a99ddac1c5cf9b53fbd Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 19 Jul 2018 06:54:16 -0400 Subject: [PATCH 11/28] Add 'const' declarations for locals. Signed-off-by: Joshua Marantz --- test/server/hot_restart_impl_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 8a88e247f55e..d4ebac0a3b46 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -107,13 +107,13 @@ TEST_F(HotRestartImplTest, Consistency) { // Generate a stat, encode it to hex, and take the hash of that hex string. We expect the hash to // vary only when the internal representation of a stat has been intentionally changed, in which // case SharedMemory::VERSION should be incremented as well. - uint64_t expected_hash = 1874506077228772558; - uint64_t max_name_length = stats_options_.maxNameLength(); + const uint64_t expected_hash = 1874506077228772558; + const uint64_t max_name_length = stats_options_.maxNameLength(); const std::string name_1(max_name_length, 'A'); Stats::RawStatData* stat_1 = hot_restart_->alloc(name_1); - uint64_t stat_size = sizeof(Stats::RawStatData) + max_name_length; - std::string stat_hex_dump_1 = Hex::encode(reinterpret_cast(stat_1), stat_size); + const uint64_t stat_size = sizeof(Stats::RawStatData) + max_name_length; + const std::string stat_hex_dump_1 = Hex::encode(reinterpret_cast(stat_1), stat_size); EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_1), expected_hash); EXPECT_EQ(name_1, stat_1->key()); hot_restart_->free(*stat_1); @@ -122,7 +122,7 @@ TEST_F(HotRestartImplTest, Consistency) { // had been initialized with the already-truncated name. const std::string name_2(max_name_length + 1, 'A'); Stats::RawStatData* stat_2 = hot_restart_->alloc(name_2); - std::string stat_hex_dump_2 = Hex::encode(reinterpret_cast(stat_2), stat_size); + const std::string stat_hex_dump_2 = Hex::encode(reinterpret_cast(stat_2), stat_size); EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_2), expected_hash); EXPECT_EQ(name_1, stat_2->key()); EXPECT_NE(name_2, stat_2->key()); // Was truncated. From e9d78cf0256e9ef92a020802df2f24bf9216ce8e Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 20 Jul 2018 13:48:03 -0400 Subject: [PATCH 12/28] Use an effectively unlimited truncation size (1M) for IsolatedStore for now, which avoids changing previous behavior. Address ambuc's comments. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.h | 16 +++++++++++----- source/common/stats/thread_local_store.cc | 8 +------- test/common/stats/stats_impl_test.cc | 10 ++++++++++ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 6866e83b90cc..27e12648004b 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -504,20 +504,26 @@ class IsolatedStoreImpl : public Store { // of option size in a static. /* explicit */ IsolatedStoreImpl(/*Stats::StatsOptions& stats_options*/) - : alloc_(stats_options_), counters_([this](const std::string& name) -> CounterSharedPtr { + : counters_([this](const std::string& name) -> CounterSharedPtr { std::string tag_extracted_name = name; std::vector tags; - return alloc_.makeCounter(name, std::move(tag_extracted_name), std::move(tags)); + return alloc_->makeCounter(name, std::move(tag_extracted_name), std::move(tags)); }), gauges_([this](const std::string& name) -> GaugeSharedPtr { std::string tag_extracted_name = name; std::vector tags; - return alloc_.makeGauge(name, std::move(tag_extracted_name), std::move(tags)); + return alloc_->makeGauge(name, std::move(tag_extracted_name), std::move(tags)); }), histograms_([this](const std::string& name) -> HistogramSharedPtr { return std::make_shared(name, *this, std::string(name), std::vector()); - }) {} + }) { + // TODO(jmarantz): for now, just set the limit to be very high, until + // we make a decision on whether to plumb in the options structure seen + // by the rest of the system. + stats_options_.max_obj_name_length_ = 1000 * 1000; + alloc_ = std::make_unique(stats_options_); + } // Stats::Scope Counter& counter(const std::string& name) override { return counters_.get(name); } @@ -561,7 +567,7 @@ class IsolatedStoreImpl : public Store { }; StatsOptionsImpl stats_options_; - HeapStatDataAllocator alloc_; + std::unique_ptr alloc_; IsolatedStatsCache counters_; IsolatedStatsCache gauges_; IsolatedStatsCache histograms_; diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index b3bd4f09a556..b2df1cce4926 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -182,14 +182,8 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( make_stat(parent_.alloc_, name, std::move(tag_extracted_name), std::move(tags)); if (stat == nullptr) { parent_.num_last_resort_stats_.inc(); - // TODO(jbuckland) Performing the fallback from non-heap allocator to heap allocator should be - // the responsibility of the non-heap allocator, not the client of the non-heap allocator. - // This branch will never be used in the non-hot-restart case, since the parent_.alloc_ object - // will throw instead of returning a nullptr; we should remove the assumption that this - // branching case is always available. stat = - make_stat(parent_.heap_allocator_, name.substr(0, parent_.statsOptions().maxNameLength()), - std::move(tag_extracted_name), std::move(tags)); + make_stat(parent_.heap_allocator_, name, std::move(tag_extracted_name), std::move(tags)); ASSERT(stat != nullptr); } central_ref = stat; diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index d9a58ef91290..7184375a6fa0 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -66,6 +66,16 @@ TEST(StatsIsolatedStoreImplTest, All) { EXPECT_EQ(2UL, store.gauges().size()); } +TEST(StatsIsolatedStoreImplTest, LongStatName) { + IsolatedStoreImpl store; + Stats::StatsOptionsImpl stats_options; + const std::string long_string(stats_options.maxNameLength() + 1, 'A'); + + ScopePtr scope = store.createScope("scope."); + Counter& counter = scope->counter(long_string); + EXPECT_EQ(absl::StrCat("scope.", long_string), counter.name()); +} + /** * Test stats macros. @see stats_macros.h */ From 77eecc5d7b26d4fa02180a9a515174509da7546b Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 23 Jul 2018 11:32:08 -0400 Subject: [PATCH 13/28] Remove some redundant code and cleanup. Signed-off-by: Joshua Marantz --- source/common/common/block_memory_hash_set.h | 2 +- source/common/stats/stats_impl.cc | 35 +++++-------------- source/common/stats/stats_impl.h | 25 ++----------- .../common/block_memory_hash_set_test.cc | 8 ++--- test/common/stats/stats_impl_test.cc | 15 -------- test/test_common/utility.h | 2 +- 6 files changed, 16 insertions(+), 71 deletions(-) diff --git a/source/common/common/block_memory_hash_set.h b/source/common/common/block_memory_hash_set.h index e28a1c59595e..5724e527067b 100644 --- a/source/common/common/block_memory_hash_set.h +++ b/source/common/common/block_memory_hash_set.h @@ -165,7 +165,7 @@ template class BlockMemoryHashSet : public Logger::LoggabletruncateAndInit(key, stats_options_); + value->initialize(key, stats_options_); ++control_->size; return ValueCreatedPair(value, true); } diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 805a39d30bbf..63c77f2c7e69 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -333,42 +333,23 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon template absl::string_view StatDataAllocatorImpl::truncateStatName(absl::string_view key) { - const uint64_t max_width = stats_options_.maxNameLength(); - if (key.size() > max_width) { + const uint64_t max_length = stats_options_.maxNameLength(); + if (key.size() > max_length) { ENVOY_LOG_MISC( warn, "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, - key.size(), max_width); - return absl::string_view(key.data(), max_width); + key.size(), max_length); + return absl::string_view(key.data(), max_length); } return key; } -void RawStatData::initialize(absl::string_view key, uint64_t xfer_size) { +void RawStatData::initialize(absl::string_view key, const StatsOptions& stats_options) { ASSERT(!initialized()); + ASSERT(key.size() <= stats_options.maxNameLength()); ref_count_ = 1; - memcpy(name_, key.data(), xfer_size); - name_[xfer_size] = '\0'; -} - -void RawStatData::checkAndInit(absl::string_view key, uint64_t num_bytes_allocated) { - uint64_t xfer_size = key.size(); - ASSERT(structSize(xfer_size) <= num_bytes_allocated); - - initialize(key, xfer_size); -} - -void RawStatData::truncateAndInit(absl::string_view key, const StatsOptions& stats_options) { - if (key.size() > stats_options.maxNameLength()) { - ENVOY_LOG_MISC( - warn, - "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, - key.size(), stats_options.maxNameLength()); - } - - // key is not necessarily nul-terminated, but we want to make sure name_ is. - uint64_t xfer_size = std::min(stats_options.maxNameLength(), key.size()); - initialize(key, xfer_size); + memcpy(name_, key.data(), key.size()); + name_[key.size()] = '\0'; } HistogramStatisticsImpl::HistogramStatisticsImpl(const histogram_t* histogram_ptr) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 27e12648004b..8474cd89f59e 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -221,14 +221,7 @@ struct RawStatData { * does not expect stat name truncation. We pass in the number of bytes allocated in order to * assert the copy is safe inline. */ - void checkAndInit(absl::string_view key, uint64_t num_bytes_allocated); - - /** - * Initializes this object to have the specified key, - * a refcount of 1, and all other values zero. Required by the BlockMemoryHashSet. StatsOptions is - * used to truncate key inline, if necessary. - */ - void truncateAndInit(absl::string_view key, const StatsOptions& stats_options); + void initialize(absl::string_view key, const StatsOptions& stats_options); /** * Returns a hash of the key. This is required by BlockMemoryHashSet. @@ -251,9 +244,6 @@ struct RawStatData { std::atomic ref_count_; std::atomic unused_; char name_[]; - -private: - void initialize(absl::string_view key, uint64_t num_bytes_allocated); }; /** @@ -495,15 +485,7 @@ template class IsolatedStatsCache { */ class IsolatedStoreImpl : public Store { public: - // TODO(jmarantz): It appears that if the configuartion overrides the max - // option length, this isolated store will not see the adjustment. I am - // not sure if that's a problem. Ideally I'd like to take the stat_options - // here we can creating the StatAllocator, but it's not immediately obvious - // where to pull that from. I think this issue existed starting with - // https://github.com/envoyproxy/envoy/pull/3629, which removed the shadowing - // of option size in a static. - - /* explicit */ IsolatedStoreImpl(/*Stats::StatsOptions& stats_options*/) + IsolatedStoreImpl() : counters_([this](const std::string& name) -> CounterSharedPtr { std::string tag_extracted_name = name; std::vector tags; @@ -518,9 +500,6 @@ class IsolatedStoreImpl : public Store { return std::make_shared(name, *this, std::string(name), std::vector()); }) { - // TODO(jmarantz): for now, just set the limit to be very high, until - // we make a decision on whether to plumb in the options structure seen - // by the rest of the system. stats_options_.max_obj_name_length_ = 1000 * 1000; alloc_ = std::make_unique(stats_options_); } diff --git a/test/common/common/block_memory_hash_set_test.cc b/test/common/common/block_memory_hash_set_test.cc index da5477cf8325..f0082da91ad5 100644 --- a/test/common/common/block_memory_hash_set_test.cc +++ b/test/common/common/block_memory_hash_set_test.cc @@ -22,10 +22,10 @@ class BlockMemoryHashSetTest : public testing::Test { // TestValue that doesn't define a hash. struct TestValueBase { absl::string_view key() const { return name; } - void truncateAndInit(absl::string_view key, const Stats::StatsOptions& stats_options) { - uint64_t xfer = std::min(stats_options.maxNameLength(), key.size()); - memcpy(name, key.data(), xfer); - name[xfer] = '\0'; + void initialize(absl::string_view key, const Stats::StatsOptions& stats_options) { + ASSERT(key.size() <= stats_options.maxNameLength()); + memcpy(name, key.data(), key.size()); + name[key.size()] = '\0'; } static uint64_t structSizeWithOptions(const Stats::StatsOptions& stats_options) { UNREFERENCED_PARAMETER(stats_options); diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index 7184375a6fa0..3578625f2c8e 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -517,21 +517,6 @@ TEST(RawStatDataTest, HeapAlloc) { alloc.free(*stat_3); } -// See also HotRestartImplTest.RawTruncateWithAllocator and -// -TEST(RawStatDataTest, TruncateWithoutAllocator) { - // RawStatData::truncateAndInit(absl::string_view key, const StatsOptions& stats_options) will - // truncate and log to ENVOY_LOG_MISC if given a key longer than the allowed - // stats_options.maxNameLength(). This mechanism is also tested in HotRestartImplTest.truncateKey. - Stats::StatsOptionsImpl stats_options; - const std::string long_string(stats_options.maxNameLength() + 1, 'A'); - RawStatData* stat = - static_cast(::calloc(RawStatData::structSizeWithOptions(stats_options), 1)); - EXPECT_LOG_CONTAINS("warning", "is too long with", - stat->truncateAndInit(long_string, stats_options)); - ::free(stat); -} - TEST(SourceImplTest, Caching) { NiceMock store; std::vector stored_counters; diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 981419024f17..f87082bd64c8 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -351,7 +351,7 @@ class TestAllocator : public RawStatDataAllocator { if (!stat_ref) { stat_ref.reset(static_cast( ::calloc(RawStatData::structSizeWithOptions(stats_options), 1))); - stat_ref->truncateAndInit(name, stats_options); + stat_ref->initialize(name, stats_options); } else { stat_ref->ref_count_++; } From 8ba7c459cd676c84728b78b4d4b09c716cda8192 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 25 Jul 2018 20:06:21 -0400 Subject: [PATCH 14/28] Use std::numeric_limits for unbounded length, dividing by 2 to avoid overflow wrap. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 8474cd89f59e..fa827c40e99f 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -500,7 +500,10 @@ class IsolatedStoreImpl : public Store { return std::make_shared(name, *this, std::string(name), std::vector()); }) { - stats_options_.max_obj_name_length_ = 1000 * 1000; + // We divide by two here to get an effectively unlimited length. If we don't divide + // by two, we'll get an integer wrap in the maxNameLength addition, and the behavior + // will not be what we intend. + stats_options_.max_obj_name_length_ = std::numeric_limits::max() / 2; alloc_ = std::make_unique(stats_options_); } From 85a91dee6ab0e830bd80c26cb59791cdee8669fa Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 26 Jul 2018 09:50:07 -0400 Subject: [PATCH 15/28] Document required fields for StatData. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.cc | 7 ++++--- source/common/stats/stats_impl.h | 3 +-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 63c77f2c7e69..faf431a6fd4d 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -168,9 +168,10 @@ void HeapStatDataAllocator::free(HeapStatData& data) { /** * Counter implementation that wraps a StatData. StatData must have data members: - * int64_t value_; - * int64_t pending_increment_; - * int16_t flags_; + * std::atomic value_; + * std::atomic pending_increment_; + * std::atomic flags_; + * std::atomic ref_count_; */ template class CounterImpl : public Counter, public MetricImpl { public: diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index fa827c40e99f..9afb2602a35c 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -391,7 +391,7 @@ struct HeapStatData { explicit HeapStatData(absl::string_view key); /** - * Returns the name as a string_view. This is required by BlockMemoryHashSet. + * @returns absl::string_view the name as a string_view. */ absl::string_view key() const { return name_; } @@ -399,7 +399,6 @@ struct HeapStatData { std::atomic pending_increment_{0}; std::atomic flags_{0}; std::atomic ref_count_{1}; - std::atomic unused_{0}; std::string name_; }; From 42aed4af72c1d8ec5b4888780ab4f3d5dadfc207 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 26 Jul 2018 22:36:03 -0400 Subject: [PATCH 16/28] Re-order functions to reduce diffs. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.cc | 31 ++++++++++++++++--------------- source/common/stats/stats_impl.h | 10 +++------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index faf431a6fd4d..104488c65956 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -151,21 +151,6 @@ HeapStatData* HeapStatDataAllocator::alloc(const std::string& name) { return existing_data; } -void HeapStatDataAllocator::free(HeapStatData& data) { - ASSERT(data.ref_count_ > 0); - if (--data.ref_count_ > 0) { - return; - } - - { - Thread::LockGuard lock(mutex_); - size_t key_removed = stats_.erase(&data); - ASSERT(key_removed == 1); - } - - delete &data; -} - /** * Counter implementation that wraps a StatData. StatData must have data members: * std::atomic value_; @@ -332,6 +317,22 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon return names; } +// TODO(jmarantz): move this below HeapStatDataAllocator::alloc. +void HeapStatDataAllocator::free(HeapStatData& data) { + ASSERT(data.ref_count_ > 0); + if (--data.ref_count_ > 0) { + return; + } + + { + Thread::LockGuard lock(mutex_); + size_t key_removed = stats_.erase(&data); + ASSERT(key_removed == 1); + } + + delete &data; +} + template absl::string_view StatDataAllocatorImpl::truncateStatName(absl::string_view key) { const uint64_t max_length = stats_options_.maxNameLength(); diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 9afb2602a35c..27dcfc9ddd91 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -426,13 +426,9 @@ class HeapStatDataAllocator : public StatDataAllocatorImpl { } }; - // TODO(jmarantz): storing the set of stats as a string-set wastes an - // enourmous amount of of memory, because all the stats are composed using the - // same set of cluster names and patterns found in - // source/common/config/well_known_names.cc. This should eventually be changed to - // store a symbol-table of decomposed stat segments (e.g. split on "." to start) and - // arrays referencing those segments. The segments can be ref-counted so they - // no longer consume memory once deleted. + // TODO(jmarantz): See https://github.com/envoyproxy/envoy/pull/3927 which can + // help reorganize the heap stats using a ref-counted symbol table to compress + // the stat strings. typedef std::unordered_set StatSet; // An unordered set of HeapStatData pointers which keys off the key() From f48bf77c88820d47adc81dd6237394c15578d875 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 27 Jul 2018 09:52:01 -0400 Subject: [PATCH 17/28] add a comment justifying use of templates for the StatAllocator. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 27dcfc9ddd91..04a70ed6fbca 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -273,6 +273,13 @@ class MetricImpl : public virtual Metric { const std::vector tags_; }; +// Partially implements a StaDataAllocator, leaving alloc & free for subclasses. +// We templatize on StatData rather than defining a virtual base StatData class +// for performance reasons; stat increment is on the hot path. +// +// The two production derivations cover using a fixed block of shared-memory for +// hot restart stat continuity, and heap allocation for more efficient RAM usage +// for when hot-restart is not required. template class StatDataAllocatorImpl : public StatDataAllocator { public: /** From ccb29e1fe198845bb259e2aacb0fad44e30e083b Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 27 Jul 2018 18:03:07 -0400 Subject: [PATCH 18/28] Fix typos Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 04a70ed6fbca..00950b6486e7 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -273,7 +273,7 @@ class MetricImpl : public virtual Metric { const std::vector tags_; }; -// Partially implements a StaDataAllocator, leaving alloc & free for subclasses. +// Partially implements a StatDataAllocator, leaving alloc & free for subclasses. // We templatize on StatData rather than defining a virtual base StatData class // for performance reasons; stat increment is on the hot path. // @@ -416,7 +416,6 @@ struct HeapStatData { class HeapStatDataAllocator : public StatDataAllocatorImpl { public: explicit HeapStatDataAllocator(const StatsOptions& options) : StatDataAllocatorImpl(options) {} - // HeapStatDataAllocator() {} ~HeapStatDataAllocator() { ASSERT(stats_.empty()); } // StatDataAllocator From 9a757c2b760403ef978b322036b223810ecdf21f Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 27 Jul 2018 19:32:06 -0400 Subject: [PATCH 19/28] Add more commentary about why templates are used rather than virtual inheritance for StatData. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 00950b6486e7..93bec83423b5 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -280,6 +280,12 @@ class MetricImpl : public virtual Metric { // The two production derivations cover using a fixed block of shared-memory for // hot restart stat continuity, and heap allocation for more efficient RAM usage // for when hot-restart is not required. +// +// Also note that RawStatData needs to live in a shared memory block, and it's +// possible, but not obvious, that a vptr would be usable across processes. In +// any case, RawStatData is currently allocated with calloc() rather than new, +// so the usual C++ compiler assistance for setting up vptrs will not be +// available. This could be resolved with placed new. template class StatDataAllocatorImpl : public StatDataAllocator { public: /** From 7f845478674964d658efe7e0cf855d7fdd3db7fb Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sat, 28 Jul 2018 16:23:31 -0400 Subject: [PATCH 20/28] Move all truncation to thread_local_store.cc. Signed-off-by: Joshua Marantz --- include/envoy/stats/stats.h | 4 +- source/common/stats/stats_impl.cc | 24 +++-------- source/common/stats/stats_impl.h | 17 ++------ source/common/stats/thread_local_store.cc | 22 +++++++--- source/common/stats/thread_local_store.h | 2 +- source/server/hot_restart_impl.cc | 5 ++- source/server/hot_restart_impl.h | 2 +- test/common/stats/stats_impl_test.cc | 8 ++-- test/common/stats/thread_local_store_test.cc | 45 +++++++++++++++++++- test/server/hot_restart_impl_test.cc | 27 ++---------- test/test_common/utility.h | 10 ++--- 11 files changed, 91 insertions(+), 75 deletions(-) diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index f36c4f5ef9c6..0f91e1200153 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -467,7 +467,7 @@ class StatDataAllocator { * @return CounterSharedPtr a counter, or nullptr if allocation failed, in which case * tag_extracted_name and tags are not moved. */ - virtual CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name, + virtual CounterSharedPtr makeCounter(absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) PURE; /** @@ -477,7 +477,7 @@ class StatDataAllocator { * @return GaugeSharedPtr a gauge, or nullptr if allocation failed, in which case * tag_extracted_name and tags are not moved. */ - virtual GaugeSharedPtr makeGauge(const std::string& name, std::string&& tag_extracted_name, + virtual GaugeSharedPtr makeGauge(absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) PURE; /** diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 104488c65956..7f74145b9d9a 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -137,8 +137,11 @@ bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector HeapStatData::HeapStatData(absl::string_view key) : name_(key.data(), key.size()) {} -HeapStatData* HeapStatDataAllocator::alloc(const std::string& name) { - auto data = std::make_unique(truncateStatName(name)); +HeapStatData* HeapStatDataAllocator::alloc(absl::string_view name) { + // Do not truncate here for non-hot-restart case; truncate at the call-site in + // ThreadLocalStoreImpl::ScopeImpl::safeMakeStat() only when allocating a heap-stat + // as a fallback when we've exhausted our shared-memory block. + auto data = std::make_unique(name); Thread::ReleasableLockGuard lock(mutex_); auto ret = stats_.insert(data.get()); HeapStatData* existing_data = *ret.first; @@ -333,19 +336,6 @@ void HeapStatDataAllocator::free(HeapStatData& data) { delete &data; } -template -absl::string_view StatDataAllocatorImpl::truncateStatName(absl::string_view key) { - const uint64_t max_length = stats_options_.maxNameLength(); - if (key.size() > max_length) { - ENVOY_LOG_MISC( - warn, - "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, - key.size(), max_length); - return absl::string_view(key.data(), max_length); - } - return key; -} - void RawStatData::initialize(absl::string_view key, const StatsOptions& stats_options) { ASSERT(!initialized()); ASSERT(key.size() <= stats_options.maxNameLength()); @@ -413,7 +403,7 @@ void SourceImpl::clearCache() { } template -CounterSharedPtr StatDataAllocatorImpl::makeCounter(const std::string& name, +CounterSharedPtr StatDataAllocatorImpl::makeCounter(absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) { StatData* data = alloc(name); @@ -425,7 +415,7 @@ CounterSharedPtr StatDataAllocatorImpl::makeCounter(const std::string& } template -GaugeSharedPtr StatDataAllocatorImpl::makeGauge(const std::string& name, +GaugeSharedPtr StatDataAllocatorImpl::makeGauge(absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) { StatData* data = alloc(name); diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 93bec83423b5..679587343387 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -295,9 +295,9 @@ template class StatDataAllocatorImpl : public StatDataAllocator : stats_options_(stats_options) {} // StatDataAllocator - CounterSharedPtr makeCounter(const std::string& name, std::string&& tag_extracted_name, + CounterSharedPtr makeCounter(absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) override; - GaugeSharedPtr makeGauge(const std::string& name, std::string&& tag_extracted_name, + GaugeSharedPtr makeGauge(absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) override; /** @@ -307,7 +307,7 @@ template class StatDataAllocatorImpl : public StatDataAllocator * by name if one already exists with the same name. This is used for intra-process * scope swapping as well as inter-process hot restart. */ - virtual StatData* alloc(const std::string& name) PURE; + virtual StatData* alloc(absl::string_view name) PURE; /** * Free a raw stat data block. The allocator should handle reference counting and only truly @@ -316,15 +316,6 @@ template class StatDataAllocatorImpl : public StatDataAllocator */ virtual void free(StatData& data) PURE; - /** - * Truncates a stat name based on the options passed into the constructor, - * issuing a warning if a truncation was needed. - * - * @param stat_name The original stat name. - * @return absl::string_view The stat name, truncated if needed to meet limits in stat_options_. - */ - absl::string_view truncateStatName(absl::string_view stat_name); - /** * @return const StatsOptions& The options passed into the constructor. */ @@ -425,7 +416,7 @@ class HeapStatDataAllocator : public StatDataAllocatorImpl { ~HeapStatDataAllocator() { ASSERT(stats_.empty()); } // StatDataAllocator - HeapStatData* alloc(const std::string& name) override; + HeapStatData* alloc(absl::string_view name) override; void free(HeapStatData& data) override; private: diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index b2df1cce4926..c2c61d457538 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -178,12 +178,24 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( if (!central_ref) { std::vector tags; std::string tag_extracted_name = parent_.getTagsForName(name, tags); + absl::string_view truncated_name = name; + const uint64_t max_length = parent_.statsOptions().maxNameLength(); + // Note that the heap-allocator does not truncate itself; we have to + // truncate here if we are using heap-allocation as a fallback due to an + // exahusted shared-memroy block + if (truncated_name.size() > max_length) { + ENVOY_LOG_MISC( + warn, + "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", + name, name.size(), max_length); + truncated_name = absl::string_view(name.data(), max_length); + } std::shared_ptr stat = - make_stat(parent_.alloc_, name, std::move(tag_extracted_name), std::move(tags)); + make_stat(parent_.alloc_, truncated_name, std::move(tag_extracted_name), std::move(tags)); if (stat == nullptr) { parent_.num_last_resort_stats_.inc(); - stat = - make_stat(parent_.heap_allocator_, name, std::move(tag_extracted_name), std::move(tags)); + stat = make_stat(parent_.heap_allocator_, truncated_name, std::move(tag_extracted_name), + std::move(tags)); ASSERT(stat != nullptr); } central_ref = stat; @@ -213,7 +225,7 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) { return safeMakeStat( final_name, central_cache_.counters_, - [](StatDataAllocator& allocator, const std::string& name, std::string&& tag_extracted_name, + [](StatDataAllocator& allocator, absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) -> CounterSharedPtr { return allocator.makeCounter(name, std::move(tag_extracted_name), std::move(tags)); }, @@ -247,7 +259,7 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) { return safeMakeStat( final_name, central_cache_.gauges_, - [](StatDataAllocator& allocator, const std::string& name, std::string&& tag_extracted_name, + [](StatDataAllocator& allocator, absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) -> GaugeSharedPtr { return allocator.makeGauge(name, std::move(tag_extracted_name), std::move(tags)); }, diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 5472998ffee2..3901e3b45ad2 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -230,7 +230,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo template using MakeStatFn = - std::function(StatDataAllocator&, const std::string& name, + std::function(StatDataAllocator&, absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags)>; diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index f80e3a97c620..319595ea12ab 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -142,10 +142,11 @@ HotRestartImpl::HotRestartImpl(Options& options) RELEASE_ASSERT(rc != -1, ""); } -Stats::RawStatData* HotRestartImpl::alloc(const std::string& name) { +Stats::RawStatData* HotRestartImpl::alloc(absl::string_view name) { // Try to find the existing slot in shared memory, otherwise allocate a new one. Thread::LockGuard lock(stat_lock_); - auto value_created = stats_set_->insert(truncateStatName(name)); + ASSERT(name.length() <= options_.statsOptions().maxNameLength()); + auto value_created = stats_set_->insert(name); Stats::RawStatData* data = value_created.first; if (data == nullptr) { return nullptr; diff --git a/source/server/hot_restart_impl.h b/source/server/hot_restart_impl.h index 39f537d55523..c24c07b10904 100644 --- a/source/server/hot_restart_impl.h +++ b/source/server/hot_restart_impl.h @@ -139,7 +139,7 @@ class HotRestartImpl : public HotRestart, static std::string hotRestartVersion(uint64_t max_num_stats, uint64_t max_stat_name_len); // RawStatDataAllocator - Stats::RawStatData* alloc(const std::string& name) override; + Stats::RawStatData* alloc(absl::string_view name) override; void free(Stats::RawStatData& data) override; private: diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index 3578625f2c8e..1a05f3d0268e 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -488,14 +488,14 @@ TEST(TagProducerTest, CheckConstructor) { "No regex specified for tag specifier and no default regex for name: 'test_extractor'"); } -// Validate truncation behavior of HeapStatData. -// Note: a similar test using RawStatData* is in test/server/hot_restart_impl_test.cc. -TEST(RawStatDataTest, HeapTruncate) { +// No truncation occurs in the implementation of HeapStatData. +TEST(RawStatDataTest, HeapNoTruncate) { Stats::StatsOptionsImpl stats_options; HeapStatDataAllocator alloc(stats_options); const std::string long_string(stats_options.maxNameLength() + 1, 'A'); HeapStatData* stat{}; - EXPECT_LOG_CONTAINS("warning", "is too long with", stat = alloc.alloc(long_string)); + EXPECT_NO_LOGS(stat = alloc.alloc(long_string)); + EXPECT_EQ(stat->key(), long_string); alloc.free(*stat); } diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 3225f4a37d37..e2efaf3edd01 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -387,7 +387,7 @@ TEST_F(StatsThreadLocalStoreTest, AllocFailed) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(alloc_, alloc("foo")).WillOnce(Return(nullptr)); + EXPECT_CALL(alloc_, alloc(absl::string_view("foo"))).WillOnce(Return(nullptr)); Counter& c1 = store_->counter("foo"); EXPECT_EQ(1UL, store_->counter("stats.overflow").value()); @@ -401,6 +401,49 @@ TEST_F(StatsThreadLocalStoreTest, AllocFailed) { EXPECT_CALL(alloc_, free(_)); } +TEST_F(StatsThreadLocalStoreTest, Truncation) { + InSequence s; + store_->initializeThreading(main_thread_dispatcher_, tls_); + + // First, with a successful RawStatData allocation: + const uint64_t max_name_length = options_.maxNameLength(); + const std::string name_1(max_name_length + 1, 'A'); + + EXPECT_CALL(alloc_, alloc(_)); + store_->counter(name_1); + + // The stats did not overflow yet. + EXPECT_EQ(0UL, store_->counter("stats.overflow").value()); + + // The name will be truncated, so we won't be able to find it with the entire name. + EXPECT_EQ(nullptr, TestUtility::findCounter(*store_, name_1).get()); + + // But we can find it based on the expected truncation. + EXPECT_NE(nullptr, TestUtility::findCounter(*store_, name_1.substr(0, max_name_length)).get()); + + // The same should be true with heap allocation, which occurs when the default + // allocator fails. + const std::string name_2(max_name_length + 1, 'B'); + EXPECT_CALL(alloc_, alloc(_)).WillOnce(Return(nullptr)); + store_->counter(name_2); + + // Same deal: the name will be truncated, so we won't be able to find it with the entire name. + EXPECT_EQ(nullptr, TestUtility::findCounter(*store_, name_1).get()); + + // But we can find it based on the expected truncation. + EXPECT_NE(nullptr, TestUtility::findCounter(*store_, name_1.substr(0, max_name_length)).get()); + + // Now the stats have overflowed. + EXPECT_EQ(1UL, store_->counter("stats.overflow").value()); + + store_->shutdownThreading(); + tls_.shutdownThread(); + + // Includes overflow, and the first raw-allocated stat, but not the failsafe stat which we + // allocated from the heap. + EXPECT_CALL(alloc_, free(_)).Times(2); +} + TEST_F(StatsThreadLocalStoreTest, ShuttingDown) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index d4ebac0a3b46..bd021217ffdd 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -91,13 +91,12 @@ TEST_F(HotRestartImplTest, versionString) { } // See also StatsImplTest.RawTruncateWithoutAllocator. -TEST_F(HotRestartImplTest, RawTruncateWithAllocator) { +TEST_F(HotRestartImplTest, RawAllocTooLarge) { setup(); const std::string long_string(hot_restart_->statsOptions().maxNameLength() + 1, 'A'); - Stats::RawStatData* stat{}; - EXPECT_LOG_CONTAINS("warning", "is too long with", stat = hot_restart_->alloc(long_string)); - hot_restart_->free(*stat); + EXPECT_DEATH_LOG_TO_STDERR(hot_restart_->alloc(long_string), "name.length"); + //"name\\.length\\(\\) \\<\\= options\\_.statsOptions\\(\\)\\.maxNameLength\\(\\)"); } // Check consistency of internal stat representation @@ -117,16 +116,6 @@ TEST_F(HotRestartImplTest, Consistency) { EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_1), expected_hash); EXPECT_EQ(name_1, stat_1->key()); hot_restart_->free(*stat_1); - - // If a stat name is truncated, we expect that its internal representation is the same as if it - // had been initialized with the already-truncated name. - const std::string name_2(max_name_length + 1, 'A'); - Stats::RawStatData* stat_2 = hot_restart_->alloc(name_2); - const std::string stat_hex_dump_2 = Hex::encode(reinterpret_cast(stat_2), stat_size); - EXPECT_EQ(HashUtil::xxHash64(stat_hex_dump_2), expected_hash); - EXPECT_EQ(name_1, stat_2->key()); - EXPECT_NE(name_2, stat_2->key()); // Was truncated. - hot_restart_->free(*stat_2); } TEST_F(HotRestartImplTest, RawAlloc) { @@ -172,16 +161,6 @@ TEST_F(HotRestartImplTest, crossAlloc) { EXPECT_EQ(stat5, stat5_prime); } -TEST_F(HotRestartImplTest, truncateKey) { - setup(); - - std::string key1(options_.statsOptions().maxNameLength(), 'a'); - Stats::RawStatData* stat1 = hot_restart_->alloc(key1); - std::string key2 = key1 + "a"; - Stats::RawStatData* stat2 = hot_restart_->alloc(key2); - EXPECT_EQ(stat1, stat2); -} - TEST_F(HotRestartImplTest, allocFail) { EXPECT_CALL(options_, maxStats()).WillRepeatedly(Return(2)); setup(); diff --git a/test/test_common/utility.h b/test/test_common/utility.h index f87082bd64c8..957e23d212ab 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -344,10 +344,10 @@ class TestAllocator : public RawStatDataAllocator { TestAllocator(const StatsOptions& stats_options) : RawStatDataAllocator(stats_options) {} ~TestAllocator() { EXPECT_TRUE(stats_.empty()); } - RawStatData* alloc(const std::string& name) override { + RawStatData* alloc(absl::string_view name) override { Stats::StatsOptionsImpl stats_options; stats_options.max_obj_name_length_ = 127; - CSmartPtr& stat_ref = stats_[name]; + CSmartPtr& stat_ref = stats_[std::string(name)]; if (!stat_ref) { stat_ref.reset(static_cast( ::calloc(RawStatData::structSizeWithOptions(stats_options), 1))); @@ -378,7 +378,7 @@ class MockedTestAllocator : public RawStatDataAllocator { public: MockedTestAllocator(const StatsOptions& stats_options) : RawStatDataAllocator(stats_options), alloc_(stats_options) { - ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](const std::string& name) -> RawStatData* { + ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](absl::string_view name) -> RawStatData* { return alloc_.alloc(name); })); @@ -386,10 +386,10 @@ class MockedTestAllocator : public RawStatDataAllocator { return alloc_.free(data); })); - EXPECT_CALL(*this, alloc("stats.overflow")); + EXPECT_CALL(*this, alloc(absl::string_view("stats.overflow"))); } - MOCK_METHOD1(alloc, RawStatData*(const std::string& name)); + MOCK_METHOD1(alloc, RawStatData*(absl::string_view name)); MOCK_METHOD1(free, void(RawStatData& data)); TestAllocator alloc_; From 7d15bf8d50d86f48d06d1023ab889b07a5c21f3c Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 29 Jul 2018 01:27:20 -0400 Subject: [PATCH 21/28] Move truncation test to thread_local_store_test.cc, and simplify code. Signed-off-by: Joshua Marantz --- include/envoy/stats/stats.h | 4 +- source/common/stats/stats_impl.h | 44 ++++----- source/common/stats/thread_local_store.cc | 35 ++++--- source/common/stats/thread_local_store.h | 6 +- source/exe/main_common.cc | 5 +- source/server/hot_restart_impl.cc | 4 +- source/server/hot_restart_impl.h | 1 + source/server/hot_restart_nop_impl.h | 2 +- test/common/stats/stats_impl_test.cc | 4 +- test/common/stats/thread_local_store_test.cc | 99 ++++++++++++++------ test/integration/server.cc | 6 +- test/mocks/server/mocks.cc | 2 +- test/mocks/server/mocks.h | 1 - test/server/hot_restart_impl_test.cc | 13 +-- test/server/http/admin_test.cc | 2 +- test/test_common/utility.h | 17 ++-- 16 files changed, 145 insertions(+), 100 deletions(-) diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 0f91e1200153..c39e41749699 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -481,9 +481,9 @@ class StatDataAllocator { std::vector&& tags) PURE; /** - * @return const StatsOptions& the options used to initialize the allocator. + * Determines whether this stats allocator requires bounded stat-name size. */ - virtual const StatsOptions& statsOptions() const PURE; + virtual bool requiresBoundedStatNameSize() const PURE; // TODO(jmarantz): create a parallel mechanism to instantiate histograms. At // the moment, histograms don't fit the same pattern of counters and gaugaes diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 679587343387..3944d8cc3c8a 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -204,8 +204,7 @@ struct RawStatData { /** * Returns the size of this struct, accounting for the length of name_ - * and padding for alignment. Required for the HeapRawStatDataAllocator, which does not truncate - * at a maximum stat name length. + * and padding for alignment. */ static uint64_t structSize(uint64_t name_size); @@ -291,8 +290,8 @@ template class StatDataAllocatorImpl : public StatDataAllocator /** * @param stats_options The stat optinos, which must live longer than the allocator. */ - explicit StatDataAllocatorImpl(const StatsOptions& stats_options) - : stats_options_(stats_options) {} + explicit StatDataAllocatorImpl(/*const StatsOptions& stats_options*/) {} + //: stats_options_(stats_options) {} // StatDataAllocator CounterSharedPtr makeCounter(absl::string_view name, std::string&& tag_extracted_name, @@ -315,17 +314,13 @@ template class StatDataAllocatorImpl : public StatDataAllocator * @param data the data returned by alloc(). */ virtual void free(StatData& data) PURE; - - /** - * @return const StatsOptions& The options passed into the constructor. - */ - const StatsOptions& statsOptions() const override { return stats_options_; } - -private: - const StatsOptions& stats_options_; }; -using RawStatDataAllocator = StatDataAllocatorImpl; +class RawStatDataAllocator : public StatDataAllocatorImpl { +public: + // StatsDataAllocator + bool requiresBoundedStatNameSize() const override { return true; } +}; /** * Implementation of HistogramStatistics for circllhist. @@ -412,13 +407,16 @@ struct HeapStatData { */ class HeapStatDataAllocator : public StatDataAllocatorImpl { public: - explicit HeapStatDataAllocator(const StatsOptions& options) : StatDataAllocatorImpl(options) {} + HeapStatDataAllocator() {} ~HeapStatDataAllocator() { ASSERT(stats_.empty()); } - // StatDataAllocator + // StatDataAllocatorImpl HeapStatData* alloc(absl::string_view name) override; void free(HeapStatData& data) override; + // StatsDataAllocator + bool requiresBoundedStatNameSize() const override { return false; } + private: struct HeapStatHash_ { size_t operator()(const HeapStatData* a) const { return HashUtil::xxHash64(a->key()); } @@ -487,23 +485,17 @@ class IsolatedStoreImpl : public Store { : counters_([this](const std::string& name) -> CounterSharedPtr { std::string tag_extracted_name = name; std::vector tags; - return alloc_->makeCounter(name, std::move(tag_extracted_name), std::move(tags)); + return alloc_.makeCounter(name, std::move(tag_extracted_name), std::move(tags)); }), gauges_([this](const std::string& name) -> GaugeSharedPtr { std::string tag_extracted_name = name; std::vector tags; - return alloc_->makeGauge(name, std::move(tag_extracted_name), std::move(tags)); + return alloc_.makeGauge(name, std::move(tag_extracted_name), std::move(tags)); }), histograms_([this](const std::string& name) -> HistogramSharedPtr { return std::make_shared(name, *this, std::string(name), std::vector()); - }) { - // We divide by two here to get an effectively unlimited length. If we don't divide - // by two, we'll get an integer wrap in the maxNameLength addition, and the behavior - // will not be what we intend. - stats_options_.max_obj_name_length_ = std::numeric_limits::max() / 2; - alloc_ = std::make_unique(stats_options_); - } + }) {} // Stats::Scope Counter& counter(const std::string& name) override { return counters_.get(name); } @@ -546,11 +538,11 @@ class IsolatedStoreImpl : public Store { const std::string prefix_; }; - StatsOptionsImpl stats_options_; - std::unique_ptr alloc_; + HeapStatDataAllocator alloc_; IsolatedStatsCache counters_; IsolatedStatsCache gauges_; IsolatedStatsCache histograms_; + const StatsOptionsImpl stats_options_; }; } // namespace Stats diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index c2c61d457538..30cab5f90408 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -14,7 +14,7 @@ namespace Stats { ThreadLocalStoreImpl::ThreadLocalStoreImpl(const Stats::StatsOptions& stats_options, StatDataAllocator& alloc) - : alloc_(alloc), heap_allocator_(stats_options), default_scope_(createScope("")), + : stats_options_(stats_options), alloc_(alloc), default_scope_(createScope("")), tag_producer_(std::make_unique()), num_last_resort_stats_(default_scope_->counter("stats.overflow")), source_(*this) {} @@ -156,6 +156,26 @@ void ThreadLocalStoreImpl::clearScopeFromCaches(uint64_t scope_id) { } } +absl::string_view ThreadLocalStoreImpl::truncateStatNameIfNeeded(absl::string_view name) { + // If the main allocator requires stat name truncation, warn and truncate, before + // attempting to allocate. + if (alloc_.requiresBoundedStatNameSize()) { + const uint64_t max_length = stats_options_.maxNameLength(); + + // Note that the heap-allocator does not truncate itself; we have to + // truncate here if we are using heap-allocation as a fallback due to an + // exahusted shared-memroy block + if (name.size() > max_length) { + ENVOY_LOG_MISC( + warn, + "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", + name, name.size(), max_length); + name = absl::string_view(name.data(), max_length); + } + } + return name; +} + std::atomic ThreadLocalStoreImpl::ScopeImpl::next_scope_id_; ThreadLocalStoreImpl::ScopeImpl::~ScopeImpl() { parent_.releaseScopeCrossThread(this); } @@ -178,18 +198,7 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( if (!central_ref) { std::vector tags; std::string tag_extracted_name = parent_.getTagsForName(name, tags); - absl::string_view truncated_name = name; - const uint64_t max_length = parent_.statsOptions().maxNameLength(); - // Note that the heap-allocator does not truncate itself; we have to - // truncate here if we are using heap-allocation as a fallback due to an - // exahusted shared-memroy block - if (truncated_name.size() > max_length) { - ENVOY_LOG_MISC( - warn, - "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", - name, name.size(), max_length); - truncated_name = absl::string_view(name.data(), max_length); - } + absl::string_view truncated_name = parent_.truncateStatNameIfNeeded(name); std::shared_ptr stat = make_stat(parent_.alloc_, truncated_name, std::move(tag_extracted_name), std::move(tags)); if (stat == nullptr) { diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 3901e3b45ad2..cc280613ce66 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -195,7 +195,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo Source& source() override { return source_; } - const Stats::StatsOptions& statsOptions() const override { return alloc_.statsOptions(); } + const Stats::StatsOptions& statsOptions() const override { return stats_options_; } private: struct TlsCacheEntry { @@ -274,9 +274,10 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void clearScopeFromCaches(uint64_t scope_id); void releaseScopeCrossThread(ScopeImpl* scope); void mergeInternal(PostMergeCb mergeCb); + absl::string_view truncateStatNameIfNeeded(absl::string_view name); + const Stats::StatsOptions& stats_options_; StatDataAllocator& alloc_; - HeapStatDataAllocator heap_allocator_; Event::Dispatcher* main_thread_dispatcher_{}; ThreadLocal::SlotPtr tls_; mutable Thread::MutexBasicLockable lock_; @@ -287,6 +288,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo std::atomic shutting_down_{}; std::atomic merge_in_progress_{}; Counter& num_last_resort_stats_; + HeapStatDataAllocator heap_allocator_; SourceImpl source_; }; diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index 6118130ec663..862c0b95c740 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -42,7 +42,6 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) { ares_library_init(ARES_LIB_INIT_ALL); Event::Libevent::Global::initialize(); RELEASE_ASSERT(Envoy::Server::validateProtoDescriptors(), ""); - const Stats::StatsOptions& stats_options = options_.statsOptions(); switch (options_.mode()) { case Server::Mode::InitOnly: @@ -53,7 +52,7 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) { } #endif if (restarter_.get() == nullptr) { - restarter_.reset(new Server::HotRestartNopImpl(stats_options)); + restarter_.reset(new Server::HotRestartNopImpl()); } tls_.reset(new ThreadLocal::InstanceImpl); @@ -70,7 +69,7 @@ MainCommonBase::MainCommonBase(OptionsImpl& options) : options_(options) { break; } case Server::Mode::Validate: - restarter_.reset(new Server::HotRestartNopImpl(stats_options)); + restarter_.reset(new Server::HotRestartNopImpl()); Logger::Registry::initialize(options_.logLevel(), options_.logFormat(), restarter_->logLock()); break; } diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index 319595ea12ab..1e7a4501f589 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -116,8 +116,7 @@ std::string SharedMemory::version(uint64_t max_num_stats, } HotRestartImpl::HotRestartImpl(Options& options) - : Stats::RawStatDataAllocator(options.statsOptions()), options_(options), - stats_set_options_(blockMemHashOptions(options.maxStats())), + : options_(options), stats_set_options_(blockMemHashOptions(options.maxStats())), shmem_(SharedMemory::initialize( RawStatDataSet::numBytes(stats_set_options_, options_.statsOptions()), options_)), log_lock_(shmem_.log_lock_), access_log_lock_(shmem_.access_log_lock_), @@ -145,6 +144,7 @@ HotRestartImpl::HotRestartImpl(Options& options) Stats::RawStatData* HotRestartImpl::alloc(absl::string_view name) { // Try to find the existing slot in shared memory, otherwise allocate a new one. Thread::LockGuard lock(stat_lock_); + // The name is truncated in ThreadLocalStore before this is called. ASSERT(name.length() <= options_.statsOptions().maxNameLength()); auto value_created = stats_set_->insert(name); Stats::RawStatData* data = value_created.first; diff --git a/source/server/hot_restart_impl.h b/source/server/hot_restart_impl.h index c24c07b10904..83b93c0fa000 100644 --- a/source/server/hot_restart_impl.h +++ b/source/server/hot_restart_impl.h @@ -141,6 +141,7 @@ class HotRestartImpl : public HotRestart, // RawStatDataAllocator Stats::RawStatData* alloc(absl::string_view name) override; void free(Stats::RawStatData& data) override; + bool requiresBoundedStatNameSize() const override { return true; } private: enum class RpcMessageType { diff --git a/source/server/hot_restart_nop_impl.h b/source/server/hot_restart_nop_impl.h index d54130dff395..8444dea459a4 100644 --- a/source/server/hot_restart_nop_impl.h +++ b/source/server/hot_restart_nop_impl.h @@ -14,7 +14,7 @@ namespace Server { */ class HotRestartNopImpl : public Server::HotRestart { public: - HotRestartNopImpl(const Stats::StatsOptions& stats_options) : stats_allocator_(stats_options) {} + HotRestartNopImpl() {} // Server::HotRestart void drainParentListeners() override {} diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index 1a05f3d0268e..d976a5a5ffdf 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -491,7 +491,7 @@ TEST(TagProducerTest, CheckConstructor) { // No truncation occurs in the implementation of HeapStatData. TEST(RawStatDataTest, HeapNoTruncate) { Stats::StatsOptionsImpl stats_options; - HeapStatDataAllocator alloc(stats_options); + HeapStatDataAllocator alloc; //(/*stats_options*/); const std::string long_string(stats_options.maxNameLength() + 1, 'A'); HeapStatData* stat{}; EXPECT_NO_LOGS(stat = alloc.alloc(long_string)); @@ -502,7 +502,7 @@ TEST(RawStatDataTest, HeapNoTruncate) { // Note: a similar test using RawStatData* is in test/server/hot_restart_impl_test.cc. TEST(RawStatDataTest, HeapAlloc) { Stats::StatsOptionsImpl stats_options; - HeapStatDataAllocator alloc(stats_options); + HeapStatDataAllocator alloc; //(stats_options); HeapStatData* stat_1 = alloc.alloc("ref_name"); ASSERT_NE(stat_1, nullptr); HeapStatData* stat_2 = alloc.alloc("ref_name"); diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index e2efaf3edd01..286f42029a06 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -28,15 +28,20 @@ namespace Stats { class StatsThreadLocalStoreTest : public testing::Test { public: - StatsThreadLocalStoreTest() : alloc_(options_) { - store_ = std::make_unique(options_, alloc_); + void SetUp() override { + alloc_ = std::make_unique(options_); + resetStoreWithAlloc(*alloc_); + } + + void resetStoreWithAlloc(StatDataAllocator& alloc) { + store_ = std::make_unique(options_, alloc); store_->addSink(sink_); } NiceMock main_thread_dispatcher_; NiceMock tls_; StatsOptionsImpl options_; - MockedTestAllocator alloc_; + std::unique_ptr alloc_; MockSink sink_; std::unique_ptr store_; }; @@ -151,7 +156,7 @@ class HistogramTest : public testing::Test { TEST_F(StatsThreadLocalStoreTest, NoTls) { InSequence s; - EXPECT_CALL(alloc_, alloc(_)).Times(2); + EXPECT_CALL(*alloc_, alloc(_)).Times(2); Counter& c1 = store_->counter("c1"); EXPECT_EQ(&c1, &store_->counter("c1")); @@ -175,7 +180,7 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) { EXPECT_EQ(2L, store_->gauges().front().use_count()); // Includes overflow stat. - EXPECT_CALL(alloc_, free(_)).Times(3); + EXPECT_CALL(*alloc_, free(_)).Times(3); store_->shutdownThreading(); } @@ -184,7 +189,7 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(alloc_, alloc(_)).Times(2); + EXPECT_CALL(*alloc_, alloc(_)).Times(2); Counter& c1 = store_->counter("c1"); EXPECT_EQ(&c1, &store_->counter("c1")); @@ -213,7 +218,7 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { EXPECT_EQ(2L, store_->gauges().front().use_count()); // Includes overflow stat. - EXPECT_CALL(alloc_, free(_)).Times(3); + EXPECT_CALL(*alloc_, free(_)).Times(3); } TEST_F(StatsThreadLocalStoreTest, BasicScope) { @@ -221,7 +226,7 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope("scope1."); - EXPECT_CALL(alloc_, alloc(_)).Times(4); + EXPECT_CALL(*alloc_, alloc(_)).Times(4); Counter& c1 = store_->counter("c1"); Counter& c2 = scope1->counter("c2"); EXPECT_EQ("c1", c1.name()); @@ -247,7 +252,7 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(alloc_, free(_)).Times(5); + EXPECT_CALL(*alloc_, free(_)).Times(5); } // Validate that we sanitize away bad characters in the stats prefix. @@ -256,7 +261,7 @@ TEST_F(StatsThreadLocalStoreTest, SanitizePrefix) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope(std::string("scope1:\0:foo.", 13)); - EXPECT_CALL(alloc_, alloc(_)); + EXPECT_CALL(*alloc_, alloc(_)); Counter& c1 = scope1->counter("c1"); EXPECT_EQ("scope1___foo.c1", c1.name()); @@ -264,7 +269,7 @@ TEST_F(StatsThreadLocalStoreTest, SanitizePrefix) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(alloc_, free(_)).Times(2); + EXPECT_CALL(*alloc_, free(_)).Times(2); } TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { @@ -272,7 +277,7 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope("scope1."); - EXPECT_CALL(alloc_, alloc(_)); + EXPECT_CALL(*alloc_, alloc(_)); scope1->counter("c1"); EXPECT_EQ(2UL, store_->counters().size()); CounterSharedPtr c1 = store_->counters().front(); @@ -287,7 +292,7 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { store_->source().clearCache(); EXPECT_EQ(1UL, store_->source().cachedCounters().size()); - EXPECT_CALL(alloc_, free(_)); + EXPECT_CALL(*alloc_, free(_)); EXPECT_EQ(1L, c1.use_count()); c1.reset(); @@ -295,7 +300,7 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(alloc_, free(_)); + EXPECT_CALL(*alloc_, free(_)); } TEST_F(StatsThreadLocalStoreTest, NestedScopes) { @@ -303,12 +308,12 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) { store_->initializeThreading(main_thread_dispatcher_, tls_); ScopePtr scope1 = store_->createScope("scope1."); - EXPECT_CALL(alloc_, alloc(_)); + EXPECT_CALL(*alloc_, alloc(_)); Counter& c1 = scope1->counter("foo.bar"); EXPECT_EQ("scope1.foo.bar", c1.name()); ScopePtr scope2 = scope1->createScope("foo."); - EXPECT_CALL(alloc_, alloc(_)); + EXPECT_CALL(*alloc_, alloc(_)); Counter& c2 = scope2->counter("bar"); EXPECT_NE(&c1, &c2); EXPECT_EQ("scope1.foo.bar", c2.name()); @@ -318,7 +323,7 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) { EXPECT_EQ(1UL, c1.value()); EXPECT_EQ(c1.value(), c2.value()); - EXPECT_CALL(alloc_, alloc(_)); + EXPECT_CALL(*alloc_, alloc(_)); Gauge& g1 = scope2->gauge("some_gauge"); EXPECT_EQ("scope1.foo.some_gauge", g1.name()); @@ -326,7 +331,7 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(alloc_, free(_)).Times(4); + EXPECT_CALL(*alloc_, free(_)).Times(4); } TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { @@ -339,7 +344,7 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { ScopePtr scope2 = store_->createScope("scope1."); // We will call alloc twice, but they should point to the same backing storage. - EXPECT_CALL(alloc_, alloc(_)).Times(2); + EXPECT_CALL(*alloc_, alloc(_)).Times(2); Counter& c1 = scope1->counter("c"); Counter& c2 = scope2->counter("c"); EXPECT_NE(&c1, &c2); @@ -354,7 +359,7 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { EXPECT_EQ(2UL, store_->counters().size()); // Gauges should work the same way. - EXPECT_CALL(alloc_, alloc(_)).Times(2); + EXPECT_CALL(*alloc_, alloc(_)).Times(2); Gauge& g1 = scope1->gauge("g"); Gauge& g2 = scope2->gauge("g"); EXPECT_NE(&g1, &g2); @@ -367,7 +372,7 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { EXPECT_EQ(1UL, store_->gauges().size()); // Deleting scope 1 will call free but will be reference counted. It still leaves scope 2 valid. - EXPECT_CALL(alloc_, free(_)).Times(2); + EXPECT_CALL(*alloc_, free(_)).Times(2); scope1.reset(); c2.inc(); EXPECT_EQ(3UL, c2.value()); @@ -380,14 +385,14 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(alloc_, free(_)).Times(3); + EXPECT_CALL(*alloc_, free(_)).Times(3); } TEST_F(StatsThreadLocalStoreTest, AllocFailed) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(alloc_, alloc(absl::string_view("foo"))).WillOnce(Return(nullptr)); + EXPECT_CALL(*alloc_, alloc(absl::string_view("foo"))).WillOnce(Return(nullptr)); Counter& c1 = store_->counter("foo"); EXPECT_EQ(1UL, store_->counter("stats.overflow").value()); @@ -398,10 +403,10 @@ TEST_F(StatsThreadLocalStoreTest, AllocFailed) { tls_.shutdownThread(); // Includes overflow but not the failsafe stat which we allocated from the heap. - EXPECT_CALL(alloc_, free(_)); + EXPECT_CALL(*alloc_, free(_)); } -TEST_F(StatsThreadLocalStoreTest, Truncation) { +TEST_F(StatsThreadLocalStoreTest, HotRestartTruncation) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); @@ -409,7 +414,7 @@ TEST_F(StatsThreadLocalStoreTest, Truncation) { const uint64_t max_name_length = options_.maxNameLength(); const std::string name_1(max_name_length + 1, 'A'); - EXPECT_CALL(alloc_, alloc(_)); + EXPECT_CALL(*alloc_, alloc(_)); store_->counter(name_1); // The stats did not overflow yet. @@ -424,7 +429,7 @@ TEST_F(StatsThreadLocalStoreTest, Truncation) { // The same should be true with heap allocation, which occurs when the default // allocator fails. const std::string name_2(max_name_length + 1, 'B'); - EXPECT_CALL(alloc_, alloc(_)).WillOnce(Return(nullptr)); + EXPECT_CALL(*alloc_, alloc(_)).WillOnce(Return(nullptr)); store_->counter(name_2); // Same deal: the name will be truncated, so we won't be able to find it with the entire name. @@ -441,14 +446,46 @@ TEST_F(StatsThreadLocalStoreTest, Truncation) { // Includes overflow, and the first raw-allocated stat, but not the failsafe stat which we // allocated from the heap. - EXPECT_CALL(alloc_, free(_)).Times(2); + EXPECT_CALL(*alloc_, free(_)).Times(2); +} + +class HeapStatsThreadLocalStoreTest : public StatsThreadLocalStoreTest { +public: + void SetUp() override { + resetStoreWithAlloc(heap_alloc_); + // Note: we do not call StatsThreadLocalStoreTest::SetUp here as that + // sets up a thread_local_store with raw stat alloc. + } + void TearDown() override { + store_.reset(); // delete before the allocator. + } + + HeapStatDataAllocator heap_alloc_; +}; + +TEST_F(HeapStatsThreadLocalStoreTest, NonHotRestartNoTruncation) { + InSequence s; + store_->initializeThreading(main_thread_dispatcher_, tls_); + + // Allocate a stat greater than the max name length. + const uint64_t max_name_length = options_.maxNameLength(); + const std::string name_1(max_name_length + 1, 'A'); + + store_->counter(name_1); + + // This works fine, and we can find it by its long name because heap-stats do not + // get truncsated. + EXPECT_NE(nullptr, TestUtility::findCounter(*store_, name_1).get()); + + store_->shutdownThreading(); + tls_.shutdownThread(); } TEST_F(StatsThreadLocalStoreTest, ShuttingDown) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - EXPECT_CALL(alloc_, alloc(_)).Times(4); + EXPECT_CALL(*alloc_, alloc(_)).Times(4); store_->counter("c1"); store_->gauge("g1"); store_->shutdownThreading(); @@ -464,7 +501,7 @@ TEST_F(StatsThreadLocalStoreTest, ShuttingDown) { tls_.shutdownThread(); // Includes overflow stat. - EXPECT_CALL(alloc_, free(_)).Times(5); + EXPECT_CALL(*alloc_, free(_)).Times(5); } TEST_F(StatsThreadLocalStoreTest, MergeDuringShutDown) { @@ -487,7 +524,7 @@ TEST_F(StatsThreadLocalStoreTest, MergeDuringShutDown) { tls_.shutdownThread(); - EXPECT_CALL(alloc_, free(_)); + EXPECT_CALL(*alloc_, free(_)); } // Histogram tests diff --git a/test/integration/server.cc b/test/integration/server.cc index 2fe7854d81bd..b2a3056a3472 100644 --- a/test/integration/server.cc +++ b/test/integration/server.cc @@ -88,12 +88,12 @@ void IntegrationTestServer::onWorkerListenerRemoved() { void IntegrationTestServer::threadRoutine(const Network::Address::IpVersion version, bool deterministic) { Server::TestOptionsImpl options(config_path_, version); - Stats::StatsOptionsImpl stats_options; - Server::HotRestartNopImpl restarter(stats_options); + Server::HotRestartNopImpl restarter; Thread::MutexBasicLockable lock; ThreadLocal::InstanceImpl tls; - Stats::HeapStatDataAllocator stats_allocator(stats_options); + Stats::HeapStatDataAllocator stats_allocator; + Stats::StatsOptionsImpl stats_options; Stats::ThreadLocalStoreImpl stats_store(stats_options, stats_allocator); stat_store_ = &stats_store; Runtime::RandomGeneratorPtr random_generator; diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 515a010fd8fb..46920ab56429 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -61,7 +61,7 @@ MockGuardDog::MockGuardDog() : watch_dog_(new NiceMock()) { } MockGuardDog::~MockGuardDog() {} -MockHotRestart::MockHotRestart() : stats_allocator_(stats_options_) { +MockHotRestart::MockHotRestart() { ON_CALL(*this, logLock()).WillByDefault(ReturnRef(log_lock_)); ON_CALL(*this, accessLogLock()).WillByDefault(ReturnRef(access_log_lock_)); ON_CALL(*this, statsAllocator()).WillByDefault(ReturnRef(stats_allocator_)); diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 03cbb1a6bb30..83fbea4dcfbb 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -178,7 +178,6 @@ class MockHotRestart : public HotRestart { private: Thread::MutexBasicLockable log_lock_; Thread::MutexBasicLockable access_log_lock_; - Stats::StatsOptionsImpl stats_options_; Stats::HeapStatDataAllocator stats_allocator_; }; diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index bd021217ffdd..8fb73bf66ae1 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -94,18 +94,19 @@ TEST_F(HotRestartImplTest, versionString) { TEST_F(HotRestartImplTest, RawAllocTooLarge) { setup(); - const std::string long_string(hot_restart_->statsOptions().maxNameLength() + 1, 'A'); + const std::string long_string(stats_options_.maxNameLength() + 1, 'A'); EXPECT_DEATH_LOG_TO_STDERR(hot_restart_->alloc(long_string), "name.length"); - //"name\\.length\\(\\) \\<\\= options\\_.statsOptions\\(\\)\\.maxNameLength\\(\\)"); } -// Check consistency of internal stat representation +// Check consistency of internal raw stat representation by comparing hash of +// memory contents against a previously recorded value. TEST_F(HotRestartImplTest, Consistency) { setup(); - // Generate a stat, encode it to hex, and take the hash of that hex string. We expect the hash to - // vary only when the internal representation of a stat has been intentionally changed, in which - // case SharedMemory::VERSION should be incremented as well. + // Generate a stat, encode it to hex, and take the hash of that hex string. We + // expect the hash to vary only when the internal representation of a stat has + // been intentionally changed, in which case SharedMemory::VERSION should be + // incremented as well. const uint64_t expected_hash = 1874506077228772558; const uint64_t max_name_length = stats_options_.maxNameLength(); diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index aa0ea1e86f0b..fcf7355357cb 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -781,7 +781,7 @@ TEST_P(AdminInstanceTest, PostRequest) { class PrometheusStatsFormatterTest : public testing::Test { protected: - PrometheusStatsFormatterTest() : alloc_(stats_options_) {} + PrometheusStatsFormatterTest() /*: alloc_(stats_options_)*/ {} void addCounter(const std::string& name, std::vector cluster_tags) { std::string tname = std::string(name); counters_.push_back(alloc_.makeCounter(name, std::move(tname), std::move(cluster_tags))); diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 957e23d212ab..e455bb24d45c 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -341,17 +341,17 @@ namespace Stats { */ class TestAllocator : public RawStatDataAllocator { public: - TestAllocator(const StatsOptions& stats_options) : RawStatDataAllocator(stats_options) {} + TestAllocator(const StatsOptions& stats_options) : stats_options_(stats_options) {} ~TestAllocator() { EXPECT_TRUE(stats_.empty()); } RawStatData* alloc(absl::string_view name) override { - Stats::StatsOptionsImpl stats_options; - stats_options.max_obj_name_length_ = 127; + // Stats::StatsOptionsImpl stats_options; + // stats_options.max_obj_name_length_ = 127; CSmartPtr& stat_ref = stats_[std::string(name)]; if (!stat_ref) { stat_ref.reset(static_cast( - ::calloc(RawStatData::structSizeWithOptions(stats_options), 1))); - stat_ref->initialize(name, stats_options); + ::calloc(RawStatData::structSizeWithOptions(stats_options_), 1))); + stat_ref->initialize(name, stats_options_); } else { stat_ref->ref_count_++; } @@ -372,12 +372,13 @@ class TestAllocator : public RawStatDataAllocator { private: static void freeAdapter(RawStatData* data) { ::free(data); } std::unordered_map> stats_; + const StatsOptions& stats_options_; }; class MockedTestAllocator : public RawStatDataAllocator { public: MockedTestAllocator(const StatsOptions& stats_options) - : RawStatDataAllocator(stats_options), alloc_(stats_options) { + : /*RawStatDataAllocator(stats_options), */ alloc_(stats_options) { ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](absl::string_view name) -> RawStatData* { return alloc_.alloc(name); })); @@ -386,11 +387,15 @@ class MockedTestAllocator : public RawStatDataAllocator { return alloc_.free(data); })); + // ON_CALL(*this, requiresBoundedStatNameSize()).WillByDefault.Invoke([] -> bool { return true; + // }); + EXPECT_CALL(*this, alloc(absl::string_view("stats.overflow"))); } MOCK_METHOD1(alloc, RawStatData*(absl::string_view name)); MOCK_METHOD1(free, void(RawStatData& data)); + // MOCK_METHOD0(requiresBoundedStatNameSize, bool()); TestAllocator alloc_; }; From 5f82615c70f405c61db42ac50330ee47eaa08354 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 29 Jul 2018 10:46:32 -0400 Subject: [PATCH 22/28] Cleanup and restore truncation-message log test. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.h | 6 +++--- test/common/stats/BUILD | 1 + test/common/stats/thread_local_store_test.cc | 3 ++- test/server/hot_restart_impl_test.cc | 5 ++++- test/test_common/utility.h | 6 ------ 5 files changed, 10 insertions(+), 11 deletions(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 3944d8cc3c8a..98de62f9554e 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -282,9 +282,9 @@ class MetricImpl : public virtual Metric { // // Also note that RawStatData needs to live in a shared memory block, and it's // possible, but not obvious, that a vptr would be usable across processes. In -// any case, RawStatData is currently allocated with calloc() rather than new, -// so the usual C++ compiler assistance for setting up vptrs will not be -// available. This could be resolved with placed new. +// any case, RawStatData is allocated from a shared-memory block rather than via +// new, so the usual C++ compiler assistance for setting up vptrs will not be +// available. This could be resolved with placed new, or another nesting level. template class StatDataAllocatorImpl : public StatDataAllocator { public: /** diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index 863c7b626e10..a157fb6cdad8 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -30,6 +30,7 @@ envoy_cc_test( "//test/mocks/server:server_mocks", "//test/mocks/stats:stats_mocks", "//test/mocks/thread_local:thread_local_mocks", + "//test/test_common:logging_lib", "//test/test_common:utility_lib", ], ) diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 286f42029a06..a32cc1bd4bb5 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -10,6 +10,7 @@ #include "test/mocks/server/mocks.h" #include "test/mocks/stats/mocks.h" #include "test/mocks/thread_local/mocks.h" +#include "test/test_common/logging.h" #include "test/test_common/utility.h" #include "absl/strings/str_split.h" @@ -415,7 +416,7 @@ TEST_F(StatsThreadLocalStoreTest, HotRestartTruncation) { const std::string name_1(max_name_length + 1, 'A'); EXPECT_CALL(*alloc_, alloc(_)); - store_->counter(name_1); + EXPECT_LOG_CONTAINS("warning", "is too long with", store_->counter(name_1)); // The stats did not overflow yet. EXPECT_EQ(0UL, store_->counter("stats.overflow").value()); diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 8fb73bf66ae1..75a361e476d0 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -90,10 +90,13 @@ TEST_F(HotRestartImplTest, versionString) { } } -// See also StatsImplTest.RawTruncateWithoutAllocator. TEST_F(HotRestartImplTest, RawAllocTooLarge) { setup(); + // When running in a server, we depend on ThreadLocalStore to do name + // truncation, to ensure consistency names between shared-memory stats and + // fallback heap-allocated stats. But we have an assertion that the size + /// is appropriate in the hot-restart allocator. const std::string long_string(stats_options_.maxNameLength() + 1, 'A'); EXPECT_DEATH_LOG_TO_STDERR(hot_restart_->alloc(long_string), "name.length"); } diff --git a/test/test_common/utility.h b/test/test_common/utility.h index e455bb24d45c..038b8cc863b1 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -345,8 +345,6 @@ class TestAllocator : public RawStatDataAllocator { ~TestAllocator() { EXPECT_TRUE(stats_.empty()); } RawStatData* alloc(absl::string_view name) override { - // Stats::StatsOptionsImpl stats_options; - // stats_options.max_obj_name_length_ = 127; CSmartPtr& stat_ref = stats_[std::string(name)]; if (!stat_ref) { stat_ref.reset(static_cast( @@ -387,15 +385,11 @@ class MockedTestAllocator : public RawStatDataAllocator { return alloc_.free(data); })); - // ON_CALL(*this, requiresBoundedStatNameSize()).WillByDefault.Invoke([] -> bool { return true; - // }); - EXPECT_CALL(*this, alloc(absl::string_view("stats.overflow"))); } MOCK_METHOD1(alloc, RawStatData*(absl::string_view name)); MOCK_METHOD1(free, void(RawStatData& data)); - // MOCK_METHOD0(requiresBoundedStatNameSize, bool()); TestAllocator alloc_; }; From 8dbaff29c0a0d9a2c7fd2abe48e4064d3f1974bb Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 29 Jul 2018 14:33:12 -0400 Subject: [PATCH 23/28] Return nullptr when HotRestartImpl::alloc is asked for a name too large, rather than asserting, as thi is easier to test in release mode. Signed-off-by: Joshua Marantz --- source/server/hot_restart_impl.cc | 6 +++++- test/server/hot_restart_impl_test.cc | 3 ++- test/test_common/utility.h | 4 ++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index 1e7a4501f589..1911d060c244 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -145,7 +145,11 @@ Stats::RawStatData* HotRestartImpl::alloc(absl::string_view name) { // Try to find the existing slot in shared memory, otherwise allocate a new one. Thread::LockGuard lock(stat_lock_); // The name is truncated in ThreadLocalStore before this is called. - ASSERT(name.length() <= options_.statsOptions().maxNameLength()); + size_t max_length = options_.statsOptions().maxNameLength(); + if (name.length() > max_length) { + ENVOY_LOG(error, "HotRestartImpl::alloc({}) exceeded max length {}", name, max_length); + return nullptr; + } auto value_created = stats_set_->insert(name); Stats::RawStatData* data = value_created.first; if (data == nullptr) { diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 75a361e476d0..1d02da03cdd8 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -98,7 +98,8 @@ TEST_F(HotRestartImplTest, RawAllocTooLarge) { // fallback heap-allocated stats. But we have an assertion that the size /// is appropriate in the hot-restart allocator. const std::string long_string(stats_options_.maxNameLength() + 1, 'A'); - EXPECT_DEATH_LOG_TO_STDERR(hot_restart_->alloc(long_string), "name.length"); + EXPECT_LOG_CONTAINS("error", "exceeded max length", + EXPECT_EQ(nullptr, hot_restart_->alloc(long_string))); } // Check consistency of internal raw stat representation by comparing hash of diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 038b8cc863b1..9ca98a0de745 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -335,6 +335,7 @@ class TestHeaderMapImpl : public HeaderMapImpl { } // namespace Http namespace Stats { + /** * This is a heap test allocator that works similar to how the shared memory allocator works in * terms of reference counting, etc. @@ -375,8 +376,7 @@ class TestAllocator : public RawStatDataAllocator { class MockedTestAllocator : public RawStatDataAllocator { public: - MockedTestAllocator(const StatsOptions& stats_options) - : /*RawStatDataAllocator(stats_options), */ alloc_(stats_options) { + MockedTestAllocator(const StatsOptions& stats_options) : alloc_(stats_options) { ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](absl::string_view name) -> RawStatData* { return alloc_.alloc(name); })); From 700d540019d8f9bacfd8563362e75987caafb198 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 30 Jul 2018 00:19:30 -0400 Subject: [PATCH 24/28] style fixes & typos Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.h | 5 ++--- source/common/stats/thread_local_store.cc | 3 +++ test/test_common/utility.cc | 20 ++++++++++++++++++++ test/test_common/utility.h | 13 ++----------- 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 98de62f9554e..5188b33c49b8 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -288,10 +288,9 @@ class MetricImpl : public virtual Metric { template class StatDataAllocatorImpl : public StatDataAllocator { public: /** - * @param stats_options The stat optinos, which must live longer than the allocator. + * @param stats_options The stat options, which must live longer than the allocator. */ - explicit StatDataAllocatorImpl(/*const StatsOptions& stats_options*/) {} - //: stats_options_(stats_options) {} + StatDataAllocatorImpl() {} // StatDataAllocator CounterSharedPtr makeCounter(absl::string_view name, std::string&& tag_extracted_name, diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 30cab5f90408..a4c5431fa37d 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -197,6 +197,9 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( std::shared_ptr& central_ref = central_cache_map[name]; if (!central_ref) { std::vector tags; + + // Tag extraction occurs on the original, untruncated name so the extraction + // can complete properly, even if the tag values are partially truncated. std::string tag_extracted_name = parent_.getTagsForName(name, tags); absl::string_view truncated_name = parent_.truncateStatNameIfNeeded(name); std::shared_ptr stat = diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 85508ea5e675..5427a548ea28 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -226,4 +226,24 @@ bool TestHeaderMapImpl::has(const std::string& key) { return get(LowerCaseString bool TestHeaderMapImpl::has(const LowerCaseString& key) { return get(key) != nullptr; } } // namespace Http + +namespace Stats { + +MockedTestAllocator::MockedTestAllocator(const StatsOptions& stats_options) + : alloc_(stats_options) { + ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](absl::string_view name) -> RawStatData* { + return alloc_.alloc(name); + })); + + ON_CALL(*this, free(_)).WillByDefault(Invoke([this](RawStatData& data) -> void { + return alloc_.free(data); + })); + + EXPECT_CALL(*this, alloc(absl::string_view("stats.overflow"))); +} + +MockedTestAllocator::~MockedTestAllocator() {} + +} // namespace Stats + } // namespace Envoy diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 9ca98a0de745..04bca5ca951c 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -376,17 +376,8 @@ class TestAllocator : public RawStatDataAllocator { class MockedTestAllocator : public RawStatDataAllocator { public: - MockedTestAllocator(const StatsOptions& stats_options) : alloc_(stats_options) { - ON_CALL(*this, alloc(_)).WillByDefault(Invoke([this](absl::string_view name) -> RawStatData* { - return alloc_.alloc(name); - })); - - ON_CALL(*this, free(_)).WillByDefault(Invoke([this](RawStatData& data) -> void { - return alloc_.free(data); - })); - - EXPECT_CALL(*this, alloc(absl::string_view("stats.overflow"))); - } + MockedTestAllocator(const StatsOptions& stats_options); + virtual ~MockedTestAllocator(); MOCK_METHOD1(alloc, RawStatData*(absl::string_view name)); MOCK_METHOD1(free, void(RawStatData& data)); From 319462b4add4be8bbb9e0039762fda7ad7d2a96c Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 30 Jul 2018 15:51:38 -0400 Subject: [PATCH 25/28] Address review comments, style issues, etc. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.h | 5 ----- source/common/stats/thread_local_store.cc | 2 +- source/server/hot_restart_impl.cc | 4 +++- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 5188b33c49b8..13dfc6a77b9c 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -287,11 +287,6 @@ class MetricImpl : public virtual Metric { // available. This could be resolved with placed new, or another nesting level. template class StatDataAllocatorImpl : public StatDataAllocator { public: - /** - * @param stats_options The stat options, which must live longer than the allocator. - */ - StatDataAllocatorImpl() {} - // StatDataAllocator CounterSharedPtr makeCounter(absl::string_view name, std::string&& tag_extracted_name, std::vector&& tags) override; diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index a4c5431fa37d..0467e47f587a 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -164,7 +164,7 @@ absl::string_view ThreadLocalStoreImpl::truncateStatNameIfNeeded(absl::string_vi // Note that the heap-allocator does not truncate itself; we have to // truncate here if we are using heap-allocation as a fallback due to an - // exahusted shared-memroy block + // exahusted shared-memory block if (name.size() > max_length) { ENVOY_LOG_MISC( warn, diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index 1911d060c244..a63b13124691 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -144,7 +144,9 @@ HotRestartImpl::HotRestartImpl(Options& options) Stats::RawStatData* HotRestartImpl::alloc(absl::string_view name) { // Try to find the existing slot in shared memory, otherwise allocate a new one. Thread::LockGuard lock(stat_lock_); - // The name is truncated in ThreadLocalStore before this is called. + // In production, the name is truncated in ThreadLocalStore before this + // is called. This is just a sanity check to make sure that actually happens; + // it is coded as an if/return-null to facilitate testing. size_t max_length = options_.statsOptions().maxNameLength(); if (name.length() > max_length) { ENVOY_LOG(error, "HotRestartImpl::alloc({}) exceeded max length {}", name, max_length); From c0f94997247b3fd156655bb6e441ab260deaa041 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 30 Jul 2018 16:00:23 -0400 Subject: [PATCH 26/28] Use ASSERT for length-check when allocating stats in hot-restart. Signed-off-by: Joshua Marantz --- source/server/hot_restart_impl.cc | 6 +----- test/server/hot_restart_impl_test.cc | 12 ------------ 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/source/server/hot_restart_impl.cc b/source/server/hot_restart_impl.cc index a63b13124691..80b4f2531307 100644 --- a/source/server/hot_restart_impl.cc +++ b/source/server/hot_restart_impl.cc @@ -147,11 +147,7 @@ Stats::RawStatData* HotRestartImpl::alloc(absl::string_view name) { // In production, the name is truncated in ThreadLocalStore before this // is called. This is just a sanity check to make sure that actually happens; // it is coded as an if/return-null to facilitate testing. - size_t max_length = options_.statsOptions().maxNameLength(); - if (name.length() > max_length) { - ENVOY_LOG(error, "HotRestartImpl::alloc({}) exceeded max length {}", name, max_length); - return nullptr; - } + ASSERT(name.length() <= options_.statsOptions().maxNameLength()); auto value_created = stats_set_->insert(name); Stats::RawStatData* data = value_created.first; if (data == nullptr) { diff --git a/test/server/hot_restart_impl_test.cc b/test/server/hot_restart_impl_test.cc index 1d02da03cdd8..4216a265c8fe 100644 --- a/test/server/hot_restart_impl_test.cc +++ b/test/server/hot_restart_impl_test.cc @@ -90,18 +90,6 @@ TEST_F(HotRestartImplTest, versionString) { } } -TEST_F(HotRestartImplTest, RawAllocTooLarge) { - setup(); - - // When running in a server, we depend on ThreadLocalStore to do name - // truncation, to ensure consistency names between shared-memory stats and - // fallback heap-allocated stats. But we have an assertion that the size - /// is appropriate in the hot-restart allocator. - const std::string long_string(stats_options_.maxNameLength() + 1, 'A'); - EXPECT_LOG_CONTAINS("error", "exceeded max length", - EXPECT_EQ(nullptr, hot_restart_->alloc(long_string))); -} - // Check consistency of internal raw stat representation by comparing hash of // memory contents against a previously recorded value. TEST_F(HotRestartImplTest, Consistency) { From 1b89d7fd6eca1a9c65eaf47636b8a759dc2d55a0 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 31 Jul 2018 12:56:25 -0400 Subject: [PATCH 27/28] Address review nits. Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.cc | 5 ++--- source/common/stats/stats_impl.h | 6 +++--- source/server/hot_restart_impl.h | 1 - 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 7f74145b9d9a..681bc77d640f 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -138,9 +138,8 @@ bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector HeapStatData::HeapStatData(absl::string_view key) : name_(key.data(), key.size()) {} HeapStatData* HeapStatDataAllocator::alloc(absl::string_view name) { - // Do not truncate here for non-hot-restart case; truncate at the call-site in - // ThreadLocalStoreImpl::ScopeImpl::safeMakeStat() only when allocating a heap-stat - // as a fallback when we've exhausted our shared-memory block. + // Any expected truncation of name is done at the callsite. No truncation is + // required to use this allocator. auto data = std::make_unique(name); Thread::ReleasableLockGuard lock(mutex_); auto ret = stats_.insert(data.get()); diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 13dfc6a77b9c..dc4b5b6ed9c0 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -421,9 +421,9 @@ class HeapStatDataAllocator : public StatDataAllocatorImpl { } }; - // TODO(jmarantz): See https://github.com/envoyproxy/envoy/pull/3927 which can - // help reorganize the heap stats using a ref-counted symbol table to compress - // the stat strings. + // TODO(jmarantz): See https://github.com/envoyproxy/envoy/pull/3927 and + // https://github.com/envoyproxy/envoy/issues/3585, which can help reorganize + // the heap stats using a ref-counted symbol table to compress the stat strings. typedef std::unordered_set StatSet; // An unordered set of HeapStatData pointers which keys off the key() diff --git a/source/server/hot_restart_impl.h b/source/server/hot_restart_impl.h index 83b93c0fa000..c24c07b10904 100644 --- a/source/server/hot_restart_impl.h +++ b/source/server/hot_restart_impl.h @@ -141,7 +141,6 @@ class HotRestartImpl : public HotRestart, // RawStatDataAllocator Stats::RawStatData* alloc(absl::string_view name) override; void free(Stats::RawStatData& data) override; - bool requiresBoundedStatNameSize() const override { return true; } private: enum class RpcMessageType { From 08fe789dfdc18369e78719f92b5bbf447355407d Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 31 Jul 2018 14:27:39 -0400 Subject: [PATCH 28/28] Correct spelling of StatDataAllocator -- this time it will stick for sure... Signed-off-by: Joshua Marantz --- source/common/stats/stats_impl.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index dc4b5b6ed9c0..a007d4a2a21d 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -312,7 +312,7 @@ template class StatDataAllocatorImpl : public StatDataAllocator class RawStatDataAllocator : public StatDataAllocatorImpl { public: - // StatsDataAllocator + // StatDataAllocator bool requiresBoundedStatNameSize() const override { return true; } }; @@ -408,7 +408,7 @@ class HeapStatDataAllocator : public StatDataAllocatorImpl { HeapStatData* alloc(absl::string_view name) override; void free(HeapStatData& data) override; - // StatsDataAllocator + // StatDataAllocator bool requiresBoundedStatNameSize() const override { return false; } private: