-
Notifications
You must be signed in to change notification settings - Fork 5.5k
dfp: adding timing information about DNS resolution #18934
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
Changes from 4 commits
6bd5c2d
18e4576
b930d82
593e8c8
1cbb49d
3af9872
06eee94
f86b33b
efc9af1
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,6 +131,13 @@ struct StreamInfoImpl : public StreamInfo { | |
| final_time_ = time_source_.monotonicTime(); | ||
| } | ||
|
|
||
| DownstreamTiming& downstreamTiming() override { | ||
| if (!downstream_timing_.has_value()) { | ||
|
Member
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 have
Contributor
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. I could go either way here, but I don't think we set these for TCP, and TCP (so redis etc.) have stream info, so I was largely trying to spare them the memory footprint.
Member
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. ah gotcha! |
||
| downstream_timing_ = DownstreamTiming(); | ||
| } | ||
| return downstream_timing_.value(); | ||
| } | ||
|
|
||
| void addBytesReceived(uint64_t bytes_received) override { bytes_received_ += bytes_received; } | ||
|
|
||
| uint64_t bytesReceived() const override { return bytes_received_; } | ||
|
|
@@ -360,6 +367,7 @@ struct StreamInfoImpl : public StreamInfo { | |
| std::string requested_server_name_; | ||
| const Http::RequestHeaderMap* request_headers_{}; | ||
| Http::RequestIdStreamInfoProviderSharedPtr request_id_provider_; | ||
| absl::optional<DownstreamTiming> downstream_timing_; | ||
| UpstreamTiming upstream_timing_; | ||
| std::string upstream_transport_failure_reason_; | ||
| absl::optional<Upstream::ClusterInfoConstSharedPtr> upstream_cluster_info_; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,15 @@ namespace Envoy { | |
| namespace Extensions { | ||
| namespace HttpFilters { | ||
| namespace DynamicForwardProxy { | ||
| namespace { | ||
|
|
||
| void latchTime(Http::StreamDecoderFilterCallbacks* decoder_callbacks, const std::string& key) { | ||
| StreamInfo::DownstreamTiming& downstream_timing = | ||
| decoder_callbacks->streamInfo().downstreamTiming(); | ||
| downstream_timing.setValue(key, decoder_callbacks->dispatcher().timeSource().monotonicTime()); | ||
| } | ||
|
|
||
| } // namespace | ||
| struct ResponseStringValues { | ||
| const std::string DnsCacheOverflow = "DNS cache overflow"; | ||
| const std::string PendingRequestOverflow = "Dynamic forward proxy pending request overflow"; | ||
|
|
@@ -46,6 +54,8 @@ void ProxyFilter::onDestroy() { | |
| } | ||
|
|
||
| Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { | ||
| latchTime(decoder_callbacks_, dnsStart()); | ||
|
Member
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'm curious about latching the time here vs. latching at other points in this function. For instance, only if the filter gets to
Contributor
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. yeah it's the difference between when we start the lookup and when we start doing "DNS work" |
||
|
|
||
| Router::RouteConstSharedPtr route = decoder_callbacks_->route(); | ||
| const Router::RouteEntry* route_entry; | ||
| if (!route || !(route_entry = route->routeEntry())) { | ||
|
|
@@ -132,6 +142,7 @@ Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& hea | |
| addHostAddressToFilterState(host.value()->address()); | ||
| } | ||
|
|
||
| latchTime(decoder_callbacks_, dnsEnd()); | ||
| return Http::FilterHeadersStatus::Continue; | ||
| } | ||
| case LoadDnsCacheEntryStatus::Loading: | ||
|
|
@@ -183,6 +194,7 @@ void ProxyFilter::onLoadDnsCacheComplete( | |
| const Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) { | ||
| ENVOY_STREAM_LOG(debug, "load DNS cache complete, continuing after adding resolved host: {}", | ||
| *decoder_callbacks_, host_info->resolvedHost()); | ||
| latchTime(decoder_callbacks_, dnsEnd()); | ||
| ASSERT(circuit_breaker_ != nullptr); | ||
| circuit_breaker_.reset(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,13 @@ class ProxyFilter | |
| public: | ||
| ProxyFilter(const ProxyFilterConfigSharedPtr& config) : config_(config) {} | ||
|
|
||
| static const std::string& dnsStart() { | ||
|
Member
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: I think you can just do |
||
| CONSTRUCT_ON_FIRST_USE(std::string, "envoy.dynamic_forward_proxy.dns_start_ms"); | ||
| } | ||
| static const std::string& dnsEnd() { | ||
| CONSTRUCT_ON_FIRST_USE(std::string, "envoy.dynamic_forward_proxy.dns_end_ms"); | ||
| } | ||
|
|
||
| // Http::PassThroughDecoderFilter | ||
| Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers, | ||
| bool end_stream) override; | ||
|
|
||
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.
Should we assert that we don't set a value twice? Not sure.
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.
yeah I didn't know if folks using this would want to do updates, so left without.