diff --git a/include/envoy/stats/allocator.h b/include/envoy/stats/allocator.h index f05f6df4f89a..5ab8dc2cdcfc 100644 --- a/include/envoy/stats/allocator.h +++ b/include/envoy/stats/allocator.h @@ -31,22 +31,22 @@ class Allocator { /** * @param name the full name of the stat. * @param tag_extracted_name the name of the stat with tag-values stripped out. - * @param tags the extracted tag values. + * @param tags the tag values. * @return CounterSharedPtr a counter, or nullptr if allocation failed, in which case * tag_extracted_name and tags are not moved. */ virtual CounterSharedPtr makeCounter(StatName name, absl::string_view tag_extracted_name, - const std::vector& tags) PURE; + const StatNameTagVector& stat_name_tags) PURE; /** * @param name the full name of the stat. * @param tag_extracted_name the name of the stat with tag-values stripped out. - * @param tags the extracted tag values. + * @param stat_name_tags the tag values. * @return GaugeSharedPtr a gauge, or nullptr if allocation failed, in which case * tag_extracted_name and tags are not moved. */ virtual GaugeSharedPtr makeGauge(StatName name, absl::string_view tag_extracted_name, - const std::vector& tags, + const StatNameTagVector& stat_name_tags, Gauge::ImportMode import_mode) PURE; virtual const SymbolTable& constSymbolTable() const PURE; diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index 6a3a4aaf5d5f..52c4226df258 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -7,6 +7,7 @@ #include "envoy/common/pure.h" #include "envoy/stats/histogram.h" #include "envoy/stats/symbol_table.h" +#include "envoy/stats/tag.h" #include "absl/types/optional.h" @@ -47,10 +48,22 @@ class Scope { virtual void deliverHistogramToSinks(const Histogram& histogram, uint64_t value) PURE; /** + * Creates a Counter from the stat name. Tag extraction will be performed on the name. * @param name The name of the stat, obtained from the SymbolTable. * @return a counter within the scope's namespace. */ - virtual Counter& counterFromStatName(StatName name) PURE; + Counter& counterFromStatName(const StatName& name) { + return counterFromStatNameWithTags(name, absl::nullopt); + } + /** + * Creates a Counter from the stat name and tags. If tags are not provided, tag extraction + * will be performed on the name. + * @param name The name of the stat, obtained from the SymbolTable. + * @param tags optionally specified tags. + * @return a counter within the scope's namespace. + */ + virtual Counter& counterFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptRef tags) PURE; /** * TODO(#6667): this variant is deprecated: use counterFromStatName. @@ -60,11 +73,25 @@ class Scope { virtual Counter& counter(const std::string& name) PURE; /** + * Creates a Gauge from the stat name. Tag extraction will be performed on the name. + * @param name The name of the stat, obtained from the SymbolTable. + * @param import_mode Whether hot-restart should accumulate this value. + * @return a gauge within the scope's namespace. + */ + Gauge& gaugeFromStatName(const StatName& name, Gauge::ImportMode import_mode) { + return gaugeFromStatNameWithTags(name, absl::nullopt, import_mode); + } + + /** + * Creates a Gauge from the stat name and tags. If tags are not provided, tag extraction + * will be performed on the name. * @param name The name of the stat, obtained from the SymbolTable. + * @param tags optionally specified tags. * @param import_mode Whether hot-restart should accumulate this value. * @return a gauge within the scope's namespace. */ - virtual Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) PURE; + virtual Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Gauge::ImportMode import_mode) PURE; /** * TODO(#6667): this variant is deprecated: use gaugeFromStatName. @@ -80,11 +107,26 @@ class Scope { virtual NullGaugeImpl& nullGauge(const std::string& name) PURE; /** + * Creates a Histogram from the stat name. Tag extraction will be performed on the name. + * @param name The name of the stat, obtained from the SymbolTable. + * @param unit The unit of measurement. + * @return a histogram within the scope's namespace with a particular value type. + */ + Histogram& histogramFromStatName(const StatName& name, Histogram::Unit unit) { + return histogramFromStatNameWithTags(name, absl::nullopt, unit); + } + + /** + * Creates a Histogram from the stat name and tags. If tags are not provided, tag extraction + * will be performed on the name. * @param name The name of the stat, obtained from the SymbolTable. + * @param tags optionally specified tags. * @param unit The unit of measurement. * @return a histogram within the scope's namespace with a particular value type. */ - virtual Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) PURE; + virtual Histogram& histogramFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptRef tags, + Histogram::Unit unit) PURE; /** * TODO(#6667): this variant is deprecated: use histogramFromStatName. diff --git a/include/envoy/stats/stats.h b/include/envoy/stats/stats.h index 1dfb8517d844..b1772fb63212 100644 --- a/include/envoy/stats/stats.h +++ b/include/envoy/stats/stats.h @@ -8,6 +8,7 @@ #include "envoy/common/pure.h" #include "envoy/stats/refcount_ptr.h" #include "envoy/stats/symbol_table.h" +#include "envoy/stats/tag.h" #include "absl/strings/string_view.h" @@ -15,7 +16,6 @@ namespace Envoy { namespace Stats { class Allocator; -struct Tag; /** * General interface for all stats objects. @@ -42,7 +42,7 @@ class Metric : public RefcountInterface { /** * Returns a vector of configurable tags to identify this Metric. */ - virtual std::vector tags() const PURE; + virtual TagVector tags() const PURE; /** * See a more detailed description in tagExtractedStatName(), which is the diff --git a/include/envoy/stats/tag.h b/include/envoy/stats/tag.h index bbc8111e5bc8..919aae81b408 100644 --- a/include/envoy/stats/tag.h +++ b/include/envoy/stats/tag.h @@ -2,6 +2,10 @@ #include +#include "envoy/stats/symbol_table.h" + +#include "absl/types/optional.h" + namespace Envoy { namespace Stats { @@ -11,7 +15,17 @@ namespace Stats { struct Tag { std::string name_; std::string value_; + + bool operator==(const Tag& other) const { + return other.name_ == name_ && other.value_ == value_; + }; }; +using TagVector = std::vector; + +using StatNameTag = std::pair; +using StatNameTagVector = std::vector; +using StatNameTagVectorOptRef = absl::optional>; + } // namespace Stats } // namespace Envoy diff --git a/include/envoy/stats/tag_extractor.h b/include/envoy/stats/tag_extractor.h index 1b6f10d32ea1..5983501d8f92 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(absl::string_view stat_name, std::vector& tags, + virtual bool extractTag(absl::string_view stat_name, TagVector& tags, IntervalSet& remove_characters) const PURE; /** diff --git a/include/envoy/stats/tag_producer.h b/include/envoy/stats/tag_producer.h index 0edfe110bfc6..91a6d1da1818 100644 --- a/include/envoy/stats/tag_producer.h +++ b/include/envoy/stats/tag_producer.h @@ -27,9 +27,9 @@ class TagProducer { * {"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. + * @param tags TagVector a set of Stats::Tag. */ - virtual std::string produceTags(absl::string_view metric_name, std::vector& tags) const PURE; + virtual std::string produceTags(absl::string_view metric_name, TagVector& tags) const PURE; }; using TagProducerPtr = std::unique_ptr; diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index fd24b770be80..c720e2ef92a1 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -52,6 +52,7 @@ envoy_cc_library( ":stats_lib", ":store_impl_lib", ":symbol_table_creator_lib", + ":tag_utility_lib", "//include/envoy/stats:stats_macros", "//source/common/stats:allocator_lib", ], @@ -68,6 +69,17 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "tag_utility_lib", + srcs = ["tag_utility.cc"], + hdrs = ["tag_utility.h"], + deps = [ + ":symbol_table_lib", + "//include/envoy/stats:stats_interface", + "//include/envoy/stats:symbol_table_interface", + ], +) + envoy_cc_library( name = "null_counter_lib", hdrs = ["null_counter.h"], @@ -232,6 +244,7 @@ envoy_cc_library( ":stats_lib", ":stats_matcher_lib", ":tag_producer_lib", + ":tag_utility_lib", "//include/envoy/thread_local:thread_local_interface", ], ) diff --git a/source/common/stats/allocator_impl.cc b/source/common/stats/allocator_impl.cc index 542fd57aef04..c83aee07ed5d 100644 --- a/source/common/stats/allocator_impl.cc +++ b/source/common/stats/allocator_impl.cc @@ -50,8 +50,9 @@ void AllocatorImpl::debugPrint() { template class StatsSharedImpl : public MetricImpl { public: StatsSharedImpl(StatName name, AllocatorImpl& alloc, absl::string_view tag_extracted_name, - const std::vector& tags) - : MetricImpl(name, tag_extracted_name, tags, alloc.symbolTable()), alloc_(alloc) {} + const StatNameTagVector& stat_name_tags) + : MetricImpl(name, tag_extracted_name, stat_name_tags, alloc.symbolTable()), + alloc_(alloc) {} ~StatsSharedImpl() override { // MetricImpl must be explicitly cleared() before destruction, otherwise it @@ -121,8 +122,8 @@ template class StatsSharedImpl : public MetricImpl class CounterImpl : public StatsSharedImpl { public: CounterImpl(StatName name, AllocatorImpl& alloc, absl::string_view tag_extracted_name, - const std::vector& tags) - : StatsSharedImpl(name, alloc, tag_extracted_name, tags) {} + const StatNameTagVector& stat_name_tags) + : StatsSharedImpl(name, alloc, tag_extracted_name, stat_name_tags) {} void removeFromSetLockHeld() EXCLUSIVE_LOCKS_REQUIRED(alloc_.mutex_) override { const size_t count = alloc_.counters_.erase(statName()); @@ -149,8 +150,8 @@ class CounterImpl : public StatsSharedImpl { class GaugeImpl : public StatsSharedImpl { public: GaugeImpl(StatName name, AllocatorImpl& alloc, absl::string_view tag_extracted_name, - const std::vector& tags, ImportMode import_mode) - : StatsSharedImpl(name, alloc, tag_extracted_name, tags) { + const StatNameTagVector& stat_name_tags, ImportMode import_mode) + : StatsSharedImpl(name, alloc, tag_extracted_name, stat_name_tags) { switch (import_mode) { case ImportMode::Accumulate: flags_ |= Flags::LogicAccumulate; @@ -228,20 +229,20 @@ class GaugeImpl : public StatsSharedImpl { }; CounterSharedPtr AllocatorImpl::makeCounter(StatName name, absl::string_view tag_extracted_name, - const std::vector& tags) { + const StatNameTagVector& stat_name_tags) { Thread::LockGuard lock(mutex_); ASSERT(gauges_.find(name) == gauges_.end()); auto iter = counters_.find(name); if (iter != counters_.end()) { return CounterSharedPtr(*iter); } - auto counter = CounterSharedPtr(new CounterImpl(name, *this, tag_extracted_name, tags)); + auto counter = CounterSharedPtr(new CounterImpl(name, *this, tag_extracted_name, stat_name_tags)); counters_.insert(counter.get()); return counter; } GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, absl::string_view tag_extracted_name, - const std::vector& tags, + const StatNameTagVector& stat_name_tags, Gauge::ImportMode import_mode) { Thread::LockGuard lock(mutex_); ASSERT(counters_.find(name) == counters_.end()); @@ -249,7 +250,8 @@ GaugeSharedPtr AllocatorImpl::makeGauge(StatName name, absl::string_view tag_ext if (iter != gauges_.end()) { return GaugeSharedPtr(*iter); } - auto gauge = GaugeSharedPtr(new GaugeImpl(name, *this, tag_extracted_name, tags, import_mode)); + auto gauge = + GaugeSharedPtr(new GaugeImpl(name, *this, tag_extracted_name, stat_name_tags, import_mode)); gauges_.insert(gauge.get()); return gauge; } diff --git a/source/common/stats/allocator_impl.h b/source/common/stats/allocator_impl.h index ca96724cae54..f6803156954c 100644 --- a/source/common/stats/allocator_impl.h +++ b/source/common/stats/allocator_impl.h @@ -24,9 +24,10 @@ class AllocatorImpl : public Allocator { // Allocator CounterSharedPtr makeCounter(StatName name, absl::string_view tag_extracted_name, - const std::vector& tags) override; + const StatNameTagVector& stat_name_tags) override; GaugeSharedPtr makeGauge(StatName name, absl::string_view tag_extracted_name, - const std::vector& tags, Gauge::ImportMode import_mode) override; + const StatNameTagVector& stat_name_tags, + Gauge::ImportMode import_mode) override; SymbolTable& symbolTable() override { return symbol_table_; } const SymbolTable& constSymbolTable() const override { return symbol_table_; } diff --git a/source/common/stats/histogram_impl.h b/source/common/stats/histogram_impl.h index b3d330c48383..c342d9ffa976 100644 --- a/source/common/stats/histogram_impl.h +++ b/source/common/stats/histogram_impl.h @@ -50,8 +50,8 @@ class HistogramStatisticsImpl : public HistogramStatistics, NonCopyable { class HistogramImplHelper : public MetricImpl { public: HistogramImplHelper(StatName name, const std::string& tag_extracted_name, - const std::vector& tags, SymbolTable& symbol_table) - : MetricImpl(name, tag_extracted_name, tags, symbol_table) {} + const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table) + : MetricImpl(name, tag_extracted_name, stat_name_tags, symbol_table) {} HistogramImplHelper(SymbolTable& symbol_table) : MetricImpl(symbol_table) {} // RefcountInterface @@ -69,9 +69,9 @@ class HistogramImplHelper : public MetricImpl { class HistogramImpl : public HistogramImplHelper { public: HistogramImpl(StatName name, Unit unit, Store& parent, const std::string& tag_extracted_name, - const std::vector& tags) - : HistogramImplHelper(name, tag_extracted_name, tags, parent.symbolTable()), unit_(unit), - parent_(parent) {} + const StatNameTagVector& stat_name_tags) + : HistogramImplHelper(name, tag_extracted_name, stat_name_tags, parent.symbolTable()), + unit_(unit), parent_(parent) {} ~HistogramImpl() override { // We must explicitly free the StatName here in order to supply the // SymbolTable reference. An RAII alternative would be to store a diff --git a/source/common/stats/isolated_store_impl.cc b/source/common/stats/isolated_store_impl.cc index 68cc712cf2f1..b2ad1e3537c6 100644 --- a/source/common/stats/isolated_store_impl.cc +++ b/source/common/stats/isolated_store_impl.cc @@ -24,14 +24,14 @@ IsolatedStoreImpl::IsolatedStoreImpl(std::unique_ptr&& symbol_table IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table) : StoreImpl(symbol_table), alloc_(symbol_table), counters_([this](StatName name) -> CounterSharedPtr { - return alloc_.makeCounter(name, toString(name), std::vector()); + return alloc_.makeCounter(name, toString(name), StatNameTagVector{}); }), gauges_([this](StatName name, Gauge::ImportMode import_mode) -> GaugeSharedPtr { - return alloc_.makeGauge(name, toString(name), std::vector(), import_mode); + return alloc_.makeGauge(name, toString(name), StatNameTagVector{}, import_mode); }), histograms_([this](StatName name, Histogram::Unit unit) -> HistogramSharedPtr { return HistogramSharedPtr( - new HistogramImpl(name, unit, *this, toString(name), std::vector())); + new HistogramImpl(name, unit, *this, toString(name), StatNameTagVector{})); }), null_counter_(new NullCounterImpl(symbol_table)), null_gauge_(new NullGaugeImpl(symbol_table)) {} diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index d6ee3bf6d495..792407a02c1e 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -13,6 +13,7 @@ #include "common/stats/null_gauge.h" #include "common/stats/store_impl.h" #include "common/stats/symbol_table_impl.h" +#include "common/stats/tag_utility.h" #include "common/stats/utility.h" #include "absl/container/flat_hash_map.h" @@ -100,18 +101,27 @@ class IsolatedStoreImpl : public StoreImpl { explicit IsolatedStoreImpl(SymbolTable& symbol_table); // Stats::Scope - Counter& counterFromStatName(StatName name) override { return counters_.get(name); } + Counter& counterFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptRef tags) override { + TagUtility::TagStatNameJoiner joiner(name, tags, symbolTable()); + Counter& counter = counters_.get(joiner.nameWithTags()); + return counter; + } ScopePtr createScope(const std::string& name) override; void deliverHistogramToSinks(const Histogram&, uint64_t) override {} - Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override { - Gauge& gauge = gauges_.get(name, import_mode); + Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Gauge::ImportMode import_mode) override { + TagUtility::TagStatNameJoiner joiner(name, tags, symbolTable()); + Gauge& gauge = gauges_.get(joiner.nameWithTags(), import_mode); gauge.mergeImportMode(import_mode); return gauge; } NullCounterImpl& nullCounter() { return *null_counter_; } NullGaugeImpl& nullGauge(const std::string&) override { return *null_gauge_; } - Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override { - Histogram& histogram = histograms_.get(name, unit); + Histogram& histogramFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Histogram::Unit unit) override { + TagUtility::TagStatNameJoiner joiner(name, tags, symbolTable()); + Histogram& histogram = histograms_.get(joiner.nameWithTags(), unit); return histogram; } CounterOptConstRef findCounter(StatName name) const override { return counters_.find(name); } diff --git a/source/common/stats/metric_impl.cc b/source/common/stats/metric_impl.cc index 5c0c6ae028d2..3cf096d4f895 100644 --- a/source/common/stats/metric_impl.cc +++ b/source/common/stats/metric_impl.cc @@ -15,20 +15,20 @@ MetricHelper::~MetricHelper() { } MetricHelper::MetricHelper(StatName name, absl::string_view tag_extracted_name, - const std::vector& tags, SymbolTable& symbol_table) { + const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table) { // Encode all the names and tags into transient storage so we can count the // required bytes. 2 is added to account for the name and 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 = 2 + 2 * tags.size(); + const uint32_t num_names = 2 + 2 * stat_name_tags.size(); absl::FixedArray names(num_names); names[0] = name; - StatNamePool pool(symbol_table); - names[1] = pool.add(tag_extracted_name); + StatNameManagedStorage storage(tag_extracted_name, symbol_table); + names[1] = storage.statName(); int index = 1; - for (auto& tag : tags) { - names[++index] = pool.add(tag.name_); - names[++index] = pool.add(tag.value_); + for (auto& stat_name_tag : stat_name_tags) { + names[++index] = stat_name_tag.first; + names[++index] = stat_name_tag.second; } symbol_table.populateList(names.begin(), num_names, stat_names_); } @@ -93,8 +93,8 @@ void MetricHelper::iterateTagStatNames(const Metric::TagStatNameIterFn& fn) cons ASSERT(state != TagValue); } -std::vector MetricHelper::tags(const SymbolTable& symbol_table) const { - std::vector tags; +TagVector MetricHelper::tags(const SymbolTable& symbol_table) const { + TagVector tags; iterateTagStatNames([&tags, &symbol_table](StatName name, StatName value) -> bool { tags.emplace_back(Tag{symbol_table.toString(name), symbol_table.toString(value)}); return true; diff --git a/source/common/stats/metric_impl.h b/source/common/stats/metric_impl.h index e176b17bda1e..250fccb9e1a2 100644 --- a/source/common/stats/metric_impl.h +++ b/source/common/stats/metric_impl.h @@ -21,13 +21,13 @@ namespace Stats { */ class MetricHelper { public: - MetricHelper(StatName name, absl::string_view tag_extracted_name, const std::vector& tags, - SymbolTable& symbol_table); + MetricHelper(StatName name, absl::string_view tag_extracted_name, + const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table); ~MetricHelper(); StatName statName() const; std::string name(const SymbolTable& symbol_table) const; - std::vector tags(const SymbolTable& symbol_table) const; + TagVector tags(const SymbolTable& symbol_table) const; StatName tagExtractedStatName() const; void iterateTagStatNames(const Metric::TagStatNameIterFn& fn) const; void clear(SymbolTable& symbol_table) { stat_names_.clear(symbol_table); } @@ -56,15 +56,15 @@ class MetricHelper { template class MetricImpl : public BaseClass { public: // TODO(jmarantz): Use StatName for tag_extracted_name. - MetricImpl(StatName name, absl::string_view tag_extracted_name, const std::vector& tags, - SymbolTable& symbol_table) - : helper_(name, tag_extracted_name, tags, symbol_table) {} + MetricImpl(StatName name, absl::string_view tag_extracted_name, + const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table) + : helper_(name, tag_extracted_name, stat_name_tags, symbol_table) {} // Empty construction of a MetricImpl; used for null stats. explicit MetricImpl(SymbolTable& symbol_table) - : MetricImpl(StatNameManagedStorage("", symbol_table).statName(), "", std::vector(), + : MetricImpl(StatNameManagedStorage("", symbol_table).statName(), "", StatNameTagVector{}, symbol_table) {} - std::vector tags() const override { return helper_.tags(constSymbolTable()); } + TagVector tags() const override { return helper_.tags(constSymbolTable()); } StatName statName() const override { return helper_.statName(); } StatName tagExtractedStatName() const override { return helper_.tagExtractedStatName(); } void iterateTagStatNames(const Metric::TagStatNameIterFn& fn) const override { diff --git a/source/common/stats/scope_prefixer.cc b/source/common/stats/scope_prefixer.cc index d4898af4f935..2365b5ecff7e 100644 --- a/source/common/stats/scope_prefixer.cc +++ b/source/common/stats/scope_prefixer.cc @@ -26,22 +26,26 @@ ScopePtr ScopePrefixer::createScope(const std::string& name) { return createScopeFromStatName(stat_name_storage.statName()); } -Counter& ScopePrefixer::counterFromStatName(StatName name) { +Counter& ScopePrefixer::counterFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptRef tags) { Stats::SymbolTable::StoragePtr stat_name_storage = scope_.symbolTable().join({prefix_.statName(), name}); - return scope_.counterFromStatName(StatName(stat_name_storage.get())); + return scope_.counterFromStatNameWithTags(StatName(stat_name_storage.get()), tags); } -Gauge& ScopePrefixer::gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) { +Gauge& ScopePrefixer::gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Gauge::ImportMode import_mode) { Stats::SymbolTable::StoragePtr stat_name_storage = scope_.symbolTable().join({prefix_.statName(), name}); - return scope_.gaugeFromStatName(StatName(stat_name_storage.get()), import_mode); + return scope_.gaugeFromStatNameWithTags(StatName(stat_name_storage.get()), tags, import_mode); } -Histogram& ScopePrefixer::histogramFromStatName(StatName name, Histogram::Unit unit) { +Histogram& ScopePrefixer::histogramFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptRef tags, + Histogram::Unit unit) { Stats::SymbolTable::StoragePtr stat_name_storage = scope_.symbolTable().join({prefix_.statName(), name}); - return scope_.histogramFromStatName(StatName(stat_name_storage.get()), unit); + return scope_.histogramFromStatNameWithTags(StatName(stat_name_storage.get()), tags, unit); } CounterOptConstRef ScopePrefixer::findCounter(StatName name) const { diff --git a/source/common/stats/scope_prefixer.h b/source/common/stats/scope_prefixer.h index 4b28367a2a89..4b6534b41019 100644 --- a/source/common/stats/scope_prefixer.h +++ b/source/common/stats/scope_prefixer.h @@ -17,22 +17,24 @@ class ScopePrefixer : public Scope { // Scope ScopePtr createScope(const std::string& name) override; - Counter& counterFromStatName(StatName name) override; - Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override; - Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override; + Counter& counterFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags) override; + Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Gauge::ImportMode import_mode) override; + Histogram& histogramFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Histogram::Unit unit) 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()); + return Scope::counterFromStatName(storage.statName()); } Gauge& gauge(const std::string& name, Gauge::ImportMode import_mode) override { StatNameManagedStorage storage(name, symbolTable()); - return gaugeFromStatName(storage.statName(), import_mode); + return Scope::gaugeFromStatName(storage.statName(), import_mode); } Histogram& histogram(const std::string& name, Histogram::Unit unit) override { StatNameManagedStorage storage(name, symbolTable()); - return histogramFromStatName(storage.statName(), unit); + return Scope::histogramFromStatName(storage.statName(), unit); } CounterOptConstRef findCounter(StatName name) const override; diff --git a/source/common/stats/stat_merger.cc b/source/common/stats/stat_merger.cc index 27b0007d8ef0..1f064785bb1c 100644 --- a/source/common/stats/stat_merger.cc +++ b/source/common/stats/stat_merger.cc @@ -132,6 +132,7 @@ void StatMerger::mergeGauges(const Protobuf::Map& gauges, } } + // TODO(snowp): Propagate tag values during hot restarts. auto& gauge_ref = temp_scope_->gaugeFromStatName(stat_name, import_mode); if (gauge_ref.importMode() == Gauge::ImportMode::NeverImport) { // On the first iteration through the loop, the gauge will not be loaded into the scope diff --git a/source/common/stats/tag_extractor_impl.cc b/source/common/stats/tag_extractor_impl.cc index a56398895c25..3c666a13e105 100644 --- a/source/common/stats/tag_extractor_impl.cc +++ b/source/common/stats/tag_extractor_impl.cc @@ -66,7 +66,7 @@ bool TagExtractorImpl::substrMismatch(absl::string_view stat_name) const { return !substr_.empty() && stat_name.find(substr_) == absl::string_view::npos; } -bool TagExtractorImpl::extractTag(absl::string_view stat_name, std::vector& tags, +bool TagExtractorImpl::extractTag(absl::string_view stat_name, TagVector& tags, IntervalSet& remove_characters) const { PERF_OPERATION(perf); diff --git a/source/common/stats/tag_extractor_impl.h b/source/common/stats/tag_extractor_impl.h index 138f25b6af7c..a63c7e1e4626 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(absl::string_view tag_extracted_name, std::vector& tags, + bool extractTag(absl::string_view tag_extracted_name, TagVector& tags, IntervalSet& remove_characters) const override; absl::string_view prefixToken() const override { return prefix_; } diff --git a/source/common/stats/tag_producer_impl.cc b/source/common/stats/tag_producer_impl.cc index 97c10ff22301..84b2ab158142 100644 --- a/source/common/stats/tag_producer_impl.cc +++ b/source/common/stats/tag_producer_impl.cc @@ -82,8 +82,8 @@ void TagProducerImpl::forEachExtractorMatching( } } -std::string TagProducerImpl::produceTags(absl::string_view metric_name, - std::vector& tags) const { +std::string TagProducerImpl::produceTags(absl::string_view metric_name, TagVector& tags) const { + // TODO(jmarantz): Skip the creation of string-based tags, creating a StatNameTagVector instead. tags.insert(tags.end(), default_tags_.begin(), default_tags_.end()); IntervalSetImpl remove_characters; forEachExtractorMatching( diff --git a/source/common/stats/tag_producer_impl.h b/source/common/stats/tag_producer_impl.h index 879357f8d8bc..e8b27307b2b8 100644 --- a/source/common/stats/tag_producer_impl.h +++ b/source/common/stats/tag_producer_impl.h @@ -38,7 +38,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(absl::string_view metric_name, std::vector& tags) const override; + std::string produceTags(absl::string_view metric_name, TagVector& tags) const override; private: friend class DefaultTagRegexTester; @@ -99,7 +99,7 @@ class TagProducerImpl : public TagProducer { // the storage for the prefix string is owned by the TagExtractor, which, depending on // implementation, may need make a copy of the prefix. absl::flat_hash_map> tag_extractor_prefix_map_; - std::vector default_tags_; + TagVector default_tags_; }; } // namespace Stats diff --git a/source/common/stats/tag_utility.cc b/source/common/stats/tag_utility.cc new file mode 100644 index 000000000000..b47d5b04c2bc --- /dev/null +++ b/source/common/stats/tag_utility.cc @@ -0,0 +1,52 @@ +#include "common/stats/tag_utility.h" + +#include "common/stats/symbol_table_impl.h" + +namespace Envoy { +namespace Stats { +namespace TagUtility { + +TagStatNameJoiner::TagStatNameJoiner(StatName prefix, StatName stat_name, + StatNameTagVectorOptRef stat_name_tags, + SymbolTable& symbol_table) { + prefix_storage_ = symbol_table.join({prefix, stat_name}); + tag_extracted_name_ = StatName(prefix_storage_.get()); + + if (stat_name_tags) { + full_name_storage_ = + joinNameAndTags(StatName(prefix_storage_.get()), *stat_name_tags, symbol_table); + name_with_tags_ = StatName(full_name_storage_.get()); + } else { + name_with_tags_ = StatName(prefix_storage_.get()); + } +} + +TagStatNameJoiner::TagStatNameJoiner(StatName stat_name, StatNameTagVectorOptRef stat_name_tags, + SymbolTable& symbol_table) { + tag_extracted_name_ = stat_name; + + if (stat_name_tags) { + full_name_storage_ = joinNameAndTags(stat_name, *stat_name_tags, symbol_table); + name_with_tags_ = StatName(full_name_storage_.get()); + } else { + name_with_tags_ = stat_name; + } +} + +SymbolTable::StoragePtr TagStatNameJoiner::joinNameAndTags(StatName name, + const StatNameTagVector& tags, + SymbolTable& symbol_table) { + std::vector stat_names; + stat_names.reserve(1 + 2 * tags.size()); + stat_names.emplace_back(name); + + for (const auto& tag : tags) { + stat_names.emplace_back(tag.first); + stat_names.emplace_back(tag.second); + } + + return symbol_table.join(stat_names); +} +} // namespace TagUtility +} // namespace Stats +} // namespace Envoy \ No newline at end of file diff --git a/source/common/stats/tag_utility.h b/source/common/stats/tag_utility.h new file mode 100644 index 000000000000..1b20adbed945 --- /dev/null +++ b/source/common/stats/tag_utility.h @@ -0,0 +1,59 @@ +#pragma once + +#include "envoy/stats/symbol_table.h" +#include "envoy/stats/tag.h" + +#include "common/stats/symbol_table_impl.h" + +namespace Envoy { +namespace Stats { +namespace TagUtility { + +/** + * Combines a stat name with an optional set of tag to create the final stat name to use. The + * resulting StatNames will be valid through the lifetime of this object and all provided stat + * names. + */ +class TagStatNameJoiner { +public: + /** + * Combines a prefix, stat name and tags into a single stat name. + * @param prefix StaName the stat prefix to use. + * @param name StaName the stat name to use. + * @param stat_name_tags optionally StatNameTagVector the stat name tags to add to the stat name. + */ + TagStatNameJoiner(StatName prefix, StatName stat_name, StatNameTagVectorOptRef stat_name_tags, + SymbolTable& symbol_table); + + /** + * Combines a stat name and tags into a single stat name. + * @param name StaName the stat name to use. + * @param stat_name_tags StatNameTagVector the stat name tags to optionally add to the stat name. + */ + TagStatNameJoiner(StatName stat_name, StatNameTagVectorOptRef stat_name_tags, + SymbolTable& symbol_table); + + /** + * @return StatName the full stat name, including the tag suffix. + */ + StatName nameWithTags() const { return name_with_tags_; } + + /** + * @return StatName the stat name without the tags appended. + */ + StatName tagExtractedName() const { return tag_extracted_name_; } + +private: + // TODO(snowp): This isn't really "tag extracted", but we'll use this for the sake of consistency + // until we can change the naming convention throughout. + StatName tag_extracted_name_; + SymbolTable::StoragePtr prefix_storage_; + SymbolTable::StoragePtr full_name_storage_; + StatName name_with_tags_; + + SymbolTable::StoragePtr joinNameAndTags(StatName name, const StatNameTagVector& stat_name_tags, + SymbolTable& symbol_table); +}; +} // namespace TagUtility +} // namespace Stats +} // namespace Envoy \ No newline at end of file diff --git a/source/common/stats/thread_local_store.cc b/source/common/stats/thread_local_store.cc index 2d33fb4ccff4..6f9bb037e823 100644 --- a/source/common/stats/thread_local_store.cc +++ b/source/common/stats/thread_local_store.cc @@ -14,6 +14,7 @@ #include "common/common/lock_guard.h" #include "common/stats/stats_matcher_impl.h" #include "common/stats/tag_producer_impl.h" +#include "common/stats/tag_utility.h" #include "absl/strings/str_join.h" @@ -263,20 +264,38 @@ ThreadLocalStoreImpl::ScopeImpl::~ScopeImpl() { prefix_.free(symbolTable()); } -// Manages tag-extraction of stat names. -class TagExtraction { +// Helper for managing the potential truncation of tags from the metric names and +// converting them to StatName. Making the tag extraction optional within this class simplifies the +// RAII ergonomics (as opposed to making the construction of this object conditional). +// +// The StatNameTagVector returned by this object will be valid as long as this object is in scope +// and the provided stat_name_tags are valid. +// +// When tag extraction is not done, this class is just a passthrough for the provided name/tags. +class StatNameTagHelper { 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_); - }); + StatNameTagHelper(ThreadLocalStoreImpl& tls, StatName name, + const absl::optional& stat_name_tags) + : pool_(tls.symbolTable()), stat_name_tags_(stat_name_tags.value_or(StatNameTagVector())) { + if (!stat_name_tags) { + TagVector tags; + tls.symbolTable().callWithStringView(name, [&tags, &tls, this](absl::string_view name_str) { + tag_extracted_name_ = tls.tagProducer().produceTags(name_str, tags); + }); + for (const auto& tag : tags) { + stat_name_tags_.emplace_back(pool_.add(tag.name_), pool_.add(tag.value_)); + } + } else { + tag_extracted_name_ = tls.symbolTable().toString(name); + } } - const std::vector& tags() { return tags_; } + const StatNameTagVector& statNameTags() const { return stat_name_tags_; } const std::string& tagExtractedName() { return tag_extracted_name_; } private: - std::vector tags_; + StatNamePool pool_; + StatNameTagVector stat_name_tags_; std::string tag_extracted_name_; }; @@ -309,18 +328,20 @@ bool ThreadLocalStoreImpl::checkAndRememberRejection(StatName name, template StatType& ThreadLocalStoreImpl::ScopeImpl::safeMakeStat( - StatName name, StatNameHashMap>& central_cache_map, + StatName full_stat_name, StatName name_no_tags, + const absl::optional& stat_name_tags, + StatNameHashMap>& central_cache_map, StatNameStorageSet& central_rejected_stats, MakeStatFn make_stat, StatRefMap* tls_cache, StatNameHashSet* tls_rejected_stats, StatType& null_stat) { if (tls_rejected_stats != nullptr && - tls_rejected_stats->find(name) != tls_rejected_stats->end()) { + tls_rejected_stats->find(full_stat_name) != tls_rejected_stats->end()) { return null_stat; } // If we have a valid cache entry, return it. if (tls_cache) { - auto pos = tls_cache->find(name); + auto pos = tls_cache->find(full_stat_name); if (pos != tls_cache->end()) { return pos->second; } @@ -329,16 +350,18 @@ 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(name); + auto iter = central_cache_map.find(full_stat_name); RefcountPtr* central_ref = nullptr; if (iter != central_cache_map.end()) { central_ref = &(iter->second); - } else if (parent_.checkAndRememberRejection(name, central_rejected_stats, tls_rejected_stats)) { + } else if (parent_.checkAndRememberRejection(full_stat_name, central_rejected_stats, + tls_rejected_stats)) { return null_stat; } else { - TagExtraction extraction(parent_, name); - RefcountPtr stat = - make_stat(parent_.alloc_, name, extraction.tagExtractedName(), extraction.tags()); + StatNameTagHelper tag_helper(parent_, name_no_tags, stat_name_tags); + + RefcountPtr stat = make_stat( + parent_.alloc_, full_stat_name, tag_helper.tagExtractedName(), tag_helper.statNameTags()); ASSERT(stat != nullptr); central_ref = ¢ral_cache_map[stat->statName()]; *central_ref = stat; @@ -366,7 +389,8 @@ ThreadLocalStoreImpl::ScopeImpl::findStatLockHeld( return std::cref(*iter->second); } -Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatName(StatName name) { +Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatNameWithTags( + const StatName& name, StatNameTagVectorOptRef stat_name_tags) { if (parent_.rejectsAll()) { return parent_.null_counter_; } @@ -380,8 +404,8 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatName(StatName 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. - Stats::SymbolTable::StoragePtr final_name = symbolTable().join({prefix_.statName(), name}); - StatName final_stat_name(final_name.get()); + TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); + Stats::StatName final_stat_name = joiner.nameWithTags(); // We now find the TLS cache. This might remain null if we don't have TLS // initialized currently. @@ -394,10 +418,11 @@ Counter& ThreadLocalStoreImpl::ScopeImpl::counterFromStatName(StatName name) { } return safeMakeStat( - final_stat_name, central_cache_->counters_, central_cache_->rejected_stats_, - [](Allocator& allocator, StatName name, absl::string_view tag_extracted_name, - const std::vector& tags) -> CounterSharedPtr { - return allocator.makeCounter(name, tag_extracted_name, tags); + final_stat_name, joiner.tagExtractedName(), stat_name_tags, central_cache_->counters_, + central_cache_->rejected_stats_, + [](Allocator& allocator, StatName name, absl::string_view final_name, + const StatNameTagVector& tags) -> CounterSharedPtr { + return allocator.makeCounter(name, final_name, tags); }, tls_cache, tls_rejected_stats, parent_.null_counter_); } @@ -418,8 +443,8 @@ void ThreadLocalStoreImpl::ScopeImpl::deliverHistogramToSinks(const Histogram& h } } -Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatName(StatName name, - Gauge::ImportMode import_mode) { +Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatNameWithTags( + const StatName& name, StatNameTagVectorOptRef stat_name_tags, Gauge::ImportMode import_mode) { if (parent_.rejectsAll()) { return parent_.null_gauge_; } @@ -432,8 +457,8 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatName(StatName 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. - Stats::SymbolTable::StoragePtr final_name = symbolTable().join({prefix_.statName(), name}); - StatName final_stat_name(final_name.get()); + TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); + StatName final_stat_name = joiner.nameWithTags(); StatRefMap* tls_cache = nullptr; StatNameHashSet* tls_rejected_stats = nullptr; @@ -444,9 +469,10 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatName(StatName name, } Gauge& gauge = safeMakeStat( - final_stat_name, central_cache_->gauges_, central_cache_->rejected_stats_, + final_stat_name, joiner.tagExtractedName(), stat_name_tags, central_cache_->gauges_, + central_cache_->rejected_stats_, [import_mode](Allocator& allocator, StatName name, absl::string_view tag_extracted_name, - const std::vector& tags) -> GaugeSharedPtr { + const StatNameTagVector& tags) -> GaugeSharedPtr { return allocator.makeGauge(name, tag_extracted_name, tags, import_mode); }, tls_cache, tls_rejected_stats, parent_.null_gauge_); @@ -454,8 +480,8 @@ Gauge& ThreadLocalStoreImpl::ScopeImpl::gaugeFromStatName(StatName name, return gauge; } -Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatName(StatName name, - Histogram::Unit unit) { +Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatNameWithTags( + const StatName& name, StatNameTagVectorOptRef stat_name_tags, Histogram::Unit unit) { if (parent_.rejectsAll()) { return parent_.null_histogram_; } @@ -468,8 +494,9 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatName(StatName 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. - Stats::SymbolTable::StoragePtr final_name = symbolTable().join({prefix_.statName(), name}); - StatName final_stat_name(final_name.get()); + + TagUtility::TagStatNameJoiner joiner(prefix_.statName(), name, stat_name_tags, symbolTable()); + StatName final_stat_name = joiner.nameWithTags(); StatNameHashMap* tls_cache = nullptr; StatNameHashSet* tls_rejected_stats = nullptr; @@ -495,10 +522,11 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::histogramFromStatName(StatName name, tls_rejected_stats)) { return parent_.null_histogram_; } else { - TagExtraction extraction(parent_, final_stat_name); + StatNameTagHelper tag_helper(parent_, joiner.tagExtractedName(), stat_name_tags); - RefcountPtr stat(new ParentHistogramImpl( - final_stat_name, unit, parent_, *this, extraction.tagExtractedName(), extraction.tags())); + RefcountPtr stat( + new ParentHistogramImpl(final_stat_name, unit, parent_, *this, + tag_helper.tagExtractedName(), tag_helper.statNameTags())); central_ref = ¢ral_cache_->histograms_[stat->statName()]; *central_ref = stat; } @@ -544,11 +572,11 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(StatName name, } } - std::vector tags; - std::string tag_extracted_name = - parent_.tagProducer().produceTags(symbolTable().toString(name), tags); + StatNameTagHelper tag_helper(parent_, name, absl::nullopt); + TlsHistogramSharedPtr hist_tls_ptr( - new ThreadLocalHistogramImpl(name, parent.unit(), tag_extracted_name, tags, symbolTable())); + new ThreadLocalHistogramImpl(name, parent.unit(), tag_helper.tagExtractedName(), + tag_helper.statNameTags(), symbolTable())); parent.addTlsHistogram(hist_tls_ptr); @@ -560,9 +588,9 @@ Histogram& ThreadLocalStoreImpl::ScopeImpl::tlsHistogram(StatName name, ThreadLocalHistogramImpl::ThreadLocalHistogramImpl(StatName name, Histogram::Unit unit, const std::string& tag_extracted_name, - const std::vector& tags, + const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table) - : HistogramImplHelper(name, tag_extracted_name, tags, symbol_table), unit_(unit), + : HistogramImplHelper(name, tag_extracted_name, stat_name_tags, symbol_table), unit_(unit), current_active_(0), used_(false), created_thread_id_(std::this_thread::get_id()), symbol_table_(symbol_table) { histograms_[0] = hist_alloc(); @@ -589,8 +617,8 @@ void ThreadLocalHistogramImpl::merge(histogram_t* target) { ParentHistogramImpl::ParentHistogramImpl(StatName name, Histogram::Unit unit, Store& parent, TlsScope& tls_scope, absl::string_view tag_extracted_name, - const std::vector& tags) - : MetricImpl(name, tag_extracted_name, tags, parent.symbolTable()), unit_(unit), + const StatNameTagVector& stat_name_tags) + : MetricImpl(name, tag_extracted_name, stat_name_tags, parent.symbolTable()), unit_(unit), parent_(parent), tls_scope_(tls_scope), interval_histogram_(hist_alloc()), cumulative_histogram_(hist_alloc()), interval_statistics_(interval_histogram_), cumulative_statistics_(cumulative_histogram_), merged_(false) {} diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 02e31bc8dc74..cfb36d447f7d 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -6,6 +6,7 @@ #include #include +#include "envoy/stats/tag.h" #include "envoy/thread_local/thread_local.h" #include "common/common/hash.h" @@ -32,8 +33,8 @@ namespace Stats { class ThreadLocalHistogramImpl : public HistogramImplHelper { public: ThreadLocalHistogramImpl(StatName name, Histogram::Unit unit, - const std::string& tag_extracted_name, const std::vector& tags, - SymbolTable& symbol_table); + const std::string& tag_extracted_name, + const StatNameTagVector& stat_name_tags, SymbolTable& symbol_table); ~ThreadLocalHistogramImpl() override; void merge(histogram_t* target); @@ -80,7 +81,8 @@ class TlsScope; class ParentHistogramImpl : public MetricImpl { public: ParentHistogramImpl(StatName name, Histogram::Unit unit, Store& parent, TlsScope& tls_scope, - absl::string_view tag_extracted_name, const std::vector& tags); + absl::string_view tag_extracted_name, + const StatNameTagVector& stat_name_tags); ~ParentHistogramImpl() override; void addTlsHistogram(const TlsHistogramSharedPtr& hist_ptr); @@ -142,6 +144,7 @@ class TlsScope : public Scope { /** * @return a ThreadLocalHistogram within the scope's namespace. * @param name name of the histogram with scope prefix attached. + * @param parent the parent histogram. */ virtual Histogram& tlsHistogram(StatName name, ParentHistogramImpl& parent) PURE; }; @@ -158,22 +161,25 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo ~ThreadLocalStoreImpl() override; // Stats::Scope - Counter& counterFromStatName(StatName name) override { - return default_scope_->counterFromStatName(name); + Counter& counterFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptRef tags) override { + return default_scope_->counterFromStatNameWithTags(name, tags); } Counter& counter(const std::string& name) override { return default_scope_->counter(name); } ScopePtr createScope(const std::string& name) override; void deliverHistogramToSinks(const Histogram& histogram, uint64_t value) override { return default_scope_->deliverHistogramToSinks(histogram, value); } - Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override { - return default_scope_->gaugeFromStatName(name, import_mode); + Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Gauge::ImportMode import_mode) override { + return default_scope_->gaugeFromStatNameWithTags(name, tags, import_mode); } Gauge& gauge(const std::string& name, Gauge::ImportMode import_mode) override { return default_scope_->gauge(name, import_mode); } - Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override { - return default_scope_->histogramFromStatName(name, unit); + Histogram& histogramFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Histogram::Unit unit) override { + return default_scope_->histogramFromStatNameWithTags(name, tags, unit); } Histogram& histogram(const std::string& name, Histogram::Unit unit) override { return default_scope_->histogram(name, unit); @@ -278,10 +284,13 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo ~ScopeImpl() override; // Stats::Scope - Counter& counterFromStatName(StatName name) override; + Counter& counterFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptRef tags) override; void deliverHistogramToSinks(const Histogram& histogram, uint64_t value) override; - Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override; - Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override; + Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Gauge::ImportMode import_mode) override; + Histogram& histogramFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Histogram::Unit unit) override; Histogram& tlsHistogram(StatName name, ParentHistogramImpl& parent) override; ScopePtr createScope(const std::string& name) override { return parent_.createScope(symbolTable().toString(prefix_.statName()) + "." + name); @@ -313,21 +322,25 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo template using MakeStatFn = std::function(Allocator&, StatName name, absl::string_view tag_extracted_name, - const std::vector& tags)>; + const StatNameTagVector& tags)>; /** * Makes a stat either by looking it up in the central cache, * generating it from the parent allocator, or as a last * result, creating it with the heap allocator. * - * @param name the full name of the stat (not tag extracted). + * @param full_stat_name the full name of the stat with appended tags. + * @param name_no_tags the full name of the stat (not tag extracted) without appended tags. + * @param stat_name_tags the tags provided at creation time. If empty, tag extraction occurs. * @param central_cache_map a map from name to the desired object in the central cache. * @param make_stat a function to generate the stat object, called if it's not in cache. * @param tls_ref possibly null reference to a cache entry for this stat, which will be * used if non-empty, or filled in if empty (and non-null). */ template - StatType& safeMakeStat(StatName name, StatNameHashMap>& central_cache_map, + StatType& safeMakeStat(StatName full_stat_name, StatName name_no_tags, + const absl::optional& stat_name_tags, + StatNameHashMap>& central_cache_map, StatNameStorageSet& central_rejected_stats, MakeStatFn make_stat, StatRefMap* tls_cache, StatNameHashSet* tls_rejected_stats, StatType& null_stat); @@ -366,7 +379,7 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo absl::flat_hash_map scope_cache_; }; - std::string getTagsForName(const std::string& name, std::vector& tags) const; + std::string getTagsForName(const std::string& name, TagVector& tags) const; void clearScopeFromCaches(uint64_t scope_id, CentralCacheEntrySharedPtr central_cache); void releaseScopeCrossThread(ScopeImpl* scope); void mergeInternal(PostMergeCb merge_cb); diff --git a/test/common/stats/allocator_impl_test.cc b/test/common/stats/allocator_impl_test.cc index 0d4955ea3d20..173f8d892e1a 100644 --- a/test/common/stats/allocator_impl_test.cc +++ b/test/common/stats/allocator_impl_test.cc @@ -39,9 +39,9 @@ class AllocatorImplTest : public testing::Test { // Allocate 2 counters of the same name, and you'll get the same object. TEST_F(AllocatorImplTest, CountersWithSameName) { StatName counter_name = makeStat("counter.name"); - CounterSharedPtr c1 = alloc_.makeCounter(counter_name, "", std::vector()); + CounterSharedPtr c1 = alloc_.makeCounter(counter_name, "", {}); EXPECT_EQ(1, c1->use_count()); - CounterSharedPtr c2 = alloc_.makeCounter(counter_name, "", std::vector()); + CounterSharedPtr c2 = alloc_.makeCounter(counter_name, "", {}); EXPECT_EQ(2, c1->use_count()); EXPECT_EQ(2, c2->use_count()); EXPECT_EQ(c1.get(), c2.get()); @@ -57,11 +57,9 @@ TEST_F(AllocatorImplTest, CountersWithSameName) { TEST_F(AllocatorImplTest, GaugesWithSameName) { StatName gauge_name = makeStat("gauges.name"); - GaugeSharedPtr g1 = - alloc_.makeGauge(gauge_name, "", std::vector(), Gauge::ImportMode::Accumulate); + GaugeSharedPtr g1 = alloc_.makeGauge(gauge_name, "", {}, Gauge::ImportMode::Accumulate); EXPECT_EQ(1, g1->use_count()); - GaugeSharedPtr g2 = - alloc_.makeGauge(gauge_name, "", std::vector(), Gauge::ImportMode::Accumulate); + GaugeSharedPtr g2 = alloc_.makeGauge(gauge_name, "", {}, Gauge::ImportMode::Accumulate); EXPECT_EQ(2, g1->use_count()); EXPECT_EQ(2, g2->use_count()); EXPECT_EQ(g1.get(), g2.get()); @@ -94,8 +92,8 @@ TEST_F(AllocatorImplTest, RefCountDecAllocRaceOrganic) { threads.push_back(thread_factory.createThread([&]() { go.WaitForNotification(); for (uint32_t i = 0; i < iters; ++i) { - alloc_.makeCounter(counter_name, "", std::vector()); - alloc_.makeGauge(gauge_name, "", std::vector(), Gauge::ImportMode::NeverImport); + alloc_.makeCounter(counter_name, "", {}); + alloc_.makeGauge(gauge_name, "", {}, Gauge::ImportMode::NeverImport); } })); } @@ -118,7 +116,7 @@ TEST_F(AllocatorImplTest, RefCountDecAllocRaceSynchronized) { alloc_.sync().enable(); alloc_.sync().waitOn(AllocatorImpl::DecrementToZeroSyncPoint); Thread::ThreadPtr thread = thread_factory.createThread([&]() { - CounterSharedPtr counter = alloc_.makeCounter(counter_name, "", std::vector()); + CounterSharedPtr counter = alloc_.makeCounter(counter_name, "", {}); counter->inc(); counter->reset(); // Blocks in thread synchronizer waiting on DecrementToZeroSyncPoint }); diff --git a/test/common/stats/metric_impl_test.cc b/test/common/stats/metric_impl_test.cc index b178842fa865..910615aca3e6 100644 --- a/test/common/stats/metric_impl_test.cc +++ b/test/common/stats/metric_impl_test.cc @@ -38,9 +38,9 @@ TEST_F(MetricImplTest, NoTags) { } TEST_F(MetricImplTest, OneTag) { - CounterSharedPtr counter = - alloc_.makeCounter(makeStat("counter.name.value"), "counter", {{"name", "value"}}); - std::vector tags = counter->tags(); + CounterSharedPtr counter = alloc_.makeCounter(makeStat("counter.name.value"), "counter", + {{makeStat("name"), makeStat("value")}}); + TagVector tags = counter->tags(); ASSERT_EQ(1, tags.size()); EXPECT_EQ("name", tags[0].name_); EXPECT_EQ("value", tags[0].value_); @@ -50,8 +50,9 @@ TEST_F(MetricImplTest, OneTag) { } TEST_F(MetricImplTest, TwoTagsIterOnce) { - CounterSharedPtr counter = alloc_.makeCounter(makeStat("counter.name.value"), "counter", - {{"name1", "value1"}, {"name2", "value2"}}); + CounterSharedPtr counter = alloc_.makeCounter( + makeStat("counter.name.value"), "counter", + {{makeStat("name1"), makeStat("value1")}, {makeStat("name2"), makeStat("value2")}}); StatName name1 = makeStat("name1"); StatName value1 = makeStat("value1"); int count = 0; @@ -65,8 +66,9 @@ TEST_F(MetricImplTest, TwoTagsIterOnce) { } TEST_F(MetricImplTest, FindTag) { - CounterSharedPtr counter = alloc_.makeCounter(makeStat("counter.name.value"), "counter", - {{"name1", "value1"}, {"name2", "value2"}}); + CounterSharedPtr counter = alloc_.makeCounter( + makeStat("counter.name.value"), "counter", + {{makeStat("name1"), makeStat("value1")}, {makeStat("name2"), makeStat("value2")}}); EXPECT_EQ(makeStat("value1"), Utility::findTag(*counter, makeStat("name1"))); EXPECT_EQ(makeStat("value2"), Utility::findTag(*counter, makeStat("name2"))); EXPECT_FALSE(Utility::findTag(*counter, makeStat("name3"))); @@ -74,4 +76,4 @@ TEST_F(MetricImplTest, FindTag) { } // namespace } // namespace Stats -} // namespace Envoy +} // namespace Envoy \ No newline at end of file diff --git a/test/common/stats/stat_test_utility.cc b/test/common/stats/stat_test_utility.cc index 5de0c5c98fd6..34450d2566b9 100644 --- a/test/common/stats/stat_test_utility.cc +++ b/test/common/stats/stat_test_utility.cc @@ -143,11 +143,12 @@ Counter& TestStore::counter(const std::string& name) { return *counter_ref; } -Counter& TestStore::counterFromStatName(StatName stat_name) { +Counter& TestStore::counterFromStatNameWithTags(const StatName& stat_name, + StatNameTagVectorOptRef tags) { std::string name = symbolTable().toString(stat_name); Counter*& counter_ref = counter_map_[name]; if (counter_ref == nullptr) { - counter_ref = &IsolatedStoreImpl::counterFromStatName(stat_name); + counter_ref = &IsolatedStoreImpl::counterFromStatNameWithTags(stat_name, tags); } else { // Ensures StatNames with the same string representation are specified // consistently using symbolic/dynamic components on every access. @@ -164,11 +165,12 @@ Gauge& TestStore::gauge(const std::string& name, Gauge::ImportMode mode) { return *gauge_ref; } -Gauge& TestStore::gaugeFromStatName(StatName stat_name, Gauge::ImportMode mode) { +Gauge& TestStore::gaugeFromStatNameWithTags(const StatName& stat_name, StatNameTagVectorOptRef tags, + Gauge::ImportMode mode) { std::string name = symbolTable().toString(stat_name); Gauge*& gauge_ref = gauge_map_[name]; if (gauge_ref == nullptr) { - gauge_ref = &IsolatedStoreImpl::gaugeFromStatName(stat_name, mode); + gauge_ref = &IsolatedStoreImpl::gaugeFromStatNameWithTags(stat_name, tags, mode); } else { ASSERT(gauge_ref->statName() == stat_name); } @@ -183,11 +185,13 @@ Histogram& TestStore::histogram(const std::string& name, Histogram::Unit unit) { return *histogram_ref; } -Histogram& TestStore::histogramFromStatName(StatName stat_name, Histogram::Unit unit) { +Histogram& TestStore::histogramFromStatNameWithTags(const StatName& stat_name, + StatNameTagVectorOptRef tags, + Histogram::Unit unit) { std::string name = symbolTable().toString(stat_name); Histogram*& histogram_ref = histogram_map_[name]; if (histogram_ref == nullptr) { - histogram_ref = &IsolatedStoreImpl::histogramFromStatName(stat_name, unit); + histogram_ref = &IsolatedStoreImpl::histogramFromStatNameWithTags(stat_name, tags, unit); } else { ASSERT(histogram_ref->statName() == stat_name); } diff --git a/test/common/stats/stat_test_utility.h b/test/common/stats/stat_test_utility.h index 1eeb9b840848..a57958da7c62 100644 --- a/test/common/stats/stat_test_utility.h +++ b/test/common/stats/stat_test_utility.h @@ -104,9 +104,11 @@ class TestStore : public IsolatedStoreImpl { Counter& counter(const std::string& name) override; Gauge& gauge(const std::string& name, Gauge::ImportMode import_mode) override; Histogram& histogram(const std::string& name, Histogram::Unit unit) override; - Counter& counterFromStatName(StatName name) override; - Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override; - Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override; + Counter& counterFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags) override; + Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Gauge::ImportMode import_mode) override; + Histogram& histogramFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Histogram::Unit unit) override; // New APIs available for tests. CounterOptConstRef findCounterByString(const std::string& name) const; diff --git a/test/common/stats/tag_extractor_impl_test.cc b/test/common/stats/tag_extractor_impl_test.cc index d4c4d5eeb4d4..c80fbf7047df 100644 --- a/test/common/stats/tag_extractor_impl_test.cc +++ b/test/common/stats/tag_extractor_impl_test.cc @@ -18,7 +18,7 @@ TEST(TagExtractorTest, TwoSubexpressions) { TagExtractorImpl tag_extractor("cluster_name", "^cluster\\.((.+?)\\.)"); EXPECT_EQ("cluster_name", tag_extractor.name()); std::string name = "cluster.test_cluster.upstream_cx_total"; - std::vector tags; + TagVector tags; IntervalSetImpl remove_characters; ASSERT_TRUE(tag_extractor.extractTag(name, tags, remove_characters)); std::string tag_extracted_name = StringUtil::removeCharacters(name, remove_characters); @@ -31,7 +31,7 @@ TEST(TagExtractorTest, TwoSubexpressions) { TEST(TagExtractorTest, SingleSubexpression) { TagExtractorImpl tag_extractor("listner_port", "^listener\\.(\\d+?\\.)"); std::string name = "listener.80.downstream_cx_total"; - std::vector tags; + TagVector tags; IntervalSetImpl remove_characters; ASSERT_TRUE(tag_extractor.extractTag(name, tags, remove_characters)); std::string tag_extracted_name = StringUtil::removeCharacters(name, remove_characters); @@ -68,10 +68,10 @@ class DefaultTagRegexTester { DefaultTagRegexTester() : tag_extractors_(envoy::config::metrics::v3::StatsConfig()) {} void testRegex(const std::string& stat_name, const std::string& expected_tag_extracted_name, - const std::vector& expected_tags) { + const TagVector& expected_tags) { // Test forward iteration through the regexes - std::vector tags; + TagVector tags; const std::string tag_extracted_name = tag_extractors_.produceTags(stat_name, tags); auto cmp = [](const Tag& lhs, const Tag& rhs) { @@ -85,7 +85,7 @@ class DefaultTagRegexTester { << fmt::format("Stat name '{}' did not produce the expected tags", stat_name); // Reverse iteration through regexes to ensure ordering invariance - std::vector rev_tags; + TagVector rev_tags; const std::string rev_tag_extracted_name = produceTagsReverse(stat_name, rev_tags); EXPECT_EQ(expected_tag_extracted_name, rev_tag_extracted_name); @@ -106,10 +106,12 @@ class DefaultTagRegexTester { * assuming we don't care about tag-order. This is in large part correct by design because * stat_name is not mutated until all the extraction is done. * @param metric_name std::string a name of Stats::Metric (Counter, Gauge, Histogram). - * @param tags std::vector& a set of Stats::Tag. + * @param tags TagVector& a set of Stats::Tag. * @return std::string the metric_name with tags removed. */ - std::string produceTagsReverse(const std::string& metric_name, std::vector& tags) const { + std::string produceTagsReverse(const std::string& metric_name, TagVector& tags) const { + // TODO(jmarantz): Skip the creation of string-based tags, creating a StatNameTagVector instead. + // Note: one discrepancy between this and TagProducerImpl::produceTags is that this // version does not add in tag_extractors_.default_tags_ into tags. That doesn't matter // for this test, however. diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 87903d376fc3..a834411994a6 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -4,11 +4,13 @@ #include #include "envoy/config/metrics/v3/stats.pb.h" +#include "envoy/stats/histogram.h" #include "common/common/c_smart_ptr.h" #include "common/event/dispatcher_impl.h" #include "common/memory/stats.h" #include "common/stats/stats_matcher_impl.h" +#include "common/stats/symbol_table_impl.h" #include "common/stats/tag_producer_impl.h" #include "common/stats/thread_local_store.h" #include "common/thread_local/thread_local_impl.h" @@ -326,6 +328,36 @@ TEST_F(StatsThreadLocalStoreTest, BasicScope) { ASSERT_TRUE(found_histogram2.has_value()); EXPECT_EQ(&h2, &found_histogram2->get()); + StatNameManagedStorage tag_key("a", *symbol_table_); + StatNameManagedStorage tag_value("b", *symbol_table_); + StatNameTagVector tags{{StatName(tag_key.statName()), StatName(tag_value.statName())}}; + + const TagVector expectedTags = {Tag{"a", "b"}}; + + { + StatNameManagedStorage storage("c3", *symbol_table_); + Counter& counter = scope1->counterFromStatNameWithTags(StatName(storage.statName()), tags); + EXPECT_EQ(expectedTags, counter.tags()); + EXPECT_EQ(&counter, &scope1->counterFromStatNameWithTags(StatName(storage.statName()), tags)); + } + { + StatNameManagedStorage storage("g3", *symbol_table_); + Gauge& gauge = scope1->gaugeFromStatNameWithTags(StatName(storage.statName()), tags, + Gauge::ImportMode::Accumulate); + EXPECT_EQ(expectedTags, gauge.tags()); + EXPECT_EQ(&gauge, &scope1->gaugeFromStatNameWithTags(StatName(storage.statName()), tags, + Gauge::ImportMode::Accumulate)); + } + { + StatNameManagedStorage storage("h3", *symbol_table_); + Histogram& histogram = scope1->histogramFromStatNameWithTags( + StatName(storage.statName()), tags, Stats::Histogram::Unit::Unspecified); + EXPECT_EQ(expectedTags, histogram.tags()); + EXPECT_EQ(&histogram, + &scope1->histogramFromStatNameWithTags(StatName(storage.statName()), tags, + Stats::Histogram::Unit::Unspecified)); + } + store_->shutdownThreading(); scope1->deliverHistogramToSinks(h1, 100); scope1->deliverHistogramToSinks(h2, 200); @@ -478,7 +510,7 @@ class LookupWithStatNameTest : public ThreadLocalStoreNoMocksTestBase {}; TEST_F(LookupWithStatNameTest, All) { ScopePtr scope1 = store_->createScope("scope1."); - Counter& c1 = store_->counterFromStatName(makeStatName("c1")); + Counter& c1 = store_->Store::counterFromStatName(makeStatName("c1")); Counter& c2 = scope1->counterFromStatName(makeStatName("c2")); EXPECT_EQ("c1", c1.name()); EXPECT_EQ("scope1.c2", c2.name()); @@ -487,7 +519,7 @@ TEST_F(LookupWithStatNameTest, All) { EXPECT_EQ(0, c1.tags().size()); EXPECT_EQ(0, c1.tags().size()); - Gauge& g1 = store_->gaugeFromStatName(makeStatName("g1"), Gauge::ImportMode::Accumulate); + Gauge& g1 = store_->Store::gaugeFromStatName(makeStatName("g1"), Gauge::ImportMode::Accumulate); Gauge& g2 = scope1->gaugeFromStatName(makeStatName("g2"), Gauge::ImportMode::Accumulate); EXPECT_EQ("g1", g1.name()); EXPECT_EQ("scope1.g2", g2.name()); @@ -497,7 +529,7 @@ TEST_F(LookupWithStatNameTest, All) { EXPECT_EQ(0, g1.tags().size()); Histogram& h1 = - store_->histogramFromStatName(makeStatName("h1"), Stats::Histogram::Unit::Unspecified); + store_->Store::histogramFromStatName(makeStatName("h1"), Stats::Histogram::Unit::Unspecified); Histogram& h2 = scope1->histogramFromStatName(makeStatName("h2"), Stats::Histogram::Unit::Unspecified); scope1->deliverHistogramToSinks(h2, 0); @@ -1368,7 +1400,7 @@ TEST_F(ClusterShutdownCleanupStarvationTest, TwelveThreadsWithBlockade) { main_dispatch_block(); // Here we show that the counter cleanups have finished, because the use-count is 1. - CounterSharedPtr counter = alloc_.makeCounter(my_counter_scoped_name_, "", std::vector()); + CounterSharedPtr counter = alloc_.makeCounter(my_counter_scoped_name_, "", StatNameTagVector{}); EXPECT_EQ(1, counter->use_count()) << "index=" << i; } } @@ -1392,7 +1424,7 @@ TEST_F(ClusterShutdownCleanupStarvationTest, TwelveThreadsWithoutBlockade) { // running the test: NumScopes*NumThreads*NumIters == 70000, We use a timer // so we don't time out on asan/tsan tests, In opt builds this test takes // less than a second, and in fastbuild it takes less than 5. - CounterSharedPtr counter = alloc_.makeCounter(my_counter_scoped_name_, "", std::vector()); + CounterSharedPtr counter = alloc_.makeCounter(my_counter_scoped_name_, "", StatNameTagVector{}); uint32_t use_count = counter->use_count() - 1; // Subtract off this instance. EXPECT_EQ((i + 1) * NumScopes * NumThreads, use_count); } diff --git a/test/integration/server.h b/test/integration/server.h index 04591bc45a99..7f256a9485b2 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -81,19 +81,22 @@ class TestScopeWrapper : public Scope { wrapped_scope_->deliverHistogramToSinks(histogram, value); } - Counter& counterFromStatName(StatName name) override { + Counter& counterFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptRef tags) override { Thread::LockGuard lock(lock_); - return wrapped_scope_->counterFromStatName(name); + return wrapped_scope_->counterFromStatNameWithTags(name, tags); } - Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override { + Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Gauge::ImportMode import_mode) override { Thread::LockGuard lock(lock_); - return wrapped_scope_->gaugeFromStatName(name, import_mode); + return wrapped_scope_->gaugeFromStatNameWithTags(name, tags, import_mode); } - Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override { + Histogram& histogramFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Histogram::Unit unit) override { Thread::LockGuard lock(lock_); - return wrapped_scope_->histogramFromStatName(name, unit); + return wrapped_scope_->histogramFromStatNameWithTags(name, tags, unit); } NullGaugeImpl& nullGauge(const std::string& str) override { return wrapped_scope_->nullGauge(str); @@ -142,9 +145,10 @@ class TestScopeWrapper : public Scope { class TestIsolatedStoreImpl : public StoreRoot { public: // Stats::Scope - Counter& counterFromStatName(StatName name) override { + Counter& counterFromStatNameWithTags(const StatName& name, + StatNameTagVectorOptRef tags) override { Thread::LockGuard lock(lock_); - return store_.counterFromStatName(name); + return store_.counterFromStatNameWithTags(name, tags); } Counter& counter(const std::string& name) override { Thread::LockGuard lock(lock_); @@ -155,17 +159,19 @@ class TestIsolatedStoreImpl : public StoreRoot { return ScopePtr{new TestScopeWrapper(lock_, store_.createScope(name))}; } void deliverHistogramToSinks(const Histogram&, uint64_t) override {} - Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override { + Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Gauge::ImportMode import_mode) override { Thread::LockGuard lock(lock_); - return store_.gaugeFromStatName(name, import_mode); + return store_.gaugeFromStatNameWithTags(name, tags, import_mode); } Gauge& gauge(const std::string& name, Gauge::ImportMode import_mode) override { Thread::LockGuard lock(lock_); return store_.gauge(name, import_mode); } - Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override { + Histogram& histogramFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef tags, + Histogram::Unit unit) override { Thread::LockGuard lock(lock_); - return store_.histogramFromStatName(name, unit); + return store_.histogramFromStatNameWithTags(name, tags, unit); } NullGaugeImpl& nullGauge(const std::string& name) override { return store_.nullGauge(name); } Histogram& histogram(const std::string& name, Histogram::Unit unit) override { diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index 9b3b7f2db855..190a61ed8d79 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -86,7 +86,7 @@ template class MockMetric : public BaseClass { // creates a deadlock in gmock and is an unintended use of mock functions. std::string name() const override { return name_.name(); } StatName statName() const override { return name_.statName(); } - std::vector tags() const override { return tags_; } + TagVector tags() const override { return tags_; } void setTagExtractedName(absl::string_view name) { tag_extracted_name_ = std::string(name); tag_extracted_stat_name_ = @@ -108,7 +108,7 @@ template class MockMetric : public BaseClass { TestSymbolTable symbol_table_; // Must outlive name_. MetricName name_; - void setTags(const std::vector& tags) { + void setTags(const TagVector& tags) { tag_pool_.clear(); tags_ = tags; for (const Tag& tag : tags) { @@ -123,7 +123,7 @@ template class MockMetric : public BaseClass { } private: - std::vector tags_; + TagVector tags_; std::vector tag_names_and_values_; std::string tag_extracted_name_; StatNamePool tag_pool_; @@ -294,13 +294,17 @@ class MockStore : public SymbolTableProvider, public StoreImpl { MOCK_METHOD(GaugeOptConstRef, findGauge, (StatName), (const)); MOCK_METHOD(HistogramOptConstRef, findHistogram, (StatName), (const)); - Counter& counterFromStatName(StatName name) override { + Counter& counterFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef) override { + // We always just respond with the mocked counter, so the tags don't matter. return counter(symbol_table_->toString(name)); } - Gauge& gaugeFromStatName(StatName name, Gauge::ImportMode import_mode) override { + Gauge& gaugeFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef, + Gauge::ImportMode import_mode) override { + // We always just respond with the mocked gauge, so the tags don't matter. return gauge(symbol_table_->toString(name), import_mode); } - Histogram& histogramFromStatName(StatName name, Histogram::Unit unit) override { + Histogram& histogramFromStatNameWithTags(const StatName& name, StatNameTagVectorOptRef, + Histogram::Unit unit) override { return histogram(symbol_table_->toString(name), unit); } diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 9000f5900ed6..2835fd8ae102 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -1625,14 +1625,17 @@ class HistogramWrapper { class PrometheusStatsFormatterTest : public testing::Test { protected: PrometheusStatsFormatterTest() - : symbol_table_(Stats::SymbolTableCreator::makeSymbolTable()), alloc_(*symbol_table_) {} + : symbol_table_(Stats::SymbolTableCreator::makeSymbolTable()), alloc_(*symbol_table_), + pool_(*symbol_table_) {} - void addCounter(const std::string& name, std::vector cluster_tags) { + ~PrometheusStatsFormatterTest() override { clearStorage(); } + + void addCounter(const std::string& name, Stats::StatNameTagVector 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) { + void addGauge(const std::string& name, Stats::StatNameTagVector cluster_tags) { Stats::StatNameManagedStorage storage(name, *symbol_table_); gauges_.push_back(alloc_.makeGauge(storage.statName(), name, cluster_tags, Stats::Gauge::ImportMode::Accumulate)); @@ -1647,8 +1650,16 @@ class PrometheusStatsFormatterTest : public testing::Test { return MockHistogramSharedPtr(new NiceMock()); } + Stats::StatName makeStat(absl::string_view name) { return pool_.add(name); } + + void clearStorage() { + pool_.clear(); + EXPECT_EQ(0, symbol_table_->numSymbols()); + } + Stats::SymbolTablePtr symbol_table_; Stats::AllocatorImpl alloc_; + Stats::StatNamePool pool_; std::vector counters_; std::vector gauges_; std::vector histograms_; @@ -1692,13 +1703,14 @@ TEST_F(PrometheusStatsFormatterTest, MetricNameCollison) { // but having different tag names and values. //`statsAsPrometheus()` should return two implying it found two unique stat names - addCounter("cluster.test_cluster_1.upstream_cx_total", {{"a.tag-name", "a.tag-value"}}); addCounter("cluster.test_cluster_1.upstream_cx_total", - {{"another_tag_name", "another_tag-value"}}); + {{makeStat("a.tag-name"), makeStat("a.tag-value")}}); + addCounter("cluster.test_cluster_1.upstream_cx_total", + {{makeStat("another_tag_name"), makeStat("another_tag-value")}}); addGauge("cluster.test_cluster_2.upstream_cx_total", - {{"another_tag_name_3", "another_tag_3-value"}}); + {{makeStat("another_tag_name_3"), makeStat("another_tag_3-value")}}); addGauge("cluster.test_cluster_2.upstream_cx_total", - {{"another_tag_name_4", "another_tag_4-value"}}); + {{makeStat("another_tag_name_4"), makeStat("another_tag_4-value")}}); Buffer::OwnedImpl response; auto size = PrometheusStatsFormatter::statsAsPrometheus(counters_, gauges_, histograms_, response, @@ -1712,13 +1724,14 @@ TEST_F(PrometheusStatsFormatterTest, UniqueMetricName) { // statsAsPrometheus() should return four implying it found // four unique stat names. - addCounter("cluster.test_cluster_1.upstream_cx_total", {{"a.tag-name", "a.tag-value"}}); + addCounter("cluster.test_cluster_1.upstream_cx_total", + {{makeStat("a.tag-name"), makeStat("a.tag-value")}}); addCounter("cluster.test_cluster_2.upstream_cx_total", - {{"another_tag_name", "another_tag-value"}}); + {{makeStat("another_tag_name"), makeStat("another_tag-value")}}); addGauge("cluster.test_cluster_3.upstream_cx_total", - {{"another_tag_name_3", "another_tag_3-value"}}); + {{makeStat("another_tag_name_3"), makeStat("another_tag_3-value")}}); addGauge("cluster.test_cluster_4.upstream_cx_total", - {{"another_tag_name_4", "another_tag_4-value"}}); + {{makeStat("another_tag_name_4"), makeStat("another_tag_4-value")}}); Buffer::OwnedImpl response; auto size = PrometheusStatsFormatter::statsAsPrometheus(counters_, gauges_, histograms_, response, @@ -1826,10 +1839,14 @@ envoy_histogram1_count{} 101100000 } TEST_F(PrometheusStatsFormatterTest, OutputWithAllMetricTypes) { - addCounter("cluster.test_1.upstream_cx_total", {{"a.tag-name", "a.tag-value"}}); - addCounter("cluster.test_2.upstream_cx_total", {{"another_tag_name", "another_tag-value"}}); - addGauge("cluster.test_3.upstream_cx_total", {{"another_tag_name_3", "another_tag_3-value"}}); - addGauge("cluster.test_4.upstream_cx_total", {{"another_tag_name_4", "another_tag_4-value"}}); + addCounter("cluster.test_1.upstream_cx_total", + {{makeStat("a.tag-name"), makeStat("a.tag-value")}}); + addCounter("cluster.test_2.upstream_cx_total", + {{makeStat("another_tag_name"), makeStat("another_tag-value")}}); + addGauge("cluster.test_3.upstream_cx_total", + {{makeStat("another_tag_name_3"), makeStat("another_tag_3-value")}}); + addGauge("cluster.test_4.upstream_cx_total", + {{makeStat("another_tag_name_4"), makeStat("another_tag_4-value")}}); const std::vector h1_values = {50, 20, 30, 70, 100, 5000, 200}; HistogramWrapper h1_cumulative; @@ -1887,10 +1904,14 @@ envoy_cluster_test_1_upstream_rq_time_count{key1="value1",key2="value2"} 7 } TEST_F(PrometheusStatsFormatterTest, OutputWithUsedOnly) { - addCounter("cluster.test_1.upstream_cx_total", {{"a.tag-name", "a.tag-value"}}); - addCounter("cluster.test_2.upstream_cx_total", {{"another_tag_name", "another_tag-value"}}); - addGauge("cluster.test_3.upstream_cx_total", {{"another_tag_name_3", "another_tag_3-value"}}); - addGauge("cluster.test_4.upstream_cx_total", {{"another_tag_name_4", "another_tag_4-value"}}); + addCounter("cluster.test_1.upstream_cx_total", + {{makeStat("a.tag-name"), makeStat("a.tag-value")}}); + addCounter("cluster.test_2.upstream_cx_total", + {{makeStat("another_tag_name"), makeStat("another_tag-value")}}); + addGauge("cluster.test_3.upstream_cx_total", + {{makeStat("another_tag_name_3"), makeStat("another_tag_3-value")}}); + addGauge("cluster.test_4.upstream_cx_total", + {{makeStat("another_tag_name_4"), makeStat("another_tag_4-value")}}); const std::vector h1_values = {50, 20, 30, 70, 100, 5000, 200}; HistogramWrapper h1_cumulative; @@ -1975,10 +1996,14 @@ TEST_F(PrometheusStatsFormatterTest, OutputWithUsedOnlyHistogram) { } TEST_F(PrometheusStatsFormatterTest, OutputWithRegexp) { - addCounter("cluster.test_1.upstream_cx_total", {{"a.tag-name", "a.tag-value"}}); - addCounter("cluster.test_2.upstream_cx_total", {{"another_tag_name", "another_tag-value"}}); - addGauge("cluster.test_3.upstream_cx_total", {{"another_tag_name_3", "another_tag_3-value"}}); - addGauge("cluster.test_4.upstream_cx_total", {{"another_tag_name_4", "another_tag_4-value"}}); + addCounter("cluster.test_1.upstream_cx_total", + {{makeStat("a.tag-name"), makeStat("a.tag-value")}}); + addCounter("cluster.test_2.upstream_cx_total", + {{makeStat("another_tag_name"), makeStat("another_tag-value")}}); + addGauge("cluster.test_3.upstream_cx_total", + {{makeStat("another_tag_name_3"), makeStat("another_tag_3-value")}}); + addGauge("cluster.test_4.upstream_cx_total", + {{makeStat("another_tag_name_4"), makeStat("another_tag_4-value")}}); const std::vector h1_values = {50, 20, 30, 70, 100, 5000, 200}; HistogramWrapper h1_cumulative;