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

Standardize prometheus naming convention for keda metrics #4854

Closed
pragmaticivan opened this issue Aug 2, 2023 · 17 comments · Fixed by #5687
Closed

Standardize prometheus naming convention for keda metrics #4854

pragmaticivan opened this issue Aug 2, 2023 · 17 comments · Fixed by #5687

Comments

@pragmaticivan
Copy link
Contributor

Proposal

Because of issues with Prometheus remote writers, it's ideal that we follow Prometheus suffix conventions for counters. If not followed strictly, external vendors supporting Prometheus remote writers might receive the metric as a Gauge instead.

Context:

Use-Case

Vendors such as newrelic can use Prometheus metrics via remote writer properly.

Is this a feature you are interested in implementing yourself?

Maybe

Anything else?

No response

@pragmaticivan pragmaticivan added feature-request All issues for new features that have not been committed to needs-discussion labels Aug 2, 2023
@pragmaticivan
Copy link
Contributor Author

We might need to deprecate existing metrics and create new ones.

@pragmaticivan
Copy link
Contributor Author

@JorTurFer, if approved, I can probably take this one.

@JorTurFer
Copy link
Member

WDYT @kedacore/keda-core-contributors ?

@zroubalik
Copy link
Member

@pragmaticivan could you please write down the list of metrics to be changed. The old and new name. Thanks!

@tomkerkhove tomkerkhove removed feature-request All issues for new features that have not been committed to needs-discussion labels Aug 17, 2023
@pragmaticivan
Copy link
Contributor Author

pragmaticivan commented Aug 28, 2023

Screenshot 2023-08-28 at 4 05 10 PM @zroubalik afaik, only the ones marked as red.

Although I was thrown off by naming some being gauge instead of a histogram.

@pragmaticivan
Copy link
Contributor Author

I also noticed some of the HELP (descriptions) don't match the description in the doc.

@pragmaticivan
Copy link
Contributor Author

Possible steps:

  1. Update Description for affected metrics to include a prefix like: Deprecation Warning - This metric is deprecated and replaced with 'new_metric'
  2. Create new metrics and add increments right below the code for the existing ones.
  3. Update docs (include deprecation notice)
  4. Update Grafana template

@tomkerkhove
Copy link
Member

Sounds like good execution steps imo, are you willing to contribute this?

For 2), let's make sure we increment new metrics first though.

@pragmaticivan
Copy link
Contributor Author

Yes, I can take that one.

@pragmaticivan
Copy link
Contributor Author

After taking a deeper look yesterday, keda_scaler_errors vs keda_scaler_errors_total seems to be the same metric, called in the same place. The only difference is that keda_scaler_errors has the labels in place, but technically you could use this one with the same intent (sum) so I don't think we need the counter for all since we already have the other metric.

@pragmaticivan
Copy link
Contributor Author

This issue might be obsolete if this is available: https://github.com/kedacore/keda/pull/4860/files 🎉

@tomkerkhove
Copy link
Member

Well not really because Prometheus metrics are not being removed; we're adding it next to each other :)

@wonko
Copy link
Contributor

wonko commented Nov 9, 2023

I made a PR with a (biased) suggestion to rework the metric names.

  • units are added (to latency)
  • some wordings have been changed in the help
  • some metrics have a new name
  • deprecated a sum() metric
  • every metric that needs a deprecation has been duplicated to provide the new metric

Open to suggestions.

Copy link

stale bot commented Jan 8, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Jan 8, 2024
@zroubalik zroubalik removed the stale All issues that are marked as stale due to inactivity label Jan 10, 2024
Copy link

stale bot commented Mar 10, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Mar 10, 2024
Copy link

stale bot commented Mar 21, 2024

This issue has been automatically closed due to inactivity.

@stale stale bot closed this as completed Mar 21, 2024
@JorTurFer
Copy link
Member

This is in progress

@JorTurFer JorTurFer reopened this Mar 24, 2024
@stale stale bot removed stale All issues that are marked as stale due to inactivity labels Mar 24, 2024
@JorTurFer JorTurFer mentioned this issue Apr 16, 2024
35 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
5 participants