From 62c1f602c7b296fbc85a912801e0a6a52c88d32e Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 30 Aug 2016 13:38:04 -0700 Subject: [PATCH 01/12] Add checking upstream host for canary to AsyncClient --- configs/configgen.sh | 2 +- source/common/http/async_client_impl.cc | 6 +- test/CMakeLists.txt | 2 +- test/common/http/async_client_impl_test.cc | 66 ++++++++++++++++++++++ 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/configs/configgen.sh b/configs/configgen.sh index ca30f30a20f7..67df58385cd3 100755 --- a/configs/configgen.sh +++ b/configs/configgen.sh @@ -1,7 +1,7 @@ #!/bin/bash SCRIPT_DIR=`dirname $0` -BUILD_DIR=build/configgen +BUILD_DIR=$2/configgen if [ ! -d $BUILD_DIR/venv ]; then virtualenv $BUILD_DIR/venv $BUILD_DIR/venv/bin/pip install -r $SCRIPT_DIR/requirements.txt diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index e39a105e1ba9..16dd4b953282 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -116,11 +116,11 @@ void AsyncRequestImpl::decodeTrailers(HeaderMapPtr&& trailers) { } void AsyncRequestImpl::onComplete() { - // TODO: Check host's canary status in addition to canary header. CodeUtility::ResponseTimingInfo info{ parent_.stats_store_, parent_.stat_prefix_, stream_encoder_->requestCompleteTime(), - response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true", true, EMPTY_STRING, - EMPTY_STRING, parent_.local_zone_name_, upstreamZone()}; + response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true" || + upstream_host_ ? upstream_host_->canary() : false, + true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone()}; CodeUtility::chargeResponseTiming(info); callbacks_.onSuccess(std::move(response_)); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 668dfbbe1f71..21e2b5c6597b 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -142,7 +142,7 @@ target_link_libraries(envoy-test anl) target_link_libraries(envoy-test dl) add_custom_target( - envoy.generate_example_configs configs/configgen.sh generated/configs + envoy.generate_example_configs configs/configgen.sh generated/configs ${PROJECT_BINARY_DIR} WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} ) diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index cbc7c9ea0526..742cbe20f4f9 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -14,6 +14,7 @@ using testing::Invoke; using testing::NiceMock; using testing::Ref; using testing::ReturnRef; +using testing::Return; namespace Http { @@ -253,4 +254,69 @@ TEST_F(AsyncClientImplTest, DisableTimer) { request->cancel(); } +TEST_F(AsyncClientImplTest, CanaryStatusTrue) { + message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); + Buffer::Instance& data = *message_->body(); + + EXPECT_CALL(conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](StreamDecoder& decoder, ConnectionPool::Callbacks& callbacks) + -> ConnectionPool::Cancellable* { + callbacks.onPoolReady(stream_encoder_, conn_pool_.host_); + response_decoder_ = &decoder; + return nullptr; + })); + + EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(ByRef(message_->headers())), false)); + EXPECT_CALL(stream_encoder_, encodeData(BufferEqual(&data), true)); + EXPECT_CALL(callbacks_, onSuccess_(_)); + + AsyncClientImpl client(cluster_, *this, stats_store_, dispatcher_, "from_az"); + client.send(std::move(message_), callbacks_, Optional()); + + HeaderMapPtr response_headers(new HeaderMapImpl{{":status", "200"}, + {"x-envoy-upstream-canary", "false"}}); + response_decoder_->decodeHeaders(std::move(response_headers), false); + EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(true)); + EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.upstream_rq_time", _)); + EXPECT_CALL(stats_store_, + deliverTimingToSinks("cluster.fake_cluster.internal.upstream_rq_time", _)); + EXPECT_CALL(stats_store_, + deliverTimingToSinks("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_time", _)); + + EXPECT_CALL(stats_store_, + deliverTimingToSinks("cluster.fake_cluster.canary.upstream_rq_time", _)); + response_decoder_->decodeData(data, true); + } + +TEST_F(AsyncClientImplTest, CanaryStatusFalse) { + message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); + Buffer::Instance& data = *message_->body(); + + EXPECT_CALL(conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](StreamDecoder& decoder, ConnectionPool::Callbacks& callbacks) + -> ConnectionPool::Cancellable* { + callbacks.onPoolReady(stream_encoder_, conn_pool_.host_); + response_decoder_ = &decoder; + return nullptr; + })); + + EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(ByRef(message_->headers())), false)); + EXPECT_CALL(stream_encoder_, encodeData(BufferEqual(&data), true)); + EXPECT_CALL(callbacks_, onSuccess_(_)); + + AsyncClientImpl client(cluster_, *this, stats_store_, dispatcher_, "from_az"); + client.send(std::move(message_), callbacks_, Optional()); + + HeaderMapPtr response_headers(new HeaderMapImpl{{":status", "200"}, + {"x-envoy-upstream-canary", "false"}}); + response_decoder_->decodeHeaders(std::move(response_headers), false); + EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(false)); + EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.upstream_rq_time", _)); + EXPECT_CALL(stats_store_, + deliverTimingToSinks("cluster.fake_cluster.internal.upstream_rq_time", _)); + EXPECT_CALL(stats_store_, + deliverTimingToSinks("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_time", _)); + response_decoder_->decodeData(data, true); + } + } // Http From bb4d3a9d0f4c07295bca47ed6837f8dea930b28f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 31 Aug 2016 13:10:47 -0700 Subject: [PATCH 02/12] Change the way upstream_canary is checked by chargeResponseData --- source/common/http/async_client_impl.cc | 19 +++++++++++++------ source/common/http/codes.cc | 2 +- source/common/http/codes.h | 1 + source/common/http/filter/ratelimit.cc | 2 +- source/common/router/router.cc | 10 ++++++---- test/common/http/async_client_impl_test.cc | 18 +++++++++--------- test/common/http/codes_test.cc | 3 ++- 7 files changed, 33 insertions(+), 22 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 16dd4b953282..720184a96291 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -79,10 +79,14 @@ void AsyncRequestImpl::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { response_->headers().iterate([](const LowerCaseString& key, const std::string& value) -> void { log_debug(" '{}':'{}'", key.get(), value); }); #endif + bool is_canary = + response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true" || upstream_host_ + ? upstream_host_->canary() + : false; CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, response_->headers(), true, EMPTY_STRING, EMPTY_STRING, - parent_.local_zone_name_, upstreamZone()}; + parent_.local_zone_name_, upstreamZone(), is_canary}; CodeUtility::chargeResponseStat(info); if (end_stream) { @@ -116,10 +120,13 @@ void AsyncRequestImpl::decodeTrailers(HeaderMapPtr&& trailers) { } void AsyncRequestImpl::onComplete() { + bool is_canary = + response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true" || upstream_host_ + ? upstream_host_->canary() + : false; + CodeUtility::ResponseTimingInfo info{ - parent_.stats_store_, parent_.stat_prefix_, stream_encoder_->requestCompleteTime(), - response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true" || - upstream_host_ ? upstream_host_->canary() : false, + parent_.stats_store_, parent_.stat_prefix_, stream_encoder_->requestCompleteTime(), is_canary, true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone()}; CodeUtility::chargeResponseTiming(info); @@ -130,7 +137,7 @@ void AsyncRequestImpl::onComplete() { void AsyncRequestImpl::onResetStream(StreamResetReason) { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, SERVICE_UNAVAILABLE_HEADER, true, EMPTY_STRING, EMPTY_STRING, - parent_.local_zone_name_, upstreamZone()}; + parent_.local_zone_name_, upstreamZone(), false}; CodeUtility::chargeResponseStat(info); callbacks_.onFailure(AsyncClient::FailureReason::Reset); cleanup(); @@ -139,7 +146,7 @@ void AsyncRequestImpl::onResetStream(StreamResetReason) { void AsyncRequestImpl::onRequestTimeout() { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, REQUEST_TIMEOUT_HEADER, true, EMPTY_STRING, EMPTY_STRING, - parent_.local_zone_name_, upstreamZone()}; + parent_.local_zone_name_, upstreamZone(), false}; CodeUtility::chargeResponseStat(info); parent_.cluster_.stats().upstream_rq_timeout_.inc(); stream_encoder_->resetStream(); diff --git a/source/common/http/codes.cc b/source/common/http/codes.cc index d74776e83ea4..3ef9350921c4 100644 --- a/source/common/http/codes.cc +++ b/source/common/http/codes.cc @@ -25,7 +25,7 @@ void CodeUtility::chargeResponseStat(const ResponseStatInfo& info) { std::string group_string = groupStringForResponseCode(static_cast(response_code)); // If the response is from a canary, also create canary stats. - if (info.response_headers_.get(Headers::get().EnvoyUpstreamCanary) == "true") { + if (info.upstream_canary_) { info.store_.counter(fmt::format("{}canary.upstream_rq_{}", info.prefix_, group_string)).inc(); info.store_.counter(fmt::format("{}canary.upstream_rq_{}", info.prefix_, response_code)).inc(); } diff --git a/source/common/http/codes.h b/source/common/http/codes.h index f200ac4d330c..248e982e9b03 100644 --- a/source/common/http/codes.h +++ b/source/common/http/codes.h @@ -27,6 +27,7 @@ class CodeUtility { const std::string& request_vcluster_name_; const std::string& from_zone_; const std::string& to_zone_; + bool upstream_canary_; }; /** diff --git a/source/common/http/filter/ratelimit.cc b/source/common/http/filter/ratelimit.cc index d50e2b7968f7..9f6539148abf 100644 --- a/source/common/http/filter/ratelimit.cc +++ b/source/common/http/filter/ratelimit.cc @@ -84,7 +84,7 @@ void RateLimitFilter::complete(RateLimit::LimitStatus status) { config_->stats().counter(cluster_ratelimit_stat_prefix_ + "over_limit").inc(); Http::CodeUtility::ResponseStatInfo info{config_->stats(), cluster_stat_prefix_, TOO_MANY_REQUESTS_HEADER, true, EMPTY_STRING, - EMPTY_STRING, EMPTY_STRING, EMPTY_STRING}; + EMPTY_STRING, EMPTY_STRING, EMPTY_STRING, false}; Http::CodeUtility::chargeResponseStat(info); break; } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 661f3ebeced9..8066013d84a7 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -98,7 +98,8 @@ void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) { Http::CodeUtility::ResponseStatInfo info{ config_->stats_store_, stat_prefix_, response_headers, downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true", - route_->virtualHostName(), request_vcluster_name_, config_->service_zone_, upstreamZone()}; + route_->virtualHostName(), request_vcluster_name_, config_->service_zone_, upstreamZone(), + upstream_request_->upstream_canary_}; Http::CodeUtility::chargeResponseStat(info); @@ -106,7 +107,7 @@ void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) { Http::CodeUtility::ResponseStatInfo info{ config_->stats_store_, alt_prefix, response_headers, downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true", "", "", - config_->service_zone_, upstreamZone()}; + config_->service_zone_, upstreamZone(), upstream_request_->upstream_canary_}; Http::CodeUtility::chargeResponseStat(info); } @@ -382,9 +383,10 @@ void Filter::onUpstreamHeaders(Http::HeaderMapPtr&& headers, bool end_stream) { std::to_string(ms.count())); } - // TODO: Check host's canary status in addition to canary header. upstream_request_->upstream_canary_ = - headers->get(Http::Headers::get().EnvoyUpstreamCanary) == "true"; + headers->get(Http::Headers::get().EnvoyUpstreamCanary) == "true" || upstream_host_ + ? upstream_host_->canary() + : false; chargeUpstreamCode(*headers); downstream_response_started_ = true; diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index 742cbe20f4f9..2bd0cee7cafc 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -13,8 +13,8 @@ using testing::ByRef; using testing::Invoke; using testing::NiceMock; using testing::Ref; -using testing::ReturnRef; using testing::Return; +using testing::ReturnRef; namespace Http { @@ -273,8 +273,8 @@ TEST_F(AsyncClientImplTest, CanaryStatusTrue) { AsyncClientImpl client(cluster_, *this, stats_store_, dispatcher_, "from_az"); client.send(std::move(message_), callbacks_, Optional()); - HeaderMapPtr response_headers(new HeaderMapImpl{{":status", "200"}, - {"x-envoy-upstream-canary", "false"}}); + HeaderMapPtr response_headers( + new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); response_decoder_->decodeHeaders(std::move(response_headers), false); EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(true)); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.upstream_rq_time", _)); @@ -283,10 +283,10 @@ TEST_F(AsyncClientImplTest, CanaryStatusTrue) { EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_time", _)); - EXPECT_CALL(stats_store_, - deliverTimingToSinks("cluster.fake_cluster.canary.upstream_rq_time", _)); + EXPECT_CALL(stats_store_, + deliverTimingToSinks("cluster.fake_cluster.canary.upstream_rq_time", _)); response_decoder_->decodeData(data, true); - } +} TEST_F(AsyncClientImplTest, CanaryStatusFalse) { message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); @@ -307,8 +307,8 @@ TEST_F(AsyncClientImplTest, CanaryStatusFalse) { AsyncClientImpl client(cluster_, *this, stats_store_, dispatcher_, "from_az"); client.send(std::move(message_), callbacks_, Optional()); - HeaderMapPtr response_headers(new HeaderMapImpl{{":status", "200"}, - {"x-envoy-upstream-canary", "false"}}); + HeaderMapPtr response_headers( + new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); response_decoder_->decodeHeaders(std::move(response_headers), false); EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(false)); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.upstream_rq_time", _)); @@ -317,6 +317,6 @@ TEST_F(AsyncClientImplTest, CanaryStatusFalse) { EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_time", _)); response_decoder_->decodeData(data, true); - } +} } // Http diff --git a/test/common/http/codes_test.cc b/test/common/http/codes_test.cc index 6e47afe93499..c1a225bb930a 100644 --- a/test/common/http/codes_test.cc +++ b/test/common/http/codes_test.cc @@ -22,7 +22,8 @@ class CodeUtilityTest : public testing::Test { } CodeUtility::ResponseStatInfo info{store_, "prefix.", headers, internal_request, - request_vhost_name, request_vcluster_name, from_az, to_az}; + request_vhost_name, request_vcluster_name, from_az, to_az, + true}; CodeUtility::chargeResponseStat(info); } From 5b8315795454a3ac76a087385775656c8b63c034 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 31 Aug 2016 17:07:01 -0700 Subject: [PATCH 03/12] Fixed router and added router tests --- source/common/router/router.cc | 10 +++-- test/common/router/router_test.cc | 62 +++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 8066013d84a7..dd0df468b56c 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -94,12 +94,16 @@ const std::string& Filter::upstreamZone() { } void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) { + bool is_canary = + response_headers.get(Http::Headers::get().EnvoyUpstreamCanary) == "true" || upstream_host_ + ? upstream_host_->canary() + : false; if (!callbacks_->requestInfo().healthCheck()) { Http::CodeUtility::ResponseStatInfo info{ config_->stats_store_, stat_prefix_, response_headers, downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true", - route_->virtualHostName(), request_vcluster_name_, config_->service_zone_, upstreamZone(), - upstream_request_->upstream_canary_}; + route_->virtualHostName(), request_vcluster_name_, config_->service_zone_, + upstreamZone(), is_canary}; Http::CodeUtility::chargeResponseStat(info); @@ -107,7 +111,7 @@ void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) { Http::CodeUtility::ResponseStatInfo info{ config_->stats_store_, alt_prefix, response_headers, downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true", "", "", - config_->service_zone_, upstreamZone(), upstream_request_->upstream_canary_}; + config_->service_zone_, upstreamZone(), is_canary}; Http::CodeUtility::chargeResponseStat(info); } diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 0aa5f78a8ec1..beb92fe40431 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -596,6 +596,7 @@ TEST_F(RouterTest, AltStatName) { new Http::HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "true"}, {"x-envoy-virtual-cluster", "hello"}}); + EXPECT_CALL(*cm_.conn_pool_.host_, canary()).WillRepeatedly(Return(true)); response_decoder->decodeHeaders(std::move(response_headers), true); EXPECT_EQ(1U, @@ -710,4 +711,65 @@ TEST(RouterFilterUtilityTest, shouldShadow) { } } +TEST_F(RouterTest, CanaryStatusTrue) { + NiceMock route_entry; + EXPECT_CALL(callbacks_.route_table_, routeForRequest(_)).WillOnce(Return(&route_entry)); + EXPECT_CALL(route_entry, timeout()).WillOnce(Return(std::chrono::milliseconds(0))); + EXPECT_CALL(callbacks_.dispatcher_, createTimer_(_)).Times(0); + + NiceMock encoder; + Http::StreamDecoder* response_decoder = nullptr; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamDecoder& decoder, Http::ConnectionPool::Callbacks& callbacks) + -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder, cm_.conn_pool_.host_); + return nullptr; + })); + + Http::HeaderMapImpl headers{{"x-envoy-upstream-alt-stat-name", "alt_stat"}, + {"x-envoy-internal", "true"}}; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + + Http::HeaderMapPtr response_headers( + new Http::HeaderMapImpl{{":status", "200"}, + {"x-envoy-upstream-canary", "false"}, + {"x-envoy-virtual-cluster", "hello"}}); + EXPECT_CALL(*cm_.conn_pool_.host_, canary()).WillRepeatedly(Return(true)); + response_decoder->decodeHeaders(std::move(response_headers), true); + + EXPECT_EQ(1U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); +} + +TEST_F(RouterTest, CanaryStatusFalse) { + NiceMock route_entry; + EXPECT_CALL(callbacks_.route_table_, routeForRequest(_)).WillOnce(Return(&route_entry)); + EXPECT_CALL(route_entry, timeout()).WillOnce(Return(std::chrono::milliseconds(0))); + EXPECT_CALL(callbacks_.dispatcher_, createTimer_(_)).Times(0); + + NiceMock encoder; + Http::StreamDecoder* response_decoder = nullptr; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamDecoder& decoder, Http::ConnectionPool::Callbacks& callbacks) + -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder, cm_.conn_pool_.host_); + return nullptr; + })); + + Http::HeaderMapImpl headers{{"x-envoy-upstream-alt-stat-name", "alt_stat"}, + {"x-envoy-internal", "true"}}; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + + Http::HeaderMapPtr response_headers( + new Http::HeaderMapImpl{{":status", "200"}, + {"x-envoy-upstream-canary", "false"}, + {"x-envoy-virtual-cluster", "hello"}}); + EXPECT_CALL(*cm_.conn_pool_.host_, canary()).WillRepeatedly(Return(false)); + response_decoder->decodeHeaders(std::move(response_headers), true); + + EXPECT_EQ(0U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); +} } // Router From 179a47f46381a2e22a79c7239f0be0973b07a6e6 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 31 Aug 2016 17:08:02 -0700 Subject: [PATCH 04/12] Fix codes test --- test/common/http/codes_test.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/common/http/codes_test.cc b/test/common/http/codes_test.cc index c1a225bb930a..4c12ae49efbc 100644 --- a/test/common/http/codes_test.cc +++ b/test/common/http/codes_test.cc @@ -17,13 +17,10 @@ class CodeUtilityTest : public testing::Test { const std::string& from_az = EMPTY_STRING, const std::string& to_az = EMPTY_STRING) { HeaderMapImpl headers{{":status", std::to_string(code)}}; - if (canary) { - headers.addViaMove("x-envoy-upstream-canary", "true"); - } CodeUtility::ResponseStatInfo info{store_, "prefix.", headers, internal_request, request_vhost_name, request_vcluster_name, from_az, to_az, - true}; + canary}; CodeUtility::chargeResponseStat(info); } From 320b42fe0e7369a53ab98d5db5f8cdf7398d1985 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 31 Aug 2016 17:08:39 -0700 Subject: [PATCH 05/12] TEMP let roman check --- source/common/http/async_client_impl.cc | 2 ++ source/common/http/codes.cc | 5 +++++ test/common/http/async_client_impl_test.cc | 13 ++++++++++--- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 720184a96291..74a3b1edf981 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -87,6 +87,8 @@ void AsyncRequestImpl::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, response_->headers(), true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone(), is_canary}; + std::cout << "In decodeHeaders with canary:"; + std::cout << is_canary; CodeUtility::chargeResponseStat(info); if (end_stream) { diff --git a/source/common/http/codes.cc b/source/common/http/codes.cc index 3ef9350921c4..38ed85ed3be6 100644 --- a/source/common/http/codes.cc +++ b/source/common/http/codes.cc @@ -26,8 +26,13 @@ void CodeUtility::chargeResponseStat(const ResponseStatInfo& info) { // If the response is from a canary, also create canary stats. if (info.upstream_canary_) { + std::cout << "\n"; + std::cout << fmt::format("{}canary.upstream_rq_{}", info.prefix_, response_code); + std::cout << "\n"; info.store_.counter(fmt::format("{}canary.upstream_rq_{}", info.prefix_, group_string)).inc(); info.store_.counter(fmt::format("{}canary.upstream_rq_{}", info.prefix_, response_code)).inc(); + std::cout << info.store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value(); + std::cout << "\n"; } // Split stats into external vs. internal. diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index 2bd0cee7cafc..a0ac42034e3f 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -274,9 +274,12 @@ TEST_F(AsyncClientImplTest, CanaryStatusTrue) { client.send(std::move(message_), callbacks_, Optional()); HeaderMapPtr response_headers( - new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); - response_decoder_->decodeHeaders(std::move(response_headers), false); + new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "true"}}); EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(true)); + response_decoder_->decodeHeaders(std::move(response_headers), false); + EXPECT_EQ(1U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); + + EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.upstream_rq_time", _)); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.internal.upstream_rq_time", _)); @@ -285,7 +288,9 @@ TEST_F(AsyncClientImplTest, CanaryStatusTrue) { EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.canary.upstream_rq_time", _)); + EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(true)); response_decoder_->decodeData(data, true); + } TEST_F(AsyncClientImplTest, CanaryStatusFalse) { @@ -309,14 +314,16 @@ TEST_F(AsyncClientImplTest, CanaryStatusFalse) { HeaderMapPtr response_headers( new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); + EXPECT_CALL(*conn_pool_.host_, canary()).WillRepeatedly(Return(false)); response_decoder_->decodeHeaders(std::move(response_headers), false); - EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(false)); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.upstream_rq_time", _)); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.internal.upstream_rq_time", _)); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_time", _)); response_decoder_->decodeData(data, true); + EXPECT_EQ(0U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); + } } // Http From b0e61de57028ae3f7ad5e71fa830a0f4497d944a Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 1 Sep 2016 11:28:38 -0700 Subject: [PATCH 06/12] Clean up Async Client test --- source/common/http/async_client_impl.cc | 2 - source/common/http/codes.cc | 5 -- source/common/router/router.cc | 6 +- test/common/http/async_client_impl_test.cc | 97 +++++++++++++++++----- 4 files changed, 81 insertions(+), 29 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 74a3b1edf981..720184a96291 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -87,8 +87,6 @@ void AsyncRequestImpl::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, response_->headers(), true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone(), is_canary}; - std::cout << "In decodeHeaders with canary:"; - std::cout << is_canary; CodeUtility::chargeResponseStat(info); if (end_stream) { diff --git a/source/common/http/codes.cc b/source/common/http/codes.cc index 38ed85ed3be6..3ef9350921c4 100644 --- a/source/common/http/codes.cc +++ b/source/common/http/codes.cc @@ -26,13 +26,8 @@ void CodeUtility::chargeResponseStat(const ResponseStatInfo& info) { // If the response is from a canary, also create canary stats. if (info.upstream_canary_) { - std::cout << "\n"; - std::cout << fmt::format("{}canary.upstream_rq_{}", info.prefix_, response_code); - std::cout << "\n"; info.store_.counter(fmt::format("{}canary.upstream_rq_{}", info.prefix_, group_string)).inc(); info.store_.counter(fmt::format("{}canary.upstream_rq_{}", info.prefix_, response_code)).inc(); - std::cout << info.store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value(); - std::cout << "\n"; } // Split stats into external vs. internal. diff --git a/source/common/router/router.cc b/source/common/router/router.cc index dd0df468b56c..561f68eb3b5c 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -94,7 +94,7 @@ const std::string& Filter::upstreamZone() { } void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) { - bool is_canary = + bool is_canary = response_headers.get(Http::Headers::get().EnvoyUpstreamCanary) == "true" || upstream_host_ ? upstream_host_->canary() : false; @@ -102,8 +102,8 @@ void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) { Http::CodeUtility::ResponseStatInfo info{ config_->stats_store_, stat_prefix_, response_headers, downstream_headers_->get(Http::Headers::get().EnvoyInternalRequest) == "true", - route_->virtualHostName(), request_vcluster_name_, config_->service_zone_, - upstreamZone(), is_canary}; + route_->virtualHostName(), request_vcluster_name_, config_->service_zone_, upstreamZone(), + is_canary}; Http::CodeUtility::chargeResponseStat(info); diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index a0ac42034e3f..2bf2545dd255 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -18,9 +18,9 @@ using testing::ReturnRef; namespace Http { -class AsyncClientImplTest : public testing::Test, public AsyncClientConnPoolFactory { +class AsyncClientImplTestBase : public testing::Test, public AsyncClientConnPoolFactory { public: - AsyncClientImplTest() { + AsyncClientImplTestBase() { HttpTestUtility::addDefaultHeaders(message_->headers()); ON_CALL(*conn_pool_.host_, zone()).WillByDefault(ReturnRef(upstream_zone_)); } @@ -34,13 +34,22 @@ class AsyncClientImplTest : public testing::Test, public AsyncClientConnPoolFact ConnectionPool::MockInstance conn_pool_; NiceMock stream_encoder_; StreamDecoder* response_decoder_{}; - NiceMock stats_store_; NiceMock* timer_; NiceMock dispatcher_; NiceMock cluster_; }; -TEST_F(AsyncClientImplTest, Basic) { +class AsyncClientImplTestMockStats : public AsyncClientImplTestBase { +public: + NiceMock stats_store_; +}; + +class AsyncClientImplTestIsolatedStats : public AsyncClientImplTestBase { +public: + Stats::IsolatedStoreImpl stats_store_; +}; + +TEST_F(AsyncClientImplTestMockStats, Basic) { message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); Buffer::Instance& data = *message_->body(); @@ -76,7 +85,7 @@ TEST_F(AsyncClientImplTest, Basic) { response_decoder_->decodeData(data, true); } -TEST_F(AsyncClientImplTest, MultipleRequests) { +TEST_F(AsyncClientImplTestMockStats, MultipleRequests) { // Send request 1 message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); Buffer::Instance& data = *message_->body(); @@ -123,7 +132,7 @@ TEST_F(AsyncClientImplTest, MultipleRequests) { response_decoder_->decodeData(data, true); } -TEST_F(AsyncClientImplTest, Trailers) { +TEST_F(AsyncClientImplTestMockStats, Trailers) { message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); Buffer::Instance& data = *message_->body(); @@ -147,7 +156,7 @@ TEST_F(AsyncClientImplTest, Trailers) { response_decoder_->decodeTrailers(HeaderMapPtr{new HeaderMapImpl{{"some", "trailer"}}}); } -TEST_F(AsyncClientImplTest, FailRequest) { +TEST_F(AsyncClientImplTestMockStats, FailRequest) { EXPECT_CALL(stats_store_, counter("cluster.fake_cluster.upstream_rq_5xx")); EXPECT_CALL(stats_store_, counter("cluster.fake_cluster.upstream_rq_503")); EXPECT_CALL(stats_store_, counter("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_503")); @@ -170,7 +179,7 @@ TEST_F(AsyncClientImplTest, FailRequest) { stream_encoder_.getStream().resetStream(StreamResetReason::RemoteReset); } -TEST_F(AsyncClientImplTest, CancelRequest) { +TEST_F(AsyncClientImplTestMockStats, CancelRequest) { EXPECT_CALL(conn_pool_, newStream(_, _)) .WillOnce(Invoke([&](StreamDecoder&, ConnectionPool::Callbacks& callbacks) -> ConnectionPool::Cancellable* { @@ -187,7 +196,7 @@ TEST_F(AsyncClientImplTest, CancelRequest) { request->cancel(); } -TEST_F(AsyncClientImplTest, PoolFailure) { +TEST_F(AsyncClientImplTestMockStats, PoolFailure) { EXPECT_CALL(stats_store_, counter("cluster.fake_cluster.upstream_rq_5xx")); EXPECT_CALL(stats_store_, counter("cluster.fake_cluster.upstream_rq_503")); EXPECT_CALL(stats_store_, counter("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_5xx")); @@ -209,7 +218,7 @@ TEST_F(AsyncClientImplTest, PoolFailure) { client.send(std::move(message_), callbacks_, Optional())); } -TEST_F(AsyncClientImplTest, RequestTimeout) { +TEST_F(AsyncClientImplTestMockStats, RequestTimeout) { EXPECT_CALL(stats_store_, counter("cluster.fake_cluster.upstream_rq_5xx")); EXPECT_CALL(stats_store_, counter("cluster.fake_cluster.upstream_rq_504")); EXPECT_CALL(stats_store_, counter("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_5xx")); @@ -235,7 +244,7 @@ TEST_F(AsyncClientImplTest, RequestTimeout) { EXPECT_EQ(1UL, cluster_.stats_store_.counter("cluster.fake_cluster.upstream_rq_timeout").value()); } -TEST_F(AsyncClientImplTest, DisableTimer) { +TEST_F(AsyncClientImplTestMockStats, DisableTimer) { EXPECT_CALL(conn_pool_, newStream(_, _)) .WillOnce(Invoke([&](StreamDecoder&, ConnectionPool::Callbacks& callbacks) -> ConnectionPool::Cancellable* { @@ -254,7 +263,7 @@ TEST_F(AsyncClientImplTest, DisableTimer) { request->cancel(); } -TEST_F(AsyncClientImplTest, CanaryStatusTrue) { +TEST_F(AsyncClientImplTestIsolatedStats, CanaryStatusCounterTrue) { message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); Buffer::Instance& data = *message_->body(); @@ -274,12 +283,62 @@ TEST_F(AsyncClientImplTest, CanaryStatusTrue) { client.send(std::move(message_), callbacks_, Optional()); HeaderMapPtr response_headers( - new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "true"}}); - EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(true)); + new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); + EXPECT_CALL(*conn_pool_.host_, canary()).WillRepeatedly(Return(true)); response_decoder_->decodeHeaders(std::move(response_headers), false); EXPECT_EQ(1U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); + response_decoder_->decodeData(data, true); +} + +TEST_F(AsyncClientImplTestIsolatedStats, CanaryStatusCounterFalse) { + message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); + Buffer::Instance& data = *message_->body(); + + EXPECT_CALL(conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](StreamDecoder& decoder, ConnectionPool::Callbacks& callbacks) + -> ConnectionPool::Cancellable* { + callbacks.onPoolReady(stream_encoder_, conn_pool_.host_); + response_decoder_ = &decoder; + return nullptr; + })); + + EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(ByRef(message_->headers())), false)); + EXPECT_CALL(stream_encoder_, encodeData(BufferEqual(&data), true)); + EXPECT_CALL(callbacks_, onSuccess_(_)); + AsyncClientImpl client(cluster_, *this, stats_store_, dispatcher_, "from_az"); + client.send(std::move(message_), callbacks_, Optional()); + HeaderMapPtr response_headers( + new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); + EXPECT_CALL(*conn_pool_.host_, canary()).WillRepeatedly(Return(false)); + response_decoder_->decodeHeaders(std::move(response_headers), false); + EXPECT_EQ(0U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); + response_decoder_->decodeData(data, true); +} + +TEST_F(AsyncClientImplTestMockStats, CanaryStatusTimingTrue) { + message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); + Buffer::Instance& data = *message_->body(); + + EXPECT_CALL(conn_pool_, newStream(_, _)) + .WillOnce(Invoke([&](StreamDecoder& decoder, ConnectionPool::Callbacks& callbacks) + -> ConnectionPool::Cancellable* { + callbacks.onPoolReady(stream_encoder_, conn_pool_.host_); + response_decoder_ = &decoder; + return nullptr; + })); + + EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(ByRef(message_->headers())), false)); + EXPECT_CALL(stream_encoder_, encodeData(BufferEqual(&data), true)); + EXPECT_CALL(callbacks_, onSuccess_(_)); + + AsyncClientImpl client(cluster_, *this, stats_store_, dispatcher_, "from_az"); + client.send(std::move(message_), callbacks_, Optional()); + + HeaderMapPtr response_headers( + new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); + response_decoder_->decodeHeaders(std::move(response_headers), false); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.upstream_rq_time", _)); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.internal.upstream_rq_time", _)); @@ -290,10 +349,9 @@ TEST_F(AsyncClientImplTest, CanaryStatusTrue) { deliverTimingToSinks("cluster.fake_cluster.canary.upstream_rq_time", _)); EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(true)); response_decoder_->decodeData(data, true); - } -TEST_F(AsyncClientImplTest, CanaryStatusFalse) { +TEST_F(AsyncClientImplTestMockStats, CanaryStatusTimingFalse) { message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); Buffer::Instance& data = *message_->body(); @@ -314,16 +372,17 @@ TEST_F(AsyncClientImplTest, CanaryStatusFalse) { HeaderMapPtr response_headers( new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); - EXPECT_CALL(*conn_pool_.host_, canary()).WillRepeatedly(Return(false)); response_decoder_->decodeHeaders(std::move(response_headers), false); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.upstream_rq_time", _)); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.internal.upstream_rq_time", _)); EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_time", _)); - response_decoder_->decodeData(data, true); - EXPECT_EQ(0U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); + EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.canary.upstream_rq_time", _)) + .Times(0); + EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(false)); + response_decoder_->decodeData(data, true); } } // Http From 740f01c5dea20e530f573e60279fb2fb7624825f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 1 Sep 2016 12:05:22 -0700 Subject: [PATCH 07/12] Check canary status onResetStream and onRequestTimeout --- source/common/http/async_client_impl.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 8a2599c338e4..b2bae28c4dd6 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -139,7 +139,8 @@ void AsyncRequestImpl::onComplete() { void AsyncRequestImpl::onResetStream(StreamResetReason) { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, SERVICE_UNAVAILABLE_HEADER, true, EMPTY_STRING, EMPTY_STRING, - parent_.local_zone_name_, upstreamZone(), false}; + parent_.local_zone_name_, upstreamZone(), + upstream_host_ ? upstream_host_.canary() : false}; CodeUtility::chargeResponseStat(info); callbacks_.onFailure(AsyncClient::FailureReason::Reset); cleanup(); @@ -148,7 +149,8 @@ void AsyncRequestImpl::onResetStream(StreamResetReason) { void AsyncRequestImpl::onRequestTimeout() { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, REQUEST_TIMEOUT_HEADER, true, EMPTY_STRING, EMPTY_STRING, - parent_.local_zone_name_, upstreamZone(), false}; + parent_.local_zone_name_, upstreamZone(), + upstream_host_ ? upstream_host_.canary() : false}; CodeUtility::chargeResponseStat(info); parent_.cluster_.stats().upstream_rq_timeout_.inc(); stream_encoder_->resetStream(); From b0768e1fdd70145d99e4cedd0b4aaccaa2fe8d5b Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 1 Sep 2016 12:13:49 -0700 Subject: [PATCH 08/12] Clean up async client test --- source/common/http/async_client_impl.cc | 4 +- test/common/http/async_client_impl_test.cc | 81 ---------------------- 2 files changed, 2 insertions(+), 83 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index b2bae28c4dd6..a596a37c3c55 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -140,7 +140,7 @@ void AsyncRequestImpl::onResetStream(StreamResetReason) { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, SERVICE_UNAVAILABLE_HEADER, true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone(), - upstream_host_ ? upstream_host_.canary() : false}; + upstream_host_ ? upstream_host_->canary() : false}; CodeUtility::chargeResponseStat(info); callbacks_.onFailure(AsyncClient::FailureReason::Reset); cleanup(); @@ -150,7 +150,7 @@ void AsyncRequestImpl::onRequestTimeout() { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, REQUEST_TIMEOUT_HEADER, true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone(), - upstream_host_ ? upstream_host_.canary() : false}; + upstream_host_ ? upstream_host_->canary() : false}; CodeUtility::chargeResponseStat(info); parent_.cluster_.stats().upstream_rq_timeout_.inc(); stream_encoder_->resetStream(); diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index e056ebc0d0ba..280108698e68 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -276,14 +276,8 @@ TEST_F(AsyncClientImplTestIsolatedStats, CanaryStatusCounterTrue) { response_decoder_ = &decoder; return nullptr; })); - - EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(ByRef(message_->headers())), false)); - EXPECT_CALL(stream_encoder_, encodeData(BufferEqual(&data), true)); - EXPECT_CALL(callbacks_, onSuccess_(_)); - AsyncClientImpl client(cluster_, *this, stats_store_, dispatcher_, "from_az"); client.send(std::move(message_), callbacks_, Optional()); - HeaderMapPtr response_headers( new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); EXPECT_CALL(*conn_pool_.host_, canary()).WillRepeatedly(Return(true)); @@ -303,14 +297,8 @@ TEST_F(AsyncClientImplTestIsolatedStats, CanaryStatusCounterFalse) { response_decoder_ = &decoder; return nullptr; })); - - EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(ByRef(message_->headers())), false)); - EXPECT_CALL(stream_encoder_, encodeData(BufferEqual(&data), true)); - EXPECT_CALL(callbacks_, onSuccess_(_)); - AsyncClientImpl client(cluster_, *this, stats_store_, dispatcher_, "from_az"); client.send(std::move(message_), callbacks_, Optional()); - HeaderMapPtr response_headers( new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); EXPECT_CALL(*conn_pool_.host_, canary()).WillRepeatedly(Return(false)); @@ -318,73 +306,4 @@ TEST_F(AsyncClientImplTestIsolatedStats, CanaryStatusCounterFalse) { EXPECT_EQ(0U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); response_decoder_->decodeData(data, true); } - -TEST_F(AsyncClientImplTestMockStats, CanaryStatusTimingTrue) { - message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); - Buffer::Instance& data = *message_->body(); - - EXPECT_CALL(conn_pool_, newStream(_, _)) - .WillOnce(Invoke([&](StreamDecoder& decoder, ConnectionPool::Callbacks& callbacks) - -> ConnectionPool::Cancellable* { - callbacks.onPoolReady(stream_encoder_, conn_pool_.host_); - response_decoder_ = &decoder; - return nullptr; - })); - - EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(ByRef(message_->headers())), false)); - EXPECT_CALL(stream_encoder_, encodeData(BufferEqual(&data), true)); - EXPECT_CALL(callbacks_, onSuccess_(_)); - - AsyncClientImpl client(cluster_, *this, stats_store_, dispatcher_, "from_az"); - client.send(std::move(message_), callbacks_, Optional()); - - HeaderMapPtr response_headers( - new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); - response_decoder_->decodeHeaders(std::move(response_headers), false); - EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.upstream_rq_time", _)); - EXPECT_CALL(stats_store_, - deliverTimingToSinks("cluster.fake_cluster.internal.upstream_rq_time", _)); - EXPECT_CALL(stats_store_, - deliverTimingToSinks("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_time", _)); - - EXPECT_CALL(stats_store_, - deliverTimingToSinks("cluster.fake_cluster.canary.upstream_rq_time", _)); - EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(true)); - response_decoder_->decodeData(data, true); -} - -TEST_F(AsyncClientImplTestMockStats, CanaryStatusTimingFalse) { - message_->body(Buffer::InstancePtr{new Buffer::OwnedImpl("test body")}); - Buffer::Instance& data = *message_->body(); - - EXPECT_CALL(conn_pool_, newStream(_, _)) - .WillOnce(Invoke([&](StreamDecoder& decoder, ConnectionPool::Callbacks& callbacks) - -> ConnectionPool::Cancellable* { - callbacks.onPoolReady(stream_encoder_, conn_pool_.host_); - response_decoder_ = &decoder; - return nullptr; - })); - - EXPECT_CALL(stream_encoder_, encodeHeaders(HeaderMapEqualRef(ByRef(message_->headers())), false)); - EXPECT_CALL(stream_encoder_, encodeData(BufferEqual(&data), true)); - EXPECT_CALL(callbacks_, onSuccess_(_)); - - AsyncClientImpl client(cluster_, *this, stats_store_, dispatcher_, "from_az"); - client.send(std::move(message_), callbacks_, Optional()); - - HeaderMapPtr response_headers( - new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); - response_decoder_->decodeHeaders(std::move(response_headers), false); - EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.upstream_rq_time", _)); - EXPECT_CALL(stats_store_, - deliverTimingToSinks("cluster.fake_cluster.internal.upstream_rq_time", _)); - EXPECT_CALL(stats_store_, - deliverTimingToSinks("cluster.fake_cluster.zone.from_az.to_az.upstream_rq_time", _)); - - EXPECT_CALL(stats_store_, deliverTimingToSinks("cluster.fake_cluster.canary.upstream_rq_time", _)) - .Times(0); - EXPECT_CALL(*conn_pool_.host_, canary()).WillOnce(Return(false)); - response_decoder_->decodeData(data, true); -} - } // Http From a0393ccd97bdd037bffcb11a845b845d7f34b347 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 1 Sep 2016 14:09:20 -0700 Subject: [PATCH 09/12] Update per comments --- source/common/http/async_client_impl.cc | 21 ++++++++------------- source/common/http/async_client_impl.h | 1 + source/common/router/router.cc | 11 ++++------- test/common/http/async_client_impl_test.cc | 3 +-- test/common/router/router_test.cc | 4 +--- 5 files changed, 15 insertions(+), 25 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index a596a37c3c55..75741cf5b507 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -68,6 +68,11 @@ const std::string& AsyncRequestImpl::upstreamZone() { return upstream_host_ ? upstream_host_->zone() : EMPTY_STRING; } +bool AsyncRequestImpl::isCanary() { + return (response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true") || + (upstream_host_ ? upstream_host_->canary() : false); +} + void AsyncRequestImpl::cancel() { ASSERT(stream_encoder_); stream_encoder_->resetStream(); @@ -81,14 +86,9 @@ void AsyncRequestImpl::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { response_->headers().iterate([](const LowerCaseString& key, const std::string& value) -> void { log_debug(" '{}':'{}'", key.get(), value); }); #endif - bool is_canary = - response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true" || upstream_host_ - ? upstream_host_->canary() - : false; - CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, response_->headers(), true, EMPTY_STRING, EMPTY_STRING, - parent_.local_zone_name_, upstreamZone(), is_canary}; + parent_.local_zone_name_, upstreamZone(), isCanary()}; CodeUtility::chargeResponseStat(info); if (end_stream) { @@ -122,14 +122,9 @@ void AsyncRequestImpl::decodeTrailers(HeaderMapPtr&& trailers) { } void AsyncRequestImpl::onComplete() { - bool is_canary = - response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true" || upstream_host_ - ? upstream_host_->canary() - : false; - CodeUtility::ResponseTimingInfo info{ - parent_.stats_store_, parent_.stat_prefix_, stream_encoder_->requestCompleteTime(), is_canary, - true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone()}; + parent_.stats_store_, parent_.stat_prefix_, stream_encoder_->requestCompleteTime(), + isCanary(), true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone()}; CodeUtility::chargeResponseTiming(info); callbacks_.onSuccess(std::move(response_)); diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 1d183dc370b6..505a00110f69 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -74,6 +74,7 @@ class AsyncRequestImpl final : public AsyncClient::Request, private: const std::string& upstreamZone(); + bool isCanary(); // Http::StreamDecoder void decodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 1219c7147db8..7a98a2328a17 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -94,10 +94,8 @@ const std::string& Filter::upstreamZone() { } void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) { - bool is_canary = - response_headers.get(Http::Headers::get().EnvoyUpstreamCanary) == "true" || upstream_host_ - ? upstream_host_->canary() - : false; + bool is_canary = (response_headers.get(Http::Headers::get().EnvoyUpstreamCanary) == "true") || + (upstream_host_ ? upstream_host_->canary() : false); if (!callbacks_->requestInfo().healthCheck()) { Http::CodeUtility::ResponseStatInfo info{ config_->stats_store_, stat_prefix_, response_headers, @@ -389,9 +387,8 @@ void Filter::onUpstreamHeaders(Http::HeaderMapPtr&& headers, bool end_stream) { } upstream_request_->upstream_canary_ = - headers->get(Http::Headers::get().EnvoyUpstreamCanary) == "true" || upstream_host_ - ? upstream_host_->canary() - : false; + (headers->get(Http::Headers::get().EnvoyUpstreamCanary) == "true") || + (upstream_host_ ? upstream_host_->canary() : false); chargeUpstreamCode(*headers); downstream_response_started_ = true; diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index 280108698e68..8a996b0ae899 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -280,7 +280,7 @@ TEST_F(AsyncClientImplTestIsolatedStats, CanaryStatusCounterTrue) { client.send(std::move(message_), callbacks_, Optional()); HeaderMapPtr response_headers( new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); - EXPECT_CALL(*conn_pool_.host_, canary()).WillRepeatedly(Return(true)); + ON_CALL(*conn_pool_.host_, canary()).WillByDefault(Return(true)); response_decoder_->decodeHeaders(std::move(response_headers), false); EXPECT_EQ(1U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); response_decoder_->decodeData(data, true); @@ -301,7 +301,6 @@ TEST_F(AsyncClientImplTestIsolatedStats, CanaryStatusCounterFalse) { client.send(std::move(message_), callbacks_, Optional()); HeaderMapPtr response_headers( new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}}); - EXPECT_CALL(*conn_pool_.host_, canary()).WillRepeatedly(Return(false)); response_decoder_->decodeHeaders(std::move(response_headers), false); EXPECT_EQ(0U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); response_decoder_->decodeData(data, true); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 019b2606f853..d715ed447075 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -596,7 +596,6 @@ TEST_F(RouterTest, AltStatName) { new Http::HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "true"}, {"x-envoy-virtual-cluster", "hello"}}); - EXPECT_CALL(*cm_.conn_pool_.host_, canary()).WillRepeatedly(Return(true)); response_decoder->decodeHeaders(std::move(response_headers), true); EXPECT_EQ(1U, @@ -736,7 +735,7 @@ TEST_F(RouterTest, CanaryStatusTrue) { new Http::HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}, {"x-envoy-virtual-cluster", "hello"}}); - EXPECT_CALL(*cm_.conn_pool_.host_, canary()).WillRepeatedly(Return(true)); + ON_CALL(*cm_.conn_pool_.host_, canary()).WillByDefault(Return(true)); response_decoder->decodeHeaders(std::move(response_headers), true); EXPECT_EQ(1U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); @@ -767,7 +766,6 @@ TEST_F(RouterTest, CanaryStatusFalse) { new Http::HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}, {"x-envoy-virtual-cluster", "hello"}}); - EXPECT_CALL(*cm_.conn_pool_.host_, canary()).WillRepeatedly(Return(false)); response_decoder->decodeHeaders(std::move(response_headers), true); EXPECT_EQ(0U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); From 82d41c8d3a4f5a43e8651a79b2c138edccd5878d Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 1 Sep 2016 14:35:24 -0700 Subject: [PATCH 10/12] Fix nits --- source/common/http/async_client_impl.cc | 12 +++++++----- source/common/http/async_client_impl.h | 2 +- test/common/router/router_test.cc | 1 + 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 75741cf5b507..b360de776af7 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -68,7 +68,7 @@ const std::string& AsyncRequestImpl::upstreamZone() { return upstream_host_ ? upstream_host_->zone() : EMPTY_STRING; } -bool AsyncRequestImpl::isCanary() { +bool AsyncRequestImpl::isUpstreamCanary() { return (response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true") || (upstream_host_ ? upstream_host_->canary() : false); } @@ -86,9 +86,10 @@ void AsyncRequestImpl::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { response_->headers().iterate([](const LowerCaseString& key, const std::string& value) -> void { log_debug(" '{}':'{}'", key.get(), value); }); #endif + CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, response_->headers(), true, EMPTY_STRING, EMPTY_STRING, - parent_.local_zone_name_, upstreamZone(), isCanary()}; + parent_.local_zone_name_, upstreamZone(), isUpstreamCanary()}; CodeUtility::chargeResponseStat(info); if (end_stream) { @@ -122,9 +123,10 @@ void AsyncRequestImpl::decodeTrailers(HeaderMapPtr&& trailers) { } void AsyncRequestImpl::onComplete() { - CodeUtility::ResponseTimingInfo info{ - parent_.stats_store_, parent_.stat_prefix_, stream_encoder_->requestCompleteTime(), - isCanary(), true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone()}; + CodeUtility::ResponseTimingInfo info{parent_.stats_store_, parent_.stat_prefix_, + stream_encoder_->requestCompleteTime(), isUpstreamCanary(), + true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, + upstreamZone()}; CodeUtility::chargeResponseTiming(info); callbacks_.onSuccess(std::move(response_)); diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 505a00110f69..dd450faed072 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -74,7 +74,7 @@ class AsyncRequestImpl final : public AsyncClient::Request, private: const std::string& upstreamZone(); - bool isCanary(); + bool isUpstreamCanary(); // Http::StreamDecoder void decodeHeaders(HeaderMapPtr&& headers, bool end_stream) override; diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index d715ed447075..815269441790 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -770,4 +770,5 @@ TEST_F(RouterTest, CanaryStatusFalse) { EXPECT_EQ(0U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value()); } + } // Router From e412ad7f914c3dc41ed653d9d24668ba00b32df2 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 1 Sep 2016 17:10:47 -0700 Subject: [PATCH 11/12] Consolidate isUpstreamCanary logic --- source/common/http/async_client_impl.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index b360de776af7..c97ac8fd3523 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -69,8 +69,8 @@ const std::string& AsyncRequestImpl::upstreamZone() { } bool AsyncRequestImpl::isUpstreamCanary() { - return (response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true") || - (upstream_host_ ? upstream_host_->canary() : false); + return (response_ ? (response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true") + : false) || (upstream_host_ ? upstream_host_->canary() : false); } void AsyncRequestImpl::cancel() { @@ -137,7 +137,7 @@ void AsyncRequestImpl::onResetStream(StreamResetReason) { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, SERVICE_UNAVAILABLE_HEADER, true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone(), - upstream_host_ ? upstream_host_->canary() : false}; + isUpstreamCanary()}; CodeUtility::chargeResponseStat(info); callbacks_.onFailure(AsyncClient::FailureReason::Reset); cleanup(); @@ -147,7 +147,7 @@ void AsyncRequestImpl::onRequestTimeout() { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, REQUEST_TIMEOUT_HEADER, true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone(), - upstream_host_ ? upstream_host_->canary() : false}; + isUpstreamCanary()}; CodeUtility::chargeResponseStat(info); parent_.cluster_.stats().upstream_rq_timeout_.inc(); stream_encoder_->resetStream(); From f3f886b1c516bf59e0afa698da6cd4ae75708427 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 1 Sep 2016 17:15:43 -0700 Subject: [PATCH 12/12] Fix format --- source/common/http/async_client_impl.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index c97ac8fd3523..5c0cd0864709 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -69,8 +69,9 @@ const std::string& AsyncRequestImpl::upstreamZone() { } bool AsyncRequestImpl::isUpstreamCanary() { - return (response_ ? (response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true") - : false) || (upstream_host_ ? upstream_host_->canary() : false); + return (response_ ? (response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true") + : false) || + (upstream_host_ ? upstream_host_->canary() : false); } void AsyncRequestImpl::cancel() { @@ -136,8 +137,7 @@ void AsyncRequestImpl::onComplete() { void AsyncRequestImpl::onResetStream(StreamResetReason) { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, SERVICE_UNAVAILABLE_HEADER, true, EMPTY_STRING, EMPTY_STRING, - parent_.local_zone_name_, upstreamZone(), - isUpstreamCanary()}; + parent_.local_zone_name_, upstreamZone(), isUpstreamCanary()}; CodeUtility::chargeResponseStat(info); callbacks_.onFailure(AsyncClient::FailureReason::Reset); cleanup(); @@ -146,8 +146,7 @@ void AsyncRequestImpl::onResetStream(StreamResetReason) { void AsyncRequestImpl::onRequestTimeout() { CodeUtility::ResponseStatInfo info{parent_.stats_store_, parent_.stat_prefix_, REQUEST_TIMEOUT_HEADER, true, EMPTY_STRING, EMPTY_STRING, - parent_.local_zone_name_, upstreamZone(), - isUpstreamCanary()}; + parent_.local_zone_name_, upstreamZone(), isUpstreamCanary()}; CodeUtility::chargeResponseStat(info); parent_.cluster_.stats().upstream_rq_timeout_.inc(); stream_encoder_->resetStream();