-
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 all 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 |
---|---|---|
|
@@ -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 |
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.
Is the only benefit here then of providing the
timeSource()
to allow for convenient threading of this object across any object that needs time and already has a dispatcher reference?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.
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.