metrics service: allow only histogram summaries by sschepens · Pull Request #25812 · envoyproxy/envoy

sschepens · 2023-02-28T12:56:51Z

Commit Message: Allow only histogram summaries in metric service
Additional Description: Histogram metrics by default send two metrics Summary (containing quantiles) and Histogram (containing all the buckets), this makes payloads extremely large when handling lots of stats. In some cases, only having the summary is enough, since we can get sum, count, and percentiles. Furthermore, it probably only makes sense to send one of both, if I were to need buckets, then Summary is probably useless.
Risk Level: low, off by default
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2023-02-28T12:57:03Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #25812 was opened by sschepens.

see: more, trace.

repokitteh-read-only · 2023-02-28T13:21:48Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #25812 was synchronize by sschepens.

see: more, trace.

jmarantz

should we have some tests for this new mode?

api/envoy/config/metrics/v3/metrics_service.proto

sschepens · 2023-02-28T13:55:27Z

should we have some tests for this new mode?

yes! on it

sschepens · 2023-02-28T23:01:23Z

@jmarantz pushed a new implementation using an enum to allow specifying which metrics you want emitted, also added tests for this.
The only thing i'm not sure is if protobuf defaults to the enum value 0 when not specified or if we need some other sort of default.

api/envoy/config/metrics/v3/metrics_service.proto

mattklein123 · 2023-03-02T16:20:29Z

/retest

repokitteh-read-only · 2023-03-02T16:20:38Z

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

🐱

Caused by: a #25812 (comment) was created by @mattklein123.

see: more, trace.

mattklein123

API LGTM. Please add a release note. I will defer to @jmarantz for code review. Thank you!

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

sschepens · 2023-03-31T14:10:52Z

added changelog and merged main

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

jmarantz

I think you need to merge main again to get rid of the spurious arm64 CI failure, which was a flake I think.

lizan

can you resolve conflicts? defer to @jmarantz to merge.

jmarantz · 2023-04-05T13:43:47Z

@sschepens you might be interested in this related issue: #11259

I have a skeletal draft PR #26472 which exposes raw histogram bins as an admin endpoint via json. It might be interesting to also access those via metrics service API, which would add a new enum choice to what you have here.

jmarantz · 2023-04-05T13:59:07Z

/wait (for main merge)

…ow-only-histogram-summaries Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

jmarantz · 2023-04-06T11:40:38Z

/retest

repokitteh-read-only · 2023-04-06T11:40:44Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #25812 (comment) was created by @jmarantz.

see: more, trace.

I think what was requested was for me to review.

Commit Message: Allow only histogram summaries in metric service Additional Description: Histogram metrics by default send two metrics Summary (containing quantiles) and Histogram (containing all the buckets), this makes payloads extremely large when handling lots of stats. In some cases, only having the summary is enough, since we can get sum, count, and percentiles. Furthermore, it probably only makes sense to send one of both, if I were to need buckets, then Summary is probably useless. Risk Level: low, off by default Testing: Docs Changes: Release Notes: Platform Specific Features: Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com> Signed-off-by: River Phillips <riverphillips1@gmail.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 #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 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>

sschepens requested a review from jmarantz as a code owner February 28, 2023 12:56

repokitteh-read-only bot added v2-freeze api labels Feb 28, 2023

repokitteh-read-only bot assigned markdroth Feb 28, 2023

sschepens force-pushed the metrics-service-allow-only-histogram-summaries branch from 8129c15 to 221dbd7 Compare February 28, 2023 13:21

sschepens requested review from adisuissa, alyssawilk, htuch, kyessenov, lizan, mattklein123 and wbpcode as code owners February 28, 2023 13:21

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Feb 28, 2023

repokitteh-read-only bot assigned mattklein123 Feb 28, 2023

sschepens force-pushed the metrics-service-allow-only-histogram-summaries branch from 221dbd7 to 971c188 Compare February 28, 2023 13:23

jmarantz reviewed Feb 28, 2023

View reviewed changes

api/envoy/config/metrics/v3/metrics_service.proto Outdated Show resolved Hide resolved

sschepens force-pushed the metrics-service-allow-only-histogram-summaries branch from 55600a4 to c123e6e Compare February 28, 2023 22:59

ramaraochavali reviewed Mar 1, 2023

View reviewed changes

api/envoy/config/metrics/v3/metrics_service.proto Outdated Show resolved Hide resolved

sschepens force-pushed the metrics-service-allow-only-histogram-summaries branch from 965a75f to bd75374 Compare March 1, 2023 17:08

mattklein123 previously requested changes Mar 2, 2023

View reviewed changes

mattklein123 assigned jmarantz and unassigned markdroth Mar 2, 2023

mattklein123 added the waiting label Mar 2, 2023

sschepens removed the request for review from lizan March 7, 2023 12:56

add changelog

3180a0f

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

sschepens dismissed jmarantz’s stale review via 3180a0f March 31, 2023 14:04

sschepens requested a review from mattklein123 March 31, 2023 14:10

fix changelog ref

b567304

Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

jmarantz previously approved these changes Apr 5, 2023

View reviewed changes

lizan previously approved these changes Apr 5, 2023

View reviewed changes

repokitteh-read-only bot removed the api label Apr 5, 2023

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

Merge remote-tracking branch 'upstream/main' into metrics-service-all…

811f13b

…ow-only-histogram-summaries Signed-off-by: Sebastian Schepens <sebastian.schepens@mercadolibre.com>

sschepens dismissed stale reviews from lizan and jmarantz via 811f13b April 5, 2023 14:16

repokitteh-read-only bot added api and removed waiting labels Apr 5, 2023

jmarantz approved these changes Apr 6, 2023

View reviewed changes

jmarantz merged commit c40aca4 into envoyproxy:main Apr 6, 2023

sschepens deleted the metrics-service-allow-only-histogram-summaries branch April 10, 2023 13:33

This was referenced Oct 24, 2023

Support prometheus summary metric types on the admin endpoint #30471

Closed

admin: Add support for prometheus summary metrics #30479

Merged

sschepens mentioned this pull request Mar 1, 2024

metrics_service: populate histogram summary sample sum #32666

Merged

Conversation

sschepens commented Feb 28, 2023

Uh oh!

repokitteh-read-only bot commented Feb 28, 2023

Uh oh!

repokitteh-read-only bot commented Feb 28, 2023

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sschepens commented Feb 28, 2023

Uh oh!

sschepens commented Feb 28, 2023

Uh oh!

Uh oh!

mattklein123 commented Mar 2, 2023

Uh oh!

repokitteh-read-only bot commented Mar 2, 2023

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

sschepens commented Mar 31, 2023

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Apr 5, 2023

Uh oh!

jmarantz commented Apr 5, 2023

Uh oh!

jmarantz commented Apr 6, 2023

Uh oh!

repokitteh-read-only bot commented Apr 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants