Skip to content

stats: fix req/resp body/hdrs sizes tracking #15908

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

Merged
jmarantz merged 57 commits into from
Jun 11, 2021
Merged

stats: fix req/resp body/hdrs sizes tracking #15908

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
6c82da4
cluster: fix req/resp body/hdrs sizes tracking
Apr 9, 2021
1b06299
Wait until route has been cached to lookup cluster info
Apr 9, 2021
dfbd515
Fix tests
Apr 9, 2021
defbc71
DRY up the code a bit
Apr 10, 2021
c4e301a
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 10, 2021
f62ae63
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 12, 2021
de9d812
Add comment noting how rq headers size calculation happens
Apr 12, 2021
4b4216e
auto type deduction fixes
Apr 13, 2021
5dc6418
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 13, 2021
00b31d1
Add integration test
Apr 13, 2021
aab0a19
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 14, 2021
b90ee63
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 14, 2021
40d3e50
Snow's feedback
Apr 15, 2021
d9359c4
Move accounting into the router filter
Apr 15, 2021
28b2e4f
Add unit test plus ...
Apr 15, 2021
89c8008
No need for extra checks of body presence
Apr 15, 2021
f57757f
Test req/resp with trailers
Apr 15, 2021
e6984c0
Accidental newline
Apr 15, 2021
54768a4
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 15, 2021
f07c7df
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 15, 2021
7eaa391
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 16, 2021
8434c88
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 16, 2021
9700510
Fix clang-tidy
Apr 16, 2021
aae8c27
Add changelog entry for the bugfix
Apr 16, 2021
c05e9c9
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 20, 2021
f3f6628
Move accounting into UpstreamRequest
Apr 20, 2021
d267c1f
DRY things up a bit more
Apr 20, 2021
67c1b04
Fix changelog
Apr 20, 2021
52ad169
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 20, 2021
b38cabb
Fix tests.
Apr 20, 2021
b9ff5b3
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 23, 2021
ce177e4
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 26, 2021
cd35f10
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 27, 2021
dd7c8df
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 27, 2021
dd6f038
Merge remote-tracking branch 'upstream/main' into issue-15879
Apr 27, 2021
cb71154
Merge remote-tracking branch 'upstream/main' into issue-15879
May 3, 2021
f1d4073
Merge remote-tracking branch 'upstream/main' into issue-15879
May 10, 2021
37b1c03
Merge remote-tracking branch 'upstream/main' into issue-15879
May 12, 2021
3e2c2e7
Partial review comments
May 12, 2021
0e38f39
Merge remote-tracking branch 'upstream/main' into issue-15879
May 12, 2021
3c87ad4
Merge remote-tracking branch 'upstream/main' into issue-15879
May 14, 2021
fb67e2d
Merge remote-tracking branch 'upstream/main' into issue-15879
May 17, 2021
b930118
Merge remote-tracking branch 'upstream/main' into issue-15879
Jun 2, 2021
ded3f64
Fix docs
Jun 2, 2021
411bc33
Merge remote-tracking branch 'upstream/main' into issue-15879
Jun 9, 2021
d643593
Test multiple upstreams when hedging.
Jun 9, 2021
ee227c3
Fold integration test into protocol_integration_test.cc
Jun 9, 2021
809318a
Merge remote-tracking branch 'upstream/main' into issue-15879
Jun 9, 2021
503687b
Add 100 continue headers
Jun 9, 2021
3839d3a
Try and see if ASSERT_NE(..., nullptr) makes the linter happy
Jun 9, 2021
1ed0fe1
Revert "Try and see if ASSERT_NE(..., nullptr) makes the linter happy"
Jun 9, 2021
b182477
More linter workarounds
Jun 9, 2021
d9eb1ee
Merge remote-tracking branch 'upstream/main' into issue-15879
Jun 10, 2021
30de437
Enable & checks histograms for Retry integration test
Jun 10, 2021
c294b37
Merge remote-tracking branch 'upstream/main' into issue-15879
Jun 10, 2021
4c3e1b2
Fix embarrassing bug 😱
Jun 10, 2021
251d22e
Add empty bodies and bodies of size 0 back
Jun 10, 2021
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
59 changes: 27 additions & 32 deletions source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,18 +669,13 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect

void ConnectionManagerImpl::ActiveStream::completeRequest() {
filter_manager_.streamInfo().onRequestComplete();
Upstream::HostDescriptionConstSharedPtr upstream_host =
connection_manager_.read_callbacks_->upstreamHost();

if (upstream_host != nullptr) {
Upstream::ClusterRequestResponseSizeStatsOptRef req_resp_stats =
upstream_host->cluster().requestResponseSizeStats();
if (req_resp_stats.has_value()) {
req_resp_stats->get().upstream_rq_body_size_.recordValue(
filter_manager_.streamInfo().bytesReceived());
req_resp_stats->get().upstream_rs_body_size_.recordValue(
filter_manager_.streamInfo().bytesSent());
}

auto req_resp_stats = clusterRequestResponseSizeStats();
Comment thread
rgs1 marked this conversation as resolved.
Outdated
if (req_resp_stats.has_value()) {
req_resp_stats->get().upstream_rq_body_size_.recordValue(
filter_manager_.streamInfo().bytesReceived());
req_resp_stats->get().upstream_rs_body_size_.recordValue(
filter_manager_.streamInfo().bytesSent());
}

if (connection_manager_.remote_close_) {
Expand Down Expand Up @@ -777,15 +772,9 @@ void ConnectionManagerImpl::ActiveStream::chargeStats(const ResponseHeaderMap& h
return;
}

Upstream::HostDescriptionConstSharedPtr upstream_host =
connection_manager_.read_callbacks_->upstreamHost();

if (upstream_host != nullptr) {
Upstream::ClusterRequestResponseSizeStatsOptRef req_resp_stats =
upstream_host->cluster().requestResponseSizeStats();
if (req_resp_stats.has_value()) {
req_resp_stats->get().upstream_rs_headers_size_.recordValue(headers.byteSize());
}
auto req_resp_stats = clusterRequestResponseSizeStats();
Comment thread
rgs1 marked this conversation as resolved.
Outdated
if (req_resp_stats.has_value()) {
req_resp_stats->get().upstream_rs_headers_size_.recordValue(headers.byteSize());
}

// No response is sent back downstream for internal redirects, so don't charge downstream stats.
Expand Down Expand Up @@ -849,17 +838,6 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
request_header_timer_.reset();
}

Upstream::HostDescriptionConstSharedPtr upstream_host =
connection_manager_.read_callbacks_->upstreamHost();

if (upstream_host != nullptr) {
Upstream::ClusterRequestResponseSizeStatsOptRef req_resp_stats =
upstream_host->cluster().requestResponseSizeStats();
if (req_resp_stats.has_value()) {
req_resp_stats->get().upstream_rq_headers_size_.recordValue(request_headers_->byteSize());
}
}

// Both saw_connection_close_ and is_head_request_ affect local replies: set
// them as early as possible.
const Protocol protocol = connection_manager_.codec_->protocol();
Expand Down Expand Up @@ -987,6 +965,11 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
ASSERT(!cached_route_);
refreshCachedRoute();

auto req_resp_stats = clusterRequestResponseSizeStats();
Comment thread
rgs1 marked this conversation as resolved.
Outdated
if (req_resp_stats.has_value()) {
req_resp_stats->get().upstream_rq_headers_size_.recordValue(request_headers_->byteSize());
}

Comment thread
rgs1 marked this conversation as resolved.
Outdated
if (!state_.is_internally_created_) { // Only mutate tracing headers on first pass.
filter_manager_.streamInfo().setTraceReason(
ConnectionManagerUtility::mutateTracingRequestHeader(
Expand Down Expand Up @@ -1574,6 +1557,18 @@ Upstream::ClusterInfoConstSharedPtr ConnectionManagerImpl::ActiveStream::cluster
return cached_cluster_info_.value();
}

Upstream::ClusterRequestResponseSizeStatsOptRef
ConnectionManagerImpl::ActiveStream::clusterRequestResponseSizeStats() {
if (cached_cluster_info_.has_value()) {
auto& cluster_info = cached_cluster_info_.value();
if (cluster_info) {
return cluster_info->requestResponseSizeStats();
}
}

return absl::nullopt;
}

Router::RouteConstSharedPtr
ConnectionManagerImpl::ActiveStream::route(const Router::RouteCallback& cb) {
if (cached_route_.has_value()) {
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/conn_manager_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
return connection_manager_.enable_internal_redirects_with_body_;
}

Upstream::ClusterRequestResponseSizeStatsOptRef clusterRequestResponseSizeStats();

void traceRequest();

// Updates the snapped_route_config_ (by reselecting scoped route configuration), if a scope is
Expand Down
78 changes: 40 additions & 38 deletions test/common/http/conn_manager_impl_test_2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2667,16 +2667,17 @@ TEST_F(HttpConnectionManagerImplTest, TestUpstreamRequestHeadersSize) {
.WillOnce(Return(FilterHeadersStatus::StopIteration));
EXPECT_CALL(*decoder_filters_[0], decodeComplete());

std::shared_ptr<NiceMock<Upstream::MockHostDescription>> host_{
new NiceMock<Upstream::MockHostDescription>()};
filter_callbacks_.upstreamHost(host_);
std::shared_ptr<Upstream::MockThreadLocalCluster> cluster =
Comment thread
rgs1 marked this conversation as resolved.
Outdated
std::make_shared<NiceMock<Upstream::MockThreadLocalCluster>>();
EXPECT_CALL(cluster_manager_, getThreadLocalCluster(_)).WillOnce(Return(cluster.get()));

EXPECT_CALL(
host_->cluster_.request_response_size_stats_store_,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rq_headers_size"), 30));
EXPECT_CALL(host_->cluster_.request_response_size_stats_store_,
auto& req_resp_stats = cluster->cluster_.info_->request_response_size_stats_store_;

EXPECT_CALL(req_resp_stats, deliverHistogramToSinks(
Property(&Stats::Metric::name, "upstream_rq_headers_size"), 163));
EXPECT_CALL(req_resp_stats,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rq_body_size"), 0));
EXPECT_CALL(host_->cluster_.request_response_size_stats_store_,
EXPECT_CALL(req_resp_stats,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rs_body_size"), 0));

Buffer::OwnedImpl fake_input("1234");
Expand Down Expand Up @@ -2709,16 +2710,17 @@ TEST_F(HttpConnectionManagerImplTest, TestUpstreamRequestBodySize) {

EXPECT_CALL(*decoder_filters_[0], decodeComplete());

std::shared_ptr<NiceMock<Upstream::MockHostDescription>> host_{
new NiceMock<Upstream::MockHostDescription>()};
filter_callbacks_.upstreamHost(host_);
std::shared_ptr<Upstream::MockThreadLocalCluster> cluster =
Comment thread
rgs1 marked this conversation as resolved.
Outdated
std::make_shared<NiceMock<Upstream::MockThreadLocalCluster>>();
EXPECT_CALL(cluster_manager_, getThreadLocalCluster(_)).WillOnce(Return(cluster.get()));

EXPECT_CALL(
host_->cluster_.request_response_size_stats_store_,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rq_headers_size"), 30));
EXPECT_CALL(host_->cluster_.request_response_size_stats_store_,
auto& req_resp_stats = cluster->cluster_.info_->request_response_size_stats_store_;

EXPECT_CALL(req_resp_stats, deliverHistogramToSinks(
Property(&Stats::Metric::name, "upstream_rq_headers_size"), 163));
EXPECT_CALL(req_resp_stats,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rq_body_size"), 5));
EXPECT_CALL(host_->cluster_.request_response_size_stats_store_,
EXPECT_CALL(req_resp_stats,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rs_body_size"), 0));

Buffer::OwnedImpl fake_input("1234");
Expand Down Expand Up @@ -2753,24 +2755,24 @@ TEST_F(HttpConnectionManagerImplTest, TestUpstreamResponseHeadersSize) {

EXPECT_CALL(*decoder_filters_[0], decodeComplete());

std::shared_ptr<NiceMock<Upstream::MockHostDescription>> host_{
new NiceMock<Upstream::MockHostDescription>()};
filter_callbacks_.upstreamHost(host_);
std::shared_ptr<Upstream::MockThreadLocalCluster> cluster =
std::make_shared<NiceMock<Upstream::MockThreadLocalCluster>>();
EXPECT_CALL(cluster_manager_, getThreadLocalCluster(_)).WillOnce(Return(cluster.get()));

auto& req_resp_stats = cluster->cluster_.info_->request_response_size_stats_store_;

EXPECT_CALL(
host_->cluster_.request_response_size_stats_store_,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rq_headers_size"), 30));
EXPECT_CALL(req_resp_stats, deliverHistogramToSinks(
Property(&Stats::Metric::name, "upstream_rq_headers_size"), 163));

// Response headers are internally mutated and we record final response headers.
// for example in the below test case, response headers are modified as
// {':status', '200' 'date', 'Mon, 06 Jul 2020 06:08:55 GMT' 'server', ''}
// whose size is 49 instead of original response headers size 10({":status", "200"}).
EXPECT_CALL(
host_->cluster_.request_response_size_stats_store_,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rs_headers_size"), 49));
EXPECT_CALL(host_->cluster_.request_response_size_stats_store_,
EXPECT_CALL(req_resp_stats, deliverHistogramToSinks(
Property(&Stats::Metric::name, "upstream_rs_headers_size"), 49));
EXPECT_CALL(req_resp_stats,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rq_body_size"), 4));
EXPECT_CALL(host_->cluster_.request_response_size_stats_store_,
EXPECT_CALL(req_resp_stats,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rs_body_size"), 0));

Buffer::OwnedImpl fake_input("1234");
Expand Down Expand Up @@ -2809,19 +2811,19 @@ TEST_F(HttpConnectionManagerImplTest, TestUpstreamResponseBodySize) {

EXPECT_CALL(*decoder_filters_[0], decodeComplete());

std::shared_ptr<NiceMock<Upstream::MockHostDescription>> host_{
new NiceMock<Upstream::MockHostDescription>()};
filter_callbacks_.upstreamHost(host_);
std::shared_ptr<Upstream::MockThreadLocalCluster> cluster =
std::make_shared<NiceMock<Upstream::MockThreadLocalCluster>>();
EXPECT_CALL(cluster_manager_, getThreadLocalCluster(_)).WillOnce(Return(cluster.get()));

EXPECT_CALL(
host_->cluster_.request_response_size_stats_store_,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rq_headers_size"), 30));
EXPECT_CALL(
host_->cluster_.request_response_size_stats_store_,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rs_headers_size"), 49));
EXPECT_CALL(host_->cluster_.request_response_size_stats_store_,
auto& req_resp_stats = cluster->cluster_.info_->request_response_size_stats_store_;

EXPECT_CALL(req_resp_stats, deliverHistogramToSinks(
Property(&Stats::Metric::name, "upstream_rq_headers_size"), 163));
EXPECT_CALL(req_resp_stats, deliverHistogramToSinks(
Property(&Stats::Metric::name, "upstream_rs_headers_size"), 49));
EXPECT_CALL(req_resp_stats,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rq_body_size"), 4));
EXPECT_CALL(host_->cluster_.request_response_size_stats_store_,
EXPECT_CALL(req_resp_stats,
deliverHistogramToSinks(Property(&Stats::Metric::name, "upstream_rs_body_size"), 11));

Buffer::OwnedImpl fake_input("1234");
Expand Down