diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index eb3dbf0fa896d..d462231368d5d 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -49,7 +49,7 @@ class Scope { virtual Counter& counterFromStatName(StatName name) PURE; /** - * TODO(jmarantz): this variant is deprecated: use counterFromStatName. + * TODO(#6667): this variant is deprecated: use counterFromStatName. * @param name The name, expressed as a string. * @return a counter within the scope's namespace. */ @@ -62,7 +62,7 @@ class Scope { virtual Gauge& gaugeFromStatName(StatName name) PURE; /** - * TODO(jmarantz): this variant is deprecated: use gaugeFromStatName. + * TODO(#6667): this variant is deprecated: use gaugeFromStatName. * @param name The name, expressed as a string. * @return a gauge within the scope's namespace. */ @@ -80,7 +80,7 @@ class Scope { virtual Histogram& histogramFromStatName(StatName name) PURE; /** - * TODO(jmarantz): this variant is deprecated: use histogramFromStatName. + * TODO(#6667): this variant is deprecated: use histogramFromStatName. * @param name The name, expressed as a string. * @return a histogram within the scope's namespace with a particular value type. */ diff --git a/include/envoy/stats/stat_data_allocator.h b/include/envoy/stats/stat_data_allocator.h index b708ffe2fb48b..625dc36e49e6b 100644 --- a/include/envoy/stats/stat_data_allocator.h +++ b/include/envoy/stats/stat_data_allocator.h @@ -35,8 +35,8 @@ class StatDataAllocator { * @return CounterSharedPtr a counter, or nullptr if allocation failed, in which case * tag_extracted_name and tags are not moved. */ - virtual CounterSharedPtr makeCounter(absl::string_view name, std::string&& tag_extracted_name, - std::vector&& tags) PURE; + virtual CounterSharedPtr makeCounter(StatName name, absl::string_view tag_extracted_name, + const std::vector& tags) PURE; /** * @param name the full name of the stat. @@ -45,8 +45,8 @@ class StatDataAllocator { * @return GaugeSharedPtr a gauge, or nullptr if allocation failed, in which case * tag_extracted_name and tags are not moved. */ - virtual GaugeSharedPtr makeGauge(absl::string_view name, std::string&& tag_extracted_name, - std::vector&& tags) PURE; + virtual GaugeSharedPtr makeGauge(StatName name, absl::string_view tag_extracted_name, + const std::vector& tags) PURE; /** * Determines whether this stats allocator requires bounded stat-name size. diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 6d191bf24f9fb..1289d93bd1b02 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -6,12 +6,14 @@ #include #include "envoy/common/pure.h" +#include "envoy/stats/symbol_table.h" #include "absl/strings/string_view.h" namespace Envoy { namespace Stats { +class StatDataAllocator; struct Tag; /** @@ -32,32 +34,39 @@ class Metric { virtual std::string name() const PURE; /** - * Returns the full name of the Metric as a nul-terminated string. The - * intention is use this as a hash-map key, so that the stat name storage - * is not duplicated in every map. You cannot use name() above for this, - * as it returns a std::string by value, as not all stat implementations - * contain the name as a std::string. - * - * Note that in the future, the plan is to replace this method with one that - * returns a reference to a symbolized representation of the elaborated string - * (see source/common/stats/symbol_table_impl.h). + * Returns the full name of the Metric as an encoded array of symbols. */ - virtual const char* nameCStr() const PURE; + virtual StatName statName() const PURE; /** * Returns a vector of configurable tags to identify this Metric. */ - virtual const std::vector& tags() const PURE; + virtual std::vector tags() const PURE; + + /** + * Returns the name of the Metric with the portions designated as tags removed + * as a string. For example, The stat name "vhost.foo.vcluster.bar.c1" would + * have "foo" extracted as the value of tag "vhost" and "bar" extracted as the + * value of tag "vcluster". Thus the tagExtractedName is simply + * "vhost.vcluster.c1". + * + * @return The stat name with all tag values extracted. + */ + virtual std::string tagExtractedName() const PURE; /** - * Returns the name of the Metric with the portions designated as tags removed. + * Returns the name of the Metric with the portions designated as tags + * removed as a StatName */ - virtual const std::string& tagExtractedName() const PURE; + virtual StatName tagExtractedStatName() const PURE; /** * Indicates whether this metric has been updated since the server was started. */ virtual bool used() const PURE; + + virtual SymbolTable& symbolTable() PURE; + virtual const SymbolTable& symbolTable() const PURE; }; /** diff --git a/include/envoy/stats/tag_extractor.h b/include/envoy/stats/tag_extractor.h index 270b782386333..361cda3e22660 100644 --- a/include/envoy/stats/tag_extractor.h +++ b/include/envoy/stats/tag_extractor.h @@ -40,7 +40,7 @@ class TagExtractor { * @param remove_characters set of intervals of character-indices to be removed from name. * @return bool indicates whether a tag was found in the name. */ - virtual bool extractTag(const std::string& stat_name, std::vector& tags, + virtual bool extractTag(absl::string_view stat_name, std::vector& tags, IntervalSet& remove_characters) const PURE; /** diff --git a/include/envoy/stats/tag_producer.h b/include/envoy/stats/tag_producer.h index 3e7986cd69d5e..5ec72877981bc 100644 --- a/include/envoy/stats/tag_producer.h +++ b/include/envoy/stats/tag_producer.h @@ -7,6 +7,8 @@ #include "envoy/common/pure.h" #include "envoy/stats/tag.h" +#include "absl/strings/string_view.h" + namespace Envoy { namespace Stats { @@ -16,12 +18,18 @@ class TagProducer { /** * Take a metric name and a vector then add proper tags into the vector and - * return an extracted metric name. + * return an extracted metric name. The tags array will be populated with + * name/value pairs extracted from the full metric name, using the regular + * expressions in source/common/config/well_known_names.cc. For example, the + * stat name "vhost.foo.vcluster.bar.c1" would have "foo" extracted as the + * value of tag "vhost" and "bar" extracted as the value of tag + * "vcluster", so this will populate tags with {"vhost", "foo"} and + * {"vcluster", "bar"}, and return "vhost.vcluster.c1". + * * @param metric_name std::string a name of Stats::Metric (Counter, Gauge, Histogram). * @param tags std::vector a set of Stats::Tag. */ - virtual std::string produceTags(const std::string& metric_name, - std::vector& tags) const PURE; + virtual std::string produceTags(absl::string_view metric_name, std::vector& tags) const PURE; }; typedef std::unique_ptr TagProducerPtr; diff --git a/source/common/http/BUILD b/source/common/http/BUILD index b947d3b5293d4..503a1d4c4e0fc 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -89,6 +89,7 @@ envoy_cc_library( "//include/envoy/stats:stats_interface", "//source/common/common:enum_to_int", "//source/common/common:utility_lib", + "//source/common/stats:symbol_table_lib", "@envoy_api//envoy/type:http_status_cc", ], ) diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index 40d5a43d11cb4..0dd466977fc98 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -56,6 +56,7 @@ envoy_cc_library( envoy_cc_library( name = "metric_impl_lib", + srcs = ["metric_impl.cc"], hdrs = ["metric_impl.h"], deps = [ ":symbol_table_lib", diff --git a/source/common/stats/heap_stat_data.cc b/source/common/stats/heap_stat_data.cc index 0ac82c3445391..42d48967a6087 100644 --- a/source/common/stats/heap_stat_data.cc +++ b/source/common/stats/heap_stat_data.cc @@ -1,35 +1,43 @@ #include "common/stats/heap_stat_data.h" #include "common/common/lock_guard.h" +#include "common/common/logger.h" #include "common/common/thread.h" #include "common/common/utility.h" namespace Envoy { namespace Stats { -HeapStatData::HeapStatData(absl::string_view key) { - StringUtil::strlcpy(name_, key.data(), key.size() + 1); +HeapStatDataAllocator::~HeapStatDataAllocator() { ASSERT(stats_.empty()); } + +HeapStatData* HeapStatData::alloc(StatName stat_name, SymbolTable& symbol_table) { + void* memory = ::malloc(sizeof(HeapStatData) + stat_name.size()); + ASSERT(memory); + symbol_table.incRefCount(stat_name); + return new (memory) HeapStatData(stat_name); } -HeapStatDataAllocator::~HeapStatDataAllocator() { ASSERT(stats_.empty()); } +void HeapStatData::free(SymbolTable& symbol_table) { + symbol_table.free(statName()); + this->~HeapStatData(); + ::free(this); // matches malloc() call above. +} -HeapStatData* HeapStatDataAllocator::alloc(absl::string_view name) { - // Any expected truncation of name is done at the callsite. No truncation is - // required to use this allocator. Note that data must be freed by calling - // its free() method, and not by destruction, thus the more complex use of - // unique_ptr. - std::unique_ptr> data( - HeapStatData::alloc(name), [](HeapStatData* d) { d->free(); }); +HeapStatData& HeapStatDataAllocator::alloc(StatName name) { + using HeapStatDataFreeFn = std::function; + std::unique_ptr data_ptr( + HeapStatData::alloc(name, symbolTable()), + [this](HeapStatData* d) { d->free(symbolTable()); }); Thread::ReleasableLockGuard lock(mutex_); - auto ret = stats_.insert(data.get()); + auto ret = stats_.insert(data_ptr.get()); HeapStatData* existing_data = *ret.first; lock.release(); if (ret.second) { - return data.release(); + return *data_ptr.release(); } ++existing_data->ref_count_; - return existing_data; + return *existing_data; } void HeapStatDataAllocator::free(HeapStatData& data) { @@ -44,19 +52,17 @@ void HeapStatDataAllocator::free(HeapStatData& data) { ASSERT(key_removed == 1); } - data.free(); + data.free(symbolTable()); } -HeapStatData* HeapStatData::alloc(absl::string_view name) { - void* memory = ::malloc(sizeof(HeapStatData) + name.size() + 1); - ASSERT(memory); - return new (memory) HeapStatData(name); -} - -void HeapStatData::free() { - this->~HeapStatData(); - ::free(this); // matches malloc() call above. +#ifndef ENVOY_CONFIG_COVERAGE +void HeapStatDataAllocator::debugPrint() { + Thread::LockGuard lock(mutex_); + for (HeapStatData* heap_stat_data : stats_) { + ENVOY_LOG_MISC(info, "{}", symbolTable().toString(heap_stat_data->statName())); + } } +#endif template class StatDataAllocatorImpl; diff --git a/source/common/stats/heap_stat_data.h b/source/common/stats/heap_stat_data.h index 799896eb3d4a6..e0636b6db492a 100644 --- a/source/common/stats/heap_stat_data.h +++ b/source/common/stats/heap_stat_data.h @@ -4,12 +4,15 @@ #include #include +#include "envoy/stats/stats.h" #include "envoy/stats/symbol_table.h" #include "common/common/hash.h" #include "common/common/thread.h" #include "common/common/thread_annotations.h" +#include "common/stats/metric_impl.h" #include "common/stats/stat_data_allocator_impl.h" +#include "common/stats/symbol_table_impl.h" #include "absl/container/flat_hash_set.h" @@ -21,71 +24,92 @@ namespace Stats { * so that it can be allocated efficiently from the heap on demand. */ struct HeapStatData { - /** - * @returns absl::string_view the name as a string_view. - */ - absl::string_view key() const { return name_; } +private: + explicit HeapStatData(StatName stat_name) { stat_name.copyToStorage(symbol_storage_); } + +public: + static HeapStatData* alloc(StatName stat_name, SymbolTable& symbol_table); - /** - * @returns std::string the name as a const char*. - */ - const char* name() const { return name_; } + void free(SymbolTable& symbol_table); + StatName statName() const { return StatName(symbol_storage_); } - static HeapStatData* alloc(absl::string_view name); - void free(); + bool operator==(const HeapStatData& rhs) const { return statName() == rhs.statName(); } + uint64_t hash() const { return statName().hash(); } std::atomic value_{0}; std::atomic pending_increment_{0}; std::atomic flags_{0}; std::atomic ref_count_{1}; - char name_[]; + SymbolTable::Storage symbol_storage_; +}; -private: - /** - * You cannot construct/destruct a HeapStatData directly with new/delete as - * it's variable-size. Use alloc()/free() methods above. - */ - explicit HeapStatData(absl::string_view name); - ~HeapStatData() {} +template class HeapStat : public Stat { +public: + HeapStat(HeapStatData& data, StatDataAllocatorImpl& alloc, + absl::string_view tag_extracted_name, const std::vector& tags) + : Stat(data, alloc, tag_extracted_name, tags) {} + + StatName statName() const override { return this->data_.statName(); } }; -/** - * Implementation of StatDataAllocator using a pure heap-based strategy, so that - * Envoy implementations that do not require hot-restart can use less memory. - */ +// Partially implements a StatDataAllocator, leaving alloc & free for subclasses. +// We templatize on StatData rather than defining a virtual base StatData class +// for performance reasons; stat increment is on the hot path. +// +// The two production derivations cover using a fixed block of shared-memory for +// hot restart stat continuity, and heap allocation for more efficient RAM usage +// for when hot-restart is not required. +// +// Also note that RawStatData needs to live in a shared memory block, and it's +// possible, but not obvious, that a vptr would be usable across processes. In +// any case, RawStatData is allocated from a shared-memory block rather than via +// new, so the usual C++ compiler assistance for setting up vptrs will not be +// available. This could be resolved with placed new, or another nesting level. class HeapStatDataAllocator : public StatDataAllocatorImpl { public: HeapStatDataAllocator(SymbolTable& symbol_table) : StatDataAllocatorImpl(symbol_table) {} ~HeapStatDataAllocator() override; - // StatDataAllocatorImpl - HeapStatData* alloc(absl::string_view name) override; + HeapStatData& alloc(StatName name); void free(HeapStatData& data) override; // StatDataAllocator bool requiresBoundedStatNameSize() const override { return false; } + CounterSharedPtr makeCounter(StatName name, absl::string_view tag_extracted_name, + const std::vector& tags) override { + return std::make_shared>>(alloc(name), *this, + tag_extracted_name, tags); + } + + GaugeSharedPtr makeGauge(StatName name, absl::string_view tag_extracted_name, + const std::vector& tags) override { + return std::make_shared>>(alloc(name), *this, + tag_extracted_name, tags); + } + +#ifndef ENVOY_CONFIG_COVERAGE + void debugPrint(); +#endif + private: struct HeapStatHash { - size_t operator()(const HeapStatData* a) const { return HashUtil::xxHash64(a->key()); } + size_t operator()(const HeapStatData* a) const { return a->hash(); } }; struct HeapStatCompare { - bool operator()(const HeapStatData* a, const HeapStatData* b) const { - return (a->key() == b->key()); - } + bool operator()(const HeapStatData* a, const HeapStatData* b) const { return *a == *b; } }; - // TODO(jmarantz): See https://github.com/envoyproxy/envoy/pull/3927 and - // https://github.com/envoyproxy/envoy/issues/3585, which can help reorganize - // the heap stats using a ref-counted symbol table to compress the stat strings. - using StatSet = absl::flat_hash_set; - // An unordered set of HeapStatData pointers which keys off the key() - // field in each object. This necessitates a custom comparator and hasher. + // field in each object. This necessitates a custom comparator and hasher, which key off of the + // StatNamePtr's own StatNamePtrHash and StatNamePtrCompare operators. + using StatSet = absl::flat_hash_set; StatSet stats_ GUARDED_BY(mutex_); - // A mutex is needed here to protect the stats_ object from both alloc() and free() operations. - // Although alloc() operations are called under existing locking, free() operations are made from - // the destructors of the individual stat objects, which are not protected by locks. + + // A mutex is needed here to protect both the stats_ object from both + // alloc() and free() operations. Although alloc() operations are called under existing locking, + // free() operations are made from the destructors of the individual stat objects, which are not + // protected by locks. Thread::MutexBasicLockable mutex_; }; diff --git a/source/common/stats/histogram_impl.h b/source/common/stats/histogram_impl.h index b3fc06141c1ae..5fceb00687ff0 100644 --- a/source/common/stats/histogram_impl.h +++ b/source/common/stats/histogram_impl.h @@ -52,40 +52,56 @@ class HistogramStatisticsImpl : public HistogramStatistics, NonCopyable { */ class HistogramImpl : public Histogram, public MetricImpl { public: - HistogramImpl(const std::string& name, Store& parent, std::string&& tag_extracted_name, - std::vector&& tags) - : MetricImpl(std::move(tag_extracted_name), std::move(tags)), parent_(parent), name_(name) {} - - // Stats:;Metric - std::string name() const override { return name_; } - const char* nameCStr() const override { return name_.c_str(); } + HistogramImpl(StatName name, Store& parent, const std::string& tag_extracted_name, + const std::vector& tags) + : MetricImpl(tag_extracted_name, tags, parent.symbolTable()), + name_(name, parent.symbolTable()), parent_(parent) {} + ~HistogramImpl() { + // We must explicitly free the StatName here using the SymbolTable reference + // we access via parent_. A pure RAII alternative would be to use + // StatNameManagedStorage rather than StatNameStorage, which will cost a total + // of 16 bytes per stat, counting the extra SymbolTable& reference here, + // plus the extra SymbolTable& reference in MetricImpl. + name_.free(symbolTable()); + + // We must explicitly free the StatName here in order to supply the + // SymbolTable reference. An RAII alternative would be to store a + // reference to the SymbolTable in MetricImpl, which would cost 8 bytes + // per stat. + MetricImpl::clear(); + } // Stats::Histogram void recordValue(uint64_t value) override { parent_.deliverHistogramToSinks(*this, value); } bool used() const override { return true; } + StatName statName() const override { return name_.statName(); } + const SymbolTable& symbolTable() const override { return parent_.symbolTable(); } + SymbolTable& symbolTable() override { return parent_.symbolTable(); } private: + StatNameStorage name_; + // This is used for delivering the histogram data to sinks. Store& parent_; - - const std::string name_; }; /** * Null histogram implementation. * No-ops on all calls and requires no underlying metric or data. */ -class NullHistogramImpl : public Histogram { +class NullHistogramImpl : public Histogram, NullMetricImpl { public: - NullHistogramImpl() {} - ~NullHistogramImpl() {} - std::string name() const override { return ""; } - const char* nameCStr() const override { return ""; } - const std::string& tagExtractedName() const override { CONSTRUCT_ON_FIRST_USE(std::string, ""); } - const std::vector& tags() const override { CONSTRUCT_ON_FIRST_USE(std::vector, {}); } + explicit NullHistogramImpl(SymbolTable& symbol_table) : NullMetricImpl(symbol_table) {} + ~NullHistogramImpl() { + // 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(); + } + void recordValue(uint64_t) override {} - bool used() const override { return false; } }; } // namespace Stats diff --git a/source/common/stats/isolated_store_impl.cc b/source/common/stats/isolated_store_impl.cc index e10dc340986f4..905ff98db06d0 100644 --- a/source/common/stats/isolated_store_impl.cc +++ b/source/common/stats/isolated_store_impl.cc @@ -24,19 +24,17 @@ IsolatedStoreImpl::IsolatedStoreImpl(std::unique_ptr&& symbol_table IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table) : StoreImpl(symbol_table), alloc_(symbol_table), - counters_([this](const std::string& name) -> CounterSharedPtr { - std::string tag_extracted_name = name; - std::vector tags; - return alloc_.makeCounter(name, std::move(tag_extracted_name), std::move(tags)); + counters_([this](StatName name) -> CounterSharedPtr { + return alloc_.makeCounter(name, alloc_.symbolTable().toString(name), std::vector()); }), - gauges_([this](const std::string& name) -> GaugeSharedPtr { - std::string tag_extracted_name = name; - std::vector tags; - return alloc_.makeGauge(name, std::move(tag_extracted_name), std::move(tags)); + gauges_([this](StatName name) -> GaugeSharedPtr { + return alloc_.makeGauge(name, alloc_.symbolTable().toString(name), std::vector()); }), - histograms_([this](const std::string& name) -> HistogramSharedPtr { - return std::make_shared(name, *this, std::string(name), std::vector()); - }) {} + histograms_([this](StatName name) -> HistogramSharedPtr { + return std::make_shared(name, *this, alloc_.symbolTable().toString(name), + std::vector()); + }), + null_gauge_(symbol_table) {} ScopePtr IsolatedStoreImpl::createScope(const std::string& name) { return std::make_unique(name, *this); diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index 0cf8207a23e83..c0c34576e0118 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -26,18 +26,18 @@ namespace Stats { */ template class IsolatedStatsCache { public: - using Allocator = std::function(const std::string& name)>; + using Allocator = std::function(StatName name)>; IsolatedStatsCache(Allocator alloc) : alloc_(alloc) {} - Base& get(const std::string& name) { + Base& get(StatName name) { auto stat = stats_.find(name); if (stat != stats_.end()) { return *stat->second; } std::shared_ptr new_stat = alloc_(name); - stats_.emplace(name, new_stat); + stats_.emplace(new_stat->statName(), new_stat); return *new_stat; } @@ -52,7 +52,7 @@ template class IsolatedStatsCache { } private: - absl::flat_hash_map> stats_; + StatNameHashMap> stats_; Allocator alloc_; }; @@ -62,12 +62,15 @@ class IsolatedStoreImpl : public StoreImpl { explicit IsolatedStoreImpl(SymbolTable& symbol_table); // Stats::Scope - Counter& counter(const std::string& name) override { return counters_.get(name); } + Counter& counterFromStatName(StatName name) override { return counters_.get(name); } ScopePtr createScope(const std::string& name) override; void deliverHistogramToSinks(const Histogram&, uint64_t) override {} - Gauge& gauge(const std::string& name) override { return gauges_.get(name); } + Gauge& gaugeFromStatName(StatName name) override { return gauges_.get(name); } NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; } - Histogram& histogram(const std::string& name) override { return histograms_.get(name); } + Histogram& histogramFromStatName(StatName name) override { + Histogram& histogram = histograms_.get(name); + return histogram; + } const Stats::StatsOptions& statsOptions() const override { return stats_options_; } // Stats::Store @@ -77,6 +80,19 @@ class IsolatedStoreImpl : public StoreImpl { return std::vector{}; } + Counter& counter(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return counterFromStatName(storage.statName()); + } + Gauge& gauge(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return gaugeFromStatName(storage.statName()); + } + Histogram& histogram(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return histogramFromStatName(storage.statName()); + } + private: IsolatedStoreImpl(std::unique_ptr&& symbol_table); diff --git a/source/common/stats/metric_impl.cc b/source/common/stats/metric_impl.cc new file mode 100644 index 0000000000000..b111758d675b1 --- /dev/null +++ b/source/common/stats/metric_impl.cc @@ -0,0 +1,80 @@ +#include "common/stats/metric_impl.h" + +#include "envoy/stats/tag.h" + +#include "common/stats/symbol_table_impl.h" + +namespace Envoy { +namespace Stats { + +MetricImpl::~MetricImpl() { + // The storage must be cleaned by a subclass of MetricImpl in its + // destructor, because the symbol-table is owned by the subclass. + // Simply call MetricImpl::clear() in the subclass dtor. + ASSERT(!stat_names_.populated()); +} + +MetricImpl::MetricImpl(absl::string_view tag_extracted_name, const std::vector& tags, + SymbolTable& symbol_table) { + // Encode all the names and tags into transient storage so we can count the + // required bytes. 1 is added to account for the tag_extracted_name, and we + // multiply the number of tags by 2 to account for the name and value of each + // tag. + const uint32_t num_names = 1 + 2 * tags.size(); + STACK_ARRAY(names, absl::string_view, num_names); + names[0] = tag_extracted_name; + int index = 0; + for (auto& tag : tags) { + names[++index] = tag.name_; + names[++index] = tag.value_; + } + symbol_table.populateList(names.begin(), num_names, stat_names_); +} + +void MetricImpl::clear() { stat_names_.clear(symbolTable()); } + +std::string MetricImpl::tagExtractedName() const { + return symbolTable().toString(tagExtractedStatName()); +} + +StatName MetricImpl::tagExtractedStatName() const { + StatName stat_name; + stat_names_.iterate([&stat_name](StatName s) -> bool { + stat_name = s; + return false; // Returning 'false' stops the iteration. + }); + return stat_name; +} + +std::vector MetricImpl::tags() const { + std::vector tags; + enum { TagExtractedName, TagName, TagValue } state = TagExtractedName; + Tag tag; + const SymbolTable& symbol_table = symbolTable(); + + // StatNameList maintains a linear ordered collection of StatNames, and we + // are mapping that into a tag-extracted name (the first element), followed + // by alternating TagName and TagValue. So we use a little state machine + // as we iterate through the stat_names_. + stat_names_.iterate([&tags, &state, &tag, &symbol_table](StatName stat_name) -> bool { + switch (state) { + case TagExtractedName: + state = TagName; + break; + case TagName: + tag.name_ = symbol_table.toString(stat_name); + state = TagValue; + break; + case TagValue: + tag.value_ = symbol_table.toString(stat_name); + tags.emplace_back(tag); + state = TagName; + } + return true; + }); + ASSERT(state != TagValue); + return tags; +} + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/metric_impl.h b/source/common/stats/metric_impl.h index 3e65e0d79d865..4fc6d160bbba3 100644 --- a/source/common/stats/metric_impl.h +++ b/source/common/stats/metric_impl.h @@ -3,10 +3,12 @@ #include #include +#include "envoy/stats/stat_data_allocator.h" #include "envoy/stats/stats.h" #include "envoy/stats/tag.h" #include "common/common/assert.h" +#include "common/stats/symbol_table_impl.h" namespace Envoy { namespace Stats { @@ -20,11 +22,14 @@ namespace Stats { */ class MetricImpl : public virtual Metric { public: - MetricImpl(std::string&& tag_extracted_name, std::vector&& tags) - : tag_extracted_name_(std::move(tag_extracted_name)), tags_(std::move(tags)) {} + MetricImpl(absl::string_view tag_extracted_name, const std::vector& tags, + SymbolTable& symbol_table); + ~MetricImpl(); - const std::string& tagExtractedName() const override { return tag_extracted_name_; } - const std::vector& tags() const override { return tags_; } + std::string name() const override { return symbolTable().toString(statName()); } + std::string tagExtractedName() const override; + std::vector tags() const override; + StatName tagExtractedStatName() const override; protected: /** @@ -34,9 +39,24 @@ class MetricImpl : public virtual Metric { static const uint8_t Used = 0x1; }; + void clear(); + +private: + StatNameList stat_names_; +}; + +class NullMetricImpl : public MetricImpl { +public: + explicit NullMetricImpl(SymbolTable& symbol_table) + : MetricImpl("", std::vector(), symbol_table), stat_name_storage_("", symbol_table) {} + + const SymbolTable& symbolTable() const override { return stat_name_storage_.symbolTable(); } + SymbolTable& symbolTable() override { return stat_name_storage_.symbolTable(); } + bool used() const override { return false; } + StatName statName() const override { return stat_name_storage_.statName(); } + private: - const std::string tag_extracted_name_; - const std::vector tags_; + StatNameManagedStorage stat_name_storage_; }; } // namespace Stats diff --git a/source/common/stats/raw_stat_data.cc b/source/common/stats/raw_stat_data.cc index 7ef23654bb7bb..cf2d141fcb686 100644 --- a/source/common/stats/raw_stat_data.cc +++ b/source/common/stats/raw_stat_data.cc @@ -24,6 +24,8 @@ uint64_t roundUpMultipleNaturalAlignment(uint64_t val) { } // namespace +RawStatDataAllocator::~RawStatDataAllocator() {} + // Normally the compiler would do this, but because name_ is a flexible-array-length // element, the compiler can't. RawStatData is put into an array in HotRestartImpl, so // it's important that each element starts on the required alignment for the type. @@ -37,7 +39,13 @@ uint64_t RawStatData::structSizeWithOptions(const StatsOptions& stats_options) { void RawStatData::initialize(absl::string_view key, const StatsOptions& stats_options) { ASSERT(!initialized()); - ASSERT(key.size() <= stats_options.maxNameLength()); + if (key.size() > stats_options.maxNameLength()) { + ENVOY_LOG_MISC( + warn, + "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, + key.size(), stats_options.maxNameLength()); + key = key.substr(0, stats_options.maxNameLength()); + } ref_count_ = 1; memcpy(name_, key.data(), key.size()); name_[key.size()] = '\0'; diff --git a/source/common/stats/raw_stat_data.h b/source/common/stats/raw_stat_data.h index ccefc082fa4c4..9688b3ef1d332 100644 --- a/source/common/stats/raw_stat_data.h +++ b/source/common/stats/raw_stat_data.h @@ -95,17 +95,56 @@ struct RawStatData { using RawStatDataSet = BlockMemoryHashSet; +template class RawStat : public Stat { +public: + RawStat(StatName stat_name, RawStatData& data, StatDataAllocatorImpl& alloc, + absl::string_view tag_extracted_name, const std::vector& tags) + : Stat(data, alloc, tag_extracted_name, tags), + stat_name_storage_(stat_name, alloc.symbolTable()) {} + ~RawStat() { stat_name_storage_.free(this->symbolTable()); } + + StatName statName() const override { return stat_name_storage_.statName(); } + +private: + StatNameStorage stat_name_storage_; +}; + class RawStatDataAllocator : public StatDataAllocatorImpl { public: RawStatDataAllocator(Thread::BasicLockable& mutex, RawStatDataSet& stats_set, const StatsOptions& options, SymbolTable& symbol_table) : StatDataAllocatorImpl(symbol_table), mutex_(mutex), stats_set_(stats_set), options_(options) {} + ~RawStatDataAllocator(); + + virtual RawStatData* alloc(absl::string_view name); // Virtual only for mocking. + void free(Stats::RawStatData& data) override; + RawStatData* allocStatName(StatName stat_name) { + return alloc(symbolTable().toString(stat_name)); + } // StatDataAllocator bool requiresBoundedStatNameSize() const override { return true; } - Stats::RawStatData* alloc(absl::string_view name) override; - void free(Stats::RawStatData& data) override; + + template + std::shared_ptr makeStat(StatName name, absl::string_view tag_extracted_name, + const std::vector& tags) { + RawStatData* raw_stat_data = allocStatName(name); + if (raw_stat_data == nullptr) { + return nullptr; + } + return std::make_shared>(name, *raw_stat_data, *this, tag_extracted_name, tags); + } + + CounterSharedPtr makeCounter(StatName name, absl::string_view tag_extracted_name, + const std::vector& tags) override { + return makeStat>(name, tag_extracted_name, tags); + } + + GaugeSharedPtr makeGauge(StatName name, absl::string_view tag_extracted_name, + const std::vector& tags) override { + return makeStat>(name, tag_extracted_name, tags); + } private: Thread::BasicLockable& mutex_; diff --git a/source/common/stats/scope_prefixer.cc b/source/common/stats/scope_prefixer.cc index 96e1e8be7b0af..521696059e4ad 100644 --- a/source/common/stats/scope_prefixer.cc +++ b/source/common/stats/scope_prefixer.cc @@ -9,22 +9,39 @@ namespace Envoy { namespace Stats { ScopePrefixer::ScopePrefixer(absl::string_view prefix, Scope& scope) - : prefix_(Utility::sanitizeStatsName(prefix)), scope_(scope) {} + : scope_(scope), prefix_(Utility::sanitizeStatsName(prefix), symbolTable()) {} + +ScopePrefixer::ScopePrefixer(StatName prefix, Scope& scope) + : scope_(scope), prefix_(prefix, symbolTable()) {} + +ScopePrefixer::~ScopePrefixer() { prefix_.free(symbolTable()); } + +ScopePtr ScopePrefixer::createScopeFromStatName(StatName name) { + SymbolTable::StoragePtr joined = symbolTable().join({prefix_.statName(), name}); + return std::make_unique(StatName(joined.get()), scope_); +} ScopePtr ScopePrefixer::createScope(const std::string& name) { - return std::make_unique(prefix_ + name, scope_); + StatNameManagedStorage stat_name_storage(Utility::sanitizeStatsName(name), symbolTable()); + return createScopeFromStatName(stat_name_storage.statName()); } Counter& ScopePrefixer::counterFromStatName(StatName name) { - return counter(symbolTable().toString(name)); + Stats::SymbolTable::StoragePtr stat_name_storage = + scope_.symbolTable().join({prefix_.statName(), name}); + return scope_.counterFromStatName(StatName(stat_name_storage.get())); } Gauge& ScopePrefixer::gaugeFromStatName(StatName name) { - return gauge(symbolTable().toString(name)); + Stats::SymbolTable::StoragePtr stat_name_storage = + scope_.symbolTable().join({prefix_.statName(), name}); + return scope_.gaugeFromStatName(StatName(stat_name_storage.get())); } Histogram& ScopePrefixer::histogramFromStatName(StatName name) { - return histogram(symbolTable().toString(name)); + Stats::SymbolTable::StoragePtr stat_name_storage = + scope_.symbolTable().join({prefix_.statName(), name}); + return scope_.histogramFromStatName(StatName(stat_name_storage.get())); } void ScopePrefixer::deliverHistogramToSinks(const Histogram& histograms, uint64_t val) { diff --git a/source/common/stats/scope_prefixer.h b/source/common/stats/scope_prefixer.h index 4871840f549e3..22bf13e8932c3 100644 --- a/source/common/stats/scope_prefixer.h +++ b/source/common/stats/scope_prefixer.h @@ -10,18 +10,30 @@ namespace Stats { class ScopePrefixer : public Scope { public: ScopePrefixer(absl::string_view prefix, Scope& scope); + ScopePrefixer(StatName prefix, Scope& scope); + ~ScopePrefixer() override; + + ScopePtr createScopeFromStatName(StatName name); // Scope ScopePtr createScope(const std::string& name) override; - Counter& counter(const std::string& name) override { return scope_.counter(prefix_ + name); } - Gauge& gauge(const std::string& name) override { return scope_.gauge(prefix_ + name); } - Histogram& histogram(const std::string& name) override { - return scope_.histogram(prefix_ + name); - } - void deliverHistogramToSinks(const Histogram& histograms, uint64_t val) override; Counter& counterFromStatName(StatName name) override; Gauge& gaugeFromStatName(StatName name) override; Histogram& histogramFromStatName(StatName name) override; + void deliverHistogramToSinks(const Histogram& histograms, uint64_t val) override; + + Counter& counter(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return counterFromStatName(storage.statName()); + } + Gauge& gauge(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return gaugeFromStatName(storage.statName()); + } + Histogram& histogram(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return histogramFromStatName(storage.statName()); + } const Stats::StatsOptions& statsOptions() const override { return scope_.statsOptions(); } const SymbolTable& symbolTable() const override { return scope_.symbolTable(); } @@ -30,8 +42,8 @@ class ScopePrefixer : public Scope { NullGaugeImpl& nullGauge(const std::string& str) override { return scope_.nullGauge(str); } private: - std::string prefix_; Scope& scope_; + StatNameStorage prefix_; }; } // namespace Stats diff --git a/source/common/stats/stat_data_allocator_impl.h b/source/common/stats/stat_data_allocator_impl.h index 5b8bfc994d62e..82db3a9562b8a 100644 --- a/source/common/stats/stat_data_allocator_impl.h +++ b/source/common/stats/stat_data_allocator_impl.h @@ -32,21 +32,6 @@ template class StatDataAllocatorImpl : public StatDataAllocator public: explicit StatDataAllocatorImpl(SymbolTable& symbol_table) : symbol_table_(symbol_table) {} - // StatDataAllocator - CounterSharedPtr makeCounter(absl::string_view name, std::string&& tag_extracted_name, - std::vector&& tags) override; - GaugeSharedPtr makeGauge(absl::string_view name, std::string&& tag_extracted_name, - std::vector&& tags) override; - - /** - * @param name the full name of the stat. - * @return StatData* a data block for a given stat name or nullptr if there is no more memory - * available for stats. The allocator should return a reference counted data location - * by name if one already exists with the same name. This is used for intra-process - * scope swapping as well as inter-process hot restart. - */ - virtual StatData* alloc(absl::string_view name) PURE; - /** * Free a raw stat data block. The allocator should handle reference counting and only truly * free the block if it is no longer needed. @@ -58,6 +43,9 @@ template class StatDataAllocatorImpl : public StatDataAllocator const SymbolTable& symbolTable() const override { return symbol_table_; } private: + // SymbolTable encodes encodes stat names as back into strings. This does not + // get guarded by a mutex, since it has its own internal mutex to guarantee + // thread safety. SymbolTable& symbol_table_; }; @@ -71,13 +59,17 @@ template class StatDataAllocatorImpl : public StatDataAllocator template class CounterImpl : public Counter, public MetricImpl { public: CounterImpl(StatData& data, StatDataAllocatorImpl& alloc, - std::string&& tag_extracted_name, std::vector&& tags) - : MetricImpl(std::move(tag_extracted_name), std::move(tags)), data_(data), alloc_(alloc) {} - ~CounterImpl() { alloc_.free(data_); } - - // Stats::Metric - std::string name() const override { return std::string(data_.name()); } - const char* nameCStr() const override { return data_.name(); } + absl::string_view tag_extracted_name, const std::vector& tags) + : MetricImpl(tag_extracted_name, tags, alloc.symbolTable()), data_(data), alloc_(alloc) {} + ~CounterImpl() override { + alloc_.free(data_); + + // 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(); + } // Stats::Counter void add(uint64_t amount) override { @@ -92,7 +84,10 @@ template class CounterImpl : public Counter, public MetricImpl bool used() const override { return data_.flags_ & Flags::Used; } uint64_t value() const override { return data_.value_; } -private: + const SymbolTable& symbolTable() const override { return alloc_.symbolTable(); } + SymbolTable& symbolTable() override { return alloc_.symbolTable(); } + +protected: StatData& data_; StatDataAllocatorImpl& alloc_; }; @@ -101,19 +96,21 @@ template class CounterImpl : public Counter, public MetricImpl * Null counter implementation. * No-ops on all calls and requires no underlying metric or data. */ -class NullCounterImpl : public Counter { +class NullCounterImpl : public Counter, NullMetricImpl { public: - NullCounterImpl() {} - ~NullCounterImpl() {} - std::string name() const override { return ""; } - const char* nameCStr() const override { return ""; } - const std::string& tagExtractedName() const override { CONSTRUCT_ON_FIRST_USE(std::string, ""); } - const std::vector& tags() const override { CONSTRUCT_ON_FIRST_USE(std::vector, {}); } + explicit NullCounterImpl(SymbolTable& symbol_table) : NullMetricImpl(symbol_table) {} + ~NullCounterImpl() 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(); + } + void add(uint64_t) override {} void inc() override {} uint64_t latch() override { return 0; } void reset() override {} - bool used() const override { return false; } uint64_t value() const override { return 0; } }; @@ -123,13 +120,17 @@ class NullCounterImpl : public Counter { template class GaugeImpl : public Gauge, public MetricImpl { public: GaugeImpl(StatData& data, StatDataAllocatorImpl& alloc, - std::string&& tag_extracted_name, std::vector&& tags) - : MetricImpl(std::move(tag_extracted_name), std::move(tags)), data_(data), alloc_(alloc) {} - ~GaugeImpl() { alloc_.free(data_); } - - // Stats::Metric - std::string name() const override { return std::string(data_.name()); } - const char* nameCStr() const override { return data_.name(); } + absl::string_view tag_extracted_name, const std::vector& tags) + : MetricImpl(tag_extracted_name, tags, alloc.symbolTable()), data_(data), alloc_(alloc) {} + ~GaugeImpl() override { + alloc_.free(data_); + + // 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(); + } // Stats::Gauge virtual void add(uint64_t amount) override { @@ -150,7 +151,10 @@ template class GaugeImpl : public Gauge, public MetricImpl { virtual uint64_t value() const override { return data_.value_; } bool used() const override { return data_.flags_ & Flags::Used; } -private: + const SymbolTable& symbolTable() const override { return alloc_.symbolTable(); } + SymbolTable& symbolTable() override { return alloc_.symbolTable(); } + +protected: StatData& data_; StatDataAllocatorImpl& alloc_; }; @@ -159,46 +163,24 @@ template class GaugeImpl : public Gauge, public MetricImpl { * Null gauge implementation. * No-ops on all calls and requires no underlying metric or data. */ -class NullGaugeImpl : public Gauge { +class NullGaugeImpl : public Gauge, NullMetricImpl { public: - NullGaugeImpl() {} - ~NullGaugeImpl() {} - std::string name() const override { return ""; } - const char* nameCStr() const override { return ""; } - const std::string& tagExtractedName() const override { CONSTRUCT_ON_FIRST_USE(std::string, ""); } - const std::vector& tags() const override { CONSTRUCT_ON_FIRST_USE(std::vector, {}); } + explicit NullGaugeImpl(SymbolTable& symbol_table) : NullMetricImpl(symbol_table) {} + ~NullGaugeImpl() 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(); + } + void add(uint64_t) override {} void inc() override {} void dec() override {} void set(uint64_t) override {} void sub(uint64_t) override {} - bool used() const override { return false; } uint64_t value() const override { return 0; } }; -template -CounterSharedPtr StatDataAllocatorImpl::makeCounter(absl::string_view name, - std::string&& tag_extracted_name, - std::vector&& tags) { - StatData* data = alloc(name); - if (data == nullptr) { - return nullptr; - } - return std::make_shared>(*data, *this, std::move(tag_extracted_name), - std::move(tags)); -} - -template -GaugeSharedPtr StatDataAllocatorImpl::makeGauge(absl::string_view name, - std::string&& tag_extracted_name, - std::vector&& tags) { - StatData* data = alloc(name); - if (data == nullptr) { - return nullptr; - } - return std::make_shared>(*data, *this, std::move(tag_extracted_name), - std::move(tags)); -} - } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index 24390a4eee294..173ad3e7a0176 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/source/common/stats/symbol_table_impl.h @@ -251,7 +251,7 @@ class SymbolTableImpl : public SymbolTable { * will fire to guard against symbol-table leaks. * * Thus this class is inconvenient to directly use as temp storage for building - * a StatName from a string. Instead it should be used via StatNameTempStorage. + * a StatName from a string. Instead it should be used via StatNameManagedStorage. */ class StatNameStorage { public: @@ -367,22 +367,34 @@ StatName StatNameStorage::statName() const { return StatName(bytes_.get()); } * Contains the backing store for a StatName and enough context so it can * self-delete through RAII. This works by augmenting StatNameStorage with a * reference to the SymbolTable&, so it has an extra 8 bytes of footprint. It - * is intended to be used in tests or as a scoped temp in a function, rather - * than stored in a larger structure such as a map, where the redundant copies - * of the SymbolTable& would be costly in aggregate. + * is intended to be used in cases where simplicity of implementation is more + * important than byte-savings, for example: + * - outside the stats system + * - in tests + * - as a scoped temp in a function + * Due to the extra 8 bytes per instance, scalability should be taken into + * account before using this as (say) a value or key in a map. In those + * scenarios, it would be better to store the SymbolTable reference once + * for the entire map. + * + * In the stat structures, we generally use StatNameStorage to avoid the + * per-stat overhead. */ -class StatNameTempStorage : public StatNameStorage { +class StatNameManagedStorage : public StatNameStorage { public: // Basic constructor for when you have a name as a string, and need to // generate symbols for it. - StatNameTempStorage(absl::string_view name, SymbolTable& table) + StatNameManagedStorage(absl::string_view name, SymbolTable& table) : StatNameStorage(name, table), symbol_table_(table) {} // Obtains new backing storage for an already existing StatName. - StatNameTempStorage(StatName src, SymbolTable& table) + StatNameManagedStorage(StatName src, SymbolTable& table) : StatNameStorage(src, table), symbol_table_(table) {} - ~StatNameTempStorage() { free(symbol_table_); } + ~StatNameManagedStorage() { free(symbol_table_); } + + SymbolTable& symbolTable() { return symbol_table_; } + const SymbolTable& symbolTable() const { return symbol_table_; } private: SymbolTable& symbol_table_; @@ -508,7 +520,7 @@ struct HeterogeneousStatNameEqual { // easier at the call-sites in thread_local_store.cc to implement this an // explicit free() method, analogous to StatNameStorage::free(), compared to // storing a SymbolTable reference in the class and doing the free in the -// destructor, like StatNameTempStorage. +// destructor, like StatNameManagedStorage. class StatNameStorageSet { public: using HashSet = diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index 092e66f0edd8b..acf440fa695e5 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -62,11 +62,11 @@ TagExtractorPtr TagExtractorImpl::createTagExtractor(const std::string& name, return TagExtractorPtr{new TagExtractorImpl(name, regex, substr)}; } -bool TagExtractorImpl::substrMismatch(const std::string& stat_name) const { - return !substr_.empty() && stat_name.find(substr_) == std::string::npos; +bool TagExtractorImpl::substrMismatch(absl::string_view stat_name) const { + return !substr_.empty() && stat_name.find(substr_) == absl::string_view::npos; } -bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector& tags, +bool TagExtractorImpl::extractTag(absl::string_view stat_name, std::vector& tags, IntervalSet& remove_characters) const { PERF_OPERATION(perf); @@ -75,9 +75,11 @@ bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector return false; } - std::smatch match; + std::match_results match; // The regex must match and contain one or more subexpressions (all after the first are ignored). - if (std::regex_search(stat_name, match, regex_) && match.size() > 1) { + if (std::regex_search(stat_name.begin(), stat_name.end(), match, + regex_) && + match.size() > 1) { // remove_subexpr is the first submatch. It represents the portion of the string to be removed. const auto& remove_subexpr = match[1]; diff --git a/source/common/stats/tag_extractor_impl.h b/source/common/stats/tag_extractor_impl.h index c72765a75234c..138f25b6af7c0 100644 --- a/source/common/stats/tag_extractor_impl.h +++ b/source/common/stats/tag_extractor_impl.h @@ -28,7 +28,7 @@ class TagExtractorImpl : public TagExtractor { TagExtractorImpl(const std::string& name, const std::string& regex, const std::string& substr = ""); std::string name() const override { return name_; } - bool extractTag(const std::string& tag_extracted_name, std::vector& tags, + bool extractTag(absl::string_view tag_extracted_name, std::vector& tags, IntervalSet& remove_characters) const override; absl::string_view prefixToken() const override { return prefix_; } @@ -37,7 +37,7 @@ class TagExtractorImpl : public TagExtractor { * @return bool indicates whether tag extraction should be skipped for this stat_name due * to a substring mismatch. */ - bool substrMismatch(const std::string& stat_name) const; + bool substrMismatch(absl::string_view stat_name) const; private: /** diff --git a/source/common/stats/tag_producer_impl.cc b/source/common/stats/tag_producer_impl.cc index d23d609c8a44a..f03a4b6553e3e 100644 --- a/source/common/stats/tag_producer_impl.cc +++ b/source/common/stats/tag_producer_impl.cc @@ -63,12 +63,12 @@ void TagProducerImpl::addExtractor(TagExtractorPtr extractor) { } void TagProducerImpl::forEachExtractorMatching( - const std::string& stat_name, std::function f) const { + absl::string_view stat_name, std::function f) const { IntervalSetImpl remove_characters; for (const TagExtractorPtr& tag_extractor : tag_extractors_without_prefix_) { f(tag_extractor); } - const std::string::size_type dot = stat_name.find('.'); + const absl::string_view::size_type dot = stat_name.find('.'); if (dot != std::string::npos) { const absl::string_view token = absl::string_view(stat_name.data(), dot); const auto iter = tag_extractor_prefix_map_.find(token); @@ -80,7 +80,7 @@ void TagProducerImpl::forEachExtractorMatching( } } -std::string TagProducerImpl::produceTags(const std::string& metric_name, +std::string TagProducerImpl::produceTags(absl::string_view metric_name, std::vector& tags) const { tags.insert(tags.end(), default_tags_.begin(), default_tags_.end()); IntervalSetImpl remove_characters; diff --git a/source/common/stats/tag_producer_impl.h b/source/common/stats/tag_producer_impl.h index 1614bcb28299e..505cb71929aac 100644 --- a/source/common/stats/tag_producer_impl.h +++ b/source/common/stats/tag_producer_impl.h @@ -37,7 +37,7 @@ class TagProducerImpl : public TagProducer { * @param metric_name std::string a name of Stats::Metric (Counter, Gauge, Histogram). * @param tags std::vector a set of Stats::Tag. */ - std::string produceTags(const std::string& metric_name, std::vector& tags) const override; + std::string produceTags(absl::string_view metric_name, std::vector& tags) const override; private: friend class DefaultTagRegexTester; @@ -89,7 +89,7 @@ class TagProducerImpl : public TagProducer { * @param stat_name const std::string& the stat name. * @param f std::function function to call for each extractor. */ - void forEachExtractorMatching(const std::string& stat_name, + void forEachExtractorMatching(absl::string_view stat_name, std::function f) const; std::vector tag_extractors_without_prefix_; diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 114b7c74d796b..b342d2793be51 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -13,6 +13,7 @@ #include "envoy/stats/stats_options.h" #include "common/common/lock_guard.h" +#include "common/stats/scope_prefixer.h" #include "common/stats/stats_matcher_impl.h" #include "common/stats/tag_producer_impl.h" @@ -26,17 +27,23 @@ ThreadLocalStoreImpl::ThreadLocalStoreImpl(const StatsOptions& stats_options, : stats_options_(stats_options), alloc_(alloc), default_scope_(createScope("")), tag_producer_(std::make_unique()), stats_matcher_(std::make_unique()), - num_last_resort_stats_(default_scope_->counter("stats.overflow")), - heap_allocator_(alloc.symbolTable()), source_(*this) {} + stats_overflow_("stats.overflow", alloc.symbolTable()), + num_last_resort_stats_(default_scope_->counterFromStatName(stats_overflow_.statName())), + heap_allocator_(alloc.symbolTable()), source_(*this), null_counter_(alloc.symbolTable()), + null_gauge_(alloc.symbolTable()), null_histogram_(alloc.symbolTable()) {} ThreadLocalStoreImpl::~ThreadLocalStoreImpl() { ASSERT(shutting_down_); default_scope_.reset(); ASSERT(scopes_.empty()); + stats_overflow_.free(symbolTable()); } void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) { stats_matcher_ = std::move(stats_matcher); + if (stats_matcher_->acceptsAll()) { + return; + } // The Filesystem and potentially other stat-registering objects are // constructed prior to the stat-matcher, and those add stats @@ -52,13 +59,13 @@ void ThreadLocalStoreImpl::setStatsMatcher(StatsMatcherPtr&& stats_matcher) { template void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass& list) { - std::vector remove_list; + std::vector remove_list; for (auto& stat : map) { if (rejects(stat.first)) { remove_list.push_back(stat.first); } } - for (const char* stat_name : remove_list) { + for (StatName stat_name : remove_list) { auto iter = map.find(stat_name); ASSERT(iter != map.end()); list.push_back(iter->second); // Save SharedPtr to the list to avoid invalidating refs to stat. @@ -66,17 +73,27 @@ void ThreadLocalStoreImpl::removeRejectedStats(StatMapClass& map, StatListClass& } } -bool ThreadLocalStoreImpl::rejects(const std::string& name) const { +bool ThreadLocalStoreImpl::rejects(StatName stat_name) const { + // Don't both elaborating the StatName there are no pattern-based + // exclusions;/inclusions. + if (stats_matcher_->acceptsAll()) { + return false; + } + // TODO(ambuc): If stats_matcher_ depends on regexes, this operation (on the // hot path) could become prohibitively expensive. Revisit this usage in the // future. - return stats_matcher_->rejects(name); + // + // Also note that the elaboration of the stat-name into a string is expensive, + // so I think it might be better to move the matcher test until after caching, + // unless its acceptsAll/rejectsAll. + return stats_matcher_->rejectsAll() || stats_matcher_->rejects(symbolTable().toString(stat_name)); } std::vector ThreadLocalStoreImpl::counters() const { // Handle de-dup due to overlapping scopes. std::vector ret; - ConstCharStarHashSet names; + StatNameHashSet names; Thread::LockGuard lock(lock_); for (ScopeImpl* scope : scopes_) { for (auto& counter : scope->central_cache_.counters_) { @@ -90,7 +107,7 @@ std::vector ThreadLocalStoreImpl::counters() const { } ScopePtr ThreadLocalStoreImpl::createScope(const std::string& name) { - std::unique_ptr new_scope(new ScopeImpl(*this, name)); + auto new_scope = std::make_unique(*this, name); Thread::LockGuard lock(lock_); scopes_.emplace(new_scope.get()); return std::move(new_scope); @@ -99,7 +116,7 @@ ScopePtr ThreadLocalStoreImpl::createScope(const std::string& name) { std::vector ThreadLocalStoreImpl::gauges() const { // Handle de-dup due to overlapping scopes. std::vector ret; - ConstCharStarHashSet names; + StatNameHashSet names; Thread::LockGuard lock(lock_); for (ScopeImpl* scope : scopes_) { for (auto& gauge : scope->central_cache_.gauges_) { @@ -180,26 +197,40 @@ void ThreadLocalStoreImpl::releaseScopeCrossThread(ScopeImpl* scope) { ASSERT(scopes_.count(scope) == 1); scopes_.erase(scope); + // This is called directly from the ScopeImpl destructor, but we can't delay + // the destruction of scope->central_cache_.central_cache_.rejected_stats_ + // to wait for all the TLS rejected_stats_ caches to be destructed, as those + // reference elements of SharedStatNameStorageSet. So simply swap out the set + // contents into a local that we can hold onto until the TLS cache is cleared + // of all references. + auto rejected_stats = new StatNameStorageSet; + rejected_stats->swap(scope->central_cache_.rejected_stats_); + const uint64_t scope_id = scope->scope_id_; + auto clean_central_cache = [this, rejected_stats]() { + rejected_stats->free(symbolTable()); + delete rejected_stats; + }; + // This can happen from any thread. We post() back to the main thread which will initiate the // cache flush operation. if (!shutting_down_ && main_thread_dispatcher_) { - main_thread_dispatcher_->post( - [this, scope_id = scope->scope_id_]() -> void { clearScopeFromCaches(scope_id); }); + main_thread_dispatcher_->post([this, clean_central_cache, scope_id]() { + clearScopeFromCaches(scope_id, clean_central_cache); + }); + } else { + clean_central_cache(); } } -std::string ThreadLocalStoreImpl::getTagsForName(const std::string& name, - std::vector& tags) const { - return tag_producer_->produceTags(name, tags); -} - -void ThreadLocalStoreImpl::clearScopeFromCaches(uint64_t scope_id) { +void ThreadLocalStoreImpl::clearScopeFromCaches(uint64_t scope_id, + const Event::PostCb& clean_central_cache) { // If we are shutting down we no longer perform cache flushes as workers may be shutting down // at the same time. if (!shutting_down_) { // Perform a cache flush on all threads. tls_->runOnAllThreads( - [this, scope_id]() -> void { tls_->getTyped().scope_cache_.erase(scope_id); }); + [this, scope_id]() -> void { tls_->getTyped().scope_cache_.erase(scope_id); }, + clean_central_cache); } } @@ -209,35 +240,68 @@ absl::string_view ThreadLocalStoreImpl::truncateStatNameIfNeeded(absl::string_vi // allocation. if (alloc_.requiresBoundedStatNameSize()) { const uint64_t max_length = stats_options_.maxNameLength(); - name = name.substr(0, max_length); + if (name.size() > max_length) { + ENVOY_LOG_MISC( + warn, + "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", + name, name.size(), max_length); + name = name.substr(0, max_length); + } } return name; } std::atomic ThreadLocalStoreImpl::ScopeImpl::next_scope_id_; -ThreadLocalStoreImpl::ScopeImpl::~ScopeImpl() { parent_.releaseScopeCrossThread(this); } +ThreadLocalStoreImpl::ScopeImpl::ScopeImpl(ThreadLocalStoreImpl& parent, const std::string& prefix) + : scope_id_(next_scope_id_++), parent_(parent), + prefix_(Utility::sanitizeStatsName(prefix), parent.symbolTable()) {} -bool ThreadLocalStoreImpl::checkAndRememberRejection(const std::string& name, - SharedStringSet& central_rejected_stats, - SharedStringSet* tls_rejected_stats) { +ThreadLocalStoreImpl::ScopeImpl::~ScopeImpl() { + parent_.releaseScopeCrossThread(this); + prefix_.free(symbolTable()); +} + +// Manages the truncation and tag-extration of stat names. Tag extraction occurs +// on the original, untruncated name so the extraction can complete properly, +// even if the tag values are partially truncated. +class TagExtraction { +public: + TagExtraction(ThreadLocalStoreImpl& tls, StatName name) { + tls.symbolTable().callWithStringView(name, [this, &tls](absl::string_view name_str) { + tag_extracted_name_ = tls.tagProducer().produceTags(name_str, tags_); + }); + } + + const std::vector& tags() { return tags_; } + const std::string& tagExtractedName() { return tag_extracted_name_; } + +private: + std::vector tags_; + std::string tag_extracted_name_; +}; + +bool ThreadLocalStoreImpl::checkAndRememberRejection(StatName name, + StatNameStorageSet& central_rejected_stats, + StatNameHashSet* tls_rejected_stats) { if (stats_matcher_->acceptsAll()) { return false; } auto iter = central_rejected_stats.find(name); - SharedString rejected_name; + const StatNameStorage* rejected_name = nullptr; if (iter != central_rejected_stats.end()) { - rejected_name = *iter; + rejected_name = &(*iter); } else { if (rejects(name)) { - rejected_name = std::make_shared(name); - central_rejected_stats.insert(rejected_name); + auto insertion = central_rejected_stats.insert(StatNameStorage(name, symbolTable())); + const StatNameStorage& rejected_name_ref = *(insertion.first); + rejected_name = &rejected_name_ref; } } if (rejected_name != nullptr) { if (tls_rejected_stats != nullptr) { - tls_rejected_stats->insert(rejected_name); + tls_rejected_stats->insert(rejected_name->statName()); } return true; } @@ -246,29 +310,20 @@ bool ThreadLocalStoreImpl::checkAndRememberRejection(const std::string& name, template StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( - const std::string& name, StatMap>& central_cache_map, - SharedStringSet& central_rejected_stats, MakeStatFn make_stat, - StatMap>* tls_cache, SharedStringSet* tls_rejected_stats, + StatName name, StatMap>& central_cache_map, + StatNameStorageSet& central_rejected_stats, MakeStatFn make_stat, + StatMap>* tls_cache, StatNameHashSet* tls_rejected_stats, StatType& null_stat) { - const char* stat_key = name.c_str(); - // We do name-rejections on the full name, prior to truncation. if (tls_rejected_stats != nullptr && - tls_rejected_stats->find(stat_key) != tls_rejected_stats->end()) { + tls_rejected_stats->find(name) != tls_rejected_stats->end()) { return null_stat; } - std::unique_ptr truncation_buffer; - absl::string_view truncated_name = parent_.truncateStatNameIfNeeded(name); - if (truncated_name.size() < name.size()) { - truncation_buffer = std::make_unique(std::string(truncated_name)); - stat_key = truncation_buffer->c_str(); // must be nul-terminated. - } - // If we have a valid cache entry, return it. if (tls_cache) { - auto pos = tls_cache->find(stat_key); + auto pos = tls_cache->find(name); if (pos != tls_cache->end()) { return *pos->second; } @@ -277,7 +332,7 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( // We must now look in the central store so we must be locked. We grab a reference to the // central store location. It might contain nothing. In this case, we allocate a new stat. Thread::LockGuard lock(parent_.lock_); - auto iter = central_cache_map.find(stat_key); + auto iter = central_cache_map.find(name); std::shared_ptr* central_ref = nullptr; if (iter != central_cache_map.end()) { central_ref = &(iter->second); @@ -285,47 +340,31 @@ StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( // Note that again we do the name-rejection lookup on the untruncated name. return null_stat; } else { - // If we had to truncate, warn now that we've missed all caches. - if (truncation_buffer != nullptr) { - ENVOY_LOG_MISC( - warn, - "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", - name, name.size(), truncation_buffer->size()); - } - - std::vector tags; - - // Tag extraction occurs on the original, untruncated name so the extraction - // can complete properly, even if the tag values are partially truncated. - std::string tag_extracted_name = parent_.getTagsForName(name, tags); + TagExtraction extraction(parent_, name); std::shared_ptr stat = - make_stat(parent_.alloc_, truncated_name, std::move(tag_extracted_name), std::move(tags)); + make_stat(parent_.alloc_, name, extraction.tagExtractedName(), extraction.tags()); if (stat == nullptr) { - // TODO(jmarantz): If make_stat fails, the actual move does not actually occur - // for tag_extracted_name and tags, so there is no use-after-move problem. - // In order to increase the readability of the code, refactoring is done here. parent_.num_last_resort_stats_.inc(); - stat = make_stat(parent_.heap_allocator_, truncated_name, - std::move(tag_extracted_name), // NOLINT(bugprone-use-after-move) - std::move(tags)); // NOLINT(bugprone-use-after-move) + stat = make_stat(parent_.heap_allocator_, name, extraction.tagExtractedName(), + extraction.tags()); ASSERT(stat != nullptr); } - central_ref = ¢ral_cache_map[stat->nameCStr()]; + central_ref = ¢ral_cache_map[stat->statName()]; *central_ref = stat; } // If we have a TLS cache, insert the stat. if (tls_cache) { - tls_cache->insert(std::make_pair((*central_ref)->nameCStr(), *central_ref)); + tls_cache->insert(std::make_pair((*central_ref)->statName(), *central_ref)); } // Finally we return the reference. return **central_ref; } -Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) { +Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatName(StatName name) { if (parent_.rejectsAll()) { - return null_counter_; + return parent_.null_counter_; } // Determine the final name based on the prefix and the passed name. @@ -337,12 +376,13 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) { // 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. - std::string final_name = prefix_ + name; + Stats::SymbolTable::StoragePtr final_name = symbolTable().join({prefix_.statName(), name}); + StatName final_stat_name(final_name.get()); // We now find the TLS cache. This might remain null if we don't have TLS // initialized currently. StatMap* tls_cache = nullptr; - SharedStringSet* tls_rejected_stats = nullptr; + StatNameHashSet* tls_rejected_stats = nullptr; if (!parent_.shutting_down_ && parent_.tls_) { TlsCacheEntry& entry = parent_.tls_->getTyped().scope_cache_[this->scope_id_]; tls_cache = &entry.counters_; @@ -350,12 +390,12 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counter(const std::string& name) { } return safeMakeStat( - final_name, central_cache_.counters_, central_cache_.rejected_stats_, - [](StatDataAllocator& allocator, absl::string_view name, std::string&& tag_extracted_name, - std::vector&& tags) -> CounterSharedPtr { - return allocator.makeCounter(name, std::move(tag_extracted_name), std::move(tags)); + final_stat_name, central_cache_.counters_, central_cache_.rejected_stats_, + [](StatDataAllocator& allocator, StatName name, absl::string_view tag_extracted_name, + const std::vector& tags) -> CounterSharedPtr { + return allocator.makeCounter(name, tag_extracted_name, tags); }, - tls_cache, tls_rejected_stats, null_counter_); + tls_cache, tls_rejected_stats, parent_.null_counter_); } void ThreadLocalStoreImpl::ScopeImpl::deliverHistogramToSinks(const Histogram& histogram, @@ -374,9 +414,9 @@ void ThreadLocalStoreImpl::ScopeImpl::deliverHistogramToSinks(const Histogram& h } } -Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) { +Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatName(StatName name) { if (parent_.rejectsAll()) { - return null_gauge_; + return parent_.null_gauge_; } // See comments in counter(). There is no super clean way (via templates or otherwise) to @@ -387,10 +427,11 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) { // a temporary, and address sanitization errors would follow. Instead we must // do a find() first, using that if it succeeds. If it fails, then after we // construct the stat we can insert it into the required maps. - std::string final_name = prefix_ + name; + Stats::SymbolTable::StoragePtr final_name = symbolTable().join({prefix_.statName(), name}); + StatName final_stat_name(final_name.get()); StatMap* tls_cache = nullptr; - SharedStringSet* tls_rejected_stats = nullptr; + StatNameHashSet* tls_rejected_stats = nullptr; if (!parent_.shutting_down_ && parent_.tls_) { TlsCacheEntry& entry = parent_.tls_->getTyped().scope_cache_[this->scope_id_]; tls_cache = &entry.gauges_; @@ -398,17 +439,17 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gauge(const std::string& name) { } return safeMakeStat( - final_name, central_cache_.gauges_, central_cache_.rejected_stats_, - [](StatDataAllocator& allocator, absl::string_view name, std::string&& tag_extracted_name, - std::vector&& tags) -> GaugeSharedPtr { - return allocator.makeGauge(name, std::move(tag_extracted_name), std::move(tags)); + final_stat_name, central_cache_.gauges_, central_cache_.rejected_stats_, + [](StatDataAllocator& allocator, StatName name, absl::string_view tag_extracted_name, + const std::vector& tags) -> GaugeSharedPtr { + return allocator.makeGauge(name, tag_extracted_name, tags); }, - tls_cache, tls_rejected_stats, null_gauge_); + tls_cache, tls_rejected_stats, parent_.null_gauge_); } -Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) { +Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatName(StatName name) { if (parent_.rejectsAll()) { - return null_histogram_; + return parent_.null_histogram_; } // See comments in counter(). There is no super clean way (via templates or otherwise) to @@ -419,85 +460,91 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogram(const std::string& name) { // a temporary, and address sanitization errors would follow. Instead we must // do a find() first, using that if it succeeds. If it fails, then after we // construct the stat we can insert it into the required maps. - std::string final_name = prefix_ + name; + Stats::SymbolTable::StoragePtr final_name = symbolTable().join({prefix_.statName(), name}); + StatName final_stat_name(final_name.get()); StatMap* tls_cache = nullptr; - SharedStringSet* tls_rejected_stats = nullptr; + StatNameHashSet* tls_rejected_stats = nullptr; if (!parent_.shutting_down_ && parent_.tls_) { TlsCacheEntry& entry = parent_.tls_->getTyped().scope_cache_[this->scope_id_]; tls_cache = &entry.parent_histograms_; - auto iter = tls_cache->find(final_name.c_str()); + auto iter = tls_cache->find(final_stat_name); if (iter != tls_cache->end()) { return *iter->second; } tls_rejected_stats = &entry.rejected_stats_; - if (tls_rejected_stats->find(final_name.c_str()) != tls_rejected_stats->end()) { - return null_histogram_; + if (tls_rejected_stats->find(final_stat_name) != tls_rejected_stats->end()) { + return parent_.null_histogram_; } } Thread::LockGuard lock(parent_.lock_); - auto iter = central_cache_.histograms_.find(final_name.c_str()); + auto iter = central_cache_.histograms_.find(final_stat_name); ParentHistogramImplSharedPtr* central_ref = nullptr; if (iter != central_cache_.histograms_.end()) { central_ref = &iter->second; - } else if (parent_.checkAndRememberRejection(final_name, central_cache_.rejected_stats_, + } else if (parent_.checkAndRememberRejection(final_stat_name, central_cache_.rejected_stats_, tls_rejected_stats)) { - return null_histogram_; + return parent_.null_histogram_; } else { - std::vector tags; - std::string tag_extracted_name = parent_.getTagsForName(final_name, tags); + TagExtraction extraction(parent_, final_stat_name); auto stat = std::make_shared( - final_name, parent_, *this, std::move(tag_extracted_name), std::move(tags)); - central_ref = ¢ral_cache_.histograms_[stat->nameCStr()]; + final_stat_name, parent_, *this, extraction.tagExtractedName(), extraction.tags()); + central_ref = ¢ral_cache_.histograms_[stat->statName()]; *central_ref = stat; } if (tls_cache != nullptr) { - tls_cache->insert(std::make_pair((*central_ref)->nameCStr(), *central_ref)); + tls_cache->insert(std::make_pair((*central_ref)->statName(), *central_ref)); } return **central_ref; } -Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(const std::string& name, +Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(StatName name, ParentHistogramImpl& parent) { // tlsHistogram() is generally not called for a histogram that is rejected by // the matcher, so no further rejection-checking is needed at this level. // TlsHistogram inherits its reject/accept status from ParentHistogram. - // See comments in counter() which explains the logic here. + // See comments in counterFromStatName() which explains the logic here. + StatMap* tls_cache = nullptr; if (!parent_.shutting_down_ && parent_.tls_) { tls_cache = &parent_.tls_->getTyped().scope_cache_[this->scope_id_].histograms_; - auto iter = tls_cache->find(name.c_str()); + auto iter = tls_cache->find(name); if (iter != tls_cache->end()) { return *iter->second; } } std::vector tags; - std::string tag_extracted_name = parent_.getTagsForName(name, tags); - TlsHistogramSharedPtr hist_tls_ptr = std::make_shared( - name, std::move(tag_extracted_name), std::move(tags)); + std::string tag_extracted_name = + parent_.tagProducer().produceTags(symbolTable().toString(name), tags); + TlsHistogramSharedPtr hist_tls_ptr = + std::make_shared(name, tag_extracted_name, tags, symbolTable()); parent.addTlsHistogram(hist_tls_ptr); if (tls_cache) { - tls_cache->insert(std::make_pair(hist_tls_ptr->nameCStr(), hist_tls_ptr)); + tls_cache->insert(std::make_pair(hist_tls_ptr->statName(), hist_tls_ptr)); } return *hist_tls_ptr; } -ThreadLocalHistogramImpl::ThreadLocalHistogramImpl(const std::string& name, - std::string&& tag_extracted_name, - std::vector&& tags) - : MetricImpl(std::move(tag_extracted_name), std::move(tags)), current_active_(0), flags_(0), - created_thread_id_(std::this_thread::get_id()), name_(name) { +ThreadLocalHistogramImpl::ThreadLocalHistogramImpl(StatName name, + absl::string_view tag_extracted_name, + const std::vector& tags, + SymbolTable& symbol_table) + : MetricImpl(tag_extracted_name, tags, symbol_table), current_active_(0), flags_(0), + created_thread_id_(std::this_thread::get_id()), name_(name, symbol_table), + symbol_table_(symbol_table) { histograms_[0] = hist_alloc(); histograms_[1] = hist_alloc(); } ThreadLocalHistogramImpl::~ThreadLocalHistogramImpl() { + MetricImpl::clear(); + name_.free(symbolTable()); hist_free(histograms_[0]); hist_free(histograms_[1]); } @@ -514,21 +561,23 @@ void ThreadLocalHistogramImpl::merge(histogram_t* target) { hist_clear(*other_histogram); } -ParentHistogramImpl::ParentHistogramImpl(const std::string& name, Store& parent, - TlsScope& tls_scope, std::string&& tag_extracted_name, - std::vector&& tags) - : MetricImpl(std::move(tag_extracted_name), std::move(tags)), parent_(parent), +ParentHistogramImpl::ParentHistogramImpl(StatName name, Store& parent, TlsScope& tls_scope, + absl::string_view tag_extracted_name, + const std::vector& tags) + : MetricImpl(tag_extracted_name, tags, parent.symbolTable()), parent_(parent), tls_scope_(tls_scope), interval_histogram_(hist_alloc()), cumulative_histogram_(hist_alloc()), interval_statistics_(interval_histogram_), cumulative_statistics_(cumulative_histogram_), - merged_(false), name_(name) {} + merged_(false), name_(name, parent.symbolTable()) {} ParentHistogramImpl::~ParentHistogramImpl() { + MetricImpl::clear(); + name_.free(symbolTable()); hist_free(interval_histogram_); hist_free(cumulative_histogram_); } void ParentHistogramImpl::recordValue(uint64_t value) { - Histogram& tls_histogram = tls_scope_.tlsHistogram(name(), *this); + Histogram& tls_histogram = tls_scope_.tlsHistogram(statName(), *this); tls_histogram.recordValue(value); parent_.deliverHistogramToSinks(*this, value); } diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index afae6779edc0b..9e223ac3a9a29 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -29,9 +29,9 @@ namespace Stats { */ class ThreadLocalHistogramImpl : public Histogram, public MetricImpl { public: - ThreadLocalHistogramImpl(const std::string& name, std::string&& tag_extracted_name, - std::vector&& tags); - ~ThreadLocalHistogramImpl(); + ThreadLocalHistogramImpl(StatName name, absl::string_view tag_extracted_name, + const std::vector& tags, SymbolTable& symbol_table); + ~ThreadLocalHistogramImpl() override; void merge(histogram_t* target); @@ -50,8 +50,9 @@ class ThreadLocalHistogramImpl : public Histogram, public MetricImpl { bool used() const override { return flags_ & Flags::Used; } // Stats::Metric - std::string name() const override { return name_; } - const char* nameCStr() const override { return name_.c_str(); } + StatName statName() const override { return name_.statName(); } + SymbolTable& symbolTable() override { return symbol_table_; } + const SymbolTable& symbolTable() const override { return symbol_table_; } private: uint64_t otherHistogramIndex() const { return 1 - current_active_; } @@ -59,10 +60,11 @@ class ThreadLocalHistogramImpl : public Histogram, public MetricImpl { histogram_t* histograms_[2]; std::atomic flags_; std::thread::id created_thread_id_; - const std::string name_; + StatNameStorage name_; + SymbolTable& symbol_table_; }; -typedef std::shared_ptr TlsHistogramSharedPtr; +using TlsHistogramSharedPtr = std::shared_ptr; class TlsScope; @@ -71,9 +73,9 @@ class TlsScope; */ class ParentHistogramImpl : public ParentHistogram, public MetricImpl { public: - ParentHistogramImpl(const std::string& name, Store& parent, TlsScope& tlsScope, - std::string&& tag_extracted_name, std::vector&& tags); - ~ParentHistogramImpl(); + ParentHistogramImpl(StatName name, Store& parent, TlsScope& tlsScope, + absl::string_view tag_extracted_name, const std::vector& tags); + ~ParentHistogramImpl() override; void addTlsHistogram(const TlsHistogramSharedPtr& hist_ptr); bool used() const override; @@ -95,8 +97,9 @@ class ParentHistogramImpl : public ParentHistogram, public MetricImpl { const std::string bucketSummary() const override; // Stats::Metric - std::string name() const override { return name_; } - const char* nameCStr() const override { return name_.c_str(); } + StatName statName() const override { return name_.statName(); } + SymbolTable& symbolTable() override { return parent_.symbolTable(); } + const SymbolTable& symbolTable() const override { return parent_.symbolTable(); } private: bool usedLockHeld() const EXCLUSIVE_LOCKS_REQUIRED(merge_lock_); @@ -110,10 +113,10 @@ class ParentHistogramImpl : public ParentHistogram, public MetricImpl { mutable Thread::MutexBasicLockable merge_lock_; std::list tls_histograms_ GUARDED_BY(merge_lock_); bool merged_; - const std::string name_; + StatNameStorage name_; }; -typedef std::shared_ptr ParentHistogramImplSharedPtr; +using ParentHistogramImplSharedPtr = std::shared_ptr; /** * Class used to create ThreadLocalHistogram in the scope. @@ -127,7 +130,7 @@ class TlsScope : public Scope { * @return a ThreadLocalHistogram within the scope's namespace. * @param name name of the histogram with scope prefix attached. */ - virtual Histogram& tlsHistogram(const std::string& name, ParentHistogramImpl& parent) PURE; + virtual Histogram& tlsHistogram(StatName name, ParentHistogramImpl& parent) PURE; }; /** @@ -137,7 +140,7 @@ class TlsScope : public Scope { class ThreadLocalStoreImpl : Logger::Loggable, public StoreRoot { public: ThreadLocalStoreImpl(const Stats::StatsOptions& stats_options, StatDataAllocator& alloc); - ~ThreadLocalStoreImpl(); + ~ThreadLocalStoreImpl() override; // Stats::Scope Counter& counterFromStatName(StatName name) override { @@ -159,6 +162,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; } const SymbolTable& symbolTable() const override { return alloc_.symbolTable(); } SymbolTable& symbolTable() override { return alloc_.symbolTable(); } + const TagProducer& tagProducer() const { return *tag_producer_; } // Stats::Store std::vector counters() const override; @@ -180,9 +184,10 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo Source& source() override { return source_; } const Stats::StatsOptions& statsOptions() const override { return stats_options_; } + absl::string_view truncateStatNameIfNeeded(absl::string_view name); private: - template using StatMap = ConstCharStarHashMap; + template using StatMap = StatNameHashMap; struct TlsCacheEntry { StatMap counters_; @@ -193,44 +198,55 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo // We keep a TLS cache of rejected stat names. This costs memory, but // reduces runtime overhead running the matcher. Moreover, once symbol // tables are integrated, rejection will need the fully elaborated string, - // and it we need to take a global symbol-table lock to run. We keep - // this char* map here in the TLS cache to avoid taking a lock to compute + // and it we need to take a global symbol-table lock to run. We keep this + // StatName set here in the TLS cache to avoid taking a lock to compute // rejection. - SharedStringSet rejected_stats_; + StatNameHashSet rejected_stats_; }; struct CentralCacheEntry { StatMap counters_; StatMap gauges_; StatMap histograms_; - SharedStringSet rejected_stats_; + StatNameStorageSet rejected_stats_; }; struct ScopeImpl : public TlsScope { - ScopeImpl(ThreadLocalStoreImpl& parent, const std::string& prefix) - : scope_id_(next_scope_id_++), parent_(parent), - prefix_(Utility::sanitizeStatsName(prefix)) {} - ~ScopeImpl(); + ScopeImpl(ThreadLocalStoreImpl& parent, const std::string& prefix); + ~ScopeImpl() override; // Stats::Scope - Counter& counter(const std::string& name) override; + Counter& counterFromStatName(StatName name) override; + void deliverHistogramToSinks(const Histogram& histogram, uint64_t value) override; + Gauge& gaugeFromStatName(StatName name) override; + Histogram& histogramFromStatName(StatName name) override; + Histogram& tlsHistogram(StatName name, ParentHistogramImpl& parent) override; + const Stats::StatsOptions& statsOptions() const override { return parent_.statsOptions(); } ScopePtr createScope(const std::string& name) override { - return parent_.createScope(prefix_ + name); + return parent_.createScope(symbolTable().toString(prefix_.statName()) + "." + name); } const SymbolTable& symbolTable() const override { return parent_.symbolTable(); } SymbolTable& symbolTable() override { return parent_.symbolTable(); } - void deliverHistogramToSinks(const Histogram& histogram, uint64_t value) override; - Gauge& gauge(const std::string& name) override; - NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; } - Histogram& histogram(const std::string& name) override; - Histogram& tlsHistogram(const std::string& name, ParentHistogramImpl& parent) override; - const Stats::StatsOptions& statsOptions() const override { return parent_.statsOptions(); } + + Counter& counter(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return counterFromStatName(storage.statName()); + } + Gauge& gauge(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return gaugeFromStatName(storage.statName()); + } + Histogram& histogram(const std::string& name) override { + StatNameManagedStorage storage(name, symbolTable()); + return histogramFromStatName(storage.statName()); + } + + NullGaugeImpl& nullGauge(const std::string&) override { return parent_.null_gauge_; } template - using MakeStatFn = - std::function(StatDataAllocator&, absl::string_view name, - std::string&& tag_extracted_name, - std::vector&& tags)>; + using MakeStatFn = std::function(StatDataAllocator&, StatName name, + absl::string_view tag_extracted_name, + const std::vector& tags)>; /** * Makes a stat either by looking it up in the central cache, @@ -244,32 +260,22 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo * used if non-empty, or filled in if empty (and non-null). */ template - StatType& safeMakeStat(const std::string& name, - StatMap>& central_cache_map, - SharedStringSet& central_rejected_stats_, MakeStatFn make_stat, + StatType& safeMakeStat(StatName name, StatMap>& central_cache_map, + StatNameStorageSet& central_rejected_stats, + MakeStatFn make_stat, StatMap>* tls_cache, - SharedStringSet* tls_rejected_stats, StatType& null_stat); - - Counter& counterFromStatName(StatName name) override { - return counter(symbolTable().toString(name)); - } - - Gauge& gaugeFromStatName(StatName name) override { return gauge(symbolTable().toString(name)); } + StatNameHashSet* tls_rejected_stats, StatType& null_stat); - Histogram& histogramFromStatName(StatName name) override { - return histogram(symbolTable().toString(name)); - } + void extractTagsAndTruncate(StatName& name, + std::unique_ptr& truncated_name_storage, + std::vector& tags, std::string& tag_extracted_name); static std::atomic next_scope_id_; const uint64_t scope_id_; ThreadLocalStoreImpl& parent_; - const std::string prefix_; + StatNameStorage prefix_; CentralCacheEntry central_cache_; - - NullCounterImpl null_counter_; - NullGaugeImpl null_gauge_; - NullHistogramImpl null_histogram_; }; struct TlsCache : public ThreadLocal::ThreadLocalObject { @@ -284,16 +290,15 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo }; std::string getTagsForName(const std::string& name, std::vector& tags) const; - void clearScopeFromCaches(uint64_t scope_id); + void clearScopeFromCaches(uint64_t scope_id, const Event::PostCb& clean_central_cache); void releaseScopeCrossThread(ScopeImpl* scope); void mergeInternal(PostMergeCb mergeCb); - absl::string_view truncateStatNameIfNeeded(absl::string_view name); - bool rejects(const std::string& name) const; + bool rejects(StatName name) const; bool rejectsAll() const { return stats_matcher_->rejectsAll(); } template void removeRejectedStats(StatMapClass& map, StatListClass& list); - bool checkAndRememberRejection(const std::string& name, SharedStringSet& central_rejected_stats, - SharedStringSet* tls_rejected_stats); + bool checkAndRememberRejection(StatName name, StatNameStorageSet& central_rejected_stats, + StatNameHashSet* tls_rejected_stats); const Stats::StatsOptions& stats_options_; StatDataAllocator& alloc_; @@ -307,10 +312,14 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo StatsMatcherPtr stats_matcher_; std::atomic shutting_down_{}; std::atomic merge_in_progress_{}; + StatNameStorage stats_overflow_; Counter& num_last_resort_stats_; HeapStatDataAllocator heap_allocator_; SourceImpl source_; + + NullCounterImpl null_counter_; NullGaugeImpl null_gauge_; + NullHistogramImpl null_histogram_; // Retain storage for deleted stats; these are no longer in maps because the // matcher-pattern was established after they were created. Since the stats diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index c5c980ee4e274..7d4cd4cb2d37f 100644 --- a/source/common/stats/utility.cc +++ b/source/common/stats/utility.cc @@ -3,10 +3,18 @@ #include #include +#include "absl/strings/match.h" + namespace Envoy { namespace Stats { std::string Utility::sanitizeStatsName(absl::string_view name) { + if (absl::EndsWith(name, ".")) { + name.remove_suffix(1); + } + if (absl::StartsWith(name, ".")) { + name.remove_prefix(1); + } std::string stats_name = std::string(name); std::replace(stats_name.begin(), stats_name.end(), ':', '_'); std::replace(stats_name.begin(), stats_name.end(), '\0', '_'); diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 58872d0ce77e0..f1cf255e47d73 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -14,7 +14,7 @@ class Utility { public: // ':' is a reserved char in statsd. Do a character replacement to avoid costly inline // translations later. - static std::string sanitizeStatsName(const absl::string_view name); + static std::string sanitizeStatsName(absl::string_view name); }; } // namespace Stats diff --git a/source/docs/stats.md b/source/docs/stats.md index 357efec2c6e08..ad088720cf350 100644 --- a/source/docs/stats.md +++ b/source/docs/stats.md @@ -101,7 +101,7 @@ followed. Stat names are replicated in several places in various forms. - * Held fully elaborated next to the values, in `RawStatData` and `HeapStatData` + * Held with the stat values, in `RawStatData` and `HeapStatData` * 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. * In static strings across the codebase where stats are referenced @@ -121,6 +121,58 @@ the key. If the `.find` fails, the actual stat must be constructed first, and then inserted into the map using its key storage. This strategy saves duplication of the keys, but costs an extra map lookup on each miss. +### Naming Representation + +When stored as flat strings, stat names can dominate Envoy memory usage when +there are a large number of clusters. Stat names typically combine a small +number of keywords, cluster names, host names, and response codes, separated by +`.`. For example `CLUSTER.upstream_cx_connect_attempts_exceeded`. There may be +thousands of clusters, and roughly 100 stats per cluster. Thus, the number +of combinations can be large. It is significantly more efficient to symbolize +each `.`-delimited token and represent stats as arrays of symbols. + +The transformation between flattened string and symbolized form is CPU-intensive +at scale. It requires parsing, encoding, and lookups in a shared map, which must +be mutex-protected. To avoid adding latency and CPU overhead while serving +requests, the tokens can be symbolized and saved in context classes, such as +[Http::CodeStatsImpl](https://github.com/envoyproxy/envoy/blob/master/source/common/http/codes.h). +Symbolization can occur on startup or when new hosts or clusters are configured +dynamically. Users of stats that are allocated dynamically per cluster, host, +etc, must explicitly store partial stat-names their class instances, which later +can be composed dynamically at runtime in order to fully elaborate counters, +gauges, etc, without taking symbol-table locks, via `SymbolTable::join()`. + +### Current State and Strategy To Deploy Symbol Tables + +As of April 1, 2019, there are a fairly large number of files that directly +lookup stats by name, e.g. via `Stats::Scope::counter(const std::string&)` in +the request path. In most cases, this runtime lookup concatenates the scope name +with a string literal or other request-dependent token to form the stat name, so +it is not possible to fully memoize the stats at startup; there must be a +runtime name lookup. + +If a PR is issued that changes the underlying representation of a stat name to +be a symbol table entry then each stat-name will need to be transformed +whenever names are looked up, which would add CPU overhead and lock contention +in the request-path, violating one of the principles of Envoy's [threading +model](https://blog.envoyproxy.io/envoy-threading-model-a8d44b922310). Before +issuing such a PR we need to first iterate through the codebase memoizing the +symbols that are used to form stat-names. + +To resolve this chicken-and-egg challenge of switching to symbol-table stat-name +representation without suffering a temporary loss of performance, we employ a +["fake" symbol table +implementation](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/fake_symbol_table_impl.h). +This implemenation uses elaborated strings as an underlying representation, but +implements the same API as the ["real" +implemention](https://github.com/envoyproxy/envoy/blob/master/source/common/stats/symbol_table_impl.h). +The underlying string representation means that there is minimal runtime +overhead compared to the current state. But once all stat-allocation call-sites +have been converted to use the abstract [SymbolTable +API](https://github.com/envoyproxy/envoy/blob/master/include/envoy/stats/symbol_table.h), +the real implementation can be swapped in, the space savings realized, and the +fake implementation deleted. + ## Tags and Tag Extraction TBD diff --git a/source/extensions/stat_sinks/hystrix/BUILD b/source/extensions/stat_sinks/hystrix/BUILD index 418c680513cca..388ad3dde8bca 100644 --- a/source/extensions/stat_sinks/hystrix/BUILD +++ b/source/extensions/stat_sinks/hystrix/BUILD @@ -37,5 +37,6 @@ envoy_cc_library( "//source/common/common:logger_lib", "//source/common/config:well_known_names", "//source/common/http:headers_lib", + "//source/common/stats:symbol_table_lib", ], ) diff --git a/source/extensions/stat_sinks/hystrix/hystrix.cc b/source/extensions/stat_sinks/hystrix/hystrix.cc index 339b7c02d1cda..f8442b2c9d52e 100644 --- a/source/extensions/stat_sinks/hystrix/hystrix.cc +++ b/source/extensions/stat_sinks/hystrix/hystrix.cc @@ -267,7 +267,8 @@ const std::string HystrixSink::printRollingWindows() { HystrixSink::HystrixSink(Server::Instance& server, const uint64_t num_buckets) : server_(server), current_index_(num_buckets > 0 ? num_buckets : DEFAULT_NUM_BUCKETS), - window_size_(current_index_ + 1) { + window_size_(current_index_ + 1), + cluster_upstream_rq_time_("cluster.upstream_rq_time", server.stats().symbolTable()) { Server::Admin& admin = server_.admin(); ENVOY_LOG(debug, "adding hystrix_event_stream endpoint to enable connection to hystrix dashboard"); @@ -327,12 +328,12 @@ void HystrixSink::flush(Stats::Source& source) { // Save a map of the relevant histograms per cluster in a convenient format. std::unordered_map time_histograms; for (const Stats::ParentHistogramSharedPtr& histogram : source.cachedHistograms()) { - if (histogram->tagExtractedName() == "cluster.upstream_rq_time") { + if (histogram->tagExtractedStatName() == cluster_upstream_rq_time_.statName()) { // TODO(mrice32): add an Envoy utility function to look up and return a tag for a metric. - auto it = std::find_if(histogram->tags().begin(), histogram->tags().end(), - [](const Stats::Tag& tag) { - return (tag.name_ == Config::TagNames::get().CLUSTER_NAME); - }); + auto tags = histogram->tags(); + auto it = std::find_if(tags.begin(), tags.end(), [](const Stats::Tag& tag) { + return (tag.name_ == Config::TagNames::get().CLUSTER_NAME); + }); // Make sure we found the cluster name tag ASSERT(it != histogram->tags().end()); diff --git a/source/extensions/stat_sinks/hystrix/hystrix.h b/source/extensions/stat_sinks/hystrix/hystrix.h index 54d26953abc8c..c37a98db15c6e 100644 --- a/source/extensions/stat_sinks/hystrix/hystrix.h +++ b/source/extensions/stat_sinks/hystrix/hystrix.h @@ -10,6 +10,8 @@ #include "envoy/stats/sink.h" #include "envoy/stats/source.h" +#include "common/stats/symbol_table_impl.h" + namespace Envoy { namespace Extensions { namespace StatSinks { @@ -155,6 +157,9 @@ class HystrixSink : public Stats::Sink, public Logger::Loggable cluster_stats_cache_map_; + + // Saved StatName for "cluster.upstream_rq_time" for fast comparisons in loop. + Stats::StatNameManagedStorage cluster_upstream_rq_time_; }; typedef std::unique_ptr HystrixSinkPtr; diff --git a/test/common/stats/heap_stat_data_test.cc b/test/common/stats/heap_stat_data_test.cc index 6f8ee288d1fde..faab893e4e50a 100644 --- a/test/common/stats/heap_stat_data_test.cc +++ b/test/common/stats/heap_stat_data_test.cc @@ -15,10 +15,28 @@ namespace { class HeapStatDataTest : public testing::Test { protected: HeapStatDataTest() : alloc_(symbol_table_) {} - ~HeapStatDataTest() {} + ~HeapStatDataTest() { clearStorage(); } + + StatNameStorage makeStatStorage(absl::string_view name) { + return StatNameStorage(name, symbol_table_); + } + + StatName makeStat(absl::string_view name) { + stat_name_storage_.emplace_back(makeStatStorage(name)); + return stat_name_storage_.back().statName(); + } + + void clearStorage() { + for (auto& stat_name_storage : stat_name_storage_) { + stat_name_storage.free(symbol_table_); + } + stat_name_storage_.clear(); + EXPECT_EQ(0, symbol_table_.numSymbols()); + } FakeSymbolTableImpl symbol_table_; HeapStatDataAllocator alloc_; + std::vector stat_name_storage_; }; // No truncation occurs in the implementation of HeapStatData. @@ -26,19 +44,20 @@ class HeapStatDataTest : public testing::Test { TEST_F(HeapStatDataTest, HeapNoTruncate) { StatsOptionsImpl stats_options; const std::string long_string(stats_options.maxNameLength() + 1, 'A'); + StatName stat_name = makeStat(long_string); HeapStatData* stat{}; - EXPECT_NO_LOGS(stat = alloc_.alloc(long_string)); - EXPECT_EQ(stat->key(), long_string); + EXPECT_NO_LOGS(stat = &alloc_.alloc(stat_name)); + EXPECT_EQ(stat->statName(), stat_name); alloc_.free(*stat); -} +}; // Note: a similar test using RawStatData* is in raw_stat_data_test.cc. TEST_F(HeapStatDataTest, HeapAlloc) { - HeapStatData* stat_1 = alloc_.alloc("ref_name"); + HeapStatData* stat_1 = &alloc_.alloc(makeStat("ref_name")); ASSERT_NE(stat_1, nullptr); - HeapStatData* stat_2 = alloc_.alloc("ref_name"); + HeapStatData* stat_2 = &alloc_.alloc(makeStat("ref_name")); ASSERT_NE(stat_2, nullptr); - HeapStatData* stat_3 = alloc_.alloc("not_ref_name"); + HeapStatData* stat_3 = &alloc_.alloc(makeStat("not_ref_name")); ASSERT_NE(stat_3, nullptr); EXPECT_EQ(stat_1, stat_2); EXPECT_NE(stat_1, stat_3); diff --git a/test/common/stats/isolated_store_impl_test.cc b/test/common/stats/isolated_store_impl_test.cc index 50c4aea7d0d95..c9e9c2cd49b7e 100644 --- a/test/common/stats/isolated_store_impl_test.cc +++ b/test/common/stats/isolated_store_impl_test.cc @@ -54,7 +54,7 @@ TEST_F(StatsIsolatedStoreImplTest, All) { 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()); Histogram& h1 = store_.histogram("h1"); Histogram& h2 = scope1->histogram("h2"); diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index 7391f45ecfbb7..a53e6ad8979c4 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/test/common/stats/symbol_table_impl_test.cc @@ -396,11 +396,11 @@ TEST_P(StatNameTest, RacingSymbolCreation) { // Block each thread on waking up a common condition variable, // so we make it likely to race on creation. creation.wait(); - StatNameTempStorage initial(stat_name_string, *table_); + StatNameManagedStorage initial(stat_name_string, *table_); creates.DecrementCount(); access.wait(); - StatNameTempStorage second(stat_name_string, *table_); + StatNameManagedStorage second(stat_name_string, *table_); accesses.DecrementCount(); wait.wait(); @@ -462,11 +462,11 @@ TEST_P(StatNameTest, MutexContentionOnExistingSymbols) { // Block each thread on waking up a common condition variable, // so we make it likely to race on creation. creation.wait(); - StatNameTempStorage initial(stat_name_string, *table_); + StatNameManagedStorage initial(stat_name_string, *table_); creates.DecrementCount(); access.wait(); - StatNameTempStorage second(stat_name_string, *table_); + StatNameManagedStorage second(stat_name_string, *table_); accesses.DecrementCount(); wait.wait(); @@ -513,11 +513,11 @@ TEST_P(StatNameTest, SharedStatNameStorageSetInsertAndFind) { for (int i = 0; i < iters; ++i) { std::string foo = absl::StrCat("foo", i); auto insertion = set.insert(StatNameStorage(foo, *table_)); - StatNameTempStorage temp_foo(foo, *table_); + StatNameManagedStorage temp_foo(foo, *table_); auto found = set.find(temp_foo.statName()); EXPECT_EQ(found->statName().data(), insertion.first->statName().data()); } - StatNameTempStorage bar("bar", *table_); + StatNameManagedStorage bar("bar", *table_); EXPECT_EQ(set.end(), set.find(bar.statName())); EXPECT_EQ(iters, set.size()); set.free(*table_); diff --git a/test/common/stats/thread_local_store_speed_test.cc b/test/common/stats/thread_local_store_speed_test.cc index 99f6c48ca4228..76ac8117a95f6 100644 --- a/test/common/stats/thread_local_store_speed_test.cc +++ b/test/common/stats/thread_local_store_speed_test.cc @@ -26,9 +26,16 @@ class ThreadLocalStorePerf { : heap_alloc_(symbol_table_), store_(options_, heap_alloc_), api_(Api::createApiForTest(store_, time_system_)) { store_.setTagProducer(std::make_unique(stats_config_)); + + Stats::TestUtil::forEachSampleStat(1000, [this](absl::string_view name) { + stat_names_.push_back(std::make_unique(name, symbol_table_)); + }); } ~ThreadLocalStorePerf() { + for (auto& stat_name_storage : stat_names_) { + stat_name_storage->free(symbol_table_); + } store_.shutdownThreading(); if (tls_) { tls_->shutdownGlobalThreading(); @@ -36,8 +43,9 @@ class ThreadLocalStorePerf { } void accessCounters() { - Stats::TestUtil::forEachSampleStat( - 1000, [this](absl::string_view name) { store_.counter(std::string(name)); }); + for (auto& stat_name_storage : stat_names_) { + store_.counterFromStatName(stat_name_storage->statName()); + } } void initThreading() { @@ -56,6 +64,7 @@ class ThreadLocalStorePerf { Event::DispatcherPtr dispatcher_; std::unique_ptr tls_; envoy::config::metrics::v2::StatsConfig stats_config_; + std::vector> stat_names_; }; } // namespace Envoy diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index c4b01a921aa3a..86c883b7412a4 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -312,7 +312,7 @@ TEST_F(StatsThreadLocalStoreTest, ScopeDelete) { EXPECT_EQ(TestUtility::findByName(store_->source().cachedCounters(), "scope1.c1"), c1); EXPECT_CALL(main_thread_dispatcher_, post(_)); - EXPECT_CALL(tls_, runOnAllThreads(_)); + EXPECT_CALL(tls_, runOnAllThreads(_, _)); scope1.reset(); EXPECT_EQ(1UL, store_->counters().size()); EXPECT_EQ(2UL, store_->source().cachedCounters().size()); @@ -447,11 +447,13 @@ TEST_F(StatsThreadLocalStoreTest, HotRestartTruncation) { // The stats did not overflow yet. EXPECT_EQ(0UL, store_->counter("stats.overflow").value()); - // The name will be truncated, so we won't be able to find it with the entire name. - EXPECT_EQ(nullptr, TestUtility::findCounter(*store_, name_1).get()); + // Truncation occurs in the underlying representation, but the by-name lookups + // are all based on the untruncated name. + EXPECT_NE(nullptr, TestUtility::findCounter(*store_, name_1).get()); - // But we can find it based on the expected truncation. - EXPECT_NE(nullptr, TestUtility::findCounter(*store_, name_1.substr(0, max_name_length)).get()); + // Outside the stats system, no Envoy code can see the truncated view, so + // lookups for truncated names will fail. + EXPECT_EQ(nullptr, TestUtility::findCounter(*store_, name_1.substr(0, max_name_length)).get()); // The same should be true with heap allocation, which occurs when the default // allocator fails. @@ -459,11 +461,11 @@ TEST_F(StatsThreadLocalStoreTest, HotRestartTruncation) { EXPECT_CALL(*alloc_, alloc(_)).WillOnce(Return(nullptr)); store_->counter(name_2); - // Same deal: the name will be truncated, so we won't be able to find it with the entire name. - EXPECT_EQ(nullptr, TestUtility::findCounter(*store_, name_1).get()); + // Same deal: the name will be truncated, but we find it with the entire name. + EXPECT_NE(nullptr, TestUtility::findCounter(*store_, name_1).get()); - // But we can find it based on the expected truncation. - EXPECT_NE(nullptr, TestUtility::findCounter(*store_, name_1.substr(0, max_name_length)).get()); + // But we can't find it based on the truncation -- that name is not visible at the API. + EXPECT_EQ(nullptr, TestUtility::findCounter(*store_, name_1.substr(0, max_name_length)).get()); // Now the stats have overflowed. EXPECT_EQ(1UL, store_->counter("stats.overflow").value()); @@ -928,7 +930,7 @@ TEST_F(HeapStatsThreadLocalStoreTest, MemoryWithoutTls) { 1000, [this](absl::string_view name) { store_->counter(std::string(name)); }); const size_t end_mem = Memory::Stats::totalCurrentlyAllocated(); EXPECT_LT(start_mem, end_mem); - EXPECT_LT(end_mem - start_mem, 28 * million); // actual value: 27203216 as of Oct 29, 2018 + EXPECT_LT(end_mem - start_mem, 20 * million); // actual value: 19601552 as of March 14, 2019 } TEST_F(HeapStatsThreadLocalStoreTest, MemoryWithTls) { @@ -951,7 +953,7 @@ TEST_F(HeapStatsThreadLocalStoreTest, MemoryWithTls) { 1000, [this](absl::string_view name) { store_->counter(std::string(name)); }); const size_t end_mem = Memory::Stats::totalCurrentlyAllocated(); EXPECT_LT(start_mem, end_mem); - EXPECT_LT(end_mem - start_mem, 31 * million); // actual value: 30482576 as of Oct 29, 2018 + EXPECT_LT(end_mem - start_mem, 23 * million); // actual value: 22880912 as of March 14, 2019 } TEST_F(StatsThreadLocalStoreTest, ShuttingDown) { diff --git a/test/extensions/stats_sinks/hystrix/hystrix_test.cc b/test/extensions/stats_sinks/hystrix/hystrix_test.cc index 26593d9b58efa..2f52df159d198 100644 --- a/test/extensions/stats_sinks/hystrix/hystrix_test.cc +++ b/test/extensions/stats_sinks/hystrix/hystrix_test.cc @@ -462,17 +462,8 @@ TEST_F(HystrixSinkTest, HistogramTest) { // Create histogram for the Hystrix sink to read. auto histogram = std::make_shared>(); histogram->name_ = "cluster." + cluster1_name_ + ".upstream_rq_time"; - const std::string tag_extracted_name = "cluster.upstream_rq_time"; - ON_CALL(*histogram, tagExtractedName()) - .WillByDefault(testing::ReturnRefOfCopy(tag_extracted_name)); - std::vector tags; - Stats::Tag tag = { - Config::TagNames::get().CLUSTER_NAME, // name_ - cluster1_name_ // value_ - }; - tags.emplace_back(tag); - ON_CALL(*histogram, tags()).WillByDefault(testing::ReturnRef(tags)); - + histogram->setTagExtractedName("cluster.upstream_rq_time"); + histogram->tags_.emplace_back(Stats::Tag{Config::TagNames::get().CLUSTER_NAME, cluster1_name_}); histogram->used_ = true; // Init with data such that the quantile value is equal to the quantile. diff --git a/test/integration/server.h b/test/integration/server.h index 844036d83619f..d8cd3cbb840a3 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -94,15 +94,15 @@ class TestScopeWrapper : public Scope { } Counter& counter(const std::string& name) override { - StatNameTempStorage storage(name, symbolTable()); + StatNameManagedStorage storage(name, symbolTable()); return counterFromStatName(storage.statName()); } Gauge& gauge(const std::string& name) override { - StatNameTempStorage storage(name, symbolTable()); + StatNameManagedStorage storage(name, symbolTable()); return gaugeFromStatName(storage.statName()); } Histogram& histogram(const std::string& name) override { - StatNameTempStorage storage(name, symbolTable()); + StatNameManagedStorage storage(name, symbolTable()); return histogramFromStatName(storage.statName()); } diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 40e0c5ae1517e..e82b274ccc559 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -209,8 +209,9 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithStats) { // 2019/03/20 6329 59015 Initial version // 2019/04/12 6477 59576 Implementing Endpoint lease... // 2019/04/23 6659 59512 Reintroduce dispatcher stats... + // 2019/04/24 6161 49415 Pack tags and tag extracted names - EXPECT_EQ(m_per_cluster, 59512); + EXPECT_EQ(m_per_cluster, 49415); } } // namespace diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index 9b1b4ea76904c..bea653c71e84c 100644 --- a/test/mocks/stats/mocks.cc +++ b/test/mocks/stats/mocks.cc @@ -17,9 +17,27 @@ using testing::ReturnRef; namespace Envoy { namespace Stats { +MockMetric::MockMetric() : name_(*this) {} +MockMetric::~MockMetric() {} + +MockMetric::MetricName::~MetricName() { + if (stat_name_storage_ != nullptr) { + stat_name_storage_->free(*mock_metric_.symbol_table_); + } +} + +void MockMetric::setTagExtractedName(absl::string_view name) { + tag_extracted_name_ = std::string(name); + tag_extracted_stat_name_ = + std::make_unique(tagExtractedName(), *symbol_table_); +} + +void MockMetric::MetricName::MetricName::operator=(absl::string_view name) { + name_ = std::string(name); + stat_name_storage_ = std::make_unique(name, mock_metric_.symbolTable()); +} + MockCounter::MockCounter() { - ON_CALL(*this, tagExtractedName()).WillByDefault(ReturnRef(name_)); - ON_CALL(*this, tags()).WillByDefault(ReturnRef(tags_)); ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_)); ON_CALL(*this, value()).WillByDefault(ReturnPointee(&value_)); ON_CALL(*this, latch()).WillByDefault(ReturnPointee(&latch_)); @@ -27,8 +45,6 @@ MockCounter::MockCounter() { MockCounter::~MockCounter() {} MockGauge::MockGauge() { - ON_CALL(*this, tagExtractedName()).WillByDefault(ReturnRef(name_)); - ON_CALL(*this, tags()).WillByDefault(ReturnRef(tags_)); ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_)); ON_CALL(*this, value()).WillByDefault(ReturnPointee(&value_)); } @@ -40,10 +56,7 @@ MockHistogram::MockHistogram() { store_->deliverHistogramToSinks(*this, value); } })); - ON_CALL(*this, tagExtractedName()).WillByDefault(ReturnRef(name_)); - ON_CALL(*this, tags()).WillByDefault(ReturnRef(tags_)); } - MockHistogram::~MockHistogram() {} MockParentHistogram::MockParentHistogram() { @@ -52,13 +65,10 @@ MockParentHistogram::MockParentHistogram() { store_->deliverHistogramToSinks(*this, value); } })); - ON_CALL(*this, tagExtractedName()).WillByDefault(ReturnRef(name_)); - ON_CALL(*this, tags()).WillByDefault(ReturnRef(tags_)); ON_CALL(*this, intervalStatistics()).WillByDefault(ReturnRef(*histogram_stats_)); ON_CALL(*this, cumulativeStatistics()).WillByDefault(ReturnRef(*histogram_stats_)); ON_CALL(*this, used()).WillByDefault(ReturnPointee(&used_)); } - MockParentHistogram::~MockParentHistogram() {} MockSource::MockSource() { @@ -75,7 +85,7 @@ MockSink::~MockSink() {} MockStore::MockStore() : StoreImpl(*fake_symbol_table_) { ON_CALL(*this, counter(_)).WillByDefault(ReturnRef(counter_)); ON_CALL(*this, histogram(_)).WillByDefault(Invoke([this](const std::string& name) -> Histogram& { - auto* histogram = new NiceMock; + auto* histogram = new NiceMock(); // symbol_table_); histogram->name_ = name; histogram->store_ = this; histograms_.emplace_back(histogram); diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index a605c08bb5437..37edb5e6a6a38 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -27,21 +27,61 @@ namespace Envoy { namespace Stats { -class MockCounter : public Counter { +class MockMetric : public virtual Metric { public: - MockCounter(); - ~MockCounter(); + MockMetric(); + ~MockMetric(); + + // This bit of C++ subterfuge allows us to support the wealth of tests that + // do metric->name_ = "foo" even though names are more complex now. Note + // that the statName is only populated if there is a symbol table. + class MetricName { + public: + explicit MetricName(MockMetric& mock_metric) : mock_metric_(mock_metric) {} + ~MetricName(); + + void operator=(absl::string_view str); + + std::string name() const { return name_; } + StatName statName() const { return stat_name_storage_->statName(); } + + private: + MockMetric& mock_metric_; + std::string name_; + std::unique_ptr stat_name_storage_; + }; + + SymbolTable& symbolTable() override { return symbol_table_.get(); } + const SymbolTable& symbolTable() const override { return symbol_table_.get(); } // Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This // creates a deadlock in gmock and is an unintended use of mock functions. - std::string name() const override { return name_; }; - const char* nameCStr() const override { return name_.c_str(); }; + std::string name() const override { return name_.name(); } + StatName statName() const override { return name_.statName(); } + std::vector tags() const override { return tags_; } + void setTagExtractedName(absl::string_view name); + std::string tagExtractedName() const override { + return tag_extracted_name_.empty() ? name() : tag_extracted_name_; + } + StatName tagExtractedStatName() const override { return tag_extracted_stat_name_->statName(); } + + Test::Global symbol_table_; // Must outlive name_. + MetricName name_; + std::vector tags_; + +private: + std::string tag_extracted_name_; + std::unique_ptr tag_extracted_stat_name_; +}; + +class MockCounter : public Counter, public MockMetric { +public: + MockCounter(); + ~MockCounter(); MOCK_METHOD1(add, void(uint64_t amount)); MOCK_METHOD0(inc, void()); MOCK_METHOD0(latch, uint64_t()); - MOCK_CONST_METHOD0(tagExtractedName, const std::string&()); - MOCK_CONST_METHOD0(tags, const std::vector&()); MOCK_METHOD0(reset, void()); MOCK_CONST_METHOD0(used, bool()); MOCK_CONST_METHOD0(value, uint64_t()); @@ -49,25 +89,16 @@ class MockCounter : public Counter { bool used_; uint64_t value_; uint64_t latch_; - std::string name_; - std::vector tags_; }; -class MockGauge : public Gauge { +class MockGauge : public Gauge, public MockMetric { public: MockGauge(); ~MockGauge(); - // Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This - // creates a deadlock in gmock and is an unintended use of mock functions. - std::string name() const override { return name_; }; - const char* nameCStr() const override { return name_.c_str(); }; - MOCK_METHOD1(add, void(uint64_t amount)); MOCK_METHOD0(dec, void()); MOCK_METHOD0(inc, void()); - MOCK_CONST_METHOD0(tagExtractedName, const std::string&()); - MOCK_CONST_METHOD0(tags, const std::vector&()); MOCK_METHOD1(set, void(uint64_t value)); MOCK_METHOD1(sub, void(uint64_t amount)); MOCK_CONST_METHOD0(used, bool()); @@ -75,52 +106,33 @@ class MockGauge : public Gauge { bool used_; uint64_t value_; - std::string name_; - std::vector tags_; }; -class MockHistogram : public Histogram { +class MockHistogram : public Histogram, public MockMetric { public: MockHistogram(); ~MockHistogram(); - // Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This - // creates a deadlock in gmock and is an unintended use of mock functions. - std::string name() const override { return name_; }; - const char* nameCStr() const override { return name_.c_str(); }; - - MOCK_CONST_METHOD0(tagExtractedName, const std::string&()); - MOCK_CONST_METHOD0(tags, const std::vector&()); MOCK_METHOD1(recordValue, void(uint64_t value)); MOCK_CONST_METHOD0(used, bool()); - std::string name_; - std::vector tags_; Store* store_; }; -class MockParentHistogram : public ParentHistogram { +class MockParentHistogram : public ParentHistogram, public MockMetric { public: MockParentHistogram(); ~MockParentHistogram(); - // Note: cannot be mocked because it is accessed as a Property in a gmock EXPECT_CALL. This - // creates a deadlock in gmock and is an unintended use of mock functions. - std::string name() const override { return name_; }; - const char* nameCStr() const override { return name_.c_str(); }; void merge() override {} const std::string quantileSummary() const override { return ""; }; const std::string bucketSummary() const override { return ""; }; MOCK_CONST_METHOD0(used, bool()); - MOCK_CONST_METHOD0(tagExtractedName, const std::string&()); - MOCK_CONST_METHOD0(tags, const std::vector&()); MOCK_METHOD1(recordValue, void(uint64_t value)); MOCK_CONST_METHOD0(cumulativeStatistics, const HistogramStatistics&()); MOCK_CONST_METHOD0(intervalStatistics, const HistogramStatistics&()); - std::string name_; - std::vector tags_; bool used_; Store* store_; std::shared_ptr histogram_stats_ = @@ -174,6 +186,18 @@ class MockStore : public SymbolTableProvider, public StoreImpl { MOCK_CONST_METHOD0(histograms, std::vector()); MOCK_CONST_METHOD0(statsOptions, const StatsOptions&()); + Counter& counterFromStatName(StatName name) override { + return counter(symbol_table_->toString(name)); + } + Gauge& gaugeFromStatName(StatName name) override { return gauge(symbol_table_->toString(name)); } + Histogram& histogramFromStatName(StatName name) override { + return histogram(symbol_table_->toString(name)); + } + + SymbolTable& symbolTable() override { return symbol_table_.get(); } + const SymbolTable& symbolTable() const override { return symbol_table_.get(); } + + Test::Global symbol_table_; testing::NiceMock counter_; std::vector> histograms_; StatsOptionsImpl stats_options_; diff --git a/test/mocks/thread_local/mocks.cc b/test/mocks/thread_local/mocks.cc index 5ccc69fcf21d9..aff789b21936b 100644 --- a/test/mocks/thread_local/mocks.cc +++ b/test/mocks/thread_local/mocks.cc @@ -11,7 +11,9 @@ namespace ThreadLocal { MockInstance::MockInstance() { ON_CALL(*this, allocateSlot()).WillByDefault(Invoke(this, &MockInstance::allocateSlot_)); - ON_CALL(*this, runOnAllThreads(_)).WillByDefault(Invoke(this, &MockInstance::runOnAllThreads_)); + ON_CALL(*this, runOnAllThreads(_)).WillByDefault(Invoke(this, &MockInstance::runOnAllThreads1_)); + ON_CALL(*this, runOnAllThreads(_, _)) + .WillByDefault(Invoke(this, &MockInstance::runOnAllThreads2_)); ON_CALL(*this, shutdownThread()).WillByDefault(Invoke(this, &MockInstance::shutdownThread_)); ON_CALL(*this, dispatcher()).WillByDefault(ReturnRef(dispatcher_)); } diff --git a/test/mocks/thread_local/mocks.h b/test/mocks/thread_local/mocks.h index af871695420dd..24393722887a7 100644 --- a/test/mocks/thread_local/mocks.h +++ b/test/mocks/thread_local/mocks.h @@ -18,6 +18,7 @@ class MockInstance : public Instance { ~MockInstance(); MOCK_METHOD1(runOnAllThreads, void(Event::PostCb cb)); + MOCK_METHOD2(runOnAllThreads, void(Event::PostCb cb, Event::PostCb main_callback)); // Server::ThreadLocal MOCK_METHOD0(allocateSlot, SlotPtr()); @@ -27,8 +28,8 @@ class MockInstance : public Instance { MOCK_METHOD0(dispatcher, Event::Dispatcher&()); SlotPtr allocateSlot_() { return SlotPtr{new SlotImpl(*this, current_slot_++)}; } - void runOnAllThreads_(Event::PostCb cb) { cb(); } - void runOnAllThreads(Event::PostCb cb, Event::PostCb main_callback) { + void runOnAllThreads1_(Event::PostCb cb) { cb(); } + void runOnAllThreads2_(Event::PostCb cb, Event::PostCb main_callback) { cb(); main_callback(); } diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 2d8beddd793d9..52ac66b6b980d 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -1269,13 +1269,13 @@ class PrometheusStatsFormatterTest : public testing::Test { PrometheusStatsFormatterTest() : alloc_(symbol_table_) {} void addCounter(const std::string& name, std::vector cluster_tags) { - std::string tname = std::string(name); - counters_.push_back(alloc_.makeCounter(name, std::move(tname), std::move(cluster_tags))); + Stats::StatNameManagedStorage storage(name, symbol_table_); + counters_.push_back(alloc_.makeCounter(storage.statName(), name, cluster_tags)); } void addGauge(const std::string& name, std::vector cluster_tags) { - std::string tname = std::string(name); - gauges_.push_back(alloc_.makeGauge(name, std::move(tname), std::move(cluster_tags))); + Stats::StatNameManagedStorage storage(name, symbol_table_); + gauges_.push_back(alloc_.makeGauge(storage.statName(), name, cluster_tags)); } void addHistogram(const Stats::ParentHistogramSharedPtr histogram) {