-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add apcupsd_ups_status and apcupsd_ups_info metrics #8
Conversation
@mdlayher Any chance you could have a look at this PR? I'm sorry that it got a bit out of hand, but i think it's worth it! 😄 |
Thank you for the PR and I appreciate the effort you've put in, but this PR has a lot of unrelated changes that make it difficult to review quickly. My free time is rather limited this week. I'll try to get to this early next week. |
@@ -0,0 +1,2 @@ | |||
.idea |
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.
Please add these to your global gitignore instead.
|
||
You can find the dashboard JSON in the `grafana` folder. | ||
|
||
![sample](https://raw.githubusercontent.com/jangrewe/apcupsd_exporter/master/grafana/screenshot.png) |
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.
This would need to reflect the path of the upstream repository.
@@ -1,19 +1,55 @@ | |||
github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973 h1:xJ4a3vCFaGF/jqvzLMYoU8P317H5OQ+Via4RmuPwCS0= |
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.
Seems like a lot of churn, did you go mod tidy
too?
@@ -41,105 +44,119 @@ var _ prometheus.Collector = &UPSCollector{} | |||
// NewUPSCollector creates a new UPSCollector. | |||
func NewUPSCollector(ss StatusSource) *UPSCollector { | |||
var ( | |||
labels = []string{"hostname", "ups_name", "model"} | |||
infoLabels = []string{"hostname", "ups_name", "model"} |
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.
Move these to package level so they can be deduplicated at this point.
|
||
UPSInfo: prometheus.NewDesc( | ||
prometheus.BuildFQName(namespace, "", "ups_info"), | ||
"Hostname, UPS Model and Name.", |
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.
"Metadata about a UPS."
More flexible for the future.
nil, | ||
), | ||
|
||
UPSStatus: prometheus.NewDesc( |
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.
I feel like it would make more sense to include the status label in UPSInfo rather than in its own metric.
Just a note to say that I have tested from the @jangrewe repo, and it works for me:
I am ambivalent, however, about the loss of metadata from the individual metrics. Including metadata in every metric is how the official snmp_exporter works. Example:
This in turn makes it very easy to use in grafana dashboards, with all of the graph lines labelled with the available metadata, and easy to filter on any particular piece of metadata. If they changed it to work the way proposed here, then it would look more like this:
This has some advantages with regards to continuity of timeseries if metadata changes, but seems to be a lot harder to work with in grafana. Let's consider the simpler case of apcupsd with this proposed patch applied. Suppose you have multiple UPSes, and are showing all the "time remaining" in a single graph with a separate line for each UPS. Once prometheus has added its own scraping labels, you would get metrics like this:
If you want your graphs labelled with the UPS name or hostname, you need to invoke some prometheus join magic. According to this article I think that adding the
I don't pretend to understand this, and it may be wrong. In any case it would have to be copy-pasted into every grafana query in the dashboard. Similarly: suppose you want a drop-down menu to select between UPSes. If you don't mind just showing the 'instance' value this is straightforward:
But if you want the dropdown to select on 'ups_name', it gets tricky.
|
For reference, discussion about it at prometheus/snmp_exporter#180 and prometheus/snmp_exporter#339 |
I ended up fixing things up, thanks for your time. |
Can this be revisited? It would be very useful to have status info in the metrics, or as a label in |
FWIW, I changed to https://github.com/damomurf/apcupsd-exporter a while ago. |
Fair enough, but I need both status metrics and the metrics that this exporter provides... Both exporters use the same command line tool so it seems a bit silly to be installing two exporters for the same tool 😅 |
This adds multiple
apcupsd_ups_status
metrics, each containing the labelstatus
with all possible values theSTATUS:
line can contain. (Label values taken from https://www.systutorials.com/docs/linux/man/8-apcaccess/)This also add the
apcupsd_ups_info
metric and moves all the redundant info labels from the other metrics to this one (as suggested by you in #6).And lastly this PR adds an example Grafana dashboard and the respective screenhot.
Added bonus: a Dockerfile (image size: 8MB), with the image available at https://hub.docker.com/r/jangrewe/apcupsd-exporter