Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @karampok)

doc/Metrics.md, line 139 at r1 (raw file):

func newOriginator() originator {
    ns, sub := Namespace, "beaconing"

What's the advantage of using an additional variable for Namespace?

doc/Metrics.md, line 144 at r1 (raw file):

    b := prom.NewCounterVec(ns, sub, "originated_beacons_total",
        "Number of beacons originated", lab.Labels())
    b.WithLabelValues(lab.Values()...).Inc()

As pointed out in chat Inc should not be needed.

doc/Metrics.md, line 144 at r1 (raw file):

    b := prom.NewCounterVec(ns, sub, "originated_beacons_total",
        "Number of beacons originated", lab.Labels())
    b.WithLabelValues(lab.Values()...).Inc()

Optimally we would use label values that will be used afterwards. Otherwise maybe we should define some default placeholders?

doc/Metrics.md, line 148 at r1 (raw file):

    r := prom.NewCounter(ns, sub, "originator_run_duration_seconds_total",
        "Originator total time spent on every periodic run")
    r.Inc()

That should not be needed.

go/beacon_srv/internal/metrics/originator.go, line 57 at r1 (raw file):

	b := prom.NewCounterVec(ns, sub, "originated_beacons_total",
		"Number of beacons originated", lab.Labels())
	b.WithLabelValues(lab.Values()...).Inc()

without Inc

go/beacon_srv/internal/metrics/originator.go, line 61 at r1 (raw file):

	r := prom.NewCounter(ns, sub, "originator_run_duration_seconds_total",
		"Originator total time spent on every periodic run")
	r.Inc()

not needed

go/beacon_srv/internal/metrics/revocation.go, line 47 at r1 (raw file):

	i := prom.NewCounterVec(ns, sub, "received_revocations_total",
		"Total number of received revocation msgs.", lab.Labels())
	i.WithLabelValues(lab.Values()...).Inc()

Without Inc

Reviewable status: 0 of 14 files reviewed, 8 unresolved discussions (waiting on @lukedirtwalker)

doc/Metrics.md, line 139 at r1 (raw file):

there is times where the ns,sub is coming from the arguments. In that case the code base looks more uniform overall.
But no strong reason.

doc/Metrics.md, line 144 at r1 (raw file):

Done.

doc/Metrics.md, line 144 at r1 (raw file):

Done.

doc/Metrics.md, line 148 at r1 (raw file):

Done.

go/beacon_srv/internal/metrics/originator.go, line 57 at r1 (raw file):

Done.

go/beacon_srv/internal/metrics/originator.go, line 61 at r1 (raw file):

Done.

go/beacon_srv/internal/metrics/revocation.go, line 47 at r1 (raw file):

Done.

go/lib/prom/prom.go, line 134 at r2 (raw file):

	}
	c := prometheus.NewCounterVec(opts, labelNames)
	SafeRegister(c)

I am not sure if this safe.
In theory nobody should depend on the fact that prometheus will panic if duplicate metric in a normal workflow.

Reviewed 14 of 14 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @karampok)

doc/Metrics.md, line 139 at r3 (raw file):

func newOriginator() originator {
    sub := "originator"

Subsystem is beaconing in the code?

doc/Metrics.md, line 310 at r3 (raw file):

nitialize always

Always initialize ...

go/beacon_srv/internal/metrics/beaconing.go, line 57 at r3 (raw file):

	return beaconing{
		receivedBeacons: prom.NewCounterVecWithLabels(ns, sub, "received_beacons_total",
			"Total number of received beacons.", BeaconingLabels{}),

Here you could simply use the old version? Or is the plan to migrate everything towards VecWithLabels?

go/lib/prom/prom.go, line 134 at r2 (raw file):

I think you need to use the return value of SafeRegister otherwise you might return a countervec that isn't registered.

go/lib/prom/prom.go, line 140 at r3 (raw file):

	}
	c := prometheus.NewCounterVec(opts, label.Labels())
	SafeRegister(c)

c := SafeRegister(c).(*prometheus.CounterVec)

Reviewable status: 12 of 14 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker)

doc/Metrics.md, line 139 at r3 (raw file):

Done.

doc/Metrics.md, line 310 at r3 (raw file):

Done.

go/beacon_srv/internal/metrics/beaconing.go, line 57 at r3 (raw file):

The VecWithLabels will enable to insert default label values that will create in prometheus a timestamp that later will be re-used. So yes, my plan would be to change the components that can make use of that.

What do you think? does it make sense? Does that make sense to try to replace all with VecWithLabels

I was thinking to create an issue replace all with VecWithLabels and add a comment in the old function not to be re-used again in a new code.

go/lib/prom/prom.go, line 140 at r3 (raw file):

Done.

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @karampok)

go/beacon_srv/internal/metrics/beaconing.go, line 57 at r3 (raw file):

I think we should replace all, and we could do it as part of this PR.

go/lib/prom/prom.go, line 142 at r4 (raw file):

	ret, ok := SafeRegister(c).(*prometheus.CounterVec)
	if !ok {
		panic("Should never happen")

Isn't that the default behavior if you don't have the , ok ? I would just leave it away, i.e. really just use:
c = SafeRegister(c).(*prometheus.CounterVec)

Reviewed 16 of 16 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karampok)

Reviewable status: complete! all files reviewed, all discussions resolved