From b4394e97f9405e9cca08c6645c6b4dcbc0f02e32 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Fri, 14 May 2021 20:38:08 -0400 Subject: [PATCH 1/9] [WIP] thrift: add req/resp size histograms Before adding tests/docs/changelog entry, I have a design question: How do we feel about reusing the existing `upstream_{rq,rs}_body_size` stats for clusters? They are originally aimed at HTTP, but I think it's probably fine to reuse them for Thrift (and maybe even some other protos, in the future). Signed-off-by: Raul Gutierrez Segales --- .../thrift_proxy/router/router_impl.cc | 23 +++++++++++++++++++ .../network/thrift_proxy/router/router_impl.h | 5 ++++ 2 files changed, 28 insertions(+) diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc index 498a86bbe61c0..69a125e454d6a 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -316,14 +316,36 @@ FilterStatus Router::messageEnd() { upstream_request_->transport_->encodeFrame(transport_buffer, *upstream_request_->metadata_, upstream_request_buffer_); + + recordRequestSize(transport_buffer.length()); upstream_request_->conn_data_->connection().write(transport_buffer, false); upstream_request_->onRequestComplete(); return FilterStatus::Continue; } +void Router::recordRequestSize(uint64_t value) { + Upstream::ClusterRequestResponseSizeStatsOptRef req_resp_stats_opt = + cluster_->requestResponseSizeStats(); + if (req_resp_stats_opt.has_value()) { + auto& req_resp_stats = req_resp_stats_opt->get(); + req_resp_stats.upstream_rq_body_size_.recordValue(value); + } +} + +void Router::recordResponseSize(uint64_t value) { + Upstream::ClusterRequestResponseSizeStatsOptRef req_resp_stats_opt = + cluster_->requestResponseSizeStats(); + if (req_resp_stats_opt.has_value()) { + auto& req_resp_stats = req_resp_stats_opt->get(); + req_resp_stats.upstream_rq_body_size_.recordValue(value); + } +} + void Router::onUpstreamData(Buffer::Instance& data, bool end_stream) { ASSERT(!upstream_request_->response_complete_); + recordResponseSize(data.length()); + if (upstream_request_->upgrade_response_ != nullptr) { ENVOY_STREAM_LOG(trace, "reading upgrade response: {} bytes", *callbacks_, data.length()); // Handle upgrade response. @@ -503,6 +525,7 @@ void Router::UpstreamRequest::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr upgrade_response_ = protocol_->attemptUpgrade(*transport_, *conn_state_, parent_.upstream_request_buffer_); if (upgrade_response_ != nullptr) { + parent_.recordRequestSize(parent_.upstream_request_buffer_.length()); conn_data_->connection().write(parent_.upstream_request_buffer_, false); return; } diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.h b/source/extensions/filters/network/thrift_proxy/router/router_impl.h index e125c69fce0b8..5a94b5ff2acb5 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -286,6 +286,11 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, POOL_HISTOGRAM_PREFIX(scope, prefix))}; } + friend class UpstreamRequest; + + void recordRequestSize(uint64_t value); + void recordResponseSize(uint64_t value); + Upstream::ClusterManager& cluster_manager_; RouterStats stats_; Stats::StatNameSetPtr stat_name_set_; From d26ce840307671c9945054519bc1d4a87a17499c Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Sat, 15 May 2021 09:40:46 -0400 Subject: [PATCH 2/9] Fix CI release runs Signed-off-by: Raul Gutierrez Segales --- .../filters/network/thrift_proxy/router/router_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.h b/source/extensions/filters/network/thrift_proxy/router/router_impl.h index 5a94b5ff2acb5..5a6f1381205f6 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -286,7 +286,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, POOL_HISTOGRAM_PREFIX(scope, prefix))}; } - friend class UpstreamRequest; + friend struct UpstreamRequest; void recordRequestSize(uint64_t value); void recordResponseSize(uint64_t value); From 8c42f41d094b1214c537285b38e25124a9e887fc Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Sat, 15 May 2021 19:57:44 -0400 Subject: [PATCH 3/9] Fix copy/pasta + DRY things up Signed-off-by: Raul Gutierrez Segales --- .../network/thrift_proxy/router/router_impl.cc | 15 +++++++++------ .../network/thrift_proxy/router/router_impl.h | 1 + 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc index 69a125e454d6a..304adaddcf141 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -324,20 +324,23 @@ FilterStatus Router::messageEnd() { } void Router::recordRequestSize(uint64_t value) { - Upstream::ClusterRequestResponseSizeStatsOptRef req_resp_stats_opt = - cluster_->requestResponseSizeStats(); - if (req_resp_stats_opt.has_value()) { - auto& req_resp_stats = req_resp_stats_opt->get(); + record([value](Upstream::ClusterRequestResponseSizeStats& req_resp_stats) { req_resp_stats.upstream_rq_body_size_.recordValue(value); - } + }); } void Router::recordResponseSize(uint64_t value) { + record([value](Upstream::ClusterRequestResponseSizeStats& req_resp_stats) { + req_resp_stats.upstream_rs_body_size_.recordValue(value); + }); +} + +void Router::record(std::function callback) { Upstream::ClusterRequestResponseSizeStatsOptRef req_resp_stats_opt = cluster_->requestResponseSizeStats(); if (req_resp_stats_opt.has_value()) { auto& req_resp_stats = req_resp_stats_opt->get(); - req_resp_stats.upstream_rq_body_size_.recordValue(value); + callback(req_resp_stats); } } diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.h b/source/extensions/filters/network/thrift_proxy/router/router_impl.h index 5a6f1381205f6..8b7c6d84aef2c 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -290,6 +290,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, void recordRequestSize(uint64_t value); void recordResponseSize(uint64_t value); + void record(std::function callback); Upstream::ClusterManager& cluster_manager_; RouterStats stats_; From 46c5b863e783104dc35778b8c7326990caec597f Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Mon, 17 May 2021 16:44:33 -0400 Subject: [PATCH 4/9] Add new stats at the router level Make things match with what #15884 does. Signed-off-by: Raul Gutierrez Segales --- .../thrift_filters/router_filter.rst | 2 ++ docs/root/version_history/current.rst | 1 + .../thrift_proxy/router/router_impl.cc | 30 ++++--------------- .../network/thrift_proxy/router/router_impl.h | 10 +++---- 4 files changed, 13 insertions(+), 30 deletions(-) diff --git a/docs/root/configuration/other_protocols/thrift_filters/router_filter.rst b/docs/root/configuration/other_protocols/thrift_filters/router_filter.rst index a6d5c50ad4d2e..0723e62215d1d 100644 --- a/docs/root/configuration/other_protocols/thrift_filters/router_filter.rst +++ b/docs/root/configuration/other_protocols/thrift_filters/router_filter.rst @@ -41,3 +41,5 @@ Since these stats utilize the underlying cluster scope, we prefix with the ``thr thrift.upstream_resp_exception, Counter, Total responses with the "Exception" message type. thrift.upstream_resp_invalid_type, Counter, Total responses with an unsupported message type. thrift.upstream_rq_time, Histogram, total rq time from rq complete to resp complete; includes oneway messages. + thrift.upstream_rq_size, Histogram, Request message size in bytes per upstream + thrift.upstream_rs_size, Histogram, Response message size in bytes per upstream diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index b00d52cbde733..3a3ec192e3459 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -68,6 +68,7 @@ New Features * http: added upstream and downstream alpha HTTP/3 support! See :ref:`quic_options ` for downstream and the new http3_protocol_options in :ref:`http_protocol_options ` for upstream HTTP/3. * listener: added ability to change an existing listener's address. * metric service: added support for sending metric tags as labels. This can be enabled by setting the :ref:`emit_tags_as_labels ` field to true. +* thrift_proxy: added per upstream metrics within the :ref:`thrift router ` for request and response size histograms. * udp_proxy: added :ref:`key ` as another hash policy to support hash based routing on any given key. Deprecated diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc index 304adaddcf141..83d885b793264 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -317,37 +317,17 @@ FilterStatus Router::messageEnd() { upstream_request_->transport_->encodeFrame(transport_buffer, *upstream_request_->metadata_, upstream_request_buffer_); - recordRequestSize(transport_buffer.length()); + recordClusterScopeHistogram({upstream_rq_size_}, Stats::Histogram::Unit::Bytes, + transport_buffer.length()); upstream_request_->conn_data_->connection().write(transport_buffer, false); upstream_request_->onRequestComplete(); return FilterStatus::Continue; } -void Router::recordRequestSize(uint64_t value) { - record([value](Upstream::ClusterRequestResponseSizeStats& req_resp_stats) { - req_resp_stats.upstream_rq_body_size_.recordValue(value); - }); -} - -void Router::recordResponseSize(uint64_t value) { - record([value](Upstream::ClusterRequestResponseSizeStats& req_resp_stats) { - req_resp_stats.upstream_rs_body_size_.recordValue(value); - }); -} - -void Router::record(std::function callback) { - Upstream::ClusterRequestResponseSizeStatsOptRef req_resp_stats_opt = - cluster_->requestResponseSizeStats(); - if (req_resp_stats_opt.has_value()) { - auto& req_resp_stats = req_resp_stats_opt->get(); - callback(req_resp_stats); - } -} - void Router::onUpstreamData(Buffer::Instance& data, bool end_stream) { ASSERT(!upstream_request_->response_complete_); - recordResponseSize(data.length()); + recordClusterScopeHistogram({upstream_rs_size_}, Stats::Histogram::Unit::Bytes, data.length()); if (upstream_request_->upgrade_response_ != nullptr) { ENVOY_STREAM_LOG(trace, "reading upgrade response: {} bytes", *callbacks_, data.length()); @@ -528,7 +508,9 @@ void Router::UpstreamRequest::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr upgrade_response_ = protocol_->attemptUpgrade(*transport_, *conn_state_, parent_.upstream_request_buffer_); if (upgrade_response_ != nullptr) { - parent_.recordRequestSize(parent_.upstream_request_buffer_.length()); + parent_.recordClusterScopeHistogram({parent_.upstream_rq_size_}, + Stats::Histogram::Unit::Bytes, + parent_.upstream_request_buffer_.length()); conn_data_->connection().write(parent_.upstream_request_buffer_, false); return; } diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.h b/source/extensions/filters/network/thrift_proxy/router/router_impl.h index 8b7c6d84aef2c..b863b9af95988 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -190,6 +190,8 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, upstream_resp_exception_(stat_name_set_->add("thrift.upstream_resp_exception")), upstream_resp_invalid_type_(stat_name_set_->add("thrift.upstream_resp_invalid_type")), upstream_rq_time_(stat_name_set_->add("thrift.upstream_rq_time")), + upstream_rq_size_(stat_name_set_->add("thrift.upstream_rq_size")), + upstream_rs_size_(stat_name_set_->add("thrift.upstream_rs_size")), passthrough_supported_(false) {} ~Router() override = default; @@ -286,12 +288,6 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, POOL_HISTOGRAM_PREFIX(scope, prefix))}; } - friend struct UpstreamRequest; - - void recordRequestSize(uint64_t value); - void recordResponseSize(uint64_t value); - void record(std::function callback); - Upstream::ClusterManager& cluster_manager_; RouterStats stats_; Stats::StatNameSetPtr stat_name_set_; @@ -305,6 +301,8 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, const Stats::StatName upstream_resp_exception_; const Stats::StatName upstream_resp_invalid_type_; const Stats::StatName upstream_rq_time_; + const Stats::StatName upstream_rq_size_; + const Stats::StatName upstream_rs_size_; ThriftFilters::DecoderFilterCallbacks* callbacks_{}; RouteConstSharedPtr route_{}; From 3d675c2f0eaa7b192db8809d8d301e6057adc060 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Mon, 17 May 2021 18:26:26 -0400 Subject: [PATCH 5/9] Update & add tests Signed-off-by: Raul Gutierrez Segales --- .../network/thrift_proxy/router_test.cc | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/test/extensions/filters/network/thrift_proxy/router_test.cc b/test/extensions/filters/network/thrift_proxy/router_test.cc index 03448be78b93a..69e24b2a23765 100644 --- a/test/extensions/filters/network/thrift_proxy/router_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_test.cc @@ -1022,6 +1022,17 @@ TEST_P(ThriftRouterFieldTypeTest, CallWithUpstreamRqTime) { EXPECT_CALL(cluster_scope, counter("thrift.upstream_resp_reply")); EXPECT_CALL(cluster_scope, counter("thrift.upstream_resp_success")); + EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rq_size", Stats::Histogram::Unit::Bytes)); + EXPECT_CALL(cluster_scope, + deliverHistogramToSinks( + testing::Property(&Stats::Metric::name, "thrift.upstream_rq_size"), _)); + EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rs_size", Stats::Histogram::Unit::Bytes)) + .Times(2); + EXPECT_CALL(cluster_scope, + deliverHistogramToSinks( + testing::Property(&Stats::Metric::name, "thrift.upstream_rs_size"), _)) + .Times(2); + startRequest(MessageType::Call); connectUpstream(); sendTrivialStruct(field_type); @@ -1297,6 +1308,41 @@ TEST_P(ThriftRouterPassthroughTest, PassthroughEnable) { ConnectionPool::PoolFailureReason::RemoteConnectionFailure); } +TEST_F(ThriftRouterTest, RequestResponseSize) { + initializeRouter(); + + Stats::MockStore cluster_scope; + ON_CALL(*context_.cluster_manager_.thread_local_cluster_.cluster_.info_, statsScope()) + .WillByDefault(ReturnRef(cluster_scope)); + + EXPECT_CALL(cluster_scope, counter("thrift.upstream_rq_call")).Times(AtLeast(1)); + EXPECT_CALL(cluster_scope, counter("thrift.upstream_resp_reply")).Times(AtLeast(1)); + EXPECT_CALL(cluster_scope, counter("thrift.upstream_resp_success")).Times(AtLeast(1)); + + EXPECT_CALL(cluster_scope, + histogram("thrift.upstream_rq_time", Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(cluster_scope, + deliverHistogramToSinks( + testing::Property(&Stats::Metric::name, "thrift.upstream_rq_time"), _)); + + EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rq_size", Stats::Histogram::Unit::Bytes)); + EXPECT_CALL(cluster_scope, + deliverHistogramToSinks( + testing::Property(&Stats::Metric::name, "thrift.upstream_rq_size"), _)); + EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rs_size", Stats::Histogram::Unit::Bytes)) + .Times(2); + EXPECT_CALL(cluster_scope, + deliverHistogramToSinks( + testing::Property(&Stats::Metric::name, "thrift.upstream_rs_size"), _)) + .Times(2); + + startRequestWithExistingConnection(MessageType::Call); + sendTrivialStruct(FieldType::I32); + completeRequest(); + returnResponse(); + destroyRouter(); +} + } // namespace Router } // namespace ThriftProxy } // namespace NetworkFilters From 520f22236a477c37492afb866834e090315e1dba Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 18 May 2021 13:29:51 -0400 Subject: [PATCH 6/9] Fix sorting Signed-off-by: Raul Gutierrez Segales --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index a41c5d3d00504..74e5583cd1c34 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -69,8 +69,8 @@ New Features * http: added upstream and downstream alpha HTTP/3 support! See :ref:`quic_options ` for downstream and the new http3_protocol_options in :ref:`http_protocol_options ` for upstream HTTP/3. * listener: added ability to change an existing listener's address. * metric service: added support for sending metric tags as labels. This can be enabled by setting the :ref:`emit_tags_as_labels ` field to true. -* thrift_proxy: added per upstream metrics within the :ref:`thrift router ` for request and response size histograms. * tcp: added support for :ref:`preconnecting `. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic. +* thrift_proxy: added per upstream metrics within the :ref:`thrift router ` for request and response size histograms. * udp_proxy: added :ref:`key ` as another hash policy to support hash based routing on any given key. Deprecated From 9c69f8c16de8ea230bc3e549f9c66da4188c8705 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 18 May 2021 13:39:41 -0400 Subject: [PATCH 7/9] Test the protocol upgrade path Signed-off-by: Raul Gutierrez Segales --- .../network/thrift_proxy/router_test.cc | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/test/extensions/filters/network/thrift_proxy/router_test.cc b/test/extensions/filters/network/thrift_proxy/router_test.cc index 69e24b2a23765..c93b22b2c450f 100644 --- a/test/extensions/filters/network/thrift_proxy/router_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_test.cc @@ -740,6 +740,14 @@ TEST_F(ThriftRouterTest, UnexpectedRouterDestroy) { } TEST_F(ThriftRouterTest, ProtocolUpgrade) { + Stats::MockStore cluster_scope; + ON_CALL(*context_.cluster_manager_.thread_local_cluster_.cluster_.info_, statsScope()) + .WillByDefault(ReturnRef(cluster_scope)); + + EXPECT_CALL(cluster_scope, counter("thrift.upstream_rq_call")).Times(1); + EXPECT_CALL(cluster_scope, counter("thrift.upstream_resp_reply")).Times(1); + EXPECT_CALL(cluster_scope, counter("thrift.upstream_resp_success")).Times(1); + initializeRouter(); startRequest(MessageType::Call); @@ -760,6 +768,25 @@ TEST_F(ThriftRouterTest, ProtocolUpgrade) { EXPECT_CALL(*protocol_, supportsUpgrade()).WillOnce(Return(true)); + EXPECT_CALL(cluster_scope, + histogram("thrift.upstream_rq_time", Stats::Histogram::Unit::Milliseconds)); + EXPECT_CALL(cluster_scope, + deliverHistogramToSinks( + testing::Property(&Stats::Metric::name, "thrift.upstream_rq_time"), _)); + + EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rq_size", Stats::Histogram::Unit::Bytes)) + .Times(2); + EXPECT_CALL(cluster_scope, + deliverHistogramToSinks( + testing::Property(&Stats::Metric::name, "thrift.upstream_rq_size"), _)) + .Times(2); + EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rs_size", Stats::Histogram::Unit::Bytes)) + .Times(4); + EXPECT_CALL(cluster_scope, + deliverHistogramToSinks( + testing::Property(&Stats::Metric::name, "thrift.upstream_rs_size"), _)) + .Times(4); + MockThriftObject* upgrade_response = new NiceMock(); EXPECT_CALL(*protocol_, attemptUpgrade(_, _, _)) @@ -796,16 +823,6 @@ TEST_F(ThriftRouterTest, ProtocolUpgrade) { completeRequest(); returnResponse(); destroyRouter(); - - EXPECT_EQ(1UL, context_.cluster_manager_.thread_local_cluster_.cluster_.info_->statsScope() - .counterFromString("thrift.upstream_rq_call") - .value()); - EXPECT_EQ(1UL, context_.cluster_manager_.thread_local_cluster_.cluster_.info_->statsScope() - .counterFromString("thrift.upstream_resp_reply") - .value()); - EXPECT_EQ(1UL, context_.cluster_manager_.thread_local_cluster_.cluster_.info_->statsScope() - .counterFromString("thrift.upstream_resp_success") - .value()); } // Test the case where an upgrade will occur, but the conn pool From af22dc9294484f25fd303c51edd28f18a8ef2c10 Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Tue, 18 May 2021 18:30:39 -0400 Subject: [PATCH 8/9] Fix format Signed-off-by: Raul Gutierrez Segales --- test/extensions/filters/network/thrift_proxy/router_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/extensions/filters/network/thrift_proxy/router_test.cc b/test/extensions/filters/network/thrift_proxy/router_test.cc index c93b22b2c450f..51f21275e0937 100644 --- a/test/extensions/filters/network/thrift_proxy/router_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_test.cc @@ -744,9 +744,9 @@ TEST_F(ThriftRouterTest, ProtocolUpgrade) { ON_CALL(*context_.cluster_manager_.thread_local_cluster_.cluster_.info_, statsScope()) .WillByDefault(ReturnRef(cluster_scope)); - EXPECT_CALL(cluster_scope, counter("thrift.upstream_rq_call")).Times(1); - EXPECT_CALL(cluster_scope, counter("thrift.upstream_resp_reply")).Times(1); - EXPECT_CALL(cluster_scope, counter("thrift.upstream_resp_success")).Times(1); + EXPECT_CALL(cluster_scope, counter("thrift.upstream_rq_call")); + EXPECT_CALL(cluster_scope, counter("thrift.upstream_resp_reply")); + EXPECT_CALL(cluster_scope, counter("thrift.upstream_resp_success")); initializeRouter(); startRequest(MessageType::Call); From 56817edb6ee1d4d4026ffba21b4183dac47fb5dc Mon Sep 17 00:00:00 2001 From: Raul Gutierrez Segales Date: Thu, 20 May 2021 11:32:36 -0400 Subject: [PATCH 9/9] Fixes: * s/rs/resp/ * account for full responses instead of partial reads * account protocol upgrade as part of their original request/response * docs update Signed-off-by: Raul Gutierrez Segales --- .../thrift_filters/router_filter.rst | 7 +++++- .../thrift_proxy/router/router_impl.cc | 15 +++++++----- .../network/thrift_proxy/router/router_impl.h | 6 +++-- .../network/thrift_proxy/router_test.cc | 24 +++++++------------ 4 files changed, 27 insertions(+), 25 deletions(-) diff --git a/docs/root/configuration/other_protocols/thrift_filters/router_filter.rst b/docs/root/configuration/other_protocols/thrift_filters/router_filter.rst index 0723e62215d1d..dae0be24d7429 100644 --- a/docs/root/configuration/other_protocols/thrift_filters/router_filter.rst +++ b/docs/root/configuration/other_protocols/thrift_filters/router_filter.rst @@ -42,4 +42,9 @@ Since these stats utilize the underlying cluster scope, we prefix with the ``thr thrift.upstream_resp_invalid_type, Counter, Total responses with an unsupported message type. thrift.upstream_rq_time, Histogram, total rq time from rq complete to resp complete; includes oneway messages. thrift.upstream_rq_size, Histogram, Request message size in bytes per upstream - thrift.upstream_rs_size, Histogram, Response message size in bytes per upstream + thrift.upstream_resp_size, Histogram, Response message size in bytes per upstream + +.. note:: + + The request and response size histograms include what's sent and received during protocol upgrade. + However, invalid responses are not included in the response size histogram. diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc index 83d885b793264..5712ea8dd171e 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.cc +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.cc @@ -317,8 +317,9 @@ FilterStatus Router::messageEnd() { upstream_request_->transport_->encodeFrame(transport_buffer, *upstream_request_->metadata_, upstream_request_buffer_); - recordClusterScopeHistogram({upstream_rq_size_}, Stats::Histogram::Unit::Bytes, - transport_buffer.length()); + request_size_ += transport_buffer.length(); + recordClusterScopeHistogram({upstream_rq_size_}, Stats::Histogram::Unit::Bytes, request_size_); + upstream_request_->conn_data_->connection().write(transport_buffer, false); upstream_request_->onRequestComplete(); return FilterStatus::Continue; @@ -327,7 +328,7 @@ FilterStatus Router::messageEnd() { void Router::onUpstreamData(Buffer::Instance& data, bool end_stream) { ASSERT(!upstream_request_->response_complete_); - recordClusterScopeHistogram({upstream_rs_size_}, Stats::Histogram::Unit::Bytes, data.length()); + response_size_ += data.length(); if (upstream_request_->upgrade_response_ != nullptr) { ENVOY_STREAM_LOG(trace, "reading upgrade response: {} bytes", *callbacks_, data.length()); @@ -356,6 +357,9 @@ void Router::onUpstreamData(Buffer::Instance& data, bool end_stream) { ThriftFilters::ResponseStatus status = callbacks_->upstreamData(data); if (status == ThriftFilters::ResponseStatus::Complete) { ENVOY_STREAM_LOG(debug, "response complete", *callbacks_); + recordClusterScopeHistogram({upstream_resp_size_}, Stats::Histogram::Unit::Bytes, + response_size_); + switch (callbacks_->responseMetadata()->messageType()) { case MessageType::Reply: incClusterScopeCounter({upstream_resp_reply_}); @@ -378,6 +382,7 @@ void Router::onUpstreamData(Buffer::Instance& data, bool end_stream) { cleanup(); return; } else if (status == ThriftFilters::ResponseStatus::Reset) { + // Note: invalid responses are not accounted in the response size histogram. ENVOY_STREAM_LOG(debug, "upstream reset", *callbacks_); upstream_request_->resetStream(); return; @@ -508,9 +513,7 @@ void Router::UpstreamRequest::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr upgrade_response_ = protocol_->attemptUpgrade(*transport_, *conn_state_, parent_.upstream_request_buffer_); if (upgrade_response_ != nullptr) { - parent_.recordClusterScopeHistogram({parent_.upstream_rq_size_}, - Stats::Histogram::Unit::Bytes, - parent_.upstream_request_buffer_.length()); + parent_.request_size_ += parent_.upstream_request_buffer_.length(); conn_data_->connection().write(parent_.upstream_request_buffer_, false); return; } diff --git a/source/extensions/filters/network/thrift_proxy/router/router_impl.h b/source/extensions/filters/network/thrift_proxy/router/router_impl.h index b863b9af95988..c12a0a34df8d5 100644 --- a/source/extensions/filters/network/thrift_proxy/router/router_impl.h +++ b/source/extensions/filters/network/thrift_proxy/router/router_impl.h @@ -191,7 +191,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, upstream_resp_invalid_type_(stat_name_set_->add("thrift.upstream_resp_invalid_type")), upstream_rq_time_(stat_name_set_->add("thrift.upstream_rq_time")), upstream_rq_size_(stat_name_set_->add("thrift.upstream_rq_size")), - upstream_rs_size_(stat_name_set_->add("thrift.upstream_rs_size")), + upstream_resp_size_(stat_name_set_->add("thrift.upstream_resp_size")), passthrough_supported_(false) {} ~Router() override = default; @@ -302,7 +302,7 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, const Stats::StatName upstream_resp_invalid_type_; const Stats::StatName upstream_rq_time_; const Stats::StatName upstream_rq_size_; - const Stats::StatName upstream_rs_size_; + const Stats::StatName upstream_resp_size_; ThriftFilters::DecoderFilterCallbacks* callbacks_{}; RouteConstSharedPtr route_{}; @@ -313,6 +313,8 @@ class Router : public Tcp::ConnectionPool::UpstreamCallbacks, Buffer::OwnedImpl upstream_request_buffer_; bool passthrough_supported_ : 1; + uint64_t request_size_{}; + uint64_t response_size_{}; }; } // namespace Router diff --git a/test/extensions/filters/network/thrift_proxy/router_test.cc b/test/extensions/filters/network/thrift_proxy/router_test.cc index 51f21275e0937..d17758c682bd5 100644 --- a/test/extensions/filters/network/thrift_proxy/router_test.cc +++ b/test/extensions/filters/network/thrift_proxy/router_test.cc @@ -774,18 +774,14 @@ TEST_F(ThriftRouterTest, ProtocolUpgrade) { deliverHistogramToSinks( testing::Property(&Stats::Metric::name, "thrift.upstream_rq_time"), _)); - EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rq_size", Stats::Histogram::Unit::Bytes)) - .Times(2); + EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rq_size", Stats::Histogram::Unit::Bytes)); EXPECT_CALL(cluster_scope, deliverHistogramToSinks( - testing::Property(&Stats::Metric::name, "thrift.upstream_rq_size"), _)) - .Times(2); - EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rs_size", Stats::Histogram::Unit::Bytes)) - .Times(4); + testing::Property(&Stats::Metric::name, "thrift.upstream_rq_size"), _)); + EXPECT_CALL(cluster_scope, histogram("thrift.upstream_resp_size", Stats::Histogram::Unit::Bytes)); EXPECT_CALL(cluster_scope, deliverHistogramToSinks( - testing::Property(&Stats::Metric::name, "thrift.upstream_rs_size"), _)) - .Times(4); + testing::Property(&Stats::Metric::name, "thrift.upstream_resp_size"), _)); MockThriftObject* upgrade_response = new NiceMock(); @@ -1043,12 +1039,10 @@ TEST_P(ThriftRouterFieldTypeTest, CallWithUpstreamRqTime) { EXPECT_CALL(cluster_scope, deliverHistogramToSinks( testing::Property(&Stats::Metric::name, "thrift.upstream_rq_size"), _)); - EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rs_size", Stats::Histogram::Unit::Bytes)) - .Times(2); + EXPECT_CALL(cluster_scope, histogram("thrift.upstream_resp_size", Stats::Histogram::Unit::Bytes)); EXPECT_CALL(cluster_scope, deliverHistogramToSinks( - testing::Property(&Stats::Metric::name, "thrift.upstream_rs_size"), _)) - .Times(2); + testing::Property(&Stats::Metric::name, "thrift.upstream_resp_size"), _)); startRequest(MessageType::Call); connectUpstream(); @@ -1346,12 +1340,10 @@ TEST_F(ThriftRouterTest, RequestResponseSize) { EXPECT_CALL(cluster_scope, deliverHistogramToSinks( testing::Property(&Stats::Metric::name, "thrift.upstream_rq_size"), _)); - EXPECT_CALL(cluster_scope, histogram("thrift.upstream_rs_size", Stats::Histogram::Unit::Bytes)) - .Times(2); + EXPECT_CALL(cluster_scope, histogram("thrift.upstream_resp_size", Stats::Histogram::Unit::Bytes)); EXPECT_CALL(cluster_scope, deliverHistogramToSinks( - testing::Property(&Stats::Metric::name, "thrift.upstream_rs_size"), _)) - .Times(2); + testing::Property(&Stats::Metric::name, "thrift.upstream_resp_size"), _)); startRequestWithExistingConnection(MessageType::Call); sendTrivialStruct(FieldType::I32);