Skip to content

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

Merged
merged 1 commit into from
Jul 11, 2025

Conversation

BradErz
Copy link
Contributor

@BradErz BradErz commented Jun 3, 2025

If the DC extension is configured, this will output metrics about the volts, amps, watts and the state of the power on/off.

@CLAassistant
Copy link

CLAassistant commented Jun 3, 2025

CLA assistant check
All committers have signed the CLA.

Comment on lines +6 to +10
"github.com/prometheus/client_golang/prometheus"
)

var (
dcCurrentGauge = prometheus.NewGauge(prometheus.GaugeOpts{
Copy link
Contributor

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.

Suggested change
"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{

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@SuperQ SuperQ Jun 12, 2025

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.

Copy link
Contributor

@IDisposable IDisposable Jun 12, 2025

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.

Copy link
Contributor Author

@BradErz BradErz Jun 14, 2025

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.

Copy link
Contributor

@SuperQ SuperQ Jun 17, 2025

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
}

Copy link
Contributor

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.

@BradErz
Copy link
Contributor Author

BradErz commented Jun 14, 2025

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.

@adamshiervani adamshiervani merged commit aada3d9 into jetkvm:dev Jul 11, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants