PS: Add segsyncer metrics

[lukedirtwalker]

Adds metrics:

# HELP ps_segment_sync_pushes_total Number of pushes towards a destination
# TYPE ps_segment_sync_pushes_total counter
ps_segment_sync_pushes_total{dst="1-ff00:0:120",result="err_nopath"} 5
ps_segment_sync_pushes_total{dst="1-ff00:0:120",result="ok_success"} 139
ps_segment_sync_pushes_total{dst="1-ff00:0:130",result="err_nopath"} 1
ps_segment_sync_pushes_total{dst="1-ff00:0:130",result="ok_success"} 143
# HELP ps_segment_sync_registrations_total Number of segments registered in down segment synchronizations
# TYPE ps_segment_sync_registrations_total counter
ps_segment_sync_registrations_total{result="ok_new",src="1-ff00:0:120"} 4
ps_segment_sync_registrations_total{result="ok_new",src="1-ff00:0:130"} 6
ps_segment_sync_registrations_total{result="ok_updated",src="1-ff00:0:120"} 93
ps_segment_sync_registrations_total{result="ok_updated",src="1-ff00:0:130"} 142

Contributes #3106

---

This change is 

[karampok]

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

---

go/path_srv/internal/handlers/segsync.go, line 85 at r1 (raw file):

	}
	logSegRecs(logger, "[syncHandler]", h.request.Peer, segSync.SegRecs)

drop the empty line (since we have not empty line anywhere)

---

go/path_srv/internal/handlers/segsync.go, line 117 at r1 (raw file):

}

func (h *syncHandler) incMetrics(labels metrics.SyncRegLabels, stats seghandler.Stats) {

This does not need to be method (no use of h).
I would hide the complexity of metrics in the metric package.

func (e ..  )IncreaseCounters( l labels, update, new float64) {
ll:=labels.WithResult(metrics.RegistrationNew)
e.registration(ll).Add(new)
...
}

(suggestion not strong opinion)

---

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

	ErrNetwork            = prom.ErrNetwork
	ErrNotClassified      = prom.ErrNotClassified
	// ErrNoPath indicates no path is available to send a message.

I would like to have comments on all or no comments. What do you prefer?

---

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

	ErrNotClassified      = prom.ErrNotClassified
	// ErrNoPath indicates no path is available to send a message.
	ErrNoPath = "err_no_path"

err_nopath

---

go/path_srv/internal/metrics/sync.go, line 25 at r1 (raw file):

// SyncRegLabels contains the label values for synchronization registration
// metrics.

(nit) We have a limit on 100 characters, commend can be the same line.

---

go/path_srv/internal/metrics/sync.go, line 70 at r1 (raw file):

// Sync contains metrics for segment synchronization.
type Sync struct {

no need to export that (and comment it)

---

go/path_srv/internal/metrics/sync.go, line 71 at r1 (raw file):

// Sync contains metrics for segment synchronization.
type Sync struct {
	regsTotal   *prometheus.CounterVec

prefix total not ideal just registrations and pushes

---

go/path_srv/internal/metrics/sync.go, line 79 at r1 (raw file):

	return Sync{
		regsTotal: prom.NewCounterVec(Namespace, subsystem, "registrations_total",
			"Number of segments received in down segment synchronizations",

There is no the word registration

---

go/path_srv/internal/segsyncer/segsyncer.go, line 113 at r1 (raw file):

			"dstIA", s.dstIA, "err", err)
		s.repErrCnt++
		metrics.Syncs.Pushes(labels.WithResult(errToMetricsLabel(ctx, err))).Inc()

Metric is something super static, the ctx should not be needed.
If it is about logging, log outside the inc counter

---

go/path_srv/internal/segsyncer/segsyncer.go, line 235 at r1 (raw file):

	case serrors.IsTimeout(err):
		return metrics.ErrTimeout
	case xerrors.Is(err, errPathDB):

Somehow if feel this can be "forgot" to be updated

I would prefer every time we encounter an error to just increase counter instead of
bubbling up the error and then re-have a logic to find the metric label value

...
	res, err := s.pathDB.Get(ctx, params)
	if err != nil {
               // metrics.Syncs.Pushes(withErrordDB).inc()
		return nil, serrors.Wrap(errPathDB, err)
	}
...

Note: I find that there should be number of error labels equals to the number of function we subcall in the main logic (.Run function), and not equal to all possible errors.

---

go/path_srv/internal/segsyncer/segsyncer.go, line 244 at r1 (raw file):

		return metrics.ErrNetwork
	default:
		log.FromCtx(ctx).Debug("[segsyncer.SegSyncer] Unclassified err", "err", err)

As i probably already said, the log should be part of the main code logic.

[lukedirtwalker]

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

---

go/path_srv/internal/handlers/segsync.go, line 85 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

drop the empty line (since we have not empty line anywhere)

Done.

---

go/path_srv/internal/handlers/segsync.go, line 117 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

This does not need to be method (no use of h).
I would hide the complexity of metrics in the metric package.

func (e ..  )IncreaseCounters( l labels, update, new float64) {
ll:=labels.WithResult(metrics.RegistrationNew)
e.registration(ll).Add(new)
...
}

(suggestion not strong opinion)

Done.

---

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

Previously, karampok (Konstantinos) wrote…

I would like to have comments on all or no comments. What do you prefer?

Done.

---

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

Previously, karampok (Konstantinos) wrote…

err_nopath

Done.

---

go/path_srv/internal/metrics/sync.go, line 25 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

(nit) We have a limit on 100 characters, commend can be the same line.

for comments we often use 80 char limit. I can also change it, if you prefer.

---

go/path_srv/internal/metrics/sync.go, line 70 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

no need to export that (and comment it)

Hm isn't it weird to expose a variable which is of non-public type?

---

go/path_srv/internal/metrics/sync.go, line 71 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

prefix total not ideal just registrations and pushes

Done.

---

go/path_srv/internal/metrics/sync.go, line 79 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

There is no the word registration

Done.

---

go/path_srv/internal/segsyncer/segsyncer.go, line 113 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Metric is something super static, the ctx should not be needed.
If it is about logging, log outside the inc counter

Done.

---

go/path_srv/internal/segsyncer/segsyncer.go, line 235 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Somehow if feel this can be "forgot" to be updated

I would prefer every time we encounter an error to just increase counter instead of
bubbling up the error and then re-have a logic to find the metric label value

...
	res, err := s.pathDB.Get(ctx, params)
	if err != nil {
               // metrics.Syncs.Pushes(withErrordDB).inc()
		return nil, serrors.Wrap(errPathDB, err)
	}
...

Note: I find that there should be number of error labels equals to the number of function we subcall in the main logic (.Run function), and not equal to all possible errors.

hm ok that approach would work in the current code, but in general the code might be in a lib. Which would mean you'd have to instrument the whole library which doesn't sound nice to me.
Also I think forgetting to update metrics happens much rather if we would distribute the incrementing to all branches. If I add a new branch I don't want to think about metrics. Like this I don't have to. If I forget to add an error I will see in the metrics err_not_classified and I can check the logs what error is missing. Otherwise you would just silently not see it.

---

go/path_srv/internal/segsyncer/segsyncer.go, line 244 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

As i probably already said, the log should be part of the main code logic.

Do you propose that every caller of this method should check whether it returned ErrNotClassified and then log?

[karampok]

Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)

---

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

Previously, lukedirtwalker (Lukas Vogel) wrote…

Done.

Still you have to tell me what do you prefer :)

---

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

	// needed to be fetched from a remote server.
	OkRequestFetched = "ok_fetched"
	// ErrParse indicates

uncomplete

---

go/path_srv/internal/metrics/sync.go, line 25 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

for comments we often use 80 char limit. I can also change it, if you prefer.

I usually keep it one line if it fits on 100char, but keep it as you feel

---

go/path_srv/internal/metrics/sync.go, line 70 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Hm isn't it weird to expose a variable which is of non-public type?

Not really, if you want to expose only the methods (interface).
But there is no strong practice here.
(my approach is keep everything unexported until I have not to)

---

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

// RegistrationSuccess increments registrations with the given stats.
func (s Sync) RegistrationSuccess(l SyncRegLabels, stats seghandler.Stats) {
	s.Registrations(l.WithResult(OkRegistrationNew)).

s.registrations directly.

Dominik once asked me not to import any new dependency but have the function
to accept ints (instead of stats struct).
Up to you, I am okay with both

---

go/path_srv/internal/segsyncer/segsyncer.go, line 235 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

hm ok that approach would work in the current code, but in general the code might be in a lib. Which would mean you'd have to instrument the whole library which doesn't sound nice to me.
Also I think forgetting to update metrics happens much rather if we would distribute the incrementing to all branches. If I add a new branch I don't want to think about metrics. Like this I don't have to. If I forget to add an error I will see in the metrics err_not_classified and I can check the logs what error is missing. Otherwise you would just silently not see it.

I think we agree.
My proposal works if we have afew branches, and when the metric logic is not getting into nested logic.

(on small note, in the future you can take for granted that you do not have logs :)
only a metric counter)

---

go/path_srv/internal/segsyncer/segsyncer.go, line 244 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Do you propose that every caller of this method should check whether it returned ErrNotClassified and then log?

I am saying that when an person would see the above log entry, he will think
that there is actually a real error happening, and then he will try go track it down
and he will not really able to say where in the code happened.

If you think want to keep the comment, just add a description that includes the metric name label value matching failed) and maybe add .Info so it will be seen

I would have had though just a

default:
 metrick.Unknown

[lukedirtwalker]

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

---

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

Previously, karampok (Konstantinos) wrote…

Still you have to tell me what do you prefer :)

sorry: I prefer a mix of both: All values that are defined in prom don't need to have additional doc, as they should inherit it. All local defined variables should have a comment.
But since we can't inherit comments it might be best to just state them always.

---

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

Previously, karampok (Konstantinos) wrote…

uncomplete

Done.

---

go/path_srv/internal/metrics/sync.go, line 25 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

I usually keep it one line if it fits on 100char, but keep it as you feel

Done.

---

go/path_srv/internal/metrics/sync.go, line 70 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Not really, if you want to expose only the methods (interface).
But there is no strong practice here.
(my approach is keep everything unexported until I have not to)

Ok since the doc expects non-public I also changed it.

---

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

Previously, karampok (Konstantinos) wrote…

s.registrations directly.

Dominik once asked me not to import any new dependency but have the function
to accept ints (instead of stats struct).
Up to you, I am okay with both

Ok changed the stats, but I still left s.Registrations instead of s.registrations it is less code and easier to read IMO.

---

go/path_srv/internal/segsyncer/segsyncer.go, line 244 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

I am saying that when an person would see the above log entry, he will think
that there is actually a real error happening, and then he will try go track it down
and he will not really able to say where in the code happened.

If you think want to keep the comment, just add a description that includes the metric name label value matching failed) and maybe add .Info so it will be seen

I would have had though just a

default:
 metrick.Unknown

Done.

[karampok]

err_no_path changed
do not forget to update the description when merge

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

---

go/path_srv/internal/metrics/sync.go, line 70 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Ok since the doc expects non-public I also changed it.

you can remove the comment also

---

go/path_srv/internal/metrics/sync.go, line 91 at r3 (raw file):

// RegistrationSuccess increments registrations with the given stats.
func (s sync) RegistrationSuccess(l SyncRegLabels, new, updated int) {

I am afraid new is go keyword, maybe inserted

[lukedirtwalker]

Updated the description.

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

---

go/path_srv/internal/metrics/sync.go, line 91 at r3 (raw file):

Previously, karampok (Konstantinos) wrote…

I am afraid new is go keyword, maybe inserted

Done.

[karampok]

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