From 832ec740ed9d054415a00b017fb2de45febd0a26 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 10 Apr 2020 09:42:23 -0400 Subject: [PATCH 01/19] initial commit; several candidate call-sites not covered yet. Signed-off-by: Joshua Marantz --- source/common/http/BUILD | 1 + source/common/http/user_agent.cc | 20 ++--- source/common/stats/utility.cc | 48 +++++++++++ source/common/stats/utility.h | 54 ++++++++++++ .../filters/network/common/redis/BUILD | 1 + .../common/redis/redis_command_stats.cc | 28 ++----- .../common/redis/redis_command_stats.h | 3 - .../filters/network/zookeeper_proxy/filter.cc | 10 +-- source/extensions/transport_sockets/tls/BUILD | 1 + .../transport_sockets/tls/context_impl.cc | 8 +- test/common/stats/BUILD | 9 ++ test/common/stats/utility_test.cc | 83 +++++++++++++++++++ 12 files changed, 223 insertions(+), 43 deletions(-) create mode 100644 test/common/stats/utility_test.cc diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 3deaff82c3052..2c046db6cefee 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -327,6 +327,7 @@ envoy_cc_library( "//include/envoy/stats:stats_macros", "//include/envoy/stats:timespan_interface", "//source/common/stats:symbol_table_lib", + "//source/common/stats:utility_lib", ], ) diff --git a/source/common/http/user_agent.cc b/source/common/http/user_agent.cc index ca2774376751f..d6804f245ec52 100644 --- a/source/common/http/user_agent.cc +++ b/source/common/http/user_agent.cc @@ -10,6 +10,7 @@ #include "common/http/headers.h" #include "common/stats/symbol_table_impl.h" +#include "common/stats/utility.h" namespace Envoy { namespace Http { @@ -30,17 +31,14 @@ void UserAgent::completeConnectionLength(Stats::Timespan& span) { UserAgentStats::UserAgentStats(Stats::StatName prefix, Stats::StatName device, Stats::Scope& scope, const UserAgentContext& context) - : downstream_cx_total_(scope.counterFromStatName(Stats::StatName( - context.symbol_table_.join({prefix, device, context.downstream_cx_total_}).get()))), - downstream_cx_destroy_remote_active_rq_(scope.counterFromStatName(Stats::StatName( - context.symbol_table_ - .join({prefix, device, context.downstream_cx_destroy_remote_active_rq_}) - .get()))), - downstream_rq_total_(scope.counterFromStatName(Stats::StatName( - context.symbol_table_.join({prefix, device, context.downstream_rq_total_}).get()))), - downstream_cx_length_ms_(scope.histogramFromStatName( - Stats::StatName( - context.symbol_table_.join({prefix, device, context.downstream_cx_length_ms_}).get()), + : downstream_cx_total_(Stats::Utility::counterFromElements( + scope, {prefix, device, context.downstream_cx_total_})), + downstream_cx_destroy_remote_active_rq_(Stats::Utility::counterFromElements( + scope, {prefix, device, context.downstream_cx_destroy_remote_active_rq_})), + downstream_rq_total_(Stats::Utility::counterFromElements( + scope, {prefix, device, context.downstream_rq_total_})), + downstream_cx_length_ms_(Stats::Utility::histogramFromElements( + scope, {prefix, device, context.downstream_cx_length_ms_}, Stats::Histogram::Unit::Milliseconds)) { downstream_cx_total_.inc(); downstream_rq_total_.inc(); diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index 18441355fd3d7..2195dda90f981 100644 --- a/source/common/stats/utility.cc +++ b/source/common/stats/utility.cc @@ -4,6 +4,7 @@ #include #include "absl/strings/match.h" +#include "absl/types/optional.h" namespace Envoy { namespace Stats { @@ -34,5 +35,52 @@ absl::optional Utility::findTag(const Metric& metric, StatName find_ta return value; } +namespace { + +// Helper class for the three Utility::*FromElements implementations to build up +// a joined StatName from a mix of StatName and string_view. +struct ElementVisitor { + ElementVisitor(SymbolTable& symbol_table) : symbol_table_(symbol_table), pool_(symbol_table) {} + + // Overloads provides for absl::visit to call. + void operator()(StatName stat_name) { stat_names_.push_back(stat_name); } + void operator()(absl::string_view name) { stat_names_.push_back(pool_.add(name)); } + + // Generates a StatName from the elements. + StatName makeStatName(const Utility::ElementVec& elements) { + for (const Utility::Element& element : elements) { + absl::visit(*this, element); + } + joined_ = symbol_table_.join(stat_names_); + return StatName(joined_.get()); + } + + SymbolTable& symbol_table_; + StatNameVec stat_names_; + StatNameDynamicPool pool_; + SymbolTable::StoragePtr joined_; +}; + +} // namespace + +Counter& Utility::counterFromElements(Scope& scope, const ElementVec& elements, + StatNameTagVectorOptConstRef tags) { + ElementVisitor visitor(scope.symbolTable()); + return scope.counterFromStatNameWithTags(visitor.makeStatName(elements), tags); +} + +Gauge& Utility::gaugeFromElements(Scope& scope, const ElementVec& elements, + Gauge::ImportMode import_mode, + StatNameTagVectorOptConstRef tags) { + ElementVisitor visitor(scope.symbolTable()); + return scope.gaugeFromStatNameWithTags(visitor.makeStatName(elements), tags, import_mode); +} + +Histogram& Utility::histogramFromElements(Scope& scope, const ElementVec& elements, + Histogram::Unit unit, StatNameTagVectorOptConstRef tags) { + ElementVisitor visitor(scope.symbolTable()); + return scope.histogramFromStatNameWithTags(visitor.makeStatName(elements), tags, unit); +} + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 0d0ed4b21bc0a..c3a69642cf0fd 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -2,6 +2,7 @@ #include +#include "envoy/stats/scope.h" #include "envoy/stats/stats.h" #include "common/stats/symbol_table_impl.h" @@ -34,6 +35,59 @@ class Utility { * @return The value of the tag, if found. */ static absl::optional findTag(const Metric& metric, StatName find_tag_name); + + // Represents a stat name token, using either a StatName or a string_view. + using Element = absl::variant; + using ElementVec = std::vector; + + /** + * Creates a counter from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * @param scope The scope in which to create the counter. + * @param elements The vector of mixed string_view and StatName + * @param tags optionally specified tags. + * @return A counter named using the joined elements. + */ + static Counter& counterFromElements(Scope& scope, const ElementVec& elements, + StatNameTagVectorOptConstRef tags = absl::nullopt); + + /** + * Creates a gauge from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * @param scope The scope in which to create the counter. + * @param elements The vector of mixed string_view and StatName + * @param import_mode Whether hot-restart should accumulate this value. + * @param tags optionally specified tags. + * @return A gauge named using the joined elements. + */ + static Gauge& gaugeFromElements(Scope& scope, const ElementVec& elements, + Gauge::ImportMode import_mode, + StatNameTagVectorOptConstRef tags = absl::nullopt); + + /** + * Creates a histogram from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * @param scope The scope in which to create the counter. + * @param elements The vector of mixed string_view and StatName + * @param unit The unit of measurement. + * @param tags optionally specified tags. + * @return A histogram named using the joined elements. + */ + static Histogram& histogramFromElements(Scope& scope, const ElementVec& elements, + Histogram::Unit unit, + StatNameTagVectorOptConstRef tags = absl::nullopt); }; } // namespace Stats diff --git a/source/extensions/filters/network/common/redis/BUILD b/source/extensions/filters/network/common/redis/BUILD index a7adc168788fa..d648832e0b1ee 100644 --- a/source/extensions/filters/network/common/redis/BUILD +++ b/source/extensions/filters/network/common/redis/BUILD @@ -97,5 +97,6 @@ envoy_cc_library( "//source/common/common:utility_lib", "//source/common/stats:symbol_table_lib", "//source/common/stats:timespan_lib", + "//source/common/stats:utility_lib", ], ) diff --git a/source/extensions/filters/network/common/redis/redis_command_stats.cc b/source/extensions/filters/network/common/redis/redis_command_stats.cc index 02307dc9c1c24..60657517333b4 100644 --- a/source/extensions/filters/network/common/redis/redis_command_stats.cc +++ b/source/extensions/filters/network/common/redis/redis_command_stats.cc @@ -1,6 +1,7 @@ #include "extensions/filters/network/common/redis/redis_command_stats.h" #include "common/stats/timespan_impl.h" +#include "common/stats/utility.h" #include "extensions/filters/network/common/redis/supported_commands.h" @@ -32,33 +33,20 @@ RedisCommandStats::RedisCommandStats(Stats::SymbolTable& symbol_table, const std Extensions::NetworkFilters::Common::Redis::SupportedCommands::mset()); } -Stats::Counter& RedisCommandStats::counter(Stats::Scope& scope, - const Stats::StatNameVec& stat_names) { - const Stats::SymbolTable::StoragePtr storage_ptr = symbol_table_.join(stat_names); - Stats::StatName full_stat_name = Stats::StatName(storage_ptr.get()); - return scope.counterFromStatName(full_stat_name); -} - -Stats::Histogram& RedisCommandStats::histogram(Stats::Scope& scope, - const Stats::StatNameVec& stat_names, - Stats::Histogram::Unit unit) { - const Stats::SymbolTable::StoragePtr storage_ptr = symbol_table_.join(stat_names); - Stats::StatName full_stat_name = Stats::StatName(storage_ptr.get()); - return scope.histogramFromStatName(full_stat_name, unit); -} - Stats::TimespanPtr RedisCommandStats::createCommandTimer(Stats::Scope& scope, Stats::StatName command, Envoy::TimeSource& time_source) { return std::make_unique( - histogram(scope, {prefix_, command, latency_}, Stats::Histogram::Unit::Microseconds), + Stats::Utility::histogramFromElements(scope, {prefix_, command, latency_}, + Stats::Histogram::Unit::Microseconds), time_source); } Stats::TimespanPtr RedisCommandStats::createAggregateTimer(Stats::Scope& scope, Envoy::TimeSource& time_source) { return std::make_unique( - histogram(scope, {prefix_, upstream_rq_time_}, Stats::Histogram::Unit::Microseconds), + Stats::Utility::histogramFromElements(scope, {prefix_, upstream_rq_time_}, + Stats::Histogram::Unit::Microseconds), time_source); } @@ -84,15 +72,15 @@ Stats::StatName RedisCommandStats::getCommandFromRequest(const RespValue& reques } void RedisCommandStats::updateStatsTotal(Stats::Scope& scope, Stats::StatName command) { - counter(scope, {prefix_, command, total_}).inc(); + Stats::Utility::counterFromElements(scope, {prefix_, command, total_}).inc(); } void RedisCommandStats::updateStats(Stats::Scope& scope, Stats::StatName command, const bool success) { if (success) { - counter(scope, {prefix_, command, success_}).inc(); + Stats::Utility::counterFromElements(scope, {prefix_, command, success_}).inc(); } else { - counter(scope, {prefix_, command, failure_}).inc(); + Stats::Utility::counterFromElements(scope, {prefix_, command, failure_}).inc(); } } diff --git a/source/extensions/filters/network/common/redis/redis_command_stats.h b/source/extensions/filters/network/common/redis/redis_command_stats.h index a2870ea4003e3..5dddb9f8303c9 100644 --- a/source/extensions/filters/network/common/redis/redis_command_stats.h +++ b/source/extensions/filters/network/common/redis/redis_command_stats.h @@ -28,9 +28,6 @@ class RedisCommandStats { return std::make_shared(symbol_table, "upstream_commands"); } - Stats::Counter& counter(Stats::Scope& scope, const Stats::StatNameVec& stat_names); - Stats::Histogram& histogram(Stats::Scope& scope, const Stats::StatNameVec& stat_names, - Stats::Histogram::Unit unit); Stats::TimespanPtr createCommandTimer(Stats::Scope& scope, Stats::StatName command, Envoy::TimeSource& time_source); Stats::TimespanPtr createAggregateTimer(Stats::Scope& scope, Envoy::TimeSource& time_source); diff --git a/source/extensions/filters/network/zookeeper_proxy/filter.cc b/source/extensions/filters/network/zookeeper_proxy/filter.cc index 331d8a476e692..517b2803c3cb8 100644 --- a/source/extensions/filters/network/zookeeper_proxy/filter.cc +++ b/source/extensions/filters/network/zookeeper_proxy/filter.cc @@ -154,11 +154,11 @@ void ZooKeeperFilter::onPing() { } void ZooKeeperFilter::onAuthRequest(const std::string& scheme) { - Stats::SymbolTable::StoragePtr storage = config_->scope_.symbolTable().join( - {config_->stat_prefix_, config_->auth_, - config_->stat_name_set_->getBuiltin(absl::StrCat(scheme, "_rq"), - config_->unknown_scheme_rq_)}); - config_->scope_.counterFromStatName(Stats::StatName(storage.get())).inc(); + Stats::Counter& counter = Stats::Utility::counterFromElements( + config_->scope_, {config_->stat_prefix_, config_->auth_, + config_->stat_name_set_->getBuiltin(absl::StrCat(scheme, "_rq"), + config_->unknown_scheme_rq_)}); + counter.inc(); setDynamicMetadata("opname", "auth"); } diff --git a/source/extensions/transport_sockets/tls/BUILD b/source/extensions/transport_sockets/tls/BUILD index 748c7b99559fb..7cf2407b61fbd 100644 --- a/source/extensions/transport_sockets/tls/BUILD +++ b/source/extensions/transport_sockets/tls/BUILD @@ -110,6 +110,7 @@ envoy_cc_library( "//source/common/network:address_lib", "//source/common/protobuf:utility_lib", "//source/common/stats:symbol_table_lib", + "//source/common/stats:utility_lib", "//source/extensions/transport_sockets/tls/private_key:private_key_manager_lib", "@envoy_api//envoy/admin/v3:pkg_cc_proto", "@envoy_api//envoy/type/matcher/v3:pkg_cc_proto", diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 7292bba9b005f..2aaec7f14b887 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -19,6 +19,7 @@ #include "common/common/utility.h" #include "common/network/address_impl.h" #include "common/protobuf/utility.h" +#include "common/stats/utility.h" #include "extensions/transport_sockets/tls/utility.h" @@ -594,10 +595,9 @@ Envoy::Ssl::ClientValidationStatus ContextImpl::verifyCertificate( void ContextImpl::incCounter(const Stats::StatName name, absl::string_view value, const Stats::StatName fallback) const { - Stats::SymbolTable& symbol_table = scope_.symbolTable(); - Stats::SymbolTable::StoragePtr storage = - symbol_table.join({name, stat_name_set_->getBuiltin(value, fallback)}); - scope_.counterFromStatName(Stats::StatName(storage.get())).inc(); + Stats::Counter& counter = Stats::Utility::counterFromElements( + scope_, {name, stat_name_set_->getBuiltin(value, fallback)}); + counter.inc(); #ifdef LOG_BUILTIN_STAT_NAMES std::cerr << absl::StrCat("Builtin ", symbol_table.toString(name), ": ", value, "\n") diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index ad8e7885cd1cb..5e38a62688363 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -241,3 +241,12 @@ envoy_cc_test_binary( "@envoy_api//envoy/config/metrics/v3:pkg_cc_proto", ], ) + +envoy_cc_test( + name = "utility_test", + srcs = ["utility_test.cc"], + deps = [ + "//source/common/stats:isolated_store_lib", + "//source/common/stats:utility_lib", + ], +) diff --git a/test/common/stats/utility_test.cc b/test/common/stats/utility_test.cc new file mode 100644 index 0000000000000..4f79b8c171ddc --- /dev/null +++ b/test/common/stats/utility_test.cc @@ -0,0 +1,83 @@ +#include + +#include "envoy/stats/stats_macros.h" + +#include "common/stats/isolated_store_impl.h" +#include "common/stats/null_counter.h" +#include "common/stats/null_gauge.h" +#include "common/stats/symbol_table_creator.h" + +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Stats { + +class UtilityTest : public testing::Test { +protected: + UtilityTest() + : symbol_table_(SymbolTableCreator::makeSymbolTable()), + store_(std::make_unique(*symbol_table_)), pool_(*symbol_table_), + tags_( + {{pool_.add("tag1"), pool_.add("value1")}, {pool_.add("tag2"), pool_.add("value2")}}) {} + + ~UtilityTest() override { + pool_.clear(); + store_.reset(); + EXPECT_EQ(0, symbol_table_->numSymbols()); + } + + SymbolTablePtr symbol_table_; + std::unique_ptr store_; + StatNamePool pool_; + StatNameTagVector tags_; +}; + +TEST_F(UtilityTest, Counters) { + ScopePtr scope = store_->createScope("scope."); + Counter& c1 = Utility::counterFromElements(*scope, {"a", "b"}); + EXPECT_EQ("scope.a.b", c1.name()); + StatName token = pool_.add("token"); + Counter& c2 = Utility::counterFromElements(*scope, {"a", token, "b"}); + EXPECT_EQ("scope.a.token.b", c2.name()); + StatName suffix = pool_.add("suffix"); + Counter& c3 = Utility::counterFromElements(*scope, {token, suffix}); + EXPECT_EQ("scope.token.suffix", c3.name()); + + Counter& ctags = Utility::counterFromElements(*scope, {"x", token, "y"}, tags_); + EXPECT_EQ("scope.x.token.y.tag1.value1.tag2.value2", ctags.name()); +} + +TEST_F(UtilityTest, Gauges) { + ScopePtr scope = store_->createScope("scope."); + Gauge& g1 = Utility::gaugeFromElements(*scope, {"a", "b"}, Gauge::ImportMode::NeverImport); + EXPECT_EQ("scope.a.b", g1.name()); + EXPECT_EQ(Gauge::ImportMode::NeverImport, g1.importMode()); + StatName token = pool_.add("token"); + Gauge& g2 = Utility::gaugeFromElements(*scope, {"a", token, "b"}, Gauge::ImportMode::Accumulate); + EXPECT_EQ("scope.a.token.b", g2.name()); + EXPECT_EQ(Gauge::ImportMode::Accumulate, g2.importMode()); + StatName suffix = pool_.add("suffix"); + Gauge& g3 = Utility::gaugeFromElements(*scope, {token, suffix}, Gauge::ImportMode::NeverImport); + EXPECT_EQ("scope.token.suffix", g3.name()); +} + +TEST_F(UtilityTest, Histograms) { + ScopePtr scope = store_->createScope("scope."); + Histogram& h1 = Utility::histogramFromElements(*scope, {"a", "b"}, Histogram::Unit::Milliseconds); + EXPECT_EQ("scope.a.b", h1.name()); + EXPECT_EQ(Histogram::Unit::Milliseconds, h1.unit()); + StatName token = pool_.add("token"); + Histogram& h2 = + Utility::histogramFromElements(*scope, {"a", token, "b"}, Histogram::Unit::Microseconds); + EXPECT_EQ("scope.a.token.b", h2.name()); + EXPECT_EQ(Histogram::Unit::Microseconds, h2.unit()); + StatName suffix = pool_.add("suffix"); + Histogram& h3 = Utility::histogramFromElements(*scope, {token, suffix}, Histogram::Unit::Bytes); + EXPECT_EQ("scope.token.suffix", h3.name()); + EXPECT_EQ(Histogram::Unit::Bytes, h3.unit()); +} + +} // namespace Stats +} // namespace Envoy From 794ae83c1df7368f4e596ff20b98cf9cfb430cf6 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 10 Apr 2020 11:16:07 -0400 Subject: [PATCH 02/19] put common testname in anon namespace. Signed-off-by: Joshua Marantz --- test/common/stats/utility_test.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/common/stats/utility_test.cc b/test/common/stats/utility_test.cc index 4f79b8c171ddc..c55533cd7b595 100644 --- a/test/common/stats/utility_test.cc +++ b/test/common/stats/utility_test.cc @@ -13,14 +13,15 @@ namespace Envoy { namespace Stats { +namespace { class UtilityTest : public testing::Test { protected: UtilityTest() : symbol_table_(SymbolTableCreator::makeSymbolTable()), store_(std::make_unique(*symbol_table_)), pool_(*symbol_table_), - tags_( - {{pool_.add("tag1"), pool_.add("value1")}, {pool_.add("tag2"), pool_.add("value2")}}) {} + tags_({{pool_.add("tag1"), pool_.add("value1")}, + {pool_.add("tag2"), pool_.add("value2")}}) {} ~UtilityTest() override { pool_.clear(); @@ -69,8 +70,7 @@ TEST_F(UtilityTest, Histograms) { EXPECT_EQ("scope.a.b", h1.name()); EXPECT_EQ(Histogram::Unit::Milliseconds, h1.unit()); StatName token = pool_.add("token"); - Histogram& h2 = - Utility::histogramFromElements(*scope, {"a", token, "b"}, Histogram::Unit::Microseconds); + Histogram& h2 = Utility::histogramFromElements(*scope, {"a", token, "b"}, Histogram::Unit::Microseconds); EXPECT_EQ("scope.a.token.b", h2.name()); EXPECT_EQ(Histogram::Unit::Microseconds, h2.unit()); StatName suffix = pool_.add("suffix"); @@ -79,5 +79,6 @@ TEST_F(UtilityTest, Histograms) { EXPECT_EQ(Histogram::Unit::Bytes, h3.unit()); } +} // namespace } // namespace Stats } // namespace Envoy From 7bd3ababb2310d2e4f53c31bf093eba5cbcadafe Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 10 Apr 2020 11:37:12 -0400 Subject: [PATCH 03/19] format Signed-off-by: Joshua Marantz --- test/common/stats/utility_test.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/common/stats/utility_test.cc b/test/common/stats/utility_test.cc index c55533cd7b595..c19a02fbf581b 100644 --- a/test/common/stats/utility_test.cc +++ b/test/common/stats/utility_test.cc @@ -20,8 +20,8 @@ class UtilityTest : public testing::Test { UtilityTest() : symbol_table_(SymbolTableCreator::makeSymbolTable()), store_(std::make_unique(*symbol_table_)), pool_(*symbol_table_), - tags_({{pool_.add("tag1"), pool_.add("value1")}, - {pool_.add("tag2"), pool_.add("value2")}}) {} + tags_( + {{pool_.add("tag1"), pool_.add("value1")}, {pool_.add("tag2"), pool_.add("value2")}}) {} ~UtilityTest() override { pool_.clear(); @@ -70,7 +70,8 @@ TEST_F(UtilityTest, Histograms) { EXPECT_EQ("scope.a.b", h1.name()); EXPECT_EQ(Histogram::Unit::Milliseconds, h1.unit()); StatName token = pool_.add("token"); - Histogram& h2 = Utility::histogramFromElements(*scope, {"a", token, "b"}, Histogram::Unit::Microseconds); + Histogram& h2 = + Utility::histogramFromElements(*scope, {"a", token, "b"}, Histogram::Unit::Microseconds); EXPECT_EQ("scope.a.token.b", h2.name()); EXPECT_EQ(Histogram::Unit::Microseconds, h2.unit()); StatName suffix = pool_.add("suffix"); From 5e1c309838a5f81cd7d369163aa2d6412dc36cb6 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 10 Apr 2020 14:31:40 -0400 Subject: [PATCH 04/19] rename test-class as it maybe is causing some kind of coverage-test collision? Signed-off-by: Joshua Marantz --- test/common/stats/utility_test.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/common/stats/utility_test.cc b/test/common/stats/utility_test.cc index c19a02fbf581b..448cb898eaacc 100644 --- a/test/common/stats/utility_test.cc +++ b/test/common/stats/utility_test.cc @@ -15,15 +15,15 @@ namespace Envoy { namespace Stats { namespace { -class UtilityTest : public testing::Test { +class StatsUtilityTest : public testing::Test { protected: - UtilityTest() + StatsUtilityTest() : symbol_table_(SymbolTableCreator::makeSymbolTable()), store_(std::make_unique(*symbol_table_)), pool_(*symbol_table_), tags_( {{pool_.add("tag1"), pool_.add("value1")}, {pool_.add("tag2"), pool_.add("value2")}}) {} - ~UtilityTest() override { + ~StatsUtilityTest() override { pool_.clear(); store_.reset(); EXPECT_EQ(0, symbol_table_->numSymbols()); @@ -35,7 +35,7 @@ class UtilityTest : public testing::Test { StatNameTagVector tags_; }; -TEST_F(UtilityTest, Counters) { +TEST_F(StatsUtilityTest, Counters) { ScopePtr scope = store_->createScope("scope."); Counter& c1 = Utility::counterFromElements(*scope, {"a", "b"}); EXPECT_EQ("scope.a.b", c1.name()); @@ -50,7 +50,7 @@ TEST_F(UtilityTest, Counters) { EXPECT_EQ("scope.x.token.y.tag1.value1.tag2.value2", ctags.name()); } -TEST_F(UtilityTest, Gauges) { +TEST_F(StatsUtilityTest, Gauges) { ScopePtr scope = store_->createScope("scope."); Gauge& g1 = Utility::gaugeFromElements(*scope, {"a", "b"}, Gauge::ImportMode::NeverImport); EXPECT_EQ("scope.a.b", g1.name()); @@ -64,7 +64,7 @@ TEST_F(UtilityTest, Gauges) { EXPECT_EQ("scope.token.suffix", g3.name()); } -TEST_F(UtilityTest, Histograms) { +TEST_F(StatsUtilityTest, Histograms) { ScopePtr scope = store_->createScope("scope."); Histogram& h1 = Utility::histogramFromElements(*scope, {"a", "b"}, Histogram::Unit::Milliseconds); EXPECT_EQ("scope.a.b", h1.name()); From 6d9aaa7ef3018a0984db9d624cb9a395b78cf7fa Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 20 Apr 2020 20:11:13 -0400 Subject: [PATCH 05/19] use new stats utility for zookeeper. Signed-off-by: Joshua Marantz --- .../filters/network/zookeeper_proxy/filter.cc | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/network/zookeeper_proxy/filter.cc b/source/extensions/filters/network/zookeeper_proxy/filter.cc index 517b2803c3cb8..e334ca7436d43 100644 --- a/source/extensions/filters/network/zookeeper_proxy/filter.cc +++ b/source/extensions/filters/network/zookeeper_proxy/filter.cc @@ -290,11 +290,10 @@ void ZooKeeperFilter::onConnectResponse(const int32_t proto_version, const int32 const std::chrono::milliseconds& latency) { config_->stats_.connect_resp_.inc(); - Stats::SymbolTable::StoragePtr storage = - config_->scope_.symbolTable().join({config_->stat_prefix_, config_->connect_latency_}); - config_->scope_ - .histogramFromStatName(Stats::StatName(storage.get()), Stats::Histogram::Unit::Milliseconds) - .recordValue(latency.count()); + Stats::Histogram& histogram = Stats::Utility::histogramFromElements( + config_->scope_, {config_->stat_prefix_, config_->connect_latency_}, + Stats::Histogram::Unit::Milliseconds); + histogram.recordValue(latency.count()); setDynamicMetadata({{"opname", "connect_response"}, {"protocol_version", std::to_string(proto_version)}, @@ -315,6 +314,12 @@ void ZooKeeperFilter::onResponse(const OpCodes opcode, const int32_t xid, const } Stats::SymbolTable::StoragePtr storage = config_->scope_.symbolTable().join({config_->stat_prefix_, opcode_latency}); + + Stats::Histogram& histogram = Stats::Utility::histogramFromElements( + config_->scope_, {config_->stat_prefix_, opcode_latency}, + Stats::Histogram::Unit::Milliseconds); + histogram.recordValue(latency.count()); + config_->scope_ .histogramFromStatName(Stats::StatName(storage.get()), Stats::Histogram::Unit::Milliseconds) .recordValue(latency.count()); From 2e3a35d415568c93ce55d7176f76448334640999 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 20 Apr 2020 20:23:08 -0400 Subject: [PATCH 06/19] mongo and zookeeper Signed-off-by: Joshua Marantz --- .../filters/network/mongo_proxy/BUILD | 1 + .../network/mongo_proxy/mongo_stats.cc | 16 +++++++-------- .../filters/network/mongo_proxy/mongo_stats.h | 7 ++++--- .../filters/network/mongo_proxy/proxy.cc | 20 +++++++++---------- .../filters/network/mongo_proxy/proxy.h | 4 ++-- .../filters/network/zookeeper_proxy/filter.cc | 4 ---- 6 files changed, 23 insertions(+), 29 deletions(-) diff --git a/source/extensions/filters/network/mongo_proxy/BUILD b/source/extensions/filters/network/mongo_proxy/BUILD index c4c08d4a6bc29..e471803285adb 100644 --- a/source/extensions/filters/network/mongo_proxy/BUILD +++ b/source/extensions/filters/network/mongo_proxy/BUILD @@ -89,6 +89,7 @@ envoy_cc_library( deps = [ "//include/envoy/stats:stats_interface", "//source/common/stats:symbol_table_lib", + "//source/common/stats:utility_lib", ], ) diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc index bf9e90ce105c9..bf1207774181b 100644 --- a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc @@ -31,23 +31,21 @@ MongoStats::MongoStats(Stats::Scope& scope, absl::string_view prefix) stat_name_set_->rememberBuiltins({"insert", "query", "update", "delete"}); } -Stats::SymbolTable::StoragePtr MongoStats::addPrefix(const std::vector& names) { - std::vector names_with_prefix; +Stats::Utility::ElementVec MongoStats::addPrefix(const Stats::Utility::ElementVec& names) { + Stats::Utility::ElementVec names_with_prefix; names_with_prefix.reserve(1 + names.size()); names_with_prefix.push_back(prefix_); names_with_prefix.insert(names_with_prefix.end(), names.begin(), names.end()); - return scope_.symbolTable().join(names_with_prefix); + return names_with_prefix; } -void MongoStats::incCounter(const std::vector& names) { - const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names); - scope_.counterFromStatName(Stats::StatName(stat_name_storage.get())).inc(); +void MongoStats::incCounter(const Stats::Utility::ElementVec& names) { + Stats::Utility::counterFromElements(scope_, addPrefix(names)).inc(); } -void MongoStats::recordHistogram(const std::vector& names, +void MongoStats::recordHistogram(const Stats::Utility::ElementVec& names, Stats::Histogram::Unit unit, uint64_t sample) { - const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names); - scope_.histogramFromStatName(Stats::StatName(stat_name_storage.get()), unit).recordValue(sample); + Stats::Utility::histogramFromElements(scope_, addPrefix(names), unit).recordValue(sample); } } // namespace MongoProxy diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.h b/source/extensions/filters/network/mongo_proxy/mongo_stats.h index f49d4d34e7bfa..0072f5dd0f45b 100644 --- a/source/extensions/filters/network/mongo_proxy/mongo_stats.h +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.h @@ -7,6 +7,7 @@ #include "envoy/stats/scope.h" #include "common/stats/symbol_table_impl.h" +#include "common/stats/utility.h" namespace Envoy { namespace Extensions { @@ -17,8 +18,8 @@ class MongoStats { public: MongoStats(Stats::Scope& scope, absl::string_view prefix); - void incCounter(const std::vector& names); - void recordHistogram(const std::vector& names, Stats::Histogram::Unit unit, + void incCounter(const Stats::Utility::ElementVec& names); + void recordHistogram(const Stats::Utility::ElementVec& names, Stats::Histogram::Unit unit, uint64_t sample); /** @@ -34,7 +35,7 @@ class MongoStats { Stats::SymbolTable& symbolTable() { return scope_.symbolTable(); } private: - Stats::SymbolTable::StoragePtr addPrefix(const std::vector& names); + Stats::Utility::ElementVec addPrefix(const Stats::Utility::ElementVec& names); Stats::Scope& scope_; Stats::StatNameSetPtr stat_name_set_; diff --git a/source/extensions/filters/network/mongo_proxy/proxy.cc b/source/extensions/filters/network/mongo_proxy/proxy.cc index fcbd3f6c52bb0..6049030ca0044 100644 --- a/source/extensions/filters/network/mongo_proxy/proxy.cc +++ b/source/extensions/filters/network/mongo_proxy/proxy.cc @@ -152,17 +152,16 @@ void ProxyFilter::decodeQuery(QueryMessagePtr&& message) { } else { // Normal query, get stats on a per collection basis first. QueryMessageInfo::QueryType query_type = active_query->query_info_.type(); - Stats::StatNameVec names; + Stats::Utility::ElementVec names; names.reserve(6); // 2 entries are added by chargeQueryStats(). names.push_back(mongo_stats_->collection_); - Stats::StatNameDynamicPool dynamic(mongo_stats_->symbolTable()); - names.push_back(dynamic.add(active_query->query_info_.collection())); + names.push_back(active_query->query_info_.collection()); chargeQueryStats(names, query_type); // Callsite stats if we have it. if (!active_query->query_info_.callsite().empty()) { names.push_back(mongo_stats_->callsite_); - names.push_back(dynamic.add(active_query->query_info_.callsite())); + names.push_back(active_query->query_info_.callsite()); chargeQueryStats(names, query_type); } @@ -180,7 +179,7 @@ void ProxyFilter::decodeQuery(QueryMessagePtr&& message) { active_query_list_.emplace_back(std::move(active_query)); } -void ProxyFilter::chargeQueryStats(Stats::StatNameVec& names, +void ProxyFilter::chargeQueryStats(Stats::Utility::ElementVec& names, QueryMessageInfo::QueryType query_type) { // names come in containing {"collection", collection}. Report stats for 1 or // 2 variations on this array, and then return with the array in the same @@ -224,15 +223,14 @@ void ProxyFilter::decodeReply(ReplyMessagePtr&& message) { } if (!active_query.query_info_.command().empty()) { - Stats::StatNameVec names{mongo_stats_->cmd_, + Stats::Utility::ElementVec names{mongo_stats_->cmd_, mongo_stats_->getBuiltin(active_query.query_info_.command(), mongo_stats_->unknown_command_)}; chargeReplyStats(active_query, names, *message); } else { // Collection stats first. - Stats::StatNameDynamicPool dynamic(mongo_stats_->symbolTable()); - Stats::StatNameVec names{mongo_stats_->collection_, - dynamic.add(active_query.query_info_.collection()), + Stats::Utility::ElementVec names{mongo_stats_->collection_, + active_query.query_info_.collection(), mongo_stats_->query_}; chargeReplyStats(active_query, names, *message); @@ -242,7 +240,7 @@ void ProxyFilter::decodeReply(ReplyMessagePtr&& message) { // to mutate the array to {"collection", collection, "callsite", callsite, "query"}. ASSERT(names.size() == 3); names.back() = mongo_stats_->callsite_; // Replaces "query". - names.push_back(dynamic.add(active_query.query_info_.callsite())); + names.push_back(active_query.query_info_.callsite()); names.push_back(mongo_stats_->query_); chargeReplyStats(active_query, names, *message); } @@ -292,7 +290,7 @@ void ProxyFilter::onDrainClose() { read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); } -void ProxyFilter::chargeReplyStats(ActiveQuery& active_query, Stats::StatNameVec& names, +void ProxyFilter::chargeReplyStats(ActiveQuery& active_query, Stats::Utility::ElementVec& names, const ReplyMessage& message) { uint64_t reply_documents_byte_size = 0; for (const Bson::DocumentSharedPtr& document : message.documents()) { diff --git a/source/extensions/filters/network/mongo_proxy/proxy.h b/source/extensions/filters/network/mongo_proxy/proxy.h index 0da6146f418e8..0a5ba93dc90b7 100644 --- a/source/extensions/filters/network/mongo_proxy/proxy.h +++ b/source/extensions/filters/network/mongo_proxy/proxy.h @@ -167,12 +167,12 @@ class ProxyFilter : public Network::Filter, // Increment counters related to queries. 'names' is passed by non-const // reference so the implementation can mutate it without copying, though it // always restores it to its prior state prior to return. - void chargeQueryStats(Stats::StatNameVec& names, QueryMessageInfo::QueryType query_type); + void chargeQueryStats(Stats::Utility::ElementVec& names, QueryMessageInfo::QueryType query_type); // Add samples to histograms related to replies. 'names' is passed by // non-const reference so the implementation can mutate it without copying, // though it always restores it to its prior state prior to return. - void chargeReplyStats(ActiveQuery& active_query, Stats::StatNameVec& names, + void chargeReplyStats(ActiveQuery& active_query, Stats::Utility::ElementVec& names, const ReplyMessage& message); void doDecode(Buffer::Instance& buffer); diff --git a/source/extensions/filters/network/zookeeper_proxy/filter.cc b/source/extensions/filters/network/zookeeper_proxy/filter.cc index e334ca7436d43..3ada43bdafcac 100644 --- a/source/extensions/filters/network/zookeeper_proxy/filter.cc +++ b/source/extensions/filters/network/zookeeper_proxy/filter.cc @@ -320,10 +320,6 @@ void ZooKeeperFilter::onResponse(const OpCodes opcode, const int32_t xid, const Stats::Histogram::Unit::Milliseconds); histogram.recordValue(latency.count()); - config_->scope_ - .histogramFromStatName(Stats::StatName(storage.get()), Stats::Histogram::Unit::Milliseconds) - .recordValue(latency.count()); - setDynamicMetadata({{"opname", opname}, {"xid", std::to_string(xid)}, {"zxid", std::to_string(zxid)}, From e57c49314a37a400aea01f7c9c8add795a72b72d Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 20 Apr 2020 20:28:57 -0400 Subject: [PATCH 07/19] fault filter, and zookeeper cleanup Signed-off-by: Joshua Marantz --- source/extensions/filters/http/fault/BUILD | 1 + source/extensions/filters/http/fault/fault_filter.cc | 5 ++--- source/extensions/filters/network/zookeeper_proxy/filter.cc | 2 -- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/http/fault/BUILD b/source/extensions/filters/http/fault/BUILD index 3c6e1775235db..749e04b67a4c8 100644 --- a/source/extensions/filters/http/fault/BUILD +++ b/source/extensions/filters/http/fault/BUILD @@ -33,6 +33,7 @@ envoy_cc_library( "//source/common/http:header_utility_lib", "//source/common/http:headers_lib", "//source/common/protobuf:utility_lib", + "//source/common/stats:utility_lib", "//source/extensions/filters/common/fault:fault_config_lib", "//source/extensions/filters/http:well_known_names", "@envoy_api//envoy/extensions/filters/http/fault/v3:pkg_cc_proto", diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index f3e277edfe6b7..72a032dae9074 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -19,6 +19,7 @@ #include "common/http/headers.h" #include "common/http/utility.h" #include "common/protobuf/utility.h" +#include "common/stats/utility.h" #include "extensions/filters/http/well_known_names.h" @@ -85,9 +86,7 @@ FaultFilterConfig::FaultFilterConfig( stats_prefix_(stat_name_set_->add(absl::StrCat(stats_prefix, "fault"))) {} void FaultFilterConfig::incCounter(Stats::StatName downstream_cluster, Stats::StatName stat_name) { - Stats::SymbolTable::StoragePtr storage = - scope_.symbolTable().join({stats_prefix_, downstream_cluster, stat_name}); - scope_.counterFromStatName(Stats::StatName(storage.get())).inc(); + Stats::Utility::counterFromElements(scope_, {stats_prefix_, downstream_cluster, stat_name}).inc(); } FaultFilter::FaultFilter(FaultFilterConfigSharedPtr config) : config_(config) {} diff --git a/source/extensions/filters/network/zookeeper_proxy/filter.cc b/source/extensions/filters/network/zookeeper_proxy/filter.cc index 3ada43bdafcac..491c7a5f61678 100644 --- a/source/extensions/filters/network/zookeeper_proxy/filter.cc +++ b/source/extensions/filters/network/zookeeper_proxy/filter.cc @@ -312,8 +312,6 @@ void ZooKeeperFilter::onResponse(const OpCodes opcode, const int32_t xid, const opname = opcode_info.opname_; opcode_latency = opcode_info.latency_name_; } - Stats::SymbolTable::StoragePtr storage = - config_->scope_.symbolTable().join({config_->stat_prefix_, opcode_latency}); Stats::Histogram& histogram = Stats::Utility::histogramFromElements( config_->scope_, {config_->stat_prefix_, opcode_latency}, From 90fd639cc193a53aa8d66d367220fde97682745f Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 20 Apr 2020 20:54:45 -0400 Subject: [PATCH 08/19] Require an explicit 'Dynamic' type annotation for dynamic strings passed to new util functions. Signed-off-by: Joshua Marantz --- .bazelversion | 2 +- source/common/stats/utility.h | 11 ++++++++-- .../filters/network/mongo_proxy/proxy.cc | 14 ++++++------ test/common/stats/utility_test.cc | 22 ++++++++++++------- 4 files changed, 31 insertions(+), 18 deletions(-) diff --git a/.bazelversion b/.bazelversion index 4a36342fcab70..ccbccc3dc6263 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -3.0.0 +2.2.0 diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index c3a69642cf0fd..65a8f76f3a55f 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -36,8 +36,15 @@ class Utility { */ static absl::optional findTag(const Metric& metric, StatName find_tag_name); - // Represents a stat name token, using either a StatName or a string_view. - using Element = absl::variant; + // Represents a stat name token, using either a StatName or a string_view, + // which will be treated as a dynamic string. We subclass string_view simply + // to make it a bit more explicit when we are creating a dynamic stat name, + // since those are expensive. + class Dynamic : public absl::string_view { + public: + Dynamic(absl::string_view s) : absl::string_view(s) {} + }; + using Element = absl::variant; using ElementVec = std::vector; /** diff --git a/source/extensions/filters/network/mongo_proxy/proxy.cc b/source/extensions/filters/network/mongo_proxy/proxy.cc index 6049030ca0044..46d90cd7fc237 100644 --- a/source/extensions/filters/network/mongo_proxy/proxy.cc +++ b/source/extensions/filters/network/mongo_proxy/proxy.cc @@ -155,13 +155,13 @@ void ProxyFilter::decodeQuery(QueryMessagePtr&& message) { Stats::Utility::ElementVec names; names.reserve(6); // 2 entries are added by chargeQueryStats(). names.push_back(mongo_stats_->collection_); - names.push_back(active_query->query_info_.collection()); + names.push_back(Stats::Utility::Dynamic(active_query->query_info_.collection())); chargeQueryStats(names, query_type); // Callsite stats if we have it. if (!active_query->query_info_.callsite().empty()) { names.push_back(mongo_stats_->callsite_); - names.push_back(active_query->query_info_.callsite()); + names.push_back(Stats::Utility::Dynamic(active_query->query_info_.callsite())); chargeQueryStats(names, query_type); } @@ -224,14 +224,14 @@ void ProxyFilter::decodeReply(ReplyMessagePtr&& message) { if (!active_query.query_info_.command().empty()) { Stats::Utility::ElementVec names{mongo_stats_->cmd_, - mongo_stats_->getBuiltin(active_query.query_info_.command(), - mongo_stats_->unknown_command_)}; + mongo_stats_->getBuiltin(active_query.query_info_.command(), + mongo_stats_->unknown_command_)}; chargeReplyStats(active_query, names, *message); } else { // Collection stats first. Stats::Utility::ElementVec names{mongo_stats_->collection_, - active_query.query_info_.collection(), - mongo_stats_->query_}; + Stats::Utility::Dynamic(active_query.query_info_.collection()), + mongo_stats_->query_}; chargeReplyStats(active_query, names, *message); // Callsite stats if we have it. @@ -240,7 +240,7 @@ void ProxyFilter::decodeReply(ReplyMessagePtr&& message) { // to mutate the array to {"collection", collection, "callsite", callsite, "query"}. ASSERT(names.size() == 3); names.back() = mongo_stats_->callsite_; // Replaces "query". - names.push_back(active_query.query_info_.callsite()); + names.push_back(Stats::Utility::Dynamic(active_query.query_info_.callsite())); names.push_back(mongo_stats_->query_); chargeReplyStats(active_query, names, *message); } diff --git a/test/common/stats/utility_test.cc b/test/common/stats/utility_test.cc index 448cb898eaacc..782ffb20aab42 100644 --- a/test/common/stats/utility_test.cc +++ b/test/common/stats/utility_test.cc @@ -37,26 +37,31 @@ class StatsUtilityTest : public testing::Test { TEST_F(StatsUtilityTest, Counters) { ScopePtr scope = store_->createScope("scope."); - Counter& c1 = Utility::counterFromElements(*scope, {"a", "b"}); + Counter& c1 = + Utility::counterFromElements(*scope, {Utility::Dynamic("a"), Utility::Dynamic("b")}); EXPECT_EQ("scope.a.b", c1.name()); StatName token = pool_.add("token"); - Counter& c2 = Utility::counterFromElements(*scope, {"a", token, "b"}); + Counter& c2 = + Utility::counterFromElements(*scope, {Utility::Dynamic("a"), token, Utility::Dynamic("b")}); EXPECT_EQ("scope.a.token.b", c2.name()); StatName suffix = pool_.add("suffix"); Counter& c3 = Utility::counterFromElements(*scope, {token, suffix}); EXPECT_EQ("scope.token.suffix", c3.name()); - Counter& ctags = Utility::counterFromElements(*scope, {"x", token, "y"}, tags_); + Counter& ctags = Utility::counterFromElements( + *scope, {Utility::Dynamic("x"), token, Utility::Dynamic("y")}, tags_); EXPECT_EQ("scope.x.token.y.tag1.value1.tag2.value2", ctags.name()); } TEST_F(StatsUtilityTest, Gauges) { ScopePtr scope = store_->createScope("scope."); - Gauge& g1 = Utility::gaugeFromElements(*scope, {"a", "b"}, Gauge::ImportMode::NeverImport); + Gauge& g1 = Utility::gaugeFromElements(*scope, {Utility::Dynamic("a"), Utility::Dynamic("b")}, + Gauge::ImportMode::NeverImport); EXPECT_EQ("scope.a.b", g1.name()); EXPECT_EQ(Gauge::ImportMode::NeverImport, g1.importMode()); StatName token = pool_.add("token"); - Gauge& g2 = Utility::gaugeFromElements(*scope, {"a", token, "b"}, Gauge::ImportMode::Accumulate); + Gauge& g2 = Utility::gaugeFromElements( + *scope, {Utility::Dynamic("a"), token, Utility::Dynamic("b")}, Gauge::ImportMode::Accumulate); EXPECT_EQ("scope.a.token.b", g2.name()); EXPECT_EQ(Gauge::ImportMode::Accumulate, g2.importMode()); StatName suffix = pool_.add("suffix"); @@ -66,12 +71,13 @@ TEST_F(StatsUtilityTest, Gauges) { TEST_F(StatsUtilityTest, Histograms) { ScopePtr scope = store_->createScope("scope."); - Histogram& h1 = Utility::histogramFromElements(*scope, {"a", "b"}, Histogram::Unit::Milliseconds); + Histogram& h1 = Utility::histogramFromElements( + *scope, {Utility::Dynamic("a"), Utility::Dynamic("b")}, Histogram::Unit::Milliseconds); EXPECT_EQ("scope.a.b", h1.name()); EXPECT_EQ(Histogram::Unit::Milliseconds, h1.unit()); StatName token = pool_.add("token"); - Histogram& h2 = - Utility::histogramFromElements(*scope, {"a", token, "b"}, Histogram::Unit::Microseconds); + Histogram& h2 = Utility::histogramFromElements( + *scope, {Utility::Dynamic("a"), token, Utility::Dynamic("b")}, Histogram::Unit::Microseconds); EXPECT_EQ("scope.a.token.b", h2.name()); EXPECT_EQ(Histogram::Unit::Microseconds, h2.unit()); StatName suffix = pool_.add("suffix"); From 91ff5ac99e615dad4a03d95863706778af17e825 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 21 Apr 2020 08:29:11 -0400 Subject: [PATCH 09/19] dynamo Signed-off-by: Joshua Marantz --- source/extensions/filters/http/dynamo/BUILD | 1 + .../filters/http/dynamo/dynamo_stats.cc | 25 ++++++++----------- .../filters/http/dynamo/dynamo_stats.h | 7 +++--- 3 files changed, 15 insertions(+), 18 deletions(-) diff --git a/source/extensions/filters/http/dynamo/BUILD b/source/extensions/filters/http/dynamo/BUILD index 9eac6935f3304..79296db8818f5 100644 --- a/source/extensions/filters/http/dynamo/BUILD +++ b/source/extensions/filters/http/dynamo/BUILD @@ -61,5 +61,6 @@ envoy_cc_library( ":dynamo_request_parser_lib", "//include/envoy/stats:stats_interface", "//source/common/stats:symbol_table_lib", + "//source/common/stats:utility_lib", ], ) diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.cc b/source/extensions/filters/http/dynamo/dynamo_stats.cc index 468c77f0a9592..57e846ffd8ddc 100644 --- a/source/extensions/filters/http/dynamo/dynamo_stats.cc +++ b/source/extensions/filters/http/dynamo/dynamo_stats.cc @@ -46,25 +46,21 @@ DynamoStats::DynamoStats(Stats::Scope& scope, const std::string& prefix) stat_name_set_->rememberBuiltins({"operation", "table"}); } -Stats::SymbolTable::StoragePtr DynamoStats::addPrefix(const Stats::StatNameVec& names) { - Stats::StatNameVec names_with_prefix; +Stats::Utility::ElementVec DynamoStats::addPrefix(const Stats::Utility::ElementVec& names) { + Stats::Utility::ElementVec names_with_prefix; names_with_prefix.reserve(1 + names.size()); names_with_prefix.push_back(prefix_); names_with_prefix.insert(names_with_prefix.end(), names.begin(), names.end()); - return scope_.symbolTable().join(names_with_prefix); + return names_with_prefix; } -void DynamoStats::incCounter(const Stats::StatNameVec& names) { - const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names); - scope_.counterFromStatName(Stats::StatName(stat_name_storage.get())).inc(); +void DynamoStats::incCounter(const Stats::Utility::ElementVec& names) { + Stats::Utility::counterFromElements(scope_, addPrefix(names)).inc(); } -void DynamoStats::recordHistogram(const Stats::StatNameVec& names, Stats::Histogram::Unit unit, +void DynamoStats::recordHistogram(const Stats::Utility::ElementVec& names, Stats::Histogram::Unit unit, uint64_t value) { - const Stats::SymbolTable::StoragePtr stat_name_storage = addPrefix(names); - Stats::Histogram& histogram = - scope_.histogramFromStatName(Stats::StatName(stat_name_storage.get()), unit); - histogram.recordValue(value); + Stats::Utility::histogramFromElements(scope_, addPrefix(names), unit).recordValue(value); } Stats::Counter& DynamoStats::buildPartitionStatCounter(const std::string& table_name, @@ -74,10 +70,9 @@ Stats::Counter& DynamoStats::buildPartitionStatCounter(const std::string& table_ absl::string_view id_last_7 = absl::string_view(partition_id).substr(partition_id.size() - 7); Stats::StatNameDynamicPool dynamic(scope_.symbolTable()); const Stats::StatName partition = dynamic.add(absl::StrCat("__partition_id=", id_last_7)); - const Stats::SymbolTable::StoragePtr stat_name_storage = - addPrefix({table_, dynamic.add(table_name), capacity_, - getBuiltin(operation, unknown_operation_), partition}); - return scope_.counterFromStatName(Stats::StatName(stat_name_storage.get())); + return Stats::Utility::counterFromElements(scope_, addPrefix({ + table_, Stats::Utility::Dynamic(table_name), capacity_, + getBuiltin(operation, unknown_operation_), partition})); } size_t DynamoStats::groupIndex(uint64_t status) { diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.h b/source/extensions/filters/http/dynamo/dynamo_stats.h index 4241ec5dd711b..c1928046aae2b 100644 --- a/source/extensions/filters/http/dynamo/dynamo_stats.h +++ b/source/extensions/filters/http/dynamo/dynamo_stats.h @@ -6,6 +6,7 @@ #include "envoy/stats/scope.h" #include "common/stats/symbol_table_impl.h" +#include "common/stats/utility.h" namespace Envoy { namespace Extensions { @@ -16,8 +17,8 @@ class DynamoStats { public: DynamoStats(Stats::Scope& scope, const std::string& prefix); - void incCounter(const Stats::StatNameVec& names); - void recordHistogram(const Stats::StatNameVec& names, Stats::Histogram::Unit unit, + void incCounter(const Stats::Utility::ElementVec& names); + void recordHistogram(const Stats::Utility::ElementVec& names, Stats::Histogram::Unit unit, uint64_t value); /** @@ -42,7 +43,7 @@ class DynamoStats { Stats::SymbolTable& symbolTable() { return scope_.symbolTable(); } private: - Stats::SymbolTable::StoragePtr addPrefix(const Stats::StatNameVec& names); + Stats::Utility::ElementVec addPrefix(const Stats::Utility::ElementVec& names); Stats::Scope& scope_; Stats::StatNameSetPtr stat_name_set_; From 85b57ed38bd19d15f0f22d1c58bc707b656323a0 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 21 Apr 2020 17:30:05 -0400 Subject: [PATCH 10/19] add ScopeSharedImpl as an alternative leaner API to Utility::*FromElements. Signed-off-by: Joshua Marantz --- .bazelversion | 2 +- include/envoy/stats/scope.h | 70 ++++++++++++++++++++++++ source/common/stats/BUILD | 11 ++++ source/common/stats/scope_prefixer.h | 3 +- source/common/stats/scope_shared_impl.cc | 53 ++++++++++++++++++ source/common/stats/scope_shared_impl.h | 22 ++++++++ source/common/stats/thread_local_store.h | 3 +- 7 files changed, 161 insertions(+), 3 deletions(-) create mode 100644 source/common/stats/scope_shared_impl.cc create mode 100644 source/common/stats/scope_shared_impl.h diff --git a/.bazelversion b/.bazelversion index ccbccc3dc6263..4a36342fcab70 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -2.2.0 +3.0.0 diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index 408655bbb8a5c..0479ee511d2ff 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -10,6 +10,7 @@ #include "envoy/stats/tag.h" #include "absl/types/optional.h" +#include "absl/types/variant.h" namespace Envoy { namespace Stats { @@ -28,6 +29,14 @@ using TextReadoutOptConstRef = absl::optional; using ScopeSharedPtr = std::shared_ptr; +class DynamicName : public absl::string_view { + public: + DynamicName(absl::string_view s) : absl::string_view(s) {} +}; + +using Element = absl::variant; +using ElementVec = std::vector; + /** * A named scope for stats. Scopes are a grouping of stats that can be acted on as a unit if needed * (for example to free/delete all of them). @@ -67,6 +76,67 @@ class Scope { virtual Counter& counterFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef tags) PURE; + /** + * Creates a counter from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * @param scope The scope in which to create the counter. + * @param elements The vector of mixed string_view and StatName + * @param tags optionally specified tags. + * @return A counter named using the joined elements. + */ + Counter& counterFromElements(const ElementVec& elements, + StatNameTagVectorOptConstRef tags = absl::nullopt) { + return counterFromElementsHelper(elements, tags); + } + virtual Counter& counterFromElementsHelper(const ElementVec& elements, + StatNameTagVectorOptConstRef tags) PURE; + + + /** + * Creates a gauge from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * @param scope The scope in which to create the counter. + * @param elements The vector of mixed string_view and StatName + * @param import_mode Whether hot-restart should accumulate this value. + * @param tags optionally specified tags. + * @return A gauge named using the joined elements. + */ + Gauge& gaugeFromElements(const ElementVec& elements, Gauge::ImportMode import_mode, + StatNameTagVectorOptConstRef tags = absl::nullopt) { + return gaugeFromElementsHelper(elements, import_mode, tags); + } + virtual Gauge& gaugeFromElementsHelper(const ElementVec& elements, Gauge::ImportMode import_mode, + StatNameTagVectorOptConstRef tags) PURE; + + /** + * Creates a histogram from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * @param scope The scope in which to create the counter. + * @param elements The vector of mixed string_view and StatName + * @param unit The unit of measurement. + * @param tags optionally specified tags. + * @return A histogram named using the joined elements. + */ + Histogram& histogramFromElements(const ElementVec& elements, Histogram::Unit unit, + StatNameTagVectorOptConstRef tags) { + return histogramFromElementsHelper(elements, unit, tags); + } + virtual Histogram& histogramFromElementsHelper(const ElementVec& elements, Histogram::Unit unit, + StatNameTagVectorOptConstRef tags) PURE; + + /** * TODO(#6667): this variant is deprecated: use counterFromStatName. * @param name The name, expressed as a string. diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index 256074df9cbfb..c50232a20ac93 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -134,12 +134,23 @@ envoy_cc_library( srcs = ["scope_prefixer.cc"], hdrs = ["scope_prefixer.h"], deps = [ + ":scope_shared_lib", ":symbol_table_lib", ":utility_lib", "//include/envoy/stats:stats_interface", ], ) +envoy_cc_library( + name = "scope_shared_lib", + srcs = ["scope_shared_impl.cc"], + hdrs = ["scope_shared_impl.h"], + deps = [ + ":symbol_table_lib", + "//include/envoy/stats:stats_interface", + ], +) + envoy_cc_library( name = "stat_merger_lib", srcs = ["stat_merger.cc"], diff --git a/source/common/stats/scope_prefixer.h b/source/common/stats/scope_prefixer.h index b7c8743756204..1cb8b6453112c 100644 --- a/source/common/stats/scope_prefixer.h +++ b/source/common/stats/scope_prefixer.h @@ -1,5 +1,6 @@ #include "envoy/stats/scope.h" +#include "common/stats/scope_shared_impl.h" #include "common/stats/symbol_table_impl.h" namespace Envoy { @@ -7,7 +8,7 @@ namespace Stats { // Implements a Scope that delegates to a passed-in scope, prefixing all names // prior to creation. -class ScopePrefixer : public Scope { +class ScopePrefixer : public ScopeSharedImpl { public: ScopePrefixer(absl::string_view prefix, Scope& scope); ScopePrefixer(StatName prefix, Scope& scope); diff --git a/source/common/stats/scope_shared_impl.cc b/source/common/stats/scope_shared_impl.cc new file mode 100644 index 0000000000000..d94549880a007 --- /dev/null +++ b/source/common/stats/scope_shared_impl.cc @@ -0,0 +1,53 @@ +#include "common/stats/scope_shared_impl.h" + +namespace Envoy { +namespace Stats { + +namespace { + +// Helper class for the three Scope::*FromElements implementations to build up +// a joined StatName from a mix of StatName and string_view. +struct ElementVisitor { + ElementVisitor(SymbolTable& symbol_table) : symbol_table_(symbol_table), pool_(symbol_table) {} + + // Overloads provides for absl::visit to call. + void operator()(StatName stat_name) { stat_names_.push_back(stat_name); } + void operator()(absl::string_view name) { stat_names_.push_back(pool_.add(name)); } + + // Generates a StatName from the elements. + StatName makeStatName(const ElementVec& elements) { + for (const Element& element : elements) { + absl::visit(*this, element); + } + joined_ = symbol_table_.join(stat_names_); + return StatName(joined_.get()); + } + + SymbolTable& symbol_table_; + StatNameVec stat_names_; + StatNameDynamicPool pool_; + SymbolTable::StoragePtr joined_; +}; + +} // namespace + +Counter& ScopeSharedImpl::counterFromElementsHelper(const ElementVec& elements, + StatNameTagVectorOptConstRef tags) { + ElementVisitor visitor(symbolTable()); + return counterFromStatNameWithTags(visitor.makeStatName(elements), tags); +} + +Gauge& ScopeSharedImpl::gaugeFromElementsHelper( + const ElementVec& elements, Gauge::ImportMode import_mode, StatNameTagVectorOptConstRef tags) { + ElementVisitor visitor(symbolTable()); + return gaugeFromStatNameWithTags(visitor.makeStatName(elements), tags, import_mode); +} + +Histogram& ScopeSharedImpl::histogramFromElementsHelper( + const ElementVec& elements, Histogram::Unit unit, StatNameTagVectorOptConstRef tags) { + ElementVisitor visitor(symbolTable()); + return histogramFromStatNameWithTags(visitor.makeStatName(elements), tags, unit); +} + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/scope_shared_impl.h b/source/common/stats/scope_shared_impl.h new file mode 100644 index 0000000000000..8ec2eb62148e1 --- /dev/null +++ b/source/common/stats/scope_shared_impl.h @@ -0,0 +1,22 @@ +#pragma once + +#include "envoy/stats/scope.h" +#include "envoy/stats/stats.h" + +#include "common/stats/symbol_table_impl.h" + +namespace Envoy { +namespace Stats { + +class ScopeSharedImpl : public Scope { + public: + Counter& counterFromElementsHelper(const ElementVec& elements, + StatNameTagVectorOptConstRef tags) override; + Gauge& gaugeFromElementsHelper(const ElementVec& elements, Gauge::ImportMode import_mode, + StatNameTagVectorOptConstRef tags) override; + Histogram& histogramFromElementsHelper(const ElementVec& elements, Histogram::Unit unit, + StatNameTagVectorOptConstRef tags) override; +}; + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 135abeb257e40..874349d320907 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -16,6 +16,7 @@ #include "common/stats/null_counter.h" #include "common/stats/null_gauge.h" #include "common/stats/null_text_readout.h" +#include "common/stats/scope_shared_impl.h" #include "common/stats/symbol_table_impl.h" #include "common/stats/utility.h" @@ -135,7 +136,7 @@ using ParentHistogramImplSharedPtr = RefcountPtr; /** * Class used to create ThreadLocalHistogram in the scope. */ -class TlsScope : public Scope { +class TlsScope : public ScopeSharedImpl { public: ~TlsScope() override = default; From 558fd2e4fac7b5b7af7f6b862ed1e095e2be45b7 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 21 Apr 2020 20:35:44 -0400 Subject: [PATCH 11/19] Revert "add ScopeSharedImpl as an alternative leaner API to Utility::*FromElements." This reverts commit 85b57ed38bd19d15f0f22d1c58bc707b656323a0. Signed-off-by: Joshua Marantz --- .bazelversion | 2 +- include/envoy/stats/scope.h | 70 ------------------------ source/common/stats/BUILD | 11 ---- source/common/stats/scope_prefixer.h | 3 +- source/common/stats/scope_shared_impl.cc | 53 ------------------ source/common/stats/scope_shared_impl.h | 22 -------- source/common/stats/thread_local_store.h | 3 +- 7 files changed, 3 insertions(+), 161 deletions(-) delete mode 100644 source/common/stats/scope_shared_impl.cc delete mode 100644 source/common/stats/scope_shared_impl.h diff --git a/.bazelversion b/.bazelversion index 4a36342fcab70..ccbccc3dc6263 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -3.0.0 +2.2.0 diff --git a/include/envoy/stats/scope.h b/include/envoy/stats/scope.h index 0479ee511d2ff..408655bbb8a5c 100644 --- a/include/envoy/stats/scope.h +++ b/include/envoy/stats/scope.h @@ -10,7 +10,6 @@ #include "envoy/stats/tag.h" #include "absl/types/optional.h" -#include "absl/types/variant.h" namespace Envoy { namespace Stats { @@ -29,14 +28,6 @@ using TextReadoutOptConstRef = absl::optional; using ScopeSharedPtr = std::shared_ptr; -class DynamicName : public absl::string_view { - public: - DynamicName(absl::string_view s) : absl::string_view(s) {} -}; - -using Element = absl::variant; -using ElementVec = std::vector; - /** * A named scope for stats. Scopes are a grouping of stats that can be acted on as a unit if needed * (for example to free/delete all of them). @@ -76,67 +67,6 @@ class Scope { virtual Counter& counterFromStatNameWithTags(const StatName& name, StatNameTagVectorOptConstRef tags) PURE; - /** - * Creates a counter from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See - * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens - * for more detail on why symbolic StatNames are preferred when possible. - * - * @param scope The scope in which to create the counter. - * @param elements The vector of mixed string_view and StatName - * @param tags optionally specified tags. - * @return A counter named using the joined elements. - */ - Counter& counterFromElements(const ElementVec& elements, - StatNameTagVectorOptConstRef tags = absl::nullopt) { - return counterFromElementsHelper(elements, tags); - } - virtual Counter& counterFromElementsHelper(const ElementVec& elements, - StatNameTagVectorOptConstRef tags) PURE; - - - /** - * Creates a gauge from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See - * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens - * for more detail on why symbolic StatNames are preferred when possible. - * - * @param scope The scope in which to create the counter. - * @param elements The vector of mixed string_view and StatName - * @param import_mode Whether hot-restart should accumulate this value. - * @param tags optionally specified tags. - * @return A gauge named using the joined elements. - */ - Gauge& gaugeFromElements(const ElementVec& elements, Gauge::ImportMode import_mode, - StatNameTagVectorOptConstRef tags = absl::nullopt) { - return gaugeFromElementsHelper(elements, import_mode, tags); - } - virtual Gauge& gaugeFromElementsHelper(const ElementVec& elements, Gauge::ImportMode import_mode, - StatNameTagVectorOptConstRef tags) PURE; - - /** - * Creates a histogram from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See - * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens - * for more detail on why symbolic StatNames are preferred when possible. - * - * @param scope The scope in which to create the counter. - * @param elements The vector of mixed string_view and StatName - * @param unit The unit of measurement. - * @param tags optionally specified tags. - * @return A histogram named using the joined elements. - */ - Histogram& histogramFromElements(const ElementVec& elements, Histogram::Unit unit, - StatNameTagVectorOptConstRef tags) { - return histogramFromElementsHelper(elements, unit, tags); - } - virtual Histogram& histogramFromElementsHelper(const ElementVec& elements, Histogram::Unit unit, - StatNameTagVectorOptConstRef tags) PURE; - - /** * TODO(#6667): this variant is deprecated: use counterFromStatName. * @param name The name, expressed as a string. diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index c50232a20ac93..256074df9cbfb 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -134,23 +134,12 @@ envoy_cc_library( srcs = ["scope_prefixer.cc"], hdrs = ["scope_prefixer.h"], deps = [ - ":scope_shared_lib", ":symbol_table_lib", ":utility_lib", "//include/envoy/stats:stats_interface", ], ) -envoy_cc_library( - name = "scope_shared_lib", - srcs = ["scope_shared_impl.cc"], - hdrs = ["scope_shared_impl.h"], - deps = [ - ":symbol_table_lib", - "//include/envoy/stats:stats_interface", - ], -) - envoy_cc_library( name = "stat_merger_lib", srcs = ["stat_merger.cc"], diff --git a/source/common/stats/scope_prefixer.h b/source/common/stats/scope_prefixer.h index 1cb8b6453112c..b7c8743756204 100644 --- a/source/common/stats/scope_prefixer.h +++ b/source/common/stats/scope_prefixer.h @@ -1,6 +1,5 @@ #include "envoy/stats/scope.h" -#include "common/stats/scope_shared_impl.h" #include "common/stats/symbol_table_impl.h" namespace Envoy { @@ -8,7 +7,7 @@ namespace Stats { // Implements a Scope that delegates to a passed-in scope, prefixing all names // prior to creation. -class ScopePrefixer : public ScopeSharedImpl { +class ScopePrefixer : public Scope { public: ScopePrefixer(absl::string_view prefix, Scope& scope); ScopePrefixer(StatName prefix, Scope& scope); diff --git a/source/common/stats/scope_shared_impl.cc b/source/common/stats/scope_shared_impl.cc deleted file mode 100644 index d94549880a007..0000000000000 --- a/source/common/stats/scope_shared_impl.cc +++ /dev/null @@ -1,53 +0,0 @@ -#include "common/stats/scope_shared_impl.h" - -namespace Envoy { -namespace Stats { - -namespace { - -// Helper class for the three Scope::*FromElements implementations to build up -// a joined StatName from a mix of StatName and string_view. -struct ElementVisitor { - ElementVisitor(SymbolTable& symbol_table) : symbol_table_(symbol_table), pool_(symbol_table) {} - - // Overloads provides for absl::visit to call. - void operator()(StatName stat_name) { stat_names_.push_back(stat_name); } - void operator()(absl::string_view name) { stat_names_.push_back(pool_.add(name)); } - - // Generates a StatName from the elements. - StatName makeStatName(const ElementVec& elements) { - for (const Element& element : elements) { - absl::visit(*this, element); - } - joined_ = symbol_table_.join(stat_names_); - return StatName(joined_.get()); - } - - SymbolTable& symbol_table_; - StatNameVec stat_names_; - StatNameDynamicPool pool_; - SymbolTable::StoragePtr joined_; -}; - -} // namespace - -Counter& ScopeSharedImpl::counterFromElementsHelper(const ElementVec& elements, - StatNameTagVectorOptConstRef tags) { - ElementVisitor visitor(symbolTable()); - return counterFromStatNameWithTags(visitor.makeStatName(elements), tags); -} - -Gauge& ScopeSharedImpl::gaugeFromElementsHelper( - const ElementVec& elements, Gauge::ImportMode import_mode, StatNameTagVectorOptConstRef tags) { - ElementVisitor visitor(symbolTable()); - return gaugeFromStatNameWithTags(visitor.makeStatName(elements), tags, import_mode); -} - -Histogram& ScopeSharedImpl::histogramFromElementsHelper( - const ElementVec& elements, Histogram::Unit unit, StatNameTagVectorOptConstRef tags) { - ElementVisitor visitor(symbolTable()); - return histogramFromStatNameWithTags(visitor.makeStatName(elements), tags, unit); -} - -} // namespace Stats -} // namespace Envoy diff --git a/source/common/stats/scope_shared_impl.h b/source/common/stats/scope_shared_impl.h deleted file mode 100644 index 8ec2eb62148e1..0000000000000 --- a/source/common/stats/scope_shared_impl.h +++ /dev/null @@ -1,22 +0,0 @@ -#pragma once - -#include "envoy/stats/scope.h" -#include "envoy/stats/stats.h" - -#include "common/stats/symbol_table_impl.h" - -namespace Envoy { -namespace Stats { - -class ScopeSharedImpl : public Scope { - public: - Counter& counterFromElementsHelper(const ElementVec& elements, - StatNameTagVectorOptConstRef tags) override; - Gauge& gaugeFromElementsHelper(const ElementVec& elements, Gauge::ImportMode import_mode, - StatNameTagVectorOptConstRef tags) override; - Histogram& histogramFromElementsHelper(const ElementVec& elements, Histogram::Unit unit, - StatNameTagVectorOptConstRef tags) override; -}; - -} // namespace Stats -} // namespace Envoy diff --git a/source/common/stats/thread_local_store.h b/source/common/stats/thread_local_store.h index 874349d320907..135abeb257e40 100644 --- a/source/common/stats/thread_local_store.h +++ b/source/common/stats/thread_local_store.h @@ -16,7 +16,6 @@ #include "common/stats/null_counter.h" #include "common/stats/null_gauge.h" #include "common/stats/null_text_readout.h" -#include "common/stats/scope_shared_impl.h" #include "common/stats/symbol_table_impl.h" #include "common/stats/utility.h" @@ -136,7 +135,7 @@ using ParentHistogramImplSharedPtr = RefcountPtr; /** * Class used to create ThreadLocalHistogram in the scope. */ -class TlsScope : public ScopeSharedImpl { +class TlsScope : public Scope { public: ~TlsScope() override = default; From fb31f208e4cb6fbae8024c609950d27535c5665e Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 21 Apr 2020 20:51:01 -0400 Subject: [PATCH 12/19] name cleanups. Signed-off-by: Joshua Marantz --- .bazelversion | 2 +- source/common/stats/utility.cc | 4 +-- source/common/stats/utility.h | 29 ++++++++++++------- .../filters/http/dynamo/dynamo_stats.cc | 10 +++---- .../filters/http/dynamo/dynamo_stats.h | 6 ++-- .../network/mongo_proxy/mongo_stats.cc | 8 ++--- .../filters/network/mongo_proxy/mongo_stats.h | 6 ++-- .../filters/network/mongo_proxy/proxy.cc | 18 ++++++------ .../filters/network/mongo_proxy/proxy.h | 4 +-- test/common/stats/utility_test.cc | 14 ++++----- 10 files changed, 54 insertions(+), 47 deletions(-) diff --git a/.bazelversion b/.bazelversion index ccbccc3dc6263..4a36342fcab70 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -2.2.0 +3.0.0 diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index 2195dda90f981..b0eb4788e5187 100644 --- a/source/common/stats/utility.cc +++ b/source/common/stats/utility.cc @@ -47,8 +47,8 @@ struct ElementVisitor { void operator()(absl::string_view name) { stat_names_.push_back(pool_.add(name)); } // Generates a StatName from the elements. - StatName makeStatName(const Utility::ElementVec& elements) { - for (const Utility::Element& element : elements) { + StatName makeStatName(const ElementVec& elements) { + for (const Element& element : elements) { absl::visit(*this, element); } joined_ = symbol_table_.join(stat_names_); diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 65a8f76f3a55f..261e96ce210dc 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -13,6 +13,24 @@ namespace Envoy { namespace Stats { +/** + * Represents a stat name token, using either a StatName or a string_view, + * which will be treated as a dynamic string. We subclass string_view simply + * to make it a bit more explicit when we are creating a dynamic stat name, + * since those are expensive. + */ +class DynamicName : public absl::string_view { + public: + DynamicName(absl::string_view s) : absl::string_view(s) {} +}; + +using Element = absl::variant; +using ElementVec = std::vector; + +//class Element : public absl::variant> { + //}; + //using ElementVec = Element; + /** * Common stats utility routines. */ @@ -36,17 +54,6 @@ class Utility { */ static absl::optional findTag(const Metric& metric, StatName find_tag_name); - // Represents a stat name token, using either a StatName or a string_view, - // which will be treated as a dynamic string. We subclass string_view simply - // to make it a bit more explicit when we are creating a dynamic stat name, - // since those are expensive. - class Dynamic : public absl::string_view { - public: - Dynamic(absl::string_view s) : absl::string_view(s) {} - }; - using Element = absl::variant; - using ElementVec = std::vector; - /** * Creates a counter from a vector of tokens which are used to create the * name. The tokens can be specified as string_view or StatName. For diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.cc b/source/extensions/filters/http/dynamo/dynamo_stats.cc index 57e846ffd8ddc..11cbdfe48cd48 100644 --- a/source/extensions/filters/http/dynamo/dynamo_stats.cc +++ b/source/extensions/filters/http/dynamo/dynamo_stats.cc @@ -46,19 +46,19 @@ DynamoStats::DynamoStats(Stats::Scope& scope, const std::string& prefix) stat_name_set_->rememberBuiltins({"operation", "table"}); } -Stats::Utility::ElementVec DynamoStats::addPrefix(const Stats::Utility::ElementVec& names) { - Stats::Utility::ElementVec names_with_prefix; +Stats::ElementVec DynamoStats::addPrefix(const Stats::ElementVec& names) { + Stats::ElementVec names_with_prefix; names_with_prefix.reserve(1 + names.size()); names_with_prefix.push_back(prefix_); names_with_prefix.insert(names_with_prefix.end(), names.begin(), names.end()); return names_with_prefix; } -void DynamoStats::incCounter(const Stats::Utility::ElementVec& names) { +void DynamoStats::incCounter(const Stats::ElementVec& names) { Stats::Utility::counterFromElements(scope_, addPrefix(names)).inc(); } -void DynamoStats::recordHistogram(const Stats::Utility::ElementVec& names, Stats::Histogram::Unit unit, +void DynamoStats::recordHistogram(const Stats::ElementVec& names, Stats::Histogram::Unit unit, uint64_t value) { Stats::Utility::histogramFromElements(scope_, addPrefix(names), unit).recordValue(value); } @@ -71,7 +71,7 @@ Stats::Counter& DynamoStats::buildPartitionStatCounter(const std::string& table_ Stats::StatNameDynamicPool dynamic(scope_.symbolTable()); const Stats::StatName partition = dynamic.add(absl::StrCat("__partition_id=", id_last_7)); return Stats::Utility::counterFromElements(scope_, addPrefix({ - table_, Stats::Utility::Dynamic(table_name), capacity_, + table_, Stats::DynamicName(table_name), capacity_, getBuiltin(operation, unknown_operation_), partition})); } diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.h b/source/extensions/filters/http/dynamo/dynamo_stats.h index c1928046aae2b..6871e05b435ae 100644 --- a/source/extensions/filters/http/dynamo/dynamo_stats.h +++ b/source/extensions/filters/http/dynamo/dynamo_stats.h @@ -17,8 +17,8 @@ class DynamoStats { public: DynamoStats(Stats::Scope& scope, const std::string& prefix); - void incCounter(const Stats::Utility::ElementVec& names); - void recordHistogram(const Stats::Utility::ElementVec& names, Stats::Histogram::Unit unit, + void incCounter(const Stats::ElementVec& names); + void recordHistogram(const Stats::ElementVec& names, Stats::Histogram::Unit unit, uint64_t value); /** @@ -43,7 +43,7 @@ class DynamoStats { Stats::SymbolTable& symbolTable() { return scope_.symbolTable(); } private: - Stats::Utility::ElementVec addPrefix(const Stats::Utility::ElementVec& names); + Stats::ElementVec addPrefix(const Stats::ElementVec& names); Stats::Scope& scope_; Stats::StatNameSetPtr stat_name_set_; diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc index bf1207774181b..2ce2a82d85520 100644 --- a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc @@ -31,19 +31,19 @@ MongoStats::MongoStats(Stats::Scope& scope, absl::string_view prefix) stat_name_set_->rememberBuiltins({"insert", "query", "update", "delete"}); } -Stats::Utility::ElementVec MongoStats::addPrefix(const Stats::Utility::ElementVec& names) { - Stats::Utility::ElementVec names_with_prefix; +Stats::ElementVec MongoStats::addPrefix(const Stats::ElementVec& names) { + Stats::ElementVec names_with_prefix; names_with_prefix.reserve(1 + names.size()); names_with_prefix.push_back(prefix_); names_with_prefix.insert(names_with_prefix.end(), names.begin(), names.end()); return names_with_prefix; } -void MongoStats::incCounter(const Stats::Utility::ElementVec& names) { +void MongoStats::incCounter(const Stats::ElementVec& names) { Stats::Utility::counterFromElements(scope_, addPrefix(names)).inc(); } -void MongoStats::recordHistogram(const Stats::Utility::ElementVec& names, +void MongoStats::recordHistogram(const Stats::ElementVec& names, Stats::Histogram::Unit unit, uint64_t sample) { Stats::Utility::histogramFromElements(scope_, addPrefix(names), unit).recordValue(sample); } diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.h b/source/extensions/filters/network/mongo_proxy/mongo_stats.h index 0072f5dd0f45b..3571c19bbca2a 100644 --- a/source/extensions/filters/network/mongo_proxy/mongo_stats.h +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.h @@ -18,8 +18,8 @@ class MongoStats { public: MongoStats(Stats::Scope& scope, absl::string_view prefix); - void incCounter(const Stats::Utility::ElementVec& names); - void recordHistogram(const Stats::Utility::ElementVec& names, Stats::Histogram::Unit unit, + void incCounter(const Stats::ElementVec& names); + void recordHistogram(const Stats::ElementVec& names, Stats::Histogram::Unit unit, uint64_t sample); /** @@ -35,7 +35,7 @@ class MongoStats { Stats::SymbolTable& symbolTable() { return scope_.symbolTable(); } private: - Stats::Utility::ElementVec addPrefix(const Stats::Utility::ElementVec& names); + Stats::ElementVec addPrefix(const Stats::ElementVec& names); Stats::Scope& scope_; Stats::StatNameSetPtr stat_name_set_; diff --git a/source/extensions/filters/network/mongo_proxy/proxy.cc b/source/extensions/filters/network/mongo_proxy/proxy.cc index 46d90cd7fc237..69e8805edabac 100644 --- a/source/extensions/filters/network/mongo_proxy/proxy.cc +++ b/source/extensions/filters/network/mongo_proxy/proxy.cc @@ -152,16 +152,16 @@ void ProxyFilter::decodeQuery(QueryMessagePtr&& message) { } else { // Normal query, get stats on a per collection basis first. QueryMessageInfo::QueryType query_type = active_query->query_info_.type(); - Stats::Utility::ElementVec names; + Stats::ElementVec names; names.reserve(6); // 2 entries are added by chargeQueryStats(). names.push_back(mongo_stats_->collection_); - names.push_back(Stats::Utility::Dynamic(active_query->query_info_.collection())); + names.push_back(Stats::DynamicName(active_query->query_info_.collection())); chargeQueryStats(names, query_type); // Callsite stats if we have it. if (!active_query->query_info_.callsite().empty()) { names.push_back(mongo_stats_->callsite_); - names.push_back(Stats::Utility::Dynamic(active_query->query_info_.callsite())); + names.push_back(Stats::DynamicName(active_query->query_info_.callsite())); chargeQueryStats(names, query_type); } @@ -179,7 +179,7 @@ void ProxyFilter::decodeQuery(QueryMessagePtr&& message) { active_query_list_.emplace_back(std::move(active_query)); } -void ProxyFilter::chargeQueryStats(Stats::Utility::ElementVec& names, +void ProxyFilter::chargeQueryStats(Stats::ElementVec& names, QueryMessageInfo::QueryType query_type) { // names come in containing {"collection", collection}. Report stats for 1 or // 2 variations on this array, and then return with the array in the same @@ -223,14 +223,14 @@ void ProxyFilter::decodeReply(ReplyMessagePtr&& message) { } if (!active_query.query_info_.command().empty()) { - Stats::Utility::ElementVec names{mongo_stats_->cmd_, + Stats::ElementVec names{mongo_stats_->cmd_, mongo_stats_->getBuiltin(active_query.query_info_.command(), mongo_stats_->unknown_command_)}; chargeReplyStats(active_query, names, *message); } else { // Collection stats first. - Stats::Utility::ElementVec names{mongo_stats_->collection_, - Stats::Utility::Dynamic(active_query.query_info_.collection()), + Stats::ElementVec names{mongo_stats_->collection_, + Stats::DynamicName(active_query.query_info_.collection()), mongo_stats_->query_}; chargeReplyStats(active_query, names, *message); @@ -240,7 +240,7 @@ void ProxyFilter::decodeReply(ReplyMessagePtr&& message) { // to mutate the array to {"collection", collection, "callsite", callsite, "query"}. ASSERT(names.size() == 3); names.back() = mongo_stats_->callsite_; // Replaces "query". - names.push_back(Stats::Utility::Dynamic(active_query.query_info_.callsite())); + names.push_back(Stats::DynamicName(active_query.query_info_.callsite())); names.push_back(mongo_stats_->query_); chargeReplyStats(active_query, names, *message); } @@ -290,7 +290,7 @@ void ProxyFilter::onDrainClose() { read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); } -void ProxyFilter::chargeReplyStats(ActiveQuery& active_query, Stats::Utility::ElementVec& names, +void ProxyFilter::chargeReplyStats(ActiveQuery& active_query, Stats::ElementVec& names, const ReplyMessage& message) { uint64_t reply_documents_byte_size = 0; for (const Bson::DocumentSharedPtr& document : message.documents()) { diff --git a/source/extensions/filters/network/mongo_proxy/proxy.h b/source/extensions/filters/network/mongo_proxy/proxy.h index 0a5ba93dc90b7..c54308f1ae386 100644 --- a/source/extensions/filters/network/mongo_proxy/proxy.h +++ b/source/extensions/filters/network/mongo_proxy/proxy.h @@ -167,12 +167,12 @@ class ProxyFilter : public Network::Filter, // Increment counters related to queries. 'names' is passed by non-const // reference so the implementation can mutate it without copying, though it // always restores it to its prior state prior to return. - void chargeQueryStats(Stats::Utility::ElementVec& names, QueryMessageInfo::QueryType query_type); + void chargeQueryStats(Stats::ElementVec& names, QueryMessageInfo::QueryType query_type); // Add samples to histograms related to replies. 'names' is passed by // non-const reference so the implementation can mutate it without copying, // though it always restores it to its prior state prior to return. - void chargeReplyStats(ActiveQuery& active_query, Stats::Utility::ElementVec& names, + void chargeReplyStats(ActiveQuery& active_query, Stats::ElementVec& names, const ReplyMessage& message); void doDecode(Buffer::Instance& buffer); diff --git a/test/common/stats/utility_test.cc b/test/common/stats/utility_test.cc index 782ffb20aab42..9d9d4da1308d7 100644 --- a/test/common/stats/utility_test.cc +++ b/test/common/stats/utility_test.cc @@ -38,30 +38,30 @@ class StatsUtilityTest : public testing::Test { TEST_F(StatsUtilityTest, Counters) { ScopePtr scope = store_->createScope("scope."); Counter& c1 = - Utility::counterFromElements(*scope, {Utility::Dynamic("a"), Utility::Dynamic("b")}); + Utility::counterFromElements(*scope, {DynamicName("a"), DynamicName("b")}); EXPECT_EQ("scope.a.b", c1.name()); StatName token = pool_.add("token"); Counter& c2 = - Utility::counterFromElements(*scope, {Utility::Dynamic("a"), token, Utility::Dynamic("b")}); + Utility::counterFromElements(*scope, {DynamicName("a"), token, DynamicName("b")}); EXPECT_EQ("scope.a.token.b", c2.name()); StatName suffix = pool_.add("suffix"); Counter& c3 = Utility::counterFromElements(*scope, {token, suffix}); EXPECT_EQ("scope.token.suffix", c3.name()); Counter& ctags = Utility::counterFromElements( - *scope, {Utility::Dynamic("x"), token, Utility::Dynamic("y")}, tags_); + *scope, {DynamicName("x"), token, DynamicName("y")}, tags_); EXPECT_EQ("scope.x.token.y.tag1.value1.tag2.value2", ctags.name()); } TEST_F(StatsUtilityTest, Gauges) { ScopePtr scope = store_->createScope("scope."); - Gauge& g1 = Utility::gaugeFromElements(*scope, {Utility::Dynamic("a"), Utility::Dynamic("b")}, + Gauge& g1 = Utility::gaugeFromElements(*scope, {DynamicName("a"), DynamicName("b")}, Gauge::ImportMode::NeverImport); EXPECT_EQ("scope.a.b", g1.name()); EXPECT_EQ(Gauge::ImportMode::NeverImport, g1.importMode()); StatName token = pool_.add("token"); Gauge& g2 = Utility::gaugeFromElements( - *scope, {Utility::Dynamic("a"), token, Utility::Dynamic("b")}, Gauge::ImportMode::Accumulate); + *scope, {DynamicName("a"), token, DynamicName("b")}, Gauge::ImportMode::Accumulate); EXPECT_EQ("scope.a.token.b", g2.name()); EXPECT_EQ(Gauge::ImportMode::Accumulate, g2.importMode()); StatName suffix = pool_.add("suffix"); @@ -72,12 +72,12 @@ TEST_F(StatsUtilityTest, Gauges) { TEST_F(StatsUtilityTest, Histograms) { ScopePtr scope = store_->createScope("scope."); Histogram& h1 = Utility::histogramFromElements( - *scope, {Utility::Dynamic("a"), Utility::Dynamic("b")}, Histogram::Unit::Milliseconds); + *scope, {DynamicName("a"), DynamicName("b")}, Histogram::Unit::Milliseconds); EXPECT_EQ("scope.a.b", h1.name()); EXPECT_EQ(Histogram::Unit::Milliseconds, h1.unit()); StatName token = pool_.add("token"); Histogram& h2 = Utility::histogramFromElements( - *scope, {Utility::Dynamic("a"), token, Utility::Dynamic("b")}, Histogram::Unit::Microseconds); + *scope, {DynamicName("a"), token, DynamicName("b")}, Histogram::Unit::Microseconds); EXPECT_EQ("scope.a.token.b", h2.name()); EXPECT_EQ(Histogram::Unit::Microseconds, h2.unit()); StatName suffix = pool_.add("suffix"); From c285307088bc82fa63a6983842e5dddea486ae99 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 21 Apr 2020 21:28:41 -0400 Subject: [PATCH 13/19] format, and convert grpc to new pattern. Signed-off-by: Joshua Marantz --- source/common/grpc/BUILD | 1 + source/common/grpc/context_impl.cc | 63 ++++++------------- source/common/grpc/context_impl.h | 5 +- source/common/stats/utility.h | 6 +- .../filters/http/dynamo/dynamo_stats.cc | 6 +- .../filters/http/dynamo/dynamo_stats.h | 3 +- .../network/mongo_proxy/mongo_stats.cc | 4 +- .../filters/network/mongo_proxy/proxy.cc | 8 +-- test/common/stats/utility_test.cc | 18 +++--- 9 files changed, 42 insertions(+), 72 deletions(-) diff --git a/source/common/grpc/BUILD b/source/common/grpc/BUILD index 29f31e66d4445..94ab9c403abfc 100644 --- a/source/common/grpc/BUILD +++ b/source/common/grpc/BUILD @@ -114,6 +114,7 @@ envoy_cc_library( "//include/envoy/stats:stats_interface", "//source/common/common:hash_lib", "//source/common/stats:symbol_table_lib", + "//source/common/stats:utility_lib", ], ) diff --git a/source/common/grpc/context_impl.cc b/source/common/grpc/context_impl.cc index f612f71cc0740..bcb6ae2830f7d 100644 --- a/source/common/grpc/context_impl.cc +++ b/source/common/grpc/context_impl.cc @@ -35,17 +35,13 @@ Stats::StatName ContextImpl::makeDynamicStatName(absl::string_view name) { } // Gets the stat prefix and underlying storage, depending on whether request_names is empty -std::pair -ContextImpl::getPrefix(Protocol protocol, const absl::optional& request_names) { +Stats::ElementVec ContextImpl::getPrefix(Protocol protocol, + const absl::optional& request_names) { const Stats::StatName protocolName = protocolStatName(protocol); if (request_names) { - Stats::SymbolTable::StoragePtr prefix_storage = - symbol_table_.join({protocolName, request_names->service_, request_names->method_}); - Stats::StatName prefix = Stats::StatName(prefix_storage.get()); - return {prefix, std::move(prefix_storage)}; - } else { - return {protocolName, nullptr}; + return Stats::ElementVec{protocolName, request_names->service_, request_names->method_}; } + return Stats::ElementVec{protocolName}; } void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, Protocol protocol, @@ -70,15 +66,11 @@ void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, Protocol prot void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, Protocol protocol, const absl::optional& request_names, bool success) { - auto prefix_and_storage = getPrefix(protocol, request_names); - Stats::StatName prefix = prefix_and_storage.first; - - const Stats::SymbolTable::StoragePtr status = - symbol_table_.join({prefix, successStatName(success)}); - const Stats::SymbolTable::StoragePtr total = symbol_table_.join({prefix, total_}); - - cluster.statsScope().counterFromStatName(Stats::StatName(status.get())).inc(); - cluster.statsScope().counterFromStatName(Stats::StatName(total.get())).inc(); + Stats::ElementVec elements = getPrefix(protocol, request_names); + elements.push_back(successStatName(success)); + Stats::Utility::counterFromElements(cluster.statsScope(), elements).inc(); + elements.back() = total_; + Stats::Utility::counterFromElements(cluster.statsScope(), elements).inc(); } void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, @@ -89,43 +81,26 @@ void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, void ContextImpl::chargeRequestMessageStat(const Upstream::ClusterInfo& cluster, const absl::optional& request_names, uint64_t amount) { - auto prefix_and_storage = getPrefix(Protocol::Grpc, request_names); - Stats::StatName prefix = prefix_and_storage.first; - - const Stats::SymbolTable::StoragePtr request_message_count = - symbol_table_.join({prefix, request_message_count_}); - - cluster.statsScope() - .counterFromStatName(Stats::StatName(request_message_count.get())) - .add(amount); + Stats::ElementVec elements = getPrefix(Protocol::Grpc, request_names); + elements.push_back(request_message_count_); + Stats::Utility::counterFromElements(cluster.statsScope(), elements).add(amount); } void ContextImpl::chargeResponseMessageStat(const Upstream::ClusterInfo& cluster, const absl::optional& request_names, uint64_t amount) { - auto prefix_and_storage = getPrefix(Protocol::Grpc, request_names); - Stats::StatName prefix = prefix_and_storage.first; - - const Stats::SymbolTable::StoragePtr response_message_count = - symbol_table_.join({prefix, response_message_count_}); - - cluster.statsScope() - .counterFromStatName(Stats::StatName(response_message_count.get())) - .add(amount); + Stats::ElementVec elements = getPrefix(Protocol::Grpc, request_names); + elements.push_back(response_message_count_); + Stats::Utility::counterFromElements(cluster.statsScope(), elements).add(amount); } void ContextImpl::chargeUpstreamStat(const Upstream::ClusterInfo& cluster, const absl::optional& request_names, std::chrono::milliseconds duration) { - auto prefix_and_storage = getPrefix(Protocol::Grpc, request_names); - Stats::StatName prefix = prefix_and_storage.first; - - const Stats::SymbolTable::StoragePtr upstream_rq_time = - symbol_table_.join({prefix, upstream_rq_time_}); - - cluster.statsScope() - .histogramFromStatName(Stats::StatName(upstream_rq_time.get()), - Stats::Histogram::Unit::Milliseconds) + Stats::ElementVec elements = getPrefix(Protocol::Grpc, request_names); + elements.push_back(upstream_rq_time_); + Stats::Utility::histogramFromElements(cluster.statsScope(), elements, + Stats::Histogram::Unit::Milliseconds) .recordValue(duration.count()); } diff --git a/source/common/grpc/context_impl.h b/source/common/grpc/context_impl.h index 9d3ddc731458b..c98a4d460b4ae 100644 --- a/source/common/grpc/context_impl.h +++ b/source/common/grpc/context_impl.h @@ -9,6 +9,7 @@ #include "common/common/hash.h" #include "common/grpc/stat_names.h" #include "common/stats/symbol_table_impl.h" +#include "common/stats/utility.h" #include "absl/types/optional.h" @@ -71,8 +72,8 @@ class ContextImpl : public Context { // or not. // Prefix will be "" if request_names is empty, or // ".." if it is not empty. - std::pair - getPrefix(Protocol protocol, const absl::optional& request_names); + Stats::ElementVec getPrefix(Protocol protocol, + const absl::optional& request_names); Stats::SymbolTable& symbol_table_; mutable Thread::MutexBasicLockable mutex_; diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 261e96ce210dc..389ea1a539ea8 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -20,17 +20,13 @@ namespace Stats { * since those are expensive. */ class DynamicName : public absl::string_view { - public: +public: DynamicName(absl::string_view s) : absl::string_view(s) {} }; using Element = absl::variant; using ElementVec = std::vector; -//class Element : public absl::variant> { - //}; - //using ElementVec = Element; - /** * Common stats utility routines. */ diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.cc b/source/extensions/filters/http/dynamo/dynamo_stats.cc index 11cbdfe48cd48..14948b20f447d 100644 --- a/source/extensions/filters/http/dynamo/dynamo_stats.cc +++ b/source/extensions/filters/http/dynamo/dynamo_stats.cc @@ -70,9 +70,9 @@ Stats::Counter& DynamoStats::buildPartitionStatCounter(const std::string& table_ absl::string_view id_last_7 = absl::string_view(partition_id).substr(partition_id.size() - 7); Stats::StatNameDynamicPool dynamic(scope_.symbolTable()); const Stats::StatName partition = dynamic.add(absl::StrCat("__partition_id=", id_last_7)); - return Stats::Utility::counterFromElements(scope_, addPrefix({ - table_, Stats::DynamicName(table_name), capacity_, - getBuiltin(operation, unknown_operation_), partition})); + return Stats::Utility::counterFromElements( + scope_, addPrefix({table_, Stats::DynamicName(table_name), capacity_, + getBuiltin(operation, unknown_operation_), partition})); } size_t DynamoStats::groupIndex(uint64_t status) { diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.h b/source/extensions/filters/http/dynamo/dynamo_stats.h index 6871e05b435ae..48399e4f4d23b 100644 --- a/source/extensions/filters/http/dynamo/dynamo_stats.h +++ b/source/extensions/filters/http/dynamo/dynamo_stats.h @@ -18,8 +18,7 @@ class DynamoStats { DynamoStats(Stats::Scope& scope, const std::string& prefix); void incCounter(const Stats::ElementVec& names); - void recordHistogram(const Stats::ElementVec& names, Stats::Histogram::Unit unit, - uint64_t value); + void recordHistogram(const Stats::ElementVec& names, Stats::Histogram::Unit unit, uint64_t value); /** * Creates the partition id stats string. The stats format is diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc index 2ce2a82d85520..6059b461f94c4 100644 --- a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc @@ -43,8 +43,8 @@ void MongoStats::incCounter(const Stats::ElementVec& names) { Stats::Utility::counterFromElements(scope_, addPrefix(names)).inc(); } -void MongoStats::recordHistogram(const Stats::ElementVec& names, - Stats::Histogram::Unit unit, uint64_t sample) { +void MongoStats::recordHistogram(const Stats::ElementVec& names, Stats::Histogram::Unit unit, + uint64_t sample) { Stats::Utility::histogramFromElements(scope_, addPrefix(names), unit).recordValue(sample); } diff --git a/source/extensions/filters/network/mongo_proxy/proxy.cc b/source/extensions/filters/network/mongo_proxy/proxy.cc index 69e8805edabac..c764c618df1b3 100644 --- a/source/extensions/filters/network/mongo_proxy/proxy.cc +++ b/source/extensions/filters/network/mongo_proxy/proxy.cc @@ -224,14 +224,14 @@ void ProxyFilter::decodeReply(ReplyMessagePtr&& message) { if (!active_query.query_info_.command().empty()) { Stats::ElementVec names{mongo_stats_->cmd_, - mongo_stats_->getBuiltin(active_query.query_info_.command(), - mongo_stats_->unknown_command_)}; + mongo_stats_->getBuiltin(active_query.query_info_.command(), + mongo_stats_->unknown_command_)}; chargeReplyStats(active_query, names, *message); } else { // Collection stats first. Stats::ElementVec names{mongo_stats_->collection_, - Stats::DynamicName(active_query.query_info_.collection()), - mongo_stats_->query_}; + Stats::DynamicName(active_query.query_info_.collection()), + mongo_stats_->query_}; chargeReplyStats(active_query, names, *message); // Callsite stats if we have it. diff --git a/test/common/stats/utility_test.cc b/test/common/stats/utility_test.cc index 9d9d4da1308d7..6f62778dc454d 100644 --- a/test/common/stats/utility_test.cc +++ b/test/common/stats/utility_test.cc @@ -37,19 +37,17 @@ class StatsUtilityTest : public testing::Test { TEST_F(StatsUtilityTest, Counters) { ScopePtr scope = store_->createScope("scope."); - Counter& c1 = - Utility::counterFromElements(*scope, {DynamicName("a"), DynamicName("b")}); + Counter& c1 = Utility::counterFromElements(*scope, {DynamicName("a"), DynamicName("b")}); EXPECT_EQ("scope.a.b", c1.name()); StatName token = pool_.add("token"); - Counter& c2 = - Utility::counterFromElements(*scope, {DynamicName("a"), token, DynamicName("b")}); + Counter& c2 = Utility::counterFromElements(*scope, {DynamicName("a"), token, DynamicName("b")}); EXPECT_EQ("scope.a.token.b", c2.name()); StatName suffix = pool_.add("suffix"); Counter& c3 = Utility::counterFromElements(*scope, {token, suffix}); EXPECT_EQ("scope.token.suffix", c3.name()); - Counter& ctags = Utility::counterFromElements( - *scope, {DynamicName("x"), token, DynamicName("y")}, tags_); + Counter& ctags = + Utility::counterFromElements(*scope, {DynamicName("x"), token, DynamicName("y")}, tags_); EXPECT_EQ("scope.x.token.y.tag1.value1.tag2.value2", ctags.name()); } @@ -60,8 +58,8 @@ TEST_F(StatsUtilityTest, Gauges) { EXPECT_EQ("scope.a.b", g1.name()); EXPECT_EQ(Gauge::ImportMode::NeverImport, g1.importMode()); StatName token = pool_.add("token"); - Gauge& g2 = Utility::gaugeFromElements( - *scope, {DynamicName("a"), token, DynamicName("b")}, Gauge::ImportMode::Accumulate); + Gauge& g2 = Utility::gaugeFromElements(*scope, {DynamicName("a"), token, DynamicName("b")}, + Gauge::ImportMode::Accumulate); EXPECT_EQ("scope.a.token.b", g2.name()); EXPECT_EQ(Gauge::ImportMode::Accumulate, g2.importMode()); StatName suffix = pool_.add("suffix"); @@ -71,8 +69,8 @@ TEST_F(StatsUtilityTest, Gauges) { TEST_F(StatsUtilityTest, Histograms) { ScopePtr scope = store_->createScope("scope."); - Histogram& h1 = Utility::histogramFromElements( - *scope, {DynamicName("a"), DynamicName("b")}, Histogram::Unit::Milliseconds); + Histogram& h1 = Utility::histogramFromElements(*scope, {DynamicName("a"), DynamicName("b")}, + Histogram::Unit::Milliseconds); EXPECT_EQ("scope.a.b", h1.name()); EXPECT_EQ(Histogram::Unit::Milliseconds, h1.unit()); StatName token = pool_.add("token"); From e418417dace8ab5286b87c0ae1e8cfb7ec1b193c Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 22 Apr 2020 21:28:52 -0400 Subject: [PATCH 14/19] add variants that accept only StatName, but are a bit leaner in implementation. Signed-off-by: Joshua Marantz --- .bazelversion | 2 +- source/common/grpc/context_impl.cc | 44 +++-------- source/common/grpc/context_impl.h | 17 +--- source/common/stats/utility.cc | 20 +++++ source/common/stats/utility.h | 79 ++++++++++++++++++- .../filters/http/dynamo/dynamo_stats.cc | 8 +- .../filters/http/fault/fault_filter.cc | 3 +- .../common/redis/redis_command_stats.cc | 14 ++-- .../filters/network/zookeeper_proxy/filter.cc | 4 +- test/common/grpc/context_impl_test.cc | 4 +- test/common/stats/utility_test.cc | 8 ++ 11 files changed, 137 insertions(+), 66 deletions(-) diff --git a/.bazelversion b/.bazelversion index 4a36342fcab70..ccbccc3dc6263 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -3.0.0 +2.2.0 diff --git a/source/common/grpc/context_impl.cc b/source/common/grpc/context_impl.cc index bcb6ae2830f7d..b86b6e960e8e4 100644 --- a/source/common/grpc/context_impl.cc +++ b/source/common/grpc/context_impl.cc @@ -4,36 +4,20 @@ #include #include "common/grpc/common.h" +#include "common/stats/utility.h" namespace Envoy { namespace Grpc { ContextImpl::ContextImpl(Stats::SymbolTable& symbol_table) - : symbol_table_(symbol_table), stat_name_pool_(symbol_table), - grpc_(stat_name_pool_.add("grpc")), grpc_web_(stat_name_pool_.add("grpc-web")), - success_(stat_name_pool_.add("success")), failure_(stat_name_pool_.add("failure")), - total_(stat_name_pool_.add("total")), zero_(stat_name_pool_.add("0")), + : stat_name_pool_(symbol_table), grpc_(stat_name_pool_.add("grpc")), + grpc_web_(stat_name_pool_.add("grpc-web")), success_(stat_name_pool_.add("success")), + failure_(stat_name_pool_.add("failure")), total_(stat_name_pool_.add("total")), + zero_(stat_name_pool_.add("0")), request_message_count_(stat_name_pool_.add("request_message_count")), response_message_count_(stat_name_pool_.add("response_message_count")), upstream_rq_time_(stat_name_pool_.add("upstream_rq_time")), stat_names_(symbol_table) {} -// Makes a stat name from a string, if we don't already have one for it. -// This always takes a lock on mutex_, and if we haven't seen the name -// before, it also takes a lock on the symbol table. -// -// TODO(jmarantz): See https://github.com/envoyproxy/envoy/pull/7008 for -// a lock-free approach to creating dynamic stat-names based on requests. -Stats::StatName ContextImpl::makeDynamicStatName(absl::string_view name) { - Thread::LockGuard lock(mutex_); - auto iter = stat_name_map_.find(name); - if (iter != stat_name_map_.end()) { - return iter->second; - } - const Stats::StatName stat_name = stat_name_pool_.add(name); - stat_name_map_[std::string(name)] = stat_name; - return stat_name; -} - // Gets the stat prefix and underlying storage, depending on whether request_names is empty Stats::ElementVec ContextImpl::getPrefix(Protocol protocol, const absl::optional& request_names) { @@ -53,14 +37,12 @@ void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, Protocol prot absl::string_view status_str = grpc_status->value().getStringView(); auto iter = stat_names_.status_names_.find(status_str); - const Stats::StatName status_stat_name = - (iter != stat_names_.status_names_.end()) ? iter->second : makeDynamicStatName(status_str); - const Stats::SymbolTable::StoragePtr stat_name_storage = - request_names ? symbol_table_.join({protocolStatName(protocol), request_names->service_, - request_names->method_, status_stat_name}) - : symbol_table_.join({protocolStatName(protocol), status_stat_name}); - - cluster.statsScope().counterFromStatName(Stats::StatName(stat_name_storage.get())).inc(); + Stats::Element status_stat_name = (iter != stat_names_.status_names_.end()) + ? Stats::Element(iter->second) + : Stats::DynamicName(status_str); + Stats::ElementVec elements = getPrefix(protocol, request_names); + elements.push_back(status_stat_name); + Stats::Utility::counterFromElements(cluster.statsScope(), elements).inc(); chargeStat(cluster, protocol, request_names, (status_str == "0")); } @@ -111,8 +93,8 @@ ContextImpl::resolveDynamicServiceAndMethod(const Http::HeaderEntry* path) { return {}; } - const Stats::StatName service = makeDynamicStatName(request_names->service_); - const Stats::StatName method = makeDynamicStatName(request_names->method_); + Stats::Element service = Stats::DynamicName(request_names->service_); + Stats::Element method = Stats::DynamicName(request_names->method_); return RequestStatNames{service, method}; } diff --git a/source/common/grpc/context_impl.h b/source/common/grpc/context_impl.h index c98a4d460b4ae..17eb105f5f15e 100644 --- a/source/common/grpc/context_impl.h +++ b/source/common/grpc/context_impl.h @@ -17,8 +17,8 @@ namespace Envoy { namespace Grpc { struct Context::RequestStatNames { - Stats::StatName service_; // supplies the service name. - Stats::StatName method_; // supplies the method name. + Stats::Element service_; // supplies the service name. + Stats::Element method_; // supplies the method name. }; class ContextImpl : public Context { @@ -60,14 +60,6 @@ class ContextImpl : public Context { StatNames& statNames() override { return stat_names_; } private: - // Makes a stat name from a string, if we don't already have one for it. - // This always takes a lock on mutex_, and if we haven't seen the name - // before, it also takes a lock on the symbol table. - // - // TODO(jmarantz): See https://github.com/envoyproxy/envoy/pull/7008 for - // a lock-free approach to creating dynamic stat-names based on requests. - Stats::StatName makeDynamicStatName(absl::string_view name); - // Gets the stat prefix and underlying storage, depending on whether request_names is empty // or not. // Prefix will be "" if request_names is empty, or @@ -75,10 +67,7 @@ class ContextImpl : public Context { Stats::ElementVec getPrefix(Protocol protocol, const absl::optional& request_names); - Stats::SymbolTable& symbol_table_; - mutable Thread::MutexBasicLockable mutex_; - Stats::StatNamePool stat_name_pool_ ABSL_GUARDED_BY(mutex_); - StringMap stat_name_map_ ABSL_GUARDED_BY(mutex_); + Stats::StatNamePool stat_name_pool_; const Stats::StatName grpc_; const Stats::StatName grpc_web_; const Stats::StatName success_; diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index b0eb4788e5187..41b1eb18f2e0c 100644 --- a/source/common/stats/utility.cc +++ b/source/common/stats/utility.cc @@ -69,6 +69,12 @@ Counter& Utility::counterFromElements(Scope& scope, const ElementVec& elements, return scope.counterFromStatNameWithTags(visitor.makeStatName(elements), tags); } +Counter& Utility::counterFromStatNames(Scope& scope, const StatNameVec& elements, + StatNameTagVectorOptConstRef tags) { + SymbolTable::StoragePtr joined = scope.symbolTable().join(elements); + return scope.counterFromStatNameWithTags(StatName(joined.get()), tags); +} + Gauge& Utility::gaugeFromElements(Scope& scope, const ElementVec& elements, Gauge::ImportMode import_mode, StatNameTagVectorOptConstRef tags) { @@ -76,11 +82,25 @@ Gauge& Utility::gaugeFromElements(Scope& scope, const ElementVec& elements, return scope.gaugeFromStatNameWithTags(visitor.makeStatName(elements), tags, import_mode); } +Gauge& Utility::gaugeFromStatNames(Scope& scope, const StatNameVec& elements, + Gauge::ImportMode import_mode, + StatNameTagVectorOptConstRef tags) { + SymbolTable::StoragePtr joined = scope.symbolTable().join(elements); + return scope.gaugeFromStatNameWithTags(StatName(joined.get()), tags, import_mode); +} + Histogram& Utility::histogramFromElements(Scope& scope, const ElementVec& elements, Histogram::Unit unit, StatNameTagVectorOptConstRef tags) { ElementVisitor visitor(scope.symbolTable()); return scope.histogramFromStatNameWithTags(visitor.makeStatName(elements), tags, unit); } +Histogram& Utility::histogramFromStatNames(Scope& scope, const StatNameVec& elements, + Histogram::Unit unit, + StatNameTagVectorOptConstRef tags) { + SymbolTable::StoragePtr joined = scope.symbolTable().join(elements); + return scope.histogramFromStatNameWithTags(StatName(joined.get()), tags, unit); +} + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 389ea1a539ea8..63c2e1a2f5659 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -14,16 +14,20 @@ namespace Envoy { namespace Stats { /** - * Represents a stat name token, using either a StatName or a string_view, - * which will be treated as a dynamic string. We subclass string_view simply - * to make it a bit more explicit when we are creating a dynamic stat name, - * since those are expensive. + * Represents a dynamically created stat name token based on absl::string_view. + * This class wrapper is used in the 'Element' variant so that call-sites + * can express explicit intent to create dynamic stat names, which are more + * expensive than symbolic stat names. We use dynamic stat names only for + * building stats based on names discovered in the line of a request. */ class DynamicName : public absl::string_view { public: DynamicName(absl::string_view s) : absl::string_view(s) {} }; +/** + * Holds either a symbolic StatName or a dynamic string. + */ using Element = absl::variant; using ElementVec = std::vector; @@ -57,6 +61,9 @@ class Utility { * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens * for more detail on why symbolic StatNames are preferred when possible. * + * See also counterFromStatNames, which is slightly faster but does not allow + * passing DynamicName(string)s as names. + * * @param scope The scope in which to create the counter. * @param elements The vector of mixed string_view and StatName * @param tags optionally specified tags. @@ -65,6 +72,24 @@ class Utility { static Counter& counterFromElements(Scope& scope, const ElementVec& elements, StatNameTagVectorOptConstRef tags = absl::nullopt); + /** + * Creates a counter from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * See also counterFromElements, which is slightly slower, but allows + * passing DynamicName(string)s as elements. + * + * @param scope The scope in which to create the counter. + * @param names The vector of StatNames + * @param tags optionally specified tags. + * @return A counter named using the joined elements. + */ + static Counter& counterFromStatNames(Scope& scope, const StatNameVec& names, + StatNameTagVectorOptConstRef tags = absl::nullopt); + /** * Creates a gauge from a vector of tokens which are used to create the * name. The tokens can be specified as string_view or StatName. For @@ -72,6 +97,9 @@ class Utility { * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens * for more detail on why symbolic StatNames are preferred when possible. * + * See also gaugeFromStatNames, which is slightly faster but does not allow + * passing DynamicName(string)s as names. + * * @param scope The scope in which to create the counter. * @param elements The vector of mixed string_view and StatName * @param import_mode Whether hot-restart should accumulate this value. @@ -82,6 +110,26 @@ class Utility { Gauge::ImportMode import_mode, StatNameTagVectorOptConstRef tags = absl::nullopt); + /** + * Creates a gauge from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * See also gaugeFromElements, which is slightly slower, but allows + * passing DynamicName(string)s as elements. + * + * @param scope The scope in which to create the counter. + * @param names The vector of StatNames + * @param import_mode Whether hot-restart should accumulate this value. + * @param tags optionally specified tags. + * @return A gauge named using the joined elements. + */ + static Gauge& gaugeFromStatNames(Scope& scope, const StatNameVec& elements, + Gauge::ImportMode import_mode, + StatNameTagVectorOptConstRef tags = absl::nullopt); + /** * Creates a histogram from a vector of tokens which are used to create the * name. The tokens can be specified as string_view or StatName. For @@ -89,6 +137,9 @@ class Utility { * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens * for more detail on why symbolic StatNames are preferred when possible. * + * See also histogramFromStatNames, which is slightly faster but does not allow + * passing DynamicName(string)s as names. + * * @param scope The scope in which to create the counter. * @param elements The vector of mixed string_view and StatName * @param unit The unit of measurement. @@ -98,6 +149,26 @@ class Utility { static Histogram& histogramFromElements(Scope& scope, const ElementVec& elements, Histogram::Unit unit, StatNameTagVectorOptConstRef tags = absl::nullopt); + + /** + * Creates a histogram from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * See also histogramFromElements, which is slightly slower, but allows + * passing DynamicName(string)s as elements. + * + * @param scope The scope in which to create the counter. + * @param elements The vector of mixed string_view and StatName + * @param unit The unit of measurement. + * @param tags optionally specified tags. + * @return A histogram named using the joined elements. + */ + static Histogram& histogramFromStatNames(Scope& scope, const StatNameVec& elements, + Histogram::Unit unit, + StatNameTagVectorOptConstRef tags = absl::nullopt); }; } // namespace Stats diff --git a/source/extensions/filters/http/dynamo/dynamo_stats.cc b/source/extensions/filters/http/dynamo/dynamo_stats.cc index 14948b20f447d..06f3770a688e6 100644 --- a/source/extensions/filters/http/dynamo/dynamo_stats.cc +++ b/source/extensions/filters/http/dynamo/dynamo_stats.cc @@ -68,11 +68,11 @@ Stats::Counter& DynamoStats::buildPartitionStatCounter(const std::string& table_ const std::string& partition_id) { // Use the last 7 characters of the partition id. absl::string_view id_last_7 = absl::string_view(partition_id).substr(partition_id.size() - 7); - Stats::StatNameDynamicPool dynamic(scope_.symbolTable()); - const Stats::StatName partition = dynamic.add(absl::StrCat("__partition_id=", id_last_7)); + std::string partition = absl::StrCat("__partition_id=", id_last_7); return Stats::Utility::counterFromElements( - scope_, addPrefix({table_, Stats::DynamicName(table_name), capacity_, - getBuiltin(operation, unknown_operation_), partition})); + scope_, + addPrefix({table_, Stats::DynamicName(table_name), capacity_, + getBuiltin(operation, unknown_operation_), Stats::DynamicName(partition)})); } size_t DynamoStats::groupIndex(uint64_t status) { diff --git a/source/extensions/filters/http/fault/fault_filter.cc b/source/extensions/filters/http/fault/fault_filter.cc index 72a032dae9074..a34e833bb12c6 100644 --- a/source/extensions/filters/http/fault/fault_filter.cc +++ b/source/extensions/filters/http/fault/fault_filter.cc @@ -86,7 +86,8 @@ FaultFilterConfig::FaultFilterConfig( stats_prefix_(stat_name_set_->add(absl::StrCat(stats_prefix, "fault"))) {} void FaultFilterConfig::incCounter(Stats::StatName downstream_cluster, Stats::StatName stat_name) { - Stats::Utility::counterFromElements(scope_, {stats_prefix_, downstream_cluster, stat_name}).inc(); + Stats::Utility::counterFromStatNames(scope_, {stats_prefix_, downstream_cluster, stat_name}) + .inc(); } FaultFilter::FaultFilter(FaultFilterConfigSharedPtr config) : config_(config) {} diff --git a/source/extensions/filters/network/common/redis/redis_command_stats.cc b/source/extensions/filters/network/common/redis/redis_command_stats.cc index 60657517333b4..63495185efc88 100644 --- a/source/extensions/filters/network/common/redis/redis_command_stats.cc +++ b/source/extensions/filters/network/common/redis/redis_command_stats.cc @@ -37,16 +37,16 @@ Stats::TimespanPtr RedisCommandStats::createCommandTimer(Stats::Scope& scope, Stats::StatName command, Envoy::TimeSource& time_source) { return std::make_unique( - Stats::Utility::histogramFromElements(scope, {prefix_, command, latency_}, - Stats::Histogram::Unit::Microseconds), + Stats::Utility::histogramFromStatNames(scope, {prefix_, command, latency_}, + Stats::Histogram::Unit::Microseconds), time_source); } Stats::TimespanPtr RedisCommandStats::createAggregateTimer(Stats::Scope& scope, Envoy::TimeSource& time_source) { return std::make_unique( - Stats::Utility::histogramFromElements(scope, {prefix_, upstream_rq_time_}, - Stats::Histogram::Unit::Microseconds), + Stats::Utility::histogramFromStatNames(scope, {prefix_, upstream_rq_time_}, + Stats::Histogram::Unit::Microseconds), time_source); } @@ -72,15 +72,15 @@ Stats::StatName RedisCommandStats::getCommandFromRequest(const RespValue& reques } void RedisCommandStats::updateStatsTotal(Stats::Scope& scope, Stats::StatName command) { - Stats::Utility::counterFromElements(scope, {prefix_, command, total_}).inc(); + Stats::Utility::counterFromStatNames(scope, {prefix_, command, total_}).inc(); } void RedisCommandStats::updateStats(Stats::Scope& scope, Stats::StatName command, const bool success) { if (success) { - Stats::Utility::counterFromElements(scope, {prefix_, command, success_}).inc(); + Stats::Utility::counterFromStatNames(scope, {prefix_, command, success_}).inc(); } else { - Stats::Utility::counterFromElements(scope, {prefix_, command, failure_}).inc(); + Stats::Utility::counterFromStatNames(scope, {prefix_, command, failure_}).inc(); } } diff --git a/source/extensions/filters/network/zookeeper_proxy/filter.cc b/source/extensions/filters/network/zookeeper_proxy/filter.cc index 491c7a5f61678..b6c38c0ec2971 100644 --- a/source/extensions/filters/network/zookeeper_proxy/filter.cc +++ b/source/extensions/filters/network/zookeeper_proxy/filter.cc @@ -154,7 +154,7 @@ void ZooKeeperFilter::onPing() { } void ZooKeeperFilter::onAuthRequest(const std::string& scheme) { - Stats::Counter& counter = Stats::Utility::counterFromElements( + Stats::Counter& counter = Stats::Utility::counterFromStatNames( config_->scope_, {config_->stat_prefix_, config_->auth_, config_->stat_name_set_->getBuiltin(absl::StrCat(scheme, "_rq"), config_->unknown_scheme_rq_)}); @@ -313,7 +313,7 @@ void ZooKeeperFilter::onResponse(const OpCodes opcode, const int32_t xid, const opcode_latency = opcode_info.latency_name_; } - Stats::Histogram& histogram = Stats::Utility::histogramFromElements( + Stats::Histogram& histogram = Stats::Utility::histogramFromStatNames( config_->scope_, {config_->stat_prefix_, opcode_latency}, Stats::Histogram::Unit::Milliseconds); histogram.recordValue(latency.count()); diff --git a/test/common/grpc/context_impl_test.cc b/test/common/grpc/context_impl_test.cc index c1fa773b25d3c..d412dd87920f6 100644 --- a/test/common/grpc/context_impl_test.cc +++ b/test/common/grpc/context_impl_test.cc @@ -73,8 +73,8 @@ TEST(GrpcContextTest, ResolveServiceAndMethod) { absl::optional request_names = context.resolveDynamicServiceAndMethod(path); EXPECT_TRUE(request_names); - EXPECT_EQ("service_name", symbol_table->toString(request_names->service_)); - EXPECT_EQ("method_name", symbol_table->toString(request_names->method_)); + EXPECT_EQ("service_name", absl::get(request_names->service_)); + EXPECT_EQ("method_name", absl::get(request_names->method_)); headers.setPath(""); EXPECT_FALSE(context.resolveDynamicServiceAndMethod(path)); headers.setPath("/"); diff --git a/test/common/stats/utility_test.cc b/test/common/stats/utility_test.cc index 6f62778dc454d..0e643702bb7a2 100644 --- a/test/common/stats/utility_test.cc +++ b/test/common/stats/utility_test.cc @@ -45,6 +45,9 @@ TEST_F(StatsUtilityTest, Counters) { StatName suffix = pool_.add("suffix"); Counter& c3 = Utility::counterFromElements(*scope, {token, suffix}); EXPECT_EQ("scope.token.suffix", c3.name()); + Counter& c4 = Utility::counterFromStatNames(*scope, {token, suffix}); + EXPECT_EQ("scope.token.suffix", c4.name()); + EXPECT_EQ(&c3, &c4); Counter& ctags = Utility::counterFromElements(*scope, {DynamicName("x"), token, DynamicName("y")}, tags_); @@ -65,6 +68,9 @@ TEST_F(StatsUtilityTest, Gauges) { StatName suffix = pool_.add("suffix"); Gauge& g3 = Utility::gaugeFromElements(*scope, {token, suffix}, Gauge::ImportMode::NeverImport); EXPECT_EQ("scope.token.suffix", g3.name()); + Gauge& g4 = Utility::gaugeFromStatNames(*scope, {token, suffix}, Gauge::ImportMode::NeverImport); + EXPECT_EQ("scope.token.suffix", g4.name()); + EXPECT_EQ(&g3, &g4); } TEST_F(StatsUtilityTest, Histograms) { @@ -82,6 +88,8 @@ TEST_F(StatsUtilityTest, Histograms) { Histogram& h3 = Utility::histogramFromElements(*scope, {token, suffix}, Histogram::Unit::Bytes); EXPECT_EQ("scope.token.suffix", h3.name()); EXPECT_EQ(Histogram::Unit::Bytes, h3.unit()); + Histogram& h4 = Utility::histogramFromStatNames(*scope, {token, suffix}, Histogram::Unit::Bytes); + EXPECT_EQ(&h3, &h4); } } // namespace From ae315a7524640c017795f262e26f8f2abe4eacf7 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 23 Apr 2020 07:53:58 -0400 Subject: [PATCH 15/19] more cleanups and add support for TextReadout Signed-off-by: Joshua Marantz --- .bazelversion | 2 +- source/common/grpc/context_impl.cc | 30 ++++++------- source/common/grpc/context_impl.h | 11 +++-- source/common/stats/utility.cc | 12 +++++ source/common/stats/utility.h | 45 ++++++++++++++++++- .../common/redis/redis_command_stats.cc | 7 +-- test/common/stats/utility_test.cc | 15 +++++++ 7 files changed, 92 insertions(+), 30 deletions(-) diff --git a/.bazelversion b/.bazelversion index ccbccc3dc6263..4a36342fcab70 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -2.2.0 +3.0.0 diff --git a/source/common/grpc/context_impl.cc b/source/common/grpc/context_impl.cc index b86b6e960e8e4..4c0e2f91ebc3e 100644 --- a/source/common/grpc/context_impl.cc +++ b/source/common/grpc/context_impl.cc @@ -19,13 +19,14 @@ ContextImpl::ContextImpl(Stats::SymbolTable& symbol_table) upstream_rq_time_(stat_name_pool_.add("upstream_rq_time")), stat_names_(symbol_table) {} // Gets the stat prefix and underlying storage, depending on whether request_names is empty -Stats::ElementVec ContextImpl::getPrefix(Protocol protocol, - const absl::optional& request_names) { +Stats::ElementVec ContextImpl::statElements(Protocol protocol, + const absl::optional& request_names, + Stats::Element suffix) { const Stats::StatName protocolName = protocolStatName(protocol); if (request_names) { - return Stats::ElementVec{protocolName, request_names->service_, request_names->method_}; + return Stats::ElementVec{protocolName, request_names->service_, request_names->method_, suffix}; } - return Stats::ElementVec{protocolName}; + return Stats::ElementVec{protocolName, suffix}; } void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, Protocol protocol, @@ -37,19 +38,17 @@ void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, Protocol prot absl::string_view status_str = grpc_status->value().getStringView(); auto iter = stat_names_.status_names_.find(status_str); - Stats::Element status_stat_name = (iter != stat_names_.status_names_.end()) - ? Stats::Element(iter->second) - : Stats::DynamicName(status_str); - Stats::ElementVec elements = getPrefix(protocol, request_names); - elements.push_back(status_stat_name); + Stats::ElementVec elements = + statElements(protocol, request_names, + (iter != stat_names_.status_names_.end()) ? Stats::Element(iter->second) + : Stats::DynamicName(status_str)); Stats::Utility::counterFromElements(cluster.statsScope(), elements).inc(); chargeStat(cluster, protocol, request_names, (status_str == "0")); } void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, Protocol protocol, const absl::optional& request_names, bool success) { - Stats::ElementVec elements = getPrefix(protocol, request_names); - elements.push_back(successStatName(success)); + Stats::ElementVec elements = statElements(protocol, request_names, successStatName(success)); Stats::Utility::counterFromElements(cluster.statsScope(), elements).inc(); elements.back() = total_; Stats::Utility::counterFromElements(cluster.statsScope(), elements).inc(); @@ -63,24 +62,21 @@ void ContextImpl::chargeStat(const Upstream::ClusterInfo& cluster, void ContextImpl::chargeRequestMessageStat(const Upstream::ClusterInfo& cluster, const absl::optional& request_names, uint64_t amount) { - Stats::ElementVec elements = getPrefix(Protocol::Grpc, request_names); - elements.push_back(request_message_count_); + Stats::ElementVec elements = statElements(Protocol::Grpc, request_names, request_message_count_); Stats::Utility::counterFromElements(cluster.statsScope(), elements).add(amount); } void ContextImpl::chargeResponseMessageStat(const Upstream::ClusterInfo& cluster, const absl::optional& request_names, uint64_t amount) { - Stats::ElementVec elements = getPrefix(Protocol::Grpc, request_names); - elements.push_back(response_message_count_); + Stats::ElementVec elements = statElements(Protocol::Grpc, request_names, response_message_count_); Stats::Utility::counterFromElements(cluster.statsScope(), elements).add(amount); } void ContextImpl::chargeUpstreamStat(const Upstream::ClusterInfo& cluster, const absl::optional& request_names, std::chrono::milliseconds duration) { - Stats::ElementVec elements = getPrefix(Protocol::Grpc, request_names); - elements.push_back(upstream_rq_time_); + Stats::ElementVec elements = statElements(Protocol::Grpc, request_names, upstream_rq_time_); Stats::Utility::histogramFromElements(cluster.statsScope(), elements, Stats::Histogram::Unit::Milliseconds) .recordValue(duration.count()); diff --git a/source/common/grpc/context_impl.h b/source/common/grpc/context_impl.h index 17eb105f5f15e..98a34695235bb 100644 --- a/source/common/grpc/context_impl.h +++ b/source/common/grpc/context_impl.h @@ -60,12 +60,11 @@ class ContextImpl : public Context { StatNames& statNames() override { return stat_names_; } private: - // Gets the stat prefix and underlying storage, depending on whether request_names is empty - // or not. - // Prefix will be "" if request_names is empty, or - // ".." if it is not empty. - Stats::ElementVec getPrefix(Protocol protocol, - const absl::optional& request_names); + // Creates an array of stat-name elements, comprising the protocol, optional + // service and method, and a suffix. + Stats::ElementVec statElements(Protocol protocol, + const absl::optional& request_names, + Stats::Element suffix); Stats::StatNamePool stat_name_pool_; const Stats::StatName grpc_; diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index 41b1eb18f2e0c..1e5347ccba1ed 100644 --- a/source/common/stats/utility.cc +++ b/source/common/stats/utility.cc @@ -102,5 +102,17 @@ Histogram& Utility::histogramFromStatNames(Scope& scope, const StatNameVec& elem return scope.histogramFromStatNameWithTags(StatName(joined.get()), tags, unit); } +TextReadout& Utility::textReadoutFromElements(Scope& scope, const ElementVec& elements, + StatNameTagVectorOptConstRef tags) { + ElementVisitor visitor(scope.symbolTable()); + return scope.textReadoutFromStatNameWithTags(visitor.makeStatName(elements), tags); +} + +TextReadout& Utility::textReadoutFromStatNames(Scope& scope, const StatNameVec& elements, + StatNameTagVectorOptConstRef tags) { + SymbolTable::StoragePtr joined = scope.symbolTable().join(elements); + return scope.textReadoutFromStatNameWithTags(StatName(joined.get()), tags); +} + } // namespace Stats } // namespace Envoy diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 63c2e1a2f5659..7e58156575acc 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -26,7 +26,12 @@ class DynamicName : public absl::string_view { }; /** - * Holds either a symbolic StatName or a dynamic string. + * Holds either a symbolic StatName or a dynamic string, for the purpose of + * composing a vector to pass to Utility::counterFromElements, etc. This is + * a programming convenience to create joined stat names. It is easier to + * call the above helpers than to use SymbolTable::join(), because the helpers + * hide the memory management of the joined storage, and they allow easier + * co-mingling of symbolic and dynamic stat-name components. */ using Element = absl::variant; using ElementVec = std::vector; @@ -169,6 +174,44 @@ class Utility { static Histogram& histogramFromStatNames(Scope& scope, const StatNameVec& elements, Histogram::Unit unit, StatNameTagVectorOptConstRef tags = absl::nullopt); + + /** + * Creates a TextReadout from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * See also TextReadoutFromStatNames, which is slightly faster but does not allow + * passing DynamicName(string)s as names. + * + * @param scope The scope in which to create the counter. + * @param elements The vector of mixed string_view and StatName + * @param unit The unit of measurement. + * @param tags optionally specified tags. + * @return A TextReadout named using the joined elements. + */ + static TextReadout& textReadoutFromElements(Scope& scope, const ElementVec& elements, + StatNameTagVectorOptConstRef tags = absl::nullopt); + + /** + * Creates a TextReadout from a vector of tokens which are used to create the + * name. The tokens can be specified as string_view or StatName. For + * tokens specified as string_view, a dynamic StatName will be created. See + * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens + * for more detail on why symbolic StatNames are preferred when possible. + * + * See also TextReadoutFromElements, which is slightly slower, but allows + * passing DynamicName(string)s as elements. + * + * @param scope The scope in which to create the counter. + * @param elements The vector of mixed string_view and StatName + * @param unit The unit of measurement. + * @param tags optionally specified tags. + * @return A TextReadout named using the joined elements. + */ + static TextReadout& textReadoutFromStatNames(Scope& scope, const StatNameVec& elements, + StatNameTagVectorOptConstRef tags = absl::nullopt); }; } // namespace Stats diff --git a/source/extensions/filters/network/common/redis/redis_command_stats.cc b/source/extensions/filters/network/common/redis/redis_command_stats.cc index 63495185efc88..5a6509cf3ae6c 100644 --- a/source/extensions/filters/network/common/redis/redis_command_stats.cc +++ b/source/extensions/filters/network/common/redis/redis_command_stats.cc @@ -77,11 +77,8 @@ void RedisCommandStats::updateStatsTotal(Stats::Scope& scope, Stats::StatName co void RedisCommandStats::updateStats(Stats::Scope& scope, Stats::StatName command, const bool success) { - if (success) { - Stats::Utility::counterFromStatNames(scope, {prefix_, command, success_}).inc(); - } else { - Stats::Utility::counterFromStatNames(scope, {prefix_, command, failure_}).inc(); - } + Stats::StatName status = success ? success_ : failure_; + Stats::Utility::counterFromStatNames(scope, {prefix_, command, status}).inc(); } } // namespace Redis diff --git a/test/common/stats/utility_test.cc b/test/common/stats/utility_test.cc index 0e643702bb7a2..8f4ec260d3bba 100644 --- a/test/common/stats/utility_test.cc +++ b/test/common/stats/utility_test.cc @@ -92,6 +92,21 @@ TEST_F(StatsUtilityTest, Histograms) { EXPECT_EQ(&h3, &h4); } +TEST_F(StatsUtilityTest, TextReadouts) { + ScopePtr scope = store_->createScope("scope."); + TextReadout& t1 = Utility::textReadoutFromElements(*scope, {DynamicName("a"), DynamicName("b")}); + EXPECT_EQ("scope.a.b", t1.name()); + StatName token = pool_.add("token"); + TextReadout& t2 = + Utility::textReadoutFromElements(*scope, {DynamicName("a"), token, DynamicName("b")}); + EXPECT_EQ("scope.a.token.b", t2.name()); + StatName suffix = pool_.add("suffix"); + TextReadout& t3 = Utility::textReadoutFromElements(*scope, {token, suffix}); + EXPECT_EQ("scope.token.suffix", t3.name()); + TextReadout& t4 = Utility::textReadoutFromStatNames(*scope, {token, suffix}); + EXPECT_EQ(&t3, &t4); +} + } // namespace } // namespace Stats } // namespace Envoy From 8aa64629923cb7dcdbed25e01ea60db11969bbd3 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 24 Apr 2020 14:44:31 -0400 Subject: [PATCH 16/19] review comments Signed-off-by: Joshua Marantz --- source/common/stats/utility.cc | 36 ++++++++++++------------ source/common/stats/utility.h | 51 ++++++++++++++-------------------- 2 files changed, 40 insertions(+), 47 deletions(-) diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index 1e5347ccba1ed..9a040f1735253 100644 --- a/source/common/stats/utility.cc +++ b/source/common/stats/utility.cc @@ -40,21 +40,23 @@ namespace { // Helper class for the three Utility::*FromElements implementations to build up // a joined StatName from a mix of StatName and string_view. struct ElementVisitor { - ElementVisitor(SymbolTable& symbol_table) : symbol_table_(symbol_table), pool_(symbol_table) {} - - // Overloads provides for absl::visit to call. - void operator()(StatName stat_name) { stat_names_.push_back(stat_name); } - void operator()(absl::string_view name) { stat_names_.push_back(pool_.add(name)); } - - // Generates a StatName from the elements. - StatName makeStatName(const ElementVec& elements) { + ElementVisitor(SymbolTable& symbol_table, const ElementVec& elements) + : symbol_table_(symbol_table), pool_(symbol_table) { for (const Element& element : elements) { absl::visit(*this, element); } joined_ = symbol_table_.join(stat_names_); - return StatName(joined_.get()); } + // Overloads provides for absl::visit to call. + void operator()(StatName stat_name) { stat_names_.push_back(stat_name); } + void operator()(absl::string_view name) { stat_names_.push_back(pool_.add(name)); } + + /** + * @return the StatName constructed by joining the elements. + */ + StatName statName() { return StatName(joined_.get()); } + SymbolTable& symbol_table_; StatNameVec stat_names_; StatNameDynamicPool pool_; @@ -65,8 +67,8 @@ struct ElementVisitor { Counter& Utility::counterFromElements(Scope& scope, const ElementVec& elements, StatNameTagVectorOptConstRef tags) { - ElementVisitor visitor(scope.symbolTable()); - return scope.counterFromStatNameWithTags(visitor.makeStatName(elements), tags); + ElementVisitor visitor(scope.symbolTable(), elements); + return scope.counterFromStatNameWithTags(visitor.statName(), tags); } Counter& Utility::counterFromStatNames(Scope& scope, const StatNameVec& elements, @@ -78,8 +80,8 @@ Counter& Utility::counterFromStatNames(Scope& scope, const StatNameVec& elements Gauge& Utility::gaugeFromElements(Scope& scope, const ElementVec& elements, Gauge::ImportMode import_mode, StatNameTagVectorOptConstRef tags) { - ElementVisitor visitor(scope.symbolTable()); - return scope.gaugeFromStatNameWithTags(visitor.makeStatName(elements), tags, import_mode); + ElementVisitor visitor(scope.symbolTable(), elements); + return scope.gaugeFromStatNameWithTags(visitor.statName(), tags, import_mode); } Gauge& Utility::gaugeFromStatNames(Scope& scope, const StatNameVec& elements, @@ -91,8 +93,8 @@ Gauge& Utility::gaugeFromStatNames(Scope& scope, const StatNameVec& elements, Histogram& Utility::histogramFromElements(Scope& scope, const ElementVec& elements, Histogram::Unit unit, StatNameTagVectorOptConstRef tags) { - ElementVisitor visitor(scope.symbolTable()); - return scope.histogramFromStatNameWithTags(visitor.makeStatName(elements), tags, unit); + ElementVisitor visitor(scope.symbolTable(), elements); + return scope.histogramFromStatNameWithTags(visitor.statName(), tags, unit); } Histogram& Utility::histogramFromStatNames(Scope& scope, const StatNameVec& elements, @@ -104,8 +106,8 @@ Histogram& Utility::histogramFromStatNames(Scope& scope, const StatNameVec& elem TextReadout& Utility::textReadoutFromElements(Scope& scope, const ElementVec& elements, StatNameTagVectorOptConstRef tags) { - ElementVisitor visitor(scope.symbolTable()); - return scope.textReadoutFromStatNameWithTags(visitor.makeStatName(elements), tags); + ElementVisitor visitor(scope.symbolTable(), elements); + return scope.textReadoutFromStatNameWithTags(visitor.statName(), tags); } TextReadout& Utility::textReadoutFromStatNames(Scope& scope, const StatNameVec& elements, diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 7e58156575acc..c13cc314dbe82 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -22,6 +22,9 @@ namespace Stats { */ class DynamicName : public absl::string_view { public: + // This is intentionally left as an implicit conversion from string_view to + // make call-sites easier to read, e.g. + // Utility::counterFromElements(*scope, {DynamicName("a"), DynamicName("b")}); DynamicName(absl::string_view s) : absl::string_view(s) {} }; @@ -61,8 +64,8 @@ class Utility { /** * Creates a counter from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See + * name. The tokens can be specified as DynamicName or StatName. For + * tokens specified as DynamicName, a dynamic StatName will be created. See * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens * for more detail on why symbolic StatNames are preferred when possible. * @@ -70,7 +73,7 @@ class Utility { * passing DynamicName(string)s as names. * * @param scope The scope in which to create the counter. - * @param elements The vector of mixed string_view and StatName + * @param elements The vector of mixed DynamicName and StatName * @param tags optionally specified tags. * @return A counter named using the joined elements. */ @@ -79,10 +82,7 @@ class Utility { /** * Creates a counter from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See - * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens - * for more detail on why symbolic StatNames are preferred when possible. + * name. The tokens must be of type StatName. * * See also counterFromElements, which is slightly slower, but allows * passing DynamicName(string)s as elements. @@ -97,8 +97,8 @@ class Utility { /** * Creates a gauge from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See + * name. The tokens can be specified as DynamicName or StatName. For + * tokens specified as DynamicName, a dynamic StatName will be created. See * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens * for more detail on why symbolic StatNames are preferred when possible. * @@ -106,7 +106,7 @@ class Utility { * passing DynamicName(string)s as names. * * @param scope The scope in which to create the counter. - * @param elements The vector of mixed string_view and StatName + * @param elements The vector of mixed DynamicName and StatName * @param import_mode Whether hot-restart should accumulate this value. * @param tags optionally specified tags. * @return A gauge named using the joined elements. @@ -117,10 +117,7 @@ class Utility { /** * Creates a gauge from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See - * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens - * for more detail on why symbolic StatNames are preferred when possible. + * name. The tokens must be of type StatName. * * See also gaugeFromElements, which is slightly slower, but allows * passing DynamicName(string)s as elements. @@ -137,8 +134,8 @@ class Utility { /** * Creates a histogram from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See + * name. The tokens can be specified as DynamicName or StatName. For + * tokens specified as DynamicName, a dynamic StatName will be created. See * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens * for more detail on why symbolic StatNames are preferred when possible. * @@ -146,7 +143,7 @@ class Utility { * passing DynamicName(string)s as names. * * @param scope The scope in which to create the counter. - * @param elements The vector of mixed string_view and StatName + * @param elements The vector of mixed DynamicName and StatName * @param unit The unit of measurement. * @param tags optionally specified tags. * @return A histogram named using the joined elements. @@ -157,16 +154,13 @@ class Utility { /** * Creates a histogram from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See - * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens - * for more detail on why symbolic StatNames are preferred when possible. + * name. The tokens must be of type StatName. * * See also histogramFromElements, which is slightly slower, but allows * passing DynamicName(string)s as elements. * * @param scope The scope in which to create the counter. - * @param elements The vector of mixed string_view and StatName + * @param elements The vector of mixed DynamicName and StatName * @param unit The unit of measurement. * @param tags optionally specified tags. * @return A histogram named using the joined elements. @@ -177,8 +171,8 @@ class Utility { /** * Creates a TextReadout from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See + * name. The tokens can be specified as DynamicName or StatName. For + * tokens specified as DynamicName, a dynamic StatName will be created. See * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens * for more detail on why symbolic StatNames are preferred when possible. * @@ -186,7 +180,7 @@ class Utility { * passing DynamicName(string)s as names. * * @param scope The scope in which to create the counter. - * @param elements The vector of mixed string_view and StatName + * @param elements The vector of mixed DynamicName and StatName * @param unit The unit of measurement. * @param tags optionally specified tags. * @return A TextReadout named using the joined elements. @@ -196,16 +190,13 @@ class Utility { /** * Creates a TextReadout from a vector of tokens which are used to create the - * name. The tokens can be specified as string_view or StatName. For - * tokens specified as string_view, a dynamic StatName will be created. See - * https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#dynamic-stat-tokens - * for more detail on why symbolic StatNames are preferred when possible. + * name. The tokens must be of type StatName. * * See also TextReadoutFromElements, which is slightly slower, but allows * passing DynamicName(string)s as elements. * * @param scope The scope in which to create the counter. - * @param elements The vector of mixed string_view and StatName + * @param elements The vector of mixed DynamicName and StatName * @param unit The unit of measurement. * @param tags optionally specified tags. * @return A TextReadout named using the joined elements. From ab073ffc9ede1aac3459aff2272defb22c452236 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 24 Apr 2020 14:47:13 -0400 Subject: [PATCH 17/19] use "using" rather than explicit ctor. Signed-off-by: Joshua Marantz --- source/common/stats/utility.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index c13cc314dbe82..219ba579031dd 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -25,7 +25,7 @@ class DynamicName : public absl::string_view { // This is intentionally left as an implicit conversion from string_view to // make call-sites easier to read, e.g. // Utility::counterFromElements(*scope, {DynamicName("a"), DynamicName("b")}); - DynamicName(absl::string_view s) : absl::string_view(s) {} + using absl::string_view::string_view; }; /** From d5adafb5b112ff89e68c948e5e7927408c621df1 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 24 Apr 2020 17:36:39 -0400 Subject: [PATCH 18/19] Use inlined vectors for Stats::Utility::ElementVec Signed-off-by: Joshua Marantz --- source/common/stats/utility.cc | 1 + source/common/stats/utility.h | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/stats/utility.cc b/source/common/stats/utility.cc index 9a040f1735253..ee3944172c0d9 100644 --- a/source/common/stats/utility.cc +++ b/source/common/stats/utility.cc @@ -42,6 +42,7 @@ namespace { struct ElementVisitor { ElementVisitor(SymbolTable& symbol_table, const ElementVec& elements) : symbol_table_(symbol_table), pool_(symbol_table) { + stat_names_.resize(elements.size()); for (const Element& element : elements) { absl::visit(*this, element); } diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index 219ba579031dd..fdfecf4587883 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -7,6 +7,7 @@ #include "common/stats/symbol_table_impl.h" +#include "absl/container/inlined_vector.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -37,7 +38,7 @@ class DynamicName : public absl::string_view { * co-mingling of symbolic and dynamic stat-name components. */ using Element = absl::variant; -using ElementVec = std::vector; +using ElementVec = absl::InlinedVector; /** * Common stats utility routines. From 969c26a3342f950e9d89386f057150687c0275b4 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 24 Apr 2020 17:58:10 -0400 Subject: [PATCH 19/19] Go back to using a constructor rather than inheriting the base class's. The latter didn't work for alll call-sites. Signed-off-by: Joshua Marantz --- source/common/stats/utility.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/stats/utility.h b/source/common/stats/utility.h index fdfecf4587883..46b72234da3ab 100644 --- a/source/common/stats/utility.h +++ b/source/common/stats/utility.h @@ -26,7 +26,7 @@ class DynamicName : public absl::string_view { // This is intentionally left as an implicit conversion from string_view to // make call-sites easier to read, e.g. // Utility::counterFromElements(*scope, {DynamicName("a"), DynamicName("b")}); - using absl::string_view::string_view; + DynamicName(absl::string_view str) : absl::string_view(str) {} }; /**