-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add mechanism to allow/deny high cardinality metrics #1102
Comments
cc @brancz @tariq1890 ^ |
Thanks for starting this conversation @dgrisonnet . The issue where we can't merge an allow and deny directive is definitely a flaw that needs fixing. However, I lean towards some simplicity. This is my personal opinion; having a What I've had in mind was having a simple interface for users to provide this configuration and some rules of precedence. For eg:- A specific metric name would take higher precedence over a regex pattern. In your case, when we allow Open to hearing the thoughts of the other maintainers on this. I am definitely open to being convinced otherwise as it's definitely possible that my proposal has some holes in it :) |
I think we made a mistake by ever trying to use the allow/deny lists for what are essentially feature gates. I think we should have explicit feature gates for metrics we want to be off by default. Then having allow/deny list being mutually exclusive is not a problem anymore. That said, I think label and annotation metrics are yet another case, where each label that we expose needs to be allow/deny listed (maybe even only allow-listed), instead of the metric itself. And that for each resource. |
Hi guys! After reading the OP's proposal, I definitely think this is needed in the project however I do think it could be ugly to use when it comes to using regex and precedence logic. I like @brancz's proposal of having feature flags/gates for each metric and then an allow/deny list IMO is simple and doesn't complicate things. |
Thank you for all the comments. I also really like @brancz's proposal of having feature gates to handle denied by default metrics. As for labels, do you think we can also apply feature gates to them? Having metrics as key and a list of labels defaulting to an empty list as value maybe? |
Yeah I think only allow-listing the labels would be the same here, unless I'm misunderstanding. |
Yes, if done this way it would be the same. I am actually more in favor of the allow-listing of labels approach rather than the allow/deny listing one. As mentioned before, we should apply labels listing to metrics like labels and annotations which I understood as applying it to metrics having an unbound number of labels. In my opinion, if we do allow/deny listing, it would only amount to reducing something that is still unbound in the end. Whilst on the other hand, by only allow-listing labels we could bound the number of labels of these metrics. |
Precisely my thought as well. |
Should we update this issue to reflect your comment, as now they are different from what I can tell. |
I'd prefer creating a new to reflect the new decisions and closing this in favour of that. |
That sounds good as well! 👍 And link to this one for clarity. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Closing as #1125 added the feature. |
Is this a BUG REPORT or FEATURE REQUEST?:
/kind feature
Context
More and more users want to expose high cardinality metrics; however, it comes with great costs for the default configuration of kube-state-metrics.
#941
#1047
Proposal
In order to reduce the number of metrics exposed by the default configuration, we could deny some metrics by default (e.g. labels and annotations) and allow users to expose them via the arguments.
Predefined deny list example:
The straight forward approach would be to append metrics denied by default to the existing deny list and remove the allowed one from it; however, in the example below, we can't really do that.
The current AllowDenyList implementation can't really be used to solve this problem since it is only using one list to allow/deny metrics and as seen in the above example we can't always merge the allowList and the denyList into a single list.
Thus, I was thinking about answering the problem in the following way:
metric-list
option taking over the behavior of the currentmetric-allowlist
.metric-allowlist
option to allow denied by default metrics.metric-allowlist
andmetric-denylist
mutually exclusive they would be complementary and mutually exclusive tometric-list
.The text was updated successfully, but these errors were encountered: