From 66572170140ff50a61fbf3238b9c7c1f3490ff30 Mon Sep 17 00:00:00 2001
From: Otto van der Schaaf <oschaaf@we-amp.com>
Date: Mon, 16 Nov 2020 18:20:20 +0100
Subject: [PATCH] Changes to address
https://github.com/envoyproxy/nighthawk/issues/569
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
---
include/nighthawk/common/factories.h | 4 ++--
source/client/client_worker_impl.cc | 2 +-
source/client/client_worker_impl.h | 2 +-
source/client/factories_impl.cc | 12 +++++-------
source/client/factories_impl.h | 4 ++--
source/client/process_impl.cc | 6 ++++--
source/common/rate_limiter_impl.cc | 8 ++++----
source/common/rate_limiter_impl.h | 4 ++--
source/common/termination_predicate_impl.cc | 4 ++--
source/common/termination_predicate_impl.h | 4 ++--
test/client_worker_test.cc | 2 +-
test/factories_test.cc | 2 +-
test/mocks/common/mock_sequencer_factory.h | 2 +-
.../common/mock_termination_predicate_factory.h | 2 +-
test/rate_limiter_test.cc | 9 +++++----
test/termination_predicate_test.cc | 2 +-
16 files changed, 35 insertions(+), 34 deletions(-)
diff --git a/include/nighthawk/common/factories.h b/include/nighthawk/common/factories.h
index 20d5eb6a6..c93097b79 100644
--- a/include/nighthawk/common/factories.h
+++ b/include/nighthawk/common/factories.h
@@ -24,7 +24,7 @@ class SequencerFactory {
const SequencerTarget& sequencer_target,
TerminationPredicatePtr&& termination_predicate,
Envoy::Stats::Scope& scope,
- const Envoy::SystemTime scheduled_starting_time) const PURE;
+ const Envoy::MonotonicTime scheduled_starting_time) const PURE;
};
class StatisticFactory {
@@ -46,7 +46,7 @@ class TerminationPredicateFactory {
virtual ~TerminationPredicateFactory() = default;
virtual TerminationPredicatePtr
create(Envoy::TimeSource& time_source, Envoy::Stats::Scope& scope,
- const Envoy::SystemTime scheduled_starting_time) const PURE;
+ const Envoy::MonotonicTime scheduled_starting_time) const PURE;
};
/**
diff --git a/source/client/client_worker_impl.cc b/source/client/client_worker_impl.cc
index a6c23542c..a5d456763 100644
--- a/source/client/client_worker_impl.cc
+++ b/source/client/client_worker_impl.cc
@@ -19,7 +19,7 @@ ClientWorkerImpl::ClientWorkerImpl(Envoy::Api::Api& api, Envoy::ThreadLocal::Ins
const SequencerFactory& sequencer_factory,
const RequestSourceFactory& request_generator_factory,
Envoy::Stats::Store& store, const int worker_number,
- const Envoy::SystemTime starting_time,
+ const Envoy::MonotonicTime starting_time,
Envoy::Tracing::HttpTracerSharedPtr& http_tracer,
const HardCodedWarmupStyle hardcoded_warmup_style)
: WorkerImpl(api, tls, store),
diff --git a/source/client/client_worker_impl.h b/source/client/client_worker_impl.h
index 0decef819..41b2660bc 100644
--- a/source/client/client_worker_impl.h
+++ b/source/client/client_worker_impl.h
@@ -33,7 +33,7 @@ class ClientWorkerImpl : public WorkerImpl, virtual public ClientWorker {
const SequencerFactory& sequencer_factory,
const RequestSourceFactory& request_generator_factory,
Envoy::Stats::Store& store, const int worker_number,
- const Envoy::SystemTime starting_time,
+ const Envoy::MonotonicTime starting_time,
Envoy::Tracing::HttpTracerSharedPtr& http_tracer,
const HardCodedWarmupStyle hardcoded_warmup_style);
StatisticPtrMap statistics() const override;
diff --git a/source/client/factories_impl.cc b/source/client/factories_impl.cc
index 01ae6c50d..2031658c4 100644
--- a/source/client/factories_impl.cc
+++ b/source/client/factories_impl.cc
@@ -65,12 +65,10 @@ BenchmarkClientPtr BenchmarkClientFactoryImpl::create(
SequencerFactoryImpl::SequencerFactoryImpl(const Options& options)
: OptionBasedFactoryImpl(options) {}
-SequencerPtr SequencerFactoryImpl::create(Envoy::TimeSource& time_source,
- Envoy::Event::Dispatcher& dispatcher,
- const SequencerTarget& sequencer_target,
- TerminationPredicatePtr&& termination_predicate,
- Envoy::Stats::Scope& scope,
- const Envoy::SystemTime scheduled_starting_time) const {
+SequencerPtr SequencerFactoryImpl::create(
+ Envoy::TimeSource& time_source, Envoy::Event::Dispatcher& dispatcher,
+ const SequencerTarget& sequencer_target, TerminationPredicatePtr&& termination_predicate,
+ Envoy::Stats::Scope& scope, const Envoy::MonotonicTime scheduled_starting_time) const {
StatisticFactoryImpl statistic_factory(options_);
Frequency frequency(options_.requestsPerSecond());
RateLimiterPtr rate_limiter = std::make_unique<ScheduledStartingRateLimiter>(
@@ -211,7 +209,7 @@ TerminationPredicateFactoryImpl::TerminationPredicateFactoryImpl(const Options&
TerminationPredicatePtr
TerminationPredicateFactoryImpl::create(Envoy::TimeSource& time_source, Envoy::Stats::Scope& scope,
- const Envoy::SystemTime scheduled_starting_time) const {
+ const Envoy::MonotonicTime scheduled_starting_time) const {
// We'll always link a predicate which checks for requests to cancel.
TerminationPredicatePtr root_predicate =
std::make_unique<StatsCounterAbsoluteThresholdTerminationPredicateImpl>(
diff --git a/source/client/factories_impl.h b/source/client/factories_impl.h
index 2c8abfd6d..2932a72bf 100644
--- a/source/client/factories_impl.h
+++ b/source/client/factories_impl.h
@@ -44,7 +44,7 @@ class SequencerFactoryImpl : public OptionBasedFactoryImpl, public SequencerFact
SequencerPtr create(Envoy::TimeSource& time_source, Envoy::Event::Dispatcher& dispatcher,
const SequencerTarget& sequencer_target,
TerminationPredicatePtr&& termination_predicate, Envoy::Stats::Scope& scope,
- const Envoy::SystemTime scheduled_starting_time) const override;
+ const Envoy::MonotonicTime scheduled_starting_time) const override;
};
class StatisticFactoryImpl : public OptionBasedFactoryImpl, public StatisticFactory {
@@ -93,7 +93,7 @@ class TerminationPredicateFactoryImpl : public OptionBasedFactoryImpl,
public:
TerminationPredicateFactoryImpl(const Options& options);
TerminationPredicatePtr create(Envoy::TimeSource& time_source, Envoy::Stats::Scope& scope,
- const Envoy::SystemTime scheduled_starting_time) const override;
+ const Envoy::MonotonicTime scheduled_starting_time) const override;
TerminationPredicate* linkConfiguredPredicates(
TerminationPredicate& last_predicate, const TerminationPredicateMap& predicates,
const TerminationPredicate::Status termination_status, Envoy::Stats::Scope& scope) const;
diff --git a/source/client/process_impl.cc b/source/client/process_impl.cc
index e65cd50f1..246dab6ea 100644
--- a/source/client/process_impl.cc
+++ b/source/client/process_impl.cc
@@ -181,8 +181,10 @@ void ProcessImpl::createWorkers(const uint32_t concurrency,
// TODO(oschaaf): Arguably, this ought to be the job of a rate limiter with awareness of the
// global status quo, which we do not have right now. This has been noted in the
// track-for-future issue.
- const auto first_worker_start =
- schedule.value_or(time_system_.systemTime() + kMinimalWorkerDelay);
+ const Envoy::MonotonicTime monotonic_now = time_system_.monotonicTime();
+ const std::chrono::nanoseconds offset =
+ schedule.has_value() ? schedule.value() - time_system_.systemTime() : kMinimalWorkerDelay;
+ const Envoy::MonotonicTime first_worker_start = monotonic_now + offset;
const double inter_worker_delay_usec =
(1. / options_.requestsPerSecond()) * 1000000 / concurrency;
int worker_number = 0;
diff --git a/source/common/rate_limiter_impl.cc b/source/common/rate_limiter_impl.cc
index 7d4aad4fc..15f1fd71b 100644
--- a/source/common/rate_limiter_impl.cc
+++ b/source/common/rate_limiter_impl.cc
@@ -53,16 +53,16 @@ void BurstingRateLimiter::releaseOne() {
}
ScheduledStartingRateLimiter::ScheduledStartingRateLimiter(
- RateLimiterPtr&& rate_limiter, const Envoy::SystemTime scheduled_starting_time)
+ RateLimiterPtr&& rate_limiter, const Envoy::MonotonicTime scheduled_starting_time)
: ForwardingRateLimiterImpl(std::move(rate_limiter)),
scheduled_starting_time_(scheduled_starting_time) {
- if (timeSource().systemTime() >= scheduled_starting_time_) {
+ if (timeSource().monotonicTime() >= scheduled_starting_time_) {
ENVOY_LOG(error, "Scheduled starting time exceeded. This may cause unintended bursty traffic.");
}
}
bool ScheduledStartingRateLimiter::tryAcquireOne() {
- if (timeSource().systemTime() < scheduled_starting_time_) {
+ if (timeSource().monotonicTime() < scheduled_starting_time_) {
aquisition_attempted_ = true;
return false;
}
@@ -76,7 +76,7 @@ bool ScheduledStartingRateLimiter::tryAcquireOne() {
}
void ScheduledStartingRateLimiter::releaseOne() {
- if (timeSource().systemTime() < scheduled_starting_time_) {
+ if (timeSource().monotonicTime() < scheduled_starting_time_) {
throw NighthawkException("Unexpected call to releaseOne()");
}
return rate_limiter_->releaseOne();
diff --git a/source/common/rate_limiter_impl.h b/source/common/rate_limiter_impl.h
index 0ddf65bbf..1d22e5a43 100644
--- a/source/common/rate_limiter_impl.h
+++ b/source/common/rate_limiter_impl.h
@@ -134,12 +134,12 @@ class ScheduledStartingRateLimiter : public ForwardingRateLimiterImpl,
* @param scheduled_starting_time The starting time
*/
ScheduledStartingRateLimiter(RateLimiterPtr&& rate_limiter,
- const Envoy::SystemTime scheduled_starting_time);
+ const Envoy::MonotonicTime scheduled_starting_time);
bool tryAcquireOne() override;
void releaseOne() override;
private:
- const Envoy::SystemTime scheduled_starting_time_;
+ const Envoy::MonotonicTime scheduled_starting_time_;
bool aquisition_attempted_{false};
};
diff --git a/source/common/termination_predicate_impl.cc b/source/common/termination_predicate_impl.cc
index d468562f8..d32f2006b 100644
--- a/source/common/termination_predicate_impl.cc
+++ b/source/common/termination_predicate_impl.cc
@@ -16,8 +16,8 @@ TerminationPredicate::Status TerminationPredicateBaseImpl::evaluateChain() {
}
TerminationPredicate::Status DurationTerminationPredicateImpl::evaluate() {
- return time_source_.systemTime() - start_ > duration_ ? TerminationPredicate::Status::TERMINATE
- : TerminationPredicate::Status::PROCEED;
+ return time_source_.monotonicTime() - start_ > duration_ ? TerminationPredicate::Status::TERMINATE
+ : TerminationPredicate::Status::PROCEED;
}
TerminationPredicate::Status StatsCounterAbsoluteThresholdTerminationPredicateImpl::evaluate() {
diff --git a/source/common/termination_predicate_impl.h b/source/common/termination_predicate_impl.h
index 9a23a8f02..c1c761345 100644
--- a/source/common/termination_predicate_impl.h
+++ b/source/common/termination_predicate_impl.h
@@ -35,13 +35,13 @@ class DurationTerminationPredicateImpl : public TerminationPredicateBaseImpl {
public:
DurationTerminationPredicateImpl(Envoy::TimeSource& time_source,
std::chrono::microseconds duration,
- const Envoy::SystemTime start)
+ const Envoy::MonotonicTime start)
: time_source_(time_source), start_(start), duration_(duration) {}
TerminationPredicate::Status evaluate() override;
private:
Envoy::TimeSource& time_source_;
- const Envoy::SystemTime start_;
+ const Envoy::MonotonicTime start_;
std::chrono::microseconds duration_;
};
diff --git a/test/client_worker_test.cc b/test/client_worker_test.cc
index 5f00cc723..8ffdf6680 100644
--- a/test/client_worker_test.cc
+++ b/test/client_worker_test.cc
@@ -118,7 +118,7 @@ TEST_F(ClientWorkerTest, BasicTest) {
auto worker = std::make_unique<ClientWorkerImpl>(
*api_, tls_, cluster_manager_ptr_, benchmark_client_factory_, termination_predicate_factory_,
sequencer_factory_, request_generator_factory_, store_, worker_number,
- time_system_.systemTime(), http_tracer_, ClientWorkerImpl::HardCodedWarmupStyle::ON);
+ time_system_.monotonicTime(), http_tracer_, ClientWorkerImpl::HardCodedWarmupStyle::ON);
worker->start();
worker->waitForCompletion();
diff --git a/test/factories_test.cc b/test/factories_test.cc
index 21cf49c9e..946ed1d45 100644
--- a/test/factories_test.cc
+++ b/test/factories_test.cc
@@ -198,7 +198,7 @@ class SequencerFactoryTest
};
auto sequencer = factory.create(api_->timeSource(), dispatcher_, dummy_sequencer_target,
std::make_unique<MockTerminationPredicate>(), stats_store_,
- time_system.systemTime() + 10ms);
+ time_system.monotonicTime() + 10ms);
EXPECT_NE(nullptr, sequencer.get());
}
};
diff --git a/test/mocks/common/mock_sequencer_factory.h b/test/mocks/common/mock_sequencer_factory.h
index 96983e24f..63c972f26 100644
--- a/test/mocks/common/mock_sequencer_factory.h
+++ b/test/mocks/common/mock_sequencer_factory.h
@@ -14,7 +14,7 @@ class MockSequencerFactory : public SequencerFactory {
const SequencerTarget& sequencer_target,
TerminationPredicatePtr&& termination_predicate,
Envoy::Stats::Scope& scope,
- const Envoy::SystemTime scheduled_starting_time));
+ const Envoy::MonotonicTime scheduled_starting_time));
};
} // namespace Nighthawk
\ No newline at end of file
diff --git a/test/mocks/common/mock_termination_predicate_factory.h b/test/mocks/common/mock_termination_predicate_factory.h
index 23aed4bf2..e37e8f128 100644
--- a/test/mocks/common/mock_termination_predicate_factory.h
+++ b/test/mocks/common/mock_termination_predicate_factory.h
@@ -12,7 +12,7 @@ class MockTerminationPredicateFactory : public TerminationPredicateFactory {
MOCK_CONST_METHOD3(create,
TerminationPredicatePtr(Envoy::TimeSource& time_source,
Envoy::Stats::Scope& scope,
- const Envoy::SystemTime scheduled_starting_time));
+ const Envoy::MonotonicTime scheduled_starting_time));
};
} // namespace Nighthawk
\ No newline at end of file
diff --git a/test/rate_limiter_test.cc b/test/rate_limiter_test.cc
index 296f88373..935cdada6 100644
--- a/test/rate_limiter_test.cc
+++ b/test/rate_limiter_test.cc
@@ -76,7 +76,8 @@ TEST_F(RateLimiterTest, ScheduledStartingRateLimiterTest) {
// scheduled delay. This should be business as usual from a functional perspective, but internally
// this rate limiter specializes on this case to log a warning message, and we want to cover that.
for (const bool starting_late : std::vector<bool>{false, true}) {
- const Envoy::SystemTime scheduled_starting_time = time_system.systemTime() + schedule_delay;
+ const Envoy::MonotonicTime scheduled_starting_time =
+ time_system.monotonicTime() + schedule_delay;
std::unique_ptr<MockRateLimiter> mock_rate_limiter = std::make_unique<MockRateLimiter>();
MockRateLimiter& unsafe_mock_rate_limiter = *mock_rate_limiter;
InSequence s;
@@ -95,7 +96,7 @@ TEST_F(RateLimiterTest, ScheduledStartingRateLimiterTest) {
}
// We should expect zero releases until it is time to start.
- while (time_system.systemTime() < scheduled_starting_time) {
+ while (time_system.monotonicTime() < scheduled_starting_time) {
EXPECT_FALSE(rate_limiter->tryAcquireOne());
time_system.advanceTimeWait(1ms);
}
@@ -108,8 +109,8 @@ TEST_F(RateLimiterTest, ScheduledStartingRateLimiterTest) {
TEST_F(RateLimiterTest, ScheduledStartingRateLimiterTestBadArgs) {
Envoy::Event::SimulatedTimeSystem time_system;
// Verify we enforce future-only scheduling.
- for (const auto& timing :
- std::vector<Envoy::SystemTime>{time_system.systemTime(), time_system.systemTime() - 10ms}) {
+ for (const auto& timing : std::vector<Envoy::MonotonicTime>{time_system.monotonicTime(),
+ time_system.monotonicTime() - 10ms}) {
std::unique_ptr<MockRateLimiter> mock_rate_limiter = std::make_unique<MockRateLimiter>();
MockRateLimiter& unsafe_mock_rate_limiter = *mock_rate_limiter;
EXPECT_CALL(unsafe_mock_rate_limiter, timeSource)
diff --git a/test/termination_predicate_test.cc b/test/termination_predicate_test.cc
index 387883152..4ab0bab53 100644
--- a/test/termination_predicate_test.cc
+++ b/test/termination_predicate_test.cc
@@ -25,7 +25,7 @@ class TerminationPredicateTest : public Test {
TEST_F(TerminationPredicateTest, DurationTerminationPredicateImplTest) {
const auto duration = 100us;
- DurationTerminationPredicateImpl pred(time_system, duration, time_system.systemTime());
+ DurationTerminationPredicateImpl pred(time_system, duration, time_system.monotonicTime());
EXPECT_EQ(pred.evaluate(), TerminationPredicate::Status::PROCEED);
// move to the edge.
time_system.advanceTimeWait(duration);