From 9f37764f5851b54c658a6b5b972df0aae3f8239f Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Fri, 3 Apr 2020 10:10:32 -0400 Subject: [PATCH 01/16] stats: add new TextReadout stat type Based on https://github.com/envoyproxy/envoy/pull/5844 by @fredlas. Signed-off-by: Misha Efimov <mef@google.com> --- include/envoy/stats/allocator.h | 9 ++ include/envoy/stats/scope.h | 33 +++++ include/envoy/stats/stats.h | 13 ++ include/envoy/stats/stats_macros.h | 3 + include/envoy/stats/store.h | 5 + source/common/stats/BUILD | 12 ++ source/common/stats/allocator_impl.cc | 32 +++++ source/common/stats/allocator_impl.h | 5 + source/common/stats/isolated_store_impl.cc | 3 + source/common/stats/isolated_store_impl.h | 32 +++++ source/common/stats/null_text_readout.h | 44 ++++++ source/common/stats/scope_prefixer.cc | 11 ++ source/common/stats/scope_prefixer.h | 7 + source/common/stats/thread_local_store.cc | 61 +++++++- source/common/stats/thread_local_store.h | 43 +++++- test/common/stats/isolated_store_impl_test.cc | 29 +++- test/common/stats/thread_local_store_test.cc | 135 +++++++++++++++++- test/integration/server.h | 33 ++++- test/mocks/stats/mocks.cc | 6 + test/mocks/stats/mocks.h | 22 +++ test/test_common/utility.cc | 5 + test/test_common/utility.h | 12 +- 22 files changed, 542 insertions(+), 13 deletions(-) create mode 100644 source/common/stats/null_text_readout.h diff --git a/include/envoy/stats/allocator.h b/include/envoy/stats/allocator.h index 681befa5f3b3..646b47c29b41 100644 --- a/include/envoy/stats/allocator.h +++ b/include/envoy/stats/allocator.h @@ -49,6 +49,15 @@ class Allocator { const StatNameTagVector& stat_name_tags, Gauge::ImportMode import_mode) PURE; + /** + * @param name the full name of the stat. + * @param tag_extracted_name the name of the stat with tag-values stripped out. + * @param tags the tag values. + * @return TextReadoutSharedPtr a text readout, or nullptr if allocation failed, in which case + * tag_extracted_name and tags are not moved. + */ + virtual TextReadoutSharedPtr makeTextReadout(StatName name, StatName tag_extracted_name, + const StatNameTagVector& stat_name_tags) PURE; virtual const SymbolTable& constSymbolTable() const PURE; virtual SymbolTable& symbolTable() PURE; diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index 03a232063c13..0ac873cc6981 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -17,12 +17,14 @@ namespace Stats { class Counter; class Gauge; class Histogram; +class TextReadout; class Scope; class NullGaugeImpl; using CounterOptConstRef = absl::optional<std::reference_wrapper<const Counter>>; using GaugeOptConstRef = absl::optional<std::reference_wrapper<const Gauge>>; using HistogramOptConstRef = absl::optional<std::reference_wrapper<const Histogram>>; +using TextReadoutOptConstRef = absl::optional<std::reference_wrapper<const TextReadout>>; using ScopePtr = std::unique_ptr<Scope>; using ScopeSharedPtr = std::shared_ptr<Scope>; @@ -136,6 +138,31 @@ class Scope { */ virtual Histogram& histogramFromString(const std::string& name, Histogram::Unit unit) PURE; + /** + * Creates a TextReadout from the stat name. Tag extraction will be performed on the name. + * @param name The name of the stat, obtained from the SymbolTable. + * @return a text readout within the scope's namespace. + */ + TextReadout& textReadoutFromStatName(const StatName& name) { + return textReadoutFromStatNameWithTags(name, absl::nullopt); + } + /** + * Creates a TextReadout from the stat name and tags. If tags are not provided, tag extraction + * will be performed on the name. + * @param name The name of the stat, obtained from the SymbolTable. + * @param tags optionally specified tags. + * @return a text readout within the scope's namespace. + */ + virtual TextReadout& textReadoutFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptConstRef tags) PURE; + + /**v + * TODO(#6667): this variant is deprecated: use textReadoutFromStatName. + * @param name The name, expressed as a string. + * @return a text readout within the scope's namespace. + */ + virtual TextReadout& textReadoutFromString(const std::string& name) PURE; + /** * @param The name of the stat, obtained from the SymbolTable. * @return a reference to a counter within the scope's namespace, if it exists. @@ -155,6 +182,12 @@ class Scope { */ virtual HistogramOptConstRef findHistogram(StatName name) const PURE; + /** + * @param The name of the stat, obtained from the SymbolTable. + * @return a reference to a text readout within the scope's namespace, if it exists. + */ + virtual TextReadoutOptConstRef findTextReadout(StatName name) const PURE; + /** * @return a reference to the symbol table. */ diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index b1772fb63212..32e0a18006b4 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -156,5 +156,18 @@ class Gauge : public Metric { using GaugeSharedPtr = RefcountPtr<Gauge>; +/** + * A string, possibly non-ASCII. + */ +class TextReadout : public virtual Metric { +public: + virtual ~TextReadout() {} + + virtual void set(const std::string& value) PURE; + virtual std::string value() const PURE; +}; + +using TextReadoutSharedPtr = RefcountPtr<TextReadout>; + } // namespace Stats } // namespace Envoy diff --git a/include/envoy/stats/stats_macros.h b/include/envoy/stats/stats_macros.h index 77d2243903f4..a37c5566e89c 100644 --- a/include/envoy/stats/stats_macros.h +++ b/include/envoy/stats/stats_macros.h @@ -39,6 +39,7 @@ namespace Envoy { #define GENERATE_COUNTER_STRUCT(NAME) Envoy::Stats::Counter& NAME##_; #define GENERATE_GAUGE_STRUCT(NAME, MODE) Envoy::Stats::Gauge& NAME##_; #define GENERATE_HISTOGRAM_STRUCT(NAME, UNIT) Envoy::Stats::Histogram& NAME##_; +#define GENERATE_TEXT_READOUT_STRUCT(NAME) Envoy::Stats::TextReadout& NAME##_; #define FINISH_STAT_DECL_(X) #X)), #define FINISH_STAT_DECL_MODE_(X, MODE) #X), Envoy::Stats::Gauge::ImportMode::MODE), @@ -57,10 +58,12 @@ static inline std::string statPrefixJoin(absl::string_view prefix, absl::string_ #define POOL_COUNTER_PREFIX(POOL, PREFIX) (POOL).counterFromString(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_ #define POOL_GAUGE_PREFIX(POOL, PREFIX) (POOL).gaugeFromString(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_MODE_ #define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogramFromString(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_UNIT_ +#define POOL_TEXT_READOUT_PREFIX(POOL, PREFIX) (POOL).textReadoutFromString(Envoy::statPrefixJoin(PREFIX, FINISH_STAT_DECL_ #define POOL_COUNTER(POOL) POOL_COUNTER_PREFIX(POOL, "") #define POOL_GAUGE(POOL) POOL_GAUGE_PREFIX(POOL, "") #define POOL_HISTOGRAM(POOL) POOL_HISTOGRAM_PREFIX(POOL, "") +#define POOL_TEXT_READOUT(POOL) POOL_TEXT_READOUT_PREFIX(POOL, "") #define NULL_STAT_DECL_(X) std::string(#X)), #define NULL_STAT_DECL_IGNORE_MODE_(X, MODE) std::string(#X)), diff --git a/include/envoy/stats/store.h b/include/envoy/stats/store.h index d959a58b01d9..158f00518a51 100644 --- a/include/envoy/stats/store.h +++ b/include/envoy/stats/store.h @@ -38,6 +38,11 @@ class Store : public Scope { */ virtual std::vector<GaugeSharedPtr> gauges() const PURE; + /** + * @return a list of all known text readouts. + */ + virtual std::vector<TextReadoutSharedPtr> textReadouts() const PURE; + /** * @return a list of all known histograms. */ diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index c720e2ef92a1..256074df9cbf 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -48,6 +48,7 @@ envoy_cc_library( ":histogram_lib", ":null_counter_lib", ":null_gauge_lib", + ":null_text_readout_lib", ":scope_prefixer_lib", ":stats_lib", ":store_impl_lib", @@ -100,6 +101,16 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "null_text_readout_lib", + hdrs = ["null_text_readout.h"], + deps = [ + ":metric_impl_lib", + ":symbol_table_lib", + "//include/envoy/stats:stats_interface", + ], +) + envoy_cc_library( name = "recent_lookups_lib", srcs = ["recent_lookups.cc"], @@ -240,6 +251,7 @@ envoy_cc_library( ":allocator_lib", ":null_counter_lib", ":null_gauge_lib", + ":null_text_readout_lib", ":scope_prefixer_lib", ":stats_lib", ":stats_matcher_lib", diff --git a/source/common/stats/allocator_impl.cc b/source/common/stats/allocator_impl.cc index 3286aeb7834e..62da300b788f 100644 --- a/source/common/stats/allocator_impl.cc +++ b/source/common/stats/allocator_impl.cc @@ -228,6 +228,25 @@ class GaugeImpl : public StatsSharedImpl<Gauge> { } }; +class TextReadoutImpl : public StatsSharedImpl<TextReadout> { +public: + TextReadoutImpl(StatName name, AllocatorImpl& alloc, StatName tag_extracted_name, + const StatNameTagVector& stat_name_tags) + : StatsSharedImpl(name, alloc, tag_extracted_name, stat_name_tags) {} + + void removeFromSetLockHeld() EXCLUSIVE_LOCKS_REQUIRED(alloc_.mutex_) override { + const size_t count = alloc_.text_readouts_.erase(statName()); + ASSERT(count == 1); + } + + // Stats::TextReadout + void set(const std::string& value) override { value_ = value; } + std::string value() const override { return value_; } + +private: + std::string value_; +}; + CounterSharedPtr AllocatorImpl::makeCounter(StatName name, StatName tag_extracted_name, const StatNameTagVector& stat_name_tags) { Thread::LockGuard lock(mutex_); @@ -256,6 +275,19 @@ GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, StatName tag_extracted_na return gauge; } +TextReadoutSharedPtr AllocatorImpl::makeTextReadout(StatName name, StatName tag_extracted_name, + const StatNameTagVector& stat_name_tags) { + Thread::LockGuard lock(mutex_); + ASSERT(gauges_.find(name) == gauges_.end()); + auto iter = text_readouts_.find(name); + if (iter != text_readouts_.end()) { + return TextReadoutSharedPtr(*iter); + } + auto text_readout = + TextReadoutSharedPtr(new TextReadoutImpl(name, *this, tag_extracted_name, stat_name_tags)); + text_readouts_.insert(text_readout.get()); + return text_readout; +} bool AllocatorImpl::isMutexLockedForTest() { bool locked = mutex_.tryLock(); if (locked) { diff --git a/source/common/stats/allocator_impl.h b/source/common/stats/allocator_impl.h index 2c4ba307f797..02e926529358 100644 --- a/source/common/stats/allocator_impl.h +++ b/source/common/stats/allocator_impl.h @@ -28,6 +28,8 @@ class AllocatorImpl : public Allocator { GaugeSharedPtr makeGauge(StatName name, StatName tag_extracted_name, const StatNameTagVector& stat_name_tags, Gauge::ImportMode import_mode) override; + TextReadoutSharedPtr makeTextReadout(StatName name, StatName tag_extracted_name, + const StatNameTagVector& stat_name_tags) override; SymbolTable& symbolTable() override { return symbol_table_; } const SymbolTable& constSymbolTable() const override { return symbol_table_; } @@ -49,6 +51,7 @@ class AllocatorImpl : public Allocator { template <class BaseClass> friend class StatsSharedImpl; friend class CounterImpl; friend class GaugeImpl; + friend class TextReadoutImpl; struct HeapStatHash { using is_transparent = void; // NOLINT(readability-identifier-naming) @@ -66,6 +69,7 @@ class AllocatorImpl : public Allocator { void removeCounterFromSetLockHeld(Counter* counter) EXCLUSIVE_LOCKS_REQUIRED(mutex_); void removeGaugeFromSetLockHeld(Gauge* gauge) EXCLUSIVE_LOCKS_REQUIRED(mutex_); + void removeTextReadoutFromSetLockHeld(Counter* counter) EXCLUSIVE_LOCKS_REQUIRED(mutex_); // An unordered set of HeapStatData pointers which keys off the key() // field in each object. This necessitates a custom comparator and hasher, which key off of the @@ -74,6 +78,7 @@ class AllocatorImpl : public Allocator { using StatSet = absl::flat_hash_set<StatType*, HeapStatHash, HeapStatCompare>; StatSet<Counter> counters_ GUARDED_BY(mutex_); StatSet<Gauge> gauges_ GUARDED_BY(mutex_); + StatSet<TextReadout> text_readouts_ GUARDED_BY(mutex_); SymbolTable& symbol_table_; diff --git a/source/common/stats/isolated_store_impl.cc b/source/common/stats/isolated_store_impl.cc index c3600c98d815..dceb080ba963 100644 --- a/source/common/stats/isolated_store_impl.cc +++ b/source/common/stats/isolated_store_impl.cc @@ -32,6 +32,9 @@ IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table) histograms_([this](StatName name, Histogram::Unit unit) -> HistogramSharedPtr { return HistogramSharedPtr(new HistogramImpl(name, unit, *this, name, StatNameTagVector{})); }), + text_readouts_([this](StatName name, bool) -> TextReadoutSharedPtr { + return alloc_.makeTextReadout(name, name, StatNameTagVector{}); + }), null_counter_(new NullCounterImpl(symbol_table)), null_gauge_(new NullGaugeImpl(symbol_table)) {} diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index 987617a9fe90..7a8e7e12f23f 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -29,11 +29,14 @@ template <class Base> class IsolatedStatsCache { using CounterAllocator = std::function<RefcountPtr<Base>(StatName name)>; using GaugeAllocator = std::function<RefcountPtr<Base>(StatName, Gauge::ImportMode)>; using HistogramAllocator = std::function<RefcountPtr<Base>(StatName, Histogram::Unit)>; + using TextReadoutAllocator = + std::function<RefcountPtr<Base>(StatName name, bool /* lowercase */)>; using BaseOptConstRef = absl::optional<std::reference_wrapper<const Base>>; IsolatedStatsCache(CounterAllocator alloc) : counter_alloc_(alloc) {} IsolatedStatsCache(GaugeAllocator alloc) : gauge_alloc_(alloc) {} IsolatedStatsCache(HistogramAllocator alloc) : histogram_alloc_(alloc) {} + IsolatedStatsCache(TextReadoutAllocator alloc) : text_readout_alloc_(alloc) {} Base& get(StatName name) { auto stat = stats_.find(name); @@ -68,6 +71,17 @@ template <class Base> class IsolatedStatsCache { return *new_stat; } + Base& get(StatName name, bool b) { + auto stat = stats_.find(name); + if (stat != stats_.end()) { + return *stat->second; + } + + RefcountPtr<Base> new_stat = text_readout_alloc_(name, b); + stats_.emplace(new_stat->statName(), new_stat); + return *new_stat; + } + std::vector<RefcountPtr<Base>> toVector() const { std::vector<RefcountPtr<Base>> vec; vec.reserve(stats_.size()); @@ -93,6 +107,7 @@ template <class Base> class IsolatedStatsCache { CounterAllocator counter_alloc_; GaugeAllocator gauge_alloc_; HistogramAllocator histogram_alloc_; + TextReadoutAllocator text_readout_alloc_; }; class IsolatedStoreImpl : public StoreImpl { @@ -124,11 +139,20 @@ class IsolatedStoreImpl : public StoreImpl { Histogram& histogram = histograms_.get(joiner.nameWithTags(), unit); return histogram; } + TextReadout& textReadoutFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptConstRef tags) override { + TagUtility::TagStatNameJoiner joiner(name, tags, symbolTable()); + TextReadout& text_readout = text_readouts_.get(joiner.nameWithTags(), false); + return text_readout; + } CounterOptConstRef findCounter(StatName name) const override { return counters_.find(name); } GaugeOptConstRef findGauge(StatName name) const override { return gauges_.find(name); } HistogramOptConstRef findHistogram(StatName name) const override { return histograms_.find(name); } + TextReadoutOptConstRef findTextReadout(StatName name) const override { + return text_readouts_.find(name); + } // Stats::Store std::vector<CounterSharedPtr> counters() const override { return counters_.toVector(); } @@ -143,6 +167,9 @@ class IsolatedStoreImpl : public StoreImpl { std::vector<ParentHistogramSharedPtr> histograms() const override { return std::vector<ParentHistogramSharedPtr>{}; } + std::vector<TextReadoutSharedPtr> textReadouts() const override { + return text_readouts_.toVector(); + } Counter& counterFromString(const std::string& name) override { StatNameManagedStorage storage(name, symbolTable()); @@ -156,6 +183,10 @@ class IsolatedStoreImpl : public StoreImpl { StatNameManagedStorage storage(name, symbolTable()); return histogramFromStatName(storage.statName(), unit); } + TextReadout& textReadoutFromString(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return textReadoutFromStatName(storage.statName()); + } private: IsolatedStoreImpl(std::unique_ptr<SymbolTable>&& symbol_table); @@ -165,6 +196,7 @@ class IsolatedStoreImpl : public StoreImpl { IsolatedStatsCache<Counter> counters_; IsolatedStatsCache<Gauge> gauges_; IsolatedStatsCache<Histogram> histograms_; + IsolatedStatsCache<TextReadout> text_readouts_; RefcountPtr<NullCounterImpl> null_counter_; RefcountPtr<NullGaugeImpl> null_gauge_; }; diff --git a/source/common/stats/null_text_readout.h b/source/common/stats/null_text_readout.h new file mode 100644 index 000000000000..3ee402ee993a --- /dev/null +++ b/source/common/stats/null_text_readout.h @@ -0,0 +1,44 @@ +#pragma once + +#include "envoy/stats/stats.h" + +#include "common/stats/metric_impl.h" + +namespace Envoy { +namespace Stats { + +/** + * Null text readout implementation. + * No-ops on all calls and requires no underlying metric or data. + */ +class NullTextReadoutImpl : public MetricImpl<TextReadout> { +public: + explicit NullTextReadoutImpl(SymbolTable& symbol_table) + : MetricImpl<TextReadout>(symbol_table), symbol_table_(symbol_table) {} + ~NullTextReadoutImpl() override { + // MetricImpl must be explicitly cleared() before destruction, otherwise it + // will not be able to access the SymbolTable& to free the symbols. An RAII + // alternative would be to store the SymbolTable reference in the + // MetricImpl, costing 8 bytes per stat. + MetricImpl::clear(symbol_table_); + } + + void set(const std::string&) override {} + std::string value() const override { return std::string(); } + + // Metric + bool used() const override { return false; } + SymbolTable& symbolTable() override { return symbol_table_; } + + // RefcountInterface + void incRefCount() override { refcount_helper_.incRefCount(); } + bool decRefCount() override { return refcount_helper_.decRefCount(); } + uint32_t use_count() const override { return refcount_helper_.use_count(); } + +private: + RefcountHelper refcount_helper_; + SymbolTable& symbol_table_; +}; + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/scope_prefixer.cc b/source/common/stats/scope_prefixer.cc index fc687c7b2341..5da4bb3686e4 100644 --- a/source/common/stats/scope_prefixer.cc +++ b/source/common/stats/scope_prefixer.cc @@ -49,6 +49,13 @@ Histogram& ScopePrefixer::histogramFromStatNameWithTags(const StatName& name, return scope_.histogramFromStatNameWithTags(StatName(stat_name_storage.get()), tags, unit); } +TextReadout& ScopePrefixer::textReadoutFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptConstRef tags) { + Stats::SymbolTable::StoragePtr stat_name_storage = + scope_.symbolTable().join({prefix_.statName(), name}); + return scope_.textReadoutFromStatNameWithTags(StatName(stat_name_storage.get()), tags); +} + CounterOptConstRef ScopePrefixer::findCounter(StatName name) const { return scope_.findCounter(name); } @@ -59,6 +66,10 @@ HistogramOptConstRef ScopePrefixer::findHistogram(StatName name) const { return scope_.findHistogram(name); } +TextReadoutOptConstRef ScopePrefixer::findTextReadout(StatName name) const { + return scope_.findTextReadout(name); +} + void ScopePrefixer::deliverHistogramToSinks(const Histogram& histograms, uint64_t val) { scope_.deliverHistogramToSinks(histograms, val); } diff --git a/source/common/stats/scope_prefixer.h b/source/common/stats/scope_prefixer.h index 948de23fba2d..b7c874375620 100644 --- a/source/common/stats/scope_prefixer.h +++ b/source/common/stats/scope_prefixer.h @@ -23,6 +23,8 @@ class ScopePrefixer : public Scope { Gauge::ImportMode import_mode) override; Histogram& histogramFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef tags, Histogram::Unit unit) override; + TextReadout& textReadoutFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptConstRef tags) override; void deliverHistogramToSinks(const Histogram& histograms, uint64_t val) override; Counter& counterFromString(const std::string& name) override { @@ -37,10 +39,15 @@ class ScopePrefixer : public Scope { StatNameManagedStorage storage(name, symbolTable()); return Scope::histogramFromStatName(storage.statName(), unit); } + TextReadout& textReadoutFromString(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return Scope::textReadoutFromStatName(storage.statName()); + } CounterOptConstRef findCounter(StatName name) const override; GaugeOptConstRef findGauge(StatName name) const override; HistogramOptConstRef findHistogram(StatName name) const override; + TextReadoutOptConstRef findTextReadout(StatName name) const override; const SymbolTable& constSymbolTable() const override { return scope_.constSymbolTable(); } SymbolTable& symbolTable() override { return scope_.symbolTable(); } diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 2d3c9645cd55..5a9a47f912fa 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -28,7 +28,7 @@ ThreadLocalStoreImpl::ThreadLocalStoreImpl(Allocator& alloc) tag_producer_(std::make_unique<TagProducerImpl>()), stats_matcher_(std::make_unique<StatsMatcherImpl>()), heap_allocator_(alloc.symbolTable()), null_counter_(alloc.symbolTable()), null_gauge_(alloc.symbolTable()), - null_histogram_(alloc.symbolTable()) {} + null_histogram_(alloc.symbolTable()), null_text_readout_(alloc.symbolTable()) {} ThreadLocalStoreImpl::~ThreadLocalStoreImpl() { ASSERT(shutting_down_ || !threading_ever_initialized_); @@ -51,6 +51,7 @@ void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) { removeRejectedStats(scope->central_cache_->counters_, deleted_counters_); removeRejectedStats(scope->central_cache_->gauges_, deleted_gauges_); removeRejectedStats(scope->central_cache_->histograms_, deleted_histograms_); + removeRejectedStats(scope->central_cache_->text_readouts_, deleted_text_readouts_); } } @@ -129,6 +130,22 @@ std::vector<GaugeSharedPtr> ThreadLocalStoreImpl::gauges() const { return ret; } +std::vector<TextReadoutSharedPtr> ThreadLocalStoreImpl::textReadouts() const { + // Handle de-dup due to overlapping scopes. + std::vector<TextReadoutSharedPtr> ret; + StatNameHashSet names; + Thread::LockGuard lock(lock_); + for (ScopeImpl* scope : scopes_) { + for (auto& text_readout : scope->central_cache_->text_readouts_) { + if (names.insert(text_readout.first).second) { + ret.push_back(text_readout.second); + } + } + } + + return ret; +} + std::vector<ParentHistogramSharedPtr> ThreadLocalStoreImpl::histograms() const { std::vector<ParentHistogramSharedPtr> ret; Thread::LockGuard lock(lock_); @@ -538,6 +555,44 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( return **central_ref; } +TextReadout& ThreadLocalStoreImpl::ScopeImpl::textReadoutFromStatNameWithTags( + const StatName& name, StatNameTagVectorOptConstRef stat_name_tags) { + if (parent_.rejectsAll()) { + return parent_.null_text_readout_; + } + + // Determine the final name based on the prefix and the passed name. + // + // Note that we can do map.find(final_name.c_str()), but we cannot do + // map[final_name.c_str()] as the char*-keyed maps would then save the pointer + // to a temporary, and address sanitization errors would follow. Instead we + // must do a find() first, using the value if it succeeds. If it fails, then + // after we construct the stat we can insert it into the required maps. This + // strategy costs an extra hash lookup for each miss, but saves time + // re-copying the string and significant memory overhead. + TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); + Stats::StatName final_stat_name = joiner.nameWithTags(); + + // We now find the TLS cache. This might remain null if we don't have TLS + // initialized currently. + StatRefMap<TextReadout>* tls_cache = nullptr; + StatNameHashSet* tls_rejected_stats = nullptr; + if (!parent_.shutting_down_ && parent_.tls_) { + TlsCacheEntry& entry = parent_.tls_->getTyped<TlsCache>().insertScope(this->scope_id_); + tls_cache = &entry.text_readouts_; + tls_rejected_stats = &entry.rejected_stats_; + } + + return safeMakeStat<TextReadout>( + final_stat_name, joiner.tagExtractedName(), stat_name_tags, central_cache_->text_readouts_, + central_cache_->rejected_stats_, + [](Allocator& allocator, StatName name, StatName tag_extracted_name, + const StatNameTagVector& tags) -> TextReadoutSharedPtr { + return allocator.makeTextReadout(name, tag_extracted_name, tags); + }, + tls_cache, tls_rejected_stats, parent_.null_text_readout_); +} + CounterOptConstRef ThreadLocalStoreImpl::ScopeImpl::findCounter(StatName name) const { return findStatLockHeld<Counter>(name, central_cache_->counters_); } @@ -556,6 +611,10 @@ HistogramOptConstRef ThreadLocalStoreImpl::ScopeImpl::findHistogram(StatName nam return std::cref(*histogram_ref); } +TextReadoutOptConstRef ThreadLocalStoreImpl::ScopeImpl::findTextReadout(StatName name) const { + return findStatLockHeld<TextReadout>(name, central_cache_->text_readouts_); +} + Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(StatName name, ParentHistogramImpl& parent) { // tlsHistogram() is generally not called for a histogram that is rejected by diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index d7ba7a5d346f..17d3983b38cd 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -15,6 +15,7 @@ #include "common/stats/histogram_impl.h" #include "common/stats/null_counter.h" #include "common/stats/null_gauge.h" +#include "common/stats/null_text_readout.h" #include "common/stats/symbol_table_impl.h" #include "common/stats/utility.h" @@ -184,6 +185,13 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo Histogram& histogramFromString(const std::string& name, Histogram::Unit unit) override { return default_scope_->histogramFromString(name, unit); } + TextReadout& textReadoutFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptConstRef tags) override { + return default_scope_->textReadoutFromStatNameWithTags(name, tags); + } + TextReadout& textReadoutFromString(const std::string& name) override { + return default_scope_->textReadoutFromString(name); + } NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; } const SymbolTable& constSymbolTable() const override { return alloc_.constSymbolTable(); } SymbolTable& symbolTable() override { return alloc_.symbolTable(); } @@ -221,9 +229,31 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo } return absl::nullopt; } + TextReadoutOptConstRef findTextReadout(StatName name) const override { + TextReadoutOptConstRef found_text_readout; + Thread::LockGuard lock(lock_); + for (ScopeImpl* scope : scopes_) { + found_text_readout = scope->findTextReadout(name); + if (found_text_readout.has_value()) { + return found_text_readout; + } + } + return absl::nullopt; + } +#if 0 + Gauge& gauge(const std::string& name) override { return default_scope_->gauge(name); } + TextReadout& textReadout(const std::string& name) override { + return default_scope_->textReadout(name); + } + Histogram& histogram(const std::string& name) override { + return default_scope_->histogram(name); + }; + +#endif // >>>>>>> pr5844 // Stats::Store std::vector<CounterSharedPtr> counters() const override; std::vector<GaugeSharedPtr> gauges() const override; + std::vector<TextReadoutSharedPtr> textReadouts() const override; std::vector<ParentHistogramSharedPtr> histograms() const override; // Stats::StoreRoot @@ -246,12 +276,13 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo template <class Stat> using StatRefMap = StatNameHashMap<std::reference_wrapper<Stat>>; struct TlsCacheEntry { - // The counters and gauges in the TLS cache are stored by reference, + // The counters, gauges and text readouts in the TLS cache are stored by reference, // depending on the CentralCache for backing store. This avoids a potential // contention-storm when destructing a scope, as the counter/gauge ref-count // decrement in allocator_impl.cc needs to hold the single allocator mutex. StatRefMap<Counter> counters_; StatRefMap<Gauge> gauges_; + StatRefMap<TextReadout> text_readouts_; // The histogram objects are not shared with the central cache, and don't // require taking a lock when decrementing their ref-count. @@ -274,6 +305,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo StatNameHashMap<CounterSharedPtr> counters_; StatNameHashMap<GaugeSharedPtr> gauges_; StatNameHashMap<ParentHistogramImplSharedPtr> histograms_; + StatNameHashMap<TextReadoutSharedPtr> text_readouts_; StatNameStorageSet rejected_stats_; SymbolTable& symbol_table_; }; @@ -293,6 +325,8 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo StatNameTagVectorOptConstRef tags, Histogram::Unit unit) override; Histogram& tlsHistogram(StatName name, ParentHistogramImpl& parent) override; + TextReadout& textReadoutFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptConstRef tags) override; ScopePtr createScope(const std::string& name) override { return parent_.createScope(symbolTable().toString(prefix_.statName()) + "." + name); } @@ -311,6 +345,10 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo StatNameManagedStorage storage(name, symbolTable()); return histogramFromStatName(storage.statName(), unit); } + TextReadout& textReadoutFromString(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return textReadoutFromStatName(storage.statName()); + } NullGaugeImpl& nullGauge(const std::string&) override { return parent_.null_gauge_; } @@ -319,6 +357,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo CounterOptConstRef findCounter(StatName name) const override; GaugeOptConstRef findGauge(StatName name) const override; HistogramOptConstRef findHistogram(StatName name) const override; + TextReadoutOptConstRef findTextReadout(StatName name) const override; template <class StatType> using MakeStatFn = std::function<RefcountPtr<StatType>( @@ -407,6 +446,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo NullCounterImpl null_counter_; NullGaugeImpl null_gauge_; NullHistogramImpl null_histogram_; + NullTextReadoutImpl null_text_readout_; // Retain storage for deleted stats; these are no longer in maps because the // matcher-pattern was established after they were created. Since the stats @@ -419,6 +459,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo std::vector<CounterSharedPtr> deleted_counters_; std::vector<GaugeSharedPtr> deleted_gauges_; std::vector<HistogramSharedPtr> deleted_histograms_; + std::vector<TextReadoutSharedPtr> deleted_text_readouts_; Thread::ThreadSynchronizer sync_; std::atomic<uint64_t> next_scope_id_{}; diff --git a/test/common/stats/isolated_store_impl_test.cc b/test/common/stats/isolated_store_impl_test.cc index ffa8e94f6915..ee618669cb2c 100644 --- a/test/common/stats/isolated_store_impl_test.cc +++ b/test/common/stats/isolated_store_impl_test.cc @@ -151,8 +151,17 @@ TEST_F(StatsIsolatedStoreImplTest, AllWithSymbolTable) { EXPECT_EQ("g1", g1.tagExtractedName()); EXPECT_EQ("scope1.g2", g2.tagExtractedName()); EXPECT_EQ(0, g1.tags().size()); - EXPECT_EQ(0, g1.tags().size()); + EXPECT_EQ(0, g2.tags().size()); + TextReadout& b1 = store_->textReadoutFromStatName(makeStatName("b1")); + TextReadout& b2 = scope1->textReadoutFromStatName(makeStatName("b2")); + EXPECT_NE(&b1, &b2); + EXPECT_EQ("b1", b1.name()); + EXPECT_EQ("scope1.b2", b2.name()); + EXPECT_EQ("b1", b1.tagExtractedName()); + EXPECT_EQ("scope1.b2", b2.tagExtractedName()); + EXPECT_EQ(0, b1.tags().size()); + EXPECT_EQ(0, b2.tags().size()); Histogram& h1 = store_->histogramFromStatName(makeStatName("h1"), Stats::Histogram::Unit::Unspecified); Histogram& h2 = @@ -176,6 +185,7 @@ TEST_F(StatsIsolatedStoreImplTest, AllWithSymbolTable) { EXPECT_EQ(4UL, store_->counters().size()); EXPECT_EQ(2UL, store_->gauges().size()); + EXPECT_EQ(2UL, store_->textReadouts().size()); } TEST_F(StatsIsolatedStoreImplTest, ConstSymtabAccessor) { @@ -197,19 +207,21 @@ TEST_F(StatsIsolatedStoreImplTest, LongStatName) { /** * Test stats macros. @see stats_macros.h */ -#define ALL_TEST_STATS(COUNTER, GAUGE, HISTOGRAM) \ +#define ALL_TEST_STATS(COUNTER, GAUGE, HISTOGRAM, TEXT_READOUT) \ COUNTER(test_counter) \ GAUGE(test_gauge, Accumulate) \ - HISTOGRAM(test_histogram, Microseconds) + HISTOGRAM(test_histogram, Microseconds) \ + TEXT_READOUT(test_text_readout) struct TestStats { - ALL_TEST_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) + ALL_TEST_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT, + GENERATE_TEXT_READOUT_STRUCT) }; TEST_F(StatsIsolatedStoreImplTest, StatsMacros) { - TestStats test_stats{ALL_TEST_STATS(POOL_COUNTER_PREFIX(*store_, "test."), - POOL_GAUGE_PREFIX(*store_, "test."), - POOL_HISTOGRAM_PREFIX(*store_, "test."))}; + TestStats test_stats{ALL_TEST_STATS( + POOL_COUNTER_PREFIX(*store_, "test."), POOL_GAUGE_PREFIX(*store_, "test."), + POOL_HISTOGRAM_PREFIX(*store_, "test."), POOL_TEXT_READOUT_PREFIX(*store_, "test."))}; Counter& counter = test_stats.test_counter_; EXPECT_EQ("test.test_counter", counter.name()); @@ -217,6 +229,9 @@ TEST_F(StatsIsolatedStoreImplTest, StatsMacros) { Gauge& gauge = test_stats.test_gauge_; EXPECT_EQ("test.test_gauge", gauge.name()); + TextReadout& textReadout = test_stats.test_text_readout_; + EXPECT_EQ("test.test_text_readout", textReadout.name()); + Histogram& histogram = test_stats.test_histogram_; EXPECT_EQ("test.test_histogram", histogram.name()); EXPECT_EQ(Histogram::Unit::Microseconds, histogram.unit()); diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 35d5a45d22d2..4228123acfbe 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -214,6 +214,9 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) { ASSERT_TRUE(found_histogram.has_value()); EXPECT_EQ(&h1, &found_histogram->get()); + TextReadout& t1 = store_->textReadoutFromString("t1"); + EXPECT_EQ(&t1, &store_->textReadoutFromString("t1")); + EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 200)); h1.recordValue(200); EXPECT_CALL(sink_, onHistogramComplete(Ref(h1), 100)); @@ -225,6 +228,9 @@ TEST_F(StatsThreadLocalStoreTest, NoTls) { EXPECT_EQ(1UL, store_->gauges().size()); EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1 EXPECT_EQ(2L, store_->gauges().front().use_count()); + EXPECT_EQ(1UL, store_->textReadouts().size()); + EXPECT_EQ(&t1, store_->textReadouts().front().get()); // front() ok when size()==1 + EXPECT_EQ(2L, store_->textReadouts().front().use_count()); store_->shutdownThreading(); } @@ -262,12 +268,19 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { ASSERT_TRUE(found_histogram.has_value()); EXPECT_EQ(&h1, &found_histogram->get()); + TextReadout& t1 = store_->textReadoutFromString("t1"); + EXPECT_EQ(&t1, &store_->textReadoutFromString("t1")); + EXPECT_EQ(1UL, store_->counters().size()); + EXPECT_EQ(&c1, TestUtility::findCounter(*store_, "c1").get()); EXPECT_EQ(2L, TestUtility::findCounter(*store_, "c1").use_count()); EXPECT_EQ(1UL, store_->gauges().size()); EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1 EXPECT_EQ(2L, store_->gauges().front().use_count()); + EXPECT_EQ(1UL, store_->textReadouts().size()); + EXPECT_EQ(&t1, store_->textReadouts().front().get()); // front() ok when size()==1 + EXPECT_EQ(2UL, store_->textReadouts().front().use_count()); store_->shutdownThreading(); tls_.shutdownThread(); @@ -278,6 +291,9 @@ TEST_F(StatsThreadLocalStoreTest, Tls) { EXPECT_EQ(1UL, store_->gauges().size()); EXPECT_EQ(&g1, store_->gauges().front().get()); // front() ok when size()==1 EXPECT_EQ(2L, store_->gauges().front().use_count()); + EXPECT_EQ(1UL, store_->textReadouts().size()); + EXPECT_EQ(&t1, store_->textReadouts().front().get()); // front() ok when size()==1 + EXPECT_EQ(2L, store_->textReadouts().front().use_count()); } TEST_F(StatsThreadLocalStoreTest, BasicScope) { @@ -328,6 +344,11 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { ASSERT_TRUE(found_histogram2.has_value()); EXPECT_EQ(&h2, &found_histogram2->get()); + TextReadout& t1 = store_->textReadoutFromString("t1"); + TextReadout& t2 = scope1->textReadoutFromString("t2"); + EXPECT_EQ("t1", t1.name()); + EXPECT_EQ("scope1.t2", t2.name()); + StatNameManagedStorage tag_key("a", *symbol_table_); StatNameManagedStorage tag_value("b", *symbol_table_); StatNameTagVector tags{{StatName(tag_key.statName()), StatName(tag_value.statName())}}; @@ -435,6 +456,9 @@ TEST_F(StatsThreadLocalStoreTest, NestedScopes) { Gauge& g1 = scope2->gaugeFromString("some_gauge", Gauge::ImportMode::Accumulate); EXPECT_EQ("scope1.foo.some_gauge", g1.name()); + TextReadout& t1 = scope2->textReadoutFromString("some_string"); + EXPECT_EQ("scope1.foo.some_string", t1.name()); + store_->shutdownThreading(); tls_.shutdownThread(); } @@ -474,6 +498,19 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { EXPECT_EQ(1UL, g2.value()); EXPECT_EQ(1UL, store_->gauges().size()); + // TextReadouts should work just like gauges. + TextReadout& t1 = scope1->textReadoutFromString("b"); + TextReadout& t2 = scope2->textReadoutFromString("b"); + EXPECT_EQ(&t1, &t2); + + t1.set("hello"); + EXPECT_EQ("hello", t1.value()); + EXPECT_EQ("hello", t2.value()); + t2.set("goodbye"); + EXPECT_EQ("goodbye", t1.value()); + EXPECT_EQ("goodbye", t2.value()); + EXPECT_EQ(1UL, store_->textReadouts().size()); + // Deleting scope 1 will call free but will be reference counted. It still leaves scope 2 valid. scope1.reset(); c2.inc(); @@ -482,6 +519,54 @@ TEST_F(StatsThreadLocalStoreTest, OverlappingScopes) { g2.set(10); EXPECT_EQ(10UL, g2.value()); EXPECT_EQ(1UL, store_->gauges().size()); + t2.set("abc"); + EXPECT_EQ("abc", t2.value()); + EXPECT_EQ(1UL, store_->textReadouts().size()); + + store_->shutdownThreading(); + tls_.shutdownThread(); +} + +TEST_F(StatsThreadLocalStoreTest, TextReadoutAllLengths) { + store_->initializeThreading(main_thread_dispatcher_, tls_); + + TextReadout& t = store_->textReadoutFromString("t"); + EXPECT_EQ("", t.value()); + std::string str; + // ASCII + for (int i = 0; i < 15; i++) { + str += ('a' + i); + t.set(str); + EXPECT_EQ(str, t.value()); + } + + // Non-ASCII + str = ""; + for (int i = 0; i < 15; i++) { + str += ('\xEE' + i); + t.set(str); + EXPECT_EQ(str, t.value()); + } + + // Null bytes ok; the TextReadout implementation doesn't use null termination in its storage + t.set(std::string("\x00", 1)); + EXPECT_EQ(std::string("\x00", 1), t.value()); + t.set(std::string("\x00\x00\x00", 3)); + EXPECT_EQ(std::string("\x00\x00\x00", 3), t.value()); + EXPECT_NE(std::string("\x00", 1), t.value()); + EXPECT_NE(std::string("", 0), t.value()); + + // No Truncation to 15 + t.set("aaaabbbbccccdddX"); + EXPECT_EQ("aaaabbbbccccdddX", t.value()); + t.set("aaaabbbbccccdddXX"); + EXPECT_EQ("aaaabbbbccccdddXX", t.value()); + t.set("aaaabbbbccccdddXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"); + // EXPECT_EQ("aaaabbbbccccddd", t.value()); + + // Can set back to empty + t.set(""); + EXPECT_EQ("", t.value()); store_->shutdownThreading(); tls_.shutdownThread(); @@ -611,6 +696,21 @@ TEST_F(StatsMatcherTLSTest, TestNoOpStatImpls) { Gauge& noop_gauge_2 = store_->gaugeFromString("noop_gauge_2", Gauge::ImportMode::Accumulate); EXPECT_EQ(&noop_gauge, &noop_gauge_2); + // TextReadout + TextReadout& noop_string = store_->textReadoutFromString("noop_string"); + EXPECT_EQ(noop_string.name(), ""); + EXPECT_EQ("", noop_string.value()); + noop_string.set("hello"); + EXPECT_EQ("", noop_string.value()); + noop_string.set("hello"); + EXPECT_EQ("", noop_string.value()); + noop_string.set("goodbye"); + EXPECT_EQ("", noop_string.value()); + noop_string.set("hello"); + EXPECT_EQ("", noop_string.value()); + TextReadout& noop_string_2 = store_->textReadoutFromString("noop_string_2"); + EXPECT_EQ(&noop_string, &noop_string_2); + // Histogram Histogram& noop_histogram = store_->histogramFromString("noop_histogram", Stats::Histogram::Unit::Unspecified); @@ -647,6 +747,8 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { store_->histogramFromString("lowercase_histogram", Stats::Histogram::Unit::Unspecified); EXPECT_EQ(lowercase_histogram.name(), "lowercase_histogram"); + TextReadout& lowercase_string = store_->textReadoutFromString("lowercase_string"); + EXPECT_EQ(lowercase_string.name(), "lowercase_string"); // And the creation of counters/gauges/histograms which have uppercase letters should fail. Counter& uppercase_counter = store_->counterFromString("UPPERCASE_counter"); EXPECT_EQ(uppercase_counter.name(), ""); @@ -663,6 +765,11 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { uppercase_gauge.inc(); EXPECT_EQ(uppercase_gauge.value(), 0); + TextReadout& uppercase_string = store_->textReadoutFromString("uppercase_STRING"); + EXPECT_EQ(uppercase_string.name(), ""); + uppercase_string.set("A STRING VALUE"); + EXPECT_EQ("", uppercase_string.value()); + // Histograms are harder to query and test, so we resort to testing that name() returns the empty // string. Histogram& uppercase_histogram = @@ -704,6 +811,7 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { Histogram& valid_histogram = store_->histogramFromString("valid_histogram", Stats::Histogram::Unit::Unspecified); + EXPECT_EQ(valid_histogram.name(), "valid_histogram"); Histogram& invalid_histogram_1 = @@ -714,6 +822,18 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { store_->histogramFromString("also_INVALID_histogram", Stats::Histogram::Unit::Unspecified); EXPECT_EQ(invalid_histogram_2.name(), ""); + TextReadout& valid_string = store_->textReadoutFromString("valid_string"); + valid_string.set("i'm valid"); + EXPECT_EQ("i'm valid", valid_string.value()); + + TextReadout& invalid_string_1 = store_->textReadoutFromString("invalid_string"); + invalid_string_1.set("nope"); + EXPECT_EQ("", invalid_string_1.value()); + + TextReadout& invalid_string_2 = store_->textReadoutFromString("also_INVLD_string"); + invalid_string_2.set("still no"); + EXPECT_EQ("", invalid_string_2.value()); + // Expected to free lowercase_counter, lowercase_gauge, valid_counter, valid_gauge store_->shutdownThreading(); } @@ -865,11 +985,14 @@ TEST_F(StatsThreadLocalStoreTest, RemoveRejectedStats) { Counter& counter = store_->counterFromString("c1"); Gauge& gauge = store_->gaugeFromString("g1", Gauge::ImportMode::Accumulate); Histogram& histogram = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); - ASSERT_EQ(1, store_->counters().size()); // "c1". + TextReadout& textReadout = store_->textReadoutFromString("t1"); + ASSERT_EQ(1, store_->counters().size()); // "stats.overflow" and "c1". EXPECT_TRUE(&counter == store_->counters()[0].get() || &counter == store_->counters()[1].get()); // counters() order is non-deterministic. ASSERT_EQ(1, store_->gauges().size()); EXPECT_EQ("g1", store_->gauges()[0]->name()); + ASSERT_EQ(1, store_->textReadouts().size()); + EXPECT_EQ("t1", store_->textReadouts()[0]->name()); ASSERT_EQ(1, store_->histograms().size()); EXPECT_EQ("h1", store_->histograms()[0]->name()); @@ -882,11 +1005,13 @@ TEST_F(StatsThreadLocalStoreTest, RemoveRejectedStats) { // They can no longer be found. EXPECT_EQ(0, store_->counters().size()); EXPECT_EQ(0, store_->gauges().size()); + EXPECT_EQ(0, store_->textReadouts().size()); EXPECT_EQ(0, store_->histograms().size()); // However, referencing the previously allocated stats will not crash. counter.inc(); gauge.inc(); + textReadout.set("fortytwo"); EXPECT_CALL(sink_, onHistogramComplete(Ref(histogram), 42)); histogram.recordValue(42); store_->shutdownThreading(); @@ -993,17 +1118,25 @@ TEST_F(StatsThreadLocalStoreTest, ShuttingDown) { store_->counterFromString("c1"); store_->gaugeFromString("g1", Gauge::ImportMode::Accumulate); + store_->textReadoutFromString("t1"); store_->shutdownThreading(); store_->counterFromString("c2"); store_->gaugeFromString("g2", Gauge::ImportMode::Accumulate); + store_->textReadoutFromString("t2"); // We do not keep ref-counts for counters and gauges in the TLS cache, so // all these stats should have a ref-count of 2: one for the SharedPtr // returned from find*(), and one for the central cache. EXPECT_EQ(2L, TestUtility::findCounter(*store_, "c1").use_count()); EXPECT_EQ(2L, TestUtility::findGauge(*store_, "g1").use_count()); + + // c1, g1, t1 should have a thread local ref, but c2, g2, t2 should not. + EXPECT_EQ(2L, TestUtility::findCounter(*store_, "c1").use_count()); + EXPECT_EQ(2L, TestUtility::findGauge(*store_, "g1").use_count()); + EXPECT_EQ(2L, TestUtility::findTextReadout(*store_, "t1").use_count()); EXPECT_EQ(2L, TestUtility::findCounter(*store_, "c2").use_count()); EXPECT_EQ(2L, TestUtility::findGauge(*store_, "g2").use_count()); + EXPECT_EQ(2L, TestUtility::findTextReadout(*store_, "t2").use_count()); store_->shutdownThreading(); tls_.shutdownThread(); diff --git a/test/integration/server.h b/test/integration/server.h index 13dfbe451147..79a4b29ecba4 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -98,6 +98,13 @@ class TestScopeWrapper : public Scope { Thread::LockGuard lock(lock_); return wrapped_scope_->histogramFromStatNameWithTags(name, tags, unit); } + + TextReadout& textReadoutFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptConstRef tags) override { + Thread::LockGuard lock(lock_); + return wrapped_scope_->textReadoutFromStatNameWithTags(name, tags); + } + NullGaugeImpl& nullGauge(const std::string& str) override { return wrapped_scope_->nullGauge(str); } @@ -114,6 +121,10 @@ class TestScopeWrapper : public Scope { StatNameManagedStorage storage(name, symbolTable()); return histogramFromStatName(storage.statName(), unit); } + TextReadout& textReadoutFromString(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return textReadoutFromStatName(storage.statName()); + } CounterOptConstRef findCounter(StatName name) const override { Thread::LockGuard lock(lock_); @@ -127,6 +138,10 @@ class TestScopeWrapper : public Scope { Thread::LockGuard lock(lock_); return wrapped_scope_->findHistogram(name); } + TextReadoutOptConstRef findTextReadout(StatName name) const override { + Thread::LockGuard lock(lock_); + return wrapped_scope_->findTextReadout(name); + } const SymbolTable& constSymbolTable() const override { return wrapped_scope_->constSymbolTable(); @@ -178,6 +193,15 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); return store_.histogramFromString(name, unit); } + TextReadout& textReadoutFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptConstRef tags) override { + Thread::LockGuard lock(lock_); + return store_.textReadoutFromStatNameWithTags(name, tags); + } + TextReadout& textReadoutFromString(const std::string& name) override { + Thread::LockGuard lock(lock_); + return store_.textReadoutFromString(name); + } CounterOptConstRef findCounter(StatName name) const override { Thread::LockGuard lock(lock_); return store_.findCounter(name); @@ -190,6 +214,10 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); return store_.findHistogram(name); } + TextReadoutOptConstRef findTextReadout(StatName name) const override { + Thread::LockGuard lock(lock_); + return store_.findTextReadout(name); + } const SymbolTable& constSymbolTable() const override { return store_.constSymbolTable(); } SymbolTable& symbolTable() override { return store_.symbolTable(); } @@ -202,7 +230,10 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); return store_.gauges(); } - + std::vector<TextReadoutSharedPtr> textReadouts() const override { + Thread::LockGuard lock(lock_); + return store_.textReadouts(); + } std::vector<ParentHistogramSharedPtr> histograms() const override { Thread::LockGuard lock(lock_); return store_.histograms(); diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index 595b2bd779e2..1b940c66de17 100644 --- a/test/mocks/stats/mocks.cc +++ b/test/mocks/stats/mocks.cc @@ -30,6 +30,12 @@ MockGauge::MockGauge() : used_(false), value_(0), import_mode_(ImportMode::Accum } MockGauge::~MockGauge() = default; +MockTextReadout::MockTextReadout() { + ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_)); + ON_CALL(*this, value()).WillByDefault(ReturnPointee(&value_)); +} +MockTextReadout::~MockTextReadout() {} + MockHistogram::MockHistogram() { ON_CALL(*this, unit()).WillByDefault(ReturnPointee(&unit_)); ON_CALL(*this, recordValue(_)).WillByDefault(Invoke([this](uint64_t value) { diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index e4336af3069a..1abe596ed348 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -245,6 +245,19 @@ class MockParentHistogram : public MockMetric<ParentHistogram> { RefcountHelper refcount_helper_; }; +class MockTextReadout : public MockMetric<TextReadout> { +public: + MockTextReadout(); + ~MockTextReadout(); + + MOCK_METHOD1(set, void(const std::string& value)); + MOCK_CONST_METHOD0(used, bool()); + MOCK_CONST_METHOD0(value, std::string()); + + bool used_; + std::string value_; +}; + class MockMetricSnapshot : public MetricSnapshot { public: MockMetricSnapshot(); @@ -253,6 +266,7 @@ class MockMetricSnapshot : public MetricSnapshot { MOCK_METHOD(const std::vector<CounterSnapshot>&, counters, ()); MOCK_METHOD(const std::vector<std::reference_wrapper<const Gauge>>&, gauges, ()); MOCK_METHOD(const std::vector<std::reference_wrapper<const ParentHistogram>>&, histograms, ()); + MOCK_METHOD(const std::vector<TextReadout>&, textReadouts, ()); std::vector<CounterSnapshot> counters_; std::vector<std::reference_wrapper<const Gauge>> gauges_; @@ -289,10 +303,13 @@ class MockStore : public SymbolTableProvider, public TestUtil::TestStore { MOCK_METHOD(std::vector<GaugeSharedPtr>, gauges, (), (const)); MOCK_METHOD(Histogram&, histogram, (const std::string&, Histogram::Unit)); MOCK_METHOD(std::vector<ParentHistogramSharedPtr>, histograms, (), (const)); + MOCK_METHOD(TextReadout&, textReadout, (const std::string&)); + MOCK_METHOD(std::vector<TextReadoutSharedPtr>, text_readouts, (), (const)); MOCK_METHOD(CounterOptConstRef, findCounter, (StatName), (const)); MOCK_METHOD(GaugeOptConstRef, findGauge, (StatName), (const)); MOCK_METHOD(HistogramOptConstRef, findHistogram, (StatName), (const)); + MOCK_METHOD(TextReadoutOptConstRef, findTextReadout, (StatName), (const)); Counter& counterFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef) override { @@ -308,6 +325,11 @@ class MockStore : public SymbolTableProvider, public TestUtil::TestStore { Histogram::Unit unit) override { return histogram(symbol_table_->toString(name), unit); } + TextReadout& textReadoutFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptConstRef) override { + // We always just respond with the mocked counter, so the tags don't matter. + return textReadout(symbol_table_->toString(name)); + } TestSymbolTable symbol_table_; testing::NiceMock<MockCounter> counter_; diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 34c7deeaa573..91b735420c41 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -141,6 +141,11 @@ Stats::GaugeSharedPtr TestUtility::findGauge(Stats::Store& store, const std::str return findByName(store.gauges(), name); } +Stats::TextReadoutSharedPtr TestUtility::findTextReadout(Stats::Store& store, + const std::string& name) { + return findByName(store.textReadouts(), name); +} + void TestUtility::waitForCounterEq(Stats::Store& store, const std::string& name, uint64_t value, Event::TestTimeSystem& time_system) { while (findCounter(store, name) == nullptr || findCounter(store, name)->value() != value) { diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 749bd4ea9b44..fc0cd744c569 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -184,7 +184,7 @@ class TestUtility { * Find a counter in a stats store. * @param store supplies the stats store. * @param name supplies the name to search for. - * @return Stats::CounterSharedPtr the counter or nullptr if there is none. + * @return Stats::CounterSharedPtr the counter, or nullptr if there is none. */ static Stats::CounterSharedPtr findCounter(Stats::Store& store, const std::string& name); @@ -192,7 +192,7 @@ class TestUtility { * Find a gauge in a stats store. * @param store supplies the stats store. * @param name supplies the name to search for. - * @return Stats::GaugeSharedPtr the gauge or nullptr if there is none. + * @return Stats::GaugeSharedPtr the gauge, or nullptr if there is none. */ static Stats::GaugeSharedPtr findGauge(Stats::Store& store, const std::string& name); @@ -236,6 +236,14 @@ class TestUtility { static void waitForGaugeEq(Stats::Store& store, const std::string& name, uint64_t value, Event::TestTimeSystem& time_system); + /** + * Find a readout in a stats store. + * @param store supplies the stats store. + * @param name supplies the name to search for. + * @return Stats::TextReadoutSharedPtr the readout, or nullptr if there is none. + */ + static Stats::TextReadoutSharedPtr findTextReadout(Stats::Store& store, const std::string& name); + /** * Convert a string list of IP addresses into a list of network addresses usable for DNS * response testing. From 7a54fa9f4e79a97fd1a0ba7879ce3b5925a9fc90 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Fri, 3 Apr 2020 16:45:32 -0400 Subject: [PATCH 02/16] Update TextReadoutImpl to have Mutex and don't have uint64 value. Signed-off-by: Misha Efimov <mef@google.com> --- include/envoy/stats/scope.h | 2 +- source/common/stats/allocator_impl.cc | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index 0ac873cc6981..d72f230347e3 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -156,7 +156,7 @@ class Scope { virtual TextReadout& textReadoutFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef tags) PURE; - /**v + /** * TODO(#6667): this variant is deprecated: use textReadoutFromStatName. * @param name The name, expressed as a string. * @return a text readout within the scope's namespace. diff --git a/source/common/stats/allocator_impl.cc b/source/common/stats/allocator_impl.cc index 62da300b788f..0f017a6d6414 100644 --- a/source/common/stats/allocator_impl.cc +++ b/source/common/stats/allocator_impl.cc @@ -39,7 +39,7 @@ void AllocatorImpl::debugPrint() { } #endif -// Counter and Gauge both inherit from from RefcountInterface and +// Counter, Gauge and TextReadout inherit from RefcountInterface and // Metric. MetricImpl takes care of most of the Metric API, but we need to cover // symbolTable() here, which we don't store directly, but get it via the alloc, // which we need in order to clean up the counter and gauge maps in that class @@ -101,10 +101,6 @@ template <class BaseClass> class StatsSharedImpl : public MetricImpl<BaseClass> protected: AllocatorImpl& alloc_; - // Holds backing store shared by both CounterImpl and GaugeImpl. CounterImpl - // adds another field, pending_increment_, that is not used in Gauge. - std::atomic<uint64_t> value_{0}; - // ref_count_ can be incremented as an atomic, without taking a new lock, as // the critical 0->1 transition occurs in makeCounter and makeGauge, which // already hold the lock. Increment also occurs when copying shared pointers, @@ -144,6 +140,7 @@ class CounterImpl : public StatsSharedImpl<Counter> { uint64_t value() const override { return value_; } private: + std::atomic<uint64_t> value_{0}; std::atomic<uint64_t> pending_increment_{0}; }; @@ -226,6 +223,8 @@ class GaugeImpl : public StatsSharedImpl<Gauge> { break; } } +private: + std::atomic<uint64_t> value_{0}; }; class TextReadoutImpl : public StatsSharedImpl<TextReadout> { From 00ba4249dc38e833558ad5e64083d6060d0d7248 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Fri, 3 Apr 2020 16:55:06 -0400 Subject: [PATCH 03/16] Use MutexBasicLockable. Signed-off-by: Misha Efimov <mef@google.com> --- source/common/stats/allocator_impl.cc | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/source/common/stats/allocator_impl.cc b/source/common/stats/allocator_impl.cc index 0f017a6d6414..1aea4cec8e74 100644 --- a/source/common/stats/allocator_impl.cc +++ b/source/common/stats/allocator_impl.cc @@ -223,6 +223,7 @@ class GaugeImpl : public StatsSharedImpl<Gauge> { break; } } + private: std::atomic<uint64_t> value_{0}; }; @@ -239,10 +240,17 @@ class TextReadoutImpl : public StatsSharedImpl<TextReadout> { } // Stats::TextReadout - void set(const std::string& value) override { value_ = value; } - std::string value() const override { return value_; } + void set(const std::string& value) override { + Thread::LockGuard lock(mutex_); + value_ = value; + } + std::string value() const override { + Thread::LockGuard lock(mutex_); + return value_; + } private: + mutable Thread::MutexBasicLockable mutex_; std::string value_; }; From c908a4f431c46185e2879d380fc81e5b2ee24536 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Tue, 7 Apr 2020 11:30:41 -0400 Subject: [PATCH 04/16] Address review comments. Signed-off-by: Misha Efimov <mef@google.com> --- include/envoy/stats/scope.h | 4 ++-- source/common/stats/allocator_impl.cc | 10 +++++++--- source/common/stats/thread_local_store.h | 9 --------- test/common/stats/thread_local_store_test.cc | 11 +++++------ test/integration/server.h | 8 ++++---- test/integration/stats_integration_test.cc | 4 ++-- test/test_common/utility.h | 6 +++--- 7 files changed, 23 insertions(+), 29 deletions(-) diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index d72f230347e3..e5a9632d74ec 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -17,9 +17,9 @@ namespace Stats { class Counter; class Gauge; class Histogram; -class TextReadout; -class Scope; class NullGaugeImpl; +class Scope; +class TextReadout; using CounterOptConstRef = absl::optional<std::reference_wrapper<const Counter>>; using GaugeOptConstRef = absl::optional<std::reference_wrapper<const Gauge>>; diff --git a/source/common/stats/allocator_impl.cc b/source/common/stats/allocator_impl.cc index 1aea4cec8e74..c4cb9121b934 100644 --- a/source/common/stats/allocator_impl.cc +++ b/source/common/stats/allocator_impl.cc @@ -241,16 +241,16 @@ class TextReadoutImpl : public StatsSharedImpl<TextReadout> { // Stats::TextReadout void set(const std::string& value) override { - Thread::LockGuard lock(mutex_); + absl::MutexLock lock(&mutex_); value_ = value; } std::string value() const override { - Thread::LockGuard lock(mutex_); + absl::MutexLock lock(&mutex_); return value_; } private: - mutable Thread::MutexBasicLockable mutex_; + mutable absl::Mutex mutex_; std::string value_; }; @@ -258,6 +258,7 @@ CounterSharedPtr AllocatorImpl::makeCounter(StatName name, StatName tag_extracte const StatNameTagVector& stat_name_tags) { Thread::LockGuard lock(mutex_); ASSERT(gauges_.find(name) == gauges_.end()); + ASSERT(text_readouts_.find(name) == text_readouts_.end()); auto iter = counters_.find(name); if (iter != counters_.end()) { return CounterSharedPtr(*iter); @@ -272,6 +273,7 @@ GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, StatName tag_extracted_na Gauge::ImportMode import_mode) { Thread::LockGuard lock(mutex_); ASSERT(counters_.find(name) == counters_.end()); + ASSERT(text_readouts_.find(name) == text_readouts_.end()); auto iter = gauges_.find(name); if (iter != gauges_.end()) { return GaugeSharedPtr(*iter); @@ -285,6 +287,7 @@ GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, StatName tag_extracted_na TextReadoutSharedPtr AllocatorImpl::makeTextReadout(StatName name, StatName tag_extracted_name, const StatNameTagVector& stat_name_tags) { Thread::LockGuard lock(mutex_); + ASSERT(counters_.find(name) == counters_.end()); ASSERT(gauges_.find(name) == gauges_.end()); auto iter = text_readouts_.find(name); if (iter != text_readouts_.end()) { @@ -295,6 +298,7 @@ TextReadoutSharedPtr AllocatorImpl::makeTextReadout(StatName name, StatName tag_ text_readouts_.insert(text_readout.get()); return text_readout; } + bool AllocatorImpl::isMutexLockedForTest() { bool locked = mutex_.tryLock(); if (locked) { diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 17d3983b38cd..135abeb257e4 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -240,16 +240,7 @@ class ThreadLocalStoreImpl : Logger::Loggable<Logger::Id::stats>, public StoreRo } return absl::nullopt; } -#if 0 - Gauge& gauge(const std::string& name) override { return default_scope_->gauge(name); } - TextReadout& textReadout(const std::string& name) override { - return default_scope_->textReadout(name); - } - Histogram& histogram(const std::string& name) override { - return default_scope_->histogram(name); - }; -#endif // >>>>>>> pr5844 // Stats::Store std::vector<CounterSharedPtr> counters() const override; std::vector<GaugeSharedPtr> gauges() const override; diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 4228123acfbe..a3e43e3cfb48 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -811,7 +811,6 @@ TEST_F(StatsMatcherTLSTest, TestExclusionRegex) { Histogram& valid_histogram = store_->histogramFromString("valid_histogram", Stats::Histogram::Unit::Unspecified); - EXPECT_EQ(valid_histogram.name(), "valid_histogram"); Histogram& invalid_histogram_1 = @@ -986,15 +985,15 @@ TEST_F(StatsThreadLocalStoreTest, RemoveRejectedStats) { Gauge& gauge = store_->gaugeFromString("g1", Gauge::ImportMode::Accumulate); Histogram& histogram = store_->histogramFromString("h1", Stats::Histogram::Unit::Unspecified); TextReadout& textReadout = store_->textReadoutFromString("t1"); - ASSERT_EQ(1, store_->counters().size()); // "stats.overflow" and "c1". + ASSERT_EQ(1, store_->counters().size()); // "c1". EXPECT_TRUE(&counter == store_->counters()[0].get() || &counter == store_->counters()[1].get()); // counters() order is non-deterministic. ASSERT_EQ(1, store_->gauges().size()); EXPECT_EQ("g1", store_->gauges()[0]->name()); - ASSERT_EQ(1, store_->textReadouts().size()); - EXPECT_EQ("t1", store_->textReadouts()[0]->name()); ASSERT_EQ(1, store_->histograms().size()); EXPECT_EQ("h1", store_->histograms()[0]->name()); + ASSERT_EQ(1, store_->textReadouts().size()); + EXPECT_EQ("t1", store_->textReadouts()[0]->name()); // Will effectively block all stats, and remove all the non-matching stats. envoy::config::metrics::v3::StatsConfig stats_config; @@ -1005,15 +1004,15 @@ TEST_F(StatsThreadLocalStoreTest, RemoveRejectedStats) { // They can no longer be found. EXPECT_EQ(0, store_->counters().size()); EXPECT_EQ(0, store_->gauges().size()); - EXPECT_EQ(0, store_->textReadouts().size()); EXPECT_EQ(0, store_->histograms().size()); + EXPECT_EQ(0, store_->textReadouts().size()); // However, referencing the previously allocated stats will not crash. counter.inc(); gauge.inc(); - textReadout.set("fortytwo"); EXPECT_CALL(sink_, onHistogramComplete(Ref(histogram), 42)); histogram.recordValue(42); + textReadout.set("fortytwo"); store_->shutdownThreading(); tls_.shutdownThread(); } diff --git a/test/integration/server.h b/test/integration/server.h index 79a4b29ecba4..22f80a09b57b 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -230,14 +230,14 @@ class TestIsolatedStoreImpl : public StoreRoot { Thread::LockGuard lock(lock_); return store_.gauges(); } - std::vector<TextReadoutSharedPtr> textReadouts() const override { - Thread::LockGuard lock(lock_); - return store_.textReadouts(); - } std::vector<ParentHistogramSharedPtr> histograms() const override { Thread::LockGuard lock(lock_); return store_.histograms(); } + std::vector<TextReadoutSharedPtr> textReadouts() const override { + Thread::LockGuard lock(lock_); + return store_.textReadouts(); + } // Stats::StoreRoot void addSink(Sink&) override {} diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index fcf2e9a9175a..56cd283b7e66 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -283,7 +283,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. EXPECT_MEMORY_EQ(m_per_cluster, 44261); - EXPECT_MEMORY_LE(m_per_cluster, 44600); + EXPECT_MEMORY_LE(m_per_cluster, 45000); } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { @@ -339,7 +339,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. EXPECT_MEMORY_EQ(m_per_cluster, 36300); - EXPECT_MEMORY_LE(m_per_cluster, 36800); + EXPECT_MEMORY_LE(m_per_cluster, 37000); } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { diff --git a/test/test_common/utility.h b/test/test_common/utility.h index fc0cd744c569..e66100184743 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -184,7 +184,7 @@ class TestUtility { * Find a counter in a stats store. * @param store supplies the stats store. * @param name supplies the name to search for. - * @return Stats::CounterSharedPtr the counter, or nullptr if there is none. + * @return Stats::CounterSharedPtr the counter or nullptr if there is none. */ static Stats::CounterSharedPtr findCounter(Stats::Store& store, const std::string& name); @@ -192,7 +192,7 @@ class TestUtility { * Find a gauge in a stats store. * @param store supplies the stats store. * @param name supplies the name to search for. - * @return Stats::GaugeSharedPtr the gauge, or nullptr if there is none. + * @return Stats::GaugeSharedPtr the gauge or nullptr if there is none. */ static Stats::GaugeSharedPtr findGauge(Stats::Store& store, const std::string& name); @@ -240,7 +240,7 @@ class TestUtility { * Find a readout in a stats store. * @param store supplies the stats store. * @param name supplies the name to search for. - * @return Stats::TextReadoutSharedPtr the readout, or nullptr if there is none. + * @return Stats::TextReadoutSharedPtr the readout or nullptr if there is none. */ static Stats::TextReadoutSharedPtr findTextReadout(Stats::Store& store, const std::string& name); From 4aceda92f3b5e4c7a03be85b3698e5e32a33e617 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Tue, 7 Apr 2020 13:49:24 -0400 Subject: [PATCH 05/16] Replace hacky bool with hacky enum. Signed-off-by: Misha Efimov <mef@google.com> --- include/envoy/stats/stats.h | 7 +++++++ source/common/stats/isolated_store_impl.cc | 2 +- source/common/stats/isolated_store_impl.h | 10 +++++----- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 32e0a18006b4..1d916c4269b7 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -161,6 +161,13 @@ using GaugeSharedPtr = RefcountPtr<Gauge>; */ class TextReadout : public virtual Metric { public: + // Text readout type is used internally to disambiguate isolated store + // constructors. In the future we can extend it to specify text encoding or + // somesuch. + enum class Type { + Default, // No particular meaning. + }; + virtual ~TextReadout() {} virtual void set(const std::string& value) PURE; diff --git a/source/common/stats/isolated_store_impl.cc b/source/common/stats/isolated_store_impl.cc index dceb080ba963..d9511916e844 100644 --- a/source/common/stats/isolated_store_impl.cc +++ b/source/common/stats/isolated_store_impl.cc @@ -32,7 +32,7 @@ IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table) histograms_([this](StatName name, Histogram::Unit unit) -> HistogramSharedPtr { return HistogramSharedPtr(new HistogramImpl(name, unit, *this, name, StatNameTagVector{})); }), - text_readouts_([this](StatName name, bool) -> TextReadoutSharedPtr { + text_readouts_([this](StatName name, TextReadout::Type) -> TextReadoutSharedPtr { return alloc_.makeTextReadout(name, name, StatNameTagVector{}); }), null_counter_(new NullCounterImpl(symbol_table)), diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index 7a8e7e12f23f..2427e71a1b31 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -29,8 +29,7 @@ template <class Base> class IsolatedStatsCache { using CounterAllocator = std::function<RefcountPtr<Base>(StatName name)>; using GaugeAllocator = std::function<RefcountPtr<Base>(StatName, Gauge::ImportMode)>; using HistogramAllocator = std::function<RefcountPtr<Base>(StatName, Histogram::Unit)>; - using TextReadoutAllocator = - std::function<RefcountPtr<Base>(StatName name, bool /* lowercase */)>; + using TextReadoutAllocator = std::function<RefcountPtr<Base>(StatName name, TextReadout::Type)>; using BaseOptConstRef = absl::optional<std::reference_wrapper<const Base>>; IsolatedStatsCache(CounterAllocator alloc) : counter_alloc_(alloc) {} @@ -71,13 +70,13 @@ template <class Base> class IsolatedStatsCache { return *new_stat; } - Base& get(StatName name, bool b) { + Base& get(StatName name, TextReadout::Type type) { auto stat = stats_.find(name); if (stat != stats_.end()) { return *stat->second; } - RefcountPtr<Base> new_stat = text_readout_alloc_(name, b); + RefcountPtr<Base> new_stat = text_readout_alloc_(name, type); stats_.emplace(new_stat->statName(), new_stat); return *new_stat; } @@ -142,7 +141,8 @@ class IsolatedStoreImpl : public StoreImpl { TextReadout& textReadoutFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef tags) override { TagUtility::TagStatNameJoiner joiner(name, tags, symbolTable()); - TextReadout& text_readout = text_readouts_.get(joiner.nameWithTags(), false); + TextReadout& text_readout = + text_readouts_.get(joiner.nameWithTags(), TextReadout::Type::Default); return text_readout; } CounterOptConstRef findCounter(StatName name) const override { return counters_.find(name); } From e1c71138d41efdbbeb048240c0304bd08fe9114e Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Thu, 9 Apr 2020 10:08:09 -0400 Subject: [PATCH 06/16] Kick CI Signed-off-by: Misha Efimov <mef@google.com> From 1a6bcbbb70c236760e8537c6ccd4fc6e683b9be4 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Thu, 9 Apr 2020 13:30:00 -0400 Subject: [PATCH 07/16] Add support for text readouts to admin stats handler html and json. Signed-off-by: Misha Efimov <mef@google.com> --- source/server/http/admin.cc | 26 ++++++++++++++++++++++++-- source/server/http/admin.h | 1 + test/server/http/admin_test.cc | 22 +++++++++++++++------- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index bea7e6c9716c..ad300c0961b2 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -384,6 +384,7 @@ void AdminImpl::addCircuitSettings(const std::string& cluster_name, const std::s resource_manager.retries().max())); } +// TODO(efimki): Add support of text readouts stats. void AdminImpl::writeClustersAsJson(Buffer::Instance& response) { envoy::admin::v3::Clusters clusters; for (auto& cluster_pair : server_.clusterManager().clusters()) { @@ -461,6 +462,7 @@ void AdminImpl::writeClustersAsJson(Buffer::Instance& response) { response.add(MessageUtil::getJsonStringFromMessage(clusters, true)); // pretty-print } +// TODO(efimki): Add support of text readouts stats. void AdminImpl::writeClustersAsText(Buffer::Instance& response) { for (auto& cluster : server_.clusterManager().clusters()) { addOutlierInfo(cluster.second.get().info()->name(), cluster.second.get().outlierDetector(), @@ -914,11 +916,18 @@ Http::Code AdminImpl::handlerStats(absl::string_view url, Http::ResponseHeaderMa } } + std::map<std::string, std::string> text_readouts; + for (const auto& text_readout : server_.stats().textReadouts()) { + if (shouldShowMetric(*text_readout, used_only, regex)) { + text_readouts.emplace(text_readout->name(), text_readout->value()); + } + } + if (const auto format_value = formatParam(params)) { if (format_value.value() == "json") { response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); - response.add( - AdminImpl::statsAsJson(all_stats, server_.stats().histograms(), used_only, regex)); + response.add(AdminImpl::statsAsJson(all_stats, text_readouts, server_.stats().histograms(), + used_only, regex)); } else if (format_value.value() == "prometheus") { return handlerPrometheusStats(url, response_headers, response, admin_stream); } else { @@ -927,6 +936,10 @@ Http::Code AdminImpl::handlerStats(absl::string_view url, Http::ResponseHeaderMa rc = Http::Code::NotFound; } } else { // Display plain stats if format query param is not there. + for (const auto& text_readout : text_readouts) { + response.add(fmt::format("{}: \"{}\"\n", text_readout.first, + Html::Utility::sanitize(text_readout.second))); + } for (const auto& stat : all_stats) { response.add(fmt::format("{}: {}\n", stat.first, stat.second)); } @@ -988,6 +1001,7 @@ std::string PrometheusStatsFormatter::metricName(const std::string& extracted_na return sanitizeName(fmt::format("envoy_{0}", extracted_name)); } +// TODO(efimki): Add support of text readouts stats. uint64_t PrometheusStatsFormatter::statsAsPrometheus( const std::vector<Stats::CounterSharedPtr>& counters, const std::vector<Stats::GaugeSharedPtr>& gauges, @@ -1062,12 +1076,20 @@ uint64_t PrometheusStatsFormatter::statsAsPrometheus( std::string AdminImpl::statsAsJson(const std::map<std::string, uint64_t>& all_stats, + const std::map<std::string, std::string>& text_readouts, const std::vector<Stats::ParentHistogramSharedPtr>& all_histograms, const bool used_only, const absl::optional<std::regex> regex, const bool pretty_print) { ProtobufWkt::Struct document; std::vector<ProtobufWkt::Value> stats_array; + for (const auto& text_readout : text_readouts) { + ProtobufWkt::Struct stat_obj; + auto* stat_obj_fields = stat_obj.mutable_fields(); + (*stat_obj_fields)["name"] = ValueUtil::stringValue(text_readout.first); + (*stat_obj_fields)["value"] = ValueUtil::stringValue(text_readout.second); + stats_array.push_back(ValueUtil::structValue(stat_obj)); + } for (const auto& stat : all_stats) { ProtobufWkt::Struct stat_obj; auto* stat_obj_fields = stat_obj.mutable_fields(); diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 82332658de56..c2de4c04a8ea 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -281,6 +281,7 @@ class AdminImpl : public Admin, (!regex.has_value() || std::regex_search(metric.name(), regex.value()))); } static std::string statsAsJson(const std::map<std::string, uint64_t>& all_stats, + const std::map<std::string, std::string>& text_readouts, const std::vector<Stats::ParentHistogramSharedPtr>& all_histograms, bool used_only, const absl::optional<std::regex> regex = absl::nullopt, diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 3099de1ebf93..c4d35b416002 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -67,9 +67,10 @@ class AdminStatsTest : public testing::TestWithParam<Network::Address::IpVersion static std::string statsAsJsonHandler(std::map<std::string, uint64_t>& all_stats, + std::map<std::string, std::string>& all_text_readouts, const std::vector<Stats::ParentHistogramSharedPtr>& all_histograms, const bool used_only, const absl::optional<std::regex> regex = absl::nullopt) { - return AdminImpl::statsAsJson(all_stats, all_histograms, used_only, regex, + return AdminImpl::statsAsJson(all_stats, all_text_readouts, all_histograms, used_only, regex, true /*pretty_print*/); } @@ -112,7 +113,8 @@ TEST_P(AdminStatsTest, StatsAsJson) { [](const Stats::ParentHistogramSharedPtr& a, const Stats::ParentHistogramSharedPtr& b) -> bool { return a->name() < b->name(); }); std::map<std::string, uint64_t> all_stats; - std::string actual_json = statsAsJsonHandler(all_stats, histograms, false); + std::map<std::string, std::string> all_text_readouts; + std::string actual_json = statsAsJsonHandler(all_stats, all_text_readouts, histograms, false); const std::string expected_json = R"EOF({ "stats": [ @@ -254,7 +256,9 @@ TEST_P(AdminStatsTest, UsedOnlyStatsAsJson) { store_->mergeHistograms([]() -> void {}); std::map<std::string, uint64_t> all_stats; - std::string actual_json = statsAsJsonHandler(all_stats, store_->histograms(), true); + std::map<std::string, std::string> all_text_readouts; + std::string actual_json = + statsAsJsonHandler(all_stats, all_text_readouts, store_->histograms(), true); // Expected JSON should not have h2 values as it is not used. const std::string expected_json = R"EOF({ @@ -352,8 +356,10 @@ TEST_P(AdminStatsTest, StatsAsJsonFilterString) { store_->mergeHistograms([]() -> void {}); std::map<std::string, uint64_t> all_stats; - std::string actual_json = statsAsJsonHandler(all_stats, store_->histograms(), false, - absl::optional<std::regex>{std::regex("[a-z]1")}); + std::map<std::string, std::string> all_text_readouts; + std::string actual_json = + statsAsJsonHandler(all_stats, all_text_readouts, store_->histograms(), false, + absl::optional<std::regex>{std::regex("[a-z]1")}); // Because this is a filter case, we don't expect to see any stats except for those containing // "h1" in their name. @@ -461,8 +467,10 @@ TEST_P(AdminStatsTest, UsedOnlyStatsAsJsonFilterString) { store_->mergeHistograms([]() -> void {}); std::map<std::string, uint64_t> all_stats; - std::string actual_json = statsAsJsonHandler(all_stats, store_->histograms(), true, - absl::optional<std::regex>{std::regex("h[12]")}); + std::map<std::string, std::string> all_text_readouts; + std::string actual_json = + statsAsJsonHandler(all_stats, all_text_readouts, store_->histograms(), true, + absl::optional<std::regex>{std::regex("h[12]")}); // Expected JSON should not have h2 values as it is not used, and should not have h3 values as // they are used but do not match. From 89626800e036680c837271c5c7a34187542aef97 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Fri, 10 Apr 2020 18:41:16 -0400 Subject: [PATCH 08/16] Address comments. Signed-off-by: Misha Efimov <mef@google.com> --- include/envoy/stats/stats.h | 2 +- source/common/stats/allocator_impl.cc | 6 +++--- source/common/stats/null_text_readout.h | 2 +- test/mocks/stats/mocks.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 4cc287d90a46..1295850859b1 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -170,7 +170,7 @@ class TextReadout : public virtual Metric { virtual ~TextReadout() {} - virtual void set(const std::string& value) PURE; + virtual void set(std::string&& value) PURE; virtual std::string value() const PURE; }; diff --git a/source/common/stats/allocator_impl.cc b/source/common/stats/allocator_impl.cc index c4cb9121b934..06db3ee37f52 100644 --- a/source/common/stats/allocator_impl.cc +++ b/source/common/stats/allocator_impl.cc @@ -240,9 +240,9 @@ class TextReadoutImpl : public StatsSharedImpl<TextReadout> { } // Stats::TextReadout - void set(const std::string& value) override { + void set(std::string&& value) override { absl::MutexLock lock(&mutex_); - value_ = value; + value_ = std::move(value); } std::string value() const override { absl::MutexLock lock(&mutex_); @@ -251,7 +251,7 @@ class TextReadoutImpl : public StatsSharedImpl<TextReadout> { private: mutable absl::Mutex mutex_; - std::string value_; + std::string value_ ABSL_GUARDED_BY(mutex_); }; CounterSharedPtr AllocatorImpl::makeCounter(StatName name, StatName tag_extracted_name, diff --git a/source/common/stats/null_text_readout.h b/source/common/stats/null_text_readout.h index 3ee402ee993a..d3e9cc832e6b 100644 --- a/source/common/stats/null_text_readout.h +++ b/source/common/stats/null_text_readout.h @@ -23,7 +23,7 @@ class NullTextReadoutImpl : public MetricImpl<TextReadout> { MetricImpl::clear(symbol_table_); } - void set(const std::string&) override {} + void set(std::string&&) override {} std::string value() const override { return std::string(); } // Metric diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 2ac733da3daf..c79ca1cf2983 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -250,7 +250,7 @@ class MockTextReadout : public MockMetric<TextReadout> { MockTextReadout(); ~MockTextReadout(); - MOCK_METHOD1(set, void(const std::string& value)); + MOCK_METHOD1(set, void(std::string&& value)); MOCK_CONST_METHOD0(used, bool()); MOCK_CONST_METHOD0(value, std::string()); From ee17a7357031c9b226894b8fa0d70223e29d6cbd Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Fri, 10 Apr 2020 20:00:57 -0400 Subject: [PATCH 09/16] Fix the test. Signed-off-by: Misha Efimov <mef@google.com> --- test/common/stats/thread_local_store_test.cc | 4 ++-- test/integration/stats_integration_test.cc | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index ed13253da820..0bbccae4985d 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -536,7 +536,7 @@ TEST_F(StatsThreadLocalStoreTest, TextReadoutAllLengths) { // ASCII for (int i = 0; i < 15; i++) { str += ('a' + i); - t.set(str); + t.set(std::string(str)); EXPECT_EQ(str, t.value()); } @@ -544,7 +544,7 @@ TEST_F(StatsThreadLocalStoreTest, TextReadoutAllLengths) { str = ""; for (int i = 0; i < 15; i++) { str += ('\xEE' + i); - t.set(str); + t.set(std::string(str)); EXPECT_EQ(str, t.value()); } diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index c08092a4b47d..0ad400465f76 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -284,8 +284,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 43349); - EXPECT_MEMORY_LE(m_per_cluster, 44000); + EXPECT_MEMORY_EQ(m_per_cluster, 44023); + EXPECT_MEMORY_LE(m_per_cluster, 44100); } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { @@ -342,8 +342,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 35557); - EXPECT_MEMORY_LE(m_per_cluster, 36000); + EXPECT_MEMORY_EQ(m_per_cluster, 36232); + EXPECT_MEMORY_LE(m_per_cluster, 36300); } TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { @@ -375,6 +375,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { // 2020/02/13 10042 1363 1655 Metadata object are shared across different clusters // and hosts. // 2020/04/02 10624 1380 1655 Use 100 clusters rather than 1000 to avoid timeouts + // 2020/04/10 10639 1412 1655 Implement TextReadout stats. // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -384,7 +385,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_host, 1380); + EXPECT_MEMORY_EQ(m_per_host, 1412); EXPECT_MEMORY_LE(m_per_host, 1655); } From 8e0b860a5d44f36c65fd45709eb5e12e819698d9 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Fri, 10 Apr 2020 20:12:44 -0400 Subject: [PATCH 10/16] Add doc to TextReadout interface definition. Signed-off-by: Misha Efimov <mef@google.com> --- include/envoy/stats/stats.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 1295850859b1..996d554a5c4f 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -170,7 +170,14 @@ class TextReadout : public virtual Metric { virtual ~TextReadout() {} + /** + * Sets the value of this TextReadout by moving the input |value| to minimize + * buffer copies under the lock. + */ virtual void set(std::string&& value) PURE; + /** + * @return the copy of this TextReadout value. + */ virtual std::string value() const PURE; }; From 285046f0d3412699bc344d50ed2e20bc73a09141 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Mon, 13 Apr 2020 15:47:05 -0400 Subject: [PATCH 11/16] Fix memory stats to match presubmit, update doc. Signed-off-by: Misha Efimov <mef@google.com> --- source/docs/stats.md | 4 +++- test/integration/stats_integration_test.cc | 7 +++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/source/docs/stats.md b/source/docs/stats.md index 023e865a0a3a..43be6992146c 100644 --- a/source/docs/stats.md +++ b/source/docs/stats.md @@ -8,6 +8,8 @@ binary program restarts. The metrics are tracked as: * Histograms: mapping ranges of values to frequency. The ranges are auto-adjusted as data accumulates. Unlike counters and gauges, histogram data is not retained across binary program restarts. + * TextReadouts: Unicode strings. Unlike counters and gauges, text readout data + is not retained across binary program restarts. In order to support restarting the Envoy binary program without losing counter and gauge values, they are passed from parent to child in an RPC protocol. @@ -79,7 +81,7 @@ followed. Stat names are replicated in several places in various forms. - * Held with the stat values, in `CounterImpl` and `GaugeImpl`, which are defined in + * Held with the stat values, in `CounterImpl`, `GaugeImpl` and `TextReadoutImpl`, which are defined in [allocator_impl.cc](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/allocator_impl.cc) * In [MetricImpl](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/metric_impl.h) in a transformed state, with tags extracted into vectors of name/value strings. diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 0ad400465f76..75012bd02116 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -284,7 +284,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 44023); + EXPECT_MEMORY_EQ(m_per_cluster, 43993); EXPECT_MEMORY_LE(m_per_cluster, 44100); } @@ -342,7 +342,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 36232); + EXPECT_MEMORY_EQ(m_per_cluster, 36201); EXPECT_MEMORY_LE(m_per_cluster, 36300); } @@ -375,7 +375,6 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { // 2020/02/13 10042 1363 1655 Metadata object are shared across different clusters // and hosts. // 2020/04/02 10624 1380 1655 Use 100 clusters rather than 1000 to avoid timeouts - // 2020/04/10 10639 1412 1655 Implement TextReadout stats. // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -385,7 +384,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeHostSizeWithStats) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_host, 1412); + EXPECT_MEMORY_EQ(m_per_host, 1380); EXPECT_MEMORY_LE(m_per_host, 1655); } From 39f393b58d84ad12538cb58684f4da18a4330f47 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Wed, 15 Apr 2020 12:57:49 -0400 Subject: [PATCH 12/16] Added TODO in stats sinks, addressed nits. Signed-off-by: Misha Efimov <mef@google.com> --- include/envoy/stats/scope.h | 1 + include/envoy/stats/sink.h | 1 + source/extensions/stat_sinks/common/statsd/statsd.cc | 2 ++ tools/code_format/check_format.py | 5 +++-- 4 files changed, 7 insertions(+), 2 deletions(-) diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index e5a9632d74ec..408655bbb8a5 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -146,6 +146,7 @@ class Scope { TextReadout& textReadoutFromStatName(const StatName& name) { return textReadoutFromStatNameWithTags(name, absl::nullopt); } + /** * Creates a TextReadout from the stat name and tags. If tags are not provided, tag extraction * will be performed on the name. diff --git a/include/envoy/stats/sink.h b/include/envoy/stats/sink.h index d58ea33220fa..f0ce08c1dd05 100644 --- a/include/envoy/stats/sink.h +++ b/include/envoy/stats/sink.h @@ -35,6 +35,7 @@ class MetricSnapshot { * @return a snapshot of all histograms. */ virtual const std::vector<std::reference_wrapper<const ParentHistogram>>& histograms() PURE; + // TODO(efimki): Add support of text readouts stats. }; /** diff --git a/source/extensions/stat_sinks/common/statsd/statsd.cc b/source/extensions/stat_sinks/common/statsd/statsd.cc index 5782935fb751..b4676d290155 100644 --- a/source/extensions/stat_sinks/common/statsd/statsd.cc +++ b/source/extensions/stat_sinks/common/statsd/statsd.cc @@ -62,6 +62,7 @@ void UdpStatsdSink::flush(Stats::MetricSnapshot& snapshot) { buildTagStr(gauge.get().tags()))); } } + // TODO(efimki): Add support of text readouts stats. } void UdpStatsdSink::onHistogramComplete(const Stats::Histogram& histogram, uint64_t value) { @@ -128,6 +129,7 @@ void TcpStatsdSink::flush(Stats::MetricSnapshot& snapshot) { tls_sink.flushGauge(gauge.get().name(), gauge.get().value()); } } + // TODO(efimki): Add support of text readouts stats. tls_sink.endFlush(true); } diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 8f02239e9d6d..6ba7d1841403 100755 --- a/tools/code_format/check_format.py +++ b/tools/code_format/check_format.py @@ -647,8 +647,9 @@ def checkSourceLine(line, file_path, reportError): if isInSubdir(file_path, 'source') and file_path.endswith('.cc') and \ ('.counterFromString(' in line or '.gaugeFromString(' in line or \ - '.histogramFromString(' in line or '->counterFromString(' in line or \ - '->gaugeFromString(' in line or '->histogramFromString(' in line): + '.histogramFromString(' in line or '.textReadoutFromString(' in line or \ + '->counterFromString(' in line or '->gaugeFromString(' in line or \ + '->histogramFromString(' in line or '->textReadoutFromString(' in line): reportError("Don't lookup stats by name at runtime; use StatName saved during construction") if re.search("envoy::[a-z0-9_:]+::[A-Z][a-z]\w*_\w*_[A-Z]{2}", line): From 473a995cc551bf1390ca82b1d5f5a31027e24907 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Wed, 15 Apr 2020 15:15:45 -0400 Subject: [PATCH 13/16] Updated comment to remove allocation failure. Signed-off-by: Misha Efimov <mef@google.com> --- include/envoy/stats/allocator.h | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/include/envoy/stats/allocator.h b/include/envoy/stats/allocator.h index 646b47c29b41..f6181553ad85 100644 --- a/include/envoy/stats/allocator.h +++ b/include/envoy/stats/allocator.h @@ -32,8 +32,7 @@ class Allocator { * @param name the full name of the stat. * @param tag_extracted_name the name of the stat with tag-values stripped out. * @param tags the tag values. - * @return CounterSharedPtr a counter, or nullptr if allocation failed, in which case - * tag_extracted_name and tags are not moved. + * @return CounterSharedPtr a counter. */ virtual CounterSharedPtr makeCounter(StatName name, StatName tag_extracted_name, const StatNameTagVector& stat_name_tags) PURE; @@ -42,8 +41,7 @@ class Allocator { * @param name the full name of the stat. * @param tag_extracted_name the name of the stat with tag-values stripped out. * @param stat_name_tags the tag values. - * @return GaugeSharedPtr a gauge, or nullptr if allocation failed, in which case - * tag_extracted_name and tags are not moved. + * @return GaugeSharedPtr a gauge. */ virtual GaugeSharedPtr makeGauge(StatName name, StatName tag_extracted_name, const StatNameTagVector& stat_name_tags, @@ -53,8 +51,7 @@ class Allocator { * @param name the full name of the stat. * @param tag_extracted_name the name of the stat with tag-values stripped out. * @param tags the tag values. - * @return TextReadoutSharedPtr a text readout, or nullptr if allocation failed, in which case - * tag_extracted_name and tags are not moved. + * @return TextReadoutSharedPtr a text readout. */ virtual TextReadoutSharedPtr makeTextReadout(StatName name, StatName tag_extracted_name, const StatNameTagVector& stat_name_tags) PURE; From 8ef123f5e30dbdee036c115380561d79c2adb29b Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Wed, 15 Apr 2020 20:18:19 -0400 Subject: [PATCH 14/16] Fix clang-tidy errors. Signed-off-by: Misha Efimov <mef@google.com> --- include/envoy/stats/stats.h | 2 +- test/mocks/stats/mocks.cc | 2 +- test/mocks/stats/mocks.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 996d554a5c4f..52463d2ee94b 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -168,7 +168,7 @@ class TextReadout : public virtual Metric { Default, // No particular meaning. }; - virtual ~TextReadout() {} + virtual ~TextReadout() override = default; /** * Sets the value of this TextReadout by moving the input |value| to minimize diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index c53435222ced..adeed55eeb5e 100644 --- a/test/mocks/stats/mocks.cc +++ b/test/mocks/stats/mocks.cc @@ -34,7 +34,7 @@ MockTextReadout::MockTextReadout() { ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_)); ON_CALL(*this, value()).WillByDefault(ReturnPointee(&value_)); } -MockTextReadout::~MockTextReadout() {} +MockTextReadout::~MockTextReadout() = default; MockHistogram::MockHistogram() { ON_CALL(*this, unit()).WillByDefault(ReturnPointee(&unit_)); diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index c79ca1cf2983..06a81e7c8cf5 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -248,7 +248,7 @@ class MockParentHistogram : public MockMetric<ParentHistogram> { class MockTextReadout : public MockMetric<TextReadout> { public: MockTextReadout(); - ~MockTextReadout(); + ~MockTextReadout() override; MOCK_METHOD1(set, void(std::string&& value)); MOCK_CONST_METHOD0(used, bool()); From db9064a7339da47eec52b7d87fad427b11453545 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Thu, 16 Apr 2020 09:48:26 -0400 Subject: [PATCH 15/16] Remove extra virtual. Signed-off-by: Misha Efimov <mef@google.com> --- include/envoy/stats/stats.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 52463d2ee94b..c723152ab549 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -168,7 +168,7 @@ class TextReadout : public virtual Metric { Default, // No particular meaning. }; - virtual ~TextReadout() override = default; + ~TextReadout() override = default; /** * Sets the value of this TextReadout by moving the input |value| to minimize From 2e8a020271da2f0fdee22134363ad3fab4926804 Mon Sep 17 00:00:00 2001 From: Misha Efimov <mef@google.com> Date: Thu, 16 Apr 2020 17:04:54 -0400 Subject: [PATCH 16/16] Kick CI Signed-off-by: Misha Efimov <mef@google.com>