-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
test infrastructure: inject time everywhere #4180
Changes from 21 commits
da7f24a
7d59de4
10422d4
282307f
fa5c8c8
8d67f55
c9a7d62
2d9f2aa
1a84061
eb2347e
21ac418
0a2514c
c642dbc
646a500
e65a754
50dbb33
be44f2d
1488d58
44d880d
6d9fac6
944d75b
28f41dd
a285196
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 |
---|---|---|
|
@@ -32,6 +32,16 @@ class Dispatcher { | |
public: | ||
virtual ~Dispatcher() {} | ||
|
||
/** | ||
* Returns a time-source to use with this dispatcher. | ||
* | ||
* TODO(#4160) the implementations currently manage timer events that | ||
* ignore the time-source, and thus can't be mocked or faked. So it's | ||
* difficult to mock time in an integration test without mocking out | ||
* the dispatcher. | ||
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. Is the only benefit here then of providing the 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 the only benefit. I think the dispatcher needs to understand time to support timers in a manner consistent with that. I'm just not biting that off in this PR. This interface makes the injection significantly easier in this PR, and will be functionally needed in the future for the dispatcher to work properly with mocked or simulated time. |
||
*/ | ||
virtual TimeSource& timeSource() PURE; | ||
|
||
/** | ||
* Clear any items in the deferred deletion queue. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,9 +131,16 @@ class FactoryContext { | |
virtual const envoy::api::v2::core::Metadata& listenerMetadata() const PURE; | ||
|
||
/** | ||
* @return SystemTimeSource& a reference to the top-level SystemTime source. | ||
* @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. | ||
*/ | ||
virtual TimeSource& timeSource() PURE; | ||
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. Why can't this be inferred from 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. It can be, but I feel like it's reasonable from an API perspective that the FilterConfig should provide direct access to its time source, even if the impl delegates that as an implementation detail. |
||
}; | ||
|
||
class ListenerFactoryContext : public FactoryContext { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,6 +192,11 @@ class Instance { | |
*/ | ||
virtual const LocalInfo::LocalInfo& localInfo() PURE; | ||
|
||
/** | ||
* @return the time source used for the server. | ||
*/ | ||
virtual TimeSource& timeSource() PURE; | ||
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. Ditto. 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 feel that time is an essential interface property of a server, independent of whether it delegates ownership of the TimeSource itself to the dispatcher. |
||
|
||
/** | ||
* @return the flush interval of stats sinks. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -209,6 +209,11 @@ class ClusterManager { | |
addThreadLocalClusterUpdateCallbacks(ClusterUpdateCallbacks& callbacks) PURE; | ||
|
||
virtual ClusterManagerFactory& clusterManagerFactory() PURE; | ||
|
||
/** | ||
* @return TimeSource& the time-source used with the cluster manager. | ||
*/ | ||
virtual TimeSource& timeSource() PURE; | ||
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. Is time really an essential property of 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. Offhand, I am not an expert in the class; this is the first I've touched it, IIRC. However, it appears based on its API to care about load-balancing, which I think typically has a time-component implied, no? From a practical perspective, I need to find a time-source from places that already have a ClusterManager plumbed in and not much else. E.g. AsyncClientImpl IMO you don't need a lot of excuses to want to know about time. Even a pure algorithm may want to employ timeouts, etc. Something I'm not proud of here is that PerfAnnotation in this PR still needs its own prod time-source, but I can't solve all problems in this PR, and I'm not sure we have a TimeSource available everywhere I might want to measure perf. 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.
In general, I'm of the persuasion that I agree with what you write above, but that doesn't necessitate making Can you see if it's practical to avoid adding to the interface here? 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. It is practical but will enlarge the CL significantly from what I can tell. Can I suggest a compromise where I add a TODO to the interfaces where it doesn't make (IMO) obvious sense (Dispatcher, arguably Server) to remove them when we've got everything plumbed directly? I think the scary one is filters; I didn't want to change how every filter gets instantiated, preferring instead to provide access to the TimeSource via FilterConfig. Happy to remove that, but it will require changing every filter. 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. Per IM I have a follow-up in progress to remove this: jmarantz@b9a9cee |
||
}; | ||
|
||
typedef std::unique_ptr<ClusterManager> ClusterManagerPtr; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,6 +86,9 @@ class PerfAnnotationContext { | |
void record(std::chrono::nanoseconds duration, absl::string_view category, | ||
absl::string_view description); | ||
|
||
/** @return MonotonicTime the current time */ | ||
MonotonicTime currentTime() { return time_source_.currentTime(); } | ||
|
||
/** | ||
* Renders the aggregated statistics as a string. | ||
* @return std::string the performance data as a formatted string. | ||
|
@@ -138,6 +141,7 @@ class PerfAnnotationContext { | |
#else | ||
DurationStatsMap duration_stats_map_; | ||
#endif | ||
ProdMonotonicTimeSource time_source_; | ||
}; | ||
|
||
/** | ||
|
@@ -162,8 +166,8 @@ class PerfOperation { | |
void record(absl::string_view category, absl::string_view description); | ||
|
||
private: | ||
MonotonicTime start_time_; | ||
PerfAnnotationContext* context_; | ||
MonotonicTime start_time_; | ||
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. Nit: why reordering? 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. Initializing context is needed (from a lazy-init static) prior to computing the start-time. |
||
}; | ||
|
||
} // namespace Envoy | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,14 +4,15 @@ | |
|
||
namespace Envoy { | ||
|
||
TokenBucketImpl::TokenBucketImpl(uint64_t max_tokens, double fill_rate, | ||
MonotonicTimeSource& time_source) | ||
TokenBucketImpl::TokenBucketImpl(uint64_t max_tokens, MonotonicTimeSource& monotonic_time_source, | ||
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. Nit: why reordering? 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. because fill_rate is optional, but time-source is not. |
||
double fill_rate) | ||
: max_tokens_(max_tokens), fill_rate_(std::abs(fill_rate)), tokens_(max_tokens), | ||
last_fill_(time_source.currentTime()), time_source_(time_source) {} | ||
last_fill_(monotonic_time_source.currentTime()), | ||
monotonic_time_source_(monotonic_time_source) {} | ||
|
||
bool TokenBucketImpl::consume(uint64_t tokens) { | ||
if (tokens_ < max_tokens_) { | ||
const auto time_now = time_source_.currentTime(); | ||
const auto time_now = monotonic_time_source_.currentTime(); | ||
tokens_ = std::min((std::chrono::duration<double>(time_now - last_fill_).count() * fill_rate_) + | ||
tokens_, | ||
max_tokens_); | ||
|
@@ -26,4 +27,4 @@ bool TokenBucketImpl::consume(uint64_t tokens) { | |
return true; | ||
} | ||
|
||
} // namespace Envoy | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,10 +16,10 @@ class TokenBucketImpl : public TokenBucket { | |
* @param max_tokens supplies the maximun number of tokens in the bucket. | ||
* @param fill_rate supplies the number of tokens that will return to the bucket on each second. | ||
* The default is 1. | ||
* @param time_source supplies the time source. The default is ProdMonotonicTimeSource. | ||
* @param time_source supplies the time source. | ||
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. Params are now out of order.. 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 fix. 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. done |
||
*/ | ||
explicit TokenBucketImpl(uint64_t max_tokens, double fill_rate = 1, | ||
MonotonicTimeSource& time_source = ProdMonotonicTimeSource::instance_); | ||
explicit TokenBucketImpl(uint64_t max_tokens, MonotonicTimeSource& time_source, | ||
double fill_rate = 1); | ||
|
||
bool consume(uint64_t tokens = 1) override; | ||
|
||
|
@@ -28,7 +28,7 @@ class TokenBucketImpl : public TokenBucket { | |
const double fill_rate_; | ||
double tokens_; | ||
MonotonicTime last_fill_; | ||
MonotonicTimeSource& time_source_; | ||
MonotonicTimeSource& monotonic_time_source_; | ||
}; | ||
|
||
} // namespace Envoy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
@param
description.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, i guess :)