-
Notifications
You must be signed in to change notification settings - Fork 185
feat(metrics): adding prometheus metrics for dc power extension #556
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
Conversation
"github.com/prometheus/client_golang/prometheus" | ||
) | ||
|
||
var ( | ||
dcCurrentGauge = prometheus.NewGauge(prometheus.GaugeOpts{ |
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 use promauto
to register metrics.
"github.com/prometheus/client_golang/prometheus" | |
) | |
var ( | |
dcCurrentGauge = prometheus.NewGauge(prometheus.GaugeOpts{ | |
"github.com/prometheus/client_golang/prometheus" | |
"github.com/prometheus/client_golang/prometheus/promauto" | |
) | |
var ( | |
dcCurrentGauge = promauto.NewGauge(prometheus.GaugeOpts{ |
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.
There is a mixed bag of recommendations over at prometheus in recent PRs
If we DO want to do swap to promauto
, we should be using .With(reg)
should be done to ensure it uses the correct prometheus.Registerer
.
The biggest win for promauto
is that it ensures you don't forget the registering, but this PR does seem to handle it correctly already.
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.
Interesting, as a Prometheus developer I don't know why they are recommending against using promauto. It's been a standard recommendation for quite a while now for exactly the "don't forget to register". We've been migrating to promauto for quite a while now.
We don't need to switch to .With(reg)
until we actually use a non-default registry.
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.
In reading over the linked PR comments, I think that discussion is only related to the fact that there is a bunch of default and non-default registry stuff going on in that specific codebase that needs cleaning up.
In this codebase, we have a very simple/clean use of the default registry, which is perfectly normal for this kind of code.
This codebase is already using promauto for all the existing metrics.
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.
Perfect! Thanks for the correction. I appreciate the internal guidance.
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.
Thanks @SuperQ it's been so long since I used the underlying raw Prometheus libraries, I wasn't even aware of promauto.
Question, do we want to always register the metrics if the extension is optional and not always present? My original thinking was that it would only be present if the extension was loaded otherwise it will register everything with a 0 value.
I guess if we do want to register them all with 0 values we should try to have a metric that shows when the DC extension is enabled? So we know if metrics should be non-zero.
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.
Usually what I do for optional metrics is put the vars into the setup function, rather then have them as package vars. You'll of course need a metrics struct like this:
type dcMetrics struct {
current prometheus.Gauge,
watts prometheus.Gauge,
volts prometheus.Gauge,
state prometheus.Gauge,
}
...
func registerDCMetrics() dcMetrics{
var m cdMetrics
m.current = promauto.NewGauge(...)
...
return m
}
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.
The other thing that could be done is to add a feature flag metric like jetkvm_dc_power_present
.
I came across this while testing. If the serial cable is unplugged while the device is live we dont seem to hook that anywhere so the power state is just left at the last value before the serial cable disconnected. This behaviour is the same in the UI as it is in the metrics i added so fixing that bug would fix both the UI and metrics. Potentially we should open this as a bug and fix it in a new PR. |
If the DC extension is configured, this will output metrics about the volts, amps, watts and the state of the power on/off.