search

discourse / prometheus_exporter

A framework for collecting and aggregating prometheus metrics
MIT License
532 stars 154 forks source link

rename metrics to match prometheus official naming conventions #184

Closed Shimokuni closed 3 years ago

Shimokuni commented 3 years ago

closes https://github.com/discourse/prometheus_exporter/issues/182

SamSaffron commented 3 years ago

thanks, looks like you are making good progress. We do need to also improve metric names and make them more consistent, can you have a look?

Shimokuni commented 3 years ago

I think this is all fixed now. https://github.com/discourse/prometheus_exporter/pull/184/commits/a845e548b62deffb9f2a61a233f9dca8dcd9a192 And I added some missing tests for unicorn. https://github.com/discourse/prometheus_exporter/pull/184/commits/9669a3b7fb8b44d8cbff1aba2add00751dbd4d9a

We do need to also improve metric names and make them more consistent, can you have a look?

For example, do you mean to fix the inconsistency that resque has a gauge type metric called resque_failed_jobs, while sidekiq has a counter type metric called sidekiq_failed_jobs_total?

Shimokuni commented 3 years ago

There were still some things that needed to be fixed. I've just finished it.https://github.com/discourse/prometheus_exporter/pull/184/commits/aff314b03a7ab55fb89cf106741d59c459310140 And I added some more missing tests. https://github.com/discourse/prometheus_exporter/pull/184/commits/1a3a31349f5af647ee56e8a4073e2a26020b8bd5

SamSaffron commented 3 years ago

This is a very nice change, going to hold off a bit on a release cause it has potential to break quite a few people, let's let it bake for 2-4 weeks.

salzig commented 2 years ago

Would've been awesome if changes like those get a graceperiod where both names are provided. We ended up in a situation where our dashboards are working for one or the other environemnt (as in stage/production), cause our deployment cycles are sadly not as fast as we wish for (Customer does it's own QA additionally, and very slow).

SamSaffron commented 2 years ago

@salzig kind of impossible to do this without some very bad side effects (exploding metric counts)

The change was flagged as a major change we did major version bump here.

salzig commented 2 years ago

@SamSaffron i don't see a problem with the name change in the major version. Maybe our setup is "strange", but name changes on one day to another are quite annoying, as we have to choose between showing the old or the new name, or query both metrics in our panels, as our Grafana installation is central across all environments.

Sorry, overall my last message was written in frustration, cause another metric (http request duration) was also renamed, and additionally lost a label which we used (status). Keeping a old version, isn't really an option. So we need to figure out how to proceed.

Thanks to all of you for providing and maintaining this as open source.