Prom stats perf improvements by rulex123 · Pull Request #24998 · envoyproxy/envoy

rulex123 · 2023-01-18T17:30:16Z

Commit Message: patch to add support for streaming/chunking of prometheus stats - basically adapts the algorithm introduced in #19693 for prometheus stats (while keeping common parts of the algorithm in a shared class to avoid code duplication).
With this patch we should be able to improve the issue described here #16139

Risk Level: only underlying implementation changes, but otherwise no change of behaviour/features. So low risk I would say.
Testing: current tests should remain unchanged (though they might be moved around due to refactoring of code); new tests are also added.

Docs Changes: n/a
Release Notes: n/a
[Optional Runtime guard:]
[Optional Fixes #Issue]: #16139

Draft PR (already went through some review) -> #24716

Benchmark data from running stats_handler_speed_test.cc on a dev box:

BEFORE

---------------------------------------------------------------------------
Benchmark                                 Time             CPU   Iterations
---------------------------------------------------------------------------
BM_AllCountersText                      744 ms          744 ms            1
BM_UsedCountersText                    76.5 ms         76.4 ms            9
BM_FilteredCountersText                2866 ms         2866 ms            1
BM_Re2FilteredCountersText              371 ms          371 ms            2
BM_AllCountersJson                      917 ms          917 ms            1
BM_UsedCountersJson                    79.0 ms         79.0 ms            9
BM_FilteredCountersJson                3087 ms         3086 ms            1
BM_Re2FilteredCountersJson              369 ms          369 ms            2
BM_AllCountersPrometheus               8125 ms         8124 ms            1
BM_UsedCountersPrometheus               334 ms          334 ms            2
BM_FilteredCountersPrometheus          3680 ms         3680 ms            1
BM_Re2FilteredCountersPrometheus       1022 ms         1022 ms            1

AFTER

---------------------------------------------------------------------------
Benchmark                                 Time             CPU   Iterations
---------------------------------------------------------------------------
BM_AllCountersText                      742 ms          742 ms            1
BM_UsedCountersText                    77.6 ms         77.6 ms            9
BM_FilteredCountersText                2862 ms         2861 ms            1
BM_Re2FilteredCountersText              509 ms          508 ms            2
BM_AllCountersJson                     1559 ms         1558 ms            1
BM_UsedCountersJson                    81.0 ms         81.0 ms            9
BM_FilteredCountersJson                2857 ms         2856 ms            1
BM_Re2FilteredCountersJson              373 ms          373 ms            2
BM_AllCountersPrometheus               1842 ms         1842 ms            1
BM_UsedCountersPrometheus              92.4 ms         92.4 ms            8
BM_FilteredCountersPrometheus          2863 ms         2862 ms            1
BM_Re2FilteredCountersPrometheus        372 ms          372 ms            2

Signed-off-by: rulex123 <erica.manno@gmail.com>

adisuissa · 2023-04-24T18:50:45Z

@ggreenway can you PTAL?

ggreenway

This is looking really good. Thanks for making the test changes more minimal; that made reviewing much easier.

/wait

ggreenway · 2023-04-27T15:46:04Z

source/server/admin/stats_render.cc

+                                               const std::string& prefixed_tag_extracted_name) {
+  auto parent_histogram = dynamic_cast<Stats::ParentHistogram*>(metric.get());
+  if (parent_histogram != nullptr) {
+    // TODO(rulex123): support for the 3 histogram "bucket modes" offered in other


I don't think this should be a TODO. There is only one valid way to represent histograms in the prometheus exposition format, and this is it. I think you can just delete the TODO comment.

This makes sense.

FYI in #26472 I have added a new bucket mode tentatively called 'detailed' which will have the raw bucket data. That may also be useful for Prom as you can see a lot more interesting data with this IMO, but it might complicate aggregation in Prom across multiple Envoys.

Don't worry Greg -- that PR is still a ways away from being atomized into probably around 4 for review, but I think the first PR will be the addition of a new bucket-mode to show the original buckets computed by circlhist.

Nevertheless I agree removing this TODO is fine for now.

Signed-off-by: rulex123 <erica.manno@gmail.com>

rulex123 · 2023-04-29T08:57:55Z

/retest

repokitteh-read-only · 2023-04-29T08:58:02Z

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

🐱

Caused by: a #24998 (comment) was created by @rulex123.

see: more, trace.

This reverts commit 1cf0187.

Commit Message: Reverts #24998 See #27173 Additional Description: Risk Level: n/a Testing: n/a Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a

Commit Message: Reproduces a scenario where it's difficult to use a streaming optimization for Prometheus stats based on scopes without introducing a bug. Context: Issue that /stats/prometheus endpoint takes too much much memory: Prometheus stats handler used too much memory. #16139 Solution for non-Prometheus endpoints to use less memory for /stats and run faster: admin: Streaming /stats implementation #19693 which I believe is working well. This solution mapped to Prometheus: Prom stats perf improvements #24998 A case where this solution has a duplicate# TYPE line due to two stats with the same tag-extracted-stat name from two different scopes: stats: prometheus scrape failed #27173 Note that the existing unit tests did not exercise that scenario so this PR adds a testcase. Signed-off-by: Joshua Marantz <jmarantz@google.com>

(i.e. doesn't cater for stats with identical tag extracted name but from different scopes). Signed-off-by: rulex123 <erica.manno@gmail.com>

Add support for streaming/chunking of prometheus stats - basically adapts the algorithm introduced in envoyproxy#19693 for prometheus stats (while keeping common parts of the algorithm in a shared class to avoid code duplication). With this patch we should be able to improve the issue described here envoyproxy#16139 Signed-off-by: rulex123 <erica.manno@gmail.com> Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>

Commit Message: Reverts envoyproxy#24998 See envoyproxy#27173 Additional Description: Risk Level: n/a Testing: n/a Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>

…xy#27239) Commit Message: Reproduces a scenario where it's difficult to use a streaming optimization for Prometheus stats based on scopes without introducing a bug. Context: Issue that /stats/prometheus endpoint takes too much much memory: Prometheus stats handler used too much memory. envoyproxy#16139 Solution for non-Prometheus endpoints to use less memory for /stats and run faster: admin: Streaming /stats implementation envoyproxy#19693 which I believe is working well. This solution mapped to Prometheus: Prom stats perf improvements envoyproxy#24998 A case where this solution has a duplicate# TYPE line due to two stats with the same tag-extracted-stat name from two different scopes: stats: prometheus scrape failed envoyproxy#27173 Note that the existing unit tests did not exercise that scenario so this PR adds a testcase. Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>

rulex123 and others added 30 commits January 18, 2023 17:38

initial implementation os PrometheusStatsRequest

0468fcb

Signed-off-by: rulex123 <erica.manno@gmail.com>

refactor PrometheusStatsRequest to use base class

1bd70a5

Signed-off-by: rulex123 <erica.manno@gmail.com>

fix some compilation errors

22bf37d

Signed-off-by: rulex123 <erica.manno@gmail.com>

update comment

ab27878

Signed-off-by: rulex123 <erica.manno@gmail.com>

format code

3a534ab

Signed-off-by: rulex123 <erica.manno@gmail.com>

refactor some more

3665d47

Signed-off-by: rulex123 <erica.manno@gmail.com>

revert tests to original expected output

218c60b

Signed-off-by: rulex123 <erica.manno@gmail.com>

final refactor and code format

38e7ce2

Signed-off-by: rulex123 <erica.manno@gmail.com>

skip TextReadouts phase unless explicitly requested

da86a9a

Signed-off-by: rulex123 <erica.manno@gmail.com>

add comments

146ca42

Signed-off-by: rulex123 <erica.manno@gmail.com>

refactor StatsRequest class to extend StatsRequestBase

d442577

Signed-off-by: rulex123 <erica.manno@gmail.com>

cleanup

66249a9

Signed-off-by: rulex123 <erica.manno@gmail.com>

fix bug

e3071e0

Signed-off-by: rulex123 <erica.manno@gmail.com>

tests for *StatsRequest classes

d359211

Signed-off-by: rulex123 <erica.manno@gmail.com>

address initial feedback on draft PR

d527822

Signed-off-by: rulex123 <erica.manno@gmail.com>

cleanup old prometheus stats code

062fdb3

Signed-off-by: rulex123 <erica.manno@gmail.com>

update admin.cc

beb8e84

Signed-off-by: rulex123 <erica.manno@gmail.com>

update speed test

1c16d7b

Signed-off-by: rulex123 <erica.manno@gmail.com>

update stats_handler_test.cc

f12b2b9

Signed-off-by: rulex123 <erica.manno@gmail.com>

set params.format_ to Prometheus

82a92df

Signed-off-by: rulex123 <erica.manno@gmail.com>

port and adapt PrometheusStatsFormatterTest

67a98b5

Signed-off-by: rulex123 <erica.manno@gmail.com>

rename file prometheus_stats.(h|cc) -> prometheus_stats_formatter.(h|cc)

de620cf

Signed-off-by: rulex123 <erica.manno@gmail.com>

rename test class

f9a73f8

Signed-off-by: rulex123 <erica.manno@gmail.com>

format code

8570d8d

Signed-off-by: rulex123 <erica.manno@gmail.com>

address more feedback

f9c8209

Signed-off-by: rulex123 <erica.manno@gmail.com>

fix compilation errors in test after renaming of classes

7a6182d

Signed-off-by: rulex123 <erica.manno@gmail.com>

format code

5445633

Signed-off-by: rulex123 <erica.manno@gmail.com>

revert unintended change

db82094

Signed-off-by: rulex123 <erica.manno@gmail.com>

format code

7c906a9

Signed-off-by: rulex123 <erica.manno@gmail.com>

add comment explaining how variant is inserted into map

93c65b0

Signed-off-by: rulex123 <erica.manno@gmail.com>

ggreenway requested changes Apr 27, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 27, 2023

remove TODO

b2da77a

Signed-off-by: rulex123 <erica.manno@gmail.com>

rulex123 dismissed jmarantz’s stale review via b2da77a April 28, 2023 07:30

repokitteh-read-only bot removed the waiting label Apr 28, 2023

jmarantz approved these changes Apr 29, 2023

View reviewed changes

ggreenway approved these changes May 1, 2023

View reviewed changes

ggreenway merged commit 1cf0187 into envoyproxy:main May 1, 2023

kyessenov mentioned this pull request May 4, 2023

fix build istio/proxy#4642

Merged

zirain mentioned this pull request May 4, 2023

stats: prometheus scrape failed #27173

Closed

jmarantz added a commit that referenced this pull request May 6, 2023

Revert "stats: prometheus performance improvements (#24998)"

4dad4ca

This reverts commit 1cf0187.

This was referenced May 6, 2023

stats: Revert "Prom stats perf improvements" #27225

Closed

stats: revert "Prom stats perf improvements" #27226

Merged

This was referenced May 6, 2023

Prometheus stats handler used too much memory. #16139

Open

stats: reproduce redundant TYPE tags problem in Prometheus. #27239

Merged

rulex123 added a commit to rulex123/envoy that referenced this pull request Jun 28, 2023

Code from envoyproxy#24998 unchanged

44d0843

(i.e. doesn't cater for stats with identical tag extracted name but from different scopes). Signed-off-by: rulex123 <erica.manno@gmail.com>

jmarantz mentioned this pull request Nov 28, 2023

admin: limit number of bytes an admin point can emit #31082

Open

This was referenced Jan 25, 2024

admin: making a streaming version of /config_dump #32054

Open

admin: making a streaming version of /clusters #32055

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prom stats perf improvements#24998

Prom stats perf improvements#24998
ggreenway merged 52 commits intoenvoyproxy:mainfrom
rulex123:prom-stats-perf-improvements

rulex123 commented Jan 18, 2023

Uh oh!

adisuissa commented Apr 24, 2023

Uh oh!

ggreenway left a comment

Uh oh!

ggreenway Apr 27, 2023

Uh oh!

jmarantz Apr 27, 2023

Uh oh!

rulex123 commented Apr 29, 2023

Uh oh!

repokitteh-read-only bot commented Apr 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

rulex123 commented Jan 18, 2023

Uh oh!

adisuissa commented Apr 24, 2023

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

jmarantz Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

rulex123 commented Apr 29, 2023

Uh oh!

repokitteh-read-only bot commented Apr 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants