envoyproxy · jmarantz · Aug 16, 2018 · Aug 16, 2018 · Aug 16, 2018 · Aug 16, 2018
diff --git a/include/envoy/common/time.h b/include/envoy/common/time.h
@@ -14,76 +14,22 @@ namespace Envoy {
 typedef std::chrono::time_point<std::chrono::system_clock> SystemTime;
 typedef std::chrono::time_point<std::chrono::steady_clock> MonotonicTime;
 
-/**
- * Abstraction for getting the current system time. Useful for testing.
- *
- * TODO(#4160): eliminate this class and pass TimeSource everywhere.
- */
-class SystemTimeSource {
-public:
-  virtual ~SystemTimeSource() {}
-
-  /**
-   * @return the current system time.
-   */
-  virtual SystemTime currentTime() PURE;
-};
-
-/**
- * Abstraction for getting the current monotonically increasing time. Useful for
- * testing.
- *
- * TODO(#4160): eliminate this class and pass TimeSource everywhere.
- */
-class MonotonicTimeSource {
-public:
-  virtual ~MonotonicTimeSource() {}
-
-  /**
-   * @return the current monotonic time.
-   */
-  virtual MonotonicTime currentTime() PURE;
-};
-
 /**
  * Captures a system-time source, capable of computing both monotonically increasing
  * and real time.
- *
- * TODO(#4160): currently this is just a container for SystemTimeSource and
- * MonotonicTimeSource but we should clean that up and just have this as the
- * base class. Once that's done, TimeSource will be a pure interface.
  */
 class TimeSource {
 public:
-  TimeSource(SystemTimeSource& system, MonotonicTimeSource& monotonic)
-      : system_(system), monotonic_(monotonic) {}
+  virtual ~TimeSource() {}
 
   /**
    * @return the current system time; not guaranteed to be monotonically increasing.
    */
-  SystemTime systemTime() { return system_.currentTime(); }
-
+  virtual SystemTime systemTime() PURE;
   /**
    * @return the current monotonic time.
    */
-  MonotonicTime monotonicTime() { return monotonic_.currentTime(); }
-
-  /**
-   * Compares two time-sources for equality; this is needed for mocks.
-   */
-  bool operator==(const TimeSource& ts) const {
-    return &system_ == &ts.system_ && &monotonic_ == &ts.monotonic_;
-  }
-
-  // TODO(jmarantz): Eliminate these methods and the SystemTimeSource and MonotonicTimeSource
-  // classes, and change method calls to work directly off of TimeSource.
-  SystemTimeSource& system() { return system_; }
-  MonotonicTimeSource& monotonic() { return monotonic_; }
-
-private:
-  // These are pointers rather than references in order to support assignment.
-  SystemTimeSource& system_;
-  MonotonicTimeSource& monotonic_;
+  virtual MonotonicTime monotonicTime() PURE;
 };
 
 } // namespace Envoy
diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h
@@ -130,13 +130,6 @@ class FactoryContext {
    */
   virtual const envoy::api::v2::core::Metadata& listenerMetadata() const PURE;
 
-  /**
-   * @return SystemTimeSource& a reference to the system time source.
-   * TODO(#4160): This method should be eliminated, and call-sites and mocks should
-   * be converted to work with timeSource() below.
-   */
-  virtual SystemTimeSource& systemTimeSource() PURE;
-
   /**
    * @return TimeSource& a reference to the time source.
    */

diff --git a/source/common/common/perf_annotation.h b/source/common/common/perf_annotation.h
@@ -87,7 +87,7 @@ class PerfAnnotationContext {
               absl::string_view description);
 
   /** @return MonotonicTime the current time */
-  MonotonicTime currentTime() { return time_source_.currentTime(); }
+  MonotonicTime currentTime() { return time_source_.monotonicTime(); }
 
   /**
    * Renders the aggregated statistics as a string.
@@ -141,7 +141,7 @@ class PerfAnnotationContext {
 #else
   DurationStatsMap duration_stats_map_;
 #endif
-  ProdMonotonicTimeSource time_source_;
+  RealTimeSource time_source_;
 };
 
 /**

diff --git a/source/common/common/token_bucket_impl.cc b/source/common/common/token_bucket_impl.cc
@@ -4,15 +4,13 @@
 
 namespace Envoy {
 
-TokenBucketImpl::TokenBucketImpl(uint64_t max_tokens, MonotonicTimeSource& monotonic_time_source,
-                                 double fill_rate)
+TokenBucketImpl::TokenBucketImpl(uint64_t max_tokens, TimeSource& time_source, double fill_rate)
     : max_tokens_(max_tokens), fill_rate_(std::abs(fill_rate)), tokens_(max_tokens),
-      last_fill_(monotonic_time_source.currentTime()),
-      monotonic_time_source_(monotonic_time_source) {}
+      last_fill_(time_source.monotonicTime()), time_source_(time_source) {}
 
 bool TokenBucketImpl::consume(uint64_t tokens) {
   if (tokens_ < max_tokens_) {
-    const auto time_now = monotonic_time_source_.currentTime();
+    const auto time_now = time_source_.monotonicTime();
     tokens_ = std::min((std::chrono::duration<double>(time_now - last_fill_).count() * fill_rate_) +
                            tokens_,
                        max_tokens_);

diff --git a/source/common/common/token_bucket_impl.h b/source/common/common/token_bucket_impl.h
@@ -18,8 +18,7 @@ class TokenBucketImpl : public TokenBucket {
    * @param fill_rate supplies the number of tokens that will return to the bucket on each second.
    * The default is 1.
    */
-  explicit TokenBucketImpl(uint64_t max_tokens, MonotonicTimeSource& time_source,
-                           double fill_rate = 1);
+  explicit TokenBucketImpl(uint64_t max_tokens, TimeSource& time_source, double fill_rate = 1);
 
   bool consume(uint64_t tokens = 1) override;
 
@@ -28,7 +27,7 @@ class TokenBucketImpl : public TokenBucket {
   const double fill_rate_;
   double tokens_;
   MonotonicTime last_fill_;
-  MonotonicTimeSource& monotonic_time_source_;
+  TimeSource& time_source_;
 };
 
 } // namespace Envoy
diff --git a/source/common/common/utility.h b/source/common/common/utility.h
@@ -96,21 +96,13 @@ class AccessLogDateTimeFormatter {
 };
 
 /**
- * Production implementation of SystemTimeSource that returns the current time.
+ * Real-time implementation of TimeSource.
  */
-class ProdSystemTimeSource : public SystemTimeSource {
+class RealTimeSource : public TimeSource {
 public:
-  // SystemTimeSource
-  SystemTime currentTime() override { return std::chrono::system_clock::now(); }
-};
-
-/**
- * Production implementation of MonotonicTimeSource that returns the current time.
- */
-class ProdMonotonicTimeSource : public MonotonicTimeSource {
-public:
-  // MonotonicTimeSource
-  MonotonicTime currentTime() override { return std::chrono::steady_clock::now(); }
+  // TimeSource
+  SystemTime systemTime() override { return std::chrono::system_clock::now(); }
+  MonotonicTime monotonicTime() override { return std::chrono::steady_clock::now(); }
 };
 
 /**

diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc
@@ -111,11 +111,9 @@ GrpcMuxWatchPtr GrpcMuxImpl::subscribe(const std::string& type_url,
   // TODO(gsagula): move TokenBucketImpl params to a config.
   if (!api_state_[type_url].subscribed_) {
     // Bucket contains 100 tokens maximum and refills at 5 tokens/sec.
-    api_state_[type_url].limit_request_ =
-        std::make_unique<TokenBucketImpl>(100, time_source_.monotonic(), 5);
+    api_state_[type_url].limit_request_ = std::make_unique<TokenBucketImpl>(100, time_source_, 5);
     // Bucket contains 1 token maximum and refills 1 token on every ~5 seconds.
-    api_state_[type_url].limit_log_ =
-        std::make_unique<TokenBucketImpl>(1, time_source_.monotonic(), 0.2);
+    api_state_[type_url].limit_log_ = std::make_unique<TokenBucketImpl>(1, time_source_, 0.2);
     api_state_[type_url].request_.set_type_url(type_url);
     api_state_[type_url].request_.mutable_node()->MergeFrom(local_info_.node());
     api_state_[type_url].subscribed_ = true;

diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h
@@ -103,7 +103,7 @@ class GrpcMuxImpl : public GrpcMux,
   std::list<std::string> subscriptions_;
   Event::TimerPtr retry_timer_;
   Runtime::RandomGenerator& random_;
-  TimeSource time_source_;
+  TimeSource& time_source_;
   BackOffStrategyPtr backoff_strategy_;
 };
 

diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h
@@ -68,7 +68,7 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>, public Dispatcher {
     return run_tid_ == 0 || run_tid_ == Thread::Thread::currentThreadId();
   }
 
-  TimeSource time_source_;
+  TimeSource& time_source_;
   Thread::ThreadId run_tid_{};
   Buffer::WatermarkFactoryPtr buffer_factory_;
   Libevent::BasePtr base_;

diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc
@@ -44,7 +44,7 @@ StaticRouteConfigProviderImpl::StaticRouteConfigProviderImpl(
     Server::Configuration::FactoryContext& factory_context,
     RouteConfigProviderManagerImpl& route_config_provider_manager)
     : config_(new ConfigImpl(config, factory_context, true)), route_config_proto_{config},
-      last_updated_(factory_context.systemTimeSource().currentTime()),
+      last_updated_(factory_context.timeSource().systemTime()),
       route_config_provider_manager_(route_config_provider_manager) {
   route_config_provider_manager_.static_route_config_providers_.insert(this);
 }
@@ -64,8 +64,8 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription(
       scope_(factory_context.scope().createScope(stat_prefix + "rds." + route_config_name_ + ".")),
       stats_({ALL_RDS_STATS(POOL_COUNTER(*scope_))}),
       route_config_provider_manager_(route_config_provider_manager),
-      manager_identifier_(manager_identifier), time_source_(factory_context.systemTimeSource()),
-      last_updated_(factory_context.systemTimeSource().currentTime()) {
+      manager_identifier_(manager_identifier), time_source_(factory_context.timeSource()),
+      last_updated_(factory_context.timeSource().systemTime()) {
   ::Envoy::Config::Utility::checkLocalInfo("rds", factory_context.localInfo());
 
   subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource<
@@ -96,7 +96,7 @@ RdsRouteConfigSubscription::~RdsRouteConfigSubscription() {
 
 void RdsRouteConfigSubscription::onConfigUpdate(const ResourceVector& resources,
                                                 const std::string& version_info) {
-  last_updated_ = time_source_.currentTime();
+  last_updated_ = time_source_.systemTime();
 
   if (resources.empty()) {
     ENVOY_LOG(debug, "Missing RouteConfiguration for {} in onConfigUpdate()", route_config_name_);

diff --git a/source/common/router/rds_impl.h b/source/common/router/rds_impl.h
@@ -135,7 +135,7 @@ class RdsRouteConfigSubscription
   RdsStats stats_;
   RouteConfigProviderManagerImpl& route_config_provider_manager_;
   const std::string manager_identifier_;
-  SystemTimeSource& time_source_;
+  TimeSource& time_source_;
   SystemTime last_updated_;
   absl::optional<LastConfigInfo> config_info_;
   envoy::api::v2::RouteConfiguration route_config_proto_;

diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc
@@ -186,9 +186,8 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots
   if (cm_config.has_outlier_detection()) {
     const std::string event_log_file_path = cm_config.outlier_detection().event_log_path();
     if (!event_log_file_path.empty()) {
-      outlier_event_logger_.reset(new Outlier::EventLoggerImpl(
-          log_manager, event_log_file_path, dispatcher_.timeSource().system(),
-          dispatcher_.timeSource().monotonic()));
+      outlier_event_logger_.reset(
+          new Outlier::EventLoggerImpl(log_manager, event_log_file_path, dispatcher_.timeSource()));
     }
   }
 
@@ -615,7 +614,7 @@ void ClusterManagerImpl::loadCluster(const envoy::api::v2::Cluster& cluster,
   }
 
   cluster_map[cluster_reference.info()->name()] = std::make_unique<ClusterData>(
-      cluster, version_info, added_via_api, std::move(new_cluster), time_source_.system());
+      cluster, version_info, added_via_api, std::move(new_cluster), time_source_);
   const auto cluster_entry_it = cluster_map.find(cluster_reference.info()->name());
 
   // If an LB is thread aware, create it here. The LB is not initialized until cluster pre-init

diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h
@@ -330,11 +330,10 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable<Logger::Id::u
 
   struct ClusterData {
     ClusterData(const envoy::api::v2::Cluster& cluster_config, const std::string& version_info,
-                bool added_via_api, ClusterSharedPtr&& cluster,
-                SystemTimeSource& system_time_source)
+                bool added_via_api, ClusterSharedPtr&& cluster, TimeSource& time_source)
         : cluster_config_(cluster_config), config_hash_(MessageUtil::hash(cluster_config)),
           version_info_(version_info), added_via_api_(added_via_api), cluster_(std::move(cluster)),
-          last_updated_(system_time_source.currentTime()) {}
+          last_updated_(time_source.systemTime()) {}
 
     bool blockUpdate(uint64_t hash) { return !added_via_api_ || config_hash_ == hash; }
 
@@ -436,7 +435,7 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable<Logger::Id::u
   std::string local_cluster_name_;
   Grpc::AsyncClientManagerPtr async_client_manager_;
   Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_;
-  TimeSource time_source_;
+  TimeSource& time_source_;
   ClusterUpdatesMap updates_map_;
   Event::Dispatcher& dispatcher_;
 };

diff --git a/source/common/upstream/health_checker_base_impl.cc b/source/common/upstream/health_checker_base_impl.cc
@@ -287,8 +287,7 @@ void HealthCheckEventLoggerImpl::logEjectUnhealthy(
   *event.mutable_host() = std::move(address);
   event.set_cluster_name(host->cluster().name());
   event.mutable_eject_unhealthy_event()->set_failure_type(failure_type);
-  TimestampUtil::systemClockToTimestamp(system_time_source_.currentTime(),
-                                        *event.mutable_timestamp());
+  TimestampUtil::systemClockToTimestamp(time_source_.systemTime(), *event.mutable_timestamp());
   // Make sure the type enums make it into the JSON
   const auto json = MessageUtil::getJsonStringFromMessage(event, /* pretty_print */ false,
                                                           /* always_print_primitive_fields */ true);
@@ -305,8 +304,7 @@ void HealthCheckEventLoggerImpl::logAddHealthy(
   *event.mutable_host() = std::move(address);
   event.set_cluster_name(host->cluster().name());
   event.mutable_add_healthy_event()->set_first_check(first_check);
-  TimestampUtil::systemClockToTimestamp(system_time_source_.currentTime(),
-                                        *event.mutable_timestamp());
+  TimestampUtil::systemClockToTimestamp(time_source_.systemTime(), *event.mutable_timestamp());
   // Make sure the type enums make it into the JSON
   const auto json = MessageUtil::getJsonStringFromMessage(event, /* pretty_print */ false,
                                                           /* always_print_primitive_fields */ true);

diff --git a/source/common/upstream/health_checker_base_impl.h b/source/common/upstream/health_checker_base_impl.h
@@ -132,9 +132,9 @@ class HealthCheckerImplBase : public HealthChecker,
 
 class HealthCheckEventLoggerImpl : public HealthCheckEventLogger {
 public:
-  HealthCheckEventLoggerImpl(AccessLog::AccessLogManager& log_manager,
-                             SystemTimeSource& system_time_source, const std::string& file_name)
-      : system_time_source_(system_time_source), file_(log_manager.createAccessLog(file_name)) {}
+  HealthCheckEventLoggerImpl(AccessLog::AccessLogManager& log_manager, TimeSource& time_source,
+                             const std::string& file_name)
+      : time_source_(time_source), file_(log_manager.createAccessLog(file_name)) {}
 
   void logEjectUnhealthy(envoy::data::core::v2alpha::HealthCheckerType health_checker_type,
                          const HostDescriptionConstSharedPtr& host,
@@ -143,7 +143,7 @@ class HealthCheckEventLoggerImpl : public HealthCheckEventLogger {
                      const HostDescriptionConstSharedPtr& host, bool first_check) override;
 
 private:
-  SystemTimeSource& system_time_source_;
+  TimeSource& time_source_;
   Filesystem::FileSharedPtr file_;
 };
 

diff --git a/source/common/upstream/health_checker_impl.cc b/source/common/upstream/health_checker_impl.cc
@@ -50,7 +50,7 @@ HealthCheckerFactory::create(const envoy::api::v2::core::HealthCheck& hc_config,
   HealthCheckEventLoggerPtr event_logger;
   if (!hc_config.event_log_path().empty()) {
     event_logger = std::make_unique<HealthCheckEventLoggerImpl>(
-        log_manager, dispatcher.timeSource().system(), hc_config.event_log_path());
+        log_manager, dispatcher.timeSource(), hc_config.event_log_path());
   }
   switch (hc_config.health_checker_case()) {
   case envoy::api::v2::core::HealthCheck::HealthCheckerCase::kHttpHealthCheck: