make ClusterInfo::traffic_stats_ a unique_ptr, so that later we can lazy-init it later. by stevenzzzz · Pull Request #24406 · envoyproxy/envoy

stevenzzzz · 2022-12-07T01:32:20Z

Commit Message: make ClusterInfo::traffic_stats_ a unique_ptr, so that later we can lazy-init it later.
Additional Description: Cribbed from #23921, this PR is to split the bulk of "." ==> "->" changes in that PR.

NOTE: in this PR the renaming from ClusterInfoImpl::stats_ --> ClusterInfoImpl::traffic_stats_ is also included, sorry for the extra diffs, but hopefully it's straightforward enough to review.

context issue: #23575

Risk Level: LOW, no behavior change.
Testing: Existing unit test.
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

…azyInit it Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only · 2022-12-07T01:32:47Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #24406 was opened by stevenzzzz.

see: more, trace.

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2022-12-07T01:39:22Z

hi Josh, PTAL.
/assign @jmarantz

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

jmarantz

OK I'm done with this round.

Let's not use multiple inheritance for the tests. And the other broad thing is to make a type nickname for your unique_ptr.

jmarantz · 2022-12-07T16:27:13Z

envoy/upstream/upstream.h


  /**
-   * @return ClusterTrafficStats& all traffic related stats for this cluster.
+   * @return std::unique_ptr<ClusterTrafficStats>&  all traffic related stats for this cluster.


can you make a nickname ClusterTrafficStatsPtr for the unique_ptr?

Since we're not passing the ownership here, just use raw pointer ClusterTrafficStats* instead of returning a reference to unique ptr?

This is a good point, Lizan. However that will not be convenient for the follow-up for this PR.

That's because we want to (once we sort out some semantic issues around stat continuity) return a LazyInit& here.

I had suggested making this change as an intermediate step because it is a large PR but is (arguably) obviously correct.

Then when we have solved stat continuity across xds updates we would be able to pass around the lazy-init reference, and then the first time a stat is incremented, we'll instantiate the block of stats.

I think this might appear more palatable once Xin adds a nickname for this.

Sounds good, if this is an intermediate state just add some comment explaining. A nickname helps as well.

jmarantz · 2022-12-07T16:29:52Z

source/common/conn_pool/conn_pool_base.cc

-  host_->cluster().trafficStats().upstream_rq_total_.inc();
-  host_->cluster().trafficStats().upstream_rq_active_.inc();
+  host_->cluster().trafficStats()->upstream_rq_total_.inc();
+  host_->cluster().trafficStats()->upstream_rq_active_.inc();


capture *host_->cluster().trafficStats() in a temp.

jmarantz · 2022-12-07T16:30:20Z

source/common/conn_pool/conn_pool_base.cc

    : parent_(parent), can_send_early_data_(can_send_early_data) {
-  parent_.host()->cluster().trafficStats().upstream_rq_pending_total_.inc();
-  parent_.host()->cluster().trafficStats().upstream_rq_pending_active_.inc();
+  parent_.host()->cluster().trafficStats()->upstream_rq_pending_total_.inc();


capture *host_->cluster().trafficStats() in a temp.

jmarantz · 2022-12-07T16:30:41Z

source/common/conn_pool/conn_pool_base.cc

-  parent_.host()->cluster().trafficStats().upstream_cx_total_.inc();
-  parent_.host()->cluster().trafficStats().upstream_cx_active_.inc();
+  parent_.host()->cluster().trafficStats()->upstream_cx_total_.inc();
+  parent_.host()->cluster().trafficStats()->upstream_cx_active_.inc();


capture *host_->cluster().trafficStats() in a temp.

jmarantz · 2022-12-07T16:34:44Z

source/extensions/filters/network/common/redis/client_impl.cc

      time_source_(dispatcher.timeSource()), redis_command_stats_(redis_command_stats),
      scope_(scope), is_transaction_client_(is_transaction_client) {
-  host->cluster().trafficStats().upstream_cx_total_.inc();
+  host->cluster().trafficStats()->upstream_cx_total_.inc();


capture *host_->cluster().trafficStats() in a temp.

source/extensions/filters/network/common/redis/client_impl.cc

test/common/conn_pool/conn_pool_base_test.cc

jmarantz · 2022-12-07T16:41:11Z

test/common/stats/thread_local_store_test.cc

-};
-
-class LookupWithStatNameTest : public ThreadLocalStoreNoMocksTestBase {};
+class LookupWithStatNameTest : public ThreadLocalStoreNoMocksTestBase, public testing::Test {};


Instead of multiple inheritance, please make ThreadLocalStoreNoMocksTestBase derive from testing::Test.

this is to be reused later in a BENCHMARK test, the testing class there inherits from not necessarily is a testing::Test.

Use composition rather than multiple inheritance for that pattern.

if not feeling strongly, I'd hope it stay this way. I am not sure how much the composition pattern would help with readability here. OTOH, the original ThreadLocalRealThreadsTestBase is already a multiple inheritance impl.

Actually I agree with you - this is consistent with other test helper classes in Envoy.

One thing that's a little concerning is that usually the needs of performance tests are not consistent from the needs of unit tests, so they may not want to share that much code, but in this case I think it is helpful to do the sharing.

test/common/stats/thread_local_store_test.cc

jmarantz · 2022-12-07T16:43:11Z

/wait

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz

Thanks, PTAL.

test/common/stats/thread_local_store_test.cc

stevenzzzz · 2022-12-07T16:57:11Z

test/common/stats/thread_local_store_test.cc

-};
-
-class LookupWithStatNameTest : public ThreadLocalStoreNoMocksTestBase {};
+class LookupWithStatNameTest : public ThreadLocalStoreNoMocksTestBase, public testing::Test {};


this is to be reused later in a BENCHMARK test, the testing class there inherits from not necessarily is a testing::Test.

test/common/conn_pool/conn_pool_base_test.cc

stevenzzzz · 2022-12-07T17:00:46Z

source/extensions/filters/network/common/redis/client_impl.cc

      time_source_(dispatcher.timeSource()), redis_command_stats_(redis_command_stats),
      scope_(scope), is_transaction_client_(is_transaction_client) {
-  host->cluster().trafficStats().upstream_cx_total_.inc();
+  host->cluster().trafficStats()->upstream_cx_total_.inc();


stevenzzzz · 2022-12-07T17:44:48Z

source/common/conn_pool/conn_pool_base.cc

-  host_->cluster().trafficStats().upstream_rq_total_.inc();
-  host_->cluster().trafficStats().upstream_rq_active_.inc();
+  host_->cluster().trafficStats()->upstream_rq_total_.inc();
+  host_->cluster().trafficStats()->upstream_rq_active_.inc();


stevenzzzz · 2022-12-07T17:53:02Z

envoy/upstream/upstream.h


  /**
-   * @return ClusterTrafficStats& all traffic related stats for this cluster.
+   * @return std::unique_ptr<ClusterTrafficStats>&  all traffic related stats for this cluster.


stevenzzzz · 2022-12-07T17:53:47Z

source/common/conn_pool/conn_pool_base.cc

    : parent_(parent), can_send_early_data_(can_send_early_data) {
-  parent_.host()->cluster().trafficStats().upstream_rq_pending_total_.inc();
-  parent_.host()->cluster().trafficStats().upstream_rq_pending_active_.inc();
+  parent_.host()->cluster().trafficStats()->upstream_rq_pending_total_.inc();


stevenzzzz · 2022-12-07T17:55:21Z

source/common/conn_pool/conn_pool_base.cc

-  parent_.host()->cluster().trafficStats().upstream_cx_total_.inc();
-  parent_.host()->cluster().trafficStats().upstream_cx_active_.inc();
+  parent_.host()->cluster().trafficStats()->upstream_cx_total_.inc();
+  parent_.host()->cluster().trafficStats()->upstream_cx_active_.inc();


jmarantz

/wait

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

envoy/upstream/upstream.h

test/common/conn_pool/conn_pool_base_test.cc

jmarantz · 2022-12-09T03:09:16Z

test/common/stats/thread_local_store_test.cc

-};
-
-class LookupWithStatNameTest : public ThreadLocalStoreNoMocksTestBase {};
+class LookupWithStatNameTest : public ThreadLocalStoreNoMocksTestBase, public testing::Test {};


Actually I agree with you - this is consistent with other test helper classes in Envoy.

One thing that's a little concerning is that usually the needs of performance tests are not consistent from the needs of unit tests, so they may not want to share that much code, but in this case I think it is helpful to do the sharing.

test/extensions/clusters/aggregate/cluster_test.cc

jmarantz · 2022-12-09T03:11:06Z

test/integration/xfcc_integration_test.cc

    }
  };
-
  for (const Stats::CounterSharedPtr& counter : test_server_->counters()) {


revert this file?

test/test_common/real_threads_test_helper.h

jmarantz · 2022-12-09T13:47:36Z

/wait

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

jmarantz · 2022-12-09T22:21:01Z

/wait

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz

Thanks

envoy/upstream/upstream.h

test/extensions/clusters/aggregate/cluster_test.cc

test/test_common/real_threads_test_helper.h

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz · 2022-12-12T16:47:45Z

updated. PTAL

…

On Mon, Dec 12, 2022 at 11:18 AM Joshua Marantz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In envoy/upstream/upstream.h <#24406 (comment)>: > @@ -765,6 +765,7 @@ MAKE_STATS_STRUCT(ClusterLbStats, ClusterLbStatNames, ALL_CLUSTER_LB_STATS); */ MAKE_STAT_NAMES_STRUCT(ClusterTrafficStatNames, ALL_CLUSTER_TRAFFIC_STATS); MAKE_STATS_STRUCT(ClusterTrafficStats, ClusterTrafficStatNames, ALL_CLUSTER_TRAFFIC_STATS); +using ClusterTrafficStatsPtr = std::unique_ptr<ClusterTrafficStats>; I think it'd be better for the CL author not to resolve comments unless it was a simple suggestion that was implemented. Otherwise that causes the reviewer to re-review and re-discover the same issue. I think if we change the nickname to `LazyClusterTrafficStats' then it can be commented right here and not at call sites, and that's what I'm learning toward. — Reply to this email directly, view it on GitHub <#24406 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA2I6TAPYEI3PMA3D444M3LWM5F6PANCNFSM6AAAAAASWGQAXM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Xin Zhuang

jmarantz

lgtm with small nits

envoy/upstream/upstream.h

jmarantz · 2022-12-13T04:25:26Z

test/extensions/filters/udp/udp_proxy/udp_proxy_filter_test.cc

-  EXPECT_EQ(5, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_->stats_
-                   .upstream_cx_tx_bytes_total_.value());
+  EXPECT_EQ(5, factory_context_.cluster_manager_.thread_local_cluster_.cluster_.info_
+                   ->traffic_stats_->upstream_cx_tx_bytes_total_.value());


I'm surprised to see this particular diff; I thought you already partitioned the stat into separate blocks in a previous PR.

the renaming? it's not included in the previous PRs.

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

jmarantz

Thanks -- this looks great!

Augustyniak · 2022-12-14T15:25:40Z

source/common/router/router.cc

    // For example, we also want to record the stats in the case of BiDi streaming APIs where we
    // might have already seen the headers.
-    cluster_->trafficStats().upstream_rq_timeout_.inc();
+    cluster_->trafficStats()->upstream_rq_timeout_.inc();


just want to point out that this whole lazy optimization change has a lot of sense from the perspective of the mobile Envoy (that runs on iOS and Android devices) too.

For the mobile applications a lot of impact could be realized if we added lazy initiation to virtual cluster stats. They are managed in multiple places but one of them is just a line below the line I commented on. Are there any plans to make the initialization of virtual cluster stats lazy too or is this outside of the scope of work/changes that you are work on?

ggreenway

LGTM

…-tools * origin/main: (59 commits) Make IsOkAndHolds matcher work with submatchers (#24498) ios: fix platform key value store (#24532) make ClusterInfo::traffic_stats_ a unique_ptr, so that later we can lazy-init it later. (#24406) quic: splitting into client and server (#24513) fixing coverage (#24529) ci: Add `examplesOnly` condition (#24465) ci: sonatype_nexus_upload.py: remove unused format argument (#24471) deps: Bump `build_bazel_rules_apple` -> 1.1.3 (#24527) deps: Bump `com_github_nghttp2_nghttp2` -> 1.51.0 (#24525) deps: Bump `rules_license` -> 0.0.4 (#24523) build(deps): bump sphinxcontrib-httpdomain from 1.8.0 to 1.8.1 in /mobile/docs (#24126) build(deps): bump github/codeql-action from 2.1.35 to 2.1.36 (#24473) build(deps): bump openpolicyagent/opa from 0.47.2-istio to 0.47.3-istio in /examples/ext_authz (#24514) build(deps): bump node from `80844b6` to `2770c78` in /examples/ext_authz/auth/http-service (#24515) build(deps): bump abseil-cpp to latest version (#24386) xDS: add xDS config tracker extension point (#23485) kafka: add shared consumer manager (#24494) coverage: Improve test coverage (#24355) deps: Bump `rules_python` -> 0.16.1 (#24344) ci: revert disable running the Objective-C integration app (#24478)" (#24496) ... Signed-off-by: JP Simard <jp@jpsim.com>

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>

make ClusterInfo::traffic_stats_ a unique_ptr, so that later we can L…

fba6b7a

…azyInit it Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz requested review from alyssawilk, jmarantz, kyessenov, lizan, mattklein123, snowp and yanavlasov as code owners December 7, 2022 01:32

repokitteh-read-only bot added the api label Dec 7, 2022

repokitteh-read-only bot assigned lizan Dec 7, 2022

revert some unwanted updates

803004f

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only bot assigned jmarantz Dec 7, 2022

stevenzzzz added 3 commits December 7, 2022 11:34

revert more unwanted updates

74059e4

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

heh, vscode is slower than git

fea4a48

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

revert more unwanted updates

b69c02c

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

jmarantz reviewed Dec 7, 2022

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Dec 7, 2022

fixes for Josh comments

9de8e10

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz commented Dec 7, 2022

View reviewed changes

jmarantz reviewed Dec 8, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 8, 2022

re add the helper files

58ba476

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only bot removed the waiting label Dec 9, 2022

jmarantz reviewed Dec 9, 2022

View reviewed changes

test/test_common/real_threads_test_helper.h Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Dec 9, 2022

Merge branch 'main' into ClusterTrafficStats_unique_ptr

58bc7d5

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only bot removed the waiting label Dec 9, 2022

repokitteh-read-only bot added the waiting label Dec 9, 2022

add comments for testing base classes.

150acb0

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

repokitteh-read-only bot removed the waiting label Dec 12, 2022

stevenzzzz commented Dec 12, 2022

View reviewed changes

envoy/upstream/upstream.h Outdated Show resolved Hide resolved

test/extensions/clusters/aggregate/cluster_test.cc Outdated Show resolved Hide resolved

test/test_common/real_threads_test_helper.h Show resolved Hide resolved

update alias

98c167a

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

jmarantz previously approved these changes Dec 13, 2022

View reviewed changes

jmarantz mentioned this pull request Dec 13, 2022

[stats] lazyinit ClusterInfo::trafficStats() #23921

Merged

jmarantz assigned Augustyniak Dec 13, 2022

move comment

d7bb525

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

stevenzzzz dismissed jmarantz’s stale review via d7bb525 December 13, 2022 14:58

stevenzzzz added 3 commits December 13, 2022 15:19

fix comment

a1dfeb3

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

Merge branch 'main' into ClusterTrafficStats_unique_ptr

c543f83

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

revert some unrelated changes that could be included in the next PR

79ce9bf

Signed-off-by: Xin Zhuang <stevenzzz@google.com>

jmarantz approved these changes Dec 13, 2022

View reviewed changes

jmarantz assigned ggreenway Dec 13, 2022

Augustyniak reviewed Dec 14, 2022

View reviewed changes

ggreenway approved these changes Dec 14, 2022

View reviewed changes

ggreenway merged commit 7d9805b into envoyproxy:main Dec 14, 2022

JuniorHsu pushed a commit to JuniorHsu/envoy that referenced this pull request Dec 15, 2022

fix conflict with envoyproxy#24406

81c5c7e

Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>

Conversation

stevenzzzz commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Dec 7, 2022

Uh oh!

stevenzzzz commented Dec 7, 2022

Uh oh!

jmarantz left a comment

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!

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!

Uh oh!

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!

Uh oh!

jmarantz commented Dec 7, 2022

Uh oh!

stevenzzzz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

stevenzzzz commented Dec 7, 2022 •

edited

Loading