Echo back and track origin request-receipt timing deltas #477
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/common/pure.h" | ||
| #include "envoy/common/time.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
||
| /** | ||
| * Interface for measuring elapsed time between events. | ||
| */ | ||
| class Stopwatch { | ||
| public: | ||
| virtual ~Stopwatch() = default; | ||
| /** | ||
| * @param time_source used to obtain a sample of the current time. | ||
| * @return uint64_t 0 on the first invocation, and the number of elapsed nanoseconds since the | ||
| * last invocation otherwise. | ||
| */ | ||
| virtual uint64_t getElapsedNsAndReset(Envoy::TimeSource& time_source) PURE; | ||
| }; | ||
|
|
||
| } // namespace Nighthawk |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| #include "common/thread_safe_monotonic_time_stopwatch.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
||
| uint64_t ThreadSafeMontonicTimeStopwatch::getElapsedNsAndReset(Envoy::TimeSource& time_source) { | ||
| Envoy::Thread::LockGuard guard(lock_); | ||
| // Note that we obtain monotonic time under lock, to ensure that start_ will be updated | ||
| // monotonically. | ||
| const Envoy::MonotonicTime new_time = time_source.monotonicTime(); | ||
| const uint64_t elapsed_ns = | ||
| start_ == Envoy::MonotonicTime::min() ? 0 : (new_time - start_).count(); | ||
| start_ = new_time; | ||
| return elapsed_ns; | ||
| } | ||
|
|
||
| } // namespace Nighthawk |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| #pragma once | ||
|
|
||
| #include "nighthawk/common/stopwatch.h" | ||
|
|
||
| #include "external/envoy/source/common/common/lock_guard.h" | ||
| #include "external/envoy/source/common/common/thread.h" | ||
|
|
||
| namespace Nighthawk { | ||
|
|
||
| /** | ||
| * Utility class for thread safe tracking of elapsed monotonic time. | ||
| * Example usage: | ||
| * | ||
| * ThreadSafeMontonicTimeStopwatch stopwatch; | ||
| * int i = 0; | ||
| * do { | ||
| * std::cerr << stopwatch.getElapsedNsAndReset() << | ||
| * "ns elapsed since last iteration." << std::endl; | ||
| * } while (++i < 100); | ||
| */ | ||
dubious90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| class ThreadSafeMontonicTimeStopwatch : public Stopwatch { | ||
| public: | ||
| /** | ||
| * Construct a new ThreadSafe & MontonicTime-based Stopwatch object. | ||
| */ | ||
| ThreadSafeMontonicTimeStopwatch() : start_(Envoy::MonotonicTime::min()) {} | ||
|
|
||
| /** | ||
| * @param time_source used to obtain a sample of the current monotonic time. | ||
| * @return uint64_t 0 on the first invocation, and the number of elapsed nanoseconds since the | ||
|
Contributor
There was a problem hiding this comment. In google-convention, we'd most likely try to use absl::duration here, rather than an int representing a unit of time. Would that be reasonable here, or no?
Member
Author
There was a problem hiding this comment. Well, while I didn't consider
Having said that, I feel this is a subjective matter, so I'd be happy to change this if you lean towards
Contributor
There was a problem hiding this comment. That makes sense. I don't know if it's worth bringing in absl::Duration just for this. If we wanted to, we could try to bring in absl::Duration on a more global change later. |
||
| * last invocation otherwise. | ||
| */ | ||
| uint64_t getElapsedNsAndReset(Envoy::TimeSource& time_source) override; | ||
|
|
||
| private: | ||
| Envoy::Thread::MutexBasicLockable lock_; | ||
| Envoy::MonotonicTime start_ GUARDED_BY(lock_); | ||
| }; | ||
|
|
||
| } // namespace Nighthawk | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| #include "server/http_time_tracking_filter.h" | ||
|
Contributor
There was a problem hiding this comment. This directory is starting to grow. Would it make sense to create a separate PR that moves this and other filters all into a
Member
Author
There was a problem hiding this comment. Yes I think that would make sense. Alternatively, we could keep this directory just for extensions and their configuration, and re-home everything else that's in there so far: configuration.h/cc (helpers), well_known_headers.h (http header name definitions). Should I file an issue to track this?
Contributor
There was a problem hiding this comment. An issue to track this would be great. I'm happy to defer to you on what makes the most sense. The |
||
|
|
||
| #include <string> | ||
|
|
||
| #include "envoy/server/filter_config.h" | ||
|
|
||
| #include "common/thread_safe_monotonic_time_stopwatch.h" | ||
|
|
||
| #include "server/configuration.h" | ||
| #include "server/well_known_headers.h" | ||
|
|
||
| #include "absl/strings/numbers.h" | ||
| #include "absl/strings/str_cat.h" | ||
|
|
||
| namespace Nighthawk { | ||
| namespace Server { | ||
|
|
||
| HttpTimeTrackingFilterConfig::HttpTimeTrackingFilterConfig( | ||
| nighthawk::server::ResponseOptions proto_config) | ||
| : server_config_(std::move(proto_config)), | ||
| stopwatch_(std::make_unique<ThreadSafeMontonicTimeStopwatch>()) {} | ||
|
|
||
| uint64_t | ||
| HttpTimeTrackingFilterConfig::getElapsedNanosSinceLastRequest(Envoy::TimeSource& time_source) { | ||
| return stopwatch_->getElapsedNsAndReset(time_source); | ||
| } | ||
|
|
||
| HttpTimeTrackingFilter::HttpTimeTrackingFilter(HttpTimeTrackingFilterConfigSharedPtr config) | ||
| : config_(std::move(config)) {} | ||
|
|
||
| Envoy::Http::FilterHeadersStatus | ||
| HttpTimeTrackingFilter::decodeHeaders(Envoy::Http::RequestHeaderMap& headers, bool /*end_stream*/) { | ||
|
Contributor
There was a problem hiding this comment. Two questions here, both likely out of ignorance.
Member
Author
There was a problem hiding this comment. My 2p:
Contributor
There was a problem hiding this comment.
|
||
| base_config_ = config_->server_config(); | ||
| const auto* request_config_header = headers.get(TestServer::HeaderNames::get().TestServerConfig); | ||
| if (request_config_header) { | ||
| json_merge_error_ = !Configuration::mergeJsonConfig( | ||
| request_config_header->value().getStringView(), base_config_, error_message_); | ||
| if (json_merge_error_) { | ||
| decoder_callbacks_->sendLocalReply( | ||
| static_cast<Envoy::Http::Code>(500), | ||
| fmt::format("time-tracking didn't understand the request: {}", error_message_), nullptr, | ||
| absl::nullopt, ""); | ||
| return Envoy::Http::FilterHeadersStatus::StopIteration; | ||
| } | ||
| } | ||
| return Envoy::Http::FilterHeadersStatus::Continue; | ||
| } | ||
|
|
||
| Envoy::Http::FilterHeadersStatus | ||
| HttpTimeTrackingFilter::encodeHeaders(Envoy::Http::ResponseHeaderMap& response_headers, bool) { | ||
| if (!json_merge_error_) { | ||
| const std::string previous_request_delta_in_response_header = | ||
| base_config_.emit_previous_request_delta_in_response_header(); | ||
| if (!previous_request_delta_in_response_header.empty() && last_request_delta_ns_ > 0) { | ||
| response_headers.appendCopy( | ||
| Envoy::Http::LowerCaseString(previous_request_delta_in_response_header), | ||
| absl::StrCat(last_request_delta_ns_)); | ||
|
Contributor
There was a problem hiding this comment. should this be SimpleAToi? I think you're trying to use StrCat here just for int conversion to an int
Member
Author
There was a problem hiding this comment. Well, we're converting an // This package contains functions for converting strings to numbers. For
--
| // converting numbers to strings, use `StrCat()` or `StrAppend()` in str_cat.h,
| // which automatically detect and convert most number values appropriately.I do agree though, that I suspect that if I would be at the other end of this review, the same thing would have popped up in my mind. Maybe we should define a shorthand that does look more intuitive in the case where we're converting a single int to string, otoh I'm not sure it's worth it?
Contributor
There was a problem hiding this comment. Oops got that backward. My bad. This probably doesn't matter unless we are worrying about very high performance here, but I think StrCat might be a heavy hand for converting one int to a string. It's optimized for concatenating strings and ints together efficiently. Happy to leave it here, and agree that it's not worth creating a helper function. |
||
| } | ||
| } | ||
| return Envoy::Http::FilterHeadersStatus::Continue; | ||
| } | ||
|
|
||
| void HttpTimeTrackingFilter::setDecoderFilterCallbacks( | ||
| Envoy::Http::StreamDecoderFilterCallbacks& callbacks) { | ||
| Envoy::Http::PassThroughFilter::setDecoderFilterCallbacks(callbacks); | ||
| last_request_delta_ns_ = | ||
| config_->getElapsedNanosSinceLastRequest(callbacks.dispatcher().timeSource()); | ||
| } | ||
|
|
||
| } // namespace Server | ||
| } // namespace Nighthawk | ||
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.
no changes required for this (not even sure if it's possible), but it would be nice if we could set this at the package level instead of on every single target in common/BUILD. But that doesn't seem to be supported by envoy_package.
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.
I think this is a good point, I filed an issue tagged as tech-debt to track this: #502