-
Notifications
You must be signed in to change notification settings - Fork 61
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
Additional Metrics #66
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thanks @elubow.
-1 on this. This will break every existing deployment that uses this exporter. If you want to make changes like this, please provide a metric relabeling config so that users can seamlessly migrate all dashboards and alerting configurations. |
@StephanErb how so? |
I only had 1 existing deployment to test it on and it didn't break it. I'm also following this up with another PR which has much better error handling. |
For example, the metric Normally changes like this are done by providing a metric relabelling config, so that both metrics can co-exist at the same time. For example, there is currently a similar issue in the Prometheus node exporter prometheus/node_exporter#830 |
I did not intend to sound so harsh. Besides of the breaking naming change this contribution looks great! I am sure we can figure it out :) |
Part of the issue here is that some of the metrics aren't even being collected all the time. For instance Also, it's only harsh until constructive feedback transpires :) Point taken, but we definitely have some issues that I don't see a path to resolution for. |
Not at all harsh - this is very fair criticism, I was trigger-happy on the merge. I'll make sure we get a relabelling config in soon. |
Additional Metrics
This pull request includes laying out all the metrics for: