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.

Sign up for GitHub

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
Merged

feat(metrics): adding prometheus metrics for dc power extension #556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions dc_metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package kvm

import (
"sync"

"github.com/prometheus/client_golang/prometheus"
)

var (
dcCurrentGauge = prometheus.NewGauge(prometheus.GaugeOpts{
Comment on lines +6 to +10
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.

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

@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.

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

@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.

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

@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.

The other thing that could be done is to add a feature flag metric like jetkvm_dc_power_present.

Name: "jetkvm_dc_current_amperes",
Help: "Current DC power consumption in amperes",
})

dcPowerGauge = prometheus.NewGauge(prometheus.GaugeOpts{
Name: "jetkvm_dc_power_watts",
Help: "DC power consumption in watts",
})

dcVoltageGauge = prometheus.NewGauge(prometheus.GaugeOpts{
Name: "jetkvm_dc_voltage_volts",
Help: "DC voltage in volts",
})

dcStateGauge = prometheus.NewGauge(prometheus.GaugeOpts{
Name: "jetkvm_dc_power_state",
Help: "DC power state (1 = on, 0 = off)",
})

dcMetricsRegistered sync.Once
)

// registerDCMetrics registers the DC power metrics with Prometheus (called once when DC control is mounted)
func registerDCMetrics() {
dcMetricsRegistered.Do(func() {
prometheus.MustRegister(dcCurrentGauge)
prometheus.MustRegister(dcPowerGauge)
prometheus.MustRegister(dcVoltageGauge)
prometheus.MustRegister(dcStateGauge)
})
}

// updateDCMetrics updates the Prometheus metrics with current DC power state values
func updateDCMetrics(state DCPowerState) {
dcCurrentGauge.Set(state.Current)
dcPowerGauge.Set(state.Power)
dcVoltageGauge.Set(state.Voltage)
if state.IsOn {
dcStateGauge.Set(1)
} else {
dcStateGauge.Set(0)
}
}
4 changes: 4 additions & 0 deletions serial.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ func pressATXResetButton(duration time.Duration) error {

func mountDCControl() error {
_ = port.SetMode(defaultMode)
registerDCMetrics()
go runDCControl()
return nil
}
Expand Down Expand Up @@ -188,6 +189,9 @@ func runDCControl() {
dcState.Current = amps
dcState.Power = watts

// Update Prometheus metrics
updateDCMetrics(dcState)

if currentSession != nil {
writeJSONRPCEvent("dcState", dcState, currentSession)
}
Expand Down