segfetcher: Add segreq & revocation metrics #3264

lukedirtwalker · 2019-10-22T07:28:10Z

Add metrics for received revocations and sent segment requests.
Also changes the naming of SD and PS revocation metrics to match.

# HELP ps_fetcher_seg_requests_total The number of segment request sent.
# TYPE ps_fetcher_seg_requests_total counter
ps_fetcher_seg_requests_total{result="ok_success"} 1
# HELP ps_received_revocations_total The amount of revocations received.
# TYPE ps_received_revocations_total counter
ps_received_revocations_total{result="err_db",src="path_reply"} 0
ps_received_revocations_total{result="err_verify",src="path_reply"} 0
ps_received_revocations_total{result="ok_success",src="notification"} 1
ps_received_revocations_total{result="ok_success",src="path_reply"} 0

Contributes #3106
Fixes #3255

This change is

karampok

P ps_fetcher_seg_requests_total The number of segment request sent, grouped by result

TYPE ps_fetcher_seg_requests_total counter

ps_fetcher_seg_requests_total{result="ok_success"} 1

HELP ps_recv_revocations_total The amount of revocations received by src type and result

TYPE ps_recv_revocations_total counter

ps_recv_revocations_total{result="err_db",src="path_reply"} 0
ps_recv_revocations_total{result="err_verify",src="path_reply"} 0
ps_recv_revocations_total{result="ok_success",src="path_reply"} 0

If I would like to set up something local (scion run), how could I produce a non-zero value for the revocation?

Reviewed 13 of 13 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @lukedirtwalker and @scrye)

go/lib/infra/modules/segfetcher/fetcher.go, line 175 at r1 (raw file):

		case <-r.FullReplyProcessed():
			// sets revocation metrics
			defer func() {

for readability I would like to have seen

defer f.metrics.UpdateRevocation( x,y,z)  // single line logic hidden

and maybe

r.Stats().GetXYZ() implemented for the stats struct

go/lib/infra/modules/segfetcher/fetcher.go, line 182 at r1 (raw file):

				revErrors := 0
				for _, err := range r.VerificationErrors() {
					if xerrors.Is(err, seghandler.ErrRevVerification) {

What other errors do we have? and what do we do with them regarding metrics?

go/lib/infra/modules/segfetcher/fetcher.go, line 190 at r1 (raw file):

			if err := r.Err(); err != nil {
				f.metrics.SegRequests(metrics.ErrProcess).Inc()

extra empty line

go/lib/infra/modules/segfetcher/fetcher.go, line 216 at r1 (raw file):

		}
	}
	return reqSet, nil

I would have expected to see the metrics.OkSuccess above that return

go/lib/infra/modules/segfetcher/internal/metrics/fetcher.go, line 39 at r1 (raw file):

func NewFetcher(namespace string) Fetcher {
	subst := "fetcher"
	requests := prom.SafeRegister(prometheus.NewCounterVec(prometheus.CounterOpts{

Could we use the prom.NewCounterWithLabel() function?

go/lib/infra/modules/segfetcher/internal/metrics/fetcher.go, line 44 at r1 (raw file):

		Name:      "seg_requests_total",
		Help:      "The number of segment request sent, grouped by result",
	}, []string{prom.LabelResult})).(*prometheus.CounterVec)

and ideally we need label struct here instead of strings (same pattern, same tests etc.)

go/lib/infra/modules/segfetcher/internal/metrics/metrics.go, line 26 at r1 (raw file):

	OkSuccess = prom.Success
	// ErrNotClassified is an error that is not further classified.
	ErrNotClassified = prom.ErrNotClassified

just a note, keep them sorted and an empty line in between
(but no strong preference)

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

var (
	// ErrRevVerification indicates an error while verifying a revocation.

If not used outside the package, I will have them local.
(but no strong preference)

go/path_srv/internal/metrics/revocation.go, line 51 at r1 (raw file):

func newRevocation() revocation {
	return revocation{
		count: prom.NewCounterVecWithLabels(Namespace, "", "recv_revocations_total",

better received

go/sciond/internal/metrics/metrics.go, line 156 at r1 (raw file):

func newRevocation() Revocation {
	return Revocation{
		count: prom.NewCounterVecWithLabels(Namespace, "", "recv_revocations_total",

received

go/sciond/internal/metrics/metrics.go, line 157 at r1 (raw file):

	return Revocation{
		count: prom.NewCounterVecWithLabels(Namespace, "", "recv_revocations_total",
			"The amount of revocations received by src type and result", RevocationLabels{}),

We usually do not describe the labels on the help messages
(but no strong preference)

Add metrics for received revocations and sent segment requests. Also changes the naming of SD and PS revocation metrics to match. ``` # HELP ps_fetcher_seg_requests_total The number of segment request sent, grouped by result # TYPE ps_fetcher_seg_requests_total counter ps_fetcher_seg_requests_total{result="ok_success"} 1 # HELP ps_recv_revocations_total The amount of revocations received by src type and result # TYPE ps_recv_revocations_total counter ps_recv_revocations_total{result="err_db",src="path_reply"} 0 ps_recv_revocations_total{result="err_verify",src="path_reply"} 0 ps_recv_revocations_total{result="ok_success",src="path_reply"} 0 ``` Contributes scionproto#3106

lukedirtwalker

Unrelated: Please use Start a new discussionbutton to discuss global issues in Reviewable.

You can simply stop a border router, .e.g. ./scion.sh run, ./scion.sh mstop *br*110*

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

go/lib/infra/modules/segfetcher/fetcher.go, line 175 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

for readability I would like to have seen

defer f.metrics.UpdateRevocation( x,y,z)  // single line logic hidden

and maybe

r.Stats().GetXYZ() implemented for the stats struct

Done.

go/lib/infra/modules/segfetcher/fetcher.go, line 182 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

What other errors do we have? and what do we do with them regarding metrics?

It could also be a segment verification error we don't have a metric for it :/

go/lib/infra/modules/segfetcher/fetcher.go, line 190 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

extra empty line

Done.

go/lib/infra/modules/segfetcher/fetcher.go, line 216 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

I would have expected to see the metrics.OkSuccess above that return

since we count requests, i.e. replies to requests we have to do the counting in the loop only. See six lines above.

go/lib/infra/modules/segfetcher/internal/metrics/fetcher.go, line 39 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Could we use the prom.NewCounterWithLabel() function?

Done.

go/lib/infra/modules/segfetcher/internal/metrics/fetcher.go, line 44 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

and ideally we need label struct here instead of strings (same pattern, same tests etc.)

Done.

go/lib/infra/modules/segfetcher/internal/metrics/metrics.go, line 26 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

just a note, keep them sorted and an empty line in between
(but no strong preference)

Sorted, but I don't think empty line is needed.

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

Previously, karampok (Konstantinos) wrote…

If not used outside the package, I will have them local.
(but no strong preference)

It's used in segfetcher package.

go/path_srv/internal/metrics/revocation.go, line 51 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

better received

Done.

go/sciond/internal/metrics/metrics.go, line 156 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

received

Done.

go/sciond/internal/metrics/metrics.go, line 157 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

We usually do not describe the labels on the help messages
(but no strong preference)

Done.

karampok

Reviewed 12 of 12 files at r2, 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @scrye)

go/lib/infra/modules/segfetcher/fetcher.go, line 182 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

It could also be a segment verification error we don't have a metric for it :/

I suppose resolve then

lukedirtwalker requested a review from scrye October 22, 2019 07:28

lukedirtwalker added the c/observability Metrics, logging, tracing label Oct 22, 2019

lukedirtwalker requested a review from karampok October 25, 2019 05:33

lukedirtwalker force-pushed the pubSegfetcherMetrics branch from 08bd9a6 to ecfa599 Compare October 25, 2019 08:09

karampok suggested changes Oct 25, 2019

View reviewed changes

lukedirtwalker added 3 commits October 25, 2019 14:32

gen file

02c51d7

r2

9054458

lukedirtwalker force-pushed the pubSegfetcherMetrics branch from ecfa599 to 9054458 Compare October 25, 2019 12:47

lukedirtwalker commented Oct 25, 2019

View reviewed changes

fix lint

a378b6c

karampok approved these changes Oct 25, 2019

View reviewed changes

lukedirtwalker merged commit c79d36a into scionproto:master Oct 25, 2019

lukedirtwalker deleted the pubSegfetcherMetrics branch October 25, 2019 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

segfetcher: Add segreq & revocation metrics #3264

segfetcher: Add segreq & revocation metrics #3264

lukedirtwalker commented Oct 22, 2019 •

edited

Loading

karampok left a comment

lukedirtwalker left a comment

karampok left a comment

segfetcher: Add segreq & revocation metrics #3264

segfetcher: Add segreq & revocation metrics #3264

Conversation

lukedirtwalker commented Oct 22, 2019 • edited Loading

karampok left a comment

Choose a reason for hiding this comment

TYPE ps_fetcher_seg_requests_total counter

HELP ps_recv_revocations_total The amount of revocations received by src type and result

TYPE ps_recv_revocations_total counter

lukedirtwalker left a comment

Choose a reason for hiding this comment

karampok left a comment

Choose a reason for hiding this comment

lukedirtwalker commented Oct 22, 2019 •

edited

Loading