-
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
Merged
Merged
Changes from 8 commits
Commits
Show all changes
53 commits
Select commit
Hold shift + click to select a range
f85d68d
router: add start timestamp header config
dio b0b741b
docs: update release notes
dio 4c80a6d
review: add comment
dio 3110ce0
review: test that the x-request-start header is not present before th…
dio 42bd291
review: use header manipulation facility instead
dio 8fcc471
Merge remote-tracking branch 'upstream/master'
dio 4e74570
Add a test sample to TestParseInternal
dio 79a3320
tsan: check RatelimitIntegrationTest failure
dio f94e020
Rename it to START_TIME_SINCE_EPOCH
dio 07dc56f
Fix format
dio 91fb3da
Merge remote-tracking branch 'upstream/master'
dio aef2ed9
Allow header parser to accept START_TIME(ARG)
dio 1970913
Add replacers
dio 9004414
Use %f as subsecond formatter
dio dd15262
Merge remote-tracking branch 'upstream/master'
dio ce1d3a0
Add test in accesss_log_formatter
dio e98f1a4
Retry ipv6_test
dio edbc437
Add a parser to initialize the format_string_
dio 8919c50
Add a benchmark for date formatter with subseconds
dio 908fc61
Merge remote-tracking branch 'upstream/master'
dio 0f417da
Remove unused typedef
dio 106c3e2
Use shared ptr to cache the format
dio cc6c7c9
Try to fix asan build take 1
dio 4dcfeb0
Try to fix asan build take 2
dio 7d53b28
Try to fix asan build take 3
dio e6cb04e
Copy the ns value
dio 96eb0c6
Update release notes
dio f475839
review: simplify parsing step
dio 724a203
review: reuse access log's formatter
dio 24518ef
Use map to cache access log formatter
dio 0f0b1bf
review: update docs
dio f0fa285
Merge remote-tracking branch 'upstream/master'
dio f78e7ad
Fix additional specifiers table format
dio 7aeb767
Use [1-9] instead of {:digit}
dio 6310447
Fix doc's indentations
dio 8a1571f
No more find()
dio 78e8430
Merge remote-tracking branch 'upstream/master'
dio 325fdc5
Move subsecond offsets computation
dio 4b9fa9e
Merge remote-tracking branch 'upstream/master'
dio 9aba1e9
Move the offsets calc to fromTimeAndPrepareSubsecondOffsets
dio 94e3add
Cleanup unused deps and vars
dio 612f829
Infer seconds_length only when it is needed
dio 96f915e
Precompute the string segments between specifiers
dio d5f082b
Try to fix asan on using stack after scope: digits
dio b85b275
Stamp per cache item.
dio 513d4bf
Fix removing an item of a map while iterating it
dio 9a16090
Remove const PLACEHOLDER
dio e5642ba
Add comments to CachedTime struct
dio bda6762
Remove last_segment_ and streamline replacement
dio ec35f6c
Add test with shorter segments
dio ea5e62f
Remove % since it is diff on osx vs. linux
dio 22a94a4
Use unordered_map instead
dio 3f01c64
Comment on the possibility to have negative offset value
dio File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| #include "gtest/gtest.h" | ||
|
|
||
| using testing::NiceMock; | ||
| using testing::Return; | ||
| using testing::ReturnPointee; | ||
| using testing::ReturnRef; | ||
|
|
||
|
|
@@ -251,6 +252,12 @@ TEST(HeaderParserTest, TestParseInternal) { | |
| absl::optional<std::string> expected_exception_; | ||
| }; | ||
|
|
||
| time_t start_time_epoch = 1522280158; | ||
| SystemTime start_time = std::chrono::system_clock::from_time_t(start_time_epoch); | ||
| const std::string timestamp = fmt::format( | ||
| "{}", | ||
| std::chrono::duration_cast<std::chrono::milliseconds>(start_time.time_since_epoch()).count()); | ||
|
|
||
| static const TestCase test_cases[] = { | ||
| // Valid inputs | ||
| {"%PROTOCOL%", {"HTTP/1.1"}, {}}, | ||
|
|
@@ -270,6 +277,7 @@ TEST(HeaderParserTest, TestParseInternal) { | |
| {"%UPSTREAM_METADATA([\"ns\", \t \"key\"])%", {"value"}, {}}, | ||
| {"%UPSTREAM_METADATA([\"ns\", \n \"key\"])%", {"value"}, {}}, | ||
| {"%UPSTREAM_METADATA( \t [ \t \"ns\" \t , \t \"key\" \t ] \t )%", {"value"}, {}}, | ||
| {"%START_TIMESTAMP%", {timestamp}, {}}, | ||
|
|
||
| // Unescaped % | ||
| {"%", {}, {"Invalid header configuration. Un-escaped % at position 0"}}, | ||
|
|
@@ -404,6 +412,8 @@ TEST(HeaderParserTest, TestParseInternal) { | |
| )EOF"); | ||
| ON_CALL(*host, metadata()).WillByDefault(ReturnRef(metadata)); | ||
|
|
||
| ON_CALL(request_info, startTime()).WillByDefault(Return(start_time)); | ||
|
|
||
| for (const auto& test_case : test_cases) { | ||
| Protobuf::RepeatedPtrField<envoy::api::v2::core::HeaderValueOption> to_add; | ||
| envoy::api::v2::core::HeaderValueOption* header = to_add.Add(); | ||
|
|
@@ -595,6 +605,10 @@ TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) { | |
| { | ||
| "key": "x-client-ip", | ||
| "value": "%CLIENT_IP%" | ||
| }, | ||
| { | ||
| "key": "x-request-start", | ||
| "value": "%START_TIMESTAMP%" | ||
| } | ||
| ] | ||
| } | ||
|
|
@@ -604,19 +618,30 @@ TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) { | |
| envoy::api::v2::route::RouteAction route_action = parseRouteFromJson(json).route(); | ||
| route_action.mutable_request_headers_to_add(0)->mutable_append()->set_value(false); | ||
| route_action.mutable_request_headers_to_add(1)->mutable_append()->set_value(false); | ||
| route_action.mutable_request_headers_to_add(2)->mutable_append()->set_value(false); | ||
|
|
||
| HeaderParserPtr req_header_parser = | ||
| Router::HeaderParser::configure(route_action.request_headers_to_add()); | ||
| Http::TestHeaderMapImpl headerMap{ | ||
| {":method", "POST"}, {"static-header", "old-value"}, {"x-client-ip", "0.0.0.0"}}; | ||
|
|
||
| NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info; | ||
| time_t start_time_epoch = 1522280158; | ||
|
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: |
||
| SystemTime start_time = std::chrono::system_clock::from_time_t(start_time_epoch); | ||
| EXPECT_CALL(request_info, startTime()).WillOnce(Return(start_time)); | ||
|
|
||
| req_header_parser->evaluateHeaders(headerMap, request_info); | ||
| EXPECT_TRUE(headerMap.has("static-header")); | ||
| EXPECT_EQ("static-value", headerMap.get_("static-header")); | ||
| EXPECT_TRUE(headerMap.has("x-client-ip")); | ||
| EXPECT_EQ("127.0.0.1", headerMap.get_("x-client-ip")); | ||
|
|
||
| uint64_t timestamp; | ||
| StringUtil::atoul(headerMap.get_("x-request-start").c_str(), timestamp); | ||
| EXPECT_EQ( | ||
| timestamp, | ||
| std::chrono::duration_cast<std::chrono::milliseconds>(start_time.time_since_epoch()).count()); | ||
|
|
||
| typedef std::map<std::string, int> CountMap; | ||
| CountMap counts; | ||
| headerMap.iterate( | ||
|
|
@@ -635,6 +660,7 @@ TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) { | |
|
|
||
| EXPECT_EQ(1, counts["static-header"]); | ||
| EXPECT_EQ(1, counts["x-client-ip"]); | ||
| EXPECT_EQ(1, counts["x-request-start"]); | ||
| } | ||
|
|
||
| TEST(HeaderParserTest, EvaluateResponseHeaders) { | ||
|
|
@@ -647,6 +673,10 @@ match: { prefix: "/new_endpoint" } | |
| key: "x-client-ip" | ||
| value: "%CLIENT_IP%" | ||
| append: true | ||
| - header: | ||
| key: "x-request-start" | ||
| value: "%START_TIMESTAMP%" | ||
| append: true | ||
| response_headers_to_remove: ["x-nope"] | ||
| )EOF"; | ||
|
|
||
|
|
@@ -657,6 +687,7 @@ match: { prefix: "/new_endpoint" } | |
| NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info; | ||
| resp_header_parser->evaluateHeaders(headerMap, request_info); | ||
| EXPECT_TRUE(headerMap.has("x-client-ip")); | ||
| EXPECT_TRUE(headerMap.has("x-request-start")); | ||
| EXPECT_TRUE(headerMap.has("x-safe")); | ||
| EXPECT_FALSE(headerMap.has("x-nope")); | ||
| } | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We already have START_TIME in access logs. I wonder, could we just use START_TIME and allow a custom format specifier that is effectively milliseconds since epoch? Then we could use the same code in both places. If we can't do that what about START_TIME_SINCE_EPOCH or something? Though I like the former better. Thoughts?
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.
@mattklein123 do you think specifically handling
since_epoch_msin start time formatting makes sense?i.e. the start time formatting is either:
std::put_time'sfmtstring for%START_TIME(%Y/%m/%dT%H:%M:%S%z %s)%, orfmt::formatfor%START_TIME(since_epoch_ms)%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.
However, I think for now we probably can go with
START_TIME_SINCE_EPOCHfor header formatter, since currently the wayAccessLogFormatParserbuild the formatter is a bit isolated to itself. WDYT?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.
This is somewhat related to #3256. It would be nice to support custom formatters apart from what is currently supported. cc @bplotnick.
I think if we did this we could support time since epoch and some other things and I think this is a better long term direction, though more work. Are you interested in picking up #3256 as well and working on both?
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.
@mattklein123 sure let me take a look at the possibilities.