From deb76f89586a1381ad53dab798479d28502c4629 Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Wed, 27 Oct 2021 22:10:50 +0000 Subject: [PATCH 01/12] Add ability to filter stats to be flushed to sinks. Signed-off-by: Pradeep Rao --- envoy/server/instance.h | 9 + envoy/stats/allocator.h | 15 ++ envoy/stats/sink.h | 17 ++ envoy/stats/store.h | 21 +++ source/common/stats/allocator_impl.cc | 87 +++++++++ source/common/stats/allocator_impl.h | 19 ++ source/common/stats/isolated_store_impl.h | 31 +++- source/common/stats/thread_local_store.cc | 61 ++++++- source/common/stats/thread_local_store.h | 15 ++ source/server/config_validation/server.h | 3 + source/server/configuration_impl.h | 7 + source/server/hot_restarting_parent.cc | 24 +-- source/server/server.cc | 21 ++- source/server/server.h | 6 + test/common/stats/allocator_impl_test.cc | 175 ++++++++++++++++++- test/common/stats/thread_local_store_test.cc | 2 +- test/integration/server.h | 30 ++++ test/mocks/server/instance.h | 2 + test/mocks/stats/mocks.h | 3 + test/server/server_test.cc | 10 ++ tools/spelling/spelling_dictionary.txt | 1 + 21 files changed, 525 insertions(+), 34 deletions(-) diff --git a/envoy/server/instance.h b/envoy/server/instance.h index 880aa4889b376..16827ba5ffa34 100644 --- a/envoy/server/instance.h +++ b/envoy/server/instance.h @@ -2,6 +2,7 @@ #include #include +#include #include #include "envoy/access_log/access_log.h" @@ -31,6 +32,11 @@ #include "envoy/upstream/cluster_manager.h" namespace Envoy { + +namespace Stats { +class SinkPredicates; +} + namespace Server { /** @@ -269,6 +275,9 @@ class Instance { * TODO(mattklein123): This can be removed when version 1.20.0 is no longer supported. */ virtual bool enableReusePortDefault() PURE; + + virtual void + setSinkPredicates(std::unique_ptr sink_predicates) PURE; }; } // namespace Server diff --git a/envoy/stats/allocator.h b/envoy/stats/allocator.h index 2924ebf0ab303..1eb387b59b6ed 100644 --- a/envoy/stats/allocator.h +++ b/envoy/stats/allocator.h @@ -18,6 +18,9 @@ namespace Envoy { namespace Stats { +class Sink; +class SinkPredicates; + /** * Abstract interface for allocating statistics. Implementations can * be created utilizing a single fixed-size block suitable for @@ -84,6 +87,18 @@ class Allocator { virtual void forEachTextReadout(std::function f_size, std::function f_stat) const PURE; + virtual void forEachSinkedCounter(std::function f_size, + std::function f_stat) const PURE; + virtual void forEachSinkedGauge(std::function f_size, + std::function f_stat) const PURE; + virtual void forEachSinkedTextReadout(std::function f_size, + std::function f_stat) const PURE; + + /** + * Set the predicates to filter stats for sink. + */ + virtual void setSinkPredicates(const SinkPredicates& sink_predicates) PURE; + // TODO(jmarantz): create a parallel mechanism to instantiate histograms. At // the moment, histograms don't fit the same pattern of counters and gauges // as they are not actually created in the context of a stats allocator. diff --git a/envoy/stats/sink.h b/envoy/stats/sink.h index ff0e607ffaa8c..f618e59d4216c 100644 --- a/envoy/stats/sink.h +++ b/envoy/stats/sink.h @@ -48,6 +48,23 @@ class MetricSnapshot { virtual SystemTime snapshotTime() const PURE; }; +/** + * A class to define predicates to filter stats for flushing to sinks. + */ +class SinkPredicates { +public: + virtual ~SinkPredicates() = default; + + /// @return true if @param counter needs to be flushed to sinks. + virtual bool includeCounter(const Counter& counter) const PURE; + /// @return true if @param gague needs to be flushed to sinks. + virtual bool includeGauge(const Gauge& gauge) const PURE; + /// @return true if @param text_readout needs to be flushed to sinks. + virtual bool includeTextReadout(const TextReadout& text_readout) const PURE; + /// @return true if @param histogram needs to be flushed to sinks. + virtual bool includeHistogram(const Histogram& histogram) const PURE; +}; + /** * A sink for stats. Each sink is responsible for writing stats to a backing store. */ diff --git a/envoy/stats/store.h b/envoy/stats/store.h index 3d456bbe7bec9..cd41e6db17df5 100644 --- a/envoy/stats/store.h +++ b/envoy/stats/store.h @@ -24,6 +24,7 @@ class Instance; namespace Stats { class Sink; +class SinkPredicates; /** * A store for all known counters, gauges, and timers. @@ -65,6 +66,21 @@ class Store : public Scope { virtual void forEachTextReadout(std::function f_size, std::function f_stat) const PURE; + + virtual void forEachHistogram(std::function f_size, + std::function f_stat) const PURE; + /** + * Iterate over all stats that need to be flushed for sink. + */ + virtual void forEachSinkedCounter(std::function f_size, + std::function f_stat) const PURE; + virtual void forEachSinkedGauge(std::function f_size, + std::function f_stat) const PURE; + virtual void forEachSinkedTextReadout(std::function f_size, + std::function f_stat) const PURE; + virtual void + forEachSinkedHistogram(std::function f_size, + std::function f_stat) const PURE; }; using StorePtr = std::unique_ptr; @@ -123,6 +139,11 @@ class StoreRoot : public Store { * method would be asserted. */ virtual void mergeHistograms(PostMergeCb merge_complete_cb) PURE; + + /** + * Set the predicates to filter stats for sink. + */ + virtual void setSinkPredicates(const SinkPredicates& sink_predicates) PURE; }; using StoreRootPtr = std::unique_ptr; diff --git a/source/common/stats/allocator_impl.cc b/source/common/stats/allocator_impl.cc index 9e8a37705e4d2..57f207827b318 100644 --- a/source/common/stats/allocator_impl.cc +++ b/source/common/stats/allocator_impl.cc @@ -3,6 +3,7 @@ #include #include +#include "envoy/stats/sink.h" #include "envoy/stats/stats.h" #include "envoy/stats/symbol_table.h" @@ -144,6 +145,7 @@ class CounterImpl : public StatsSharedImpl { void removeFromSetLockHeld() ABSL_EXCLUSIVE_LOCKS_REQUIRED(alloc_.mutex_) override { const size_t count = alloc_.counters_.erase(statName()); ASSERT(count == 1); + alloc_.sinked_counters_.erase(this); } // Stats::Counter @@ -188,6 +190,7 @@ class GaugeImpl : public StatsSharedImpl { void removeFromSetLockHeld() override ABSL_EXCLUSIVE_LOCKS_REQUIRED(alloc_.mutex_) { const size_t count = alloc_.gauges_.erase(statName()); ASSERT(count == 1); + alloc_.sinked_gauges_.erase(this); } // Stats::Gauge @@ -260,6 +263,7 @@ class TextReadoutImpl : public StatsSharedImpl { void removeFromSetLockHeld() ABSL_EXCLUSIVE_LOCKS_REQUIRED(alloc_.mutex_) override { const size_t count = alloc_.text_readouts_.erase(statName()); ASSERT(count == 1); + alloc_.sinked_text_readouts_.erase(this); } // Stats::TextReadout @@ -289,6 +293,11 @@ CounterSharedPtr AllocatorImpl::makeCounter(StatName name, StatName tag_extracte } auto counter = CounterSharedPtr(makeCounterInternal(name, tag_extracted_name, stat_name_tags)); counters_.insert(counter.get()); + // Add counter to sinked_counters_ if it matches the sink predicate. + if (sink_predicates_ && sink_predicates_->includeCounter(*counter)) { + auto val = sinked_counters_.insert(counter.get()); + ASSERT(val.second); + } return counter; } @@ -305,6 +314,11 @@ GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, StatName tag_extracted_na auto gauge = GaugeSharedPtr(new GaugeImpl(name, *this, tag_extracted_name, stat_name_tags, import_mode)); gauges_.insert(gauge.get()); + // Add gauge to sinked_gauges_ if it matches the sink predicate. + if (sink_predicates_ && sink_predicates_->includeGauge(*gauge)) { + auto val = sinked_gauges_.insert(gauge.get()); + ASSERT(val.second); + } return gauge; } @@ -320,6 +334,11 @@ TextReadoutSharedPtr AllocatorImpl::makeTextReadout(StatName name, StatName tag_ auto text_readout = TextReadoutSharedPtr(new TextReadoutImpl(name, *this, tag_extracted_name, stat_name_tags)); text_readouts_.insert(text_readout.get()); + // Add text_readout to sinked_text_readouts_ if it matches the sink predicate. + if (sink_predicates_ && sink_predicates_->includeTextReadout(*text_readout)) { + auto val = sinked_text_readouts_.insert(text_readout.get()); + ASSERT(val.second); + } return text_readout; } @@ -369,6 +388,71 @@ void AllocatorImpl::forEachTextReadout(std::function f_size, } } +void AllocatorImpl::forEachSinkedCounter(std::function f_size, + std::function f_stat) const { + if (sink_predicates_ != nullptr) { + Thread::LockGuard lock(mutex_); + f_size(sinked_counters_.size()); + for (auto counter : sinked_counters_) { + f_stat(*counter); + } + } else { + forEachCounter(f_size, f_stat); + } +} + +void AllocatorImpl::forEachSinkedGauge(std::function f_size, + std::function f_stat) const { + if (sink_predicates_ != nullptr) { + Thread::LockGuard lock(mutex_); + f_size(sinked_gauges_.size()); + for (auto gauge : sinked_gauges_) { + f_stat(*gauge); + } + } else { + forEachGauge(f_size, f_stat); + } +} + +void AllocatorImpl::forEachSinkedTextReadout( + std::function f_size, + std::function f_stat) const { + if (sink_predicates_ != nullptr) { + Thread::LockGuard lock(mutex_); + f_size(sinked_text_readouts_.size()); + for (auto text_readout : sinked_text_readouts_) { + f_stat(*text_readout); + } + } else { + forEachTextReadout(f_size, f_stat); + } +} + +void AllocatorImpl::setSinkPredicates(const SinkPredicates& sink_predicates) { + Thread::LockGuard lock(mutex_); + ASSERT(sink_predicates_ == nullptr); + sink_predicates_ = &sink_predicates; + + // Add counters to the set of sinked counters. + for (auto& counter : counters_) { + if (sink_predicates_->includeCounter(*counter)) { + sinked_counters_.emplace(counter); + } + } + // Add gauges to the set of sinked gauges. + for (auto& gauge : gauges_) { + if (sink_predicates_->includeGauge(*gauge)) { + sinked_gauges_.insert(gauge); + } + } + // Add text_readouts to the set of sinked text readouts. + for (auto& text_readout : text_readouts_) { + if (sink_predicates_->includeTextReadout(*text_readout)) { + sinked_text_readouts_.insert(text_readout); + } + } +} + void AllocatorImpl::markCounterForDeletion(const CounterSharedPtr& counter) { Thread::LockGuard lock(mutex_); auto iter = counters_.find(counter->statName()); @@ -380,6 +464,7 @@ void AllocatorImpl::markCounterForDeletion(const CounterSharedPtr& counter) { // Duplicates are ASSERTed in ~AllocatorImpl. deleted_counters_.emplace_back(*iter); counters_.erase(iter); + sinked_counters_.erase(counter.get()); } void AllocatorImpl::markGaugeForDeletion(const GaugeSharedPtr& gauge) { @@ -393,6 +478,7 @@ void AllocatorImpl::markGaugeForDeletion(const GaugeSharedPtr& gauge) { // Duplicates are ASSERTed in ~AllocatorImpl. deleted_gauges_.emplace_back(*iter); gauges_.erase(iter); + sinked_gauges_.erase(gauge.get()); } void AllocatorImpl::markTextReadoutForDeletion(const TextReadoutSharedPtr& text_readout) { @@ -406,6 +492,7 @@ void AllocatorImpl::markTextReadoutForDeletion(const TextReadoutSharedPtr& text_ // Duplicates are ASSERTed in ~AllocatorImpl. deleted_text_readouts_.emplace_back(*iter); text_readouts_.erase(iter); + sinked_text_readouts_.erase(text_readout.get()); } } // namespace Stats diff --git a/source/common/stats/allocator_impl.h b/source/common/stats/allocator_impl.h index 806e7dc8612fd..b53b308e5f525 100644 --- a/source/common/stats/allocator_impl.h +++ b/source/common/stats/allocator_impl.h @@ -15,6 +15,8 @@ namespace Envoy { namespace Stats { +class SinkPredicates; + class AllocatorImpl : public Allocator { public: static const char DecrementToZeroSyncPoint[]; @@ -42,6 +44,14 @@ class AllocatorImpl : public Allocator { void forEachTextReadout(std::function, std::function) const override; + void forEachSinkedCounter(std::function f_size, + std::function f_stat) const override; + void forEachSinkedGauge(std::function f_size, + std::function f_stat) const override; + void forEachSinkedTextReadout(std::function f_size, + std::function f_stat) const override; + + void setSinkPredicates(SinkPredicates const& sink_predicates) override; #ifndef ENVOY_CONFIG_COVERAGE void debugPrint(); #endif @@ -93,6 +103,15 @@ class AllocatorImpl : public Allocator { std::vector deleted_gauges_ ABSL_GUARDED_BY(mutex_); std::vector deleted_text_readouts_ ABSL_GUARDED_BY(mutex_); + template + using StatPointerSet = absl::flat_hash_set>; + // Stat pointers that participate in the flush to sink process. + StatPointerSet sinked_counters_ ABSL_GUARDED_BY(mutex_); + StatPointerSet sinked_gauges_ ABSL_GUARDED_BY(mutex_); + StatPointerSet sinked_text_readouts_ ABSL_GUARDED_BY(mutex_); + + // Predicates used to filter stats to be flushed. + const SinkPredicates* sink_predicates_ = nullptr; SymbolTable& symbol_table_; Thread::ThreadSynchronizer sync_; diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index ebff944da7eff..2c3b65ce9e7b2 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -103,7 +103,9 @@ template class IsolatedStatsCache { void forEachStat(std::function f_size, std::function f_stat) const { - f_size(stats_.size()); + if (f_size != nullptr) { + f_size(stats_.size()); + } for (auto const& stat : stats_) { f_stat(*stat.second); } @@ -229,6 +231,33 @@ class IsolatedStoreImpl : public StoreImpl { text_readouts_.forEachStat(f_size, f_stat); } + void forEachHistogram(std::function f_size, + std::function f_stat) const override { + UNREFERENCED_PARAMETER(f_size); + UNREFERENCED_PARAMETER(f_stat); + } + + void forEachSinkedCounter(std::function f_size, + std::function f_stat) const override { + forEachCounter(f_size, f_stat); + } + + void forEachSinkedGauge(std::function f_size, + std::function f_stat) const override { + forEachGauge(f_size, f_stat); + } + + void forEachSinkedTextReadout(std::function f_size, + std::function f_stat) const override { + forEachTextReadout(f_size, f_stat); + } + + void forEachSinkedHistogram(std::function f_size, + std::function f_stat) const override { + (void)f_size; + (void)f_stat; + } + private: IsolatedStoreImpl(std::unique_ptr&& symbol_table); diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 97960182096c0..d381ee2a328d8 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -90,6 +90,7 @@ void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) { for (uint32_t i = first_histogram_index; i < deleted_histograms_.size(); ++i) { uint32_t erased = histogram_set_.erase(deleted_histograms_[i].get()); ASSERT(erased == 1); + sinked_histograms_.erase(deleted_histograms_[i].get()); } } } @@ -221,6 +222,7 @@ void ThreadLocalStoreImpl::shutdownThreading() { histogram->setShuttingDown(true); } histogram_set_.clear(); + sinked_histograms_.clear(); } void ThreadLocalStoreImpl::mergeHistograms(PostMergeCb merge_complete_cb) { @@ -243,9 +245,7 @@ void ThreadLocalStoreImpl::mergeHistograms(PostMergeCb merge_complete_cb) { void ThreadLocalStoreImpl::mergeInternal(PostMergeCb merge_complete_cb) { if (!shutting_down_) { - for (const ParentHistogramSharedPtr& histogram : histograms()) { - histogram->merge(); - } + forEachHistogram(nullptr, [](ParentHistogram& histogram) { histogram.merge(); }); merge_complete_cb(); merge_in_progress_ = false; } @@ -678,6 +678,9 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( *buckets, parent_.next_histogram_id_++); if (!parent_.shutting_down_) { parent_.histogram_set_.insert(stat.get()); + if (parent_.sink_predicates_ && parent_.sink_predicates_->includeHistogram(*stat)) { + parent_.sinked_histograms_.insert(stat.get()); + } } } } @@ -867,6 +870,7 @@ bool ThreadLocalStoreImpl::decHistogramRefCount(ParentHistogramImpl& hist, if (!shutting_down_) { const size_t count = histogram_set_.erase(hist.statName()); ASSERT(shutting_down_ || count == 1); + sinked_histograms_.erase(&hist); } return true; } @@ -956,22 +960,67 @@ bool ParentHistogramImpl::usedLockHeld() const { void ThreadLocalStoreImpl::forEachCounter(std::function f_size, std::function f_stat) const { - Thread::LockGuard lock(lock_); alloc_.forEachCounter(f_size, f_stat); } void ThreadLocalStoreImpl::forEachGauge(std::function f_size, std::function f_stat) const { - Thread::LockGuard lock(lock_); alloc_.forEachGauge(f_size, f_stat); } void ThreadLocalStoreImpl::forEachTextReadout( std::function f_size, std::function f_stat) const { - Thread::LockGuard lock(lock_); alloc_.forEachTextReadout(f_size, f_stat); } +void ThreadLocalStoreImpl::forEachHistogram( + std::function f_size, std::function f_stat) const { + Thread::LockGuard lock(hist_mutex_); + if (f_size != nullptr) { + f_size(histogram_set_.size()); + } + for (auto histogram : histogram_set_) { + f_stat(*histogram); + } +} + +void ThreadLocalStoreImpl::forEachSinkedCounter(std::function f_size, + std::function f_stat) const { + alloc_.forEachSinkedCounter(f_size, f_stat); +} + +void ThreadLocalStoreImpl::forEachSinkedGauge(std::function f_size, + std::function f_stat) const { + alloc_.forEachSinkedGauge(f_size, f_stat); +} + +void ThreadLocalStoreImpl::forEachSinkedTextReadout( + std::function f_size, + std::function f_stat) const { + alloc_.forEachSinkedTextReadout(f_size, f_stat); +} + +void ThreadLocalStoreImpl::forEachSinkedHistogram( + std::function f_size, std::function f_stat) const { + if (sink_predicates_ != nullptr) { + Thread::LockGuard lock(hist_mutex_); + + if (f_size != nullptr) { + f_size(sinked_histograms_.size()); + } + for (auto histogram : sinked_histograms_) { + f_stat(*histogram); + } + } else { + forEachHistogram(f_size, f_stat); + } +} + +void ThreadLocalStoreImpl::setSinkPredicates(const SinkPredicates& sink_predicates) { + sink_predicates_ = &sink_predicates; + alloc_.setSinkPredicates(sink_predicates); +} + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 742e1fc3c04d6..1c74cc95bb2ac 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -252,6 +252,8 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void forEachTextReadout(std::function f_size, std::function f_stat) const override; + void forEachHistogram(std::function f_size, + std::function f_stat) const override; // Stats::StoreRoot void addSink(Sink& sink) override { timer_sinks_.push_back(sink); } @@ -267,6 +269,17 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo Histogram& tlsHistogram(ParentHistogramImpl& parent, uint64_t id); + void forEachSinkedCounter(std::function f_size, + std::function f_stat) const override; + void forEachSinkedGauge(std::function f_size, + std::function f_stat) const override; + void forEachSinkedTextReadout(std::function f_size, + std::function f_stat) const override; + void forEachSinkedHistogram(std::function f_size, + std::function f_stat) const override; + + void setSinkPredicates(const SinkPredicates& sink_predicates) override; + /** * @return a thread synchronizer object used for controlling thread behavior in tests. */ @@ -500,6 +513,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo StatNameHashSet* tls_rejected_stats); TlsCache& tlsCache() { return **tls_cache_; } + const SinkPredicates* sink_predicates_ = nullptr; Allocator& alloc_; Event::Dispatcher* main_thread_dispatcher_{}; using TlsCacheSlot = ThreadLocal::TypedSlotPtr; @@ -530,6 +544,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo mutable Thread::MutexBasicLockable hist_mutex_; StatSet histogram_set_ ABSL_GUARDED_BY(hist_mutex_); + StatSet sinked_histograms_ ABSL_GUARDED_BY(hist_mutex_); // Retain storage for deleted stats; these are no longer in maps because the // matcher-pattern was established after they were created. Since the stats diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index b86959385c3c5..a5294685510e2 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -125,6 +125,9 @@ class ValidationInstance final : Logger::Loggable, void setDefaultTracingConfig(const envoy::config::trace::v3::Tracing& tracing_config) override { http_context_.setDefaultTracingConfig(tracing_config); } + void setSinkPredicates(std::unique_ptr sink_predicates) override { + (void)sink_predicates; + } // Server::ListenerComponentFactory LdsApiPtr createLdsApi(const envoy::config::core::v3::ConfigSource& lds_config, diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 7efa5e22fda54..9e1f2f0c11e31 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -56,10 +56,17 @@ class StatsConfigImpl : public StatsConfig { void addSink(Stats::SinkPtr sink) { sinks_.emplace_back(std::move(sink)); } + const Stats::SinkPredicates* sinkPredicates() const { return sink_predicates_.get(); } + + void setSinkPredicates(std::unique_ptr sink_predicates) { + sink_predicates_ = std::move(sink_predicates); + } + private: std::list sinks_; std::chrono::milliseconds flush_interval_; bool flush_on_admin_{false}; + std::unique_ptr sink_predicates_; }; /** diff --git a/source/server/hot_restarting_parent.cc b/source/server/hot_restarting_parent.cc index 14c012b70cbca..fab51c4fc7b53 100644 --- a/source/server/hot_restarting_parent.cc +++ b/source/server/hot_restarting_parent.cc @@ -128,26 +128,26 @@ HotRestartingParent::Internal::getListenSocketsForChild(const HotRestartMessage: // magnitude of memory usage that they are meant to avoid, since this map holds full-string // names. The problem can be solved by splitting the export up over many chunks. void HotRestartingParent::Internal::exportStatsToChild(HotRestartMessage::Reply::Stats* stats) { - for (const auto& gauge : server_->stats().gauges()) { - if (gauge->used()) { - const std::string name = gauge->name(); - (*stats->mutable_gauges())[name] = gauge->value(); - recordDynamics(stats, name, gauge->statName()); + server_->stats().forEachSinkedGauge(nullptr, [this, stats](Stats::Gauge& gauge) mutable { + if (gauge.used()) { + const std::string name = gauge.name(); + (*stats->mutable_gauges())[name] = gauge.value(); + recordDynamics(stats, name, gauge.statName()); } - } + }); - for (const auto& counter : server_->stats().counters()) { - if (counter->used()) { + server_->stats().forEachSinkedCounter(nullptr, [this, stats](Stats::Counter& counter) mutable { + if (counter.used()) { // The hot restart parent is expected to have stopped its normal stat exporting (and so // latching) by the time it begins exporting to the hot restart child. - uint64_t latched_value = counter->latch(); + uint64_t latched_value = counter.latch(); if (latched_value > 0) { - const std::string name = counter->name(); + const std::string name = counter.name(); (*stats->mutable_counter_deltas())[name] = latched_value; - recordDynamics(stats, name, counter->statName()); + recordDynamics(stats, name, counter.statName()); } } - } + }); stats->set_memory_allocated(Memory::Stats::totalCurrentlyAllocated()); stats->set_num_connections(server_->listenerManager().numConnections()); } diff --git a/source/server/server.cc b/source/server/server.cc index 6af174a4bd4e9..6af7eeeb94425 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -20,6 +20,7 @@ #include "envoy/server/bootstrap_extension_config.h" #include "envoy/server/instance.h" #include "envoy/server/options.h" +#include "envoy/stats/histogram.h" #include "envoy/stats/stats.h" #include "envoy/upstream/cluster_manager.h" @@ -168,7 +169,7 @@ void InstanceImpl::failHealthcheck(bool fail) { } MetricSnapshotImpl::MetricSnapshotImpl(Stats::Store& store, TimeSource& time_source) { - store.forEachCounter( + store.forEachSinkedCounter( [this](std::size_t size) mutable { snapped_counters_.reserve(size); counters_.reserve(size); @@ -178,7 +179,7 @@ MetricSnapshotImpl::MetricSnapshotImpl(Stats::Store& store, TimeSource& time_sou counters_.push_back({counter.latch(), counter}); }); - store.forEachGauge( + store.forEachSinkedGauge( [this](std::size_t size) mutable { snapped_gauges_.reserve(size); gauges_.reserve(size); @@ -189,13 +190,17 @@ MetricSnapshotImpl::MetricSnapshotImpl(Stats::Store& store, TimeSource& time_sou gauges_.push_back(gauge); }); - snapped_histograms_ = store.histograms(); - histograms_.reserve(snapped_histograms_.size()); - for (const auto& histogram : snapped_histograms_) { - histograms_.push_back(*histogram); - } + store.forEachSinkedHistogram( + [this](std::size_t size) mutable { + snapped_histograms_.reserve(size); + histograms_.reserve(size); + }, + [this](Stats::ParentHistogram& histogram) mutable { + snapped_histograms_.push_back(Stats::ParentHistogramSharedPtr(&histogram)); + histograms_.push_back(histogram); + }); - store.forEachTextReadout( + store.forEachSinkedTextReadout( [this](std::size_t size) mutable { snapped_text_readouts_.reserve(size); text_readouts_.reserve(size); diff --git a/source/server/server.h b/source/server/server.h index 19ac1b5ce169a..179dd9a3f9106 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -293,6 +293,11 @@ class InstanceImpl final : Logger::Loggable, Quic::QuicStatNames& quicStatNames() { return quic_stat_names_; } + void setSinkPredicates(std::unique_ptr sink_predicates) override { + sink_predicates_ = std::move(sink_predicates); + stats_store_.setSinkPredicates(*sink_predicates_); + } + // ServerLifecycleNotifier ServerLifecycleNotifier::HandlePtr registerCallback(Stage stage, StageCallback callback) override; ServerLifecycleNotifier::HandlePtr @@ -390,6 +395,7 @@ class InstanceImpl final : Logger::Loggable, Quic::QuicStatNames quic_stat_names_; ServerFactoryContextImpl server_contexts_; absl::optional enable_reuse_port_default_; + std::unique_ptr sink_predicates_; bool stats_flush_in_progress_ : 1; diff --git a/test/common/stats/allocator_impl_test.cc b/test/common/stats/allocator_impl_test.cc index cc06acedaef1e..018ba3d169469 100644 --- a/test/common/stats/allocator_impl_test.cc +++ b/test/common/stats/allocator_impl_test.cc @@ -1,6 +1,8 @@ #include #include +#include "envoy/stats/sink.h" + #include "source/common/stats/allocator_impl.h" #include "test/test_common/logging.h" @@ -41,6 +43,31 @@ class AllocatorImplTest : public testing::Test { bool are_stats_marked_for_deletion_ = false; }; +class SinkPredicatesImpl : public SinkPredicates { +public: + ~SinkPredicatesImpl() override = default; + StatNameHashSet& sinkedStatNames() { return sinked_stat_names_; } + + bool includeCounter(const Counter& counter) const override { + return sinked_stat_names_.find(counter.statName()) != sinked_stat_names_.end(); + } + + bool includeGauge(const Gauge& gauge) const override { + return sinked_stat_names_.find(gauge.statName()) != sinked_stat_names_.end(); + } + + bool includeTextReadout(const TextReadout& text_readout) const override { + return sinked_stat_names_.find(text_readout.statName()) != sinked_stat_names_.end(); + } + + bool includeHistogram(const Histogram& histogram) const override { + return sinked_stat_names_.find(histogram.statName()) != sinked_stat_names_.end(); + } + +private: + StatNameHashSet sinked_stat_names_; +}; + // Allocate 2 counters of the same name, and you'll get the same object. TEST_F(AllocatorImplTest, CountersWithSameName) { StatName counter_name = makeStat("counter.name"); @@ -149,7 +176,7 @@ TEST_F(AllocatorImplTest, ForEachCounter) { size_t num_iterations = 0; alloc_.forEachCounter([&num_counters](std::size_t size) { num_counters = size; }, [&num_iterations, &stat_names](Stats::Counter& counter) { - EXPECT_EQ(stat_names.count(counter.statName()), 1); + EXPECT_NE(stat_names.find(counter.statName()), stat_names.end()); ++num_iterations; }); EXPECT_EQ(num_counters, 11); @@ -202,7 +229,7 @@ TEST_F(AllocatorImplTest, ForEachGauge) { size_t num_iterations = 0; alloc_.forEachGauge([&num_gauges](std::size_t size) { num_gauges = size; }, [&num_iterations, &stat_names](Stats::Gauge& gauge) { - EXPECT_EQ(stat_names.count(gauge.statName()), 1); + EXPECT_NE(stat_names.find(gauge.statName()), stat_names.end()); ++num_iterations; }); EXPECT_EQ(num_gauges, 11); @@ -255,7 +282,7 @@ TEST_F(AllocatorImplTest, ForEachTextReadout) { size_t num_iterations = 0; alloc_.forEachTextReadout([&num_text_readouts](std::size_t size) { num_text_readouts = size; }, [&num_iterations, &stat_names](Stats::TextReadout& text_readout) { - EXPECT_EQ(stat_names.count(text_readout.statName()), 1); + EXPECT_NE(stat_names.find(text_readout.statName()), stat_names.end()); ++num_iterations; }); EXPECT_EQ(num_text_readouts, 11); @@ -309,7 +336,7 @@ TEST_F(AllocatorImplTest, ForEachWithNullSizeLambda) { } size_t num_iterations = 0; alloc_.forEachCounter(nullptr, [&num_iterations](Stats::Counter& counter) { - (void)counter; + UNREFERENCED_PARAMETER(counter); ++num_iterations; }); EXPECT_EQ(num_iterations, num_stats); @@ -321,7 +348,7 @@ TEST_F(AllocatorImplTest, ForEachWithNullSizeLambda) { } num_iterations = 0; alloc_.forEachGauge(nullptr, [&num_iterations](Stats::Gauge& gauge) { - (void)gauge; + UNREFERENCED_PARAMETER(gauge); ++num_iterations; }); EXPECT_EQ(num_iterations, num_stats); @@ -333,7 +360,7 @@ TEST_F(AllocatorImplTest, ForEachWithNullSizeLambda) { } num_iterations = 0; alloc_.forEachTextReadout(nullptr, [&num_iterations](Stats::TextReadout& text_readout) { - (void)text_readout; + UNREFERENCED_PARAMETER(text_readout); ++num_iterations; }); EXPECT_EQ(num_iterations, num_stats); @@ -410,6 +437,142 @@ TEST_F(AllocatorImplTest, AskForDeletedStat) { EXPECT_EQ(rejected_text_readout.value(), "deleted value"); } +TEST_F(AllocatorImplTest, ForEachSinkedCounter) { + + SinkPredicatesImpl sink_predicates; + std::vector sinked_counters; + std::vector unsinked_counters; + + alloc_.setSinkPredicates(sink_predicates); + + const size_t num_stats = 11; + + for (size_t idx = 0; idx < num_stats; ++idx) { + auto stat_name = makeStat(absl::StrCat("counter.", idx)); + // sink every 3rd stat + if ((idx + 1) % 3 == 0) { + sink_predicates.sinkedStatNames().insert(stat_name); + sinked_counters.emplace_back(alloc_.makeCounter(stat_name, StatName(), {})); + } else { + unsinked_counters.emplace_back(alloc_.makeCounter(stat_name, StatName(), {})); + } + } + + EXPECT_EQ(sinked_counters.size(), 3); + EXPECT_EQ(unsinked_counters.size(), 8); + + size_t num_sinked_counters = 0; + size_t num_iterations = 0; + alloc_.forEachSinkedCounter( + [&num_sinked_counters](std::size_t size) { num_sinked_counters = size; }, + [&num_iterations, &sink_predicates](Stats::Counter& counter) { + EXPECT_NE(sink_predicates.sinkedStatNames().find(counter.statName()), + sink_predicates.sinkedStatNames().end()); + ++num_iterations; + }); + EXPECT_EQ(num_sinked_counters, 3); + EXPECT_EQ(num_iterations, 3); + + // Erase all sinked stats. + sinked_counters.clear(); + num_iterations = 0; + alloc_.forEachSinkedCounter( + [&num_sinked_counters](std::size_t size) { num_sinked_counters = size; }, + [&num_iterations](Stats::Counter&) { ++num_iterations; }); + EXPECT_EQ(num_sinked_counters, 0); + EXPECT_EQ(num_iterations, 0); +} + +TEST_F(AllocatorImplTest, ForEachSinkedGauge) { + + SinkPredicatesImpl sink_predicates; + std::vector sinked_gauges; + std::vector unsinked_gauges; + + alloc_.setSinkPredicates(sink_predicates); + const size_t num_stats = 11; + + for (size_t idx = 0; idx < num_stats; ++idx) { + auto stat_name = makeStat(absl::StrCat("gauge.", idx)); + // sink every 5th stat + if ((idx + 1) % 5 == 0) { + sink_predicates.sinkedStatNames().insert(stat_name); + sinked_gauges.emplace_back( + alloc_.makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate)); + } else { + unsinked_gauges.emplace_back( + alloc_.makeGauge(stat_name, StatName(), {}, Gauge::ImportMode::Accumulate)); + } + } + + EXPECT_EQ(sinked_gauges.size(), 2); + EXPECT_EQ(unsinked_gauges.size(), 9); + + size_t num_sinked_gauges = 0; + size_t num_iterations = 0; + alloc_.forEachSinkedGauge([&num_sinked_gauges](std::size_t size) { num_sinked_gauges = size; }, + [&num_iterations, &sink_predicates](Stats::Gauge& gauge) { + EXPECT_NE(sink_predicates.sinkedStatNames().find(gauge.statName()), + sink_predicates.sinkedStatNames().end()); + ++num_iterations; + }); + EXPECT_EQ(num_sinked_gauges, 2); + EXPECT_EQ(num_iterations, 2); + + // Erase all sinked stats. + sinked_gauges.clear(); + num_iterations = 0; + alloc_.forEachSinkedGauge([&num_sinked_gauges](std::size_t size) { num_sinked_gauges = size; }, + [&num_iterations](Stats::Gauge&) { ++num_iterations; }); + EXPECT_EQ(num_sinked_gauges, 0); + EXPECT_EQ(num_iterations, 0); +} + +TEST_F(AllocatorImplTest, ForEachSinkedTextReadout) { + + SinkPredicatesImpl sink_predicates; + std::vector sinked_text_readouts; + std::vector unsinked_text_readouts; + + alloc_.setSinkPredicates(sink_predicates); + const size_t num_stats = 11; + + for (size_t idx = 0; idx < num_stats; ++idx) { + auto stat_name = makeStat(absl::StrCat("text_readout.", idx)); + // sink every 2nd stat + if ((idx + 1) % 2 == 0) { + sink_predicates.sinkedStatNames().insert(stat_name); + sinked_text_readouts.emplace_back(alloc_.makeTextReadout(stat_name, StatName(), {})); + } else { + unsinked_text_readouts.emplace_back(alloc_.makeTextReadout(stat_name, StatName(), {})); + } + } + + EXPECT_EQ(sinked_text_readouts.size(), 5); + EXPECT_EQ(unsinked_text_readouts.size(), 6); + + size_t num_sinked_text_readouts = 0; + size_t num_iterations = 0; + alloc_.forEachSinkedTextReadout( + [&num_sinked_text_readouts](std::size_t size) { num_sinked_text_readouts = size; }, + [&num_iterations, &sink_predicates](Stats::TextReadout& text_readout) { + EXPECT_NE(sink_predicates.sinkedStatNames().find(text_readout.statName()), + sink_predicates.sinkedStatNames().end()); + ++num_iterations; + }); + EXPECT_EQ(num_sinked_text_readouts, 5); + EXPECT_EQ(num_iterations, 5); + + // Erase all sinked stats. + sinked_text_readouts.clear(); + num_iterations = 0; + alloc_.forEachSinkedTextReadout( + [&num_sinked_text_readouts](std::size_t size) { num_sinked_text_readouts = size; }, + [&num_iterations](Stats::TextReadout&) { ++num_iterations; }); + EXPECT_EQ(num_sinked_text_readouts, 0); + EXPECT_EQ(num_iterations, 0); +} + } // namespace } // namespace Stats } // namespace Envoy diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 0847d9605df29..9b95e15200e36 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -1801,7 +1801,7 @@ TEST_F(HistogramThreadTest, ScopeOverlap) { 2 * NumThreads, ") "))); // Now clear everything, and synchronize the system by calling mergeHistograms(). - // THere should be no more ParentHistograms or TlsHistograms. + // There should be no more ParentHistograms or TlsHistograms. scope2.reset(); histograms.clear(); mergeHistograms(); diff --git a/test/integration/server.h b/test/integration/server.h index 880b899d2f3a9..97255032d3628 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -9,6 +9,7 @@ #include "envoy/config/listener/v3/listener.pb.h" #include "envoy/server/options.h" #include "envoy/server/process_context.h" +#include "envoy/stats/histogram.h" #include "envoy/stats/stats.h" #include "source/common/common/assert.h" @@ -296,6 +297,35 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); store_.forEachTextReadout(f_size, f_stat); } + void forEachHistogram(std::function f_size, + std::function f_stat) const override { + Thread::LockGuard lock(lock_); + store_.forEachHistogram(f_size, f_stat); + } + void forEachSinkedCounter(std::function f_size, + std::function f_stat) const override { + Thread::LockGuard lock(lock_); + store_.forEachSinkedCounter(f_size, f_stat); + } + void forEachSinkedGauge(std::function f_size, + std::function f_stat) const override { + Thread::LockGuard lock(lock_); + store_.forEachSinkedGauge(f_size, f_stat); + } + void forEachSinkedTextReadout(std::function f_size, + std::function f_stat) const override { + Thread::LockGuard lock(lock_); + store_.forEachSinkedTextReadout(f_size, f_stat); + } + void forEachSinkedHistogram(std::function f_size, + std::function f_stat) const override { + Thread::LockGuard lock(lock_); + store_.forEachSinkedHistogram(f_size, f_stat); + } + void setSinkPredicates(const SinkPredicates& sink_predicates) override { + UNREFERENCED_PARAMETER(sink_predicates); + } + Counter& counterFromString(const std::string& name) override { Thread::LockGuard lock(lock_); return store_.counterFromString(name); diff --git a/test/mocks/server/instance.h b/test/mocks/server/instance.h index de4f51099823c..b759408083aac 100644 --- a/test/mocks/server/instance.h +++ b/test/mocks/server/instance.h @@ -87,6 +87,7 @@ class MockInstance : public Instance { MOCK_METHOD(Configuration::ServerFactoryContext&, serverFactoryContext, ()); MOCK_METHOD(Configuration::TransportSocketFactoryContext&, transportSocketFactoryContext, ()); MOCK_METHOD(bool, enableReusePortDefault, ()); + MOCK_METHOD(void, setSinkPredicates, (std::unique_ptr)); void setDefaultTracingConfig(const envoy::config::trace::v3::Tracing& tracing_config) override { http_context_.setDefaultTracingConfig(tracing_config); @@ -141,6 +142,7 @@ class MockStatsConfig : public virtual StatsConfig { MOCK_METHOD(const std::list&, sinks, (), (const)); MOCK_METHOD(std::chrono::milliseconds, flushInterval, (), (const)); MOCK_METHOD(bool, flushOnAdmin, (), (const)); + MOCK_METHOD(const Stats::SinkPredicates*, sinkPredicates, (), (const)); }; class MockServerFactoryContext : public virtual ServerFactoryContext { diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 3043a2c35fbb4..adc57f1f40ec4 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -293,6 +293,9 @@ class MockStore : public TestUtil::TestStore { MOCK_METHOD(void, forEachTextReadout, (std::function, std::function), (const)); + MOCK_METHOD(void, forEachSinkedHistogram, + (std::function, std::function), + (const)); MOCK_METHOD(CounterOptConstRef, findCounter, (StatName), (const)); MOCK_METHOD(GaugeOptConstRef, findGauge, (StatName), (const)); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 8e6acff4fe85d..a5c78470b6dc2 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -92,6 +92,16 @@ TEST(ServerInstanceUtil, flushHelper) { Stats::ParentHistogramSharedPtr parent_histogram(new Stats::MockParentHistogram()); std::vector parent_histograms = {parent_histogram}; ON_CALL(mock_store, histograms).WillByDefault(Return(parent_histograms)); + ON_CALL(mock_store, forEachSinkedHistogram) + .WillByDefault([&](std::function f_size, + std::function f_stat) { + if (f_size != nullptr) { + f_size(parent_histograms.size()); + } + for (auto& histogram : parent_histograms) { + f_stat(*histogram); + } + }); EXPECT_CALL(*sink, flush(_)).WillOnce(Invoke([](Stats::MetricSnapshot& snapshot) { EXPECT_TRUE(snapshot.counters().empty()); EXPECT_TRUE(snapshot.gauges().empty()); diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 46e6fb03ec87d..5964402098022 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -1101,6 +1101,7 @@ siginfo signalstack siloed sim +sinked sizeof smatch snapshotted From e0f6a99cb809480f1c6f78ea8012a733f579122e Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Tue, 9 Nov 2021 01:24:46 +0000 Subject: [PATCH 02/12] Add tests for histograms. Signed-off-by: Pradeep Rao --- envoy/stats/allocator.h | 2 +- envoy/stats/sink.h | 8 +-- envoy/stats/store.h | 2 +- source/common/stats/allocator_impl.cc | 18 +++--- source/common/stats/allocator_impl.h | 5 +- source/common/stats/thread_local_store.cc | 13 +++-- source/common/stats/thread_local_store.h | 4 +- source/server/configuration_impl.h | 7 --- source/server/server.h | 4 +- test/common/stats/thread_local_store_test.cc | 58 +++++++++++++++++++ test/integration/server.h | 2 +- .../server_stats_flush_benchmark_test.cc | 44 +++++++++++++- 12 files changed, 131 insertions(+), 36 deletions(-) diff --git a/envoy/stats/allocator.h b/envoy/stats/allocator.h index 1eb387b59b6ed..e96e2b83865bc 100644 --- a/envoy/stats/allocator.h +++ b/envoy/stats/allocator.h @@ -97,7 +97,7 @@ class Allocator { /** * Set the predicates to filter stats for sink. */ - virtual void setSinkPredicates(const SinkPredicates& sink_predicates) PURE; + virtual void setSinkPredicates(SinkPredicates& sink_predicates) PURE; // TODO(jmarantz): create a parallel mechanism to instantiate histograms. At // the moment, histograms don't fit the same pattern of counters and gauges diff --git a/envoy/stats/sink.h b/envoy/stats/sink.h index f618e59d4216c..fb383b3a3c629 100644 --- a/envoy/stats/sink.h +++ b/envoy/stats/sink.h @@ -56,13 +56,13 @@ class SinkPredicates { virtual ~SinkPredicates() = default; /// @return true if @param counter needs to be flushed to sinks. - virtual bool includeCounter(const Counter& counter) const PURE; + virtual bool includeCounter(const Counter& counter) PURE; /// @return true if @param gague needs to be flushed to sinks. - virtual bool includeGauge(const Gauge& gauge) const PURE; + virtual bool includeGauge(const Gauge& gauge) PURE; /// @return true if @param text_readout needs to be flushed to sinks. - virtual bool includeTextReadout(const TextReadout& text_readout) const PURE; + virtual bool includeTextReadout(const TextReadout& text_readout) PURE; /// @return true if @param histogram needs to be flushed to sinks. - virtual bool includeHistogram(const Histogram& histogram) const PURE; + virtual bool includeHistogram(const Histogram& histogram) PURE; }; /** diff --git a/envoy/stats/store.h b/envoy/stats/store.h index cd41e6db17df5..a790be413b6f0 100644 --- a/envoy/stats/store.h +++ b/envoy/stats/store.h @@ -143,7 +143,7 @@ class StoreRoot : public Store { /** * Set the predicates to filter stats for sink. */ - virtual void setSinkPredicates(const SinkPredicates& sink_predicates) PURE; + virtual void setSinkPredicates(std::unique_ptr sink_predicates) PURE; }; using StoreRootPtr = std::unique_ptr; diff --git a/source/common/stats/allocator_impl.cc b/source/common/stats/allocator_impl.cc index 57f207827b318..e0b80b4511f21 100644 --- a/source/common/stats/allocator_impl.cc +++ b/source/common/stats/allocator_impl.cc @@ -294,7 +294,7 @@ CounterSharedPtr AllocatorImpl::makeCounter(StatName name, StatName tag_extracte auto counter = CounterSharedPtr(makeCounterInternal(name, tag_extracted_name, stat_name_tags)); counters_.insert(counter.get()); // Add counter to sinked_counters_ if it matches the sink predicate. - if (sink_predicates_ && sink_predicates_->includeCounter(*counter)) { + if (sink_predicates_.has_value() && sink_predicates_->includeCounter(*counter)) { auto val = sinked_counters_.insert(counter.get()); ASSERT(val.second); } @@ -315,7 +315,7 @@ GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, StatName tag_extracted_na GaugeSharedPtr(new GaugeImpl(name, *this, tag_extracted_name, stat_name_tags, import_mode)); gauges_.insert(gauge.get()); // Add gauge to sinked_gauges_ if it matches the sink predicate. - if (sink_predicates_ && sink_predicates_->includeGauge(*gauge)) { + if (sink_predicates_.has_value() && sink_predicates_->includeGauge(*gauge)) { auto val = sinked_gauges_.insert(gauge.get()); ASSERT(val.second); } @@ -335,7 +335,7 @@ TextReadoutSharedPtr AllocatorImpl::makeTextReadout(StatName name, StatName tag_ TextReadoutSharedPtr(new TextReadoutImpl(name, *this, tag_extracted_name, stat_name_tags)); text_readouts_.insert(text_readout.get()); // Add text_readout to sinked_text_readouts_ if it matches the sink predicate. - if (sink_predicates_ && sink_predicates_->includeTextReadout(*text_readout)) { + if (sink_predicates_.has_value() && sink_predicates_->includeTextReadout(*text_readout)) { auto val = sinked_text_readouts_.insert(text_readout.get()); ASSERT(val.second); } @@ -390,7 +390,7 @@ void AllocatorImpl::forEachTextReadout(std::function f_size, void AllocatorImpl::forEachSinkedCounter(std::function f_size, std::function f_stat) const { - if (sink_predicates_ != nullptr) { + if (sink_predicates_.has_value()) { Thread::LockGuard lock(mutex_); f_size(sinked_counters_.size()); for (auto counter : sinked_counters_) { @@ -403,7 +403,7 @@ void AllocatorImpl::forEachSinkedCounter(std::function f_size void AllocatorImpl::forEachSinkedGauge(std::function f_size, std::function f_stat) const { - if (sink_predicates_ != nullptr) { + if (sink_predicates_.has_value()) { Thread::LockGuard lock(mutex_); f_size(sinked_gauges_.size()); for (auto gauge : sinked_gauges_) { @@ -417,7 +417,7 @@ void AllocatorImpl::forEachSinkedGauge(std::function f_size, void AllocatorImpl::forEachSinkedTextReadout( std::function f_size, std::function f_stat) const { - if (sink_predicates_ != nullptr) { + if (sink_predicates_.has_value()) { Thread::LockGuard lock(mutex_); f_size(sinked_text_readouts_.size()); for (auto text_readout : sinked_text_readouts_) { @@ -428,10 +428,10 @@ void AllocatorImpl::forEachSinkedTextReadout( } } -void AllocatorImpl::setSinkPredicates(const SinkPredicates& sink_predicates) { +void AllocatorImpl::setSinkPredicates(SinkPredicates& sink_predicates) { Thread::LockGuard lock(mutex_); - ASSERT(sink_predicates_ == nullptr); - sink_predicates_ = &sink_predicates; + ASSERT(!sink_predicates_.has_value()); + sink_predicates_.emplace(sink_predicates); // Add counters to the set of sinked counters. for (auto& counter : counters_) { diff --git a/source/common/stats/allocator_impl.h b/source/common/stats/allocator_impl.h index b53b308e5f525..6ec75ac747772 100644 --- a/source/common/stats/allocator_impl.h +++ b/source/common/stats/allocator_impl.h @@ -2,6 +2,7 @@ #include +#include "envoy/common/optref.h" #include "envoy/stats/allocator.h" #include "envoy/stats/stats.h" #include "envoy/stats/symbol_table.h" @@ -51,7 +52,7 @@ class AllocatorImpl : public Allocator { void forEachSinkedTextReadout(std::function f_size, std::function f_stat) const override; - void setSinkPredicates(SinkPredicates const& sink_predicates) override; + void setSinkPredicates(SinkPredicates& sink_predicates) override; #ifndef ENVOY_CONFIG_COVERAGE void debugPrint(); #endif @@ -111,7 +112,7 @@ class AllocatorImpl : public Allocator { StatPointerSet sinked_text_readouts_ ABSL_GUARDED_BY(mutex_); // Predicates used to filter stats to be flushed. - const SinkPredicates* sink_predicates_ = nullptr; + OptRef sink_predicates_; SymbolTable& symbol_table_; Thread::ThreadSynchronizer sync_; diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index d381ee2a328d8..acd49e5143137 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -678,7 +678,7 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( *buckets, parent_.next_histogram_id_++); if (!parent_.shutting_down_) { parent_.histogram_set_.insert(stat.get()); - if (parent_.sink_predicates_ && parent_.sink_predicates_->includeHistogram(*stat)) { + if (parent_.sink_predicates_.get() && parent_.sink_predicates_->includeHistogram(*stat)) { parent_.sinked_histograms_.insert(stat.get()); } } @@ -1003,7 +1003,7 @@ void ThreadLocalStoreImpl::forEachSinkedTextReadout( void ThreadLocalStoreImpl::forEachSinkedHistogram( std::function f_size, std::function f_stat) const { - if (sink_predicates_ != nullptr) { + if (sink_predicates_.get() != nullptr) { Thread::LockGuard lock(hist_mutex_); if (f_size != nullptr) { @@ -1017,9 +1017,12 @@ void ThreadLocalStoreImpl::forEachSinkedHistogram( } } -void ThreadLocalStoreImpl::setSinkPredicates(const SinkPredicates& sink_predicates) { - sink_predicates_ = &sink_predicates; - alloc_.setSinkPredicates(sink_predicates); +void ThreadLocalStoreImpl::setSinkPredicates(std::unique_ptr sink_predicates) { + sink_predicates_ = std::move(sink_predicates); + ASSERT(sink_predicates_.get() != nullptr); + if (sink_predicates_.get() != nullptr) { + alloc_.setSinkPredicates(*sink_predicates_); + } } } // namespace Stats diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 1c74cc95bb2ac..e229c9cc6062c 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -278,7 +278,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void forEachSinkedHistogram(std::function f_size, std::function f_stat) const override; - void setSinkPredicates(const SinkPredicates& sink_predicates) override; + void setSinkPredicates(std::unique_ptr sink_predicates) override; /** * @return a thread synchronizer object used for controlling thread behavior in tests. @@ -513,7 +513,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo StatNameHashSet* tls_rejected_stats); TlsCache& tlsCache() { return **tls_cache_; } - const SinkPredicates* sink_predicates_ = nullptr; + std::unique_ptr sink_predicates_ = nullptr; Allocator& alloc_; Event::Dispatcher* main_thread_dispatcher_{}; using TlsCacheSlot = ThreadLocal::TypedSlotPtr; diff --git a/source/server/configuration_impl.h b/source/server/configuration_impl.h index 9e1f2f0c11e31..7efa5e22fda54 100644 --- a/source/server/configuration_impl.h +++ b/source/server/configuration_impl.h @@ -56,17 +56,10 @@ class StatsConfigImpl : public StatsConfig { void addSink(Stats::SinkPtr sink) { sinks_.emplace_back(std::move(sink)); } - const Stats::SinkPredicates* sinkPredicates() const { return sink_predicates_.get(); } - - void setSinkPredicates(std::unique_ptr sink_predicates) { - sink_predicates_ = std::move(sink_predicates); - } - private: std::list sinks_; std::chrono::milliseconds flush_interval_; bool flush_on_admin_{false}; - std::unique_ptr sink_predicates_; }; /** diff --git a/source/server/server.h b/source/server/server.h index 179dd9a3f9106..0c3312f8b31f8 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -294,8 +294,7 @@ class InstanceImpl final : Logger::Loggable, Quic::QuicStatNames& quicStatNames() { return quic_stat_names_; } void setSinkPredicates(std::unique_ptr sink_predicates) override { - sink_predicates_ = std::move(sink_predicates); - stats_store_.setSinkPredicates(*sink_predicates_); + stats_store_.setSinkPredicates(std::move(sink_predicates)); } // ServerLifecycleNotifier @@ -395,7 +394,6 @@ class InstanceImpl final : Logger::Loggable, Quic::QuicStatNames quic_stat_names_; ServerFactoryContextImpl server_contexts_; absl::optional enable_reuse_port_default_; - std::unique_ptr sink_predicates_; bool stats_flush_in_progress_ : 1; diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 9b95e15200e36..e6dbab179234f 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -1,10 +1,12 @@ #include +#include #include #include #include "envoy/config/metrics/v3/stats.pb.h" #include "envoy/stats/histogram.h" +#include "envoy/stats/sink.h" #include "source/common/common/c_smart_ptr.h" #include "source/common/event/dispatcher_impl.h" #include "source/common/memory/stats.h" @@ -1564,6 +1566,62 @@ TEST_F(HistogramTest, ParentHistogramBucketSummary) { "B3.6e+06(1,1)", parent_histogram->bucketSummary()); } + +class SinkPredicatesImpl : public SinkPredicates { +public: + ~SinkPredicatesImpl() override = default; + absl::flat_hash_set& sinkedStatNames() { return sinked_stat_names_; } + bool includeCounter(const Counter&) override { return false; } + bool includeGauge(const Gauge&) override { return false; } + bool includeTextReadout(const TextReadout&) override { return false; } + bool includeHistogram(const Histogram& histogram) override { + return sinked_stat_names_.find(histogram.tagExtractedName()) != sinked_stat_names_.end(); + } + +private: + absl::flat_hash_set sinked_stat_names_; +}; + +TEST_F(HistogramTest, ForEachSinkedHistogram) { + std::unique_ptr moved_sink_predicates = + std::make_unique(); + SinkPredicatesImpl* sink_predicates = moved_sink_predicates.get(); + std::vector> sinked_histograms; + std::vector> unsinked_histograms; + + store_->setSinkPredicates(std::move(moved_sink_predicates)); + + const size_t num_stats = 11; + + for (size_t idx = 0; idx < num_stats; ++idx) { + auto stat_name = absl::StrCat("histogram.", idx); + // sink every 3rd stat + if ((idx + 1) % 3 == 0) { + sink_predicates->sinkedStatNames().insert(stat_name); + sinked_histograms.emplace_back( + store_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified)); + } else { + unsinked_histograms.emplace_back( + store_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified)); + } + } + + EXPECT_EQ(sinked_histograms.size(), 3); + EXPECT_EQ(unsinked_histograms.size(), 8); + + size_t num_sinked_histograms = 0; + size_t num_iterations = 0; + store_->forEachSinkedHistogram( + [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, + [&num_iterations, sink_predicates](Stats::ParentHistogram& histogram) { + EXPECT_NE(sink_predicates->sinkedStatNames().find(histogram.tagExtractedName()), + sink_predicates->sinkedStatNames().end()); + ++num_iterations; + }); + EXPECT_EQ(num_sinked_histograms, 3); + EXPECT_EQ(num_iterations, 3); +} + class ThreadLocalRealThreadsTestBase : public Thread::RealThreadsTestHelper, public ThreadLocalStoreNoMocksTestBase { protected: diff --git a/test/integration/server.h b/test/integration/server.h index 97255032d3628..bcabe592ead56 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -322,7 +322,7 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); store_.forEachSinkedHistogram(f_size, f_stat); } - void setSinkPredicates(const SinkPredicates& sink_predicates) override { + void setSinkPredicates(std::unique_ptr sink_predicates) override { UNREFERENCED_PARAMETER(sink_predicates); } diff --git a/test/server/server_stats_flush_benchmark_test.cc b/test/server/server_stats_flush_benchmark_test.cc index 98790c5c65d56..3fb4b3f1447bb 100644 --- a/test/server/server_stats_flush_benchmark_test.cc +++ b/test/server/server_stats_flush_benchmark_test.cc @@ -19,10 +19,30 @@ namespace Envoy { +class SinkPredicatesImpl : public Stats::SinkPredicates { +public: + bool includeCounter(const Stats::Counter&) override { return (++num_counters_) % 10 == 0; } + bool includeGauge(const Stats::Gauge&) override { return (++num_gauges_) % 10 == 0; } + bool includeTextReadout(const Stats::TextReadout&) override { + return (++num_text_readouts_) % 10 == 0; + } + bool includeHistogram(const Stats::Histogram&) override { return (++num_histograms_) % 10 == 0; } + +private: + size_t num_counters_ = 0; + size_t num_gauges_ = 0; + size_t num_text_readouts_ = 0; + size_t num_histograms_ = 0; +}; + class StatsSinkFlushSpeedTest { public: - StatsSinkFlushSpeedTest(size_t const num_stats) + StatsSinkFlushSpeedTest(size_t const num_stats, bool set_sink_predicates = false) : pool_(symbol_table_), stats_allocator_(symbol_table_), stats_store_(stats_allocator_) { + if (set_sink_predicates) { + stats_store_.setSinkPredicates( + std::unique_ptr{std::make_unique()}); + } // Create counters for (uint64_t idx = 0; idx < num_stats; ++idx) { @@ -40,6 +60,12 @@ class StatsSinkFlushSpeedTest { auto stat_name = pool_.add(absl::StrCat("text_readout.", idx)); stats_store_.textReadoutFromStatName(stat_name).set(absl::StrCat("text_readout.", idx)); } + + // Create histograms + for (uint64_t idx = 0; idx < num_stats; ++idx) { + std::string stat_name(absl::StrCat("histogram.", idx)); + stats_store_.histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified); + } } void test(::benchmark::State& state) { @@ -69,6 +95,22 @@ static void bmFlushToSinks(::benchmark::State& state) { StatsSinkFlushSpeedTest speed_test(state.range(0)); speed_test.test(state); } + +static void bmFlushToSinksWithPredicatesSet(::benchmark::State& state) { + // Skip expensive benchmarks for unit tests. + if (benchmark::skipExpensiveBenchmarks() && state.range(0) > 100) { + state.SkipWithError("Skipping expensive benchmark"); + return; + } + + StatsSinkFlushSpeedTest speed_test(state.range(0), true); + speed_test.test(state); +} + BENCHMARK(bmFlushToSinks)->Unit(::benchmark::kMillisecond)->RangeMultiplier(10)->Range(10, 1000000); +BENCHMARK(bmFlushToSinksWithPredicatesSet) + ->Unit(::benchmark::kMillisecond) + ->RangeMultiplier(10) + ->Range(10, 1000000); } // namespace Envoy From d82fb8f82672f7b64263ae9406a94f5a16cde11a Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Tue, 9 Nov 2021 16:24:06 +0000 Subject: [PATCH 03/12] Fix format. Signed-off-by: Pradeep Rao --- source/common/stats/allocator_impl.h | 3 +-- test/common/stats/allocator_impl_test.cc | 11 ++++------- test/common/stats/thread_local_store_test.cc | 2 +- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/source/common/stats/allocator_impl.h b/source/common/stats/allocator_impl.h index 6ec75ac747772..3e7832ef744e4 100644 --- a/source/common/stats/allocator_impl.h +++ b/source/common/stats/allocator_impl.h @@ -104,8 +104,7 @@ class AllocatorImpl : public Allocator { std::vector deleted_gauges_ ABSL_GUARDED_BY(mutex_); std::vector deleted_text_readouts_ ABSL_GUARDED_BY(mutex_); - template - using StatPointerSet = absl::flat_hash_set>; + template using StatPointerSet = absl::flat_hash_set; // Stat pointers that participate in the flush to sink process. StatPointerSet sinked_counters_ ABSL_GUARDED_BY(mutex_); StatPointerSet sinked_gauges_ ABSL_GUARDED_BY(mutex_); diff --git a/test/common/stats/allocator_impl_test.cc b/test/common/stats/allocator_impl_test.cc index 018ba3d169469..60dd9eb4c0c98 100644 --- a/test/common/stats/allocator_impl_test.cc +++ b/test/common/stats/allocator_impl_test.cc @@ -48,19 +48,19 @@ class SinkPredicatesImpl : public SinkPredicates { ~SinkPredicatesImpl() override = default; StatNameHashSet& sinkedStatNames() { return sinked_stat_names_; } - bool includeCounter(const Counter& counter) const override { + bool includeCounter(const Counter& counter) override { return sinked_stat_names_.find(counter.statName()) != sinked_stat_names_.end(); } - bool includeGauge(const Gauge& gauge) const override { + bool includeGauge(const Gauge& gauge) override { return sinked_stat_names_.find(gauge.statName()) != sinked_stat_names_.end(); } - bool includeTextReadout(const TextReadout& text_readout) const override { + bool includeTextReadout(const TextReadout& text_readout) override { return sinked_stat_names_.find(text_readout.statName()) != sinked_stat_names_.end(); } - bool includeHistogram(const Histogram& histogram) const override { + bool includeHistogram(const Histogram& histogram) override { return sinked_stat_names_.find(histogram.statName()) != sinked_stat_names_.end(); } @@ -438,7 +438,6 @@ TEST_F(AllocatorImplTest, AskForDeletedStat) { } TEST_F(AllocatorImplTest, ForEachSinkedCounter) { - SinkPredicatesImpl sink_predicates; std::vector sinked_counters; std::vector unsinked_counters; @@ -484,7 +483,6 @@ TEST_F(AllocatorImplTest, ForEachSinkedCounter) { } TEST_F(AllocatorImplTest, ForEachSinkedGauge) { - SinkPredicatesImpl sink_predicates; std::vector sinked_gauges; std::vector unsinked_gauges; @@ -529,7 +527,6 @@ TEST_F(AllocatorImplTest, ForEachSinkedGauge) { } TEST_F(AllocatorImplTest, ForEachSinkedTextReadout) { - SinkPredicatesImpl sink_predicates; std::vector sinked_text_readouts; std::vector unsinked_text_readouts; diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index e6dbab179234f..95397eb37ba6b 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -5,8 +5,8 @@ #include "envoy/config/metrics/v3/stats.pb.h" #include "envoy/stats/histogram.h" - #include "envoy/stats/sink.h" + #include "source/common/common/c_smart_ptr.h" #include "source/common/event/dispatcher_impl.h" #include "source/common/memory/stats.h" From 5798431fd09383024e119096da8ebc756b3af374 Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Wed, 1 Dec 2021 21:44:53 +0000 Subject: [PATCH 04/12] Fix tests. Signed-off-by: Pradeep Rao --- envoy/stats/allocator.h | 2 +- envoy/stats/store.h | 2 +- source/common/stats/thread_local_store.cc | 17 +++++++---------- test/common/stats/allocator_impl_test.cc | 1 + test/mocks/stats/mocks.h | 2 ++ test/server/server_test.cc | 1 - 6 files changed, 12 insertions(+), 13 deletions(-) diff --git a/envoy/stats/allocator.h b/envoy/stats/allocator.h index 223b1cab47068..ae0a075116339 100644 --- a/envoy/stats/allocator.h +++ b/envoy/stats/allocator.h @@ -96,7 +96,7 @@ class Allocator { virtual void forEachSinkedTextReadout(SizeFn f_size, StatFn f_stat) const PURE; /** - * Set the predicates to filter counters, gauges and text readouts for sink. + * Set the predicates to filter stats for sink. */ virtual void setSinkPredicates(std::unique_ptr&& sink_predicates) PURE; diff --git a/envoy/stats/store.h b/envoy/stats/store.h index 28b9ab00d40aa..e8950639d3e46 100644 --- a/envoy/stats/store.h +++ b/envoy/stats/store.h @@ -135,7 +135,7 @@ class StoreRoot : public Store { virtual void mergeHistograms(PostMergeCb merge_complete_cb) PURE; /** - * Set predicates for filtering counters, gauges and text readouts to be flushed to sinks. + * Set predicates for filtering stats to be flushed to sinks. * Note that if the sink predicates object is set, we do not send non-sink stats over to the * child process during hot restart. This will result in the admin stats console being wrong * during hot restart. diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index fd50e36731720..892ecd6122eea 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -182,14 +182,10 @@ std::vector ThreadLocalStoreImpl::textReadouts() const { std::vector ThreadLocalStoreImpl::histograms() const { std::vector ret; - Thread::LockGuard lock(hist_mutex_); - { - ret.reserve(histogram_set_.size()); - for (const auto& histogram_ptr : histogram_set_) { - ret.emplace_back(histogram_ptr); - } - } - + forEachHistogram([&ret](std::size_t size) mutable { ret.reserve(size); }, + [&ret](ParentHistogram& histogram) mutable { + ret.emplace_back(ParentHistogramSharedPtr(&histogram)); + }); return ret; } @@ -678,7 +674,8 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( *buckets, parent_.next_histogram_id_++); if (!parent_.shutting_down_) { parent_.histogram_set_.insert(stat.get()); - if (parent_.sink_predicates_.get() && parent_.sink_predicates_->includeHistogram(*stat)) { + if (parent_.sink_predicates_.has_value() && + parent_.sink_predicates_->includeHistogram(*stat)) { parent_.sinked_histograms_.insert(stat.get()); } } @@ -995,7 +992,7 @@ void ThreadLocalStoreImpl::forEachSinkedTextReadout(SizeFn f_size, void ThreadLocalStoreImpl::forEachSinkedHistogram(SizeFn f_size, StatFn f_stat) const { - if (sink_predicates_.get() != nullptr) { + if (sink_predicates_.has_value()) { Thread::LockGuard lock(hist_mutex_); if (f_size != nullptr) { diff --git a/test/common/stats/allocator_impl_test.cc b/test/common/stats/allocator_impl_test.cc index a4c0eb2d271b4..9329bfa7f2fbf 100644 --- a/test/common/stats/allocator_impl_test.cc +++ b/test/common/stats/allocator_impl_test.cc @@ -59,6 +59,7 @@ class TestSinkPredicates : public SinkPredicates { bool includeTextReadout(const TextReadout& text_readout) override { return sinked_stat_names_.find(text_readout.statName()) != sinked_stat_names_.end(); } + bool includeHistogram(const Histogram&) override { return false; } private: StatNameHashSet sinked_stat_names_; diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 42837995662a5..52fe586fb4a03 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -271,6 +271,7 @@ class MockSinkPredicates : public SinkPredicates { MOCK_METHOD(bool, includeCounter, (const Counter&)); MOCK_METHOD(bool, includeGauge, (const Gauge&)); MOCK_METHOD(bool, includeTextReadout, (const TextReadout&)); + MOCK_METHOD(bool, includeHistogram, (const Histogram&)); }; class MockStore : public TestUtil::TestStore { @@ -299,6 +300,7 @@ class MockStore : public TestUtil::TestStore { MOCK_METHOD(void, forEachGauge, (SizeFn, StatFn), (const)); MOCK_METHOD(void, forEachTextReadout, (SizeFn, StatFn), (const)); MOCK_METHOD(void, forEachHistogram, (SizeFn, StatFn), (const)); + MOCK_METHOD(void, forEachSinkedHistogram, (SizeFn, StatFn), (const)); MOCK_METHOD(CounterOptConstRef, findCounter, (StatName), (const)); MOCK_METHOD(GaugeOptConstRef, findGauge, (StatName), (const)); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index a5c78470b6dc2..ff273272660aa 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -91,7 +91,6 @@ TEST(ServerInstanceUtil, flushHelper) { NiceMock mock_store; Stats::ParentHistogramSharedPtr parent_histogram(new Stats::MockParentHistogram()); std::vector parent_histograms = {parent_histogram}; - ON_CALL(mock_store, histograms).WillByDefault(Return(parent_histograms)); ON_CALL(mock_store, forEachSinkedHistogram) .WillByDefault([&](std::function f_size, std::function f_stat) { From b430e670c2638e62d9ebfd0544e1d262070fa75c Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Thu, 2 Dec 2021 03:35:56 +0000 Subject: [PATCH 05/12] Add tests. Signed-off-by: Pradeep Rao --- source/common/stats/thread_local_store.cc | 8 +++ test/common/stats/thread_local_store_test.cc | 67 +++++++++++++++++++- 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 892ecd6122eea..380d2de0baa5d 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -1011,6 +1011,14 @@ void ThreadLocalStoreImpl::setSinkPredicates(std::unique_ptr&& s if (sink_predicates != nullptr) { sink_predicates_.emplace(*sink_predicates); alloc_.setSinkPredicates(std::move(sink_predicates)); + // Add histograms to the set of sinked histograms. + Thread::LockGuard lock(hist_mutex_); + sinked_histograms_.clear(); + for (auto& histogram : histogram_set_) { + if (sink_predicates_->includeHistogram(*histogram)) { + sinked_histograms_.insert(histogram); + } + } } } diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 5b58666d34c25..9ed935ab18128 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -1582,6 +1582,41 @@ class SinkPredicatesTest : public SinkPredicates { absl::flat_hash_set sinked_stat_names_; }; +TEST_F(HistogramTest, ForEachHistogram) { + std::vector> histograms; + + const size_t num_stats = 11; + for (size_t idx = 0; idx < num_stats; ++idx) { + auto stat_name = absl::StrCat("histogram.", idx); + histograms.emplace_back( + store_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified)); + } + EXPECT_EQ(histograms.size(), 11); + + size_t num_histograms = 0; + size_t num_iterations = 0; + store_->forEachHistogram([&num_histograms](std::size_t size) { num_histograms = size; }, + [&num_iterations](Stats::ParentHistogram&) { ++num_iterations; }); + EXPECT_EQ(num_histograms, 11); + EXPECT_EQ(num_iterations, 11); + + Histogram& deleted_histogram = histograms[4]; + + // Verify that rejecting histograms removes them from the iteration set. + envoy::config::metrics::v3::StatsConfig stats_config_; + stats_config_.mutable_stats_matcher()->set_reject_all(true); + store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); + num_histograms = 0; + num_iterations = 0; + store_->forEachHistogram([&num_histograms](std::size_t size) { num_histograms = size; }, + [&num_iterations](Stats::ParentHistogram&) { ++num_iterations; }); + EXPECT_EQ(num_histograms, 0); + EXPECT_EQ(num_iterations, 0); + + // Verify that we can access the local reference without a crash. + (void)deleted_histogram.unit(); +} + TEST_F(HistogramTest, ForEachSinkedHistogram) { std::unique_ptr moved_sink_predicates = std::make_unique(); @@ -1589,11 +1624,25 @@ TEST_F(HistogramTest, ForEachSinkedHistogram) { std::vector> sinked_histograms; std::vector> unsinked_histograms; - store_->setSinkPredicates(std::move(moved_sink_predicates)); - const size_t num_stats = 11; + // Create some histograms before setting the predicates. + for (size_t idx = 0; idx < num_stats / 2; ++idx) { + auto stat_name = absl::StrCat("histogram.", idx); + // sink every 3rd stat + if ((idx + 1) % 3 == 0) { + sink_predicates->sinkedStatNames().insert(stat_name); + sinked_histograms.emplace_back( + store_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified)); + } else { + unsinked_histograms.emplace_back( + store_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified)); + } + } - for (size_t idx = 0; idx < num_stats; ++idx) { + store_->setSinkPredicates(std::move(moved_sink_predicates)); + + // Create some histograms after setting the predicates. + for (size_t idx = num_stats / 2; idx < num_stats; ++idx) { auto stat_name = absl::StrCat("histogram.", idx); // sink every 3rd stat if ((idx + 1) % 3 == 0) { @@ -1620,6 +1669,18 @@ TEST_F(HistogramTest, ForEachSinkedHistogram) { }); EXPECT_EQ(num_sinked_histograms, 3); EXPECT_EQ(num_iterations, 3); + + // Verify that rejecting histograms removes them from the sink set. + envoy::config::metrics::v3::StatsConfig stats_config_; + stats_config_.mutable_stats_matcher()->set_reject_all(true); + store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); + num_sinked_histograms = 0; + num_iterations = 0; + store_->forEachSinkedHistogram( + [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, + [&num_iterations](Stats::ParentHistogram&) { ++num_iterations; }); + EXPECT_EQ(num_sinked_histograms, 0); + EXPECT_EQ(num_iterations, 0); } class ThreadLocalRealThreadsTestBase : public Thread::RealThreadsTestHelper, From 2544e4f2a08090af9a8a77e4242c7db93c09b045 Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Tue, 4 Jan 2022 20:29:33 +0000 Subject: [PATCH 06/12] Address feedback. Signed-off-by: Pradeep Rao --- source/common/stats/isolated_store_impl.h | 4 +- source/server/server.cc | 14 +-- test/common/stats/allocator_impl_test.cc | 40 ++---- test/common/stats/stat_test_utility.h | 23 ++++ test/common/stats/thread_local_store_test.cc | 123 +++++++++++++++---- 5 files changed, 138 insertions(+), 66 deletions(-) diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index c2e349e269338..46fe400dbdb16 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -245,8 +245,8 @@ class IsolatedStoreImpl : public StoreImpl { } void forEachSinkedHistogram(SizeFn f_size, StatFn f_stat) const override { - (void)f_size; - (void)f_stat; + UNREFERENCED_PARAMETER(f_size); + UNREFERENCED_PARAMETER(f_stat); } private: diff --git a/source/server/server.cc b/source/server/server.cc index 8d142e7dfb8ca..350671fb0bd08 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -170,38 +170,38 @@ void InstanceImpl::failHealthcheck(bool fail) { MetricSnapshotImpl::MetricSnapshotImpl(Stats::Store& store, TimeSource& time_source) { store.forEachSinkedCounter( - [this](std::size_t size) mutable { + [this](std::size_t size) { snapped_counters_.reserve(size); counters_.reserve(size); }, - [this](Stats::Counter& counter) mutable { + [this](Stats::Counter& counter) { snapped_counters_.push_back(Stats::CounterSharedPtr(&counter)); counters_.push_back({counter.latch(), counter}); }); store.forEachSinkedGauge( - [this](std::size_t size) mutable { + [this](std::size_t size) { snapped_gauges_.reserve(size); gauges_.reserve(size); }, - [this](Stats::Gauge& gauge) mutable { + [this](Stats::Gauge& gauge) { ASSERT(gauge.importMode() != Stats::Gauge::ImportMode::Uninitialized); snapped_gauges_.push_back(Stats::GaugeSharedPtr(&gauge)); gauges_.push_back(gauge); }); store.forEachSinkedHistogram( - [this](std::size_t size) mutable { + [this](std::size_t size) { snapped_histograms_.reserve(size); histograms_.reserve(size); }, - [this](Stats::ParentHistogram& histogram) mutable { + [this](Stats::ParentHistogram& histogram) { snapped_histograms_.push_back(Stats::ParentHistogramSharedPtr(&histogram)); histograms_.push_back(histogram); }); store.forEachSinkedTextReadout( - [this](std::size_t size) mutable { + [this](std::size_t size) { snapped_text_readouts_.reserve(size); text_readouts_.reserve(size); }, diff --git a/test/common/stats/allocator_impl_test.cc b/test/common/stats/allocator_impl_test.cc index 9329bfa7f2fbf..859cc0c6d3618 100644 --- a/test/common/stats/allocator_impl_test.cc +++ b/test/common/stats/allocator_impl_test.cc @@ -6,6 +6,7 @@ #include "source/common/stats/allocator_impl.h" +#include "test/common/stats/stat_test_utility.h" #include "test/test_common/logging.h" #include "test/test_common/thread_factory_for_test.h" @@ -44,27 +45,6 @@ class AllocatorImplTest : public testing::Test { bool are_stats_marked_for_deletion_ = false; }; -class TestSinkPredicates : public SinkPredicates { -public: - ~TestSinkPredicates() override = default; - StatNameHashSet& sinkedStatNames() { return sinked_stat_names_; } - - // SinkPredicates - bool includeCounter(const Counter& counter) override { - return sinked_stat_names_.find(counter.statName()) != sinked_stat_names_.end(); - } - bool includeGauge(const Gauge& gauge) override { - return sinked_stat_names_.find(gauge.statName()) != sinked_stat_names_.end(); - } - bool includeTextReadout(const TextReadout& text_readout) override { - return sinked_stat_names_.find(text_readout.statName()) != sinked_stat_names_.end(); - } - bool includeHistogram(const Histogram&) override { return false; } - -private: - StatNameHashSet sinked_stat_names_; -}; - // Allocate 2 counters of the same name, and you'll get the same object. TEST_F(AllocatorImplTest, CountersWithSameName) { StatName counter_name = makeStat("counter.name"); @@ -435,9 +415,9 @@ TEST_F(AllocatorImplTest, AskForDeletedStat) { } TEST_F(AllocatorImplTest, ForEachSinkedCounter) { - std::unique_ptr moved_sink_predicates = - std::make_unique(); - TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); + std::unique_ptr moved_sink_predicates = + std::make_unique(); + Stats::TestUtil::TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); std::vector sinked_counters; std::vector unsinked_counters; @@ -481,9 +461,9 @@ TEST_F(AllocatorImplTest, ForEachSinkedCounter) { } TEST_F(AllocatorImplTest, ForEachSinkedGauge) { - std::unique_ptr moved_sink_predicates = - std::make_unique(); - TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); + std::unique_ptr moved_sink_predicates = + std::make_unique(); + Stats::TestUtil::TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); std::vector sinked_gauges; std::vector unsinked_gauges; @@ -527,9 +507,9 @@ TEST_F(AllocatorImplTest, ForEachSinkedGauge) { } TEST_F(AllocatorImplTest, ForEachSinkedTextReadout) { - std::unique_ptr moved_sink_predicates = - std::make_unique(); - TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); + std::unique_ptr moved_sink_predicates = + std::make_unique(); + Stats::TestUtil::TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); std::vector sinked_text_readouts; std::vector unsinked_text_readouts; diff --git a/test/common/stats/stat_test_utility.h b/test/common/stats/stat_test_utility.h index 875dcf3b5a2fb..f610689845a46 100644 --- a/test/common/stats/stat_test_utility.h +++ b/test/common/stats/stat_test_utility.h @@ -219,6 +219,29 @@ std::vector serializeDeserializeNumber(uint64_t number); // Serializes a string into a MemBlock and then decodes it. void serializeDeserializeString(absl::string_view in); +class TestSinkPredicates : public SinkPredicates { +public: + ~TestSinkPredicates() override = default; + StatNameHashSet& sinkedStatNames() { return sinked_stat_names_; } + + // SinkPredicates + bool includeCounter(const Counter& counter) override { + return sinked_stat_names_.find(counter.statName()) != sinked_stat_names_.end(); + } + bool includeGauge(const Gauge& gauge) override { + return sinked_stat_names_.find(gauge.statName()) != sinked_stat_names_.end(); + } + bool includeTextReadout(const TextReadout& text_readout) override { + return sinked_stat_names_.find(text_readout.statName()) != sinked_stat_names_.end(); + } + bool includeHistogram(const Histogram& histogram) override { + return sinked_stat_names_.find(histogram.statName()) != sinked_stat_names_.end(); + } + +private: + StatNameHashSet sinked_stat_names_; +}; + } // namespace TestUtil } // namespace Stats } // namespace Envoy diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 9ed935ab18128..b1b2754149bc4 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -1,7 +1,9 @@ #include #include #include +#include #include +#include #include "envoy/config/metrics/v3/stats.pb.h" #include "envoy/stats/histogram.h" @@ -1567,21 +1569,6 @@ TEST_F(HistogramTest, ParentHistogramBucketSummary) { parent_histogram->bucketSummary()); } -class SinkPredicatesTest : public SinkPredicates { -public: - ~SinkPredicatesTest() override = default; - absl::flat_hash_set& sinkedStatNames() { return sinked_stat_names_; } - bool includeCounter(const Counter&) override { return false; } - bool includeGauge(const Gauge&) override { return false; } - bool includeTextReadout(const TextReadout&) override { return false; } - bool includeHistogram(const Histogram& histogram) override { - return sinked_stat_names_.find(histogram.tagExtractedName()) != sinked_stat_names_.end(); - } - -private: - absl::flat_hash_set sinked_stat_names_; -}; - TEST_F(HistogramTest, ForEachHistogram) { std::vector> histograms; @@ -1614,28 +1601,31 @@ TEST_F(HistogramTest, ForEachHistogram) { EXPECT_EQ(num_iterations, 0); // Verify that we can access the local reference without a crash. - (void)deleted_histogram.unit(); + EXPECT_EQ(deleted_histogram.unit(), Stats::Histogram::Unit::Unspecified); } TEST_F(HistogramTest, ForEachSinkedHistogram) { - std::unique_ptr moved_sink_predicates = - std::make_unique(); - SinkPredicatesTest* sink_predicates = moved_sink_predicates.get(); + StatNamePool pool(store_->symbolTable()); + + std::unique_ptr moved_sink_predicates = + std::make_unique(); + Stats::TestUtil::TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); std::vector> sinked_histograms; std::vector> unsinked_histograms; const size_t num_stats = 11; // Create some histograms before setting the predicates. for (size_t idx = 0; idx < num_stats / 2; ++idx) { - auto stat_name = absl::StrCat("histogram.", idx); - // sink every 3rd stat + auto name = absl::StrCat("histogram.", idx); + Stats::StatName stat_name = pool.add(name); + // sink every 3rd stat if ((idx + 1) % 3 == 0) { sink_predicates->sinkedStatNames().insert(stat_name); sinked_histograms.emplace_back( - store_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified)); + store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); } else { unsinked_histograms.emplace_back( - store_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified)); + store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); } } @@ -1643,15 +1633,16 @@ TEST_F(HistogramTest, ForEachSinkedHistogram) { // Create some histograms after setting the predicates. for (size_t idx = num_stats / 2; idx < num_stats; ++idx) { - auto stat_name = absl::StrCat("histogram.", idx); + auto name = absl::StrCat("histogram.", idx); + Stats::StatName stat_name = pool.add(name); // sink every 3rd stat if ((idx + 1) % 3 == 0) { sink_predicates->sinkedStatNames().insert(stat_name); sinked_histograms.emplace_back( - store_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified)); + store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); } else { unsinked_histograms.emplace_back( - store_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified)); + store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); } } @@ -1663,7 +1654,7 @@ TEST_F(HistogramTest, ForEachSinkedHistogram) { store_->forEachSinkedHistogram( [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, [&num_iterations, sink_predicates](Stats::ParentHistogram& histogram) { - EXPECT_NE(sink_predicates->sinkedStatNames().find(histogram.tagExtractedName()), + EXPECT_NE(sink_predicates->sinkedStatNames().find(histogram.statName()), sink_predicates->sinkedStatNames().end()); ++num_iterations; }); @@ -1932,5 +1923,83 @@ TEST_F(HistogramThreadTest, ScopeOverlap) { store_->histogramFromString("histogram_after_shutdown", Histogram::Unit::Unspecified); } +size_t getMemoryUsage() { +#ifndef WIN32 + std::ifstream status("/proc/self/status", std::ios_base::in); + std::string line; + const std::string key("VmSize:"); + while (status.good() && !status.eof()) { + std::getline(status, line); + const auto pos = line.find(key); + if (pos != std::string::npos) { + return std::stoull(line.substr(pos + key.size())); + } + } +#endif + return 0; +} + +// Verify that recording values for histograms that are not sinked does not +// cause them to grow in memory over time. +TEST_F(HistogramTest, SinkedHistogramMemoryTest) { + StatNamePool pool(store_->symbolTable()); + std::unique_ptr moved_sink_predicates = + std::make_unique(); + Stats::TestUtil::TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); + Stats::StatName stat_name = pool.add("h1"); + sink_predicates->sinkedStatNames().insert(stat_name); + store_->setSinkPredicates(std::move(moved_sink_predicates)); + + Histogram& h1 = store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified); + Histogram& h2 = store_->histogramFromString("h2", Stats::Histogram::Unit::Unspecified); + + EXPECT_EQ("h1", h1.name()); + EXPECT_EQ("h2", h2.name()); + + // Create a random number generator to populate histogram values + std::random_device random_d; + std::mt19937_64 generator(random_d()); + std::uniform_int_distribution distribution(0, 1000); + + const size_t num_flushes = 512; + const size_t num_updates = 20; + size_t memoryUsageOld = 0; + // Outer loop for flushes. + for (size_t idx = 0; idx < num_flushes; ++idx) { + // Inner loop to record histogram values. + for (size_t idx2 = 0; idx2 < num_updates; ++idx2) { + const uint64_t h1_val = distribution(generator); + EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), h1_val)); + h1.recordValue(h1_val); + const uint64_t h2_val = distribution(generator); + EXPECT_CALL(sink_, onHistogramComplete(Ref(h2), h2_val)); + h2.recordValue(h2_val); + } + + store_->mergeHistograms([this, sink_predicates]() -> void { + size_t num_iterations = 0; + size_t num_sinked_histograms = 0; + store_->forEachSinkedHistogram( + [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, + [&num_iterations, sink_predicates](Stats::ParentHistogram& histogram) { + EXPECT_NE(sink_predicates->sinkedStatNames().find(histogram.statName()), + sink_predicates->sinkedStatNames().end()); + ++num_iterations; + }); + EXPECT_EQ(num_sinked_histograms, 1); + EXPECT_EQ(num_iterations, 1); + }); + size_t memoryUsageNew = getMemoryUsage(); +#ifndef WIN32 + EXPECT_NE(memoryUsageNew, 0); +#endif + if (memoryUsageOld == 0) { + memoryUsageOld = memoryUsageNew; + } else { + EXPECT_EQ(memoryUsageNew, memoryUsageOld); + } + } +} + } // namespace Stats } // namespace Envoy From 560a6f9c2829c3058e6301b4ca1e0fb7b9c6ed67 Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Wed, 19 Jan 2022 22:19:13 +0000 Subject: [PATCH 07/12] Fix test. Signed-off-by: Pradeep Rao --- test/common/stats/thread_local_store_test.cc | 98 +++++++------------- 1 file changed, 33 insertions(+), 65 deletions(-) diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index b1b2754149bc4..e53f661a2ab68 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -3,7 +3,6 @@ #include #include #include -#include #include "envoy/config/metrics/v3/stats.pb.h" #include "envoy/stats/histogram.h" @@ -1923,25 +1922,9 @@ TEST_F(HistogramThreadTest, ScopeOverlap) { store_->histogramFromString("histogram_after_shutdown", Histogram::Unit::Unspecified); } -size_t getMemoryUsage() { -#ifndef WIN32 - std::ifstream status("/proc/self/status", std::ios_base::in); - std::string line; - const std::string key("VmSize:"); - while (status.good() && !status.eof()) { - std::getline(status, line); - const auto pos = line.find(key); - if (pos != std::string::npos) { - return std::stoull(line.substr(pos + key.size())); - } - } -#endif - return 0; -} - -// Verify that recording values for histograms that are not sinked does not -// cause them to grow in memory over time. -TEST_F(HistogramTest, SinkedHistogramMemoryTest) { +// Verify that histograms that are not flushed to sinks are merged in the call +// to mergeHistograms +TEST_F(HistogramTest, UnsinkedHistogramsAreMerged) { StatNamePool pool(store_->symbolTable()); std::unique_ptr moved_sink_predicates = std::make_unique(); @@ -1950,55 +1933,40 @@ TEST_F(HistogramTest, SinkedHistogramMemoryTest) { sink_predicates->sinkedStatNames().insert(stat_name); store_->setSinkPredicates(std::move(moved_sink_predicates)); - Histogram& h1 = store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified); - Histogram& h2 = store_->histogramFromString("h2", Stats::Histogram::Unit::Unspecified); + auto& h1 = static_cast( + store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); + stat_name = pool.add("h2"); + auto& h2 = static_cast( + store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("h2", h2.name()); + EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 5)); + EXPECT_CALL(sink_, onHistogramComplete(Ref(h2), 5)); + + h1.recordValue(5); + h2.recordValue(5); + + EXPECT_THAT(h1.cumulativeStatistics().bucketSummary(), HasSubstr(" B10: 0,")); + EXPECT_THAT(h2.cumulativeStatistics().bucketSummary(), HasSubstr(" B10: 0,")); + + store_->mergeHistograms([this, sink_predicates]() -> void { + size_t num_iterations = 0; + size_t num_sinked_histograms = 0; + store_->forEachSinkedHistogram( + [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, + [&num_iterations, sink_predicates](Stats::ParentHistogram& histogram) { + EXPECT_NE(sink_predicates->sinkedStatNames().find(histogram.statName()), + sink_predicates->sinkedStatNames().end()); + ++num_iterations; + }); + EXPECT_EQ(num_sinked_histograms, 1); + EXPECT_EQ(num_iterations, 1); + }); - // Create a random number generator to populate histogram values - std::random_device random_d; - std::mt19937_64 generator(random_d()); - std::uniform_int_distribution distribution(0, 1000); - - const size_t num_flushes = 512; - const size_t num_updates = 20; - size_t memoryUsageOld = 0; - // Outer loop for flushes. - for (size_t idx = 0; idx < num_flushes; ++idx) { - // Inner loop to record histogram values. - for (size_t idx2 = 0; idx2 < num_updates; ++idx2) { - const uint64_t h1_val = distribution(generator); - EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), h1_val)); - h1.recordValue(h1_val); - const uint64_t h2_val = distribution(generator); - EXPECT_CALL(sink_, onHistogramComplete(Ref(h2), h2_val)); - h2.recordValue(h2_val); - } - - store_->mergeHistograms([this, sink_predicates]() -> void { - size_t num_iterations = 0; - size_t num_sinked_histograms = 0; - store_->forEachSinkedHistogram( - [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, - [&num_iterations, sink_predicates](Stats::ParentHistogram& histogram) { - EXPECT_NE(sink_predicates->sinkedStatNames().find(histogram.statName()), - sink_predicates->sinkedStatNames().end()); - ++num_iterations; - }); - EXPECT_EQ(num_sinked_histograms, 1); - EXPECT_EQ(num_iterations, 1); - }); - size_t memoryUsageNew = getMemoryUsage(); -#ifndef WIN32 - EXPECT_NE(memoryUsageNew, 0); -#endif - if (memoryUsageOld == 0) { - memoryUsageOld = memoryUsageNew; - } else { - EXPECT_EQ(memoryUsageNew, memoryUsageOld); - } - } + EXPECT_THAT(h1.cumulativeStatistics().bucketSummary(), HasSubstr(" B10: 1,")); + EXPECT_THAT(h2.cumulativeStatistics().bucketSummary(), HasSubstr(" B10: 1,")); + EXPECT_EQ(h1.cumulativeStatistics().bucketSummary(), h2.cumulativeStatistics().bucketSummary()); } } // namespace Stats From eb8911750b8429fcfe6ac6b6d09731777a880b87 Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Thu, 20 Jan 2022 14:19:13 +0000 Subject: [PATCH 08/12] Add method to get sink predicates from StoreRoot. Signed-off-by: Pradeep Rao --- envoy/stats/store.h | 3 ++ source/common/stats/thread_local_store.h | 1 + test/common/stats/thread_local_store_test.cc | 29 ++++++++++---------- test/integration/server.h | 2 ++ 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/envoy/stats/store.h b/envoy/stats/store.h index e8950639d3e46..9fbaa75505c8b 100644 --- a/envoy/stats/store.h +++ b/envoy/stats/store.h @@ -5,6 +5,7 @@ #include #include "envoy/common/pure.h" +#include "envoy/common/optref.h" #include "envoy/stats/histogram.h" #include "envoy/stats/scope.h" #include "envoy/stats/stats.h" @@ -141,6 +142,8 @@ class StoreRoot : public Store { * during hot restart. */ virtual void setSinkPredicates(std::unique_ptr&& sink_predicates) PURE; + + virtual OptRef sinkPredicates() PURE; }; using StoreRootPtr = std::unique_ptr; diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 25b4b25fd5705..4ce4812f25af3 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -269,6 +269,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void forEachSinkedHistogram(SizeFn f_size, StatFn f_stat) const override; void setSinkPredicates(std::unique_ptr&& sink_predicates) override; + OptRef sinkPredicates() override { return sink_predicates_; } /** * @return a thread synchronizer object used for controlling thread behavior in tests. diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index e53f661a2ab68..5562efddff470 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -1606,9 +1606,8 @@ TEST_F(HistogramTest, ForEachHistogram) { TEST_F(HistogramTest, ForEachSinkedHistogram) { StatNamePool pool(store_->symbolTable()); - std::unique_ptr moved_sink_predicates = + std::unique_ptr sink_predicates = std::make_unique(); - Stats::TestUtil::TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); std::vector> sinked_histograms; std::vector> unsinked_histograms; @@ -1628,7 +1627,7 @@ TEST_F(HistogramTest, ForEachSinkedHistogram) { } } - store_->setSinkPredicates(std::move(moved_sink_predicates)); + store_->setSinkPredicates(std::move(sink_predicates)); // Create some histograms after setting the predicates. for (size_t idx = num_stats / 2; idx < num_stats; ++idx) { @@ -1636,7 +1635,9 @@ TEST_F(HistogramTest, ForEachSinkedHistogram) { Stats::StatName stat_name = pool.add(name); // sink every 3rd stat if ((idx + 1) % 3 == 0) { - sink_predicates->sinkedStatNames().insert(stat_name); + static_cast(store_->sinkPredicates().ptr()) + ->sinkedStatNames() + .insert(stat_name); sinked_histograms.emplace_back( store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); } else { @@ -1652,7 +1653,8 @@ TEST_F(HistogramTest, ForEachSinkedHistogram) { size_t num_iterations = 0; store_->forEachSinkedHistogram( [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, - [&num_iterations, sink_predicates](Stats::ParentHistogram& histogram) { + [&num_iterations, sink_predicates = static_cast( + store_->sinkPredicates().ptr())](Stats::ParentHistogram& histogram) { EXPECT_NE(sink_predicates->sinkedStatNames().find(histogram.statName()), sink_predicates->sinkedStatNames().end()); ++num_iterations; @@ -1926,12 +1928,11 @@ TEST_F(HistogramThreadTest, ScopeOverlap) { // to mergeHistograms TEST_F(HistogramTest, UnsinkedHistogramsAreMerged) { StatNamePool pool(store_->symbolTable()); - std::unique_ptr moved_sink_predicates = - std::make_unique(); - Stats::TestUtil::TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); + store_->setSinkPredicates(std::make_unique()); + auto& sink_predicates = + static_cast(store_->sinkPredicates().ref()); Stats::StatName stat_name = pool.add("h1"); - sink_predicates->sinkedStatNames().insert(stat_name); - store_->setSinkPredicates(std::move(moved_sink_predicates)); + sink_predicates.sinkedStatNames().insert(stat_name); auto& h1 = static_cast( store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); @@ -1950,14 +1951,14 @@ TEST_F(HistogramTest, UnsinkedHistogramsAreMerged) { EXPECT_THAT(h1.cumulativeStatistics().bucketSummary(), HasSubstr(" B10: 0,")); EXPECT_THAT(h2.cumulativeStatistics().bucketSummary(), HasSubstr(" B10: 0,")); - store_->mergeHistograms([this, sink_predicates]() -> void { + store_->mergeHistograms([this, &sink_predicates]() -> void { size_t num_iterations = 0; size_t num_sinked_histograms = 0; store_->forEachSinkedHistogram( [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, - [&num_iterations, sink_predicates](Stats::ParentHistogram& histogram) { - EXPECT_NE(sink_predicates->sinkedStatNames().find(histogram.statName()), - sink_predicates->sinkedStatNames().end()); + [&num_iterations, &sink_predicates](Stats::ParentHistogram& histogram) { + EXPECT_NE(sink_predicates.sinkedStatNames().find(histogram.statName()), + sink_predicates.sinkedStatNames().end()); ++num_iterations; }); EXPECT_EQ(num_sinked_histograms, 1); diff --git a/test/integration/server.h b/test/integration/server.h index f0960ecbeeea3..21b5ab962e951 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -318,6 +318,8 @@ class TestIsolatedStoreImpl : public StoreRoot { UNREFERENCED_PARAMETER(sink_predicates); } + OptRef sinkPredicates() override { return OptRef{}; } + Counter& counterFromString(const std::string& name) override { Thread::LockGuard lock(lock_); return store_.counterFromString(name); From 9ae13c698585bba445e8372f0ec93760c6fd8554 Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Thu, 20 Jan 2022 14:46:12 +0000 Subject: [PATCH 09/12] Fix Format. Signed-off-by: Pradeep Rao --- envoy/stats/store.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/envoy/stats/store.h b/envoy/stats/store.h index 9fbaa75505c8b..5c9325ccf05ed 100644 --- a/envoy/stats/store.h +++ b/envoy/stats/store.h @@ -4,8 +4,8 @@ #include #include -#include "envoy/common/pure.h" #include "envoy/common/optref.h" +#include "envoy/common/pure.h" #include "envoy/stats/histogram.h" #include "envoy/stats/scope.h" #include "envoy/stats/stats.h" From f68aa44fe2954b81588f08c76df12e3f23734ae5 Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Thu, 20 Jan 2022 23:00:57 +0000 Subject: [PATCH 10/12] Address feedback. Signed-off-by: Pradeep Rao --- test/common/stats/thread_local_store_test.cc | 135 +++++++++---------- 1 file changed, 65 insertions(+), 70 deletions(-) diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 5562efddff470..2c25123fea80f 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -248,8 +248,8 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) { g1.set(0); EXPECT_EQ(0, found_gauge->get().value()); - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); - EXPECT_EQ(&h1, &store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified)); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); + EXPECT_EQ(&h1, &store_->histogramFromString("h1", Histogram::Unit::Unspecified)); StatNameManagedStorage h1_name("h1", symbol_table_); auto found_histogram = store_->findHistogram(h1_name.statName()); ASSERT_TRUE(found_histogram.has_value()); @@ -302,8 +302,8 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { g1.set(0); EXPECT_EQ(0, found_gauge->get().value()); - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); - EXPECT_EQ(&h1, &store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified)); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); + EXPECT_EQ(&h1, &store_->histogramFromString("h1", Histogram::Unit::Unspecified)); StatNameManagedStorage h1_name("h1", symbol_table_); auto found_histogram = store_->findHistogram(h1_name.statName()); ASSERT_TRUE(found_histogram.has_value()); @@ -369,8 +369,8 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { ASSERT_TRUE(found_gauge2.has_value()); EXPECT_EQ(&g2, &found_gauge2->get()); - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); - Histogram& h2 = scope1->histogramFromString("h2", Stats::Histogram::Unit::Unspecified); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); + Histogram& h2 = scope1->histogramFromString("h2", Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("scope1.h2", h2.name()); EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 100)); @@ -413,12 +413,11 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { } { StatNameManagedStorage storage("h3", symbol_table_); - Histogram& histogram = scope1->histogramFromStatNameWithTags( - StatName(storage.statName()), tags, Stats::Histogram::Unit::Unspecified); + Histogram& histogram = scope1->histogramFromStatNameWithTags(StatName(storage.statName()), tags, + Histogram::Unit::Unspecified); EXPECT_EQ(expectedTags, histogram.tags()); - EXPECT_EQ(&histogram, - &scope1->histogramFromStatNameWithTags(StatName(storage.statName()), tags, - Stats::Histogram::Unit::Unspecified)); + EXPECT_EQ(&histogram, &scope1->histogramFromStatNameWithTags(StatName(storage.statName()), tags, + Histogram::Unit::Unspecified)); } tls_.shutdownGlobalThreading(); @@ -714,9 +713,8 @@ TEST_F(LookupWithStatNameTest, All) { EXPECT_EQ(0, g1.tags().size()); Histogram& h1 = - store_->Store::histogramFromStatName(makeStatName("h1"), Stats::Histogram::Unit::Unspecified); - Histogram& h2 = - scope1->histogramFromStatName(makeStatName("h2"), Stats::Histogram::Unit::Unspecified); + store_->Store::histogramFromStatName(makeStatName("h1"), Histogram::Unit::Unspecified); + Histogram& h2 = scope1->histogramFromStatName(makeStatName("h2"), Histogram::Unit::Unspecified); scope1->deliverHistogramToSinks(h2, 0); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("scope1.h2", h2.name()); @@ -760,7 +758,7 @@ class StatsMatcherTLSTest : public StatsThreadLocalStoreTest { uint64_t memoryConsumedAddingClusterStats() { StatNamePool pool(symbol_table_); std::vector stat_names; - Stats::TestUtil::forEachSampleStat(1000, false, [&pool, &stat_names](absl::string_view name) { + TestUtil::forEachSampleStat(1000, false, [&pool, &stat_names](absl::string_view name) { stat_names.push_back(pool.add(name)); }); @@ -837,12 +835,12 @@ TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) { // Histogram Histogram& noop_histogram = - store_->histogramFromString("noop_histogram", Stats::Histogram::Unit::Unspecified); + store_->histogramFromString("noop_histogram", Histogram::Unit::Unspecified); EXPECT_EQ(noop_histogram.name(), ""); EXPECT_FALSE(noop_histogram.used()); - EXPECT_EQ(Stats::Histogram::Unit::Null, noop_histogram.unit()); + EXPECT_EQ(Histogram::Unit::Null, noop_histogram.unit()); Histogram& noop_histogram_2 = - store_->histogramFromString("noop_histogram_2", Stats::Histogram::Unit::Unspecified); + store_->histogramFromString("noop_histogram_2", Histogram::Unit::Unspecified); EXPECT_EQ(&noop_histogram, &noop_histogram_2); } @@ -865,7 +863,7 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { store_->gaugeFromString("lowercase_gauge", Gauge::ImportMode::Accumulate); EXPECT_EQ(lowercase_gauge.name(), "lowercase_gauge"); Histogram& lowercase_histogram = - store_->histogramFromString("lowercase_histogram", Stats::Histogram::Unit::Unspecified); + store_->histogramFromString("lowercase_histogram", Histogram::Unit::Unspecified); EXPECT_EQ(lowercase_histogram.name(), "lowercase_histogram"); TextReadout& lowercase_string = store_->textReadoutFromString("lowercase_string"); @@ -894,7 +892,7 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { // Histograms are harder to query and test, so we resort to testing that name() returns the empty // string. Histogram& uppercase_histogram = - store_->histogramFromString("upperCASE_histogram", Stats::Histogram::Unit::Unspecified); + store_->histogramFromString("upperCASE_histogram", Histogram::Unit::Unspecified); EXPECT_EQ(uppercase_histogram.name(), ""); // Adding another exclusion rule -- now we reject not just uppercase stats but those starting with @@ -931,15 +929,15 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { EXPECT_EQ(invalid_gauge_2.value(), 0); Histogram& valid_histogram = - store_->histogramFromString("valid_histogram", Stats::Histogram::Unit::Unspecified); + store_->histogramFromString("valid_histogram", Histogram::Unit::Unspecified); EXPECT_EQ(valid_histogram.name(), "valid_histogram"); Histogram& invalid_histogram_1 = - store_->histogramFromString("invalid_histogram", Stats::Histogram::Unit::Unspecified); + store_->histogramFromString("invalid_histogram", Histogram::Unit::Unspecified); EXPECT_EQ(invalid_histogram_1.name(), ""); Histogram& invalid_histogram_2 = - store_->histogramFromString("also_INVALID_histogram", Stats::Histogram::Unit::Unspecified); + store_->histogramFromString("also_INVALID_histogram", Histogram::Unit::Unspecified); EXPECT_EQ(invalid_histogram_2.name(), ""); TextReadout& valid_string = store_->textReadoutFromString("valid_string"); @@ -964,7 +962,7 @@ TEST_F(StatsMatcherTLSTest, RejectPrefixDot) { store_->initializeThreading(main_thread_dispatcher_, tls_); stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->set_prefix( "cluster."); // Prefix match can be executed symbolically. - store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); + store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); uint64_t mem_consumed = memoryConsumedAddingClusterStats(); // No memory is consumed at all while rejecting stats from "prefix." @@ -980,7 +978,7 @@ TEST_F(StatsMatcherTLSTest, RejectPrefixNoDot) { store_->initializeThreading(main_thread_dispatcher_, tls_); stats_config_.mutable_stats_matcher()->mutable_exclusion_list()->add_patterns()->set_prefix( "cluster"); // No dot at the end means we have to compare as strings. - store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); + store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); uint64_t mem_consumed = memoryConsumedAddingClusterStats(); // Memory is consumed at all while rejecting stats from "prefix" in proportion @@ -1101,7 +1099,7 @@ class RememberStatsMatcherTest : public testing::TestWithParam { LookupStatFn lookupHistogramFn() { return [this](const std::string& stat_name) -> std::string { - return scope_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified).name(); + return scope_->histogramFromString(stat_name, Histogram::Unit::Unspecified).name(); }; } @@ -1164,7 +1162,7 @@ TEST_F(StatsThreadLocalStoreTest, RemoveRejectedStats) { store_->initializeThreading(main_thread_dispatcher_, tls_); Counter& counter = store_->counterFromString("c1"); Gauge& gauge = store_->gaugeFromString("g1", Gauge::ImportMode::Accumulate); - Histogram& histogram = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); + Histogram& histogram = store_->histogramFromString("h1", Histogram::Unit::Unspecified); TextReadout& textReadout = store_->textReadoutFromString("t1"); ASSERT_EQ(1, store_->counters().size()); // "c1". EXPECT_TRUE(&counter == store_->counters()[0].get() || @@ -1346,7 +1344,7 @@ TEST_F(StatsThreadLocalStoreTest, MergeDuringShutDown) { InSequence s; store_->initializeThreading(main_thread_dispatcher_, tls_); - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 1)); @@ -1382,7 +1380,7 @@ TEST(ThreadLocalStoreThreadTest, ConstructDestruct) { // Histogram tests TEST_F(HistogramTest, BasicSingleHistogramMerge) { - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); expectCallAndAccumulate(h1, 0); @@ -1398,8 +1396,8 @@ TEST_F(HistogramTest, BasicSingleHistogramMerge) { } TEST_F(HistogramTest, BasicMultiHistogramMerge) { - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); - Histogram& h2 = store_->histogramFromString("h2", Stats::Histogram::Unit::Unspecified); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); + Histogram& h2 = store_->histogramFromString("h2", Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("h2", h2.name()); @@ -1411,8 +1409,8 @@ TEST_F(HistogramTest, BasicMultiHistogramMerge) { } TEST_F(HistogramTest, MultiHistogramMultipleMerges) { - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); - Histogram& h2 = store_->histogramFromString("h2", Stats::Histogram::Unit::Unspecified); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); + Histogram& h2 = store_->histogramFromString("h2", Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("h2", h2.name()); @@ -1442,8 +1440,8 @@ TEST_F(HistogramTest, MultiHistogramMultipleMerges) { TEST_F(HistogramTest, BasicScopeHistogramMerge) { ScopePtr scope1 = store_->createScope("scope1."); - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); - Histogram& h2 = scope1->histogramFromString("h2", Stats::Histogram::Unit::Unspecified); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); + Histogram& h2 = scope1->histogramFromString("h2", Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("scope1.h2", h2.name()); @@ -1453,8 +1451,8 @@ TEST_F(HistogramTest, BasicScopeHistogramMerge) { } TEST_F(HistogramTest, BasicHistogramSummaryValidate) { - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); - Histogram& h2 = store_->histogramFromString("h2", Stats::Histogram::Unit::Unspecified); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); + Histogram& h2 = store_->histogramFromString("h2", Histogram::Unit::Unspecified); expectCallAndAccumulate(h1, 1); @@ -1493,7 +1491,7 @@ TEST_F(HistogramTest, BasicHistogramSummaryValidate) { // Validates the summary after known value merge in to same histogram. TEST_F(HistogramTest, BasicHistogramMergeSummary) { - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); for (size_t i = 0; i < 50; ++i) { expectCallAndAccumulate(h1, i); @@ -1521,8 +1519,8 @@ TEST_F(HistogramTest, BasicHistogramMergeSummary) { TEST_F(HistogramTest, BasicHistogramUsed) { ScopePtr scope1 = store_->createScope("scope1."); - Histogram& h1 = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); - Histogram& h2 = scope1->histogramFromString("h2", Stats::Histogram::Unit::Unspecified); + Histogram& h1 = store_->histogramFromString("h1", Histogram::Unit::Unspecified); + Histogram& h2 = scope1->histogramFromString("h2", Histogram::Unit::Unspecified); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("scope1.h2", h2.name()); @@ -1551,8 +1549,7 @@ TEST_F(HistogramTest, BasicHistogramUsed) { TEST_F(HistogramTest, ParentHistogramBucketSummary) { ScopePtr scope1 = store_->createScope("scope1."); - Histogram& histogram = - store_->histogramFromString("histogram", Stats::Histogram::Unit::Unspecified); + Histogram& histogram = store_->histogramFromString("histogram", Histogram::Unit::Unspecified); store_->mergeHistograms([]() -> void {}); ASSERT_EQ(1, store_->histograms().size()); ParentHistogramSharedPtr parent_histogram = store_->histograms()[0]; @@ -1569,20 +1566,19 @@ TEST_F(HistogramTest, ParentHistogramBucketSummary) { } TEST_F(HistogramTest, ForEachHistogram) { - std::vector> histograms; + std::vector> histograms; const size_t num_stats = 11; for (size_t idx = 0; idx < num_stats; ++idx) { auto stat_name = absl::StrCat("histogram.", idx); - histograms.emplace_back( - store_->histogramFromString(stat_name, Stats::Histogram::Unit::Unspecified)); + histograms.emplace_back(store_->histogramFromString(stat_name, Histogram::Unit::Unspecified)); } EXPECT_EQ(histograms.size(), 11); size_t num_histograms = 0; size_t num_iterations = 0; store_->forEachHistogram([&num_histograms](std::size_t size) { num_histograms = size; }, - [&num_iterations](Stats::ParentHistogram&) { ++num_iterations; }); + [&num_iterations](ParentHistogram&) { ++num_iterations; }); EXPECT_EQ(num_histograms, 11); EXPECT_EQ(num_iterations, 11); @@ -1595,35 +1591,35 @@ TEST_F(HistogramTest, ForEachHistogram) { num_histograms = 0; num_iterations = 0; store_->forEachHistogram([&num_histograms](std::size_t size) { num_histograms = size; }, - [&num_iterations](Stats::ParentHistogram&) { ++num_iterations; }); + [&num_iterations](ParentHistogram&) { ++num_iterations; }); EXPECT_EQ(num_histograms, 0); EXPECT_EQ(num_iterations, 0); // Verify that we can access the local reference without a crash. - EXPECT_EQ(deleted_histogram.unit(), Stats::Histogram::Unit::Unspecified); + EXPECT_EQ(deleted_histogram.unit(), Histogram::Unit::Unspecified); } TEST_F(HistogramTest, ForEachSinkedHistogram) { StatNamePool pool(store_->symbolTable()); - std::unique_ptr sink_predicates = - std::make_unique(); - std::vector> sinked_histograms; - std::vector> unsinked_histograms; + std::unique_ptr sink_predicates = + std::make_unique(); + std::vector> sinked_histograms; + std::vector> unsinked_histograms; const size_t num_stats = 11; // Create some histograms before setting the predicates. for (size_t idx = 0; idx < num_stats / 2; ++idx) { auto name = absl::StrCat("histogram.", idx); - Stats::StatName stat_name = pool.add(name); + StatName stat_name = pool.add(name); // sink every 3rd stat if ((idx + 1) % 3 == 0) { sink_predicates->sinkedStatNames().insert(stat_name); sinked_histograms.emplace_back( - store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); + store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); } else { unsinked_histograms.emplace_back( - store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); + store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); } } @@ -1632,17 +1628,17 @@ TEST_F(HistogramTest, ForEachSinkedHistogram) { // Create some histograms after setting the predicates. for (size_t idx = num_stats / 2; idx < num_stats; ++idx) { auto name = absl::StrCat("histogram.", idx); - Stats::StatName stat_name = pool.add(name); + StatName stat_name = pool.add(name); // sink every 3rd stat if ((idx + 1) % 3 == 0) { - static_cast(store_->sinkPredicates().ptr()) + static_cast(store_->sinkPredicates().ptr()) ->sinkedStatNames() .insert(stat_name); sinked_histograms.emplace_back( - store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); + store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); } else { unsinked_histograms.emplace_back( - store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); + store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); } } @@ -1653,8 +1649,8 @@ TEST_F(HistogramTest, ForEachSinkedHistogram) { size_t num_iterations = 0; store_->forEachSinkedHistogram( [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, - [&num_iterations, sink_predicates = static_cast( - store_->sinkPredicates().ptr())](Stats::ParentHistogram& histogram) { + [&num_iterations, sink_predicates = static_cast( + store_->sinkPredicates().ptr())](ParentHistogram& histogram) { EXPECT_NE(sink_predicates->sinkedStatNames().find(histogram.statName()), sink_predicates->sinkedStatNames().end()); ++num_iterations; @@ -1670,7 +1666,7 @@ TEST_F(HistogramTest, ForEachSinkedHistogram) { num_iterations = 0; store_->forEachSinkedHistogram( [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, - [&num_iterations](Stats::ParentHistogram&) { ++num_iterations; }); + [&num_iterations](ParentHistogram&) { ++num_iterations; }); EXPECT_EQ(num_sinked_histograms, 0); EXPECT_EQ(num_iterations, 0); } @@ -1845,8 +1841,7 @@ class HistogramThreadTest : public ThreadLocalRealThreadsTestBase { TEST_F(HistogramThreadTest, MakeHistogramsAndRecordValues) { foreachThread([this]() { - Histogram& histogram = - store_->histogramFromString("my_hist", Stats::Histogram::Unit::Unspecified); + Histogram& histogram = store_->histogramFromString("my_hist", Histogram::Unit::Unspecified); histogram.recordValue(42); }); @@ -1928,17 +1923,17 @@ TEST_F(HistogramThreadTest, ScopeOverlap) { // to mergeHistograms TEST_F(HistogramTest, UnsinkedHistogramsAreMerged) { StatNamePool pool(store_->symbolTable()); - store_->setSinkPredicates(std::make_unique()); + store_->setSinkPredicates(std::make_unique()); auto& sink_predicates = - static_cast(store_->sinkPredicates().ref()); - Stats::StatName stat_name = pool.add("h1"); + static_cast(store_->sinkPredicates().ref()); + StatName stat_name = pool.add("h1"); sink_predicates.sinkedStatNames().insert(stat_name); auto& h1 = static_cast( - store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); + store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); stat_name = pool.add("h2"); auto& h2 = static_cast( - store_->histogramFromStatName(stat_name, Stats::Histogram::Unit::Unspecified)); + store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); EXPECT_EQ("h1", h1.name()); EXPECT_EQ("h2", h2.name()); @@ -1956,7 +1951,7 @@ TEST_F(HistogramTest, UnsinkedHistogramsAreMerged) { size_t num_sinked_histograms = 0; store_->forEachSinkedHistogram( [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, - [&num_iterations, &sink_predicates](Stats::ParentHistogram& histogram) { + [&num_iterations, &sink_predicates](ParentHistogram& histogram) { EXPECT_NE(sink_predicates.sinkedStatNames().find(histogram.statName()), sink_predicates.sinkedStatNames().end()); ++num_iterations; From 6e6635ab335cef41ac34d3214304e1522e97611d Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Wed, 26 Jan 2022 14:30:34 +0000 Subject: [PATCH 11/12] Add for each iteration for histograms. Signed-off-by: Pradeep Rao --- envoy/server/instance.h | 2 +- envoy/stats/allocator.h | 2 +- envoy/stats/sink.h | 5 - envoy/stats/store.h | 6 +- source/common/stats/isolated_store_impl.h | 5 - source/common/stats/thread_local_store.cc | 31 ----- source/common/stats/thread_local_store.h | 3 - source/server/server.cc | 2 +- test/common/stats/allocator_impl_test.cc | 39 ++++-- test/common/stats/stat_test_utility.h | 23 ---- test/common/stats/thread_local_store_test.cc | 118 ------------------ test/integration/server.h | 6 - test/mocks/stats/mocks.h | 2 - .../server_stats_flush_benchmark_test.cc | 2 - test/server/server_test.cc | 2 +- 15 files changed, 34 insertions(+), 214 deletions(-) diff --git a/envoy/server/instance.h b/envoy/server/instance.h index 4d590ec6b421c..f779abb42a173 100644 --- a/envoy/server/instance.h +++ b/envoy/server/instance.h @@ -277,7 +277,7 @@ class Instance { virtual bool enableReusePortDefault() PURE; /** - * Set predicates for filtering stats to be flushed to sinks. + * Set predicates for filtering counters, gauges and text readouts to be flushed to sinks. */ virtual void setSinkPredicates(std::unique_ptr&& sink_predicates) PURE; diff --git a/envoy/stats/allocator.h b/envoy/stats/allocator.h index ae0a075116339..223b1cab47068 100644 --- a/envoy/stats/allocator.h +++ b/envoy/stats/allocator.h @@ -96,7 +96,7 @@ class Allocator { virtual void forEachSinkedTextReadout(SizeFn f_size, StatFn f_stat) const PURE; /** - * Set the predicates to filter stats for sink. + * Set the predicates to filter counters, gauges and text readouts for sink. */ virtual void setSinkPredicates(std::unique_ptr&& sink_predicates) PURE; diff --git a/envoy/stats/sink.h b/envoy/stats/sink.h index 2f8b572cbd3c0..a0d914416bd9d 100644 --- a/envoy/stats/sink.h +++ b/envoy/stats/sink.h @@ -69,11 +69,6 @@ class SinkPredicates { * @return true if @param text_readout needs to be flushed to sinks. */ virtual bool includeTextReadout(const TextReadout& text_readout) PURE; - - /* - * @return true if @param histogram needs to be flushed to sinks. - */ - virtual bool includeHistogram(const Histogram& histogram) PURE; }; /** diff --git a/envoy/stats/store.h b/envoy/stats/store.h index a9fd63bc0609f..a7be0c6d57738 100644 --- a/envoy/stats/store.h +++ b/envoy/stats/store.h @@ -4,7 +4,6 @@ #include #include -#include "envoy/common/optref.h" #include "envoy/common/pure.h" #include "envoy/stats/histogram.h" #include "envoy/stats/scope.h" @@ -82,7 +81,6 @@ class Store : public Scope { virtual void forEachSinkedCounter(SizeFn f_size, StatFn f_stat) const PURE; virtual void forEachSinkedGauge(SizeFn f_size, StatFn f_stat) const PURE; virtual void forEachSinkedTextReadout(SizeFn f_size, StatFn f_stat) const PURE; - virtual void forEachSinkedHistogram(SizeFn f_size, StatFn f_stat) const PURE; }; using StorePtr = std::unique_ptr; @@ -143,14 +141,12 @@ class StoreRoot : public Store { virtual void mergeHistograms(PostMergeCb merge_complete_cb) PURE; /** - * Set predicates for filtering stats to be flushed to sinks. + * Set predicates for filtering counters, gauges and text readouts to be flushed to sinks. * Note that if the sink predicates object is set, we do not send non-sink stats over to the * child process during hot restart. This will result in the admin stats console being wrong * during hot restart. */ virtual void setSinkPredicates(std::unique_ptr&& sink_predicates) PURE; - - virtual OptRef sinkPredicates() PURE; }; using StoreRootPtr = std::unique_ptr; diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index 1f41a8a7f3069..879648189977d 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -254,11 +254,6 @@ class IsolatedStoreImpl : public StoreImpl { forEachTextReadout(f_size, f_stat); } - void forEachSinkedHistogram(SizeFn f_size, StatFn f_stat) const override { - UNREFERENCED_PARAMETER(f_size); - UNREFERENCED_PARAMETER(f_stat); - } - private: IsolatedStoreImpl(std::unique_ptr&& symbol_table); diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index bc11c7993ce84..b0825f2dee89b 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -90,7 +90,6 @@ void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) { for (uint32_t i = first_histogram_index; i < deleted_histograms_.size(); ++i) { uint32_t erased = histogram_set_.erase(deleted_histograms_[i].get()); ASSERT(erased == 1); - sinked_histograms_.erase(deleted_histograms_[i].get()); } } } @@ -216,7 +215,6 @@ void ThreadLocalStoreImpl::shutdownThreading() { histogram->setShuttingDown(true); } histogram_set_.clear(); - sinked_histograms_.clear(); } void ThreadLocalStoreImpl::mergeHistograms(PostMergeCb merge_complete_cb) { @@ -675,10 +673,6 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( *buckets, parent_.next_histogram_id_++); if (!parent_.shutting_down_) { parent_.histogram_set_.insert(stat.get()); - if (parent_.sink_predicates_.has_value() && - parent_.sink_predicates_->includeHistogram(*stat)) { - parent_.sinked_histograms_.insert(stat.get()); - } } } } @@ -868,7 +862,6 @@ bool ThreadLocalStoreImpl::decHistogramRefCount(ParentHistogramImpl& hist, if (!shutting_down_) { const size_t count = histogram_set_.erase(hist.statName()); ASSERT(shutting_down_ || count == 1); - sinked_histograms_.erase(&hist); } return true; } @@ -1003,35 +996,11 @@ void ThreadLocalStoreImpl::forEachSinkedTextReadout(SizeFn f_size, alloc_.forEachSinkedTextReadout(f_size, f_stat); } -void ThreadLocalStoreImpl::forEachSinkedHistogram(SizeFn f_size, - StatFn f_stat) const { - if (sink_predicates_.has_value()) { - Thread::LockGuard lock(hist_mutex_); - - if (f_size != nullptr) { - f_size(sinked_histograms_.size()); - } - for (auto histogram : sinked_histograms_) { - f_stat(*histogram); - } - } else { - forEachHistogram(f_size, f_stat); - } -} - void ThreadLocalStoreImpl::setSinkPredicates(std::unique_ptr&& sink_predicates) { ASSERT(sink_predicates != nullptr); if (sink_predicates != nullptr) { sink_predicates_.emplace(*sink_predicates); alloc_.setSinkPredicates(std::move(sink_predicates)); - // Add histograms to the set of sinked histograms. - Thread::LockGuard lock(hist_mutex_); - sinked_histograms_.clear(); - for (auto& histogram : histogram_set_) { - if (sink_predicates_->includeHistogram(*histogram)) { - sinked_histograms_.insert(histogram); - } - } } } diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 3dc2f580e9af7..e7e973cd6acc7 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -267,10 +267,8 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo void forEachSinkedCounter(SizeFn f_size, StatFn f_stat) const override; void forEachSinkedGauge(SizeFn f_size, StatFn f_stat) const override; void forEachSinkedTextReadout(SizeFn f_size, StatFn f_stat) const override; - void forEachSinkedHistogram(SizeFn f_size, StatFn f_stat) const override; void setSinkPredicates(std::unique_ptr&& sink_predicates) override; - OptRef sinkPredicates() override { return sink_predicates_; } /** * @return a thread synchronizer object used for controlling thread behavior in tests. @@ -546,7 +544,6 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo mutable Thread::MutexBasicLockable hist_mutex_; StatSet histogram_set_ ABSL_GUARDED_BY(hist_mutex_); - StatSet sinked_histograms_ ABSL_GUARDED_BY(hist_mutex_); // Retain storage for deleted stats; these are no longer in maps because the // matcher-pattern was established after they were created. Since the stats diff --git a/source/server/server.cc b/source/server/server.cc index 61346636f4c70..57aeb9b225fbe 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -200,7 +200,7 @@ MetricSnapshotImpl::MetricSnapshotImpl(Stats::Store& store, TimeSource& time_sou gauges_.push_back(gauge); }); - store.forEachSinkedHistogram( + store.forEachHistogram( [this](std::size_t size) { snapped_histograms_.reserve(size); histograms_.reserve(size); diff --git a/test/common/stats/allocator_impl_test.cc b/test/common/stats/allocator_impl_test.cc index 859cc0c6d3618..a4c0eb2d271b4 100644 --- a/test/common/stats/allocator_impl_test.cc +++ b/test/common/stats/allocator_impl_test.cc @@ -6,7 +6,6 @@ #include "source/common/stats/allocator_impl.h" -#include "test/common/stats/stat_test_utility.h" #include "test/test_common/logging.h" #include "test/test_common/thread_factory_for_test.h" @@ -45,6 +44,26 @@ class AllocatorImplTest : public testing::Test { bool are_stats_marked_for_deletion_ = false; }; +class TestSinkPredicates : public SinkPredicates { +public: + ~TestSinkPredicates() override = default; + StatNameHashSet& sinkedStatNames() { return sinked_stat_names_; } + + // SinkPredicates + bool includeCounter(const Counter& counter) override { + return sinked_stat_names_.find(counter.statName()) != sinked_stat_names_.end(); + } + bool includeGauge(const Gauge& gauge) override { + return sinked_stat_names_.find(gauge.statName()) != sinked_stat_names_.end(); + } + bool includeTextReadout(const TextReadout& text_readout) override { + return sinked_stat_names_.find(text_readout.statName()) != sinked_stat_names_.end(); + } + +private: + StatNameHashSet sinked_stat_names_; +}; + // Allocate 2 counters of the same name, and you'll get the same object. TEST_F(AllocatorImplTest, CountersWithSameName) { StatName counter_name = makeStat("counter.name"); @@ -415,9 +434,9 @@ TEST_F(AllocatorImplTest, AskForDeletedStat) { } TEST_F(AllocatorImplTest, ForEachSinkedCounter) { - std::unique_ptr moved_sink_predicates = - std::make_unique(); - Stats::TestUtil::TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); + std::unique_ptr moved_sink_predicates = + std::make_unique(); + TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); std::vector sinked_counters; std::vector unsinked_counters; @@ -461,9 +480,9 @@ TEST_F(AllocatorImplTest, ForEachSinkedCounter) { } TEST_F(AllocatorImplTest, ForEachSinkedGauge) { - std::unique_ptr moved_sink_predicates = - std::make_unique(); - Stats::TestUtil::TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); + std::unique_ptr moved_sink_predicates = + std::make_unique(); + TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); std::vector sinked_gauges; std::vector unsinked_gauges; @@ -507,9 +526,9 @@ TEST_F(AllocatorImplTest, ForEachSinkedGauge) { } TEST_F(AllocatorImplTest, ForEachSinkedTextReadout) { - std::unique_ptr moved_sink_predicates = - std::make_unique(); - Stats::TestUtil::TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); + std::unique_ptr moved_sink_predicates = + std::make_unique(); + TestSinkPredicates* sink_predicates = moved_sink_predicates.get(); std::vector sinked_text_readouts; std::vector unsinked_text_readouts; diff --git a/test/common/stats/stat_test_utility.h b/test/common/stats/stat_test_utility.h index f610689845a46..875dcf3b5a2fb 100644 --- a/test/common/stats/stat_test_utility.h +++ b/test/common/stats/stat_test_utility.h @@ -219,29 +219,6 @@ std::vector serializeDeserializeNumber(uint64_t number); // Serializes a string into a MemBlock and then decodes it. void serializeDeserializeString(absl::string_view in); -class TestSinkPredicates : public SinkPredicates { -public: - ~TestSinkPredicates() override = default; - StatNameHashSet& sinkedStatNames() { return sinked_stat_names_; } - - // SinkPredicates - bool includeCounter(const Counter& counter) override { - return sinked_stat_names_.find(counter.statName()) != sinked_stat_names_.end(); - } - bool includeGauge(const Gauge& gauge) override { - return sinked_stat_names_.find(gauge.statName()) != sinked_stat_names_.end(); - } - bool includeTextReadout(const TextReadout& text_readout) override { - return sinked_stat_names_.find(text_readout.statName()) != sinked_stat_names_.end(); - } - bool includeHistogram(const Histogram& histogram) override { - return sinked_stat_names_.find(histogram.statName()) != sinked_stat_names_.end(); - } - -private: - StatNameHashSet sinked_stat_names_; -}; - } // namespace TestUtil } // namespace Stats } // namespace Envoy diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index a85fcc83e7008..edb0faa12d40f 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -1656,78 +1656,6 @@ TEST_F(HistogramTest, ForEachHistogram) { EXPECT_EQ(deleted_histogram.unit(), Histogram::Unit::Unspecified); } -TEST_F(HistogramTest, ForEachSinkedHistogram) { - StatNamePool pool(store_->symbolTable()); - - std::unique_ptr sink_predicates = - std::make_unique(); - std::vector> sinked_histograms; - std::vector> unsinked_histograms; - - const size_t num_stats = 11; - // Create some histograms before setting the predicates. - for (size_t idx = 0; idx < num_stats / 2; ++idx) { - auto name = absl::StrCat("histogram.", idx); - StatName stat_name = pool.add(name); - // sink every 3rd stat - if ((idx + 1) % 3 == 0) { - sink_predicates->sinkedStatNames().insert(stat_name); - sinked_histograms.emplace_back( - store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); - } else { - unsinked_histograms.emplace_back( - store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); - } - } - - store_->setSinkPredicates(std::move(sink_predicates)); - - // Create some histograms after setting the predicates. - for (size_t idx = num_stats / 2; idx < num_stats; ++idx) { - auto name = absl::StrCat("histogram.", idx); - StatName stat_name = pool.add(name); - // sink every 3rd stat - if ((idx + 1) % 3 == 0) { - static_cast(store_->sinkPredicates().ptr()) - ->sinkedStatNames() - .insert(stat_name); - sinked_histograms.emplace_back( - store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); - } else { - unsinked_histograms.emplace_back( - store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); - } - } - - EXPECT_EQ(sinked_histograms.size(), 3); - EXPECT_EQ(unsinked_histograms.size(), 8); - - size_t num_sinked_histograms = 0; - size_t num_iterations = 0; - store_->forEachSinkedHistogram( - [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, - [&num_iterations, sink_predicates = static_cast( - store_->sinkPredicates().ptr())](ParentHistogram& histogram) { - EXPECT_NE(sink_predicates->sinkedStatNames().find(histogram.statName()), - sink_predicates->sinkedStatNames().end()); - ++num_iterations; - }); - EXPECT_EQ(num_sinked_histograms, 3); - EXPECT_EQ(num_iterations, 3); - - // Verify that rejecting histograms removes them from the sink set. - envoy::config::metrics::v3::StatsConfig stats_config_; - stats_config_.mutable_stats_matcher()->set_reject_all(true); - store_->setStatsMatcher(std::make_unique(stats_config_, symbol_table_)); - num_sinked_histograms = 0; - num_iterations = 0; - store_->forEachSinkedHistogram( - [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, - [&num_iterations](ParentHistogram&) { ++num_iterations; }); - EXPECT_EQ(num_sinked_histograms, 0); - EXPECT_EQ(num_iterations, 0); -} - class ThreadLocalRealThreadsTestBase : public Thread::RealThreadsTestHelper, public ThreadLocalStoreNoMocksTestBase { protected: @@ -1976,51 +1904,5 @@ TEST_F(HistogramThreadTest, ScopeOverlap) { store_->histogramFromString("histogram_after_shutdown", Histogram::Unit::Unspecified); } -// Verify that histograms that are not flushed to sinks are merged in the call -// to mergeHistograms -TEST_F(HistogramTest, UnsinkedHistogramsAreMerged) { - StatNamePool pool(store_->symbolTable()); - store_->setSinkPredicates(std::make_unique()); - auto& sink_predicates = - static_cast(store_->sinkPredicates().ref()); - StatName stat_name = pool.add("h1"); - sink_predicates.sinkedStatNames().insert(stat_name); - - auto& h1 = static_cast( - store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); - stat_name = pool.add("h2"); - auto& h2 = static_cast( - store_->histogramFromStatName(stat_name, Histogram::Unit::Unspecified)); - - EXPECT_EQ("h1", h1.name()); - EXPECT_EQ("h2", h2.name()); - EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 5)); - EXPECT_CALL(sink_, onHistogramComplete(Ref(h2), 5)); - - h1.recordValue(5); - h2.recordValue(5); - - EXPECT_THAT(h1.cumulativeStatistics().bucketSummary(), HasSubstr(" B10: 0,")); - EXPECT_THAT(h2.cumulativeStatistics().bucketSummary(), HasSubstr(" B10: 0,")); - - store_->mergeHistograms([this, &sink_predicates]() -> void { - size_t num_iterations = 0; - size_t num_sinked_histograms = 0; - store_->forEachSinkedHistogram( - [&num_sinked_histograms](std::size_t size) { num_sinked_histograms = size; }, - [&num_iterations, &sink_predicates](ParentHistogram& histogram) { - EXPECT_NE(sink_predicates.sinkedStatNames().find(histogram.statName()), - sink_predicates.sinkedStatNames().end()); - ++num_iterations; - }); - EXPECT_EQ(num_sinked_histograms, 1); - EXPECT_EQ(num_iterations, 1); - }); - - EXPECT_THAT(h1.cumulativeStatistics().bucketSummary(), HasSubstr(" B10: 1,")); - EXPECT_THAT(h2.cumulativeStatistics().bucketSummary(), HasSubstr(" B10: 1,")); - EXPECT_EQ(h1.cumulativeStatistics().bucketSummary(), h2.cumulativeStatistics().bucketSummary()); -} - } // namespace Stats } // namespace Envoy diff --git a/test/integration/server.h b/test/integration/server.h index 675318e62d3c9..6c228624a3206 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -316,16 +316,10 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); store_.forEachSinkedTextReadout(f_size, f_stat); } - void forEachSinkedHistogram(Stats::SizeFn f_size, StatFn f_stat) const override { - Thread::LockGuard lock(lock_); - store_.forEachSinkedHistogram(f_size, f_stat); - } void setSinkPredicates(std::unique_ptr&& sink_predicates) override { UNREFERENCED_PARAMETER(sink_predicates); } - OptRef sinkPredicates() override { return OptRef{}; } - Counter& counterFromString(const std::string& name) override { Thread::LockGuard lock(lock_); return store_.counterFromString(name); diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 52fe586fb4a03..42837995662a5 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -271,7 +271,6 @@ class MockSinkPredicates : public SinkPredicates { MOCK_METHOD(bool, includeCounter, (const Counter&)); MOCK_METHOD(bool, includeGauge, (const Gauge&)); MOCK_METHOD(bool, includeTextReadout, (const TextReadout&)); - MOCK_METHOD(bool, includeHistogram, (const Histogram&)); }; class MockStore : public TestUtil::TestStore { @@ -300,7 +299,6 @@ class MockStore : public TestUtil::TestStore { MOCK_METHOD(void, forEachGauge, (SizeFn, StatFn), (const)); MOCK_METHOD(void, forEachTextReadout, (SizeFn, StatFn), (const)); MOCK_METHOD(void, forEachHistogram, (SizeFn, StatFn), (const)); - MOCK_METHOD(void, forEachSinkedHistogram, (SizeFn, StatFn), (const)); MOCK_METHOD(CounterOptConstRef, findCounter, (StatName), (const)); MOCK_METHOD(GaugeOptConstRef, findGauge, (StatName), (const)); diff --git a/test/server/server_stats_flush_benchmark_test.cc b/test/server/server_stats_flush_benchmark_test.cc index 98b8b446c41e0..895967523c624 100644 --- a/test/server/server_stats_flush_benchmark_test.cc +++ b/test/server/server_stats_flush_benchmark_test.cc @@ -26,13 +26,11 @@ class TestSinkPredicates : public Stats::SinkPredicates { bool includeTextReadout(const Stats::TextReadout&) override { return (++num_text_readouts_) % 10 == 0; } - bool includeHistogram(const Stats::Histogram&) override { return (++num_histograms_) % 10 == 0; } private: size_t num_counters_ = 0; size_t num_gauges_ = 0; size_t num_text_readouts_ = 0; - size_t num_histograms_ = 0; }; class StatsSinkFlushSpeedTest { diff --git a/test/server/server_test.cc b/test/server/server_test.cc index ff273272660aa..5230712b2ec54 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -91,7 +91,7 @@ TEST(ServerInstanceUtil, flushHelper) { NiceMock mock_store; Stats::ParentHistogramSharedPtr parent_histogram(new Stats::MockParentHistogram()); std::vector parent_histograms = {parent_histogram}; - ON_CALL(mock_store, forEachSinkedHistogram) + ON_CALL(mock_store, forEachHistogram) .WillByDefault([&](std::function f_size, std::function f_stat) { if (f_size != nullptr) { From d1dda13c3706cbdd318706cfece9334a8b1072a4 Mon Sep 17 00:00:00 2001 From: Pradeep Rao Date: Tue, 1 Feb 2022 20:52:34 +0000 Subject: [PATCH 12/12] Address feedback. Signed-off-by: Pradeep Rao --- source/common/stats/thread_local_store.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index b0825f2dee89b..6bc37b85f3bc0 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -966,7 +966,7 @@ void ThreadLocalStoreImpl::forEachHistogram(SizeFn f_size, StatFn