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

go/beacon_srv/internal/beaconing/propagator.go, line 261 at r1 (raw file):

		}
		defer func() {
			metrics.Beaconing.PropagatorIntfTime(labels).Add(time.Since(time.Now()).Seconds())

that does not work
before it worked because the arguments are evaluated here and the counter was increased after the method has finished.

go/beacon_srv/internal/beaconing/propagator.go, line 290 at r1 (raw file):

			p.logger.Error("[beaconing.Propagator] Unable to send packet",
				"egIfid", egIfid, "err", err)
			metrics.Beaconing.PropagatorTotalBeacons(labels).Inc()

this should be ErrSend

go/beacon_srv/internal/metrics/beaconing.go, line 97 at r1 (raw file):

}

func (e *beaconing) PropagatorTotalRunTime() prometheus.Counter {

hmm, having a lot of Propagator* smells like you want to split that one out into a new struct.

soon you will have {Propagator,Originator,Registrar}* for a most of the counters.

Reviewable status: 3 of 8 files reviewed, 3 unresolved discussions (waiting on @kormat and @oncilla)

go/beacon_srv/internal/beaconing/propagator.go, line 261 at r1 (raw file):

Done.

go/beacon_srv/internal/beaconing/propagator.go, line 290 at r1 (raw file):

Done.

go/beacon_srv/internal/metrics/beaconing.go, line 97 at r1 (raw file):

I was not able to find a common name pattern, so split files is done

Reviewed 5 of 5 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kormat)

Reviewed 10 of 11 files at r3.
Reviewable status: 13 of 14 files reviewed, 10 unresolved discussions (waiting on @karampok and @kormat)

go/beacon_srv/internal/beaconing/originator.go, line 161 at r3 (raw file):

	intf := o.cfg.Intfs.Get(o.ifID)
	if intf == nil {
		l := metrics.OriginatorLabels{EgIfID: 0, Result: metrics.ErrCreate}

This should probably be it's own error result

Here you can use EgIfID because it comes from the BS configuration, and not some outside input.

go/beacon_srv/internal/beaconing/originator.go, line 162 at r3 (raw file):

	if intf == nil {
		l := metrics.OriginatorLabels{EgIfID: 0, Result: metrics.ErrCreate}
		metrics.Originator.Beacons(l)

.Inc()?

go/beacon_srv/internal/beaconing/originator.go, line 171 at r3 (raw file):

	if err != nil {
		l := metrics.OriginatorLabels{EgIfID: o.ifID, Result: metrics.ErrCreate}
		metrics.Originator.Beacons(l)

ditto

go/beacon_srv/internal/beaconing/originator.go, line 185 at r3 (raw file):

	if err != nil {
		l := metrics.OriginatorLabels{EgIfID: o.ifID, Result: metrics.ErrSend}
		metrics.Originator.Beacons(l)

ditto

go/beacon_srv/internal/beaconing/originator.go, line 190 at r3 (raw file):

	o.onSuccess(intf)
	l := metrics.OriginatorLabels{EgIfID: o.ifID, Result: metrics.Success}
	metrics.Originator.Beacons(l)

ditto

go/beacon_srv/internal/metrics/originator.go, line 41 at r3 (raw file):

type originator struct {
	originatedBeacons prometheus.CounterVec

should be *prometheus.CounterVec

go/beacon_srv/internal/metrics/originator.go, line 48 at r3 (raw file):

	ns, sub := Namespace, "beaconing"
	return originator{
		originatedBeacons: *prom.NewCounterVec(ns, sub, "originated_beacons_total",

do not dereference pointer here

go/beacon_srv/internal/metrics/originator.go, line 50 at r3 (raw file):

		originatedBeacons: *prom.NewCounterVec(ns, sub, "originated_beacons_total",
			"Number of beacons originated", OriginatorLabels{}.Labels()),
		runtime: prom.NewCounter(ns, sub, "originator_run_durations_seconds_total",

s/durations/duration

go/beacon_srv/internal/metrics/registrar.go, line 77 at r3 (raw file):

func (e *registrar) Beacons(l RegistrarLabels) prometheus.Counter {
	return e.registeredBeacons.WithLabelValues(l.Values()...)
}

missing newline

go/beacon_srv/internal/metrics/registrar.go, line 81 at r3 (raw file):

	l := TypeOnlyLabel{SegType: s}
	return e.runtime.WithLabelValues(l.Values()...)
}

ditto

Reviewed 1 of 11 files at r3.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @karampok and @kormat)

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

func (e *originator) Runtime() prometheus.Counter {
	return e.runtime
}

missing new line

Reviewable status: 11 of 14 files reviewed, 11 unresolved discussions (waiting on @kormat and @oncilla)

go/beacon_srv/internal/beaconing/originator.go, line 161 at r3 (raw file):

Done.

go/beacon_srv/internal/beaconing/originator.go, line 162 at r3 (raw file):

Done.

go/beacon_srv/internal/beaconing/originator.go, line 171 at r3 (raw file):

Done.

go/beacon_srv/internal/beaconing/originator.go, line 185 at r3 (raw file):

Done.

go/beacon_srv/internal/beaconing/originator.go, line 190 at r3 (raw file):

Done.

go/beacon_srv/internal/metrics/originator.go, line 41 at r3 (raw file):

why, is not everything the same?
https://github.com/scionproto/scion/blob/master/go/beacon_srv/internal/beaconing/metrics/metrics.go#L178

or
https://github.com/scionproto/scion/blob/master/go/beacon_srv/internal/metrics/revocation.go#L38

go/beacon_srv/internal/metrics/originator.go, line 48 at r3 (raw file):

same as above

go/beacon_srv/internal/metrics/originator.go, line 50 at r3 (raw file):

Done.

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

Done.

go/beacon_srv/internal/metrics/registrar.go, line 77 at r3 (raw file):

Done.

go/beacon_srv/internal/metrics/registrar.go, line 81 at r3 (raw file):

Done.

Reviewable status: 9 of 14 files reviewed, 11 unresolved discussions (waiting on @kormat and @oncilla)

go/beacon_srv/internal/metrics/originator.go, line 41 at r3 (raw file):

Done.

go/beacon_srv/internal/metrics/originator.go, line 48 at r3 (raw file):

Done.

Reviewed 6 of 6 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karampok and @kormat)

go/beacon_srv/internal/beaconing/originator.go, line 159 at r4 (raw file):

// originateBeacon originates a beacon on the given ifid.
func (o *beaconOriginator) originateBeacon(ctx context.Context) error {
	labels := metrics.OriginatorLabels{EgIfID: o.ifID, Result: metrics.ErrVerify}

should be metrics.Success, no?

go/beacon_srv/internal/beaconing/propagator.go, line 268 at r4 (raw file):

				"beacon", bseg, "err", err)
			labels.Result = metrics.ErrCreate
			metrics.Propagator.Beacons(labels).Inc()

I like WithResults more, but this is fine too.

go/beacon_srv/internal/metrics/originator.go, line 41 at r3 (raw file):

because of past mistakes :sad:

Reviewable status: 13 of 14 files reviewed, 1 unresolved discussion (waiting on @kormat and @oncilla)

go/beacon_srv/internal/beaconing/originator.go, line 159 at r4 (raw file):

Done.

go/beacon_srv/internal/beaconing/propagator.go, line 268 at r4 (raw file):

The label is being used in the defer. So I need to modify that field.

go/beacon_srv/internal/metrics/originator.go, line 41 at r3 (raw file):

it's okay, you have saved many mistakes of me ;)

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

go/beacon_srv/internal/beaconing/propagator.go, line 268 at r4 (raw file):

ah, I didn't realize.

Maybe add a comment inside the deferred function:

// This captures the labels variable such that it can be modified in the code below.

Reviewable status: 13 of 14 files reviewed, 1 unresolved discussion (waiting on @karampok, @kormat, and @oncilla)

go/beacon_srv/internal/beaconing/propagator.go, line 268 at r4 (raw file):

roar as loud as you can 🦁

s/bellow/below , also add period at the end.

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kormat)

Reviewed 3 of 3 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kormat)

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kormat)