-
Notifications
You must be signed in to change notification settings - Fork 5.5k
access_log, router: add subsecond specifier for START_TIME #3269
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 36 commits
f85d68d
b0b741b
4c80a6d
3110ce0
42bd291
8fcc471
4e74570
79a3320
f94e020
07dc56f
91fb3da
aef2ed9
1970913
9004414
dd15262
ce1d3a0
e98f1a4
edbc437
8919c50
908fc61
0f417da
106c3e2
cc6c7c9
4dcfeb0
7d53b28
e6cb04e
96eb0c6
f475839
724a203
24518ef
0f0b1bf
f0fa285
f78e7ad
7aeb767
6310447
8a1571f
78e8430
325fdc5
4b9fa9e
9aba1e9
94e3add
612f829
96f915e
d5f082b
b85b275
513d4bf
9a16090
e5642ba
bda6762
ec35f6c
ea5e62f
22a94a4
3f01c64
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 |
|---|---|---|
|
|
@@ -10,8 +10,10 @@ | |
| #include "envoy/common/exception.h" | ||
|
|
||
| #include "common/common/assert.h" | ||
| #include "common/common/empty_string.h" | ||
| #include "common/common/fmt.h" | ||
| #include "common/common/hash.h" | ||
| #include "common/singleton/const_singleton.h" | ||
|
|
||
| #include "absl/strings/ascii.h" | ||
| #include "absl/strings/match.h" | ||
|
|
@@ -20,8 +22,88 @@ | |
| #include "spdlog/spdlog.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
||
| namespace { | ||
|
|
||
| class SubsecondConstantValues { | ||
| public: | ||
| const char PLACEHOLDER_CHAR{'?'}; | ||
| const std::string PLACEHOLDER{"?????????"}; | ||
| const std::regex PATTERN{"%([1-9])?f", std::regex::optimize}; | ||
| }; | ||
|
|
||
| typedef ConstSingleton<SubsecondConstantValues> SubsecondConstants; | ||
|
|
||
| } // namespace | ||
|
|
||
| std::string DateFormatter::fromTime(const SystemTime& time) const { | ||
| return fromTime(std::chrono::system_clock::to_time_t(time)); | ||
| struct CachedTime { | ||
| std::chrono::seconds epoch_time_seconds; | ||
| // A map is used to keep different formatted format strings at a given second. | ||
| std::unordered_map<std::string, const std::string> formatted; | ||
| }; | ||
| static thread_local CachedTime cached_time; | ||
|
|
||
| const std::chrono::nanoseconds epoch_time_ns = | ||
| std::chrono::duration_cast<std::chrono::nanoseconds>(time.time_since_epoch()); | ||
|
|
||
| const std::chrono::seconds epoch_time_seconds = | ||
| std::chrono::duration_cast<std::chrono::seconds>(epoch_time_ns); | ||
|
|
||
| if (cached_time.formatted.find(format_string_) == cached_time.formatted.end() || | ||
| cached_time.epoch_time_seconds != epoch_time_seconds) { | ||
| cached_time.formatted.emplace( | ||
| std::make_pair(format_string_, fromTime(std::chrono::system_clock::to_time_t(time)))); | ||
|
|
||
| cached_time.epoch_time_seconds = epoch_time_seconds; | ||
| } | ||
|
|
||
| if (subseconds_.empty()) { | ||
| return cached_time.formatted.at(format_string_); | ||
|
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. @brian-pane could you do a review pass on the cached time optimization? Thanks. |
||
| } | ||
|
|
||
| // Copy the current cached formatted format string, then replace its subseconds part. | ||
| std::string formatted = cached_time.formatted.at(format_string_); | ||
| const std::string value = fmt::FormatInt(epoch_time_ns.count()).str(); | ||
| for (const auto subsecond : subseconds_) { | ||
| // TODO(dio): Infer the length of second from parsing step. Currently, it is defaulted to 10. | ||
|
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. Probably needs to be fixed before merging. |
||
| const std::string digits = value.substr(10, subsecond.width_); | ||
| formatted.replace(subsecond.position_, subsecond.width_, digits); | ||
| } | ||
|
|
||
| // TODO(dio): Assert the formatted length. | ||
| return formatted; | ||
| } | ||
|
|
||
| std::string DateFormatter::parse(const std::string& format_string) { | ||
| auto now = std::chrono::system_clock::now(); | ||
| time_t current_time = std::chrono::system_clock::to_time_t(now); | ||
| tm current_tm; | ||
| gmtime_r(¤t_time, ¤t_tm); | ||
|
|
||
| std::string new_format_string = format_string; | ||
| std::smatch matched; | ||
| size_t position = 0; | ||
| size_t previous = 0; | ||
| std::array<char, 1024> buf; | ||
| while (regex_search(new_format_string, matched, SubsecondConstants::get().PATTERN)) { | ||
| const std::string& width_specifier = matched[1]; | ||
| const size_t width = width_specifier.empty() ? SubsecondConstants::get().PLACEHOLDER.size() | ||
| : width_specifier.at(0) - '0'; | ||
| new_format_string.replace(matched.position(), matched.length(), | ||
| SubsecondConstants::get().PLACEHOLDER.substr(0, width)); | ||
|
|
||
| const std::string part = new_format_string.substr(previous, matched.position() - previous); | ||
| const size_t formatted_length = strftime(&buf[0], buf.size(), part.c_str(), ¤t_tm); | ||
|
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 think the part above now looks good, but I don't think you want to do anything that uses the current time representation here (it seems dangerous, since the formatted time at parse may be very different than the formatted time at request time). Here's my understanding:
The reason you need to do this additional complexity right now is due to how the cached time string works. I think what makes sense is to do the offset computation when you regenerate the cached item, in conjunctino with the Sorry I missed this the first couple of rounds, that's the crux of it I think.
Member
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. @htuch I decided to have I'm not super happy about it since it is a bit complex. Want to get your input on this. Thanks! |
||
|
|
||
| // Save subsecond's position and width to be used later at data path. | ||
| SubsecondSpecifier subsecond(position + formatted_length, width); | ||
| subseconds_.emplace_back(subsecond); | ||
|
|
||
| position += formatted_length + width; | ||
| previous = matched.position() + width; | ||
| } | ||
| return new_format_string; | ||
| } | ||
|
|
||
| std::string DateFormatter::fromTime(time_t time) const { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ namespace Envoy { | |
| */ | ||
| class DateFormatter { | ||
| public: | ||
| DateFormatter(const std::string& format_string) : format_string_(format_string) {} | ||
| DateFormatter(const std::string& format_string) : format_string_(parse(format_string)) {} | ||
|
|
||
| /** | ||
| * @return std::string representing the GMT/UTC time based on the input time. | ||
|
|
@@ -48,7 +48,18 @@ class DateFormatter { | |
| const std::string& formatString() const { return format_string_; } | ||
|
|
||
| private: | ||
| std::string format_string_; | ||
| std::string parse(const std::string& format_string); | ||
|
|
||
| struct SubsecondSpecifier { | ||
| SubsecondSpecifier(const size_t position, const size_t width) | ||
| : position_(position), width_(width) {} | ||
|
|
||
| const size_t position_; | ||
| const size_t width_; | ||
|
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. Comments on all these fields please. |
||
| }; | ||
| std::vector<SubsecondSpecifier> subseconds_; | ||
|
|
||
| const std::string format_string_; | ||
| }; | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,7 @@ class RequestInfoHeaderFormatter : public HeaderFormatter { | |
| private: | ||
| std::function<std::string(const Envoy::RequestInfo::RequestInfo&)> field_extractor_; | ||
| const bool append_; | ||
| std::map<std::string, std::vector<AccessLog::FormatterPtr>> start_time_formatters_; | ||
|
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.
Member
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. Ah, OK. Why did I put it as a map? 😅.
Member
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. @htuch updated. |
||
| }; | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,38 @@ static void BM_AccessLogDateTimeFormatter(benchmark::State& state) { | |
| } | ||
| BENCHMARK(BM_AccessLogDateTimeFormatter); | ||
|
|
||
| // This benchmark is basically similar with the above BM_AccessLogDateTimeFormatter, the only | ||
| // difference is the format string input for the Envoy::DateFormatter. | ||
| static void BM_DateTimeFormatterWithSubseconds(benchmark::State& state) { | ||
| int outputBytes = 0; | ||
|
|
||
| Envoy::SystemTime time(std::chrono::seconds(1522796769)); | ||
| std::mt19937 prng(1); | ||
| std::uniform_int_distribution<long> distribution(-10, 20); | ||
| Envoy::DateFormatter date_formatter("%Y-%m-%dT%H:%M:%s.%3f"); | ||
| for (auto _ : state) { | ||
| time += std::chrono::milliseconds(static_cast<int>(distribution(prng))); | ||
| outputBytes += date_formatter.fromTime(time).length(); | ||
| } | ||
| benchmark::DoNotOptimize(outputBytes); | ||
| } | ||
| BENCHMARK(BM_DateTimeFormatterWithSubseconds); | ||
|
|
||
| static void BM_DateTimeFormatterWithoutSubseconds(benchmark::State& state) { | ||
| int outputBytes = 0; | ||
|
|
||
| Envoy::SystemTime time(std::chrono::seconds(1522796769)); | ||
| std::mt19937 prng(1); | ||
| std::uniform_int_distribution<long> distribution(-10, 20); | ||
| Envoy::DateFormatter date_formatter("%Y-%m-%dT%H:%M:%s"); | ||
| for (auto _ : state) { | ||
| time += std::chrono::milliseconds(static_cast<int>(distribution(prng))); | ||
| outputBytes += date_formatter.fromTime(time).length(); | ||
| } | ||
| benchmark::DoNotOptimize(outputBytes); | ||
| } | ||
| BENCHMARK(BM_DateTimeFormatterWithoutSubseconds); | ||
|
|
||
|
Member
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. The current benchmarking result |
||
| static void BM_RTrimStringView(benchmark::State& state) { | ||
| int accum = 0; | ||
| for (auto _ : state) { | ||
|
|
||
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.
You probably don't need this to be a constant. You can just do something like
std::string(N, '?')later on to generate a string of lengthN.