From 78ef3829a676aead394b4fdf06c5742c4a2f046f Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 3 Oct 2019 14:40:28 -0400 Subject: [PATCH] stats: Ensure sanity of stats prefixes (#8445) * Ensure sanity of stats prefixes. Signed-off-by: Joshua Marantz --- include/envoy/stats/stats_macros.h | 25 +++++++++++++++---- source/common/stats/fake_symbol_table_impl.h | 1 + source/common/stats/symbol_table_impl.cc | 1 + source/common/tcp_proxy/tcp_proxy.cc | 2 +- .../filters/network/mongo_proxy/config.cc | 2 +- .../network/mongo_proxy/mongo_stats.cc | 2 +- .../filters/network/mongo_proxy/mongo_stats.h | 2 +- .../network/mysql_proxy/mysql_config.cc | 2 +- .../filters/network/zookeeper_proxy/BUILD | 1 + .../filters/network/zookeeper_proxy/config.cc | 2 +- .../filters/network/zookeeper_proxy/filter.cc | 1 + .../common/router/router_upstream_log_test.cc | 2 +- test/common/stats/symbol_table_impl_test.cc | 6 ++--- .../filters/http/router/config_test.cc | 8 +++--- .../network/mysql_proxy/mysql_filter_test.cc | 2 +- 15 files changed, 39 insertions(+), 20 deletions(-) diff --git a/include/envoy/stats/stats_macros.h b/include/envoy/stats/stats_macros.h index 0959ff38aabc..14d7a88a37cb 100644 --- a/include/envoy/stats/stats_macros.h +++ b/include/envoy/stats/stats_macros.h @@ -5,6 +5,11 @@ #include "envoy/stats/histogram.h" #include "envoy/stats/stats.h" +#include "common/common/assert.h" + +#include "absl/strings/match.h" +#include "absl/strings/str_cat.h" + namespace Envoy { /** * These are helper macros for allocating "fixed" stats throughout the code base in a way that @@ -37,12 +42,22 @@ namespace Envoy { #define GENERATE_GAUGE_STRUCT(NAME, MODE) Envoy::Stats::Gauge& NAME##_; #define GENERATE_HISTOGRAM_STRUCT(NAME) Envoy::Stats::Histogram& NAME##_; -#define FINISH_STAT_DECL_(X) + std::string(#X)), -#define FINISH_STAT_DECL_MODE_(X, MODE) + std::string(#X), Envoy::Stats::Gauge::ImportMode::MODE), +#define FINISH_STAT_DECL_(X) #X)), +#define FINISH_STAT_DECL_MODE_(X, MODE) #X), Envoy::Stats::Gauge::ImportMode::MODE), + +static inline std::string statPrefixJoin(absl::string_view prefix, absl::string_view token) { + if (prefix.empty()) { + return std::string(token); + } else if (absl::EndsWith(prefix, ".")) { + // TODO(jmarantz): eliminate this case -- remove all the trailing dots from prefixes. + return absl::StrCat(prefix, token); + } + return absl::StrCat(prefix, ".", token); +} -#define POOL_COUNTER_PREFIX(POOL, PREFIX) (POOL).counter(PREFIX FINISH_STAT_DECL_ -#define POOL_GAUGE_PREFIX(POOL, PREFIX) (POOL).gauge(PREFIX FINISH_STAT_DECL_MODE_ -#define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogram(PREFIX FINISH_STAT_DECL_ +#define POOL_COUNTER_PREFIX(POOL, PREFIX) (POOL).counter(statPrefixJoin(PREFIX, FINISH_STAT_DECL_ +#define POOL_GAUGE_PREFIX(POOL, PREFIX) (POOL).gauge(statPrefixJoin(PREFIX, FINISH_STAT_DECL_MODE_ +#define POOL_HISTOGRAM_PREFIX(POOL, PREFIX) (POOL).histogram(statPrefixJoin(PREFIX, FINISH_STAT_DECL_ #define POOL_COUNTER(POOL) POOL_COUNTER_PREFIX(POOL, "") #define POOL_GAUGE(POOL) POOL_GAUGE_PREFIX(POOL, "") diff --git a/source/common/stats/fake_symbol_table_impl.h b/source/common/stats/fake_symbol_table_impl.h index 8c43dc40b730..058cb20f23df 100644 --- a/source/common/stats/fake_symbol_table_impl.h +++ b/source/common/stats/fake_symbol_table_impl.h @@ -138,6 +138,7 @@ class FakeSymbolTableImpl : public SymbolTable { } StoragePtr encodeHelper(absl::string_view name) const { + ASSERT(!absl::EndsWith(name, ".")); auto bytes = std::make_unique(name.size() + StatNameSizeEncodingBytes); uint8_t* buffer = SymbolTableImpl::writeLengthReturningNext(name.size(), bytes.get()); memcpy(buffer, name.data(), name.size()); diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc index b0ea68d88890..745898fa7758 100644 --- a/source/common/stats/symbol_table_impl.cc +++ b/source/common/stats/symbol_table_impl.cc @@ -301,6 +301,7 @@ void SymbolTableImpl::debugPrint() const { #endif SymbolTable::StoragePtr SymbolTableImpl::encode(absl::string_view name) { + ASSERT(!absl::EndsWith(name, ".")); Encoding encoding; addTokensToEncoding(name, encoding); auto bytes = std::make_unique(encoding.bytesRequired()); diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 0a1987fe7a13..ed7a85ebbb19 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -56,7 +56,7 @@ Config::WeightedClusterEntry::WeightedClusterEntry( Config::SharedConfig::SharedConfig( const envoy::config::filter::network::tcp_proxy::v2::TcpProxy& config, Server::Configuration::FactoryContext& context) - : stats_scope_(context.scope().createScope(fmt::format("tcp.{}.", config.stat_prefix()))), + : stats_scope_(context.scope().createScope(fmt::format("tcp.{}", config.stat_prefix()))), stats_(generateStats(*stats_scope_)) { if (config.has_idle_timeout()) { idle_timeout_ = diff --git a/source/extensions/filters/network/mongo_proxy/config.cc b/source/extensions/filters/network/mongo_proxy/config.cc index 24539b6bd953..7f6f19541678 100644 --- a/source/extensions/filters/network/mongo_proxy/config.cc +++ b/source/extensions/filters/network/mongo_proxy/config.cc @@ -20,7 +20,7 @@ Network::FilterFactoryCb MongoProxyFilterConfigFactory::createFilterFactoryFromP ASSERT(!proto_config.stat_prefix().empty()); - const std::string stat_prefix = fmt::format("mongo.{}.", proto_config.stat_prefix()); + const std::string stat_prefix = fmt::format("mongo.{}", proto_config.stat_prefix()); AccessLogSharedPtr access_log; if (!proto_config.access_log().empty()) { access_log.reset(new AccessLog(proto_config.access_log(), context.accessLogManager(), diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc index d23de1b6f49b..d569a73e1bad 100644 --- a/source/extensions/filters/network/mongo_proxy/mongo_stats.cc +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.cc @@ -13,7 +13,7 @@ namespace Extensions { namespace NetworkFilters { namespace MongoProxy { -MongoStats::MongoStats(Stats::Scope& scope, const std::string& prefix) +MongoStats::MongoStats(Stats::Scope& scope, absl::string_view prefix) : scope_(scope), stat_name_set_(scope.symbolTable().makeSet("Mongo")), prefix_(stat_name_set_->add(prefix)), callsite_(stat_name_set_->add("callsite")), cmd_(stat_name_set_->add("cmd")), collection_(stat_name_set_->add("collection")), diff --git a/source/extensions/filters/network/mongo_proxy/mongo_stats.h b/source/extensions/filters/network/mongo_proxy/mongo_stats.h index aeb657f13526..57373693daa9 100644 --- a/source/extensions/filters/network/mongo_proxy/mongo_stats.h +++ b/source/extensions/filters/network/mongo_proxy/mongo_stats.h @@ -15,7 +15,7 @@ namespace MongoProxy { class MongoStats { public: - MongoStats(Stats::Scope& scope, const std::string& prefix); + MongoStats(Stats::Scope& scope, absl::string_view prefix); void incCounter(const std::vector& names); void recordHistogram(const std::vector& names, uint64_t sample); diff --git a/source/extensions/filters/network/mysql_proxy/mysql_config.cc b/source/extensions/filters/network/mysql_proxy/mysql_config.cc index ebfe4e3c3525..30ec557c2ef1 100644 --- a/source/extensions/filters/network/mysql_proxy/mysql_config.cc +++ b/source/extensions/filters/network/mysql_proxy/mysql_config.cc @@ -25,7 +25,7 @@ NetworkFilters::MySQLProxy::MySQLConfigFactory::createFilterFactoryFromProtoType ASSERT(!proto_config.stat_prefix().empty()); - const std::string stat_prefix = fmt::format("mysql.{}.", proto_config.stat_prefix()); + const std::string stat_prefix = fmt::format("mysql.{}", proto_config.stat_prefix()); MySQLFilterConfigSharedPtr filter_config( std::make_shared(stat_prefix, context.scope())); diff --git a/source/extensions/filters/network/zookeeper_proxy/BUILD b/source/extensions/filters/network/zookeeper_proxy/BUILD index 8956cd975fe1..934f23a53032 100644 --- a/source/extensions/filters/network/zookeeper_proxy/BUILD +++ b/source/extensions/filters/network/zookeeper_proxy/BUILD @@ -32,6 +32,7 @@ envoy_cc_library( "//source/common/common:enum_to_int", "//source/common/network:filter_lib", "//source/common/stats:symbol_table_lib", + "//source/common/stats:utility_lib", "//source/extensions/filters/network:well_known_names", ], ) diff --git a/source/extensions/filters/network/zookeeper_proxy/config.cc b/source/extensions/filters/network/zookeeper_proxy/config.cc index bc1fabe860cb..8caecadb2148 100644 --- a/source/extensions/filters/network/zookeeper_proxy/config.cc +++ b/source/extensions/filters/network/zookeeper_proxy/config.cc @@ -24,7 +24,7 @@ Network::FilterFactoryCb ZooKeeperConfigFactory::createFilterFactoryFromProtoTyp ASSERT(!proto_config.stat_prefix().empty()); - const std::string stat_prefix = fmt::format("{}.zookeeper.", proto_config.stat_prefix()); + const std::string stat_prefix = fmt::format("{}.zookeeper", proto_config.stat_prefix()); const uint32_t max_packet_bytes = PROTOBUF_GET_WRAPPED_OR_DEFAULT(proto_config, max_packet_bytes, 1024 * 1024); diff --git a/source/extensions/filters/network/zookeeper_proxy/filter.cc b/source/extensions/filters/network/zookeeper_proxy/filter.cc index bae059cf2757..e84f7c2e57fe 100644 --- a/source/extensions/filters/network/zookeeper_proxy/filter.cc +++ b/source/extensions/filters/network/zookeeper_proxy/filter.cc @@ -8,6 +8,7 @@ #include "common/common/enum_to_int.h" #include "common/common/fmt.h" #include "common/common/logger.h" +#include "common/stats/utility.h" #include "extensions/filters/network/well_known_names.h" diff --git a/test/common/router/router_upstream_log_test.cc b/test/common/router/router_upstream_log_test.cc index 0b74b3e068d3..9b0bad05cebe 100644 --- a/test/common/router/router_upstream_log_test.cc +++ b/test/common/router/router_upstream_log_test.cc @@ -87,7 +87,7 @@ class RouterUpstreamLogTest : public testing::Test { current_upstream_log->CopyFrom(upstream_log.value()); } - config_.reset(new FilterConfig("prefix", context_, ShadowWriterPtr(new MockShadowWriter()), + config_.reset(new FilterConfig("prefix.", context_, ShadowWriterPtr(new MockShadowWriter()), router_proto)); router_.reset(new TestFilter(*config_)); router_->setDecoderFilterCallbacks(callbacks_); diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index 6c7e5f32dbfc..7f37f2c7f02e 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/test/common/stats/symbol_table_impl_test.cc @@ -81,7 +81,7 @@ INSTANTIATE_TEST_SUITE_P(StatNameTest, StatNameTest, TEST_P(StatNameTest, AllocFree) { encodeDecode("hello.world"); } TEST_P(StatNameTest, TestArbitrarySymbolRoundtrip) { - const std::vector stat_names = {"", " ", " ", ",", "\t", "$", "%", "`", "."}; + const std::vector stat_names = {"", " ", " ", ",", "\t", "$", "%", "`", ".x"}; for (auto& stat_name : stat_names) { EXPECT_EQ(stat_name, encodeDecode(stat_name)); } @@ -101,8 +101,8 @@ TEST_P(StatNameTest, Test100KSymbolsRoundtrip) { } TEST_P(StatNameTest, TestUnusualDelimitersRoundtrip) { - const std::vector stat_names = {".", "..", "...", "foo", "foo.", - ".foo", ".foo.", ".foo..", "..foo.", "..foo.."}; + const std::vector stat_names = {".x", "..x", "...x", "foo", "foo.x", + ".foo", ".foo.x", ".foo..x", "..foo.x", "..foo..x"}; for (auto& stat_name : stat_names) { EXPECT_EQ(stat_name, encodeDecode(stat_name)); } diff --git a/test/extensions/filters/http/router/config_test.cc b/test/extensions/filters/http/router/config_test.cc index 70e0079558a0..c22afccdb256 100644 --- a/test/extensions/filters/http/router/config_test.cc +++ b/test/extensions/filters/http/router/config_test.cc @@ -27,7 +27,7 @@ TEST(RouterFilterConfigTest, RouterFilterInJson) { Json::ObjectSharedPtr json_config = Json::Factory::loadFromString(json_string); NiceMock context; RouterFilterConfig factory; - Http::FilterFactoryCb cb = factory.createFilterFactory(*json_config, "stats", context); + Http::FilterFactoryCb cb = factory.createFilterFactory(*json_config, "stats.", context); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); cb(filter_callback); @@ -59,7 +59,7 @@ TEST(RouterFilterConfigTest, RouterFilterWithUnsupportedStrictHeaderCheck) { NiceMock context; RouterFilterConfig factory; EXPECT_THROW_WITH_MESSAGE( - factory.createFilterFactoryFromProto(router_config, "stats", context), + factory.createFilterFactoryFromProto(router_config, "stats.", context), ProtoValidationException, "Proto constraint validation failed (RouterValidationError.StrictCheckHeaders[i]: " "[\"value must be in list \" [" @@ -77,7 +77,7 @@ TEST(RouterFilterConfigTest, RouterV2Filter) { NiceMock context; RouterFilterConfig factory; - Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(router_config, "stats", context); + Http::FilterFactoryCb cb = factory.createFilterFactoryFromProto(router_config, "stats.", context); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); cb(filter_callback); @@ -87,7 +87,7 @@ TEST(RouterFilterConfigTest, RouterFilterWithEmptyProtoConfig) { NiceMock context; RouterFilterConfig factory; Http::FilterFactoryCb cb = - factory.createFilterFactoryFromProto(*factory.createEmptyConfigProto(), "stats", context); + factory.createFilterFactoryFromProto(*factory.createEmptyConfigProto(), "stats.", context); Http::MockFilterChainFactoryCallbacks filter_callback; EXPECT_CALL(filter_callback, addStreamDecoderFilter(_)); cb(filter_callback); diff --git a/test/extensions/filters/network/mysql_proxy/mysql_filter_test.cc b/test/extensions/filters/network/mysql_proxy/mysql_filter_test.cc index 43ee4bf7802c..0d2e971d5ce9 100644 --- a/test/extensions/filters/network/mysql_proxy/mysql_filter_test.cc +++ b/test/extensions/filters/network/mysql_proxy/mysql_filter_test.cc @@ -30,7 +30,7 @@ class MySQLFilterTest : public testing::Test, public MySQLTestUtils { MySQLFilterConfigSharedPtr config_; std::unique_ptr filter_; Stats::IsolatedStoreImpl scope_; - std::string stat_prefix_{"test"}; + std::string stat_prefix_{"test."}; NiceMock filter_callbacks_; };