admin: Add support for prometheus summary metrics by andybradshaw · Pull Request #30479 · envoyproxy/envoy

andybradshaw · 2023-10-25T03:16:39Z

Commit Message: Add support for prometheus summary metrics on the admin endpoint
Additional Description: Adds support emitting prometheus "summary" metrics for the internal histogram quantiles by supplying a query parameter. Multiple modes are supported, as in #25812, and can be either histogram, summary, or histogram,summary.
Risk Level: Low, no changes to existing default behavior
Testing: Added unit tests for histogram, summary, and summary+histogram emission
Docs Changes: Added documentation to the admin home page, and to the published admin docs around an optional query parameter.
Release Notes: Added a note in the small_feature section.

Fixes #30471

repokitteh-read-only · 2023-10-25T03:16:44Z

Hi @andybradshaw, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #30479 was opened by andybradshaw.

see: more, trace.

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

ggreenway

/wait

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

jmarantz · 2023-10-26T14:56:03Z

/wait

…ermine summary emission Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

jmarantz · 2023-10-27T17:04:19Z

-    are reported. Supported modes are ``histogram`` and ``summary``, referring to the corresponding prometheus metric
-    types.
+    The ``/stats/prometheus`` endpoint can now emit prometheus ``summary`` metric types by explicitly setting the
+    ``histogram_buckets`` query parameter to ``none``.


I would vote for renaming "none" to "summary" in this PR, leaving 'none' as a synonym for "summary".

jmarantz · 2023-10-27T17:16:39Z

    of the text readout stat changes, which could create an unbounded number of time series.

-  .. http:get:: /stats?format=prometheus&histogram_emit_mode=histogram,summary
+  .. http:get:: /stats?format=prometheus&histogram_buckets=none


For text and json, all 4 histogram_buckets modes are supported, and currently for prom, that is ignored, and in this PR you are adding support for 'summary'.

Right now you don't have 'detailed' or 'cumulative' supported for Prom and I'd consider that tech-debt; we don't have to solve it in this PR but we should leave TODOs. Actually TBH I'm not sure why 'cumulative' is ever useful but someone wanted it in the past and added it.

And we should document exactly what happens if the user specifies a not-yet-supported option like prometheus/detailed or prometheus/cumulative.

WDYT?

The prometheus exposition format defines exactly how histograms are expressed. The only configurable/changeable behavior is the published buckets. No other options should be supported on prometheus histograms. (With the exception of switching to summaries instead of histograms; that is also well-defined in the exposition format)

FWIW, the prometheus format lines up pretty well with cumulative. I believe the reason is so that arbitrary buckets can be dropped to down-sample and save space, without having to change any of the non-dropped bucket values (in the TSDB)

jmarantz · 2023-10-27T17:24:28Z

needs main merge also

/wait

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

ggreenway · 2024-01-17T16:46:29Z

+  for (size_t i = 0; i < supported_quantiles.size(); ++i) {
+    double quantile = supported_quantiles[i];
+    double value = computed_quantiles[i];
+    output.append(fmt::format("{0}{{{1}quantile=\"{2}\"}} {3}\n", prefixed_tag_extracted_name,


I'm not sure what the default formatting for a double is for {3} here. Do you think some specific precision should be specified?

Yeah, that's probably a good idea, although I'm a little confused by this comment about fixed-point not being supported, since it seems pretty well documented in the fmt library? Not sure what a sane default precision should be... first thought was to use .32g like in the bucket output above.

Honestly, not sure about the fmtlib f format vs that comment. I'm pretty sure I wrote the envoy comment. But I don't recall the situation. Probably either that I completely missed f in the docs, or it didn't work in the way we needed (maybe not enough significant digits or something?).

I'd say for now use the existing .32g format, and someone should circle back in a separate change and see if we can switch them all to something better or not.

ggreenway · 2024-01-17T16:48:32Z

+  const std::string tags = PrometheusStatsFormatter::formattedTags(histogram.tags());
+  const std::string hist_tags = histogram.tags().empty() ? EMPTY_STRING : (tags + ",");
+
+  const Stats::HistogramStatistics& stats = histogram.cumulativeStatistics();


Given that a summary can't be aggregated in the same way a histogram can, would it make more sense to use intervalStatistics() here? https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#summary suggests that the last 5-10 minutes of data are preferable; we don't have any way to get that time window though with the collected data, at least not as a rolling window.

Yeah, this is a bit unfortunate, as I would imagine getting a rolling window would be quite a bit more work...

Another approach could be to use tags to emit both the interval and cumulative summaries as the quantileSummary function does?

I'm not sure if prometheus (the server/scraper) would ingest everything properly with two sets of quantiles, only different in tags. I'm not sure what the right thing to do here is. Agreed that getting the rolling window would be a lot more work.

ggreenway · 2024-01-17T16:50:00Z

+    double quantile = supported_quantiles[i];
+    double value = computed_quantiles[i];
+    output.append(fmt::format("{0}{{{1}quantile=\"{2}\"}} {3}\n", prefixed_tag_extracted_name,
+                              hist_tags, quantile, std::isnan(value) ? 0 : value));


The spec says that NaN is an allowed value (https://prometheus.io/docs/instrumenting/exposition_formats/). Should we leave it as-is here instead of translating it to 0?

Updated to emit nan.

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

ggreenway

Overall I think this is looking pretty good. One additional doc improvement.

@jmarantz do you want to take another look also?

ggreenway · 2024-02-13T16:52:27Z

+
+  Optional ``histogram_buckets`` query parameter is used to control how histogram metrics get reported.
+  If unset, histograms get reported as the "histogram" prometheus metric type, but can also be used to
+  emit prometheus "summary" metrics if set to ``summary``.


I think it's worth documenting that each emitted summary is over the interval of the last stats flush interval (and link to stats_flush_interval docs).

jmarantz · 2024-02-21T14:10:40Z

lgtm still; and still needs main merge.

/wait

github-actions · 2024-03-22T16:01:55Z

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: Andy Bradshaw <abradshaw@palantir.com>

request was addressed

jmarantz · 2024-04-01T13:32:06Z

/retest

jmarantz · 2024-04-01T13:37:34Z

@andybradshaw CI is being a little flaky I think. It says 'presubmit' failed but I couldn't find an actual error.

Can you merge main to restart it?

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Commit Message: Add support for prometheus summary metrics on the admin endpoint Additional Description: Adds support emitting prometheus "summary" metrics for the internal histogram quantiles by supplying a query parameter. Multiple modes are supported, as in envoyproxy#25812, and can be either histogram, summary, or histogram,summary. Risk Level: Low, no changes to existing default behavior Testing: Added unit tests for histogram, summary, and summary+histogram emission Docs Changes: Added documentation to the admin home page, and to the published admin docs around an optional query parameter. Release Notes: Added a note in the small_feature section. Fixes envoyproxy#30471 Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

andybradshaw added 3 commits October 24, 2023 23:19

Add support for prometheus summary metrics

4670c05

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Add documentation

00700fe

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Fix changelog alphabetization

664eb0d

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

andybradshaw force-pushed the ab/add-prometheus-summary-metrics branch from 39b4ab3 to 664eb0d Compare October 25, 2023 03:19

alyssawilk assigned zuercher and jmarantz Oct 25, 2023

ggreenway requested changes Oct 25, 2023

View reviewed changes

Comment thread source/server/admin/prometheus_stats.cc Outdated

Comment thread docs/root/operations/admin.rst Outdated

repokitteh-read-only Bot added the waiting label Oct 25, 2023

ggreenway self-assigned this Oct 25, 2023

Remove ability to emit both summaries and histogram in the same request

b2b3074

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

repokitteh-read-only Bot removed the waiting label Oct 25, 2023

jmarantz reviewed Oct 26, 2023

View reviewed changes

Comment thread source/server/admin/admin.cc Outdated

Comment thread source/server/admin/stats_params.h Outdated

repokitteh-read-only Bot added the waiting label Oct 26, 2023

Update prometheus handler to use histogram_buckets query param to det…

9125ac7

…ermine summary emission Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

repokitteh-read-only Bot removed the waiting label Oct 27, 2023

jmarantz reviewed Oct 27, 2023

View reviewed changes

repokitteh-read-only Bot added the waiting label Oct 27, 2023

Update histogram buckets to include a summary type

5aa4128

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

repokitteh-read-only Bot removed the waiting label Nov 2, 2023

andybradshaw added 8 commits November 2, 2023 11:09

Merge branch 'main' into ab/add-prometheus-summary-metrics

9511493

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Update docs and changelog to reflect changes

71f936d

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Remove HistogramBucketsMode::NoBuckets

7b20d49

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Stop checking for query param in prometheus handler

d30ad12

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Fix docs typo

553189b

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Remove unnecessary import

3e24114

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Fix invalid input test

73e1097

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Fix formatting

4a3062e

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

repokitteh-read-only Bot added the waiting label Jan 17, 2024

ggreenway requested changes Jan 17, 2024

View reviewed changes

Merge branch 'main' into ab/add-prometheus-summary-metrics

24b164c

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

andybradshaw dismissed jmarantz’s stale review via 24b164c January 17, 2024 20:03

repokitteh-read-only Bot removed the waiting label Jan 17, 2024

ggreenway added the waiting label Jan 18, 2024

PR feedback

342d356

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

repokitteh-read-only Bot removed the waiting label Feb 5, 2024

Fix summary with no values test

fe26285

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

ravenblackx requested a review from ggreenway February 9, 2024 15:58

ggreenway previously requested changes Feb 13, 2024

View reviewed changes

repokitteh-read-only Bot added the waiting label Feb 21, 2024

github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 22, 2024

Merge branch 'main' into ab/add-prometheus-summary-metrics

c35f67f

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

repokitteh-read-only Bot removed the waiting label Mar 25, 2024

github-actions Bot removed the stale stalebot believes this issue/PR has not been touched recently label Mar 25, 2024

andybradshaw added 4 commits March 25, 2024 13:38

Fix test

8d13c1b

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Fix format

b6c51b5

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Update summary docs

f82e638

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

Merge branch 'main' into ab/add-prometheus-summary-metrics

c4ef3a1

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

jmarantz approved these changes Mar 28, 2024

View reviewed changes

jmarantz enabled auto-merge (squash) March 28, 2024 21:24

Merge branch 'main' into ab/add-prometheus-summary-metrics

657d5b5

Signed-off-by: Andy Bradshaw <abradshaw@palantir.com>

jmarantz merged commit 014a68f into envoyproxy:main Apr 1, 2024

Conversation

andybradshaw commented Oct 25, 2023

Uh oh!

repokitteh-read-only Bot commented Oct 25, 2023

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz commented Oct 26, 2023

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!

Uh oh!

Uh oh!

jmarantz commented Oct 27, 2023

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!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Feb 21, 2024

Uh oh!

github-actions Bot commented Mar 22, 2024

Uh oh!

jmarantz commented Apr 1, 2024

Uh oh!

jmarantz commented Apr 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants