stats: fix req/resp body/hdrs sizes tracking by rgs1 · Pull Request #15908 · envoyproxy/envoy

rgs1 · 2021-04-09T21:47:50Z

Perform the accounting for these stats from UpstreamRequest, where we
get access to the correct numbers.

Things fixed:

values are recorded now (they weren't previously)
values will be exactly what's sent/received from upstream since they are recorded
in the router after/before other filters
the destination cluster will be the right one
reqs/resps with trailers are handled (they weren't previously)
added an integration test to prevent regressions

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

Using the upstream_host to find the upstream cluster doesn't work from ActiveStream, because it may not be available anymore by the time we look it up. So use ClusterInfo instead. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2021-04-09T21:48:18Z

I'll follow-up with updated unit tests and an integration test.

rgs1 · 2021-04-09T21:48:25Z

/wait

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2021-04-09T22:05:57Z

/wait

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2021-04-10T00:32:44Z

/wait

source/common/http/conn_manager_impl.cc

rgs1 · 2021-04-11T01:47:09Z

Need to an integration test.

/wait

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

jmarantz

looks good; just a few nits about auto

source/common/http/conn_manager_impl.cc

test/common/http/conn_manager_impl_test_2.cc

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

jmarantz

@envoyproxy/senior-maintainers

rgs1 · 2021-04-13T15:47:07Z

Btw, I think we could merge this as is and I'll follow-up with an integration test to avoid regressions. Thanks!

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

alyssawilk

Looking good! One functional nit I'm sorry I missed in the first pass, but nearly there regardless :-)

source/common/router/upstream_request.h

source/common/router/upstream_request.cc

test/integration/protocol_integration_test.cc

test/integration/server_stats.h

test/extensions/upstreams/http/tcp/upstream_request_test.cc

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2021-06-10T18:37:09Z

/wait

This went through because I was using the same payload size for req/resp bodies - changed that to avoid the same issue in the future. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2021-06-10T18:55:17Z

/wait

We can circle back in the future if there's merit in excluding these values. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

alyssawilk · 2021-06-10T20:18:13Z

@jmarantz any further comments?

alyssawilk · 2021-06-10T20:18:36Z

oh actually you handed over to me, I'll assume it's all good :-)

All changes addessed

antoniovicente · 2021-06-11T20:44:54Z

test/test_common/utility.cc

+  while (true) {
+    auto histo = findByName<Stats::ParentHistogramSharedPtr>(store.histograms(), name);
+    if (histo) {
+      if (histo->cumulativeStatistics().sampleCount()) {


this call is not thread safe and is triggering TSAN errors, please fix ASAP.

https://dev.azure.com/cncf/4684fb3d-0389-4e0b-8251-221942316e06/_apis/build/builds/78702/logs/189

Hmm, how come TSAN didn't complain with the original diff? I'll send a fix in a bit

You sure it's me, seeing quic integration failing TSAN too:

https://dev.azure.com/cncf/envoy/_build/results?buildId=78720&view=logs&jobId=d1f76054-8f79-554b-6f4a-11d6a63b8e00&j=d1f76054-8f79-554b-6f4a-11d6a63b8e00&t=266e17e3-d213-54b5-deef-0dcee01da137

Well, the integration test I touched did become flaky so maybe it is me:

//test/integration:protocol_integration_test FLAKY, failed in 1 out of 6 in 227.4s Stats over 6 runs: max = 227.4s, min = 214.3s, avg = 219.7s, dev = 5.2s /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy/bazel-out/k8-dbg/testlogs/test/integration/protocol_integration_test/shard_5_of_5/test_attempts/attempt_1.log //test/integration:quic_http_integration_test FAILED in 4 out of 8 in 37.9s Stats over 8 runs: max = 37.9s, min = 33.8s, avg = 35.5s, dev = 1.3s

Looking into it...

the quic TSAN failure is here: https://gist.github.com/rgs1/1d352d45a78779b14e68cde4ac39e254

Ok now I see mine:

https://gist.github.com/rgs1/bbd811c453484d8887a0448cc8c3d99c

This race is acceptable, so I think we can tell TSAN to ignore it.

The TSAN failure involves waitUntilHistogramHasSamples which was introduced in this PR. That's why I suspect it is related to this.

2021-06-11T18:24:15.8290833Z #0 Envoy::Stats::HistogramStatisticsImpl::sampleCount() const /proc/self/cwd/./source/common/stats/histogram_impl.h:62:50 (protocol_integration_test+0x7c8b879)
2021-06-11T18:24:15.8292040Z #1 Envoy::TestUtility::waitUntilHistogramHasSamples(Envoy::Stats::Store&, std::__1::basic_string<char, std::__1::char_traits, std::__1::allocator > const&, Envoy::Event::TestTimeSystem&, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l> >) /proc/self/cwd/test/test_common/utility.cc:233:41 (protocol_integration_test+0x794eef8)

Possible fix: #16946

This was introduced by envoyproxy#15908. We need to read a histogram's from the main thread to avoid data races between writes (from the main thread) and reads (from a worker thread). Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

This was introduced by #15908. We need to read a histogram's from the main thread to avoid data races between writes (from the main thread) and reads (from a worker thread). Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Perform the accounting for these stats from UpstreamRequest, where we get access to the correct numbers. Things fixed: values are recorded now (they weren't previously) values will be exactly what's sent/received from upstream since they are recorded in the router after/before other filters the destination cluster will be the right one reqs/resps with trailers are handled (they weren't previously) added an integration test to prevent regressions Fixes envoyproxy#15879. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

…xy#16946) This was introduced by envoyproxy#15908. We need to read a histogram's from the main thread to avoid data races between writes (from the main thread) and reads (from a worker thread). Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

cluster: fix req/resp body/hdrs sizes tracking

6c82da4

Using the upstream_host to find the upstream cluster doesn't work from ActiveStream, because it may not be available anymore by the time we look it up. So use ClusterInfo instead. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 changed the title ~~cluster: fix req/resp body/hdrs sizes tracking~~ stats: fix req/resp body/hdrs sizes tracking Apr 9, 2021

repokitteh-read-only bot added the waiting label Apr 9, 2021

Wait until route has been cached to lookup cluster info

1b06299

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only bot removed the waiting label Apr 9, 2021

repokitteh-read-only bot added the waiting label Apr 9, 2021

Fix tests

dfbd515

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only bot removed the waiting label Apr 10, 2021

DRY up the code a bit

defbc71

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only bot added the waiting label Apr 10, 2021

Merge remote-tracking branch 'upstream/main' into issue-15879

c4e301a

repokitteh-read-only bot removed the waiting label Apr 10, 2021

rgs1 commented Apr 11, 2021

View reviewed changes

source/common/http/conn_manager_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Apr 11, 2021

htuch assigned jmarantz Apr 11, 2021

Raul Gutierrez Segales added 2 commits April 12, 2021 18:54

Merge remote-tracking branch 'upstream/main' into issue-15879

f62ae63

Add comment noting how rq headers size calculation happens

de9d812

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only bot removed the waiting label Apr 12, 2021

jmarantz reviewed Apr 12, 2021

View reviewed changes

auto type deduction fixes

4b4216e

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

jmarantz previously approved these changes Apr 13, 2021

View reviewed changes

Raul Gutierrez Segales added 2 commits April 13, 2021 13:47

Merge remote-tracking branch 'upstream/main' into issue-15879

5dc6418

Add integration test

00b31d1

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 dismissed jmarantz’s stale review via 00b31d1 April 13, 2021 23:03

alyssawilk reviewed Jun 10, 2021

View reviewed changes

alyssawilk added the waiting label Jun 10, 2021

Merge remote-tracking branch 'upstream/main' into issue-15879

d9eb1ee

zuercher mentioned this pull request Jun 10, 2021

Metrics are not emitted for track_cluster_stats.request_response_sizes #16906

Closed

Raul Gutierrez Segales added 2 commits June 10, 2021 14:09

Enable & checks histograms for Retry integration test

30de437

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Merge remote-tracking branch 'upstream/main' into issue-15879

c294b37

repokitteh-read-only bot removed the waiting label Jun 10, 2021

repokitteh-read-only bot added the waiting label Jun 10, 2021

Fix embarrassing bug 😱

4c3e1b2

This went through because I was using the same payload size for req/resp bodies - changed that to avoid the same issue in the future. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only bot removed the waiting label Jun 10, 2021

repokitteh-read-only bot added the waiting label Jun 10, 2021

Add empty bodies and bodies of size 0 back

251d22e

We can circle back in the future if there's merit in excluding these values. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only bot removed the waiting label Jun 10, 2021

alyssawilk approved these changes Jun 10, 2021

View reviewed changes

jmarantz approved these changes Jun 11, 2021

View reviewed changes

jmarantz merged commit f3f8df1 into envoyproxy:main Jun 11, 2021

antoniovicente reviewed Jun 11, 2021

View reviewed changes

rgs1 mentioned this pull request Jun 11, 2021

TSAN: fix data race when reading a histogram's sample count #16946

Merged

rgs1 mentioned this pull request Dec 22, 2021

Request/response header bytes calculation content #19293

Closed

Conversation

rgs1 commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgs1 commented Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgs1 commented Apr 9, 2021

Uh oh!

rgs1 commented Apr 9, 2021

Uh oh!

rgs1 commented Apr 10, 2021

Uh oh!

Uh oh!

rgs1 commented Apr 11, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

rgs1 commented Apr 13, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgs1 commented Jun 10, 2021

Uh oh!

rgs1 commented Jun 10, 2021

Uh oh!

alyssawilk commented Jun 10, 2021

Uh oh!

alyssawilk commented Jun 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rgs1 commented Apr 9, 2021 •

edited

Loading

rgs1 commented Apr 9, 2021 •

edited

Loading