Skip to content
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.

Sign up for GitHub

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

Network 475 check upstream for canary status #49

Merged
merged 13 commits into from
Sep 2, 2016
Merged

Network 475 check upstream for canary status #49

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
62c1f60
Add checking upstream host for canary to AsyncClient
Aug 30, 2016
bb4d3a9
Change the way upstream_canary is checked by chargeResponseData
Aug 31, 2016
5b83157
Fixed router and added router tests
Sep 1, 2016
179a47f
Fix codes test
Sep 1, 2016
320b42f
TEMP let roman check
Sep 1, 2016
b0e61de
Clean up Async Client test
Sep 1, 2016
4a5509d
Merge branch 'master' of github.com:lyft/envoy into NETWORK-475-check…
Sep 1, 2016
740f01c
Check canary status onResetStream and onRequestTimeout
Sep 1, 2016
b0768e1
Clean up async client test
Sep 1, 2016
a0393cc
Update per comments
Sep 1, 2016
82d41c8
Fix nits
Sep 1, 2016
e412ad7
Consolidate isUpstreamCanary logic
Sep 2, 2016
f3f886b
Fix format
Sep 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions source/common/http/async_client_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,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 =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: keep new line

response_->headers().get(Headers::get().EnvoyUpstreamCanary) == "true" || upstream_host_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic confusing, put parens around the 2nd or case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

? 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) {
Expand Down Expand Up @@ -118,11 +122,14 @@ void AsyncRequestImpl::decodeTrailers(HeaderMapPtr&& trailers) {
}

void AsyncRequestImpl::onComplete() {
// TODO: Check host's canary status in addition to canary header.
bool is_canary =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put this into a small helper function, call from both places, don't need local variable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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", true, EMPTY_STRING,
EMPTY_STRING, parent_.local_zone_name_, upstreamZone()};
parent_.stats_store_, parent_.stat_prefix_, stream_encoder_->requestCompleteTime(), is_canary,
true, EMPTY_STRING, EMPTY_STRING, parent_.local_zone_name_, upstreamZone()};
CodeUtility::chargeResponseTiming(info);

callbacks_.onSuccess(std::move(response_));
Expand All @@ -132,7 +139,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};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here as well

CodeUtility::chargeResponseStat(info);
callbacks_.onFailure(AsyncClient::FailureReason::Reset);
cleanup();
Expand All @@ -141,7 +148,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};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should be able to get if the request was done to canary or not by looking at upstream_host_

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RomanDzhabarov so for this one and the one above we should only rely on upstream_host_ right? Because both headers won't be charged with canary info.

CodeUtility::chargeResponseStat(info);
parent_.cluster_.stats().upstream_rq_timeout_.inc();
stream_encoder_->resetStream();
Expand Down
2 changes: 1 addition & 1 deletion source/common/http/codes.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ void CodeUtility::chargeResponseStat(const ResponseStatInfo& info) {
std::string group_string = groupStringForResponseCode(static_cast<Code>(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();
}
Expand Down
1 change: 1 addition & 0 deletions source/common/http/codes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
};

/**
Expand Down
2 changes: 1 addition & 1 deletion source/common/http/filter/ratelimit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
14 changes: 10 additions & 4 deletions source/common/router/router.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,19 +94,24 @@ const std::string& Filter::upstreamZone() {
}

void Filter::chargeUpstreamCode(const Http::HeaderMap& response_headers) {
bool is_canary =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parens

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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()};
route_->virtualHostName(), request_vcluster_name_, config_->service_zone_, upstreamZone(),
is_canary};

Http::CodeUtility::chargeResponseStat(info);

for (const std::string& alt_prefix : alt_stat_prefixes_) {
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(), is_canary};

Http::CodeUtility::chargeResponseStat(info);
}
Expand Down Expand Up @@ -383,9 +388,10 @@ void Filter::onUpstreamHeaders(Http::HeaderMapPtr&& headers, bool end_stream) {
std::to_string(ms.count()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parens

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}

// 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;
Expand Down
154 changes: 143 additions & 11 deletions test/common/http/async_client_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ using testing::ByRef;
using testing::Invoke;
using testing::NiceMock;
using testing::Ref;
using testing::Return;
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_));
}
Expand All @@ -35,13 +36,22 @@ class AsyncClientImplTest : public testing::Test, public AsyncClientConnPoolFact
ConnectionPool::MockInstance conn_pool_;
NiceMock<MockStreamEncoder> stream_encoder_;
StreamDecoder* response_decoder_{};
NiceMock<Stats::MockStore> stats_store_;
NiceMock<Event::MockTimer>* timer_;
NiceMock<Event::MockDispatcher> dispatcher_;
NiceMock<Upstream::MockCluster> cluster_;
};

TEST_F(AsyncClientImplTest, Basic) {
class AsyncClientImplTestMockStats : public AsyncClientImplTestBase {
public:
NiceMock<Stats::MockStore> 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();

Expand Down Expand Up @@ -77,7 +87,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();
Expand Down Expand Up @@ -124,7 +134,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();

Expand All @@ -148,7 +158,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"));
Expand All @@ -171,7 +181,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* {
Expand All @@ -188,7 +198,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"));
Expand All @@ -210,7 +220,7 @@ TEST_F(AsyncClientImplTest, PoolFailure) {
client.send(std::move(message_), callbacks_, Optional<std::chrono::milliseconds>()));
}

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"));
Expand All @@ -236,7 +246,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* {
Expand All @@ -255,4 +265,126 @@ TEST_F(AsyncClientImplTest, DisableTimer) {
request->cancel();
}

TEST_F(AsyncClientImplTestIsolatedStats, CanaryStatusCounterTrue) {
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<std::chrono::milliseconds>());

HeaderMapPtr response_headers(
new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}});
EXPECT_CALL(*conn_pool_.host_, canary()).WillRepeatedly(Return(true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ON_CALL

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

response_decoder_->decodeHeaders(std::move(response_headers), false);
EXPECT_EQ(1U, stats_store_.counter("cluster.fake_cluster.canary.upstream_rq_200").value());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use expect call here instead of EXPECT_EQ as stats_store_ is MockInstance and not the Isolated one

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<std::chrono::milliseconds>());

HeaderMapPtr response_headers(
new HeaderMapImpl{{":status", "200"}, {"x-envoy-upstream-canary", "false"}});
EXPECT_CALL(*conn_pool_.host_, canary()).WillRepeatedly(Return(false));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete or confirm at least once

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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<std::chrono::milliseconds>());

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<std::chrono::milliseconds>());

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
6 changes: 2 additions & 4 deletions test/common/http/codes_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +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};
request_vhost_name, request_vcluster_name, from_az, to_az,
canary};

CodeUtility::chargeResponseStat(info);
}
Expand Down
Loading