Change the way upstream_canary is checked by chargeResponseData

envoyproxy · Aug 31, 2016 · bb4d3a9 · bb4d3a9
1 parent 62c1f60
commit bb4d3a9
Show file tree

Hide file tree

Showing 7 changed files with 33 additions and 22 deletions.
diff --git 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
@@ -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();
   }

diff --git 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
@@ -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
@@ -98,15 +98,16 @@ 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);
 
     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(), 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
@@ -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<std::chrono::milliseconds>());
 
-  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<std::chrono::milliseconds>());
 
-  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
@@ -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);
   }