dfp: adding timing information about DNS resolution#18934
dfp: adding timing information about DNS resolution#18934alyssawilk merged 9 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Hey @mattklein123 pinging you for high level API thoughts. |
At a high level I think using filter state is fine for this, but I have 2 questions/comments:
|
|
Agree this should have docs, but I wanted to make sure we agreed on where the info was stored, then sort out a doc location :-) I like the idea of having a general struct. What do we think of a hash map from timing string to ms_since_epoch or ms_since_start? We could even TODO moving the backing store of the existing stream into metrics over to that and maybe even remove the timings APIs? Then we can doc up a well known filter states doc parallel to well known dynamic metadata? I think if we went that route I would lean towards adding it as a first class member to stream into APIs, but I'm good with whatever. |
Sure this sounds fine to me. The only thing I'm not sure of is whether we should key by string vs. just have some type of timing object that avoids hash lookups? |
|
if we want it to be extensible, what would you suggest other than hash map? Given we're updating once per request/response I'm not worried about the hash lookup cost. |
Ah OK got it. sure makes sense to me. |
|
@alyssawilk do you want @mattklein123 to take a pass, or do you want me to now that you two discussed high level API concerns? |
|
yeah if you'd be up for a pass that'd be great |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| } | ||
|
|
||
| DownstreamTiming& downstreamTiming() override { | ||
| if (!downstream_timing_.has_value()) { |
There was a problem hiding this comment.
Why have downstream_timing_ as optional? Seems like we could just have the struct if we are constructing on absence?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| Http::FilterHeadersStatus ProxyFilter::decodeHeaders(Http::RequestHeaderMap& headers, bool) { | ||
| latchTime(decoder_callbacks_, dnsStart()); |
There was a problem hiding this comment.
I'm curious about latching the time here vs. latching at other points in this function. For instance, only if the filter gets to loadDnsCacheEntry?
There was a problem hiding this comment.
yeah it's the difference between when we start the lookup and when we start doing "DNS work"
I can go either way - moved.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Merge main and then I can take a look? /wait |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM with small optional comments.
|
|
||
| class DownstreamTiming { | ||
| public: | ||
| void setValue(absl::string_view key, MonotonicTime value) { timings_[key] = value; } |
There was a problem hiding this comment.
Should we assert that we don't set a value twice? Not sure.
There was a problem hiding this comment.
yeah I didn't know if folks using this would want to do updates, so left without.
| public: | ||
| ProxyFilter(const ProxyFilterConfigSharedPtr& config) : config_(config) {} | ||
|
|
||
| static const std::string& dnsStart() { |
There was a problem hiding this comment.
nit: I think you can just do static constexpr absl::string_view = ... for these.
3 participants
Risk Level: low
Testing: new unit, integration tests
Docs Changes:
Release Notes:
Part of envoyproxy/envoy-mobile#1520