stats: Add option to switch between fake and real symbol-tables on th…

…e command-line. (envoyproxy#7882)

* Add option to switch between fake and real symbol-tables on the command-line.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

docs/root/intro/version_history.rst

@@ -28,6 +28,7 @@ Version history
 * http: added the ability to :ref:`merge adjacent slashes<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.merge_slashes>` in the path.
 * listeners: added :ref:`continue_on_listener_filters_timeout <envoy_api_field_Listener.continue_on_listener_filters_timeout>` to configure whether a listener will still create a connection when listener filters time out.
 * listeners: added :ref:`HTTP inspector listener filter <config_listener_filters_http_inspector>`.
+* performance: stats symbol table implementation (disabled by default; to test it, add "--use-fake-symbol-table 0" to the command-line arguments when starting Envoy).
 * redis: added :ref:`read_policy <envoy_api_field_config.filter.network.redis_proxy.v2.RedisProxy.ConnPoolSettings.read_policy>` to allow reading from redis replicas for Redis Cluster deployments.
 * rbac: added support for DNS SAN as :ref:`principal_name <envoy_api_field_config.rbac.v2.Principal.Authenticated.principal_name>`.
 * lua: extended `httpCall()` and `respond()` APIs to accept headers with entry values that can be a string or table of strings.

include/envoy/server/options.h

@@ -184,6 +184,11 @@ class Options {
    */
   virtual bool libeventBufferEnabled() const PURE;
 
+  /**
+   * @return whether to use the fake symbol table implementation.
+   */
+  virtual bool fakeSymbolTableEnabled() const PURE;
+
   /**
    * @return bool indicating whether cpuset size should determine the number of worker threads.
    */

include/envoy/stats/symbol_table.h

@@ -185,7 +185,7 @@ class SymbolTable {
   virtual StoragePtr encode(absl::string_view name) PURE;
 };
 
-using SharedSymbolTable = std::shared_ptr<SymbolTable>;
+using SymbolTablePtr = std::unique_ptr<SymbolTable>;
 
 } // namespace Stats
 } // namespace Envoy

source/common/stats/BUILD

@@ -50,6 +50,7 @@ envoy_cc_library(
         ":scope_prefixer_lib",
         ":stats_lib",
         ":store_impl_lib",
+        ":symbol_table_creator_lib",
         "//include/envoy/stats:stats_macros",
         "//source/common/stats:allocator_lib",
     ],
@@ -155,6 +156,17 @@ envoy_cc_library(
     ],
 )
 
+envoy_cc_library(
+    name = "symbol_table_creator_lib",
+    srcs = ["symbol_table_creator.cc"],
+    hdrs = ["symbol_table_creator.h"],
+    external_deps = ["abseil_base"],
+    deps = [
+        ":fake_symbol_table_lib",
+        ":symbol_table_lib",
+    ],
+)
+
 envoy_cc_library(
     name = "fake_symbol_table_lib",
     hdrs = ["fake_symbol_table_impl.h"],

source/common/stats/isolated_store_impl.cc

@@ -8,13 +8,13 @@
 #include "common/stats/fake_symbol_table_impl.h"
 #include "common/stats/histogram_impl.h"
 #include "common/stats/scope_prefixer.h"
+#include "common/stats/symbol_table_creator.h"
 #include "common/stats/utility.h"
 
 namespace Envoy {
 namespace Stats {
 
-IsolatedStoreImpl::IsolatedStoreImpl()
-    : IsolatedStoreImpl(std::make_unique<FakeSymbolTableImpl>()) {}
+IsolatedStoreImpl::IsolatedStoreImpl() : IsolatedStoreImpl(SymbolTableCreator::makeSymbolTable()) {}
 
 IsolatedStoreImpl::IsolatedStoreImpl(std::unique_ptr<SymbolTable>&& symbol_table)
     : IsolatedStoreImpl(*symbol_table) {

source/common/stats/isolated_store_impl.h

@@ -133,7 +133,7 @@ class IsolatedStoreImpl : public StoreImpl {
 private:
   IsolatedStoreImpl(std::unique_ptr<SymbolTable>&& symbol_table);
 
-  std::unique_ptr<SymbolTable> symbol_table_storage_;
+  SymbolTablePtr symbol_table_storage_;
   AllocatorImpl alloc_;
   IsolatedStatsCache<Counter> counters_;
   IsolatedStatsCache<Gauge> gauges_;

source/common/stats/symbol_table_creator.cc

@@ -0,0 +1,24 @@
+#include "common/stats/symbol_table_creator.h"
+
+namespace Envoy {
+namespace Stats {
+
+bool SymbolTableCreator::initialized_ = false;
+bool SymbolTableCreator::use_fake_symbol_tables_ = true;
+
+SymbolTablePtr SymbolTableCreator::initAndMakeSymbolTable(bool use_fake) {
+  ASSERT(!initialized_ || (use_fake_symbol_tables_ == use_fake));
+  use_fake_symbol_tables_ = use_fake;
+  return makeSymbolTable();
+}
+
+SymbolTablePtr SymbolTableCreator::makeSymbolTable() {
+  initialized_ = true;
+  if (use_fake_symbol_tables_) {
+    return std::make_unique<FakeSymbolTableImpl>();
+  }
+  return std::make_unique<SymbolTableImpl>();
+}
+
+} // namespace Stats
+} // namespace Envoy

source/common/stats/symbol_table_creator.h

@@ -0,0 +1,57 @@
+#pragma once
+
+#include "common/stats/fake_symbol_table_impl.h"
+#include "common/stats/symbol_table_impl.h"
+
+namespace Envoy {
+namespace Stats {
+
+namespace TestUtil {
+class SymbolTableCreatorTestPeer;
+}
+
+class SymbolTableCreator {
+public:
+  /**
+   * Initializes the symbol-table creation system. Once this is called, it is a
+   * runtime assertion to call this again in production code, changing the
+   * use_fakes setting. However, tests can change the setting via
+   * TestUtil::SymbolTableCreatorTestPeer::setUseFakeSymbolTables(use_fakes).
+   *
+   * @param use_fakes Whether to use fake symbol tables; typically from a command-line option.
+   * @return a SymbolTable.
+   */
+  static SymbolTablePtr initAndMakeSymbolTable(bool use_fakes);
+
+  /**
+   * Factory method to create SymbolTables. This is needed to help make it
+   * possible to flag-flip use of real symbol tables, and ultimately should be
+   * removed.
+   *
+   * @return a SymbolTable.
+   */
+  static SymbolTablePtr makeSymbolTable();
+
+  /**
+   * @return whether the system is initialized to use fake symbol tables.
+   */
+  static bool useFakeSymbolTables() { return use_fake_symbol_tables_; }
+
+private:
+  friend class TestUtil::SymbolTableCreatorTestPeer;
+
+  /**
+   * Sets whether fake or real symbol tables should be used. Tests that alter
+   * this should restore previous value at the end of the test. This must be
+   * called via TestUtil::SymbolTableCreatorTestPeer.
+   *
+   * *param use_fakes whether to use fake symbol tables.
+   */
+  static void setUseFakeSymbolTables(bool use_fakes) { use_fake_symbol_tables_ = use_fakes; }
+
+  static bool initialized_;
+  static bool use_fake_symbol_tables_;
+};
+
+} // namespace Stats
+} // namespace Envoy

source/exe/BUILD

@@ -70,7 +70,7 @@ envoy_cc_library(
         "//source/common/api:os_sys_calls_lib",
         "//source/common/common:compiler_requirements_lib",
         "//source/common/common:perf_annotation_lib",
-        "//source/common/stats:fake_symbol_table_lib",
+        "//source/common/stats:symbol_table_creator_lib",
         "//source/server:hot_restart_lib",
         "//source/server:hot_restart_nop_lib",
         "//source/server/config_validation:server_lib",

source/exe/main_common.cc

@@ -7,6 +7,7 @@
 #include "common/common/compiler_requirements.h"
 #include "common/common/perf_annotation.h"
 #include "common/network/utility.h"
+#include "common/stats/symbol_table_creator.h"
 #include "common/stats/thread_local_store.h"
 
 #include "server/config_validation/server.h"
@@ -45,7 +46,9 @@ MainCommonBase::MainCommonBase(const OptionsImpl& options, Event::TimeSystem& ti
                                Filesystem::Instance& file_system,
                                std::unique_ptr<ProcessContext> process_context)
     : options_(options), component_factory_(component_factory), thread_factory_(thread_factory),
-      file_system_(file_system), stats_allocator_(symbol_table_) {
+      file_system_(file_system), symbol_table_(Stats::SymbolTableCreator::initAndMakeSymbolTable(
+                                     options_.fakeSymbolTableEnabled())),
+      stats_allocator_(*symbol_table_) {
   switch (options_.mode()) {
   case Server::Mode::InitOnly:
   case Server::Mode::Serve: {

source/exe/main_common.h

@@ -67,10 +67,10 @@ class MainCommonBase {
 protected:
   ProcessWide process_wide_; // Process-wide state setup/teardown.
   const Envoy::OptionsImpl& options_;
-  Stats::FakeSymbolTableImpl symbol_table_;
   Server::ComponentFactory& component_factory_;
   Thread::ThreadFactory& thread_factory_;
   Filesystem::Instance& file_system_;
+  Stats::SymbolTablePtr symbol_table_;
   Stats::AllocatorImpl stats_allocator_;
 
   std::unique_ptr<ThreadLocal::InstanceImpl> tls_;

source/server/BUILD

@@ -378,6 +378,7 @@ envoy_cc_library(
         "//source/common/runtime:runtime_lib",
         "//source/common/secret:secret_manager_impl_lib",
         "//source/common/singleton:manager_impl_lib",
+        "//source/common/stats:symbol_table_creator_lib",
         "//source/common/stats:thread_local_store_lib",
         "//source/common/upstream:cluster_manager_lib",
         "//source/common/upstream:health_discovery_service_lib",

source/server/options_impl.cc

@@ -112,7 +112,9 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv,
   TCLAP::ValueArg<bool> use_libevent_buffer("", "use-libevent-buffers",
                                             "Use the original libevent buffer implementation",
                                             false, false, "bool", cmd);
-
+  TCLAP::ValueArg<bool> use_fake_symbol_table("", "use-fake-symbol-table",
+                                              "Use fake symbol table implementation", false, true,
+                                              "bool", cmd);
   cmd.setExceptionHandling(false);
   try {
     cmd.parse(argc, argv);
@@ -136,6 +138,7 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv,
   mutex_tracing_enabled_ = enable_mutex_tracing.getValue();
 
   libevent_buffer_enabled_ = use_libevent_buffer.getValue();
+  fake_symbol_table_enabled_ = use_fake_symbol_table.getValue();
   cpuset_threads_ = cpuset_threads.getValue();
 
   log_level_ = default_log_level;
@@ -300,6 +303,7 @@ OptionsImpl::OptionsImpl(const std::string& service_cluster, const std::string&
       service_cluster_(service_cluster), service_node_(service_node), service_zone_(service_zone),
       file_flush_interval_msec_(10000), drain_time_(600), parent_shutdown_time_(900),
       mode_(Server::Mode::Serve), hot_restart_disabled_(false), signal_handling_enabled_(true),
-      mutex_tracing_enabled_(false), cpuset_threads_(false), libevent_buffer_enabled_(false) {}
+      mutex_tracing_enabled_(false), cpuset_threads_(false), libevent_buffer_enabled_(false),
+      fake_symbol_table_enabled_(false) {}
 
 } // namespace Envoy

source/server/options_impl.h

@@ -81,6 +81,9 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
   void setRejectUnknownFieldsDynamic(bool reject_unknown_dynamic_fields) {
     reject_unknown_dynamic_fields_ = reject_unknown_dynamic_fields;
   }
+  void setFakeSymbolTableEnabled(bool fake_symbol_table_enabled) {
+    fake_symbol_table_enabled_ = fake_symbol_table_enabled;
+  }
 
   // Server::Options
   uint64_t baseId() const override { return base_id_; }
@@ -117,6 +120,7 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
   bool signalHandlingEnabled() const override { return signal_handling_enabled_; }
   bool mutexTracingEnabled() const override { return mutex_tracing_enabled_; }
   bool libeventBufferEnabled() const override { return libevent_buffer_enabled_; }
+  bool fakeSymbolTableEnabled() const override { return fake_symbol_table_enabled_; }
   Server::CommandLineOptionsPtr toCommandLineOptions() const override;
   void parseComponentLogLevels(const std::string& component_log_levels);
   bool cpusetThreadsEnabled() const override { return cpuset_threads_; }
@@ -152,6 +156,7 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
   bool mutex_tracing_enabled_;
   bool cpuset_threads_;
   bool libevent_buffer_enabled_;
+  bool fake_symbol_table_enabled_;
   uint32_t count_;
 };
 

test/common/grpc/context_impl_test.cc

@@ -18,7 +18,7 @@ namespace Grpc {
 
 TEST(GrpcContextTest, ChargeStats) {
   NiceMock<Upstream::MockClusterInfo> cluster;
-  Envoy::Test::Global<Stats::FakeSymbolTableImpl> symbol_table_;
+  Stats::TestSymbolTable symbol_table_;
   Stats::StatNamePool pool(*symbol_table_);
   const Stats::StatName service = pool.add("service");
   const Stats::StatName method = pool.add("method");
@@ -58,7 +58,7 @@ TEST(GrpcContextTest, ResolveServiceAndMethod) {
   Http::HeaderMapImpl headers;
   Http::HeaderEntry& path = headers.insertPath();
   path.value(std::string("/service_name/method_name"));
-  Envoy::Test::Global<Stats::FakeSymbolTableImpl> symbol_table;
+  Stats::TestSymbolTable symbol_table;
   ContextImpl context(*symbol_table);
   absl::optional<Context::RequestNames> request_names = context.resolveServiceAndMethod(&path);
   EXPECT_TRUE(request_names);

test/common/grpc/grpc_client_integration_test_harness.h

@@ -416,7 +416,7 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest {
   FakeHttpConnectionPtr fake_connection_;
   std::vector<FakeStreamPtr> fake_streams_;
   const Protobuf::MethodDescriptor* method_descriptor_;
-  Envoy::Test::Global<Stats::FakeSymbolTableImpl> symbol_table_;
+  Stats::TestSymbolTable symbol_table_;
   Stats::IsolatedStoreImpl* stats_store_ = new Stats::IsolatedStoreImpl(*symbol_table_);
   Api::ApiPtr api_;
   Event::DispatcherPtr dispatcher_;

test/common/http/BUILD

@@ -161,6 +161,7 @@ envoy_cc_fuzz_test(
         "//source/common/http:date_provider_lib",
         "//source/common/network:address_lib",
         "//source/common/network:utility_lib",
+        "//source/common/stats:symbol_table_creator_lib",
         "//test/fuzz:utility_lib",
         "//test/mocks/access_log:access_log_mocks",
         "//test/mocks/http:http_mocks",

test/common/http/codes_test.cc

@@ -8,6 +8,7 @@
 #include "common/common/empty_string.h"
 #include "common/http/codes.h"
 #include "common/http/header_map_impl.h"
+#include "common/stats/symbol_table_creator.h"
 
 #include "test/mocks/stats/mocks.h"
 #include "test/test_common/printers.h"
@@ -45,7 +46,7 @@ class CodeUtilityTest : public testing::Test {
     code_stats_.chargeResponseStat(info);
   }
 
-  Envoy::Test::Global<Stats::FakeSymbolTableImpl> symbol_table_;
+  Stats::TestSymbolTable symbol_table_;
   Stats::IsolatedStoreImpl global_store_;
   Stats::IsolatedStoreImpl cluster_scope_;
   Http::CodeStatsImpl code_stats_;
@@ -276,13 +277,14 @@ TEST_F(CodeUtilityTest, ResponseTimingTest) {
 
 class CodeStatsTest : public testing::Test {
 protected:
-  CodeStatsTest() : code_stats_(symbol_table_) {}
+  CodeStatsTest()
+      : symbol_table_(Stats::SymbolTableCreator::makeSymbolTable()), code_stats_(*symbol_table_) {}
 
   absl::string_view stripTrailingDot(absl::string_view prefix) {
     return CodeStatsImpl::stripTrailingDot(prefix);
   }
 
-  Stats::FakeSymbolTableImpl symbol_table_;
+  Stats::SymbolTablePtr symbol_table_;
   CodeStatsImpl code_stats_;
 };
 

test/common/http/conn_manager_impl_fuzz_test.cc

@@ -19,6 +19,7 @@
 #include "common/http/exception.h"
 #include "common/network/address_impl.h"
 #include "common/network/utility.h"
+#include "common/stats/symbol_table_creator.h"
 
 #include "test/common/http/conn_manager_impl_common.h"
 #include "test/common/http/conn_manager_impl_fuzz.pb.h"
@@ -382,8 +383,8 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) {
   FuzzConfig config;
   NiceMock<Network::MockDrainDecision> drain_close;
   NiceMock<Runtime::MockRandomGenerator> random;
-  Stats::FakeSymbolTableImpl symbol_table;
-  Http::ContextImpl http_context(symbol_table);
+  Stats::SymbolTablePtr symbol_table(Stats::SymbolTableCreator::makeSymbolTable());
+  Http::ContextImpl http_context(*symbol_table);
   NiceMock<Runtime::MockLoader> runtime;
   NiceMock<LocalInfo::MockLocalInfo> local_info;
   NiceMock<Upstream::MockClusterManager> cluster_manager;

test/common/router/config_impl_test.cc

@@ -109,7 +109,7 @@ class ConfigImplTestBase {
     return factory_context_.scope().symbolTable().toString(name);
   }
 
-  Test::Global<Stats::FakeSymbolTableImpl> symbol_table_;
+  Stats::TestSymbolTable symbol_table_;
   Api::ApiPtr api_;
   NiceMock<Server::Configuration::MockFactoryContext> factory_context_;
 };

test/common/stats/BUILD

@@ -61,6 +61,7 @@ envoy_cc_test_library(
     deps = [
         "//source/common/common:assert_lib",
         "//source/common/memory:stats_lib",
+        "//source/common/stats:symbol_table_creator_lib",
     ],
 )
 

test/common/stats/stat_test_utility.h

@@ -2,6 +2,7 @@
 
 #include "common/common/logger.h"
 #include "common/memory/stats.h"
+#include "common/stats/symbol_table_creator.h"
 
 #include "absl/strings/str_join.h"
 #include "absl/strings/string_view.h"
@@ -104,6 +105,13 @@ class MemoryTest {
     }                                                                                              \
   } while (false)
 
+class SymbolTableCreatorTestPeer {
+public:
+  static void setUseFakeSymbolTables(bool use_fakes) {
+    SymbolTableCreator::setUseFakeSymbolTables(use_fakes);
+  }
+};
+
 } // namespace TestUtil
 } // namespace Stats
 } // namespace Envoy