diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index 53d27f3ae5c96..eb3dbf0fa896d 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -43,11 +43,27 @@ class Scope { virtual void deliverHistogramToSinks(const Histogram& histogram, uint64_t value) PURE; /** + * @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; + + /** + * TODO(jmarantz): this variant is deprecated: use counterFromStatName. + * @param name The name, expressed as a string. * @return a counter within the scope's namespace. */ virtual Counter& counter(const std::string& name) PURE; /** + * @param name The name of the stat, obtained from the SymbolTable. + * @return a gauge within the scope's namespace. + */ + virtual Gauge& gaugeFromStatName(StatName name) PURE; + + /** + * TODO(jmarantz): this variant is deprecated: use gaugeFromStatName. + * @param name The name, expressed as a string. * @return a gauge within the scope's namespace. */ virtual Gauge& gauge(const std::string& name) PURE; @@ -58,6 +74,14 @@ class Scope { virtual NullGaugeImpl& nullGauge(const std::string& name) PURE; /** + * @param name The name of the stat, obtained from the SymbolTable. + * @return a histogram within the scope's namespace with a particular value type. + */ + virtual Histogram& histogramFromStatName(StatName name) PURE; + + /** + * TODO(jmarantz): 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. */ virtual Histogram& histogram(const std::string& name) PURE; diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index 7efb91a9f9442..40d5a43d11cb4 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -45,8 +45,10 @@ envoy_cc_library( deps = [ ":fake_symbol_table_lib", ":histogram_lib", + ":scope_prefixer_lib", ":stats_lib", ":stats_options_lib", + ":store_impl_lib", "//include/envoy/stats:stats_macros", "//source/common/stats:heap_stat_data_lib", ], @@ -62,6 +64,15 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "store_impl_lib", + hdrs = ["store_impl.h"], + deps = [ + ":symbol_table_lib", + "//include/envoy/stats:stats_interface", + ], +) + envoy_cc_library( name = "raw_stat_data_lib", srcs = ["raw_stat_data.cc"], @@ -76,6 +87,17 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "scope_prefixer_lib", + srcs = ["scope_prefixer.cc"], + hdrs = ["scope_prefixer.h"], + deps = [ + ":symbol_table_lib", + ":utility_lib", + "//include/envoy/stats:stats_interface", + ], +) + envoy_cc_library( name = "source_impl_lib", srcs = ["source_impl.cc"], @@ -195,6 +217,7 @@ envoy_cc_library( hdrs = ["thread_local_store.h"], deps = [ ":heap_stat_data_lib", + ":scope_prefixer_lib", ":stats_lib", ":stats_matcher_lib", ":tag_producer_lib", diff --git a/source/common/stats/isolated_store_impl.cc b/source/common/stats/isolated_store_impl.cc index afec3df91aaa4..e10dc340986f4 100644 --- a/source/common/stats/isolated_store_impl.cc +++ b/source/common/stats/isolated_store_impl.cc @@ -8,6 +8,7 @@ #include "common/common/utility.h" #include "common/stats/fake_symbol_table_impl.h" #include "common/stats/histogram_impl.h" +#include "common/stats/scope_prefixer.h" #include "common/stats/utility.h" namespace Envoy { @@ -22,7 +23,7 @@ IsolatedStoreImpl::IsolatedStoreImpl(std::unique_ptr&& symbol_table } IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table) - : symbol_table_(symbol_table), alloc_(symbol_table_), + : StoreImpl(symbol_table), alloc_(symbol_table), counters_([this](const std::string& name) -> CounterSharedPtr { std::string tag_extracted_name = name; std::vector tags; @@ -37,32 +38,8 @@ IsolatedStoreImpl::IsolatedStoreImpl(SymbolTable& symbol_table) return std::make_shared(name, *this, std::string(name), std::vector()); }) {} -struct IsolatedScopeImpl : public Scope { - IsolatedScopeImpl(IsolatedStoreImpl& parent, const std::string& prefix) - : parent_(parent), prefix_(Utility::sanitizeStatsName(prefix)) {} - - // Stats::Scope - ScopePtr createScope(const std::string& name) override { - return ScopePtr{new IsolatedScopeImpl(parent_, prefix_ + name)}; - } - void deliverHistogramToSinks(const Histogram&, uint64_t) override {} - Counter& counter(const std::string& name) override { return parent_.counter(prefix_ + name); } - Gauge& gauge(const std::string& name) override { return parent_.gauge(prefix_ + name); } - NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; } - Histogram& histogram(const std::string& name) override { - return parent_.histogram(prefix_ + name); - } - const Stats::StatsOptions& statsOptions() const override { return parent_.statsOptions(); } - const SymbolTable& symbolTable() const override { return parent_.symbolTable(); } - SymbolTable& symbolTable() override { return parent_.symbolTable(); } - - IsolatedStoreImpl& parent_; - NullGaugeImpl null_gauge_; - const std::string prefix_; -}; - ScopePtr IsolatedStoreImpl::createScope(const std::string& name) { - return ScopePtr{new IsolatedScopeImpl(*this, name)}; + return std::make_unique(name, *this); } } // namespace Stats diff --git a/source/common/stats/isolated_store_impl.h b/source/common/stats/isolated_store_impl.h index 1537ffb9fdf10..0cf8207a23e83 100644 --- a/source/common/stats/isolated_store_impl.h +++ b/source/common/stats/isolated_store_impl.h @@ -12,6 +12,7 @@ #include "common/common/utility.h" #include "common/stats/heap_stat_data.h" #include "common/stats/stats_options_impl.h" +#include "common/stats/store_impl.h" #include "common/stats/symbol_table_impl.h" #include "common/stats/utility.h" @@ -55,7 +56,7 @@ template class IsolatedStatsCache { Allocator alloc_; }; -class IsolatedStoreImpl : public Store { +class IsolatedStoreImpl : public StoreImpl { public: IsolatedStoreImpl(); explicit IsolatedStoreImpl(SymbolTable& symbol_table); @@ -66,13 +67,8 @@ class IsolatedStoreImpl : public Store { void deliverHistogramToSinks(const Histogram&, uint64_t) override {} Gauge& gauge(const std::string& name) override { return gauges_.get(name); } NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; } - Histogram& histogram(const std::string& name) override { - Histogram& histogram = histograms_.get(name); - return histogram; - } + Histogram& histogram(const std::string& name) override { return histograms_.get(name); } const Stats::StatsOptions& statsOptions() const override { return stats_options_; } - const SymbolTable& symbolTable() const override { return symbol_table_; } - virtual SymbolTable& symbolTable() override { return symbol_table_; } // Stats::Store std::vector counters() const override { return counters_.toVector(); } @@ -85,7 +81,6 @@ class IsolatedStoreImpl : public Store { IsolatedStoreImpl(std::unique_ptr&& symbol_table); std::unique_ptr symbol_table_storage_; - SymbolTable& symbol_table_; HeapStatDataAllocator alloc_; IsolatedStatsCache counters_; IsolatedStatsCache gauges_; diff --git a/source/common/stats/scope_prefixer.cc b/source/common/stats/scope_prefixer.cc new file mode 100644 index 0000000000000..96e1e8be7b0af --- /dev/null +++ b/source/common/stats/scope_prefixer.cc @@ -0,0 +1,35 @@ +#include "common/stats/scope_prefixer.h" + +#include "envoy/stats/scope.h" + +#include "common/stats/symbol_table_impl.h" +#include "common/stats/utility.h" + +namespace Envoy { +namespace Stats { + +ScopePrefixer::ScopePrefixer(absl::string_view prefix, Scope& scope) + : prefix_(Utility::sanitizeStatsName(prefix)), scope_(scope) {} + +ScopePtr ScopePrefixer::createScope(const std::string& name) { + return std::make_unique(prefix_ + name, scope_); +} + +Counter& ScopePrefixer::counterFromStatName(StatName name) { + return counter(symbolTable().toString(name)); +} + +Gauge& ScopePrefixer::gaugeFromStatName(StatName name) { + return gauge(symbolTable().toString(name)); +} + +Histogram& ScopePrefixer::histogramFromStatName(StatName name) { + return histogram(symbolTable().toString(name)); +} + +void ScopePrefixer::deliverHistogramToSinks(const Histogram& histograms, uint64_t val) { + scope_.deliverHistogramToSinks(histograms, val); +} + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/scope_prefixer.h b/source/common/stats/scope_prefixer.h new file mode 100644 index 0000000000000..4871840f549e3 --- /dev/null +++ b/source/common/stats/scope_prefixer.h @@ -0,0 +1,38 @@ +#include "envoy/stats/scope.h" + +#include "common/stats/symbol_table_impl.h" + +namespace Envoy { +namespace Stats { + +// Implements a Scope that delegates to a passed-in scope, prefixing all names +// prior to creation. +class ScopePrefixer : public Scope { +public: + ScopePrefixer(absl::string_view prefix, Scope& scope); + + // 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; + + const Stats::StatsOptions& statsOptions() const override { return scope_.statsOptions(); } + const SymbolTable& symbolTable() const override { return scope_.symbolTable(); } + virtual SymbolTable& symbolTable() override { return scope_.symbolTable(); } + + NullGaugeImpl& nullGauge(const std::string& str) override { return scope_.nullGauge(str); } + +private: + std::string prefix_; + Scope& scope_; +}; + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/store_impl.h b/source/common/stats/store_impl.h new file mode 100644 index 0000000000000..94b2db6b06e1c --- /dev/null +++ b/source/common/stats/store_impl.h @@ -0,0 +1,36 @@ +#pragma once + +#include "envoy/stats/stats.h" +#include "envoy/stats/store.h" + +#include "common/stats/symbol_table_impl.h" + +namespace Envoy { +namespace Stats { + +/** + * Implements common parts of the Store API needed by multiple derivations of Store. + */ +class StoreImpl : public Store { +public: + explicit StoreImpl(SymbolTable& symbol_table) : symbol_table_(symbol_table) {} + + 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_; } + const SymbolTable& symbolTable() const override { return symbol_table_; } + +private: + SymbolTable& symbol_table_; +}; + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 9180517387d42..afae6779edc0b 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -12,6 +12,7 @@ #include "common/stats/heap_stat_data.h" #include "common/stats/histogram_impl.h" #include "common/stats/source_impl.h" +#include "common/stats/symbol_table_impl.h" #include "common/stats/utility.h" #include "absl/container/flat_hash_map.h" @@ -139,16 +140,23 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo ~ThreadLocalStoreImpl(); // Stats::Scope + Counter& counterFromStatName(StatName name) override { + return default_scope_->counterFromStatName(name); + } 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) override { + return default_scope_->gaugeFromStatName(name); + } Gauge& gauge(const std::string& name) override { return default_scope_->gauge(name); } + Histogram& histogramFromStatName(StatName name) override { + return default_scope_->histogramFromStatName(name); + } + Histogram& histogram(const std::string& name) override { return default_scope_->histogram(name); } NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; } - Histogram& histogram(const std::string& name) override { - return default_scope_->histogram(name); - }; const SymbolTable& symbolTable() const override { return alloc_.symbolTable(); } SymbolTable& symbolTable() override { return alloc_.symbolTable(); } @@ -242,6 +250,16 @@ class ThreadLocalStoreImpl : Logger::Loggable, public StoreRo 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)); } + + Histogram& histogramFromStatName(StatName name) override { + return histogram(symbolTable().toString(name)); + } + static std::atomic next_scope_id_; const uint64_t scope_id_; diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index afc4e8c4f4ca5..c5c980ee4e274 100644 --- a/source/common/stats/utility.cc +++ b/source/common/stats/utility.cc @@ -6,8 +6,8 @@ namespace Envoy { namespace Stats { -std::string Utility::sanitizeStatsName(const std::string& name) { - std::string stats_name = name; +std::string Utility::sanitizeStatsName(absl::string_view name) { + std::string stats_name = std::string(name); std::replace(stats_name.begin(), stats_name.end(), ':', '_'); std::replace(stats_name.begin(), stats_name.end(), '\0', '_'); return stats_name; diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 18081360640c9..58872d0ce77e0 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -2,6 +2,8 @@ #include +#include "absl/strings/string_view.h" + namespace Envoy { namespace Stats { @@ -12,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 std::string& name); + static std::string sanitizeStatsName(const absl::string_view name); }; } // namespace Stats diff --git a/test/common/stats/isolated_store_impl_test.cc b/test/common/stats/isolated_store_impl_test.cc index b2aaa80693fdd..50c4aea7d0d95 100644 --- a/test/common/stats/isolated_store_impl_test.cc +++ b/test/common/stats/isolated_store_impl_test.cc @@ -11,11 +11,34 @@ namespace Envoy { namespace Stats { -TEST(StatsIsolatedStoreImplTest, All) { - IsolatedStoreImpl store; +class StatsIsolatedStoreImplTest : public testing::Test { +protected: + ~StatsIsolatedStoreImplTest() override { clearStorage(); } + + void clearStorage() { + for (auto& stat_name_storage : stat_name_storage_) { + stat_name_storage.free(store_.symbolTable()); + } + stat_name_storage_.clear(); + EXPECT_EQ(0, store_.symbolTable().numSymbols()); + } + + StatName makeStatName(absl::string_view name) { + stat_name_storage_.emplace_back(makeStatStorage(name)); + return stat_name_storage_.back().statName(); + } + + StatNameStorage makeStatStorage(absl::string_view name) { + return StatNameStorage(name, store_.symbolTable()); + } + + IsolatedStoreImpl store_; + std::vector stat_name_storage_; +}; - ScopePtr scope1 = store.createScope("scope1."); - Counter& c1 = store.counter("c1"); +TEST_F(StatsIsolatedStoreImplTest, All) { + ScopePtr scope1 = store_.createScope("scope1."); + Counter& c1 = store_.counter("c1"); Counter& c2 = scope1->counter("c2"); EXPECT_EQ("c1", c1.name()); EXPECT_EQ("scope1.c2", c2.name()); @@ -24,7 +47,7 @@ TEST(StatsIsolatedStoreImplTest, All) { EXPECT_EQ(0, c1.tags().size()); EXPECT_EQ(0, c1.tags().size()); - Gauge& g1 = store.gauge("g1"); + Gauge& g1 = store_.gauge("g1"); Gauge& g2 = scope1->gauge("g2"); EXPECT_EQ("g1", g1.name()); EXPECT_EQ("scope1.g2", g2.name()); @@ -33,7 +56,7 @@ TEST(StatsIsolatedStoreImplTest, All) { EXPECT_EQ(0, g1.tags().size()); EXPECT_EQ(0, g1.tags().size()); - Histogram& h1 = store.histogram("h1"); + Histogram& h1 = store_.histogram("h1"); Histogram& h2 = scope1->histogram("h2"); scope1->deliverHistogramToSinks(h2, 0); EXPECT_EQ("h1", h1.name()); @@ -52,16 +75,58 @@ TEST(StatsIsolatedStoreImplTest, All) { ScopePtr scope3 = scope1->createScope(std::string("foo:\0:.", 7)); EXPECT_EQ("scope1.foo___.bar", scope3->counter("bar").name()); - EXPECT_EQ(4UL, store.counters().size()); - EXPECT_EQ(2UL, store.gauges().size()); + EXPECT_EQ(4UL, store_.counters().size()); + EXPECT_EQ(2UL, store_.gauges().size()); +} + +TEST_F(StatsIsolatedStoreImplTest, AllWithSymbolTable) { + ScopePtr scope1 = store_.createScope("scope1."); + Counter& c1 = store_.counterFromStatName(makeStatName("c1")); + Counter& c2 = scope1->counterFromStatName(makeStatName("c2")); + EXPECT_EQ("c1", c1.name()); + EXPECT_EQ("scope1.c2", c2.name()); + EXPECT_EQ("c1", c1.tagExtractedName()); + EXPECT_EQ("scope1.c2", c2.tagExtractedName()); + EXPECT_EQ(0, c1.tags().size()); + EXPECT_EQ(0, c1.tags().size()); + + Gauge& g1 = store_.gaugeFromStatName(makeStatName("g1")); + Gauge& g2 = scope1->gaugeFromStatName(makeStatName("g2")); + EXPECT_EQ("g1", g1.name()); + EXPECT_EQ("scope1.g2", g2.name()); + EXPECT_EQ("g1", g1.tagExtractedName()); + EXPECT_EQ("scope1.g2", g2.tagExtractedName()); + EXPECT_EQ(0, g1.tags().size()); + EXPECT_EQ(0, g1.tags().size()); + + Histogram& h1 = store_.histogramFromStatName(makeStatName("h1")); + Histogram& h2 = scope1->histogramFromStatName(makeStatName("h2")); + scope1->deliverHistogramToSinks(h2, 0); + EXPECT_EQ("h1", h1.name()); + EXPECT_EQ("scope1.h2", h2.name()); + EXPECT_EQ("h1", h1.tagExtractedName()); + EXPECT_EQ("scope1.h2", h2.tagExtractedName()); + EXPECT_EQ(0, h1.tags().size()); + EXPECT_EQ(0, h2.tags().size()); + h1.recordValue(200); + h2.recordValue(200); + + ScopePtr scope2 = scope1->createScope("foo."); + EXPECT_EQ("scope1.foo.bar", scope2->counterFromStatName(makeStatName("bar")).name()); + + // Validate that we sanitize away bad characters in the stats prefix. + ScopePtr scope3 = scope1->createScope(std::string("foo:\0:.", 7)); + EXPECT_EQ("scope1.foo___.bar", scope3->counter("bar").name()); + + EXPECT_EQ(4UL, store_.counters().size()); + EXPECT_EQ(2UL, store_.gauges().size()); } -TEST(StatsIsolatedStoreImplTest, LongStatName) { - IsolatedStoreImpl store; +TEST_F(StatsIsolatedStoreImplTest, LongStatName) { Stats::StatsOptionsImpl stats_options; const std::string long_string(stats_options.maxNameLength() + 1, 'A'); - ScopePtr scope = store.createScope("scope."); + ScopePtr scope = store_.createScope("scope."); Counter& counter = scope->counter(long_string); EXPECT_EQ(absl::StrCat("scope.", long_string), counter.name()); } @@ -80,11 +145,10 @@ struct TestStats { ALL_TEST_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT) }; -TEST(StatsMacros, All) { - IsolatedStoreImpl stats_store; - TestStats test_stats{ALL_TEST_STATS(POOL_COUNTER_PREFIX(stats_store, "test."), - POOL_GAUGE_PREFIX(stats_store, "test."), - POOL_HISTOGRAM_PREFIX(stats_store, "test."))}; +TEST_F(StatsIsolatedStoreImplTest, StatsMacros) { + TestStats test_stats{ALL_TEST_STATS(POOL_COUNTER_PREFIX(store_, "test."), + POOL_GAUGE_PREFIX(store_, "test."), + POOL_HISTOGRAM_PREFIX(store_, "test."))}; Counter& counter = test_stats.test_counter_; EXPECT_EQ("test.test_counter", counter.name()); diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 363a97d63a30e..c4b01a921aa3a 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -476,6 +476,81 @@ TEST_F(StatsThreadLocalStoreTest, HotRestartTruncation) { EXPECT_CALL(*alloc_, free(_)).Times(2); } +class LookupWithStatNameTest : public testing::Test { +public: + LookupWithStatNameTest() : alloc_(symbol_table_), store_(options_, alloc_) {} + ~LookupWithStatNameTest() override { + store_.shutdownThreading(); + clearStorage(); + } + + void clearStorage() { + for (auto& stat_name_storage : stat_name_storage_) { + stat_name_storage.free(store_.symbolTable()); + } + stat_name_storage_.clear(); + EXPECT_EQ(0, store_.symbolTable().numSymbols()); + } + + StatName makeStatName(absl::string_view name) { + stat_name_storage_.emplace_back(makeStatStorage(name)); + return stat_name_storage_.back().statName(); + } + + StatNameStorage makeStatStorage(absl::string_view name) { + return StatNameStorage(name, store_.symbolTable()); + } + + Stats::FakeSymbolTableImpl symbol_table_; + HeapStatDataAllocator alloc_; + StatsOptionsImpl options_; + ThreadLocalStoreImpl store_; + std::vector stat_name_storage_; +}; + +TEST_F(LookupWithStatNameTest, All) { + ScopePtr scope1 = store_.createScope("scope1."); + Counter& c1 = store_.counterFromStatName(makeStatName("c1")); + Counter& c2 = scope1->counterFromStatName(makeStatName("c2")); + EXPECT_EQ("c1", c1.name()); + EXPECT_EQ("scope1.c2", c2.name()); + EXPECT_EQ("c1", c1.tagExtractedName()); + EXPECT_EQ("scope1.c2", c2.tagExtractedName()); + EXPECT_EQ(0, c1.tags().size()); + EXPECT_EQ(0, c1.tags().size()); + + Gauge& g1 = store_.gaugeFromStatName(makeStatName("g1")); + Gauge& g2 = scope1->gaugeFromStatName(makeStatName("g2")); + EXPECT_EQ("g1", g1.name()); + EXPECT_EQ("scope1.g2", g2.name()); + EXPECT_EQ("g1", g1.tagExtractedName()); + EXPECT_EQ("scope1.g2", g2.tagExtractedName()); + EXPECT_EQ(0, g1.tags().size()); + EXPECT_EQ(0, g1.tags().size()); + + Histogram& h1 = store_.histogramFromStatName(makeStatName("h1")); + Histogram& h2 = scope1->histogramFromStatName(makeStatName("h2")); + scope1->deliverHistogramToSinks(h2, 0); + EXPECT_EQ("h1", h1.name()); + EXPECT_EQ("scope1.h2", h2.name()); + EXPECT_EQ("h1", h1.tagExtractedName()); + EXPECT_EQ("scope1.h2", h2.tagExtractedName()); + EXPECT_EQ(0, h1.tags().size()); + EXPECT_EQ(0, h2.tags().size()); + h1.recordValue(200); + h2.recordValue(200); + + ScopePtr scope2 = scope1->createScope("foo."); + EXPECT_EQ("scope1.foo.bar", scope2->counterFromStatName(makeStatName("bar")).name()); + + // Validate that we sanitize away bad characters in the stats prefix. + ScopePtr scope3 = scope1->createScope(std::string("foo:\0:.", 7)); + EXPECT_EQ("scope1.foo___.bar", scope3->counter("bar").name()); + + EXPECT_EQ(5UL, store_.counters().size()); // The 4 objects created plus stats.overflow. + EXPECT_EQ(2UL, store_.gauges().size()); +} + class StatsMatcherTLSTest : public StatsThreadLocalStoreTest { public: envoy::config::metrics::v2::StatsConfig stats_config_; diff --git a/test/integration/server.h b/test/integration/server.h index 8f07317102c13..a33cab4d9e499 100644 --- a/test/integration/server.h +++ b/test/integration/server.h @@ -75,21 +75,35 @@ class TestScopeWrapper : public Scope { wrapped_scope_->deliverHistogramToSinks(histogram, value); } - Counter& counter(const std::string& name) override { + Counter& counterFromStatName(StatName name) override { Thread::LockGuard lock(lock_); - return wrapped_scope_->counter(name); + return wrapped_scope_->counterFromStatName(name); } - Gauge& gauge(const std::string& name) override { + Gauge& gaugeFromStatName(StatName name) override { Thread::LockGuard lock(lock_); - return wrapped_scope_->gauge(name); + return wrapped_scope_->gaugeFromStatName(name); } - NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; } + Histogram& histogramFromStatName(StatName name) override { + Thread::LockGuard lock(lock_); + return wrapped_scope_->histogramFromStatName(name); + } + NullGaugeImpl& nullGauge(const std::string& str) override { + return wrapped_scope_->nullGauge(str); + } + Counter& counter(const std::string& name) override { + StatNameTempStorage storage(name, symbolTable()); + return counterFromStatName(storage.statName()); + } + Gauge& gauge(const std::string& name) override { + StatNameTempStorage storage(name, symbolTable()); + return gaugeFromStatName(storage.statName()); + } Histogram& histogram(const std::string& name) override { - Thread::LockGuard lock(lock_); - return wrapped_scope_->histogram(name); + StatNameTempStorage storage(name, symbolTable()); + return histogramFromStatName(storage.statName()); } const SymbolTable& symbolTable() const override { return wrapped_scope_->symbolTable(); } @@ -100,7 +114,6 @@ class TestScopeWrapper : public Scope { Thread::MutexBasicLockable& lock_; ScopePtr wrapped_scope_; StatsOptionsImpl stats_options_; - NullGaugeImpl null_gauge_; }; /** @@ -111,6 +124,10 @@ class TestIsolatedStoreImpl : public StoreRoot { public: TestIsolatedStoreImpl() : source_(*this) {} // Stats::Scope + Counter& counterFromStatName(StatName name) override { + Thread::LockGuard lock(lock_); + return store_.counterFromStatName(name); + } Counter& counter(const std::string& name) override { Thread::LockGuard lock(lock_); return store_.counter(name); @@ -120,16 +137,26 @@ class TestIsolatedStoreImpl : public StoreRoot { return ScopePtr{new TestScopeWrapper(lock_, store_.createScope(name))}; } void deliverHistogramToSinks(const Histogram&, uint64_t) override {} + Gauge& gaugeFromStatName(StatName name) override { + Thread::LockGuard lock(lock_); + return store_.gaugeFromStatName(name); + } Gauge& gauge(const std::string& name) override { Thread::LockGuard lock(lock_); return store_.gauge(name); } - NullGaugeImpl& nullGauge(const std::string&) override { return null_gauge_; } + Histogram& histogramFromStatName(StatName name) override { + Thread::LockGuard lock(lock_); + return store_.histogramFromStatName(name); + } + NullGaugeImpl& nullGauge(const std::string& name) override { return store_.nullGauge(name); } Histogram& histogram(const std::string& name) override { Thread::LockGuard lock(lock_); return store_.histogram(name); } const StatsOptions& statsOptions() const override { return stats_options_; } + const SymbolTable& symbolTable() const override { return store_.symbolTable(); } + SymbolTable& symbolTable() override { return store_.symbolTable(); } // Stats::Store std::vector counters() const override { @@ -155,15 +182,11 @@ class TestIsolatedStoreImpl : public StoreRoot { void mergeHistograms(PostMergeCb) override {} Source& source() override { return source_; } - const SymbolTable& symbolTable() const override { return store_.symbolTable(); } - SymbolTable& symbolTable() override { return store_.symbolTable(); } - private: mutable Thread::MutexBasicLockable lock_; IsolatedStoreImpl store_; SourceImpl source_; StatsOptionsImpl stats_options_; - NullGaugeImpl null_gauge_; }; } // namespace Stats diff --git a/test/mocks/stats/BUILD b/test/mocks/stats/BUILD index 711c90c01fc29..6539e818467bc 100644 --- a/test/mocks/stats/BUILD +++ b/test/mocks/stats/BUILD @@ -21,6 +21,7 @@ envoy_cc_mock( "//source/common/stats:histogram_lib", "//source/common/stats:isolated_store_lib", "//source/common/stats:stats_lib", + "//source/common/stats:store_impl_lib", "//test/mocks:common_lib", "//test/test_common:global_lib", ], diff --git a/test/mocks/stats/mocks.cc b/test/mocks/stats/mocks.cc index 1b132a3b378c9..9b1b4ea76904c 100644 --- a/test/mocks/stats/mocks.cc +++ b/test/mocks/stats/mocks.cc @@ -72,7 +72,7 @@ MockSource::~MockSource() {} MockSink::MockSink() {} MockSink::~MockSink() {} -MockStore::MockStore() { +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; diff --git a/test/mocks/stats/mocks.h b/test/mocks/stats/mocks.h index ed628a5d8f815..a605c08bb5437 100644 --- a/test/mocks/stats/mocks.h +++ b/test/mocks/stats/mocks.h @@ -18,6 +18,7 @@ #include "common/stats/fake_symbol_table_impl.h" #include "common/stats/histogram_impl.h" #include "common/stats/isolated_store_impl.h" +#include "common/stats/store_impl.h" #include "test/test_common/global.h" @@ -150,7 +151,12 @@ class MockSink : public Sink { MOCK_METHOD2(onHistogramComplete, void(const Histogram& histogram, uint64_t value)); }; -class MockStore : public Store { +class SymbolTableProvider { +public: + Test::Global fake_symbol_table_; +}; + +class MockStore : public SymbolTableProvider, public StoreImpl { public: MockStore(); ~MockStore(); @@ -168,10 +174,6 @@ class MockStore : public Store { MOCK_CONST_METHOD0(histograms, std::vector()); MOCK_CONST_METHOD0(statsOptions, const StatsOptions&()); - 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_;