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

Use labels for mapping metric types to metrics #219

Merged
merged 3 commits into from
Nov 4, 2020
Merged

Conversation

mikkeloscar
Copy link
Contributor

@mikkeloscar mikkeloscar commented Oct 30, 2020

For historical reasons (i.e. use of previous autoscaling API versions) we have been mapping metric names to the different collector types. I.e. if you wanted a metric based on prometheus you would name the metric prometheus-query.
This design has the downside that multiple metrics will have the same name and it becomes hard to distinguish as reported in #104
In some cases it can even prevent having more than a single metric of a certain type i.e. if you use the zmon-check metric and have the same check-id but different complex keys defined (complex keys must be defined as annotation, and this will overlap if you have more than a single zmon-check metric defined).

It was initially done because we didn't have any other identifiers in the HPA spec, at least for custom metrics, and this limitation was accidentally carried over for External metrics even though they have labels which can be used for such extra information. With v2beta2 we have selector/labels for all the metric types and this can then be used to store this information.

This PR introduces support for multiple metrics of the same type by adding a convention of a type label on the metrics e.g. type: prometheus which is used to identify which collector type should be used.

I have also aligned some of the naming for the types so they are more simple i.e. prometheus-query becomes type prometheus.

To provide backwards compatibility the old metric names are still supported, but marked as legacy so we eventually can deprecate and remove them.

Fix #104

@szuecs
Copy link
Member

szuecs commented Nov 2, 2020

@mikkeloscar can we log a warning in case old style is used?
This would help operators to migrate safely.

Signed-off-by: Mikkel Oscar Lyderik Larsen <mikkel.larsen@zalando.de>
Signed-off-by: Mikkel Oscar Lyderik Larsen <mikkel.larsen@zalando.de>
@mikkeloscar
Copy link
Contributor Author

@szuecs added a warning log

@szuecs
Copy link
Member

szuecs commented Nov 2, 2020

👍

Signed-off-by: Mikkel Oscar Lyderik Larsen <mikkel.larsen@zalando.de>
@mikkeloscar
Copy link
Contributor Author

Found a bug where the fallback didn't work in case some labels were defined for the metric. Fixed and added tests to cover this.

@mikkeloscar
Copy link
Contributor Author

👍

2 similar comments
@csenol
Copy link
Contributor

csenol commented Nov 3, 2020

👍

@szuecs
Copy link
Member

szuecs commented Nov 4, 2020

👍

@szuecs szuecs merged commit df0ed1f into master Nov 4, 2020
@szuecs szuecs deleted the label-indentify branch November 4, 2020 19:40
jonathanbeber added a commit that referenced this pull request Jun 1, 2021
This commit removes the logic that checks for an identifier label in the
http collector config. It also removes the documentation on the README
that mentions that the metric has to be in an old format, removed in
the #219.

Fixes #331

Signed-off-by: Jonathan Juares Beber <jonathanbeber@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hard to distinguish which metrics query is in play ... maybe this is a feature than a defect
3 participants