From 81d9a8ade58e361199ddec62e9914e8ecd0c7706 Mon Sep 17 00:00:00 2001
From: Yuchen Dai <silentdai@gmail.com>
Date: Fri, 10 Sep 2021 05:51:21 +0000
Subject: [PATCH 1/6] logger: use server stats scope at grpc logger

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
---
 .../common/grpc_access_logger.h               | 21 ++++++++---------
 .../access_loggers/grpc/config_utils.cc       | 16 +++++++++++--
 .../grpc/grpc_access_log_impl.cc              |  4 ++--
 .../grpc/grpc_access_log_impl.h               |  2 +-
 .../access_loggers/grpc/http_config.cc        |  6 ++---
 .../grpc/http_grpc_access_log_impl.cc         |  7 +++---
 .../grpc/http_grpc_access_log_impl.h          |  4 +---
 .../access_loggers/grpc/tcp_config.cc         |  3 +--
 .../grpc/tcp_grpc_access_log_impl.cc          |  7 +++---
 .../grpc/tcp_grpc_access_log_impl.h           |  4 +---
 .../open_telemetry/access_log_impl.cc         |  7 +++---
 .../open_telemetry/access_log_impl.h          |  4 +---
 .../access_loggers/open_telemetry/config.cc   |  2 +-
 .../open_telemetry/grpc_access_log_impl.cc    |  4 ++--
 .../open_telemetry/grpc_access_log_impl.h     |  2 +-
 .../common/grpc_access_logger_test.cc         | 23 +++++++------------
 .../grpc/grpc_access_log_impl_test.cc         |  2 +-
 .../grpc/http_grpc_access_log_impl_test.cc    |  8 +++----
 .../open_telemetry/access_log_impl_test.cc    |  9 ++++----
 .../grpc_access_log_impl_test.cc              |  2 +-
 20 files changed, 65 insertions(+), 72 deletions(-)

diff --git a/source/extensions/access_loggers/common/grpc_access_logger.h b/source/extensions/access_loggers/common/grpc_access_logger.h
index 416d19164662..7e5967976e28 100644
--- a/source/extensions/access_loggers/common/grpc_access_logger.h
+++ b/source/extensions/access_loggers/common/grpc_access_logger.h
@@ -68,9 +68,8 @@ template <typename GrpcAccessLogger, typename ConfigProto> class GrpcAccessLogge
    * @param config supplies the configuration for the logger.
    * @return GrpcAccessLoggerSharedPtr ready for logging requests.
    */
-  virtual typename GrpcAccessLogger::SharedPtr getOrCreateLogger(const ConfigProto& config,
-                                                                 GrpcAccessLoggerType logger_type,
-                                                                 Stats::Scope& scope) PURE;
+  virtual typename GrpcAccessLogger::SharedPtr
+  getOrCreateLogger(const ConfigProto& config, GrpcAccessLoggerType logger_type) PURE;
 };
 
 template <typename LogRequest, typename LogResponse> class GrpcAccessLogClient {
@@ -252,15 +251,14 @@ class GrpcAccessLoggerCache : public Singleton::Instance,
 
   GrpcAccessLoggerCache(Grpc::AsyncClientManager& async_client_manager, Stats::Scope& scope,
                         ThreadLocal::SlotAllocator& tls)
-      : async_client_manager_(async_client_manager), scope_(scope), tls_slot_(tls.allocateSlot()) {
+      : scope_(scope), async_client_manager_(async_client_manager), tls_slot_(tls.allocateSlot()) {
     tls_slot_->set([](Event::Dispatcher& dispatcher) {
       return std::make_shared<ThreadLocalCache>(dispatcher);
     });
   }
 
-  typename GrpcAccessLogger::SharedPtr getOrCreateLogger(const ConfigProto& config,
-                                                         GrpcAccessLoggerType logger_type,
-                                                         Stats::Scope& scope) override {
+  typename GrpcAccessLogger::SharedPtr
+  getOrCreateLogger(const ConfigProto& config, GrpcAccessLoggerType logger_type) override {
     // TODO(euroelessar): Consider cleaning up loggers.
     auto& cache = tls_slot_->getTyped<ThreadLocalCache>();
     const auto cache_key = std::make_pair(MessageUtil::hash(config), logger_type);
@@ -273,12 +271,14 @@ class GrpcAccessLoggerCache : public Singleton::Instance,
         async_client_manager_.factoryForGrpcService(config.grpc_service(), scope_, false)
             ->createUncachedRawAsyncClient(),
         std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config, buffer_flush_interval, 1000)),
-        PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, buffer_size_bytes, 16384), cache.dispatcher_,
-        scope);
+        PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, buffer_size_bytes, 16384), cache.dispatcher_);
     cache.access_loggers_.emplace(cache_key, logger);
     return logger;
   }
 
+protected:
+  Stats::Scope& scope_;
+
 private:
   /**
    * Per-thread cache.
@@ -297,10 +297,9 @@ class GrpcAccessLoggerCache : public Singleton::Instance,
   virtual typename GrpcAccessLogger::SharedPtr
   createLogger(const ConfigProto& config, const Grpc::RawAsyncClientSharedPtr& client,
                std::chrono::milliseconds buffer_flush_interval_msec, uint64_t max_buffer_size_bytes,
-               Event::Dispatcher& dispatcher, Stats::Scope& scope) PURE;
+               Event::Dispatcher& dispatcher) PURE;
 
   Grpc::AsyncClientManager& async_client_manager_;
-  Stats::Scope& scope_;
   ThreadLocal::SlotPtr tls_slot_;
 };
 
diff --git a/source/extensions/access_loggers/grpc/config_utils.cc b/source/extensions/access_loggers/grpc/config_utils.cc
index 0010109617cd..45a3b62c4ba7 100644
--- a/source/extensions/access_loggers/grpc/config_utils.cc
+++ b/source/extensions/access_loggers/grpc/config_utils.cc
@@ -14,9 +14,21 @@ GrpcCommon::GrpcAccessLoggerCacheSharedPtr
 getGrpcAccessLoggerCacheSingleton(Server::Configuration::CommonFactoryContext& context) {
   return context.singletonManager().getTyped<GrpcCommon::GrpcAccessLoggerCacheImpl>(
       SINGLETON_MANAGER_REGISTERED_NAME(grpc_access_logger_cache), [&context] {
+        auto* filter_factory_context =
+            dynamic_cast<Server::Configuration::FactoryContext*>(&context);
+        // debug only: other candidates could be server factory context. The scope there is good.
+
+        FANCY_LOG(trace, "in {} access log cache is create from {}", __FUNCTION__,
+                  filter_factory_context != nullptr ? "unsafe filter factory context"
+                                                    : "safe server/listener factory context");
+        FANCY_LOG(trace, "maybe unsafe scope addr = {} ", static_cast<void*>(&context.scope()));
+
+        auto& scope = filter_factory_context == nullptr
+                          ? context.scope()
+                          : filter_factory_context->getServerFactoryContext().scope();
         return std::make_shared<GrpcCommon::GrpcAccessLoggerCacheImpl>(
-            context.clusterManager().grpcAsyncClientManager(), context.scope(),
-            context.threadLocal(), context.localInfo());
+            context.clusterManager().grpcAsyncClientManager(), scope, context.threadLocal(),
+            context.localInfo());
       });
 }
 } // namespace GrpcCommon
diff --git a/source/extensions/access_loggers/grpc/grpc_access_log_impl.cc b/source/extensions/access_loggers/grpc/grpc_access_log_impl.cc
index e544a52af191..ca45d2c5acaf 100644
--- a/source/extensions/access_loggers/grpc/grpc_access_log_impl.cc
+++ b/source/extensions/access_loggers/grpc/grpc_access_log_impl.cc
@@ -53,10 +53,10 @@ GrpcAccessLoggerImpl::SharedPtr GrpcAccessLoggerCacheImpl::createLogger(
     const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& config,
     const Grpc::RawAsyncClientSharedPtr& client,
     std::chrono::milliseconds buffer_flush_interval_msec, uint64_t max_buffer_size_bytes,
-    Event::Dispatcher& dispatcher, Stats::Scope& scope) {
+    Event::Dispatcher& dispatcher) {
   return std::make_shared<GrpcAccessLoggerImpl>(client, config.log_name(),
                                                 buffer_flush_interval_msec, max_buffer_size_bytes,
-                                                dispatcher, local_info_, scope);
+                                                dispatcher, local_info_, scope_);
 }
 
 } // namespace GrpcCommon
diff --git a/source/extensions/access_loggers/grpc/grpc_access_log_impl.h b/source/extensions/access_loggers/grpc/grpc_access_log_impl.h
index 43b542327476..c502f4365d89 100644
--- a/source/extensions/access_loggers/grpc/grpc_access_log_impl.h
+++ b/source/extensions/access_loggers/grpc/grpc_access_log_impl.h
@@ -54,7 +54,7 @@ class GrpcAccessLoggerCacheImpl
   createLogger(const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& config,
                const Grpc::RawAsyncClientSharedPtr& client,
                std::chrono::milliseconds buffer_flush_interval_msec, uint64_t max_buffer_size_bytes,
-               Event::Dispatcher& dispatcher, Stats::Scope& scope) override;
+               Event::Dispatcher& dispatcher) override;
 
   const LocalInfo::LocalInfo& local_info_;
 };
diff --git a/source/extensions/access_loggers/grpc/http_config.cc b/source/extensions/access_loggers/grpc/http_config.cc
index 4d333f6d91fd..fa69f555b48b 100644
--- a/source/extensions/access_loggers/grpc/http_config.cc
+++ b/source/extensions/access_loggers/grpc/http_config.cc
@@ -27,9 +27,9 @@ AccessLog::InstanceSharedPtr HttpGrpcAccessLogFactory::createAccessLogInstance(
       const envoy::extensions::access_loggers::grpc::v3::HttpGrpcAccessLogConfig&>(
       config, context.messageValidationVisitor());
 
-  return std::make_shared<HttpGrpcAccessLog>(std::move(filter), proto_config, context.threadLocal(),
-                                             GrpcCommon::getGrpcAccessLoggerCacheSingleton(context),
-                                             context.scope());
+  return std::make_shared<HttpGrpcAccessLog>(
+      std::move(filter), proto_config, context.threadLocal(),
+      GrpcCommon::getGrpcAccessLoggerCacheSingleton(context));
 }
 
 ProtobufTypes::MessagePtr HttpGrpcAccessLogFactory::createEmptyConfigProto() {
diff --git a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc
index f35715c37ad9..7c8a4fa36d1a 100644
--- a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc
+++ b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.cc
@@ -26,9 +26,8 @@ HttpGrpcAccessLog::ThreadLocalLogger::ThreadLocalLogger(
 HttpGrpcAccessLog::HttpGrpcAccessLog(
     AccessLog::FilterPtr&& filter,
     envoy::extensions::access_loggers::grpc::v3::HttpGrpcAccessLogConfig config,
-    ThreadLocal::SlotAllocator& tls, GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache,
-    Stats::Scope& scope)
-    : Common::ImplBase(std::move(filter)), scope_(scope), config_(std::move(config)),
+    ThreadLocal::SlotAllocator& tls, GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache)
+    : Common::ImplBase(std::move(filter)), config_(std::move(config)),
       tls_slot_(tls.allocateSlot()), access_logger_cache_(std::move(access_logger_cache)) {
   for (const auto& header : config_.additional_request_headers_to_log()) {
     request_headers_to_log_.emplace_back(header);
@@ -44,7 +43,7 @@ HttpGrpcAccessLog::HttpGrpcAccessLog(
   Envoy::Config::Utility::checkTransportVersion(config_.common_config());
   tls_slot_->set([this](Event::Dispatcher&) {
     return std::make_shared<ThreadLocalLogger>(access_logger_cache_->getOrCreateLogger(
-        config_.common_config(), Common::GrpcAccessLoggerType::HTTP, scope_));
+        config_.common_config(), Common::GrpcAccessLoggerType::HTTP));
   });
 }
 
diff --git a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.h b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.h
index d403596f8dec..72409381a42f 100644
--- a/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.h
+++ b/source/extensions/access_loggers/grpc/http_grpc_access_log_impl.h
@@ -29,8 +29,7 @@ class HttpGrpcAccessLog : public Common::ImplBase {
   HttpGrpcAccessLog(AccessLog::FilterPtr&& filter,
                     envoy::extensions::access_loggers::grpc::v3::HttpGrpcAccessLogConfig config,
                     ThreadLocal::SlotAllocator& tls,
-                    GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache,
-                    Stats::Scope& scope);
+                    GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache);
 
 private:
   /**
@@ -48,7 +47,6 @@ class HttpGrpcAccessLog : public Common::ImplBase {
                const Http::ResponseTrailerMap& response_trailers,
                const StreamInfo::StreamInfo& stream_info) override;
 
-  Stats::Scope& scope_;
   const envoy::extensions::access_loggers::grpc::v3::HttpGrpcAccessLogConfig config_;
   const ThreadLocal::SlotPtr tls_slot_;
   const GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache_;
diff --git a/source/extensions/access_loggers/grpc/tcp_config.cc b/source/extensions/access_loggers/grpc/tcp_config.cc
index 185a76e934d8..65c2416c74b7 100644
--- a/source/extensions/access_loggers/grpc/tcp_config.cc
+++ b/source/extensions/access_loggers/grpc/tcp_config.cc
@@ -28,8 +28,7 @@ AccessLog::InstanceSharedPtr TcpGrpcAccessLogFactory::createAccessLogInstance(
       config, context.messageValidationVisitor());
 
   return std::make_shared<TcpGrpcAccessLog>(std::move(filter), proto_config, context.threadLocal(),
-                                            GrpcCommon::getGrpcAccessLoggerCacheSingleton(context),
-                                            context.scope());
+                                            GrpcCommon::getGrpcAccessLoggerCacheSingleton(context));
 }
 
 ProtobufTypes::MessagePtr TcpGrpcAccessLogFactory::createEmptyConfigProto() {
diff --git a/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc
index 7fbcee911d5b..99925602feb8 100644
--- a/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc
+++ b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.cc
@@ -20,14 +20,13 @@ TcpGrpcAccessLog::ThreadLocalLogger::ThreadLocalLogger(GrpcCommon::GrpcAccessLog
 TcpGrpcAccessLog::TcpGrpcAccessLog(
     AccessLog::FilterPtr&& filter,
     envoy::extensions::access_loggers::grpc::v3::TcpGrpcAccessLogConfig config,
-    ThreadLocal::SlotAllocator& tls, GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache,
-    Stats::Scope& scope)
-    : Common::ImplBase(std::move(filter)), scope_(scope), config_(std::move(config)),
+    ThreadLocal::SlotAllocator& tls, GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache)
+    : Common::ImplBase(std::move(filter)), config_(std::move(config)),
       tls_slot_(tls.allocateSlot()), access_logger_cache_(std::move(access_logger_cache)) {
   Config::Utility::checkTransportVersion(config_.common_config());
   tls_slot_->set([this](Event::Dispatcher&) {
     return std::make_shared<ThreadLocalLogger>(access_logger_cache_->getOrCreateLogger(
-        config_.common_config(), Common::GrpcAccessLoggerType::TCP, scope_));
+        config_.common_config(), Common::GrpcAccessLoggerType::TCP));
   });
 }
 
diff --git a/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h
index fba13f16d6c6..aaaf1ecc7064 100644
--- a/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h
+++ b/source/extensions/access_loggers/grpc/tcp_grpc_access_log_impl.h
@@ -28,8 +28,7 @@ class TcpGrpcAccessLog : public Common::ImplBase {
   TcpGrpcAccessLog(AccessLog::FilterPtr&& filter,
                    envoy::extensions::access_loggers::grpc::v3::TcpGrpcAccessLogConfig config,
                    ThreadLocal::SlotAllocator& tls,
-                   GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache,
-                   Stats::Scope& scope);
+                   GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache);
 
 private:
   /**
@@ -47,7 +46,6 @@ class TcpGrpcAccessLog : public Common::ImplBase {
                const Http::ResponseTrailerMap& response_trailers,
                const StreamInfo::StreamInfo& stream_info) override;
 
-  Stats::Scope& scope_;
   const envoy::extensions::access_loggers::grpc::v3::TcpGrpcAccessLogConfig config_;
   const ThreadLocal::SlotPtr tls_slot_;
   const GrpcCommon::GrpcAccessLoggerCacheSharedPtr access_logger_cache_;
diff --git a/source/extensions/access_loggers/open_telemetry/access_log_impl.cc b/source/extensions/access_loggers/open_telemetry/access_log_impl.cc
index 48c4166395da..55aa8f4ea8ea 100644
--- a/source/extensions/access_loggers/open_telemetry/access_log_impl.cc
+++ b/source/extensions/access_loggers/open_telemetry/access_log_impl.cc
@@ -35,15 +35,14 @@ AccessLog::ThreadLocalLogger::ThreadLocalLogger(GrpcAccessLoggerSharedPtr logger
 AccessLog::AccessLog(
     ::Envoy::AccessLog::FilterPtr&& filter,
     envoy::extensions::access_loggers::open_telemetry::v3alpha::OpenTelemetryAccessLogConfig config,
-    ThreadLocal::SlotAllocator& tls, GrpcAccessLoggerCacheSharedPtr access_logger_cache,
-    Stats::Scope& scope)
-    : Common::ImplBase(std::move(filter)), scope_(scope), tls_slot_(tls.allocateSlot()),
+    ThreadLocal::SlotAllocator& tls, GrpcAccessLoggerCacheSharedPtr access_logger_cache)
+    : Common::ImplBase(std::move(filter)), tls_slot_(tls.allocateSlot()),
       access_logger_cache_(std::move(access_logger_cache)) {
 
   Envoy::Config::Utility::checkTransportVersion(config.common_config());
   tls_slot_->set([this, config](Event::Dispatcher&) {
     return std::make_shared<ThreadLocalLogger>(access_logger_cache_->getOrCreateLogger(
-        config.common_config(), Common::GrpcAccessLoggerType::HTTP, scope_));
+        config.common_config(), Common::GrpcAccessLoggerType::HTTP));
   });
 
   ProtobufWkt::Struct body_format;
diff --git a/source/extensions/access_loggers/open_telemetry/access_log_impl.h b/source/extensions/access_loggers/open_telemetry/access_log_impl.h
index 1bd6b34804c8..5c20620ee0c2 100644
--- a/source/extensions/access_loggers/open_telemetry/access_log_impl.h
+++ b/source/extensions/access_loggers/open_telemetry/access_log_impl.h
@@ -36,8 +36,7 @@ class AccessLog : public Common::ImplBase {
   AccessLog(::Envoy::AccessLog::FilterPtr&& filter,
             envoy::extensions::access_loggers::open_telemetry::v3alpha::OpenTelemetryAccessLogConfig
                 config,
-            ThreadLocal::SlotAllocator& tls, GrpcAccessLoggerCacheSharedPtr access_logger_cache,
-            Stats::Scope& scope);
+            ThreadLocal::SlotAllocator& tls, GrpcAccessLoggerCacheSharedPtr access_logger_cache);
 
 private:
   /**
@@ -55,7 +54,6 @@ class AccessLog : public Common::ImplBase {
                const Http::ResponseTrailerMap& response_trailers,
                const StreamInfo::StreamInfo& stream_info) override;
 
-  Stats::Scope& scope_;
   const ThreadLocal::SlotPtr tls_slot_;
   const GrpcAccessLoggerCacheSharedPtr access_logger_cache_;
   std::unique_ptr<Formatter::StructFormatter> body_formatter_;
diff --git a/source/extensions/access_loggers/open_telemetry/config.cc b/source/extensions/access_loggers/open_telemetry/config.cc
index 2b0f02f3acd2..ca8e42358702 100644
--- a/source/extensions/access_loggers/open_telemetry/config.cc
+++ b/source/extensions/access_loggers/open_telemetry/config.cc
@@ -43,7 +43,7 @@ AccessLogFactory::createAccessLogInstance(const Protobuf::Message& config,
           config, context.messageValidationVisitor());
 
   return std::make_shared<AccessLog>(std::move(filter), proto_config, context.threadLocal(),
-                                     getAccessLoggerCacheSingleton(context), context.scope());
+                                     getAccessLoggerCacheSingleton(context));
 }
 
 ProtobufTypes::MessagePtr AccessLogFactory::createEmptyConfigProto() {
diff --git a/source/extensions/access_loggers/open_telemetry/grpc_access_log_impl.cc b/source/extensions/access_loggers/open_telemetry/grpc_access_log_impl.cc
index 215f7cfba9e4..3f8fa1328768 100644
--- a/source/extensions/access_loggers/open_telemetry/grpc_access_log_impl.cc
+++ b/source/extensions/access_loggers/open_telemetry/grpc_access_log_impl.cc
@@ -76,10 +76,10 @@ GrpcAccessLoggerImpl::SharedPtr GrpcAccessLoggerCacheImpl::createLogger(
     const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& config,
     const Grpc::RawAsyncClientSharedPtr& client,
     std::chrono::milliseconds buffer_flush_interval_msec, uint64_t max_buffer_size_bytes,
-    Event::Dispatcher& dispatcher, Stats::Scope& scope) {
+    Event::Dispatcher& dispatcher) {
   return std::make_shared<GrpcAccessLoggerImpl>(client, config.log_name(),
                                                 buffer_flush_interval_msec, max_buffer_size_bytes,
-                                                dispatcher, local_info_, scope);
+                                                dispatcher, local_info_, scope_);
 }
 
 } // namespace OpenTelemetry
diff --git a/source/extensions/access_loggers/open_telemetry/grpc_access_log_impl.h b/source/extensions/access_loggers/open_telemetry/grpc_access_log_impl.h
index 7af83f529de4..85aa0ad8d694 100644
--- a/source/extensions/access_loggers/open_telemetry/grpc_access_log_impl.h
+++ b/source/extensions/access_loggers/open_telemetry/grpc_access_log_impl.h
@@ -68,7 +68,7 @@ class GrpcAccessLoggerCacheImpl
   createLogger(const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& config,
                const Grpc::RawAsyncClientSharedPtr& client,
                std::chrono::milliseconds buffer_flush_interval_msec, uint64_t max_buffer_size_bytes,
-               Event::Dispatcher& dispatcher, Stats::Scope& scope) override;
+               Event::Dispatcher& dispatcher) override;
 
   const LocalInfo::LocalInfo& local_info_;
 };
diff --git a/test/extensions/access_loggers/common/grpc_access_logger_test.cc b/test/extensions/access_loggers/common/grpc_access_logger_test.cc
index f2e125df17e0..bc41c3033da9 100644
--- a/test/extensions/access_loggers/common/grpc_access_logger_test.cc
+++ b/test/extensions/access_loggers/common/grpc_access_logger_test.cc
@@ -322,9 +322,9 @@ class MockGrpcAccessLoggerCache
   createLogger(const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig&,
                const Grpc::RawAsyncClientSharedPtr& client,
                std::chrono::milliseconds buffer_flush_interval_msec, uint64_t max_buffer_size_bytes,
-               Event::Dispatcher& dispatcher, Stats::Scope& scope) override {
+               Event::Dispatcher& dispatcher) override {
     return std::make_shared<MockGrpcAccessLoggerImpl>(
-        std::move(client), buffer_flush_interval_msec, max_buffer_size_bytes, dispatcher, scope,
+        std::move(client), buffer_flush_interval_msec, max_buffer_size_bytes, dispatcher, scope_,
         "mock_access_log_prefix.", mockMethodDescriptor());
   }
 };
@@ -354,38 +354,31 @@ class GrpcAccessLoggerCacheTest : public testing::Test {
 };
 
 TEST_F(GrpcAccessLoggerCacheTest, Deduplication) {
-  Stats::IsolatedStoreImpl scope;
-
   envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig config;
   config.set_log_name("log-1");
   config.mutable_grpc_service()->mutable_envoy_grpc()->set_cluster_name("cluster-1");
 
   expectClientCreation();
   MockGrpcAccessLoggerImpl::SharedPtr logger1 =
-      logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope);
-  EXPECT_EQ(logger1,
-            logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope));
+      logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP);
+  EXPECT_EQ(logger1, logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP));
 
   // Do not deduplicate different types of logger
   expectClientCreation();
-  EXPECT_NE(logger1,
-            logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::TCP, scope));
+  EXPECT_NE(logger1, logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::TCP));
 
   // Changing log name leads to another logger.
   config.set_log_name("log-2");
   expectClientCreation();
-  EXPECT_NE(logger1,
-            logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope));
+  EXPECT_NE(logger1, logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP));
 
   config.set_log_name("log-1");
-  EXPECT_EQ(logger1,
-            logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope));
+  EXPECT_EQ(logger1, logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP));
 
   // Changing cluster name leads to another logger.
   config.mutable_grpc_service()->mutable_envoy_grpc()->set_cluster_name("cluster-2");
   expectClientCreation();
-  EXPECT_NE(logger1,
-            logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope));
+  EXPECT_NE(logger1, logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP));
 }
 
 } // namespace
diff --git a/test/extensions/access_loggers/grpc/grpc_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/grpc_access_log_impl_test.cc
index 3e5e4f58f900..2ae61941d8ae 100644
--- a/test/extensions/access_loggers/grpc/grpc_access_log_impl_test.cc
+++ b/test/extensions/access_loggers/grpc/grpc_access_log_impl_test.cc
@@ -156,7 +156,7 @@ TEST_F(GrpcAccessLoggerCacheImplTest, LoggerCreation) {
   config.mutable_buffer_size_bytes()->set_value(BUFFER_SIZE_BYTES);
 
   GrpcAccessLoggerSharedPtr logger =
-      logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope_);
+      logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP);
   // Note that the local info node() method is mocked, so the node is not really configurable.
   grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF(
   identifier:
diff --git a/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc
index 8c78f302f561..0f47ff530080 100644
--- a/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc
+++ b/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc
@@ -45,7 +45,7 @@ class MockGrpcAccessLoggerCache : public GrpcCommon::GrpcAccessLoggerCache {
   // GrpcAccessLoggerCache
   MOCK_METHOD(GrpcCommon::GrpcAccessLoggerSharedPtr, getOrCreateLogger,
               (const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& config,
-               Common::GrpcAccessLoggerType logger_type, Stats::Scope& scope));
+               Common::GrpcAccessLoggerType logger_type));
 };
 
 class HttpGrpcAccessLogTest : public testing::Test {
@@ -58,17 +58,17 @@ class HttpGrpcAccessLogTest : public testing::Test {
     config_.mutable_common_config()->add_filter_state_objects_to_log("serialized");
     config_.mutable_common_config()->set_transport_api_version(
         envoy::config::core::v3::ApiVersion::V3);
-    EXPECT_CALL(*logger_cache_, getOrCreateLogger(_, _, _))
+    EXPECT_CALL(*logger_cache_, getOrCreateLogger(_, _))
         .WillOnce(
             [this](const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig&
                        config,
-                   Common::GrpcAccessLoggerType logger_type, Stats::Scope&) {
+                   Common::GrpcAccessLoggerType logger_type) {
               EXPECT_EQ(config.DebugString(), config_.common_config().DebugString());
               EXPECT_EQ(Common::GrpcAccessLoggerType::HTTP, logger_type);
               return logger_;
             });
     access_log_ = std::make_unique<HttpGrpcAccessLog>(AccessLog::FilterPtr{filter_}, config_, tls_,
-                                                      logger_cache_, scope_);
+                                                      logger_cache_);
   }
 
   void expectLog(const std::string& expected_log_entry_yaml) {
diff --git a/test/extensions/access_loggers/open_telemetry/access_log_impl_test.cc b/test/extensions/access_loggers/open_telemetry/access_log_impl_test.cc
index f815e3a3bd98..db85d106f55c 100644
--- a/test/extensions/access_loggers/open_telemetry/access_log_impl_test.cc
+++ b/test/extensions/access_loggers/open_telemetry/access_log_impl_test.cc
@@ -52,7 +52,7 @@ class MockGrpcAccessLoggerCache : public GrpcAccessLoggerCache {
   // GrpcAccessLoggerCache
   MOCK_METHOD(GrpcAccessLoggerSharedPtr, getOrCreateLogger,
               (const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig& config,
-               Common::GrpcAccessLoggerType logger_type, Stats::Scope& scope));
+               Common::GrpcAccessLoggerType logger_type));
 };
 
 class AccessLogTest : public testing::Test {
@@ -82,17 +82,16 @@ string_value: "x-request-header: %REQ(x-request-header)%, protocol: %PROTOCOL%"
     config_.mutable_common_config()->set_log_name("test_log");
     config_.mutable_common_config()->set_transport_api_version(
         envoy::config::core::v3::ApiVersion::V3);
-    EXPECT_CALL(*logger_cache_, getOrCreateLogger(_, _, _))
+    EXPECT_CALL(*logger_cache_, getOrCreateLogger(_, _))
         .WillOnce(
             [this](const envoy::extensions::access_loggers::grpc::v3::CommonGrpcAccessLogConfig&
                        config,
-                   Common::GrpcAccessLoggerType logger_type, Stats::Scope&) {
+                   Common::GrpcAccessLoggerType logger_type) {
               EXPECT_EQ(config.DebugString(), config_.common_config().DebugString());
               EXPECT_EQ(Common::GrpcAccessLoggerType::HTTP, logger_type);
               return logger_;
             });
-    access_log_ =
-        std::make_unique<AccessLog>(FilterPtr{filter_}, config_, tls_, logger_cache_, scope_);
+    access_log_ = std::make_unique<AccessLog>(FilterPtr{filter_}, config_, tls_, logger_cache_);
   }
 
   void expectLog(const std::string& expected_log_entry_yaml) {
diff --git a/test/extensions/access_loggers/open_telemetry/grpc_access_log_impl_test.cc b/test/extensions/access_loggers/open_telemetry/grpc_access_log_impl_test.cc
index 850ae1dfa4cd..427eb8457d83 100644
--- a/test/extensions/access_loggers/open_telemetry/grpc_access_log_impl_test.cc
+++ b/test/extensions/access_loggers/open_telemetry/grpc_access_log_impl_test.cc
@@ -179,7 +179,7 @@ TEST_F(GrpcAccessLoggerCacheImplTest, LoggerCreation) {
   config.mutable_buffer_size_bytes()->set_value(BUFFER_SIZE_BYTES);
 
   GrpcAccessLoggerSharedPtr logger =
-      logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP, scope_);
+      logger_cache_.getOrCreateLogger(config, Common::GrpcAccessLoggerType::HTTP);
   grpc_access_logger_impl_test_helper_.expectStreamMessage(R"EOF(
   resource_logs:
     resource:

From 667f70274fe5002fa2f1ba0a9e76a1ae445cd409 Mon Sep 17 00:00:00 2001
From: Yuchen Dai <silentdai@gmail.com>
Date: Mon, 13 Sep 2021 16:11:56 +0000
Subject: [PATCH 2/6] comment

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
---
 source/extensions/access_loggers/grpc/config_utils.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/source/extensions/access_loggers/grpc/config_utils.cc b/source/extensions/access_loggers/grpc/config_utils.cc
index 45a3b62c4ba7..17e79b1aaeb0 100644
--- a/source/extensions/access_loggers/grpc/config_utils.cc
+++ b/source/extensions/access_loggers/grpc/config_utils.cc
@@ -16,9 +16,9 @@ getGrpcAccessLoggerCacheSingleton(Server::Configuration::CommonFactoryContext& c
       SINGLETON_MANAGER_REGISTERED_NAME(grpc_access_logger_cache), [&context] {
         auto* filter_factory_context =
             dynamic_cast<Server::Configuration::FactoryContext*>(&context);
-        // debug only: other candidates could be server factory context. The scope there is good.
-
-        FANCY_LOG(trace, "in {} access log cache is create from {}", __FUNCTION__,
+        // Note that the factory context can be server factory context. The life of the scope in
+        // server factory context is good.
+        FANCY_LOG(trace, "in {} access log cache is created from {}", __FUNCTION__,
                   filter_factory_context != nullptr ? "unsafe filter factory context"
                                                     : "safe server/listener factory context");
         FANCY_LOG(trace, "maybe unsafe scope addr = {} ", static_cast<void*>(&context.scope()));

From 915babb717114b918ea6887ffa05f8739a6ecbc2 Mon Sep 17 00:00:00 2001
From: Yuchen Dai <silentdai@gmail.com>
Date: Wed, 15 Sep 2021 07:03:50 +0000
Subject: [PATCH 3/6] add serverScope()

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
---
 envoy/server/factory_context.h                   |  5 +++++
 envoy/upstream/cluster_factory.h                 |  2 ++
 source/common/upstream/upstream_impl.cc          | 11 +++++++----
 .../access_loggers/grpc/config_utils.cc          | 16 ++--------------
 .../access_loggers/grpc/http_config.cc           |  1 -
 source/server/filter_chain_manager_impl.h        |  2 ++
 source/server/listener_impl.h                    |  2 ++
 source/server/server.h                           |  1 +
 test/mocks/server/factory_context.cc             |  1 +
 test/mocks/server/factory_context.h              |  1 +
 test/mocks/server/instance.cc                    |  1 +
 test/mocks/server/instance.h                     |  1 +
 test/mocks/server/listener_factory_context.cc    |  1 +
 test/mocks/server/listener_factory_context.h     |  1 +
 14 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/envoy/server/factory_context.h b/envoy/server/factory_context.h
index 3624f32fe95b..44fdc8400c1d 100644
--- a/envoy/server/factory_context.h
+++ b/envoy/server/factory_context.h
@@ -89,6 +89,11 @@ class FactoryContextBase {
    */
   virtual Stats::Scope& scope() PURE;
 
+  /**
+   * @return Stats::Scope& the server wide stats scope.
+   */
+  virtual Stats::Scope& serverScope() PURE;
+
   /**
    * @return ThreadLocal::SlotAllocator& the thread local storage engine for the server. This is
    *         used to allow runtime lockless updates to configuration, etc. across multiple threads.
diff --git a/envoy/upstream/cluster_factory.h b/envoy/upstream/cluster_factory.h
index 9440e374c0ed..a6f5d70e6132 100644
--- a/envoy/upstream/cluster_factory.h
+++ b/envoy/upstream/cluster_factory.h
@@ -75,6 +75,8 @@ class ClusterFactoryContext : public Server::Configuration::FactoryContextBase {
 
   // Server::Configuration::FactoryContextBase
   Stats::Scope& scope() override { return stats(); }
+
+  Stats::Scope& serverScope() override { return stats(); }
 };
 
 /**
diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc
index 2eec93be7891..e5a373a76caf 100644
--- a/source/common/upstream/upstream_impl.cc
+++ b/source/common/upstream/upstream_impl.cc
@@ -676,10 +676,11 @@ class FactoryContextImpl : public Server::Configuration::CommonFactoryContext {
   // other contexts taken from TransportSocketFactoryContext.
   FactoryContextImpl(Stats::Scope& stats_scope, Envoy::Runtime::Loader& runtime,
                      Server::Configuration::TransportSocketFactoryContext& c)
-      : admin_(c.admin()), stats_scope_(stats_scope), cluster_manager_(c.clusterManager()),
-        local_info_(c.localInfo()), dispatcher_(c.dispatcher()), runtime_(runtime),
-        singleton_manager_(c.singletonManager()), tls_(c.threadLocal()), api_(c.api()),
-        options_(c.options()), message_validation_visitor_(c.messageValidationVisitor()) {}
+      : admin_(c.admin()), server_scope_(c.stats()), stats_scope_(stats_scope),
+        cluster_manager_(c.clusterManager()), local_info_(c.localInfo()),
+        dispatcher_(c.dispatcher()), runtime_(runtime), singleton_manager_(c.singletonManager()),
+        tls_(c.threadLocal()), api_(c.api()), options_(c.options()),
+        message_validation_visitor_(c.messageValidationVisitor()) {}
 
   Upstream::ClusterManager& clusterManager() override { return cluster_manager_; }
   Event::Dispatcher& dispatcher() override { return dispatcher_; }
@@ -687,6 +688,7 @@ class FactoryContextImpl : public Server::Configuration::CommonFactoryContext {
   const LocalInfo::LocalInfo& localInfo() const override { return local_info_; }
   Envoy::Runtime::Loader& runtime() override { return runtime_; }
   Stats::Scope& scope() override { return stats_scope_; }
+  Stats::Scope& serverScope() override { return server_scope_; }
   Singleton::Manager& singletonManager() override { return singleton_manager_; }
   ThreadLocal::SlotAllocator& threadLocal() override { return tls_; }
   Server::Admin& admin() override { return admin_; }
@@ -719,6 +721,7 @@ class FactoryContextImpl : public Server::Configuration::CommonFactoryContext {
 
 private:
   Server::Admin& admin_;
+  Stats::Scope& server_scope_;
   Stats::Scope& stats_scope_;
   Upstream::ClusterManager& cluster_manager_;
   const LocalInfo::LocalInfo& local_info_;
diff --git a/source/extensions/access_loggers/grpc/config_utils.cc b/source/extensions/access_loggers/grpc/config_utils.cc
index 17e79b1aaeb0..e74a2892a826 100644
--- a/source/extensions/access_loggers/grpc/config_utils.cc
+++ b/source/extensions/access_loggers/grpc/config_utils.cc
@@ -14,21 +14,9 @@ GrpcCommon::GrpcAccessLoggerCacheSharedPtr
 getGrpcAccessLoggerCacheSingleton(Server::Configuration::CommonFactoryContext& context) {
   return context.singletonManager().getTyped<GrpcCommon::GrpcAccessLoggerCacheImpl>(
       SINGLETON_MANAGER_REGISTERED_NAME(grpc_access_logger_cache), [&context] {
-        auto* filter_factory_context =
-            dynamic_cast<Server::Configuration::FactoryContext*>(&context);
-        // Note that the factory context can be server factory context. The life of the scope in
-        // server factory context is good.
-        FANCY_LOG(trace, "in {} access log cache is created from {}", __FUNCTION__,
-                  filter_factory_context != nullptr ? "unsafe filter factory context"
-                                                    : "safe server/listener factory context");
-        FANCY_LOG(trace, "maybe unsafe scope addr = {} ", static_cast<void*>(&context.scope()));
-
-        auto& scope = filter_factory_context == nullptr
-                          ? context.scope()
-                          : filter_factory_context->getServerFactoryContext().scope();
         return std::make_shared<GrpcCommon::GrpcAccessLoggerCacheImpl>(
-            context.clusterManager().grpcAsyncClientManager(), scope, context.threadLocal(),
-            context.localInfo());
+            context.clusterManager().grpcAsyncClientManager(), context.serverScope(),
+            context.threadLocal(), context.localInfo());
       });
 }
 } // namespace GrpcCommon
diff --git a/source/extensions/access_loggers/grpc/http_config.cc b/source/extensions/access_loggers/grpc/http_config.cc
index eda1f0c30214..5d3b79510067 100644
--- a/source/extensions/access_loggers/grpc/http_config.cc
+++ b/source/extensions/access_loggers/grpc/http_config.cc
@@ -27,7 +27,6 @@ AccessLog::InstanceSharedPtr HttpGrpcAccessLogFactory::createAccessLogInstance(
       const envoy::extensions::access_loggers::grpc::v3::HttpGrpcAccessLogConfig&>(
       config, context.messageValidationVisitor());
 
-  
   const auto service_config = proto_config.common_config().grpc_service();
   if (service_config.has_envoy_grpc()) {
     context.clusterManager().checkActiveStaticCluster(service_config.envoy_grpc().cluster_name());
diff --git a/source/server/filter_chain_manager_impl.h b/source/server/filter_chain_manager_impl.h
index 8218e8977712..181b13c174f4 100644
--- a/source/server/filter_chain_manager_impl.h
+++ b/source/server/filter_chain_manager_impl.h
@@ -66,6 +66,7 @@ class PerFilterChainFactoryContextImpl : public Configuration::FilterChainFactor
   const LocalInfo::LocalInfo& localInfo() const override;
   Envoy::Runtime::Loader& runtime() override;
   Stats::Scope& scope() override;
+  Stats::Scope& serverScope() override { return parent_context_.serverScope(); }
   Singleton::Manager& singletonManager() override;
   OverloadManager& overloadManager() override;
   ThreadLocal::SlotAllocator& threadLocal() override;
@@ -151,6 +152,7 @@ class FactoryContextImpl : public Configuration::FactoryContext {
   const LocalInfo::LocalInfo& localInfo() const override;
   Envoy::Runtime::Loader& runtime() override;
   Stats::Scope& scope() override;
+  Stats::Scope& serverScope() override { return server_.stats(); }
   Singleton::Manager& singletonManager() override;
   OverloadManager& overloadManager() override;
   ThreadLocal::SlotAllocator& threadLocal() override;
diff --git a/source/server/listener_impl.h b/source/server/listener_impl.h
index 0114ff9c9e33..0b80babc587c 100644
--- a/source/server/listener_impl.h
+++ b/source/server/listener_impl.h
@@ -114,6 +114,7 @@ class ListenerFactoryContextBaseImpl final : public Configuration::FactoryContex
   Init::Manager& initManager() override;
   const LocalInfo::LocalInfo& localInfo() const override;
   Envoy::Runtime::Loader& runtime() override;
+  Stats::Scope& serverScope() override { return server_.stats(); }
   Stats::Scope& scope() override;
   Singleton::Manager& singletonManager() override;
   OverloadManager& overloadManager() override;
@@ -188,6 +189,7 @@ class PerListenerFactoryContextImpl : public Configuration::ListenerFactoryConte
   const LocalInfo::LocalInfo& localInfo() const override;
   Envoy::Runtime::Loader& runtime() override;
   Stats::Scope& scope() override;
+  Stats::Scope& serverScope() override { return listener_factory_context_base_->serverScope(); }
   Singleton::Manager& singletonManager() override;
   OverloadManager& overloadManager() override;
   ThreadLocal::Instance& threadLocal() override;
diff --git a/source/server/server.h b/source/server/server.h
index 9234f6c2ae7b..17370ee4c34f 100644
--- a/source/server/server.h
+++ b/source/server/server.h
@@ -182,6 +182,7 @@ class ServerFactoryContextImpl : public Configuration::ServerFactoryContext,
   }
   Envoy::Runtime::Loader& runtime() override { return server_.runtime(); }
   Stats::Scope& scope() override { return *server_scope_; }
+  Stats::Scope& serverScope() override { return *server_scope_; }
   Singleton::Manager& singletonManager() override { return server_.singletonManager(); }
   ThreadLocal::Instance& threadLocal() override { return server_.threadLocal(); }
   Admin& admin() override { return server_.admin(); }
diff --git a/test/mocks/server/factory_context.cc b/test/mocks/server/factory_context.cc
index c8a3e082414c..fa9f55d5f890 100644
--- a/test/mocks/server/factory_context.cc
+++ b/test/mocks/server/factory_context.cc
@@ -29,6 +29,7 @@ MockFactoryContext::MockFactoryContext()
   ON_CALL(*this, localInfo()).WillByDefault(ReturnRef(local_info_));
   ON_CALL(*this, runtime()).WillByDefault(ReturnRef(runtime_loader_));
   ON_CALL(*this, scope()).WillByDefault(ReturnRef(scope_));
+  ON_CALL(*this, serverScope()).WillByDefault(ReturnRef(scope_));
   ON_CALL(*this, singletonManager()).WillByDefault(ReturnRef(*singleton_manager_));
   ON_CALL(*this, threadLocal()).WillByDefault(ReturnRef(thread_local_));
   ON_CALL(*this, admin()).WillByDefault(ReturnRef(admin_));
diff --git a/test/mocks/server/factory_context.h b/test/mocks/server/factory_context.h
index ec7e6f8659ef..4ea02a143034 100644
--- a/test/mocks/server/factory_context.h
+++ b/test/mocks/server/factory_context.h
@@ -33,6 +33,7 @@ class MockFactoryContext : public virtual FactoryContext {
   MOCK_METHOD(ServerLifecycleNotifier&, lifecycleNotifier, ());
   MOCK_METHOD(Envoy::Runtime::Loader&, runtime, ());
   MOCK_METHOD(Stats::Scope&, scope, ());
+  MOCK_METHOD(Stats::Scope&, serverScope, ());
   MOCK_METHOD(Singleton::Manager&, singletonManager, ());
   MOCK_METHOD(OverloadManager&, overloadManager, ());
   MOCK_METHOD(ThreadLocal::Instance&, threadLocal, ());
diff --git a/test/mocks/server/instance.cc b/test/mocks/server/instance.cc
index f19f81dc4afc..54b7c1390388 100644
--- a/test/mocks/server/instance.cc
+++ b/test/mocks/server/instance.cc
@@ -66,6 +66,7 @@ MockServerFactoryContext::MockServerFactoryContext()
   ON_CALL(*this, localInfo()).WillByDefault(ReturnRef(local_info_));
   ON_CALL(*this, runtime()).WillByDefault(ReturnRef(runtime_loader_));
   ON_CALL(*this, scope()).WillByDefault(ReturnRef(scope_));
+  ON_CALL(*this, serverScope()).WillByDefault(ReturnRef(scope_));
   ON_CALL(*this, singletonManager()).WillByDefault(ReturnRef(*singleton_manager_));
   ON_CALL(*this, threadLocal()).WillByDefault(ReturnRef(thread_local_));
   ON_CALL(*this, admin()).WillByDefault(ReturnRef(admin_));
diff --git a/test/mocks/server/instance.h b/test/mocks/server/instance.h
index 455e82eebfec..575ee23e3537 100644
--- a/test/mocks/server/instance.h
+++ b/test/mocks/server/instance.h
@@ -155,6 +155,7 @@ class MockServerFactoryContext : public virtual ServerFactoryContext {
   MOCK_METHOD(const LocalInfo::LocalInfo&, localInfo, (), (const));
   MOCK_METHOD(Envoy::Runtime::Loader&, runtime, ());
   MOCK_METHOD(Stats::Scope&, scope, ());
+  MOCK_METHOD(Stats::Scope&, serverScope, ());
   MOCK_METHOD(Singleton::Manager&, singletonManager, ());
   MOCK_METHOD(ThreadLocal::Instance&, threadLocal, ());
   MOCK_METHOD(Server::Admin&, admin, ());
diff --git a/test/mocks/server/listener_factory_context.cc b/test/mocks/server/listener_factory_context.cc
index bf2cc8992247..bf2e1070f8c5 100644
--- a/test/mocks/server/listener_factory_context.cc
+++ b/test/mocks/server/listener_factory_context.cc
@@ -28,6 +28,7 @@ MockListenerFactoryContext::MockListenerFactoryContext()
   ON_CALL(*this, random()).WillByDefault(ReturnRef(random_));
   ON_CALL(*this, runtime()).WillByDefault(ReturnRef(runtime_loader_));
   ON_CALL(*this, scope()).WillByDefault(ReturnRef(scope_));
+  ON_CALL(*this, serverScope()).WillByDefault(ReturnRef(scope_));
   ON_CALL(*this, singletonManager()).WillByDefault(ReturnRef(*singleton_manager_));
   ON_CALL(*this, threadLocal()).WillByDefault(ReturnRef(thread_local_));
   ON_CALL(*this, admin()).WillByDefault(ReturnRef(admin_));
diff --git a/test/mocks/server/listener_factory_context.h b/test/mocks/server/listener_factory_context.h
index 095aad5931dc..ed944c117a84 100644
--- a/test/mocks/server/listener_factory_context.h
+++ b/test/mocks/server/listener_factory_context.h
@@ -35,6 +35,7 @@ class MockListenerFactoryContext : public ListenerFactoryContext {
   MOCK_METHOD(Envoy::Random::RandomGenerator&, random, ());
   MOCK_METHOD(Envoy::Runtime::Loader&, runtime, ());
   MOCK_METHOD(Stats::Scope&, scope, ());
+  MOCK_METHOD(Stats::Scope&, serverScope, ());
   MOCK_METHOD(Singleton::Manager&, singletonManager, ());
   MOCK_METHOD(OverloadManager&, overloadManager, ());
   MOCK_METHOD(ThreadLocal::Instance&, threadLocal, ());

From 949689f7fd8280b035bf6993aa3b65c7e6cfa967 Mon Sep 17 00:00:00 2001
From: Yuchen Dai <silentdai@gmail.com>
Date: Fri, 17 Sep 2021 17:59:06 +0000
Subject: [PATCH 4/6] fix build

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
---
 source/server/factory_context_base_impl.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/source/server/factory_context_base_impl.h b/source/server/factory_context_base_impl.h
index 4b56a110ccf2..51632fe47e00 100644
--- a/source/server/factory_context_base_impl.h
+++ b/source/server/factory_context_base_impl.h
@@ -30,6 +30,7 @@ class FactoryContextBaseImpl : public Configuration::FactoryContextBase {
     return validation_visitor_;
   };
   Stats::Scope& scope() override { return scope_; };
+  Stats::Scope& serverScope() override { return scope_; }
   ThreadLocal::SlotAllocator& threadLocal() override { return thread_local_; };
 
 private:

From caa4d4e220eabf4f8c780c17ccdbb9e89abbfea2 Mon Sep 17 00:00:00 2001
From: Yuchen Dai <silentdai@gmail.com>
Date: Thu, 23 Sep 2021 22:00:41 +0000
Subject: [PATCH 5/6] add integration test

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
---
 test/integration/BUILD                        |  1 +
 .../multiplexed_upstream_integration_test.cc  |  2 -
 test/integration/xds_integration_test.cc      | 53 +++++++++++++++++++
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/test/integration/BUILD b/test/integration/BUILD
index 08e2cf881426..b5ba86740e41 100644
--- a/test/integration/BUILD
+++ b/test/integration/BUILD
@@ -1445,6 +1445,7 @@ envoy_cc_test(
     deps = [
         ":http_integration_lib",
         ":http_protocol_integration_lib",
+        "//source/extensions/access_loggers/grpc:http_config",
         "//source/extensions/filters/listener/tls_inspector:config",
         "//source/extensions/filters/listener/tls_inspector:tls_inspector_lib",
         "//source/extensions/filters/network/tcp_proxy:config",
diff --git a/test/integration/multiplexed_upstream_integration_test.cc b/test/integration/multiplexed_upstream_integration_test.cc
index 6dedddbdea61..64ce3b6e63f0 100644
--- a/test/integration/multiplexed_upstream_integration_test.cc
+++ b/test/integration/multiplexed_upstream_integration_test.cc
@@ -543,8 +543,6 @@ TEST_P(Http2UpstreamIntegrationTest, ConfigureHttpOverGrpcLogs) {
   config_helper_.addConfigModifier(
       [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
               hcm) -> void {
-        const std::string access_log_name =
-            TestEnvironment::temporaryPath(TestUtility::uniqueFilename());
         // Configure just enough of an upstream access log to reference the upstream headers.
         const std::string yaml_string = R"EOF(
 name: router
diff --git a/test/integration/xds_integration_test.cc b/test/integration/xds_integration_test.cc
index 7a11e6a15647..60396572788b 100644
--- a/test/integration/xds_integration_test.cc
+++ b/test/integration/xds_integration_test.cc
@@ -597,6 +597,59 @@ TEST_P(LdsIntegrationTest, NewListenerWithBadPostListenSocketOption) {
   test_server_->waitForCounterGe("listener_manager.listener_create_failure", 1);
 }
 
+// Verify the grpc cached logger is available after the initial logger filter is destroyed.
+TEST_P(LdsIntegrationTest, GrpcLoggerSurvivesAfterReloadConfig) {
+  autonomous_upstream_ = true;
+  const std::string grpc_logger_string = R"EOF(
+    name: grpc_accesslog
+    typed_config:
+      "@type": type.googleapis.com/envoy.extensions.access_loggers.grpc.v3.HttpGrpcAccessLogConfig
+      common_config:
+        log_name: bar
+        transport_api_version: V3
+        grpc_service:
+          envoy_grpc:
+            cluster_name: cluster_0
+  )EOF";
+
+  config_helper_.addConfigModifier(
+      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+              hcm) { TestUtility::loadFromYaml(grpc_logger_string, *hcm.add_access_log()); });
+  initialize();
+  // Given we're using LDS in this test, initialize() will not complete until
+  // the initial LDS file has loaded.
+  EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value());
+
+  // HTTP 1.0 is disabled by default.
+  std::string response;
+  sendRawHttpAndWaitForResponse(lookupPort("http"), "GET / HTTP/1.0\r\n\r\n", &response, true);
+  EXPECT_TRUE(response.find("HTTP/1.1 426 Upgrade Required\r\n") == 0);
+
+  test_server_->waitForCounterGe("access_logs.grpc_access_log.logs_written", 1);
+
+  // Create a new config with HTTP/1.0 proxying.
+  ConfigHelper new_config_helper(
+      version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap()));
+  new_config_helper.addConfigModifier(
+      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+              hcm) {
+        hcm.mutable_http_protocol_options()->set_accept_http_10(true);
+        hcm.mutable_http_protocol_options()->set_default_host_for_http_10("default.com");
+        TestUtility::loadFromYaml(grpc_logger_string, *hcm.add_access_log());
+      });
+
+  // Create an LDS response with the new config, and reload config.
+  new_config_helper.setLds("1");
+  test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 1);
+  test_server_->waitForCounterGe("listener_manager.lds.update_success", 2);
+
+  // HTTP 1.0 should now be enabled.
+  std::string response2;
+  sendRawHttpAndWaitForResponse(lookupPort("http"), "GET / HTTP/1.0\r\n\r\n", &response2, false);
+  EXPECT_THAT(response2, HasSubstr("HTTP/1.0 200 OK\r\n"));
+  test_server_->waitForCounterGe("access_logs.grpc_access_log.logs_written", 2);
+}
+
 // Sample test making sure our config framework informs on listener failure.
 TEST_P(LdsIntegrationTest, FailConfigLoad) {
   config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {

From a5e81e55a843f7d1c8197ab20ba38ecc71ca054b Mon Sep 17 00:00:00 2001
From: Yuchen Dai <silentdai@gmail.com>
Date: Tue, 28 Sep 2021 05:27:32 +0000
Subject: [PATCH 6/6] amend test case

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
---
 test/integration/base_integration_test.h |  6 ++-
 test/integration/xds_integration_test.cc | 49 ++++++++++++++++++------
 2 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/test/integration/base_integration_test.h b/test/integration/base_integration_test.h
index d629b0572ff3..71ebbb5628e3 100644
--- a/test/integration/base_integration_test.h
+++ b/test/integration/base_integration_test.h
@@ -253,8 +253,10 @@ class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> {
    *
    * @param port the port to connect to.
    * @param raw_http the data to send.
-   * @param response the response data will be sent here
-   * @param if the connection should be terminated once '\r\n\r\n' has been read.
+   * @param response the response data will be sent here.
+   * @param disconnect_after_headers_complete if the connection should be terminated once "\r\n\r\n"
+   *        has been read.
+   * @param transport_socket the transport socket of the created client connection.
    **/
   void sendRawHttpAndWaitForResponse(int port, const char* raw_http, std::string* response,
                                      bool disconnect_after_headers_complete = false,
diff --git a/test/integration/xds_integration_test.cc b/test/integration/xds_integration_test.cc
index 60396572788b..f0a0dec1f52e 100644
--- a/test/integration/xds_integration_test.cc
+++ b/test/integration/xds_integration_test.cc
@@ -598,8 +598,12 @@ TEST_P(LdsIntegrationTest, NewListenerWithBadPostListenSocketOption) {
 }
 
 // Verify the grpc cached logger is available after the initial logger filter is destroyed.
+// Regression test for https://github.com/envoyproxy/envoy/issues/18066
 TEST_P(LdsIntegrationTest, GrpcLoggerSurvivesAfterReloadConfig) {
   autonomous_upstream_ = true;
+  // The grpc access logger connection never closes. It's ok to see an incomplete logging stream.
+  autonomous_allow_incomplete_streams_ = true;
+
   const std::string grpc_logger_string = R"EOF(
     name: grpc_accesslog
     typed_config:
@@ -612,6 +616,10 @@ TEST_P(LdsIntegrationTest, GrpcLoggerSurvivesAfterReloadConfig) {
             cluster_name: cluster_0
   )EOF";
 
+  config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
+    auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0);
+    listener->set_stat_prefix("listener_0");
+  });
   config_helper_.addConfigModifier(
       [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
               hcm) { TestUtility::loadFromYaml(grpc_logger_string, *hcm.add_access_log()); });
@@ -620,14 +628,23 @@ TEST_P(LdsIntegrationTest, GrpcLoggerSurvivesAfterReloadConfig) {
   // the initial LDS file has loaded.
   EXPECT_EQ(1, test_server_->counter("listener_manager.lds.update_success")->value());
 
-  // HTTP 1.0 is disabled by default.
+  // HTTP 1.1 is allowed and the connection is kept open until the listener update.
   std::string response;
-  sendRawHttpAndWaitForResponse(lookupPort("http"), "GET / HTTP/1.0\r\n\r\n", &response, true);
-  EXPECT_TRUE(response.find("HTTP/1.1 426 Upgrade Required\r\n") == 0);
-
-  test_server_->waitForCounterGe("access_logs.grpc_access_log.logs_written", 1);
-
-  // Create a new config with HTTP/1.0 proxying.
+  auto connection =
+      createConnectionDriver(lookupPort("http"), "GET / HTTP/1.1\r\nHost: host\r\n\r\n",
+                             [&response, &dispatcher = *dispatcher_](
+                                 Network::ClientConnection&, const Buffer::Instance& data) -> void {
+                               response.append(data.toString());
+                               if (response.find("\r\n\r\n") != std::string::npos) {
+                                 dispatcher.exit();
+                               }
+                             });
+  connection->run();
+  EXPECT_TRUE(response.find("HTTP/1.1 200") == 0);
+
+  test_server_->waitForCounterEq("access_logs.grpc_access_log.logs_written", 1);
+
+  // Create a new config with HTTP/1.0 proxying. The goal is to trigger a listener update.
   ConfigHelper new_config_helper(
       version_, *api_, MessageUtil::getJsonStringFromMessageOrDie(config_helper_.bootstrap()));
   new_config_helper.addConfigModifier(
@@ -635,19 +652,27 @@ TEST_P(LdsIntegrationTest, GrpcLoggerSurvivesAfterReloadConfig) {
               hcm) {
         hcm.mutable_http_protocol_options()->set_accept_http_10(true);
         hcm.mutable_http_protocol_options()->set_default_host_for_http_10("default.com");
-        TestUtility::loadFromYaml(grpc_logger_string, *hcm.add_access_log());
       });
 
   // Create an LDS response with the new config, and reload config.
   new_config_helper.setLds("1");
   test_server_->waitForCounterGe("listener_manager.listener_in_place_updated", 1);
-  test_server_->waitForCounterGe("listener_manager.lds.update_success", 2);
+  test_server_->waitForCounterEq("listener_manager.lds.update_success", 2);
 
-  // HTTP 1.0 should now be enabled.
+  // Wait until the http 1.1 connection is destroyed due to the listener update. It indicates the
+  // listener starts draining.
+  test_server_->waitForGaugeEq("listener.listener_0.downstream_cx_active", 0);
+  // Wait until all the draining filter chain is gone. It indicates the old listener and filter
+  // chains are destroyed.
+  test_server_->waitForGaugeEq("listener_manager.total_filter_chains_draining", 0);
+
+  // Verify that the new listener config is applied.
   std::string response2;
-  sendRawHttpAndWaitForResponse(lookupPort("http"), "GET / HTTP/1.0\r\n\r\n", &response2, false);
+  sendRawHttpAndWaitForResponse(lookupPort("http"), "GET / HTTP/1.0\r\n\r\n", &response2, true);
   EXPECT_THAT(response2, HasSubstr("HTTP/1.0 200 OK\r\n"));
-  test_server_->waitForCounterGe("access_logs.grpc_access_log.logs_written", 2);
+
+  // Verify that the grpc access logger is available after the listener update.
+  test_server_->waitForCounterEq("access_logs.grpc_access_log.logs_written", 2);
 }
 
 // Sample test making sure our config framework informs on listener failure.