stats: Make an object for the FIlesystem (Filesystem::Instance) to ho…

…ld the stats counters. (envoyproxy#5031) Signed-off-by: Joshua Marantz <[email protected]>
lizan · Nov 26, 2018 · a59709c · a59709c
1 parent 5a287a8
commit a59709c
Show file tree

Hide file tree

Showing 43 changed files with 200 additions and 119 deletions.
diff --git a/include/envoy/api/api.h b/include/envoy/api/api.h
@@ -35,8 +35,7 @@ class Api {
    */
   virtual Filesystem::FileSharedPtr createFile(const std::string& path,
                                                Event::Dispatcher& dispatcher,
-                                               Thread::BasicLockable& lock,
-                                               Stats::Store& stats_store) PURE;
+                                               Thread::BasicLockable& lock) PURE;
 
   /**
    * @return bool whether a file exists and can be opened for read on disk.

diff --git a/source/common/access_log/access_log_manager_impl.cc b/source/common/access_log/access_log_manager_impl.cc
@@ -16,7 +16,7 @@ Filesystem::FileSharedPtr AccessLogManagerImpl::createAccessLog(const std::strin
     return access_logs_[file_name];
   }
 
-  access_logs_[file_name] = api_.createFile(file_name, dispatcher_, lock_, stats_store_);
+  access_logs_[file_name] = api_.createFile(file_name, dispatcher_, lock_);
   return access_logs_[file_name];
 }
 

diff --git a/source/common/access_log/access_log_manager_impl.h b/source/common/access_log/access_log_manager_impl.h
@@ -11,9 +11,8 @@ namespace AccessLog {
 
 class AccessLogManagerImpl : public AccessLogManager {
 public:
-  AccessLogManagerImpl(Api::Api& api, Event::Dispatcher& dispatcher, Thread::BasicLockable& lock,
-                       Stats::Store& stats_store)
-      : api_(api), dispatcher_(dispatcher), lock_(lock), stats_store_(stats_store) {}
+  AccessLogManagerImpl(Api::Api& api, Event::Dispatcher& dispatcher, Thread::BasicLockable& lock)
+      : api_(api), dispatcher_(dispatcher), lock_(lock) {}
 
   // AccessLog::AccessLogManager
   void reopen() override;
@@ -23,7 +22,6 @@ class AccessLogManagerImpl : public AccessLogManager {
   Api::Api& api_;
   Event::Dispatcher& dispatcher_;
   Thread::BasicLockable& lock_;
-  Stats::Store& stats_store_;
   std::unordered_map<std::string, Filesystem::FileSharedPtr> access_logs_;
 };
 

diff --git a/source/common/api/api_impl.cc b/source/common/api/api_impl.cc
@@ -11,17 +11,17 @@ namespace Envoy {
 namespace Api {
 
 Impl::Impl(std::chrono::milliseconds file_flush_interval_msec,
-           Thread::ThreadFactory& thread_factory)
-    : file_flush_interval_msec_(file_flush_interval_msec), thread_factory_(thread_factory) {}
+           Thread::ThreadFactory& thread_factory, Stats::Store& stats_store)
+    : thread_factory_(thread_factory),
+      file_system_(file_flush_interval_msec, thread_factory, stats_store) {}
 
 Event::DispatcherPtr Impl::allocateDispatcher(Event::TimeSystem& time_system) {
   return Event::DispatcherPtr{new Event::DispatcherImpl(time_system)};
 }
 
 Filesystem::FileSharedPtr Impl::createFile(const std::string& path, Event::Dispatcher& dispatcher,
-                                           Thread::BasicLockable& lock, Stats::Store& stats_store) {
-  return std::make_shared<Filesystem::FileImpl>(path, dispatcher, lock, stats_store, *this,
-                                                file_flush_interval_msec_);
+                                           Thread::BasicLockable& lock) {
+  return file_system_.createFile(path, dispatcher, lock);
 }
 
 bool Impl::fileExists(const std::string& path) { return Filesystem::fileExists(path); }

diff --git a/source/common/api/api_impl.h b/source/common/api/api_impl.h
@@ -8,6 +8,8 @@
 #include "envoy/filesystem/filesystem.h"
 #include "envoy/thread/thread.h"
 
+#include "common/filesystem/filesystem_impl.h"
+
 namespace Envoy {
 namespace Api {
 
@@ -16,20 +18,21 @@ namespace Api {
  */
 class Impl : public Api::Api {
 public:
-  Impl(std::chrono::milliseconds file_flush_interval_msec, Thread::ThreadFactory& thread_factory);
+  Impl(std::chrono::milliseconds file_flush_interval_msec, Thread::ThreadFactory& thread_factory,
+       Stats::Store& stats_store);
 
   // Api::Api
   Event::DispatcherPtr allocateDispatcher(Event::TimeSystem& time_system) override;
   Filesystem::FileSharedPtr createFile(const std::string& path, Event::Dispatcher& dispatcher,
-                                       Thread::BasicLockable& lock,
-                                       Stats::Store& stats_store) override;
+                                       Thread::BasicLockable& lock) override;
   bool fileExists(const std::string& path) override;
   std::string fileReadToEnd(const std::string& path) override;
   Thread::ThreadPtr createThread(std::function<void()> thread_routine) override;
+  Filesystem::Instance& fileSystem() { return file_system_; }
 
 private:
-  std::chrono::milliseconds file_flush_interval_msec_;
   Thread::ThreadFactory& thread_factory_;
+  Filesystem::Instance file_system_;
 };
 
 } // namespace Api

diff --git a/source/common/filesystem/filesystem_impl.cc b/source/common/filesystem/filesystem_impl.cc
@@ -94,18 +94,31 @@ bool illegalPath(const std::string& path) {
   }
 }
 
+Instance::Instance(std::chrono::milliseconds file_flush_interval_msec,
+                   Thread::ThreadFactory& thread_factory, Stats::Store& stats_store)
+    : file_flush_interval_msec_(file_flush_interval_msec),
+      file_stats_{FILESYSTEM_STATS(POOL_COUNTER_PREFIX(stats_store, "filesystem."),
+                                   POOL_GAUGE_PREFIX(stats_store, "filesystem."))},
+      thread_factory_(thread_factory) {}
+
+FileSharedPtr Instance::createFile(const std::string& path, Event::Dispatcher& dispatcher,
+                                   Thread::BasicLockable& lock,
+                                   std::chrono::milliseconds file_flush_interval_msec) {
+  return std::make_shared<Filesystem::FileImpl>(path, dispatcher, lock, file_stats_,
+                                                file_flush_interval_msec, thread_factory_);
+};
+
 FileImpl::FileImpl(const std::string& path, Event::Dispatcher& dispatcher,
-                   Thread::BasicLockable& lock, Stats::Store& stats_store, Api::Api& api,
-                   std::chrono::milliseconds flush_interval_msec)
+                   Thread::BasicLockable& lock, FileSystemStats& stats,
+                   std::chrono::milliseconds flush_interval_msec,
+                   Thread::ThreadFactory& thread_factory)
     : path_(path), file_lock_(lock), flush_timer_(dispatcher.createTimer([this]() -> void {
         stats_.flushed_by_timer_.inc();
         flush_event_.notifyOne();
         flush_timer_->enableTimer(flush_interval_msec_);
       })),
-      os_sys_calls_(Api::OsSysCallsSingleton::get()), api_(api),
-      flush_interval_msec_(flush_interval_msec),
-      stats_{FILESYSTEM_STATS(POOL_COUNTER_PREFIX(stats_store, "filesystem."),
-                              POOL_GAUGE_PREFIX(stats_store, "filesystem."))} {
+      os_sys_calls_(Api::OsSysCallsSingleton::get()), thread_factory_(thread_factory),
+      flush_interval_msec_(flush_interval_msec), stats_(stats) {
   open();
 }
 
@@ -250,7 +263,7 @@ void FileImpl::write(absl::string_view data) {
 }
 
 void FileImpl::createFlushStructures() {
-  flush_thread_ = api_.createThread([this]() -> void { flushThreadFunc(); });
+  flush_thread_ = thread_factory_.createThread([this]() -> void { flushThreadFunc(); });
   flush_timer_->enableTimer(flush_interval_msec_);
 }
 

diff --git a/source/common/filesystem/filesystem_impl.h b/source/common/filesystem/filesystem_impl.h
@@ -32,6 +32,44 @@ struct FileSystemStats {
 
 namespace Filesystem {
 
+/**
+ * Captures state, properties, and stats of a file-system.
+ */
+class Instance {
+public:
+  Instance(std::chrono::milliseconds file_flush_interval_msec,
+           Thread::ThreadFactory& thread_factory, Stats::Store& store);
+
+  /**
+   * Creates a file, overriding the flush-interval set in the class.
+   *
+   * @param path The path of the file to open.
+   * @param dispatcher The dispatcher used for set up timers to run flush().
+   * @param lock The lock.
+   * @param file_flush_interval_msec Number of milliseconds to delay before flushing.
+   */
+  FileSharedPtr createFile(const std::string& path, Event::Dispatcher& dispatcher,
+                           Thread::BasicLockable& lock,
+                           std::chrono::milliseconds file_flush_interval_msec);
+
+  /**
+   * Creates a file, using the default flush-interval for the class.
+   *
+   * @param path The path of the file to open.
+   * @param dispatcher The dispatcher used for set up timers to run flush().
+   * @param lock The lock.
+   */
+  FileSharedPtr createFile(const std::string& path, Event::Dispatcher& dispatcher,
+                           Thread::BasicLockable& lock) {
+    return createFile(path, dispatcher, lock, file_flush_interval_msec_);
+  }
+
+private:
+  const std::chrono::milliseconds file_flush_interval_msec_;
+  FileSystemStats file_stats_;
+  Thread::ThreadFactory& thread_factory_;
+};
+
 /**
  * @return bool whether a file exists on disk and can be opened for read.
  */
@@ -83,7 +121,8 @@ bool illegalPath(const std::string& path);
 class FileImpl : public File {
 public:
   FileImpl(const std::string& path, Event::Dispatcher& dispatcher, Thread::BasicLockable& lock,
-           Stats::Store& stats_store, Api::Api& api, std::chrono::milliseconds flush_interval_msec);
+           FileSystemStats& stats_, std::chrono::milliseconds flush_interval_msec,
+           Thread::ThreadFactory& thread_factory);
   ~FileImpl();
 
   // Filesystem::File
@@ -147,11 +186,11 @@ class FileImpl : public File {
                                             // final write to disk.
   Event::TimerPtr flush_timer_;
   Api::OsSysCalls& os_sys_calls_;
-  Api::Api& api_;
+  Thread::ThreadFactory& thread_factory_;
   const std::chrono::milliseconds flush_interval_msec_; // Time interval buffer gets flushed no
                                                         // matter if it reached the MIN_FLUSH_SIZE
                                                         // or not.
-  FileSystemStats stats_;
+  FileSystemStats& stats_;
 };
 
 } // namespace Filesystem

diff --git a/source/server/config_validation/api.cc b/source/server/config_validation/api.cc
@@ -6,8 +6,8 @@ namespace Envoy {
 namespace Api {
 
 ValidationImpl::ValidationImpl(std::chrono::milliseconds file_flush_interval_msec,
-                               Thread::ThreadFactory& thread_factory)
-    : Impl(file_flush_interval_msec, thread_factory) {}
+                               Thread::ThreadFactory& thread_factory, Stats::Store& stats_store)
+    : Impl(file_flush_interval_msec, thread_factory, stats_store) {}
 
 Event::DispatcherPtr ValidationImpl::allocateDispatcher(Event::TimeSystem& time_system) {
   return Event::DispatcherPtr{new Event::ValidationDispatcher(time_system)};

diff --git a/source/server/config_validation/api.h b/source/server/config_validation/api.h
@@ -16,7 +16,7 @@ namespace Api {
 class ValidationImpl : public Impl {
 public:
   ValidationImpl(std::chrono::milliseconds file_flush_interval_msec,
-                 Thread::ThreadFactory& thread_factory);
+                 Thread::ThreadFactory& thread_factory, Stats::Store& stats_store);
 
   Event::DispatcherPtr allocateDispatcher(Event::TimeSystem&) override;
 };

diff --git a/source/server/config_validation/server.cc b/source/server/config_validation/server.cc
@@ -43,10 +43,10 @@ ValidationInstance::ValidationInstance(Options& options, Event::TimeSystem& time
                                        ComponentFactory& component_factory,
                                        Thread::ThreadFactory& thread_factory)
     : options_(options), time_system_(time_system), stats_store_(store),
-      api_(new Api::ValidationImpl(options.fileFlushIntervalMsec(), thread_factory)),
+      api_(new Api::ValidationImpl(options.fileFlushIntervalMsec(), thread_factory, store)),
       dispatcher_(api_->allocateDispatcher(time_system)),
       singleton_manager_(new Singleton::ManagerImpl()),
-      access_log_manager_(*api_, *dispatcher_, access_log_lock, store), mutex_tracer_(nullptr) {
+      access_log_manager_(*api_, *dispatcher_, access_log_lock), mutex_tracer_(nullptr) {
   try {
     initialize(options, local_address, component_factory);
   } catch (const EnvoyException& e) {

diff --git a/source/server/server.cc b/source/server/server.cc
@@ -53,15 +53,16 @@ InstanceImpl::InstanceImpl(Options& options, Event::TimeSystem& time_system,
                            ThreadLocal::Instance& tls, Thread::ThreadFactory& thread_factory)
     : shutdown_(false), options_(options), time_system_(time_system), restarter_(restarter),
       start_time_(time(nullptr)), original_start_time_(start_time_), stats_store_(store),
-      thread_local_(tls), api_(new Api::Impl(options.fileFlushIntervalMsec(), thread_factory)),
+      thread_local_(tls),
+      api_(new Api::Impl(options.fileFlushIntervalMsec(), thread_factory, store)),
       secret_manager_(std::make_unique<Secret::SecretManagerImpl>()),
       dispatcher_(api_->allocateDispatcher(time_system)),
       singleton_manager_(new Singleton::ManagerImpl()),
       handler_(new ConnectionHandlerImpl(ENVOY_LOGGER(), *dispatcher_)),
       random_generator_(std::move(random_generator)), listener_component_factory_(*this),
       worker_factory_(thread_local_, *api_, hooks, time_system),
       dns_resolver_(dispatcher_->createDnsResolver({})),
-      access_log_manager_(*api_, *dispatcher_, access_log_lock, store), terminated_(false),
+      access_log_manager_(*api_, *dispatcher_, access_log_lock), terminated_(false),
       mutex_tracer_(options.mutexTracingEnabled() ? &Envoy::MutexTracerImpl::getOrCreateTracer()
                                                   : nullptr) {
 

diff --git a/test/common/access_log/access_log_manager_impl_test.cc b/test/common/access_log/access_log_manager_impl_test.cc
@@ -25,10 +25,10 @@ TEST(AccessLogManagerImpl, reopenAllFiles) {
 
   std::shared_ptr<Filesystem::MockFile> log1(new Filesystem::MockFile());
   std::shared_ptr<Filesystem::MockFile> log2(new Filesystem::MockFile());
-  AccessLogManagerImpl access_log_manager(api, dispatcher, lock, stats_store);
-  EXPECT_CALL(api, createFile("foo", _, _, _)).WillOnce(Return(log1));
+  AccessLogManagerImpl access_log_manager(api, dispatcher, lock);
+  EXPECT_CALL(api, createFile("foo", _, _)).WillOnce(Return(log1));
   access_log_manager.createAccessLog("foo");
-  EXPECT_CALL(api, createFile("bar", _, _, _)).WillOnce(Return(log2));
+  EXPECT_CALL(api, createFile("bar", _, _)).WillOnce(Return(log2));
   access_log_manager.createAccessLog("bar");
 
   // Make sure that getting the access log with the same name returns the same underlying file.

diff --git a/test/common/api/BUILD b/test/common/api/BUILD
@@ -13,6 +13,7 @@ envoy_cc_test(
     srcs = ["api_impl_test.cc"],
     deps = [
         "//source/common/api:api_lib",
+        "//source/common/stats:isolated_store_lib",
         "//test/test_common:environment_lib",
         "//test/test_common:utility_lib",
     ],

diff --git a/test/common/api/api_impl_test.cc b/test/common/api/api_impl_test.cc
@@ -2,6 +2,7 @@
 #include <string>
 
 #include "common/api/api_impl.h"
+#include "common/stats/isolated_store_impl.h"
 
 #include "test/test_common/environment.h"
 #include "test/test_common/utility.h"
@@ -11,20 +12,24 @@
 namespace Envoy {
 namespace Api {
 
-TEST(ApiImplTest, readFileToEnd) {
-  Impl api(std::chrono::milliseconds(1000), Thread::threadFactoryForTest());
+class ApiImplTest : public testing::Test {
+protected:
+  ApiImplTest() : api_(createApiForTest(store_)) {}
 
+  Stats::IsolatedStoreImpl store_;
+  ApiPtr api_;
+};
+
+TEST_F(ApiImplTest, readFileToEnd) {
   const std::string data = "test read To End\nWith new lines.";
   const std::string file_path = TestEnvironment::writeStringToFileForTest("test_api_envoy", data);
 
-  EXPECT_EQ(data, api.fileReadToEnd(file_path));
+  EXPECT_EQ(data, api_->fileReadToEnd(file_path));
 }
 
-TEST(ApiImplTest, fileExists) {
-  Impl api(std::chrono::milliseconds(1000), Thread::threadFactoryForTest());
-
-  EXPECT_TRUE(api.fileExists("/dev/null"));
-  EXPECT_FALSE(api.fileExists("/dev/blahblahblah"));
+TEST_F(ApiImplTest, fileExists) {
+  EXPECT_TRUE(api_->fileExists("/dev/null"));
+  EXPECT_FALSE(api_->fileExists("/dev/blahblahblah"));
 }
 
 } // namespace Api

diff --git a/test/common/event/BUILD b/test/common/event/BUILD
@@ -15,6 +15,7 @@ envoy_cc_test(
         "//source/common/api:api_lib",
         "//source/common/event:dispatcher_includes",
         "//source/common/event:dispatcher_lib",
+        "//source/common/stats:isolated_store_lib",
         "//test/mocks:common_lib",
         "//test/test_common:test_time_lib",
         "//test/test_common:utility_lib",

diff --git a/test/common/event/dispatched_thread_impl_test.cc b/test/common/event/dispatched_thread_impl_test.cc
@@ -24,13 +24,13 @@ namespace Event {
 class DispatchedThreadTest : public testing::Test {
 protected:
   DispatchedThreadTest()
-      : config_(1000, 1000, 1000, 1000), api_(Api::createApiForTest()),
+      : config_(1000, 1000, 1000, 1000), api_(Api::createApiForTest(fakestats_)),
         thread_(*api_, test_time_.timeSystem()),
         guard_dog_(fakestats_, config_, test_time_.timeSystem(), *api_) {}
 
   void SetUp() { thread_.start(guard_dog_); }
   NiceMock<Server::Configuration::MockMain> config_;
-  NiceMock<Stats::MockStore> fakestats_;
+  Stats::IsolatedStoreImpl fakestats_;
   DangerousDeprecatedTestTime test_time_;
   Api::ApiPtr api_;
   DispatchedThreadImpl thread_;

diff --git a/test/common/event/dispatcher_impl_test.cc b/test/common/event/dispatcher_impl_test.cc
@@ -5,6 +5,7 @@
 #include "common/api/api_impl.h"
 #include "common/common/lock_guard.h"
 #include "common/event/dispatcher_impl.h"
+#include "common/stats/isolated_store_impl.h"
 
 #include "test/mocks/common.h"
 #include "test/test_common/test_time.h"
@@ -61,9 +62,10 @@ TEST(DeferredDeleteTest, DeferredDelete) {
 class DispatcherImplTest : public ::testing::Test {
 protected:
   DispatcherImplTest()
-      : dispatcher_(std::make_unique<DispatcherImpl>(test_time_.timeSystem())),
+      : api_(Api::createApiForTest(stat_store_)),
+        dispatcher_(std::make_unique<DispatcherImpl>(test_time_.timeSystem())),
         work_finished_(false) {
-    dispatcher_thread_ = Thread::threadFactoryForTest().createThread([this]() {
+    dispatcher_thread_ = api_->createThread([this]() {
       // Must create a keepalive timer to keep the dispatcher from exiting.
       std::chrono::milliseconds time_interval(500);
       keepalive_timer_ = dispatcher_->createTimer(
@@ -81,6 +83,8 @@ class DispatcherImplTest : public ::testing::Test {
 
   DangerousDeprecatedTestTime test_time_;
 
+  Stats::IsolatedStoreImpl stat_store_;
+  Api::ApiPtr api_;
   Thread::ThreadPtr dispatcher_thread_;
   DispatcherPtr dispatcher_;
   Thread::MutexBasicLockable mu_;