stats: add ability to view histogram buckets to the /stats endpoint by VillePihlava · Pull Request #19586 · envoyproxy/envoy

VillePihlava · 2022-01-18T15:01:31Z

Commit Message:

Add ability to view histogram buckets to the /stats endpoint through the histogram_buckets query parameter.

Additional Description:

Using /stats?histogram_buckets=cumulative or /stats?histogram_buckets=disjoint will change the output of histograms to a bucket summary with cumulative or disjoint buckets. Can be used with format=json, usedonly, or filter.

Risk Level: Low
Testing: Unit testing, manual testing
Docs Changes: Added documentation about histogram_buckets query parameter.
Release Notes: Added
Platform Specific Features: N/A
Fixes #19378

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

lizan · 2022-01-19T18:40:34Z

/assign-from @envoyproxy/first-pass-reviewers

repokitteh-read-only · 2022-01-19T18:40:38Z

@envoyproxy/first-pass-reviewers assignee is @mathetake

🐱

Caused by: a #19586 (comment) was created by @lizan.

see: more, trace.

jmarantz · 2022-01-19T18:53:28Z

docs/root/operations/admin.rst

  Full-string matching can be specified with begin- and end-line anchors. (i.e.
  ``/stats?filter=^server.concurrency$``)

+  .. http:get:: /stats?histogram_buckets


I have a draft PR #19546 which will hopefully be ready soon, where I introduce a 'type=' which makes me wonder about exactly how best to control this.

First -- is there any reason not to always be in histogram_buckets mode? Is there any reason you'd want to see the current mode?

The new PR introduces a '&type=' query-param which has possible values (Counters, Gauges, Histograms, TextReadouts, or All). I'm unsure whether this would best fit as a new category, or as a boolean property which only makes sense if you are displaying histograms.

Also, what are your thoughts about how this parameter works with format=json or format=prometheus? Or does this only make sense for text?

First -- is there any reason not to always be in histogram_buckets mode? Is there any reason you'd want to see the current mode?

I don't have enough experience on this to give a good opinion, but I didn't find a use for the current mode.

The new PR introduces a '&type=' query-param which has possible values (Counters, Gauges, Histograms, TextReadouts, or All). I'm unsure whether this would best fit as a new category, or as a boolean property which only makes sense if you are displaying histograms.

I think a boolean property would suit this best.

Also, what are your thoughts about how this parameter works with format=json or format=prometheus? Or does this only make sense for text?

From what I've understood, the current way format=prometheus displays histogram buckets cumulatively is the correct way for prometheus, so it seems like changing this would be unnecessary.

Using format=json outputs the same quantile summary data available from the plain text endpoint for histograms. If the quantile summary is replaced by the histogram_buckets output in the plain text endpoint, I think it should be replaced here too.

So...can you add a test for the JSON mode as well then?

I'll work on a JSON mode next. Also something that came to mind is that should there be a query parameter or an option of seeing the original overlapping (or cumulative) histogram buckets? It should be easy to implement if there is any use for it.

I think it's fine to have a query-param for the new mode. The absence of that query-params means you want the old mode. Am I missing something?

I see -- there are really 3 choices. Can you just have one query-param with 3 options then (default value being 'none'), rather than bools? I think that'll look better in the UI I've got pending -- see image in description of #18670

Yes, one query-param with 2 options sounds much better. I think something like this would work, although there might be a better word for nonoverlapping:
/stats?histogram_buckets=cumulative
/stats?histogram_buckets=nonoverlapping

oh...a better word for nonoverlapping might be "disjoint"

Thank you! I'll start using this

jmarantz · 2022-01-19T18:55:38Z

source/common/stats/thread_local_store.cc

+      previous_computed_interval_bucket = current_computed_interval_bucket;
+      previous_computed_cumulative_bucket = current_computed_cumulative_bucket;
+    }
+    return absl::StrJoin(bucket_summary, " ");


I feel like we should have this API return the array, and have a separate layer that joins it as strings.

For example, if I have a richer HTML display of histograms I might like to be able to render these values graphically.

Moreover, I think it'd be great to return a structured result that's an array of triples, and do the formatting in the stats handler (e.g. json-population vs emitting strings manually)

I'll start looking into this

I added nonoverlappingComputedBuckets() to HistogramStatistics which can be used from the ParentHistogram. It returns an array with the desired values as output. Does this accomplish what you meant? I changed the nonoverlappingBucketSummary() to use the new method. Should I also move the summary to the stats handler? Originally I wrote it here because it is similar to the existing bucketSummary().

This is better but I still think the formatting of this could be removed from the histogram and go into the stats handler class.

E.g. one thing I was thinking (in a follow-up after an in-progress PR) is to have HTML-mode actually render the histograms graphically. So the more we do via structured API, and the less we commit to string representations inside the Histogram class, the more flexible we are about how to present the data in the stats handler.

I moved computeDisjointBucketSummary() to the stats handler.

jmarantz · 2022-01-20T17:09:23Z

/wait

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

jmarantz · 2022-01-25T14:24:03Z

docs/root/operations/admin.rst

  Full-string matching can be specified with begin- and end-line anchors. (i.e.
  ``/stats?filter=^server.concurrency$``)

+  .. http:get:: /stats?histogram_buckets


So...can you add a test for the JSON mode as well then?

jmarantz · 2022-01-25T14:25:52Z

envoy/stats/histogram.h

+   * Returns nonoverlapping version of computedBuckets(). This vector is
+   * guaranteed to be the same length as supportedBuckets().
+   */
+  virtual const std::vector<uint64_t> nonoverlappingComputedBuckets() const PURE;


2 nits :)

https://clang.llvm.org/extra/clang-tidy/checks/readability-avoid-const-params-in-decls.html -- you can just return the vector without making it const in the declaration -- that's a no-op

rename new function to computeNonOverlappingBuckets(). The function is doing the computation and returning the result, not returning an already-existing result, so I think it's better to start the function name with the verb.

sorry that was the wrong clang link; it's this one that's applicable: https://clang.llvm.org/extra/clang-tidy/checks/readability-const-return-type.html

jmarantz · 2022-01-25T14:27:13Z

envoy/stats/histogram.h

+  /**
+   * Returns the bucket summary representation with nonoverlapping buckets.
+   */
+  virtual const std::string nonoverlappingBucketSummary() const PURE;


computeNonOverlappingBucketSummary()

remove the const prefix -- also do this for bucketSummary() which shouldn't have the const prefix either.

Fixed. Removed the const prefix from quantileSummary() too

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

jmarantz

this is looking much better - -still a few comments remain.

Thanks!

docs/root/operations/admin.rst

jmarantz · 2022-01-26T13:44:11Z

source/common/stats/histogram_impl.cc

+  std::vector<uint64_t> buckets;
+  buckets.reserve(computed_buckets_.size());
+  uint64_t previous_computed_bucket = 0;
+  for (size_t i = 0; i < computed_buckets_.size(); ++i) {


nit; for (uint64_t computed_bucket : computed_buckets_)

jmarantz · 2022-01-26T13:45:37Z

source/common/stats/thread_local_store.cc

+        interval_statistics_.computeDisjointBuckets();
+    const std::vector<uint64_t> disjoint_cumulative_buckets =
+        cumulative_statistics_.computeDisjointBuckets();
+    bucket_summary.reserve(supported_buckets.size());


Paranoia nit: ASSERT here that the 3 array lengths are the same, and make the loop go to their min value?

Created ASSERT and looped to min value

jmarantz · 2022-01-26T13:51:39Z

source/common/stats/thread_local_store.cc

+      previous_computed_interval_bucket = current_computed_interval_bucket;
+      previous_computed_cumulative_bucket = current_computed_cumulative_bucket;
+    }
+    return absl::StrJoin(bucket_summary, " ");


This is better but I still think the formatting of this could be removed from the histogram and go into the stats handler class.

E.g. one thing I was thinking (in a follow-up after an in-progress PR) is to have HTML-mode actually render the histograms graphically. So the more we do via structured API, and the less we commit to string representations inside the Histogram class, the more flexible we are about how to present the data in the stats handler.

jmarantz · 2022-01-29T16:39:13Z

/wait

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

…ectors. Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

jmarantz · 2022-02-04T00:57:26Z

@VillePihlava are you going to push this forward any further? I felt like we were converging.

…reate tests, and other small changes. Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

…uckets-stats-endpoint Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

VillePihlava · 2022-02-04T10:53:22Z

@jmarantz Sorry for the delay! Had to work on something else for a while. I'll start looking into the JSON output next.

jmarantz · 2022-02-07T13:51:42Z

/wait

VillePihlava · 2022-02-22T17:16:04Z

@jmarantz ValueUtil::numberValue converts all uint64_t values to doubles. I think this is unnecessary for histogram values, but it was also previously in use with counters and gauges (which according to my understanding are also of type uint64_t).

envoy/source/server/admin/stats_handler.cc

Lines 220 to 226 in d13f9c2

    
           for (const auto& stat : all_stats) { 
        
             ProtobufWkt::Struct stat_obj; 
        
             auto* stat_obj_fields = stat_obj.mutable_fields(); 
        
             (*stat_obj_fields)["name"] = ValueUtil::stringValue(stat.first); 
        
             (*stat_obj_fields)["value"] = ValueUtil::numberValue(stat.second); 
        
             stats_array.push_back(ValueUtil::structValue(stat_obj)); 
        
           }

I'm pretty new to using anything related to protocol buffers, so it would be nice to know if this is ok. I asked this in a previous comment but wasn't sure if it was already answered.

jmarantz · 2022-02-22T17:31:39Z

Yeah I think the whole JSON serialization machinery is really heavyweight and needs a re-think. But this seems orthogonal to your PR assuming you are not changing it. Maybe another follow-up to avoid pointless conversions and protobuf overhead would be warranted?

For this PR my suggestion is to add comments into the code for your observation and maybe open an issue.

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

VillePihlava · 2022-02-22T18:46:28Z

Thanks for the reply! I added some comments and will open up an issue.

VillePihlava · 2022-02-23T06:33:32Z

/retest

repokitteh-read-only · 2022-02-23T06:33:34Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #19586 (comment) was created by @VillePihlava.

see: more, trace.

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

VillePihlava · 2022-02-23T11:11:23Z

/retest

repokitteh-read-only · 2022-02-23T11:11:26Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #19586 (comment) was created by @VillePihlava.

see: more, trace.

VillePihlava · 2022-02-23T12:00:27Z

Found that an issue exists already, so I won't be creating a duplicate: #10411

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

jmarantz · 2022-02-24T15:18:22Z

Needs main merge

jmarantz · 2022-02-24T15:20:37Z

@ggreenway it probably makes sense for you to be the senior maintainer for this one as @snowp is not available atm.

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

VillePihlava · 2022-02-25T13:43:27Z

/retest

repokitteh-read-only · 2022-02-25T13:43:30Z

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

🐱

Caused by: a #19586 (comment) was created by @VillePihlava.

see: more, trace.

rojkov · 2022-03-01T07:51:31Z

@ggreenway gentle ping

ggreenway

This looks great!

/wait

test/server/admin/stats_handler_test.cc

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Add histogram bucket information to /stats endpoint.

96ca9f8

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

VillePihlava mentioned this pull request Jan 18, 2022

cryptomb: add queue size statistics #19180

Merged

repokitteh-read-only bot assigned mathetake Jan 19, 2022

jmarantz self-assigned this Jan 19, 2022

jmarantz reviewed Jan 19, 2022

View reviewed changes

mathetake removed their assignment Jan 19, 2022

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

Add nonoverlappingComputedBuckets() and fix spelling.

cc874ef

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

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

jmarantz reviewed Jan 25, 2022

View reviewed changes

Change naming and remove unnecessary const prefixes.

bdf901f

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

jmarantz reviewed Jan 26, 2022

View reviewed changes

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

VillePihlava added 2 commits January 30, 2022 22:56

Refactor computeDisjointBuckets().

0b90dc3

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Add ASSERT to computeDisjointBucketSummary and loop to min value of v…

cea16a5

…ectors. Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

VillePihlava added 5 commits February 4, 2022 01:49

Change histogram_buckets query parameter to have 2 possible values, c…

1c5b193

…reate tests, and other small changes. Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Change documentation.

d00801c

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Fix format.

c010782

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Merge branch 'main' of github.com:VillePihlava/envoy into histogram-b…

e0d0700

…uckets-stats-endpoint Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Change current.rst to alphabetical order.

3a7e6c9

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

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

Fix comment.

00c963c

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

repokitteh-read-only bot assigned snowp Feb 22, 2022

Add comments about ValueUtil::numberValue.

47ca897

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

VillePihlava dismissed jmarantz’s stale review via 47ca897 February 22, 2022 18:42

Merge branch 'main' into histogram-buckets-stats-endpoint

9d62d48

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Kick CI

b18d0f3

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

jmarantz mentioned this pull request Feb 24, 2022

admin: Streaming /stats implementation #19693

Merged

jmarantz assigned ggreenway and unassigned snowp Feb 24, 2022

VillePihlava added 2 commits February 25, 2022 01:36

Merge branch 'main' into histogram-buckets-stats-endpoint

be1b95b

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Change current.rst.

c4369be

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

ggreenway requested changes Mar 1, 2022

View reviewed changes

test/server/admin/stats_handler_test.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Mar 1, 2022

VillePihlava added 2 commits March 2, 2022 00:05

Change setHistogramBucketSettings test helper function and add comments.

3691e2a

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

Merge branch 'main' into histogram-buckets-stats-endpoint

c296df9

Signed-off-by: Ville Pihlava <ville.pihlava@intel.com>

repokitteh-read-only bot removed the waiting label Mar 2, 2022

ggreenway approved these changes Mar 2, 2022

View reviewed changes

ggreenway merged commit 848cfe7 into envoyproxy:main Mar 2, 2022

Conversation

VillePihlava commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan commented Jan 19, 2022

Uh oh!

repokitteh-read-only bot commented Jan 19, 2022

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 Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jmarantz commented Jan 20, 2022

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!

jmarantz left a comment

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 commented Jan 29, 2022

Uh oh!

jmarantz commented Feb 4, 2022

Uh oh!

VillePihlava commented Feb 4, 2022

Uh oh!

jmarantz commented Feb 7, 2022

Uh oh!

VillePihlava commented Feb 22, 2022

Uh oh!

VillePihlava commented Jan 18, 2022 •

edited

Loading

jmarantz Jan 25, 2022 •

edited

Loading

jmarantz commented Feb 22, 2022 •

edited

Loading