-
Notifications
You must be signed in to change notification settings - Fork 94
Schedule option #573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Schedule option #573
Changes from 4 commits
bc0b643
b6af281
6657217
e8de757
69410ab
9b419c0
fcc37e9
a286055
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,12 @@ | |
| #include <memory> | ||
|
|
||
| #include "envoy/common/pure.h" | ||
| #include "envoy/common/time.h" | ||
|
|
||
| #include "nighthawk/common/statistic.h" | ||
|
|
||
| #include "absl/types/optional.h" | ||
|
|
||
| namespace Nighthawk { | ||
| namespace Client { | ||
|
|
||
|
|
@@ -26,7 +29,8 @@ class OutputCollector { | |
| */ | ||
| virtual void addResult(absl::string_view name, const std::vector<StatisticPtr>& statistics, | ||
| const std::map<std::string, uint64_t>& counters, | ||
| const std::chrono::nanoseconds execution_duration) PURE; | ||
| const std::chrono::nanoseconds execution_duration, | ||
| const absl::optional<Envoy::SystemTime>& first_acquisition_time) PURE; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add comment for first_acquisition_time?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will likely be addressed when the comment is added, but I'm not sure i understand what acquisition means in this context. |
||
| /** | ||
| * Directly sets the output value. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not actionable, just making sure I'm understanding - this is where we're moving back to MonotonicTime to prevent #569 ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that is right. |
||
| }; | ||
|
|
||
| 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; | ||
| }; | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,12 @@ class RateLimiter { | |
| * @return Envoy::TimeSource& time_source used to track time. | ||
| */ | ||
| virtual Envoy::TimeSource& timeSource() PURE; | ||
|
|
||
| /** | ||
| * @return absl::optional<Envoy::SystemTime> Time of the first acquisition, if any. | ||
| */ | ||
| virtual absl::optional<Envoy::SystemTime> firstAcquisitionTime() const PURE; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might have just missed it in this pull request (if so, please point me to the right file), but while I'm seeing code that writes this field or passes it around, I'm not finding where it's actually used functionally. What is its intended purpose?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This gets consumed over at https://github.com/envoyproxy/nighthawk/pull/573/files#diff-e528c3973715873e2ec7fcac6a842e5d9a59d6523a77bf1219d295dd4c7285cdR539 and ends up in the output as |
||
|
|
||
| /** | ||
| * @return std::chrono::nanoseconds elapsed since the first call to tryAcquireOne(). Used by some | ||
| * rate limiter implementations to compute acquisition rate. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |
| #include "external/envoy/source/server/server.h" | ||
|
|
||
| #include "absl/strings/str_replace.h" | ||
| #include "absl/types/optional.h" | ||
|
|
||
| // TODO(oschaaf): See if we can leverage a static module registration like Envoy does to avoid the | ||
| // ifdefs in this file. | ||
|
|
@@ -164,7 +165,8 @@ bool ProcessImpl::requestExecutionCancellation() { | |
| return true; | ||
| } | ||
|
|
||
| void ProcessImpl::createWorkers(const uint32_t concurrency) { | ||
| void ProcessImpl::createWorkers(const uint32_t concurrency, | ||
| const absl::optional<Envoy::SystemTime>& schedule) { | ||
| // TODO(oschaaf): Expose kMinimalDelay in configuration. | ||
| const std::chrono::milliseconds kMinimalWorkerDelay = 500ms + (concurrency * 50ms); | ||
| ASSERT(workers_.empty()); | ||
|
|
@@ -179,7 +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 | ||
|
oschaaf marked this conversation as resolved.
Outdated
|
||
| // 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 = time_system_.systemTime() + kMinimalWorkerDelay; | ||
| const Envoy::MonotonicTime monotonic_now = time_system_.monotonicTime(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Three comments here:
|
||
| const std::chrono::nanoseconds offset = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So now, batching behavior of nighthawk acts differently depending on whether or not we are using a schedule which doesn't seem right.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This computes the start of the first worker, we will still be injecting an offset between each worker below. I refactored this for clarity, hopefully it's easier to read now. |
||
| schedule.has_value() ? schedule.value() - time_system_.systemTime() : kMinimalWorkerDelay; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const Envoy::MonotonicTime first_worker_start = monotonic_now + offset; | ||
| const double inter_worker_delay_usec = | ||
| (1. / options_.requestsPerSecond()) * 1000000 / concurrency; | ||
| int worker_number = 0; | ||
|
|
@@ -445,7 +450,13 @@ void ProcessImpl::setupStatsSinks(const envoy::config::bootstrap::v3::Bootstrap& | |
| } | ||
|
|
||
| bool ProcessImpl::runInternal(OutputCollector& collector, const std::vector<UriPtr>& uris, | ||
| const UriPtr& request_source_uri, const UriPtr& tracing_uri) { | ||
| const UriPtr& request_source_uri, const UriPtr& tracing_uri, | ||
| const absl::optional<Envoy::SystemTime>& schedule) { | ||
| const Envoy::SystemTime now = time_system_.systemTime(); | ||
| if (schedule.value_or(now) < now) { | ||
| ENVOY_LOG(error, "Scheduled execution date already transpired."); | ||
| return false; | ||
| } | ||
| { | ||
| auto guard = std::make_unique<Envoy::Thread::LockGuard>(workers_lock_); | ||
| if (cancelled_) { | ||
|
|
@@ -461,7 +472,7 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const std::vector<UriP | |
| store_root_.setTagProducer(Envoy::Config::Utility::createTagProducer(bootstrap)); | ||
| } | ||
|
|
||
| createWorkers(number_of_workers); | ||
| createWorkers(number_of_workers, schedule); | ||
| tls_.registerThread(*dispatcher_, true); | ||
| store_root_.initializeThreading(*dispatcher_, tls_); | ||
| runtime_singleton_ = std::make_unique<Envoy::Runtime::ScopedLoaderSingleton>( | ||
|
|
@@ -522,15 +533,26 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const std::vector<UriP | |
|
|
||
| int i = 0; | ||
| std::chrono::nanoseconds total_execution_duration = 0ns; | ||
| absl::optional<Envoy::SystemTime> first_acquisition_time = absl::nullopt; | ||
|
|
||
| for (auto& worker : workers_) { | ||
| auto sequencer_execution_duration = worker->phase().sequencer().executionDuration(); | ||
| absl::optional<Envoy::SystemTime> worker_first_acquisition_time = | ||
| worker->phase().sequencer().rate_limiter().firstAcquisitionTime(); | ||
| if (worker_first_acquisition_time.has_value()) { | ||
| first_acquisition_time = | ||
| first_acquisition_time.has_value() | ||
| ? std::min(first_acquisition_time.value(), worker_first_acquisition_time.value()) | ||
| : worker_first_acquisition_time.value(); | ||
| } | ||
| // We don't write per-worker results if we only have a single worker, because the global | ||
| // results will be precisely the same. | ||
| if (workers_.size() > 1) { | ||
| StatisticFactoryImpl statistic_factory(options_); | ||
| collector.addResult(fmt::format("worker_{}", i), | ||
| vectorizeStatisticPtrMap(worker->statistics()), | ||
| worker->threadLocalCounterValues(), sequencer_execution_duration); | ||
| worker->threadLocalCounterValues(), sequencer_execution_duration, | ||
| worker_first_acquisition_time); | ||
| } | ||
| total_execution_duration += sequencer_execution_duration; | ||
| i++; | ||
|
|
@@ -545,7 +567,7 @@ bool ProcessImpl::runInternal(OutputCollector& collector, const std::vector<UriP | |
| store_root_, [](absl::string_view, uint64_t value) { return value > 0; }); | ||
| StatisticFactoryImpl statistic_factory(options_); | ||
| collector.addResult("global", mergeWorkerStatistics(workers_), counters, | ||
| total_execution_duration / workers_.size()); | ||
| total_execution_duration / workers_.size(), first_acquisition_time); | ||
| return counters.find("sequencer.failed_terminations") == counters.end(); | ||
| } | ||
|
|
||
|
|
@@ -585,7 +607,7 @@ bool ProcessImpl::run(OutputCollector& collector) { | |
| } | ||
|
|
||
| try { | ||
| return runInternal(collector, uris, request_source_uri, tracing_uri); | ||
| return runInternal(collector, uris, request_source_uri, tracing_uri, options_.schedule()); | ||
| } catch (Envoy::EnvoyException& ex) { | ||
| ENVOY_LOG(error, "Fatal exception: {}", ex.what()); | ||
| throw; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.