stats: support custom histogram bucket #2539

zirain · 2022-11-02T07:55:39Z

after istio/istio#41441, add first class api to support custom histogram bucket

mesh/v1alpha1/proxy.proto

howardjohn · 2022-11-02T15:08:20Z

mesh/v1alpha1/proxy.proto

+  message ProxyStatsHistogramBucketSetting {
+    // Specifies a matcher for stats and the buckets that matching stats should use.
+    // The match is applied to the original stat name before tag-extraction,
+    // for example `cluster.exampleclustername.upstream_cx_length_ms`.


Does this apply for istio_request_total type metrics? Or only envoy ones?

istio/istio#41441 change the implement of istio.stats (with Telemetry API), after that this work for all stats.

ok

Telemetry already has a way of selecting metrics. I am a bit worried about using a different mechanism..

How can it be applied to counter metric?

it only works for Histogram metric

I want to re-iterate my concern that we are creating a new mechanism for selecting metrics when we already have one.

unlike Telemetry API, The match is applied to the original stat name before tag-extraction

zirain · 2022-11-03T02:57:36Z

@hzxuzhonghu @howardjohn should support this on ProxyConfig?

hzxuzhonghu · 2022-11-04T03:48:09Z

ProxyConfig should only include xds client metadata, but later it is extended to include some other data plane config, which is not covered by API. For this case, telemetry api can apply to a specific proxy, so I feel proxy-related metric setting should be there.

zirain · 2022-11-04T04:06:04Z

ProxyConfig should only include xds client metadata, but later it is extended to include some other data plane config, which is not covered by API. For this case, telemetry api can apply to a specific proxy, so I feel proxy-related metric setting should be there.

this's actually part of statsconfig in bootstrap, similar to ProxyStatsMatcher.InclusionPrefixes

mesh/v1alpha1/proxy.proto

hzxuzhonghu · 2022-11-04T06:21:41Z

type/v1alpha1/string_match.proto

+option go_package="istio.io/api/type/v1alpha1";
+
+// Describes how to match a given string. Match is case-sensitive.
+message StringMatch {


I think there is already such a API, which is used by VS

there is two message StringMatch in this repo, they may share same API just like WorkloadSelector?

hzxuzhonghu · 2022-11-04T06:22:31Z

this's actually part of statsconfig in bootstrap, similar to ProxyStatsMatcher.InclusionPrefixes

IC,

kyessenov · 2022-11-09T16:10:02Z

LGTM. I think it's useful for customizing standard Envoy metrics, too, so it can't be in just the stats filter. Looks like it's mostly verbatim copied into bootstrap, so it should be noted that it requires a proxy restart.

zirain · 2022-11-10T01:40:24Z

LGTM. I think it's useful for customizing standard Envoy metrics, too, so it can't be in just the stats filter. Looks like it's mostly verbatim copied into bootstrap, so it should be noted that it requires a proxy restart.

update notes

nrjpoddar · 2022-11-14T09:48:56Z

This is a bit confusing - We were trying to get away from ProxyConfig options for Telemetry in lieu of Telemetry API. Why is the new API not used?

Secondly, it's unclear what all metrics does it effect. What happens if you update a histogram metrics buckets? Can you make sense of old and new data? Does it use the same name?

zirain · 2022-11-14T10:37:43Z

This is a bit confusing - We were trying to get away from ProxyConfig options for Telemetry in lieu of Telemetry API. Why is the new API not used?

this's actually part of bootstrap, Temeletry API cannot achieve

Secondly, it's unclear what all metrics does it effect. What happens if you update a histogram metrics buckets? Can you make sense of old and new data? Does it use the same name?

it will not change metrics name, only change the default buckets of histogram metric.

zirain · 2022-12-01T01:08:20Z

kindly ping @nrjpoddar

zirain · 2022-12-09T07:28:58Z

Telemetry API cannot change configuration in bootstrap, BDS(bootstrap discovery request) also will not help in this scenario.

nrjpoddar

Overall LGTM but would need Telemetry WG experts to give their approval.

nrjpoddar · 2022-12-09T14:41:07Z

mesh/v1alpha1/proxy.proto

+    // Specifies a matcher for stats and the buckets that matching stats should use.
+    // The match is applied to the original stat name before tag-extraction,
+    // for example `cluster.exampleclustername.upstream_cx_length_ms`.
+    istio.type.v1alpha1.StringMatch match = 1;


Why a new type?

there is two protoo StringMatch in this repo, they may share same API just like WorkloadSelector?

I can reuse the one with VS.

costinm

I have a few concerns here:

ProxyConfig vs Telemetry - not sure there is a clear answer on why it's kept in ProxyConfig
the regex matching - I don't think it's a good idea in general, but for metrics in particular.
the shape of the API - I think it's reasonable to customize metrics, but I would expect an
API where for each metric (by name - we don't have that many) we would have some settings associated to that metric - buckets, etc.

But most important concern: we are in the middle of few major changes, with
OpenTelemetry maturing, Ambient, K8S Gateway happening. We also know that
Istio telemetry 'cardinality' and miss-match with other telemetry standards is a problem.
One of the options to improve and better integrate with OTel - for example generate the of the high-cardinality metrics from otel access logs ( historically istio metrics were designed with Mixer - which was access log based, so less of a problem ).

I would really like to see a doc or have a discussion about the future of telemetry and
how it fits with the evolving world around us - and which APIs belong in Istio and which
in open telemetry.

costinm · 2022-12-09T16:23:29Z

Regarding the bootstrap - I think this is an implementation detail of Envoy that leaks into API without any good reason.

We can use any of the Istio APIs in bootstrap - just need to inject them into the pod like we do for ProxyConfig, or get them over XDS before starting envoy ( agent already gets the root CAs from Istiod ). We can get all the config in agent
already.

Ambient on the other hand is likely to not have any injection at all - and a lot of the changes that depend on injecting
to a pod will no longer work ( Waypoint is shared ). I think any new API should take into account ambient, given
the major simplification and perf improvements it brings.

zirain · 2022-12-10T07:31:15Z

ProxyConfig vs Telemetry - not sure there is a clear answer on why it's kept in ProxyConfig

AFAIK, Telemetry used to generate configuration for stats filter, and as a part of bootstrap config, that's why I put this on ProxyConfig.

the regex matching - I don't think it's a good idea in general, but for metrics in particular.

I agree that regex it's not a good idea, but it's widely used in envoy statistic system.

I would really like to see a doc or have a discussion about the future of telemetry and
how it fits with the evolving world around us - and which APIs belong in Istio and which
in open telemetry.

that's another topic, which is too large for this PR (IMO, this's minor improvement)
Before v1.17, this won't work on a wasm filter, it's not a bad good idea to provide an api from the beginning of switching to native filter.

costinm · 2022-12-10T17:30:44Z

On Fri, Dec 9, 2022 at 11:31 PM zirain ***@***.***> wrote: ProxyConfig vs Telemetry - not sure there is a clear answer on why it's kept in ProxyConfig AFAIK, Telemetry used to generate configuration for stats filter, and as a part of bootstrap config, that's why I put this on ProxyConfig.

We keep confusing implementation details in envoy with Istio API. The fact Envoy happens to configure something in bootstrap should not have any impact on the API surface. The agent can get telemetry configs and generate whatever bootstrap it needs - and when we move to ztunnel in ambient it may generate whatever rust needs. the regex matching - I don't think it's a good idea in general, but for

metrics in particular. I agree that regex it's not a good idea, but it's widely used in envoy statistic system.

Envoy configs are huge and usually just a reflection of its internal implementation details. There are a lot of features in Envoy we choose not to expose in Istio API - and even more that won't be exposed in K8S Gateway or OTel. Istio API should be implementable with ztunnel or proxyless gRPC. If users absolutely need an envoy config - we have EnvoyFilter, and I believe it is now possible to also have the bootstrap generated dynamically by Istiod, for users who want to take strong dependencies on envoy internal impl.

I would really like to see a doc or have a discussion about the future of telemetry and how it fits with the evolving world around us - and which APIs belong in Istio and which in open telemetry. that's another topic, which is too large for this PR (IMO, this's minor improvement) Before v1.17, this won't work on a wasm filter, it's not a bad good idea to provide an api from the beginning of switching to native filter.

Any API we add is going to make the transition to ambient and OTel a bit harder. I have nothing against the feature - it seems quite useful - but we must take into account what happens outside of Istio and where we want to go long term. For example, if this historgram setting will be used for TCP metrics - how would they be implemented by ztunnel (where there is no ProxyConfig - since it's shared per node - and no envoy ) ? And if we switch to OTel - does the collector or instrumentation have a similar config ?

…

— Reply to this email directly, view it on GitHub <#2539 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2SYJDPLK7RTO5YVVMLWMQWU3ANCNFSM6AAAAAARUZK47M> . You are receiving this because you commented.Message ID: ***@***.***>

kyessenov · 2022-12-10T20:14:35Z

To resolve some ambiguities:

Histogram buckets affect all sinks (OTel in the future, prometheus now), except Wasm ones (due to ABI). There's definitive user interest, so whatever chooses to implement Telemetry API must do it (e.g. if ztunnel chooses to use telemetry API, which is not given).
Telemetry API should define them for Istio metrics, and we need to ask users to use BDS or set them before injection. @costinm this would require capability to read telemetry APIs in injection for latter.
Leave Envoy metrics alone, continue using BDS or custom bootstrap which works already. I'd argue to actually get rid of most of ProxyConfig and just use BDS with EnvoyFilters but we haven't made BDS the default.

lei-tang · 2022-12-14T17:44:53Z

LGTM.

istio-testing · 2022-12-19T22:03:05Z

@zirain: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

zirain · 2022-12-23T03:05:36Z

cannot make an agreement, close this first.

zirain requested a review from a team as a code owner November 2, 2022 07:55

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 2, 2022

zirain mentioned this pull request Nov 2, 2022

[WIP] stats: support histogram bucket istio/istio#41730

Closed

zirain added the release-notes-none Indicates a PR that does not require release notes. label Nov 2, 2022

hzxuzhonghu reviewed Nov 2, 2022

View reviewed changes

mesh/v1alpha1/proxy.proto Show resolved Hide resolved

howardjohn reviewed Nov 2, 2022

View reviewed changes

hzxuzhonghu reviewed Nov 4, 2022

View reviewed changes

mesh/v1alpha1/proxy.proto Show resolved Hide resolved

hzxuzhonghu reviewed Nov 4, 2022

View reviewed changes

zirain mentioned this pull request Nov 5, 2022

Cannot shorten metric interval istio/istio#41763

Closed

zirain force-pushed the histogram_bucket branch from 47b37e1 to c3637a1 Compare November 9, 2022 07:13

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 9, 2022

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 22, 2022

zirain added 5 commits November 23, 2022 09:26

stats: support custom histogram bucket

e9ae70d

fix build

bc70aba

add example

59fe7b8

update example

db14024

update note

dbb67de

zirain force-pushed the histogram_bucket branch from 886f4b0 to dbb67de Compare November 23, 2022 01:26

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 23, 2022

fix gen

14e5f93

nrjpoddar reviewed Dec 9, 2022

View reviewed changes

costinm reviewed Dec 9, 2022

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 19, 2022

zirain closed this Dec 23, 2022

kyessenov mentioned this pull request Jun 2, 2023

stats: allow custom histogram buckets #2800

Merged

zirain deleted the histogram_bucket branch September 15, 2023 01:55

stats: support custom histogram bucket #2539

stats: support custom histogram bucket #2539

Uh oh!

Conversation

zirain commented Nov 2, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zirain commented Nov 3, 2022

Uh oh!

hzxuzhonghu commented Nov 4, 2022

Uh oh!

zirain commented Nov 4, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu commented Nov 4, 2022

Uh oh!

kyessenov commented Nov 9, 2022

Uh oh!

zirain commented Nov 10, 2022

Uh oh!

nrjpoddar commented Nov 14, 2022

Uh oh!

zirain commented Nov 14, 2022

Uh oh!

zirain commented Dec 1, 2022

Uh oh!

zirain commented Dec 9, 2022

Uh oh!

nrjpoddar left a comment

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!

costinm left a comment • 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!

costinm commented Dec 9, 2022

Uh oh!

zirain commented Dec 10, 2022

Uh oh!

costinm commented Dec 10, 2022 via email

Uh oh!

kyessenov commented Dec 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lei-tang commented Dec 14, 2022

Uh oh!

istio-testing commented Dec 19, 2022

Uh oh!

zirain commented Dec 23, 2022

Uh oh!

Uh oh!

costinm left a comment •

edited

Loading

kyessenov commented Dec 10, 2022 •

edited

Loading