kube-state-metrics icon indicating copy to clipboard operation
kube-state-metrics copied to clipboard

Improve --metric-labels-allowlist handling of non-plural resource names

Open dmitri-lerko opened this issue 3 years ago • 6 comments

What would you like to be added: --metric-labels-allowlist to support singular form pod vs pods for resources or warn when user uses singular form of common, predefined resources.

Why is this needed: Currently, it is trivial to make a mistake when configuring KSM's labels allowlist and provide a singular form of resource name pod, namespace etc. This would not result in any error or warning, but also will not make KSM capture the metrics. This behaviour can confuse the end user.

Describe the solution you'd like Option 1: Support both singular and plural forms. Option 2: Warn when singular form is discovered.

Additional context Happy to code the solution once we agree on the approach to take :)

dmitri-lerko avatar Jul 13 '22 10:07 dmitri-lerko

Thanks, I think this would make sense. I'm just not sure if it is simple to implement without explicit discovery against the API server.

fpetkovski avatar Jul 13 '22 11:07 fpetkovski

For option 2 - would layman's approach of validating s or es ending of resource name and warning along the lines:

Resource x does not use a required plural form. Ignoring resource x in --metrics-labels-allowlist argument.

work?

dmitri-lerko avatar Jul 13 '22 11:07 dmitri-lerko

Since we have an explicit list of resources from which we export metrics, I think I would prefer a solution where we check against that list and emit a warning if the resource is not present. We can also add a hint to use plural forms, but I wouldn't base it on the ending of a word.

fpetkovski avatar Jul 13 '22 16:07 fpetkovski

/assign

rexagod avatar Sep 02 '22 06:09 rexagod

@dmitri-lerko PLMK if you're not working on this at the moment and I'll pick it back up.

rexagod avatar Sep 02 '22 18:09 rexagod

Do we want to diverge from the behavior that --resource follows (fail on singulars), and just warn the user in case of --metric-labels-allowlist? In my PR I've assumed the fact that a singular resource input comes from the user as a mistake, in which case, we fail. But LMK if we still just want to warn the user and I'll make the change.

rexagod avatar Sep 02 '22 20:09 rexagod

I would still treat resources in the singular form as invalid, it will just complexify the codebase to have to support both and we would have to propagate that change to all the options that assume that only plural resources are provided.

But I agree that we should definitely add some explicit warnings, I would even go as far as exiting with an error when ksm is misconfigured.

dgrisonnet avatar Oct 21 '22 16:10 dgrisonnet