Reviewed 2 of 11 files at r1.
Reviewable status: 2 of 11 files reviewed, 2 unresolved discussions (waiting on @karampok and @lukedirtwalker)

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

import (
	"github.com/scionproto/scion/go/lib/infra"

This is begging for an import cycle - go/lib/infra imports a lot of libraries.

Reviewable status: 2 of 11 files reviewed, 3 unresolved discussions (waiting on @karampok and @lukedirtwalker)

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

// RevocationLabels define the labels attached to revocation metrics.
type RevocationLabels struct {

I'm struggling to understand these labels. For locally issued revocations, IfID makes sense, but Dst does not. For received revocations, neither make sense (you'd want a SrcIA label instead).

Reviewable status: 2 of 11 files reviewed, 3 unresolved discussions (waiting on @karampok and @lukedirtwalker)

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

For receiving revocations, the label should also mention the transmission type (e.g. ctrl_pld, or scmp).

Reviewable status: 2 of 14 files reviewed, 5 unresolved discussions (waiting on @karampok and @lukedirtwalker)

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

	sub := "revocation"
	labels := RevocationLabels{}.Labels()

Need a metric for issuing a revocation. It should have a label to say whether it's a fresh revocation, or an update of an existing one.

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

	return exporterR{
		out: *prom.NewCounterVec(Namespace, sub, "transmit_rev_total",

This is very vague. The BS has 3 different ways of sending revocations:

• to local PS
• to all local BRs
• to the local ISD core (when/if #3168 is implemented)

There should be a label to distinguish between these cases.

Reviewable status: 1 of 19 files reviewed, 6 unresolved discussions (waiting on @karampok, @kormat, and @lukedirtwalker)

a discussion (no related file):
@karampok : my mental model of how the BS sends revocations was wrong, so i would instead change the metric to be:

bs_ifstate_sent_revocations{dst="core|ps|br"}

Reviewable status: 1 of 18 files reviewed, 6 unresolved discussions (waiting on @kormat and @oncilla)

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

done

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

Done.

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

Done.

Reviewed 3 of 11 files at r1, 6 of 13 files at r2, 6 of 9 files at r3.
Reviewable status: 16 of 18 files reviewed, 19 unresolved discussions (waiting on @karampok, @kormat, and @oncilla)

go/beacon_srv/internal/ifstate/metrics.go, line 47 at r3 (raw file):

			State:   string(intf.State()),
		}
		mc <- metrics.Ifstate.IfstateMetric(l, up)

this indirection seems a bit artificial to me. Why not use prometheus.MustNewConstMetric instead?

go/beacon_srv/internal/ifstate/revoker.go, line 91 at r3 (raw file):

			IfID:    ifid,
			NeighAS: intf.TopoInfo().ISD_AS,
			State:   metrics.RevNew,

State: metrics.RevRenew

go/beacon_srv/internal/ifstate/revoker.go, line 100 at r3 (raw file):

		if intf.Expire() && !r.hasValidRevocation(intf) {
			if intf.State() == Revoked {
				labelsIssued.State = metrics.RevRenew

move with next if statement
and assign metrics.RevNew instead

go/beacon_srv/internal/ifstate/revoker.go, line 112 at r3 (raw file):

			}
			if err := intf.Revoke(srev); err != nil {
				logger.Error("[ifstate.Revoker] Failed to revoke!", "ifid", ifid, "err", err)

I'm surprised that there is no label/counter for this.

go/beacon_srv/internal/ifstate/revoker.go, line 124 at r3 (raw file):

	if len(revs) > 0 {
		wg := &sync.WaitGroup{}
		l := metrics.RevocationLabels{Result: metrics.Success}

not used?

go/beacon_srv/internal/metrics/ifstate.go, line 24 at r3 (raw file):

)

type exporterI struct {

to me exporterI is a lot less obvious then ifstate.
But it is private so 🤷‍♂️

go/beacon_srv/internal/metrics/ifstate.go, line 40 at r3 (raw file):

		issued: *prom.NewCounterVec(Namespace, sub, "issued_revocations_total",
			"Total number of issued revocations.", IssuedLabels{}.Labels()),
		duration: *prom.NewCounterVec(Namespace, sub, "revocations_duration_seconds",

seconds_total

go/beacon_srv/internal/metrics/ifstate.go, line 65 at r3 (raw file):

// Transmit return the duration counter
func (e *exporterI) Transmit(l SentLabels) prometheus.Counter {

keep it consistent with sent

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

const (
	// DstBR indicates the destination to be Border Router.

group const blocks by purpose

e.g. namespace should be its own block
destinations should be their own block
etc.

This makes for much nicer godoc.

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

	// RevRenew indicates a renew of an already issued revocation.
	RevRenew string = "renew"
	// RevFromCTRL indicates that revocation was sent by ctrl.

"sent in a control payload."

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

	RevRenew string = "renew"
	// RevFromCTRL indicates that revocation was sent by ctrl.
	RevFromCTRL string = "ctrl"

Ctrl

go/beacon_srv/internal/revocation/handler.go, line 88 at r1 (raw file):

tbh, SendAckHelper should expose the error.

If code does not care, it can just ignore the return value.
If it does care, it can use it.

go/beacon_srv/internal/revocation/metrics.go, line 2 at r3 (raw file):

// Copyright 2019 Anapaya Systems
//

Why does this exist?

go/beacon_srv/internal/revocation/metrics.go, line 24 at r3 (raw file):

const (
	// Success indicates a successful result.
	Success string = prom.Success

drop this. Client code should just use metrics.{Success,ErrProcess} instead.

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

go/beacon_srv/internal/ifstate/metrics.go, line 47 at r3 (raw file):

because I do not want to export the .Values() function. I know that we already export it but I want to keep it possible to make it private.

go/beacon_srv/internal/ifstate/revoker.go, line 91 at r3 (raw file):

Done

go/beacon_srv/internal/ifstate/revoker.go, line 100 at r3 (raw file):

Done

go/beacon_srv/internal/ifstate/revoker.go, line 112 at r3 (raw file):

What do you mean? that there should be a metric with error result?

go/beacon_srv/internal/ifstate/revoker.go, line 124 at r3 (raw file):

Done.

go/beacon_srv/internal/metrics/ifstate.go, line 24 at r3 (raw file):

Done.

go/beacon_srv/internal/metrics/ifstate.go, line 40 at r3 (raw file):

Done.

go/beacon_srv/internal/metrics/ifstate.go, line 65 at r3 (raw file):

Done.

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

do we want to be sorted also?

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

Done.

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

all strings in prom.* are lowercase

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

Done.

go/beacon_srv/internal/revocation/handler.go, line 88 at r1 (raw file):

it is not needed for the metrics in that PR

go/beacon_srv/internal/revocation/metrics.go, line 2 at r3 (raw file):

So this file existed in revocation/metrics/metrics.go link and the import of metrics was a conflict.
Since I am not feeling comfortable to refactor this part, i just place it here and the call now does no use the metrics package

go/beacon_srv/internal/revocation/metrics.go, line 24 at r3 (raw file):

other part of my changes

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

go/beacon_srv/internal/metrics/ifstate.go, line 88 at r3 (raw file):

	IfID    common.IFIDType
	NeighAS addr.IA
	State   string

I don't think we want to expose this, it's going away when #2926 gets implemented. I saw that i had accidentally left the state label in #3104, so i've updated it to remove it.

Reviewable status: 14 of 18 files reviewed, 16 unresolved discussions (waiting on @karampok and @oncilla)

go/beacon_srv/internal/ifstate/metrics.go, line 23 at r4 (raw file):

var _ prometheus.Collector = (*Collector)(nil)

Why remove this?

Reviewable status: 13 of 18 files reviewed, 16 unresolved discussions (waiting on @kormat and @oncilla)

go/beacon_srv/internal/ifstate/metrics.go, line 47 at r3 (raw file):

Done. unless you have strong preference.

go/beacon_srv/internal/ifstate/metrics.go, line 23 at r4 (raw file):

It does not seem that we need that. Is there a specific reason we had that in the first moment?

go/beacon_srv/internal/metrics/ifstate.go, line 88 at r3 (raw file):

Done.

Reviewable status: 13 of 18 files reviewed, 15 unresolved discussions (waiting on @karampok and @oncilla)

go/beacon_srv/internal/ifstate/metrics.go, line 23 at r4 (raw file):

Yes. It ensures at compile time that the type does implement what the docstring claims.

(On a side note - if you don't know why something is there, it would be better to ask than to remove it silently)

Reviewable status: 13 of 18 files reviewed, 15 unresolved discussions (waiting on @kormat and @oncilla)

go/beacon_srv/internal/ifstate/metrics.go, line 23 at r4 (raw file):

me removing this == me asking ;)

To my understanding, when compile time (aka go test or go build) the compiler will complain if the type does not implement the interface that should. This is a standard golang approach. In specific according to https://golang.org/doc/effective_go.html#blank_implements we don't need

 By convention, such declarations are only used when there are no static conversions already present in the code, which is a rare event.

Reviewed 1 of 9 files at r3, 2 of 3 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @karampok and @kormat)

go/beacon_srv/internal/ifstate/metrics.go, line 23 at r4 (raw file):

I agree with @karampok on this.

Either code compiles, and all is fine.
Or code does not compile.

The check is superfluous.

go/beacon_srv/internal/ifstate/revoker.go, line 112 at r3 (raw file):

🤷‍♂️
just expressing my surprise 😅

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

meh, no strong opinion

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

No, I mean s/CTRL/Ctrl

go/beacon_srv/internal/revocation/handler.go, line 88 at r1 (raw file):

Sure.

go/beacon_srv/internal/revocation/metrics.go, line 2 at r3 (raw file):

ack.

go/beacon_srv/internal/revocation/metrics.go, line 24 at r3 (raw file):

What does that mean?

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

go/beacon_srv/internal/ifstate/metrics.go, line 23 at r4 (raw file):

This library will compile fine without it; compilation would fail on anywhere that used it as a prometheus.Collector. If there is no such code at a given point in time (or it's commented out, etc), then we won't notice any issues until potentially much later.

We use this pattern a lot:

$ git grep 'var _' -- go/ | wc -l
271

It makes it explicit what interfaces a type implements, so it acts as documentation as well as safety.

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

go/beacon_srv/internal/ifstate/metrics.go, line 23 at r4 (raw file):

Without it, if I change the type not to implement the interface it fails, so we will notice any issue instantly.
What I agree with you is that this PR does not need to have this change.

In order to move forward, I suggest that we defer the conversation elsewhere so to include the opinion of the rest of the team.
And if we decide to remove it, then to do it in a specific PR.

Reviewed 3 of 11 files at r1, 2 of 13 files at r2, 1 of 9 files at r3, 1 of 1 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karampok)

go/lib/prom/promtest/promtest.go, line 28 at r7 (raw file):

// CheckLabelsStruct checks that labels has a Values and Labels method. It also
// checks that labels.Labels() returns the struct field names.
func CheckLabelsStruct(t *testing.T, xLabels interface{}) {

This change isn't mentioned in the PR description.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @karampok)

go/beacon_srv/internal/revocation/metrics.go, line 24 at r7 (raw file):

var (
	errBeaconStore   = &infra.HandlerResult{Result: "err_beaconstore", Status: prom.StatusErr}
	errBeaconStoreTo = &infra.HandlerResult{

I'd use Tout as a short-hand for timeout.

Reviewable status: 17 of 18 files reviewed, 2 unresolved discussions (waiting on @kormat)

go/beacon_srv/internal/revocation/metrics.go, line 24 at r7 (raw file):

Done but
this file already existed in revocation/metrics/metrics.go link and the import of metrics was a conflict.
Since I am not feeling comfortable to refactor this part, i just placed in a new location and the call now does not use the metrics package. I am sure it will be refactored once I touch this part of metrics

go/lib/prom/promtest/promtest.go, line 28 at r7 (raw file):

Done.

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

a discussion (no related file):
PR description needs an update (the metrics are the old versions). I'd suggest using this as the bottom part of the description:

Note: the BS does not currently support revocations received over SCMP.

Also:

• Improve the promtest check function

There's no need to mention not sending revocations to the core as that isn't settled yet.

go/beacon_srv/internal/revocation/handler.go, line 41 at r7 (raw file):

	timeout  time.Duration
}

Add a FIXME to add support for revocations received over SCMP.

go/beacon_srv/internal/revocation/metrics.go, line 24 at r7 (raw file):

My comment was specific to this line - errBeaconStoreTo where To apparently stands for "timeout". I wasn't talking about renaming err_beaconstore_timeout.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @karampok and @kormat)

go/beacon_srv/internal/revocation/handler.go, line 41 at r7 (raw file):

reference #3166 in the fixme

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

a discussion (no related file):

Done.

go/beacon_srv/internal/revocation/handler.go, line 41 at r7 (raw file):

Done.

go/beacon_srv/internal/revocation/metrics.go, line 24 at r7 (raw file):

is not yet "apparently" to me, done

Reviewable status: 15 of 18 files reviewed, 2 unresolved discussions (waiting on @karampok, @kormat, and @oncilla)

a discussion (no related file):

If you're using , you don't need to indent the block - as it is github is displaying the as literals.

If you use - instead of *, github will format the "Also" list correctly.

go/beacon_srv/internal/revocation/handler.go, line 41 at r7 (raw file):

Use full urls for referenced issues.

Reviewable status: 15 of 18 files reviewed, 2 unresolved discussions (waiting on @karampok, @kormat, and @oncilla)

a discussion (no related file):

Ah, that got scrambled. If you're using ``` you don't need to indent the block.

Reviewed 1 of 3 files at r9.
Reviewable status: 16 of 18 files reviewed, 1 unresolved discussion (waiting on @kormat and @oncilla)

Reviewed 2 of 3 files at r9.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kormat)

Dismissed @kormat from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved