Skip to content
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

rafctor LookupCluster #44468

Merged
merged 4 commits into from Apr 25, 2023
Merged

rafctor LookupCluster #44468

merged 4 commits into from Apr 25, 2023

Conversation

zirain
Copy link
Member

@zirain zirain commented Apr 21, 2023

Please provide a description of this PR:

  1. remove duplicate code
  2. introduce new metric for lookup fail

@zirain zirain requested review from a team as code owners April 21, 2023 08:27
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 21, 2023
)
}

var providerLookupClusterFailures = monitoring.NewSum(
Copy link
Member

Choose a reason for hiding this comment

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

I think Gauge is better, we can record which cluster look up failed rather than always increasing the num

Copy link
Member Author

@zirain zirain Apr 23, 2023

Choose a reason for hiding this comment

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

when to decrease the count?

Copy link
Member

Choose a reason for hiding this comment

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

Please refer to envoyfilter patching metric, the sum number here is meaningless, it oculd be unlimited big

Copy link
Member Author

Choose a reason for hiding this comment

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

For a counter, user should care about rate or increase.
if you are talking about pilot_envoy_filter_status, it's disabled by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @ramaraochavali , do you know why it's disabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is disabled because of possible performance impact. People who want will only enable it in dev envs

Copy link
Member Author

Choose a reason for hiding this comment

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

After take a look at istio.io/pkg/monitoring, it's impossible to reset all metrics to 0 when services or telemetries chaged.
cc @hzxuzhonghu

@istio-testing istio-testing merged commit ba329a9 into istio:master Apr 25, 2023
28 checks passed
@zirain zirain deleted the lookup-cluster branch April 25, 2023 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants