-
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 4 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 |
|---|---|---|
|
|
@@ -192,6 +192,12 @@ void Filter::sendLocalReply(Http::Code code, const std::string& body, | |
| Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool end_stream) { | ||
| downstream_headers_ = &headers; | ||
|
|
||
| // If defined, set the specified start timestamp header with timestamp in milliseconds. | ||
| if (!config_.start_timestamp_header_.get().empty()) { | ||
|
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. add an explanation comment
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. Added in 4c80a6d. |
||
| headers.addReferenceKey(config_.start_timestamp_header_, | ||
| std::chrono::system_clock::now().time_since_epoch().count()); | ||
| } | ||
|
|
||
| // Only increment rq total stat if we actually decode headers here. This does not count requests | ||
| // that get handled by earlier filters. | ||
| config_.stats_.rq_total_.inc(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,10 +63,10 @@ class TestFilter : public Filter { | |
|
|
||
| class RouterTestBase : public testing::Test { | ||
| public: | ||
| RouterTestBase(bool start_child_span) | ||
| RouterTestBase(bool start_child_span, const std::string& start_timestamp_header = EMPTY_STRING) | ||
| : shadow_writer_(new MockShadowWriter()), | ||
| config_("test.", local_info_, stats_store_, cm_, runtime_, random_, | ||
| ShadowWriterPtr{shadow_writer_}, true, start_child_span), | ||
| ShadowWriterPtr{shadow_writer_}, true, start_child_span, start_timestamp_header), | ||
| router_(config_) { | ||
| router_.setDecoderFilterCallbacks(callbacks_); | ||
| upstream_locality_.set_zone("to_az"); | ||
|
|
@@ -1628,6 +1628,28 @@ TEST_F(RouterTest, DirectResponseWithBody) { | |
| EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); | ||
| } | ||
|
|
||
| TEST_F(RouterTest, NoStartTimestampHeader) { | ||
| absl::optional<Http::Protocol> downstream_protocol{Http::Protocol::Http2}; | ||
| EXPECT_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()) | ||
| .WillOnce(Return(Upstream::ClusterInfo::Features::USE_DOWNSTREAM_PROTOCOL)); | ||
| EXPECT_CALL(callbacks_.request_info_, protocol()).WillOnce(ReturnPointee(&downstream_protocol)); | ||
|
|
||
| EXPECT_CALL(cm_, httpConnPoolForCluster(_, _, Http::Protocol::Http2, _)); | ||
| EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); | ||
| expectResponseTimerCreate(); | ||
|
|
||
| Http::TestHeaderMapImpl headers; | ||
| HttpTestUtility::addDefaultHeaders(headers); | ||
| router_.decodeHeaders(headers, true); | ||
|
|
||
| const Http::LowerCaseString header("x-request-start"); | ||
| EXPECT_EQ(nullptr, headers.get(header)); | ||
|
|
||
| EXPECT_CALL(cancellable_, cancel()); | ||
| router_.onDestroy(); | ||
| EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); | ||
| } | ||
|
|
||
| TEST(RouterFilterUtilityTest, finalTimeout) { | ||
| { | ||
| NiceMock<MockRouteEntry> route; | ||
|
|
@@ -2117,5 +2139,36 @@ TEST_F(RouterTestChildSpan, ResetFlow) { | |
| encoder.stream_.resetStream(Http::StreamResetReason::RemoteReset); | ||
| } | ||
|
|
||
| class RouterTestStartTimestampHeader : public RouterTestBase { | ||
| public: | ||
| RouterTestStartTimestampHeader() : RouterTestBase(false, "X-Request-Start") {} | ||
| }; | ||
|
|
||
| TEST_F(RouterTestStartTimestampHeader, WithStartTimestampHeader) { | ||
| absl::optional<Http::Protocol> downstream_protocol{Http::Protocol::Http2}; | ||
| EXPECT_CALL(*cm_.thread_local_cluster_.cluster_.info_, features()) | ||
| .WillOnce(Return(Upstream::ClusterInfo::Features::USE_DOWNSTREAM_PROTOCOL)); | ||
| EXPECT_CALL(callbacks_.request_info_, protocol()).WillOnce(ReturnPointee(&downstream_protocol)); | ||
|
|
||
| EXPECT_CALL(cm_, httpConnPoolForCluster(_, _, Http::Protocol::Http2, _)); | ||
| EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); | ||
| expectResponseTimerCreate(); | ||
|
|
||
| Http::TestHeaderMapImpl headers; | ||
| HttpTestUtility::addDefaultHeaders(headers); | ||
|
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. test that the
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. Added in 3110ce0. |
||
| const Http::LowerCaseString header("x-request-start"); | ||
| EXPECT_EQ(nullptr, headers.get(header)); | ||
| router_.decodeHeaders(headers, true); | ||
| EXPECT_NE(nullptr, headers.get(header)); | ||
|
|
||
| uint64_t timestamp; | ||
| StringUtil::atoul(headers.get(header)->value().c_str(), timestamp); | ||
| EXPECT_GT(timestamp, 0); | ||
|
|
||
| EXPECT_CALL(cancellable_, cancel()); | ||
| router_.onDestroy(); | ||
| EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); | ||
| } | ||
|
|
||
| } // namespace Router | ||
| } // namespace Envoy | ||
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.
Can we reuse the existing user defined headers mechanism for this? E.g. you could configure
x-request-startwith value%START_TIMESTAMP%?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.
Oh yes, +1. That seems better.
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.
@htuch thanks for the hint! Yes, I think that's the right direction (I should went to that direction in the first place, since apparently
RequestInfohas that useful.startTime()😔).I added the corresponding changes using that approach. Thanks!
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.
That's cleaner, thanks for suggesting!