doc: Add metrics doc #3162

oncilla · 2019-09-18T11:09:38Z

Add a metrics doc that outlines the way we instrument the code.

This change is

kormat

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

doc/Metrics.md, line 104 at r1 (raw file):

strucst

s/to/the/

doc/Metrics.md, line 136 at r1 (raw file):

        beacons: prom.NewCounterVec(Namespace, sub, "beacons_total",
            "Total number of beacons originated.", OriginatorLabels.Labels()),
        time: prom.NewCounter(Namespace, sub, "time_seconds_total",

I think "duration" is a better name here (following the example of http_request_duration_seconds in https://prometheus.io/docs/practices/naming/).

doc/Metrics.md, line 184 at r1 (raw file):

func (in *input) BytesWith(l SocketLabels) prometheus.Counter {

This should be PktSize

doc/Metrics.md, line 202 at r1 (raw file):

func (s *Sender) Run(ctx context.Context) {
    l := metrics.KeepaliveLabel{IfID: ifid, Result: metrics.ErrNotClassified}

You need to drop ifid from here, and instead set l.Ifid at the top of the loop.

doc/Metrics.md, line 247 at r1 (raw file):

1. Namespace should be one word
1. Subsystem should be one word (if present)
1. Use values that can be search with regex. E.g. prepend `err_` for every error result.

searched

kormat

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

doc/Metrics.md, line 202 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

You need to drop ifid from here, and instead set l.Ifid at the top of the loop.

Actually i think you want to move this entire line to the top of the loop.

oncilla

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @kormat and @lukedirtwalker)

doc/Metrics.md, line 104 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

strucst
s/to/the/

Done.

doc/Metrics.md, line 136 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I think "duration" is a better name here (following the example of http_request_duration_seconds in https://prometheus.io/docs/practices/naming/).

Done.

doc/Metrics.md, line 184 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should be PktSize

Done.

doc/Metrics.md, line 202 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

Actually i think you want to move this entire line to the top of the loop.

yes, you're right.
Done.

doc/Metrics.md, line 247 at r1 (raw file):

Previously, kormat (Stephen Shirley) wrote…

searched

Done.

karampok · 2019-09-18T12:16:27Z

doc/Metrics.md, line 119 at r3 (raw file):

}

func (l *OrigniatorLabels) Labels() []string{

If we export that, then we need a comment so everyone to follow that

karampok · 2019-09-18T12:17:19Z

doc/Metrics.md, line 114 at r3 (raw file):

// beacon_srv/internal/metrics/originator.go

type OriginatorLabels struct {

Comment for the linter?
Suggestiong
// KeepaliveLabels is used by clients to pass in a safe way labels
// values to prometheus metric types (e.g. counter).

karampok · 2019-09-18T12:19:24Z

doc/Metrics.md, line 213 at r3 (raw file):

        sentIfids = append(sentIfids, ifid)
        l.Result = metrics.Success
        metrics.Keepalive.Transmit(l).Inc()

Ideally the usage example should be mapped to the example definition. For example what is Transmit? expecting to find somewhere above

karampok · 2019-09-18T12:21:25Z

doc/Metrics.md, line 80 at r3 (raw file):

import "github.com/scionproto/scion/go/lib/prom"

const Namespace = "bs"

Is there a reason why we need to export it? Then let's agree on a comment

karampok · 2019-09-18T12:22:32Z

doc/Metrics.md, line 123 at r3 (raw file):

}

func (l *OrigniatorLabels) Values() []string {

there a typo (it was why i could search where is being used :) )

karampok · 2019-09-18T12:24:10Z

doc/Metrics.md, line 26 at r3 (raw file):

│       │    ├── originator.go
│       │    ├── propagator.go
│       │    └── registrar.go

For completeness, the metric/keepalive should be there also

kormat

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

oncilla

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

doc/Metrics.md, line 80 at r3 (raw file):

Previously, karampok (Konstantinos) wrote…

Is there a reason why we need to export it? Then let's agree on a comment

Done.

doc/Metrics.md, line 114 at r3 (raw file):

Previously, karampok (Konstantinos) wrote…

Comment for the linter?
Suggestiong
// KeepaliveLabels is used by clients to pass in a safe way labels
// values to prometheus metric types (e.g. counter).

Added more generic string.

doc/Metrics.md, line 119 at r3 (raw file):

Previously, karampok (Konstantinos) wrote…

If we export that, then we need a comment so everyone to follow that

This is a trade-off between testability and minimal API design.
see: https://github.com/scionproto/scion/blob/043fdd7da50d946d65d43278979c6955c1f66ae7/go/lib/prom/promtest/promtest.go

doc/Metrics.md, line 123 at r3 (raw file):

Previously, karampok (Konstantinos) wrote…

there a typo (it was why i could search where is being used :) )

Done.

doc/Metrics.md, line 213 at r3 (raw file):

Previously, karampok (Konstantinos) wrote…

Ideally the usage example should be mapped to the example definition. For example what is Transmit? expecting to find somewhere above

Done.

oncilla

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

doc/Metrics.md, line 26 at r3 (raw file):

Previously, karampok (Konstantinos) wrote…

For completeness, the metric/keepalive should be there also

Done.

karampok

Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)

kormat

Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lukedirtwalker)

karampok

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

doc/Metrics.md, line 31 at r4 (raw file):

│       │    └── sender.go
│       └── beaconing
│            ├── keepalive.go

this is under metrics, no?

doc/Metrics.md, line 124 at r4 (raw file):

// Labels returns the list of labels.
func (l *OriginatorLabels) Labels() []string{

do we need the *pointer?

oncilla

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @karampok and @kormat)

doc/Metrics.md, line 31 at r4 (raw file):

Previously, karampok (Konstantinos) wrote…

this is under metrics, no?

ah, yes.
Done.

doc/Metrics.md, line 124 at r4 (raw file):

Previously, karampok (Konstantinos) wrote…

do we need the *pointer?

we don't have to, but it's best practice to keep the receiver types consistent.
https://golang.org/doc/faq#methods_on_values_or_pointers

karampok

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

doc/Metrics.md, line 124 at r4 (raw file):

Previously, Oncilla wrote…

we don't have to, but it's best practice to keep the receiver types consistent.
https://golang.org/doc/faq#methods_on_values_or_pointers

I would keep both without

oncilla

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

doc/Metrics.md, line 124 at r4 (raw file):

Previously, karampok (Konstantinos) wrote…

I would keep both without

Done.

karampok

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

Add a metrics doc that outlines the way we instrument the code.

oncilla added the c/observability Metrics, logging, tracing label Sep 18, 2019

oncilla requested review from lukedirtwalker and kormat September 18, 2019 11:09

kormat suggested changes Sep 18, 2019

View reviewed changes

oncilla force-pushed the pub-metrics-gist branch from f8e6697 to e99d7bb Compare September 18, 2019 11:50

oncilla commented Sep 18, 2019

View reviewed changes

kormat approved these changes Sep 18, 2019

View reviewed changes

oncilla force-pushed the pub-metrics-gist branch from e99d7bb to 0710dc1 Compare September 18, 2019 13:37

oncilla commented Sep 18, 2019

View reviewed changes

karampok approved these changes Sep 18, 2019

View reviewed changes

kormat approved these changes Sep 19, 2019

View reviewed changes

karampok approved these changes Sep 19, 2019

View reviewed changes

lukedirtwalker removed their request for review September 19, 2019 14:37

oncilla force-pushed the pub-metrics-gist branch from 0649703 to f43db34 Compare September 20, 2019 08:32

oncilla commented Sep 20, 2019

View reviewed changes

karampok approved these changes Sep 20, 2019

View reviewed changes

oncilla commented Sep 20, 2019

View reviewed changes

karampok approved these changes Sep 20, 2019

View reviewed changes

oncilla added 4 commits September 23, 2019 12:58

doc: Add metrics doc

66af87a

Add a metrics doc that outlines the way we instrument the code.

internal sub-package

e26dc40

feedback

c93c141

add godoc to sample code

57d45dd

oncilla added 5 commits September 23, 2019 12:58

add keepalive to example

07e0975

add namespace comment

fd85024

add to file tree

2e05433

feedback

958d226

value receiver

3ce50f6

oncilla force-pushed the pub-metrics-gist branch from e5c0ca4 to 3ce50f6 Compare September 23, 2019 10:58

oncilla merged commit 7c7df84 into scionproto:master Sep 23, 2019

oncilla deleted the pub-metrics-gist branch September 23, 2019 11:08

doc: Add metrics doc #3162

doc: Add metrics doc #3162

Uh oh!

Conversation

oncilla commented Sep 18, 2019 • edited by kormat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kormat left a comment

Choose a reason for hiding this comment

Uh oh!

kormat left a comment

Choose a reason for hiding this comment

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

karampok commented Sep 18, 2019

Uh oh!

karampok commented Sep 18, 2019

Uh oh!

karampok commented Sep 18, 2019

Uh oh!

karampok commented Sep 18, 2019

Uh oh!

karampok commented Sep 18, 2019

Uh oh!

karampok commented Sep 18, 2019

Uh oh!

kormat left a comment

Choose a reason for hiding this comment

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

karampok left a comment

Choose a reason for hiding this comment

Uh oh!

kormat left a comment

Choose a reason for hiding this comment

Uh oh!

karampok left a comment

Choose a reason for hiding this comment

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

karampok left a comment

Choose a reason for hiding this comment

Uh oh!

oncilla left a comment

Choose a reason for hiding this comment

Uh oh!

karampok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oncilla commented Sep 18, 2019 •

edited by kormat

Loading