stats: Ensure sanity of stats prefixes (envoyproxy#8445)

* Ensure sanity of stats prefixes.

Signed-off-by: Joshua Marantz <[email protected]>

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, "")

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<Storage>(name.size() + StatNameSizeEncodingBytes);
     uint8_t* buffer = SymbolTableImpl::writeLengthReturningNext(name.size(), bytes.get());
     memcpy(buffer, name.data(), name.size());

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<Storage>(encoding.bytesRequired());

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_ =

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(),

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")),

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<Stats::StatName>& names);
   void recordHistogram(const std::vector<Stats::StatName>& names, uint64_t sample);

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<MySQLFilterConfig>(stat_prefix, context.scope()));

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",
     ],
 )

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);
 

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"
 

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_);

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<std::string> stat_names = {"", " ", "  ", ",", "\t", "$", "%", "`", "."};
+  const std::vector<std::string> 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<std::string> stat_names = {".",    "..",    "...",    "foo",    "foo.",
-                                               ".foo", ".foo.", ".foo..", "..foo.", "..foo.."};
+  const std::vector<std::string> 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));
   }

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<Server::Configuration::MockFactoryContext> 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<Server::Configuration::MockFactoryContext> 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<Server::Configuration::MockFactoryContext> 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<Server::Configuration::MockFactoryContext> 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);

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<MySQLFilter> filter_;
   Stats::IsolatedStoreImpl scope_;
-  std::string stat_prefix_{"test"};
+  std::string stat_prefix_{"test."};
   NiceMock<Network::MockReadFilterCallbacks> filter_callbacks_;
 };