Skip to content

stats: allow custom histogram buckets #2800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 6, 2023

Conversation

kyessenov
Copy link
Contributor

Compromise to #2539, to prefer over the bootstrap EnvoyFilter.

@zirain

@kyessenov kyessenov requested a review from a team as a code owner June 2, 2023 20:56
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 2, 2023
@kyessenov kyessenov added the release-notes-none Indicates a PR that does not require release notes. label Jun 2, 2023
@kyessenov
Copy link
Contributor Author

/retest

Copy link
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this's a feature only supported by envoy, using annotation is LGTM.

Copy link
Contributor

@costinm costinm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's reasonable - we have quite a few things exposed as annotations.

One concern about pod annotations is that in Ambient they will likely cease functioning - except for Waypoints, where the annotation will go on the Gateway resource. But that's the case for most CRDs we have with workload selector, so nothing special.

On this note - would be good to add K8S Gateway as resource that will support the annotation ( they get copied to the deployment AFAIK ).

- name: sidecar.istio.io/statsHistogramBuckets
featureStatus: Alpha
description: |
Specifies the semicolon separated custom histogram buckets with a prefix matcher. Example: `istio:1,5,10,50,100,500,1000,5000,10000;envoy:1,5,10,25,50,100,250,500,1000,2500,5000,10000`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • document the default
  • why do we need istio and envoy ? Can one be used for both ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do need them separate, should we just make this json? Its not that complicated but we are essentially creating a micro-language we need to parse.

if we just make them one then its a simple CSV so no problem

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Prefix is not needed but usually used.

@costinm
Copy link
Contributor

costinm commented Jun 6, 2023

Worth mentioning that customizing stats bucket for specific pods is actually best granularity for such an API - the defaults should work for most common cases, overrides are needed for pods that have unusual behavior, like long lived connections ( istiod itself ).

Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall as an experimental annotation, just one comment on the formatting (doesn't need to block, just want to make sure it is at least ACKed before moving forward)

- name: sidecar.istio.io/statsHistogramBuckets
featureStatus: Alpha
description: |
Specifies the semicolon separated custom histogram buckets with a prefix matcher. Example: `istio:1,5,10,50,100,500,1000,5000,10000;envoy:1,5,10,25,50,100,250,500,1000,2500,5000,10000`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do need them separate, should we just make this json? Its not that complicated but we are essentially creating a micro-language we need to parse.

if we just make them one then its a simple CSV so no problem

Signed-off-by: Kuat Yessenov <[email protected]>
@howardjohn howardjohn added the do-not-merge/hold Block automatic merging of a PR. label Jun 6, 2023
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment, otherwise LGTM. feel free to remove hold

@@ -199,6 +199,14 @@ annotations:
resources:
- Pod

- name: sidecar.istio.io/statsHistogramBuckets
featureStatus: Alpha
description: Specifies the custom histogram buckets with a prefix matcher to separate the Istio mesh metrics from the Envoy stats, e.g. `{"istio":{1,5,10,50,100,500,1000,5000,10000},"envoy":{1,5,10,25,50,100,250,500,1000,2500,5000,10000}}`. Default buckets are `{0.5,1,5,10,25,50,100,250,500,1000,2500,5000,10000,30000,60000,300000,600000,1800000,3600000}`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: Specifies the custom histogram buckets with a prefix matcher to separate the Istio mesh metrics from the Envoy stats, e.g. `{"istio":{1,5,10,50,100,500,1000,5000,10000},"envoy":{1,5,10,25,50,100,250,500,1000,2500,5000,10000}}`. Default buckets are `{0.5,1,5,10,25,50,100,250,500,1000,2500,5000,10000,30000,60000,300000,600000,1800000,3600000}`.
description: Specifies the custom histogram buckets with a prefix matcher to separate the Istio mesh metrics from the Envoy stats, e.g. `{"istio":[1,5,10,50,100,500,1000,5000,10000],"envoy":[1,5,10,25,50,100,250,500,1000,2500,5000,10000]}`. Default buckets are `[0.5,1,5,10,25,50,100,250,500,1000,2500,5000,10000,30000,60000,300000,600000,1800000,3600000]`.

Current one is not valid json

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we specify what the unit is as well? milliseconds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no unit. We have both byte count and milliseconds in use.

Signed-off-by: Kuat Yessenov <[email protected]>
@kyessenov kyessenov removed the do-not-merge/hold Block automatic merging of a PR. label Jun 6, 2023
@istio-testing istio-testing merged commit 870636f into istio:master Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes-none Indicates a PR that does not require release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants