upstream: implement UpstreamLocalityStats.loadMetricStats #18534

pianiststickman · 2021-10-08T20:04:07Z

Implements a simple API allowing extensions to populate UpstreamLocalityStats.loadMetricStats in load reports, stored as a flat hash map of keys and values that may vary at runtime. Individual stats are accumulated by calling add(), which combines stats with the same name; the aggregated stats are retrieved by calling latch(), which also clears the current load metrics. The load metrics are latched and read by the load stats reporter.

Signed-off-by: Eugene Chan eugenechan@google.com

Commit Message: upstream: implement UpstreamLocalityStats.loadMetricStats
Additional Description: Implements a simple API allowing extensions to populate UpstreamLocalityStats.loadMetricStats in load reports, stored as a flat hash map of keys and values that may vary at runtime. Individual stats are accumulated by calling add(), which combines stats with the same name; the aggregated stats are retrieved by calling latch(), which also clears the current load metrics. The load metrics are latched and read by the load stats reporter.

Note: this implementation puts a copy of the stat name into every host that receives a copy of that metric. This can be improved by putting a single copy of the stat name into a thread-local key->index map and using the index as the key to the stat map instead.
Risk Level: low
Testing: CI
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

UpstreamLocalityStats.loadMetricStats in load reports Signed-off-by: Eugene Chan <eugenechan@google.com>

pianiststickman · 2021-10-08T20:04:28Z

cc @htuch @jmarantz

jmarantz

I think I'm missing some context as I was not aware of the load-stats-reporter infrastructure.

Where do these stats get consumed?

envoy/upstream/host_description.h

source/common/upstream/load_stats_reporter.cc

envoy/upstream/host_description.h

source/common/upstream/load_stats_reporter.cc

Signed-off-by: Eugene Chan <eugenechan@google.com>

pianiststickman · 2021-10-11T17:29:20Z

I think I'm missing some context as I was not aware of the load-stats-reporter infrastructure.

Where do these stats get consumed?

These stats are sent as part of LoadStatsRequest to the management server. (See https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/endpoint/v3/load_report.proto#envoy-v3-api-msg-config-endpoint-v3-upstreamlocalitystats)

pianiststickman · 2021-10-18T14:45:24Z

/retest

repokitteh-read-only · 2021-10-18T14:45:28Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18534 (comment) was created by @pianiststickman.

see: more, trace.

snowp · 2021-10-27T12:56:33Z

@jmarantz @htuch ping on this since it seems to have stalled. Who owns LRS nowadays?

htuch · 2021-10-29T04:44:58Z

I think maybe @adisuissa can take this one, since LRS is a pretty important part of control plane.

adisuissa

Thanks for working on this.
Left a few comments with some questions.

envoy/upstream/host_description.h

source/common/upstream/load_stats_reporter.cc

envoy/upstream/host_description.h

Signed-off-by: Eugene Chan <eugenechan@google.com>

pianiststickman · 2021-11-05T15:04:12Z

Thanks, PTAL

/retest

repokitteh-read-only · 2021-11-05T15:04:16Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18534 (comment) was created by @pianiststickman.

see: more, trace.

adisuissa

Code changes LGTM modulo the design comment.

yanavlasov · 2021-11-10T15:07:27Z

@pianiststickman can you clarify comments please so we can submit it?

yanavlasov · 2021-11-10T15:07:34Z

/wait

Signed-off-by: Eugene Chan <eugenechan@google.com>

jmarantz

So this PR has no effect on Envoy other than to make the HostDescription object 8 bytes bigger.

Can you put a pointer to a doc explaining how this can be used, and its scaling limitations, into C++ comments and the PR description?

Thank you!

source/common/upstream/upstream_impl.h

envoy/upstream/host_description.h

rojkov · 2021-11-16T08:18:54Z

/wait

jmarantz · 2021-11-16T13:36:50Z

/wait

github-actions · 2021-12-30T20:01:21Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Eugene Chan <eugenechan@google.com>

pianiststickman · 2022-01-04T22:58:39Z

PTAL, and apologies for the delay on this. I've added comments explaining the performance considerations of the current implementation. I think since this is purely an implementation detail (putting key names in thread-local storage, eliminating contention on the locks used to hold the stat maps) I'd prefer to take a pragmatic approach and dispense with that when it becomes necessary. Let me know what you think.

jmarantz

if you merge main, maybe the coverage test will pass. Otherwise you might have to dive into the CI report and see what's wrong. Sometimes coverage fails because we have added lines of code that are not covered with the new tests, sometimes it's just a flake or a consequence of non-hermetic tests, since the coverage tests run all in one process.

source/common/upstream/upstream_impl.h

envoy/upstream/host_description.h

source/common/upstream/upstream_impl.h

jmarantz · 2022-01-05T04:13:01Z

source/common/upstream/upstream_impl.h

+ * Implementation of LoadMetricStats.
+ *
+ * TODO(pianiststickman): this implementation puts a copy of the stat name into every host that
+ * receives a copy of that metric. This can be improved by putting a single copy of the stat name


Also note here that it takes a lock on the hot-path.

I really think this should not be deployed as is, but I understand why you want to add the interface and iterate on the perf.

envoy/upstream/host_description.h

test/common/upstream/load_stats_reporter_test.cc

jmarantz · 2022-01-05T15:04:57Z

/wait

Signed-off-by: Eugene Chan <eugenechan@google.com>

pianiststickman · 2022-01-11T16:51:39Z

/retest

repokitteh-read-only · 2022-01-11T16:51:43Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18534 (comment) was created by @pianiststickman.

see: more, trace.

…metric-stats

jmarantz

/assign-from envoyproxy/senior-maintainers

jmarantz · 2022-01-12T18:28:32Z

test/common/upstream/load_stats_reporter_test.cc

+  ON_CALL(cm_, clusters()).WillByDefault(Return(cluster_info));
+  deliverLoadStatsResponse({"foo"});
+  // First stats report on timer tick.
+  time_system_.setMonotonicTime(std::chrono::microseconds(4));


consider using advanceTimeWait with a delta, rather than just setting the time to an absolute time.

repokitteh-read-only · 2022-01-12T18:30:31Z

envoyproxy/senior-maintainers assignee is @htuch

🐱

Caused by: a #18534 (review) was submitted by @jmarantz.

see: more, trace.

jmarantz · 2022-01-12T18:52:53Z

source/common/upstream/upstream_impl.cc

@@ -218,6 +218,27 @@ HostVector filterHosts(const absl::node_hash_set<HostSharedPtr>& hosts,

 } // namespace

+// TODO(pianiststickman): this implementation takes a lock on the hot path and puts a copy of the


I just want to call out this warning so the senior-maintainer sees it.

We have more detailed notes on how to avoid taking a lock on every request for this use-case, but @pianiststickman wants to merge this before building out the thread-local impl to establish the API even with a potential perf bottleneck and then iterate.

Note that this code can't be reached in production without adding new C++ APIs, so it will not impact existing Envoy deployments.

htuch · 2022-01-13T02:59:00Z

We need another senior maintainer, since both Josh and PR author are Googlers :)

htuch · 2022-01-13T02:59:13Z

/assign-from envoyproxy/senior-maintainers

repokitteh-read-only · 2022-01-13T02:59:16Z

envoyproxy/senior-maintainers assignee is @htuch

🐱

Caused by: a #18534 (comment) was created by @htuch.

see: more, trace.

htuch · 2022-01-13T02:59:33Z

/assign-from envoyproxy/senior-maintainers

repokitteh-read-only · 2022-01-13T02:59:36Z

envoyproxy/senior-maintainers assignee is @zuercher

🐱

Caused by: a #18534 (comment) was created by @htuch.

see: more, trace.

zuercher

Looks fine to me given the proviso that the performance issue addressed before this becomes available for general use.

…#18534) Implements a simple API allowing extensions to populate UpstreamLocalityStats.loadMetricStats in load reports, stored as a flat hash map of keys and values that may vary at runtime. Individual stats are accumulated by calling add(), which combines stats with the same name; the aggregated stats are retrieved by calling latch(), which also clears the current load metrics. The load metrics are latched and read by the load stats reporter. Note: this implementation puts a copy of the stat name into every host that receives a copy of that metric. This can be improved by putting a single copy of the stat name into a thread-local key->index map and using the index as the key to the stat map instead. Risk Level: low Testing: CI Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Eugene Chan <eugenechan@google.com> Signed-off-by: Josh Perry <josh.perry@mx.com>

Implement simple API to allow extensions to populate

63e980d

UpstreamLocalityStats.loadMetricStats in load reports Signed-off-by: Eugene Chan <eugenechan@google.com>

jmarantz reviewed Oct 8, 2021

View reviewed changes

jmarantz assigned pradeepcrao Oct 8, 2021

Review feedback

38a58a5

Signed-off-by: Eugene Chan <eugenechan@google.com>

htuch assigned adisuissa Oct 29, 2021

adisuissa reviewed Nov 2, 2021

View reviewed changes

pianiststickman added 2 commits November 4, 2021 17:15

Review feedback

502eaee

Signed-off-by: Eugene Chan <eugenechan@google.com>

clang-tidy fixes

c8fbd1d

Signed-off-by: Eugene Chan <eugenechan@google.com>

adisuissa reviewed Nov 5, 2021

View reviewed changes

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

yanavlasov assigned jmarantz Nov 10, 2021

Expand on comment to add()

aa41c3c

Signed-off-by: Eugene Chan <eugenechan@google.com>

repokitteh-read-only bot removed the waiting label Nov 11, 2021

jmarantz reviewed Nov 12, 2021

View reviewed changes

source/common/upstream/upstream_impl.h Outdated Show resolved Hide resolved

source/common/upstream/upstream_impl.h Outdated Show resolved Hide resolved

source/common/upstream/upstream_impl.h Outdated Show resolved Hide resolved

jmarantz reviewed Nov 12, 2021

View reviewed changes

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

repokitteh-read-only bot added the waiting label Nov 16, 2021

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 30, 2021

Implementation to cc file, add performance TODO, review feedback

3b76fed

Signed-off-by: Eugene Chan <eugenechan@google.com>

repokitteh-read-only bot removed the waiting label Jan 4, 2022

github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 5, 2022

jmarantz reviewed Jan 5, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 5, 2022

Review feedback

bee1428

Signed-off-by: Eugene Chan <eugenechan@google.com>

repokitteh-read-only bot removed the waiting label Jan 10, 2022

Merge branch 'main' of https://github.com/envoyproxy/envoy into load-…

40c0ed9

…metric-stats

jmarantz approved these changes Jan 12, 2022

View reviewed changes

repokitteh-read-only bot assigned htuch Jan 12, 2022

jmarantz reviewed Jan 12, 2022

View reviewed changes

htuch removed their assignment Jan 13, 2022

repokitteh-read-only bot assigned zuercher Jan 13, 2022

zuercher approved these changes Jan 13, 2022

View reviewed changes

htuch merged commit fe9dc99 into envoyproxy:main Jan 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upstream: implement UpstreamLocalityStats.loadMetricStats #18534

upstream: implement UpstreamLocalityStats.loadMetricStats #18534

pianiststickman commented Oct 8, 2021 •

edited

Loading

pianiststickman commented Oct 8, 2021

jmarantz left a comment

pianiststickman commented Oct 11, 2021

pianiststickman commented Oct 18, 2021

repokitteh-read-only bot commented Oct 18, 2021

snowp commented Oct 27, 2021

htuch commented Oct 29, 2021

adisuissa left a comment

pianiststickman commented Nov 5, 2021

repokitteh-read-only bot commented Nov 5, 2021

adisuissa left a comment

yanavlasov commented Nov 10, 2021

yanavlasov commented Nov 10, 2021

jmarantz left a comment

rojkov commented Nov 16, 2021

jmarantz commented Nov 16, 2021

github-actions bot commented Dec 30, 2021

pianiststickman commented Jan 4, 2022 •

edited

Loading

jmarantz left a comment

jmarantz Jan 5, 2022 •

edited

Loading

pianiststickman Jan 11, 2022

jmarantz commented Jan 5, 2022

pianiststickman commented Jan 11, 2022

repokitteh-read-only bot commented Jan 11, 2022

jmarantz left a comment

jmarantz Jan 12, 2022

repokitteh-read-only bot commented Jan 12, 2022

jmarantz Jan 12, 2022

htuch commented Jan 13, 2022

htuch commented Jan 13, 2022

repokitteh-read-only bot commented Jan 13, 2022

htuch commented Jan 13, 2022

repokitteh-read-only bot commented Jan 13, 2022

zuercher left a comment

		@@ -218,6 +218,27 @@ HostVector filterHosts(const absl::node_hash_set<HostSharedPtr>& hosts,

		} // namespace

		// TODO(pianiststickman): this implementation takes a lock on the hot path and puts a copy of the

upstream: implement UpstreamLocalityStats.loadMetricStats #18534

upstream: implement UpstreamLocalityStats.loadMetricStats #18534

Conversation

pianiststickman commented Oct 8, 2021 • edited Loading

pianiststickman commented Oct 8, 2021

jmarantz left a comment

Choose a reason for hiding this comment

pianiststickman commented Oct 11, 2021

pianiststickman commented Oct 18, 2021

repokitteh-read-only bot commented Oct 18, 2021

snowp commented Oct 27, 2021

htuch commented Oct 29, 2021

adisuissa left a comment

Choose a reason for hiding this comment

pianiststickman commented Nov 5, 2021

repokitteh-read-only bot commented Nov 5, 2021

adisuissa left a comment

Choose a reason for hiding this comment

yanavlasov commented Nov 10, 2021

yanavlasov commented Nov 10, 2021

jmarantz left a comment

Choose a reason for hiding this comment

rojkov commented Nov 16, 2021

jmarantz commented Nov 16, 2021

github-actions bot commented Dec 30, 2021

pianiststickman commented Jan 4, 2022 • edited Loading

jmarantz left a comment

Choose a reason for hiding this comment

jmarantz Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

pianiststickman Jan 11, 2022

Choose a reason for hiding this comment

jmarantz commented Jan 5, 2022

pianiststickman commented Jan 11, 2022

repokitteh-read-only bot commented Jan 11, 2022

jmarantz left a comment

Choose a reason for hiding this comment

jmarantz Jan 12, 2022

Choose a reason for hiding this comment

repokitteh-read-only bot commented Jan 12, 2022

jmarantz Jan 12, 2022

Choose a reason for hiding this comment

htuch commented Jan 13, 2022

htuch commented Jan 13, 2022

repokitteh-read-only bot commented Jan 13, 2022

htuch commented Jan 13, 2022

repokitteh-read-only bot commented Jan 13, 2022

zuercher left a comment

Choose a reason for hiding this comment

pianiststickman commented Oct 8, 2021 •

edited

Loading

pianiststickman commented Jan 4, 2022 •

edited

Loading

jmarantz Jan 5, 2022 •

edited

Loading