PS: Improve segment registration metrics

[lukedirtwalker]

Also change pathdb interface to differentiate updates/inserts.
Also change seghandler to provide statistics.

Contributes #3106

---

This change is 

[oncilla]

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

---

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter.go, line 64 at r1 (raw file):

// Insert inserts a hidden path segment into the the underlying PathDB
func (rw *readWriter) Insert(ctx context.Context, seg *seg.Meta,

Is the caller not interested in the stats?

---

go/path_srv/internal/handlers/segreg.go, line 37 at r1 (raw file):

const (
	regLabelType   = "type"

I would prefer this to be in a sub-package handlers/metrics similar to the BS.

---

go/path_srv/internal/handlers/segreg.go, line 64 at r1 (raw file):

func NewSegRegHandler(args HandlerArgs) infra.Handler {
	regsTotal = prom.NewCounterVec(metrics.Namespace, "", "registrations_total",

This will blow up when we call NewSegRegHandler twice.
But we probably won't do that so 🤷‍♂️

---

go/path_srv/internal/handlers/segreg.go, line 94 at r1 (raw file):

		regLabelSrc:    "?",
		regLabelResult: regResultErrInt,
	}

you can do defer regsTotal.With(labels).Inc() instead of calling it everywhere.
This works because With just takes the reference of labels. You can still modify the entries afterwards.

---

go/path_srv/internal/handlers/segreg.go, line 166 at r1 (raw file):

// different types.
func classifySegs(segReg *path_mgmt.SegReg) string {
	segTypes := make(map[string]struct{}, 1)

use proto.PathSegType instead of string

[karampok]

---

go/lib/infra/modules/seghandler/seghandler.go, line 93 at r1 (raw file):

			verifyErrs, err := h.storeResults(ctx, verifiedUnits, hpGroupID, &result.stats)
			allVerifyErrs = append(allVerifyErrs, verifyErrs...)
			result.early <- result.stats.SegDB.Total()

Who is reading from the the result.early

[karampok]

Reviewed 17 of 17 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker)

[lukedirtwalker]

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

---

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter.go, line 64 at r1 (raw file):

Previously, Oncilla wrote…

Is the caller not interested in the stats?

Done.

---

go/lib/infra/modules/seghandler/seghandler.go, line 93 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Who is reading from the the result.early

result.early is given out to users of the lib via result.EarlyTriggerProcessed() so whoever calls that.
Currently it's not used. Sciond used that once to be able to return results to clients earlier.

---

go/path_srv/internal/handlers/segreg.go, line 37 at r1 (raw file):

Previously, Oncilla wrote…

I would prefer this to be in a sub-package handlers/metrics similar to the BS.

Done.

---

go/path_srv/internal/handlers/segreg.go, line 64 at r1 (raw file):

Previously, Oncilla wrote…

This will blow up when we call NewSegRegHandler twice.
But we probably won't do that so 🤷‍♂️

Done.

---

go/path_srv/internal/handlers/segreg.go, line 94 at r1 (raw file):

Previously, Oncilla wrote…

you can do defer regsTotal.With(labels).Inc() instead of calling it everywhere.
This works because With just takes the reference of labels. You can still modify the entries afterwards.

no in the ok case we have to handle it differently because there could theoretically multiple segments.

---

go/path_srv/internal/handlers/segreg.go, line 166 at r1 (raw file):

Previously, Oncilla wrote…

use proto.PathSegType instead of string

Done.

[oncilla]

Reviewed 13 of 13 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @karampok and @lukedirtwalker)

---

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

// CheckLabelsStruct checks that labels has a Values and Labels method. It also
// checks that Labels returns the struct field names.

labels

---

go/path_srv/internal/metrics/BUILD.bazel.orig, line 1 at r2 (raw file):

load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")

delete

---

go/path_srv/internal/metrics/metrics.go, line 35 at r2 (raw file):

	RegistrationNew       = "new"
	RegiststrationUpdated = "updated"
	ErrParse              = "err_parse"

this will be a common error

---

go/path_srv/internal/metrics/metrics.go, line 38 at r2 (raw file):

	ErrInternal           = prom.ErrInternal
	ErrCrypto             = "err_crypto"
	ErrDB                 = "err_db"

this probably too

---

go/path_srv/internal/metrics/registration.go, line 29 at r2 (raw file):

// regResults lists all possible results for registrations.
var regResults = []string{RegistrationNew, RegiststrationUpdated, ErrCrypto, ErrDB, ErrInternal}

what is this used for?
also it's lying

[lukedirtwalker]

Reviewable status: 22 of 26 files reviewed, 6 unresolved discussions (waiting on @karampok and @oncilla)

---

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

Previously, Oncilla wrote…

labels

Done.

---

go/path_srv/internal/metrics/BUILD.bazel.orig, line 1 at r2 (raw file):

Previously, Oncilla wrote…

delete

Done.

---

go/path_srv/internal/metrics/metrics.go, line 35 at r2 (raw file):

Previously, Oncilla wrote…

this will be a common error

Done.

---

go/path_srv/internal/metrics/metrics.go, line 38 at r2 (raw file):

Previously, Oncilla wrote…

this probably too

Done.

---

go/path_srv/internal/metrics/registration.go, line 29 at r2 (raw file):

Previously, Oncilla wrote…

what is this used for?
also it's lying

for the description below.

[karampok]

---

go/path_srv/internal/metrics/registration.go, line 29 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

for the description below.

I would avoid having that because once you put a new value (e.g. a const in metric.go), then you forget easily to add the on the documentation.
Is it important to be on the documentation?

[karampok]

Reviewed 1 of 13 files at r2, 4 of 5 files at r3.
Reviewable status: 25 of 26 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

---

go/lib/prom/promtest/promtest.go, line 29 at r3 (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, labels interface{}) {

If we can test using the the reflect pkg then we might be able to have a fixed implementation of the methods. Does that make sense?

[karampok]

---

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

Previously, karampok (Konstantinos) wrote…

If we can test using the the reflect pkg then we might be able to have a fixed implementation of the methods. Does that make sense?

something like that i have in mind https://github.com/fatih/structs

[lukedirtwalker]

Reviewable status: 25 of 26 files reviewed, 6 unresolved discussions (waiting on @karampok and @oncilla)

---

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

Previously, karampok (Konstantinos) wrote…

something like that i have in mind https://github.com/fatih/structs

Yeah, unfortunately we are not allowed to use reflect in non-testing code :sad-panda:

[lukedirtwalker]

Reviewable status: 25 of 26 files reviewed, 6 unresolved discussions (waiting on @karampok and @oncilla)

---

go/path_srv/internal/metrics/registration.go, line 29 at r2 (raw file):

Previously, karampok (Konstantinos) wrote…

I would avoid having that because once you put a new value (e.g. a const in metric.go), then you forget easily to add the on the documentation.
Is it important to be on the documentation?

I don't know. It was in the request: #3106 (comment)

[oncilla]

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

---

go/path_srv/internal/metrics/registration.go, line 29 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I don't know. It was in the request: #3106 (comment)

🤷‍♂️

[karampok]

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

---

go/path_srv/internal/metrics/registration.go, line 29 at r2 (raw file):

Previously, Oncilla wrote…

🤷‍♂️

if it is a requirement

[oncilla]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved

[oncilla]

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