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

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

	return beaconing{
		in: *prom.NewCounterVec(Namespace, sub, "receive_beacons_total",

received

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

go/beacon_srv/internal/beacon/store.go, line 230 at r1 (raw file):

// Beacon that do not match policy are not inserted and do not cause an error.
func (s *baseStore) InsertBeacon(ctx context.Context, beacon Beacon) (InsertStats, error) {
	tx, err := s.db.BeginTransaction(ctx, nil)

since you decided to move to one beacon at a time, there is no need for db transaction anymore.

go/beacon_srv/internal/beaconing/handler.go, line 85 at r1 (raw file):

	labels := metrics.BeaconingLabels{}
	if peer, ok := h.request.Peer.(*snet.Addr); ok {

you cannot do that. The peer might be lying about its IA.

I suggest you restructure the code a bit:

1. modify getIFID() (common.IFIDType, addr.IA, error)
2. call getIFID here
3. pass ifid to buildBeacon
4. profit

go/beacon_srv/internal/beaconing/handler.go, line 91 at r1 (raw file):

	if err != nil {
		labels.Result = metrics.ErrProcess
		metrics.Beaconing.Receives(labels).Inc()

What do you think about the labels.WithResult(..) pattern?
It clutters code a lot less.

go/beacon_srv/internal/beaconing/handler.go, line 95 at r1 (raw file):

	}
	logger.Trace("[BeaconHandler] Received", "beacon", b)
	labels.InIfID = b.InIfId

the horrors of our inconsistent naming still haunt us. :)
👍 for using the correct casing. (we should update InIfID at some point in a different PR)

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

const (
	// BcnNew indicated beacon inserted for the first time.
	BcnNew = "ok_new"

Bcn looks pretty weird.
I would go for:

• OkNew
• OkUpdated (I'm in favor of replacing ok_renew with ok_updated, but ultimately @kormat has to decide that.)
• OkFiltered

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

	return beaconing{
		in: *prom.NewCounterVec(Namespace, sub, "receive_beacons_total",

"received_beacons_total"

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

}

func (e *beaconing) Receives(l BeaconingLabels) prometheus.Counter {

Received

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

// GetResultValue return result label value given insert stats.
func GetResultValue(s beacon.InsertStats) string {

oh, no, You don't want to do that.
When we add metrics to beacon.Store this creates a beautiful import cycle.
Take update, inserted int as arguments

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

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

Oh, dedupe

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

a discussion (no related file):
"received_" instead of "receive_" in metric names.

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

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

I agree with ok_updated.

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

go/beacon_srv/internal/beacon/store.go, line 230 at r1 (raw file):

For me looks it looks better approach to have a insertBeacon instead of insertBeacons given we do not use the latter functionality. Once we need it, then it has to be refactored according to the future requirements (e.g. maybe array instead of variadic args etc.)

go/beacon_srv/internal/beaconing/handler.go, line 85 at r1 (raw file):

I did something different, what do you think?

go/beacon_srv/internal/beaconing/handler.go, line 91 at r1 (raw file):

How about this version? As it is know there is no need for many return errors.
If we do want many errors then we can change

go/beacon_srv/internal/beaconing/handler.go, line 95 at r1 (raw file):

yes we should!

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

Done.

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

Done.

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

Done.

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

Done.

Reviewed 1 of 5 files at r2.
Reviewable status: 7 of 11 files reviewed, 8 unresolved discussions (waiting on @karampok and @oncilla)

go/beacon_srv/internal/beaconstorage/store.go, line 44 at r2 (raw file):

	SegmentsToRegister(ctx context.Context, segType proto.PathSegType) (
		<-chan beacon.BeaconOrErr, error)
	// InsertBeacon adds verified beacon to the store. Beacon that

The grammar took a hit, and the wording could be clearer anyway:

// InsertBeacon adds a verified beacon to the store, ignoring revocations.

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

OkUpdated = "ok_renew"

You need to change the value as well.

go/beacon_srv/internal/metrics/beaconing.go, line 30 at r2 (raw file):

	// OkUpdated indicates existing beacon in db was updated.
	OkUpdated = "ok_renew"
	// OkFiltered indicates bnacon was filtered.

"indicates beacon was filtered by policy".

Reviewed 2 of 5 files at r2.
Reviewable status: 9 of 11 files reviewed, 7 unresolved discussions (waiting on @karampok and @oncilla)

go/beacon_srv/internal/beacon/store.go, line 230 at r1 (raw file):

sure, sgtm

go/beacon_srv/internal/beaconing/handler.go, line 85 at r1 (raw file):

That does not work.
You still use the peer.IA, which can easily lead to a state exhaustion attack.

I.e. the attacker sends a lot of bogus beacons iterating all possible IA numbers.
The metrics will start a time series for every used IA - > bad time.

go/beacon_srv/internal/beaconing/handler.go, line 91 at r1 (raw file):

It is less flexible, and causes more code.
I would stick to labels.WithResult

go/beacon_srv/internal/beaconing/handler.go, line 98 at r2 (raw file):

	if err := h.inserter.PreFilter(b); err != nil {
		logger.Trace("[BeaconHandler] Beacon pre-filtered", "err", err)
		metrics.Beaconing.ReceivedWithError(labels).Inc()

now you changed ErrPrefiltered to ErrProcessed

go/beacon_srv/internal/beaconing/handler.go, line 105 at r2 (raw file):

	if err != nil {
		logger.Trace("[BeaconHandler] Beacon validate as entry", "err", err)
		metrics.Beaconing.ReceivedWithError(labels).Inc()

now you changed the error from ErrVerify to ErrProcessed

go/beacon_srv/internal/beaconing/handler.go, line 189 at r2 (raw file):

		}
	}
	return ret, nil

you never set ret?

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

go/beacon_srv/internal/beaconing/handler.go, line 85 at r1 (raw file):

This part of the code was left by mistake, something was not committed/pushed.
Sorry for that

go/beacon_srv/internal/beaconing/handler.go, line 91 at r1 (raw file):

Done.

go/beacon_srv/internal/beaconing/handler.go, line 98 at r2 (raw file):

I have added more values for the errors.

go/beacon_srv/internal/beaconing/handler.go, line 105 at r2 (raw file):

Done.

go/beacon_srv/internal/beaconing/handler.go, line 189 at r2 (raw file):

Done. Is this an accurate way to get the AS? I am using the intID, what is inside beacon in the path segments and the topology?

go/beacon_srv/internal/beaconstorage/store.go, line 44 at r2 (raw file):

Done.

go/beacon_srv/internal/metrics/beaconing.go, line 30 at r2 (raw file):

Done.

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

	ErrValidate = "err_validate"

	// OkNew indicates beacon inserted for the first time.

I am not sure where is the ideal location for these variables

• inside the metric/metric.go
• inside the metric/beaconing.go
• or mix (e.g. OkNew in beaconing.go, ErrDB in metric.go)

I picked the first.

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

go/beacon_srv/internal/beacon/store.go, line 227 at r3 (raw file):

// InsertBeacon adds a verified beacon to the store.
// Beacon that contain revoked interfaces are inserted and do not cause an error.

This sentence needs to be rephrased:

A beacon that contains revoked interfaces is inserted and does not cause an error.
If the beacon does not match any policy, it is not inserted, but does not cause an error.

go/beacon_srv/internal/beaconing/handler.go, line 189 at r2 (raw file):

no, what you have to do is: intf.TopoInfo().ISD_AS.

See my comment above.

go/beacon_srv/internal/beaconing/handler.go, line 94 at r3 (raw file):

	if err := h.inserter.PreFilter(b); err != nil {
		logger.Trace("[BeaconHandler] Beacon pre-filtered", "err", err)
		metrics.Beaconing.Received(labels.WithResult(metrics.ErrPrefilter)).Inc()

here, neigh_as is still empty. This means, in the metrics we will see neigh_as unset and set, which looks odd.

received_beacons_total(in_if_id="1", neigh_as="", result="err_prefilter")
received_beacons_total(in_if_id="1", neigh_as="1-ff00:0:111", result="ok_new")

I suggest you do something like:
https://gist.github.com/Oncilla/67ff00d269302189c15362a93b041004

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

I think this is the correct location.

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

go/beacon_srv/internal/beacon/store.go, line 227 at r3 (raw file):

Done.

go/beacon_srv/internal/beaconing/handler.go, line 94 at r3 (raw file):

Done.

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

Reviewed 1 of 5 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kormat)

Reviewed 5 of 5 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kormat)

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

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

go/beacon_srv/internal/metrics/beaconing.go, line 69 at r6 (raw file):

// GetResultValue return result label value given insert stats.
func GetResultValue(ins, upd, flt int) string {
	switch {

This seems a bit overly complex. As this approach can't represent any mix of activities(*), why not just:

switch {
  case flt > 0:  return OkFiltered
  case upd > 0: return OkUpdated
  case ins > 0: return OkNew
  default: return OkOld
}

• If later you want to support a mix of results, then you'll need to iterate over the struct members and potentially increment a different counter for each, so this would need to be rewritten.

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

go/beacon_srv/internal/metrics/beaconing.go, line 69 at r6 (raw file):

We can go with that way since it is more readable.

In all cases, it seems that the insertStats struct will have be extended to include a method that will return the desire-generic action.

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

go/beacon_srv/internal/metrics/beaconing.go, line 72 at r7 (raw file):

	case upd > 0:
		return OkUpdated
	case flt > 0:

I'd put this as the first case, out of paranoia.

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @kormat)

go/beacon_srv/internal/metrics/beaconing.go, line 72 at r7 (raw file):

Done.

Reviewed 1 of 1 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved