discovery: Add metrics for itopo and idiscovery #3181

oncilla · 2019-09-24T14:49:08Z

Metrics added for idiscovery:

    # HELP idiscovery_file_writes_total The total number of file writes on updated topology
    # TYPE idiscovery_file_writes_total counter
    idiscovery_file_writes_total{result="ok_success",type="static"} 1
    # HELP idiscovery_sent_requests_total The total number of requests sent to the discovey service
    # TYPE idiscovery_sent_requests_total counter
    idiscovery_sent_requests_total{result="err_request",type="dynamic"} 1
    idiscovery_sent_requests_total{result="ok_success",type="dynamic"} 8
    idiscovery_sent_requests_total{result="ok_success",type="static"} 1

Metrics added for itopo:

    # HELP itopo_creation_time_seconds The creation time specified in the current topology.Remains set for dynamic topology, even when inactive.
    # TYPE itopo_creation_time_seconds gauge
    itopo_creation_time_seconds{type="static"} 0
    itopo_creation_time_seconds{type="dynamic"} 1.5693351735612462e+09
    # HELP itopo_dynamic_active Indicate whether the dynamic topology is set and active. 0=inactive, 1=active.
    # TYPE itopo_dynamic_active gauge
    itopo_dynamic_active 0
    # HELP itopo_expiry_time_seconds The expiry time specified in the current topology. Set to +Inf, if TTL is zero.Remains set for dynamic topology, even when inactive.
    # TYPE itopo_expiry_time_seconds gauge
    itopo_expiry_time_seconds{type="static"} +Inf
    itopo_expiry_time_seconds{type="dynamic"} 1.5693351735612462e+09
    # HELP itopo_last_updates Timestamp of the last update attempts.
    # TYPE itopo_last_updates gauge
    itopo_last_updates{result="ok_success",type="dynamic"} 1.56933517356125e+09
    itopo_last_updates{result="ok_success",type="static"} 1.5693351375621483e+09
    # HELP itopo_updates_total The total number of updates.
    # TYPE itopo_updates_total counter
    itopo_updates_total{result="ok_success",type="dynamic"} 8
    itopo_updates_total{result="ok_success",type="static"} 2

fixes #3097

This change is

karampok

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @kormat and @oncilla)

go/lib/infra/modules/idiscovery/idiscovery.go, line 358 at r1 (raw file):

	}
	metrics.Fetcher.Sent(l).Inc()
	metrics.Fetcher.File(l).Inc()

this two line sending a metric here surprises me :)

go/lib/infra/modules/idiscovery/idiscovery.go, line 375 at r1 (raw file):

}

func (t *task) metricType() string {

I would like to avoid having help function for the labels. If we cannot avoid I think inside the metric package is better.
Maybe a function the received boolean getMytype(dynamic boolean) so as not to pass the the t.mode

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

// Topology types.
const (

usually we have one const section and block. each block sorted

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

// Result labels.
const (
	Success      = prom.Success

We should discuss if we want to comment that part.
We sometimes do, we sometimes don't. I vote yes.

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

// Metrics initialization.
var (

no need (, one liner

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

// FetcherLabels defines the requests label set.
type FetcherLabels struct {
	Type   string

one liner looks golang

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

type fetcher struct {
	sent *prometheus.CounterVec

one liner looks more golang

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

}

func (r fetcher) Sent(l FetcherLabels) prometheus.Counter {

yes we should try to keep the generic comment

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

}

func (r fetcher) File(l FetcherLabels) prometheus.Counter {

generic comment

go/lib/infra/modules/itopo/itopo.go, line 251 at r1 (raw file):

func (s *state) dropDynamic() {
	s.topo.dynamic = nil
	call(s.clbks.DropDynamic)

state.dropDynamics and callback.DropDynamic, confuses me. Different names?
I will avoid hiding the metrics inside a function unless we have to repeat the code many times

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

// Namespace is the metrics namespace for the infra topology module.
const Namespace = "itopo"

usually one block of const

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

// Result labels.
const (
	Success     = prom.Success

usually commented

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

)

// Metrics initialization.

drop

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

// Metrics initialization.
var (
	Current = newCurrent()

comment

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

}

func (c current) Timestamp(l CurrentLabels) prometheus.Gauge {

generic comment

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

// UpdateLabels defines the update label set.
type UpdateLabels struct {
	Type   string

one liner (but keep your preference)

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

}

func (u updates) Last(l UpdateLabels) prometheus.Gauge {

comment

oncilla

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

go/lib/infra/modules/idiscovery/idiscovery.go, line 358 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

this two line sending a metric here surprises me :)

refactored to reduce your surprise :)

go/lib/infra/modules/idiscovery/idiscovery.go, line 375 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

I would like to avoid having help function for the labels. If we cannot avoid I think inside the metric package is better.
Maybe a function the received boolean getMytype(dynamic boolean) so as not to pass the the t.mode

Done.

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

Previously, karampok (Konstantinos) wrote…

usually we have one const section and block. each block sorted

For better documentation, I prefer having separate blocks.

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

Previously, karampok (Konstantinos) wrote…

We should discuss if we want to comment that part.
We sometimes do, we sometimes don't. I vote yes.

Done.

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

Previously, karampok (Konstantinos) wrote…

no need (, one liner

Done.

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

Previously, karampok (Konstantinos) wrote…

one liner looks golang

Done.

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

Previously, karampok (Konstantinos) wrote…

one liner looks more golang

Done.

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

Previously, karampok (Konstantinos) wrote…

yes we should try to keep the generic comment

I don't really see the value.
This is on a private sturct, so linter will not complain, and godoc will not show it
🤷‍♂️

Still added it.

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

Previously, karampok (Konstantinos) wrote…

generic comment

Done.

go/lib/infra/modules/itopo/itopo.go, line 251 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

state.dropDynamics and callback.DropDynamic, confuses me. Different names?
I will avoid hiding the metrics inside a function unless we have to repeat the code many times

callback.DropDynamic is the callback when the dynamic topology is dropped.
I think that is a very accurate name.

I feel like this is the only place where it makes sense to update that metric.

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

Previously, karampok (Konstantinos) wrote…

usually one block of const

Done.

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

Previously, karampok (Konstantinos) wrote…

usually commented

Done.

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

Previously, karampok (Konstantinos) wrote…

drop

Done.

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

Previously, karampok (Konstantinos) wrote…

comment

Done.

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

Previously, karampok (Konstantinos) wrote…

generic comment

Done.

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

Previously, karampok (Konstantinos) wrote…

one liner (but keep your preference)

Done.

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

Previously, karampok (Konstantinos) wrote…

comment

Done.

karampok

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kormat and @oncilla)

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

Previously, Oncilla wrote…

Done.

ok

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

Previously, Oncilla wrote…

I don't really see the value.
This is on a private sturct, so linter will not complain, and godoc will not show it
🤷‍♂️

Still added it.

I was under the impression that it would complain, but I am wrong.
And if it is not in godoc, I am not sure long-term how a person can you the setup without reading a code

(I mean it seems public method in a private struct seems wrong)

oncilla

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

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

Previously, karampok (Konstantinos) wrote…

I was under the impression that it would complain, but I am wrong.
And if it is not in godoc, I am not sure long-term how a person can you the setup without reading a code

(I mean it seems public method in a private struct seems wrong)

Public methods on private structs are perfectly fine.

e.g. when you want to implement an interface, but not expose the actual implementing type.

Metrics added for idiscovery: # HELP idiscovery_file_writes_total The total number of file writes on updated topology # TYPE idiscovery_file_writes_total counter idiscovery_file_writes_total{result="ok_success",type="static"} 1 # HELP idiscovery_sent_requests_total The total number of requests sent to the discovey service # TYPE idiscovery_sent_requests_total counter idiscovery_sent_requests_total{result="err_request",type="dynamic"} 1 idiscovery_sent_requests_total{result="ok_success",type="dynamic"} 8 idiscovery_sent_requests_total{result="ok_success",type="static"} 1 Metrics added for itopo: # HELP itopo_current_active Indicate whether the current topology is active. 0=inactive, 1=active. # TYPE itopo_current_active gauge itopo_current_active{type="dynamic"} 1 itopo_current_active{type="static"} 1 # HELP itopo_current_timestamp The timestamp of the current topology. Remains set, even when inactive. # TYPE itopo_current_timestamp gauge itopo_current_timestamp{type="dynamic"} 1.5693351735612462e+09 itopo_current_timestamp{type="static"} 1.5693351375621471e+09 # HELP itopo_current_ttl_seconds The TTL of the current topology. 0 indicates no TTL. Remains set, even when inactive. # TYPE itopo_current_ttl_seconds gauge itopo_current_ttl_seconds{type="dynamic"} 6e+10 itopo_current_ttl_seconds{type="static"} 0 # HELP itopo_last_updates Timestamp of the last update attempts. # TYPE itopo_last_updates gauge itopo_last_updates{result="ok_success",type="dynamic"} 1.56933517356125e+09 itopo_last_updates{result="ok_success",type="static"} 1.5693351375621483e+09 # HELP itopo_updates_total The total number of updates. # TYPE itopo_updates_total counter itopo_updates_total{result="ok_success",type="dynamic"} 8 itopo_updates_total{result="ok_success",type="static"} 2 fixes scionproto#3097

- itopo_creation_time_seconds (-> 0 when not set) - itopo_expiry_time_seconds (-> +Inf when not set) - itopo_dynamic_active Metrics added for idiscovery: # HELP idiscovery_file_writes_total The total number of file writes on updated topology # TYPE idiscovery_file_writes_total counter idiscovery_file_writes_total{result="ok_success",type="static"} 1 # HELP idiscovery_sent_requests_total The total number of requests sent to the discovey service # TYPE idiscovery_sent_requests_total counter idiscovery_sent_requests_total{result="err_request",type="dynamic"} 1 idiscovery_sent_requests_total{result="ok_success",type="dynamic"} 8 idiscovery_sent_requests_total{result="ok_success",type="static"} 1 Metrics added for itopo: # HELP itopo_creation_time_seconds The creation time specified in the current topology.Remains set for dynamic topology, even when inactive. # TYPE itopo_creation_time_seconds gauge itopo_creation_time_seconds{type="static"} 0 itopo_creation_time_seconds{type="dynamic"} 1.5693351735612462e+09 # HELP itopo_dynamic_active Indicate whether the dynamic topology is set and active. 0=inactive, 1=active. # TYPE itopo_dynamic_active gauge itopo_dynamic_active 0 # HELP itopo_expiry_time_seconds The expiry time specified in the current topology. Set to +Inf, if TTL is zero.Remains set for dynamic topology, even when inactive. # TYPE itopo_expiry_time_seconds gauge itopo_expiry_time_seconds{type="static"} +Inf itopo_expiry_time_seconds{type="dynamic"} 1.5693351735612462e+09 # HELP itopo_last_updates Timestamp of the last update attempts. # TYPE itopo_last_updates gauge itopo_last_updates{result="ok_success",type="dynamic"} 1.56933517356125e+09 itopo_last_updates{result="ok_success",type="static"} 1.5693351375621483e+09 # HELP itopo_updates_total The total number of updates. # TYPE itopo_updates_total counter itopo_updates_total{result="ok_success",type="dynamic"} 8 itopo_updates_total{result="ok_success",type="static"} 2

karampok

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

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

oncilla added this to the Q3S3 milestone Sep 24, 2019

oncilla requested review from karampok and kormat September 24, 2019 14:49

karampok suggested changes Sep 25, 2019

View reviewed changes

oncilla commented Sep 25, 2019

View reviewed changes

karampok approved these changes Sep 25, 2019

View reviewed changes

oncilla commented Sep 25, 2019

View reviewed changes

oncilla added 3 commits September 26, 2019 14:48

feedback

b9a3764

oncilla force-pushed the pub-metrics-discovery branch from 058e2d9 to cbc4a6d Compare September 26, 2019 12:48

oncilla removed the request for review from kormat September 26, 2019 12:59

karampok approved these changes Sep 26, 2019

View reviewed changes

oncilla merged commit fbf1482 into scionproto:master Sep 26, 2019

oncilla deleted the pub-metrics-discovery branch September 26, 2019 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

discovery: Add metrics for itopo and idiscovery #3181

discovery: Add metrics for itopo and idiscovery #3181

oncilla commented Sep 24, 2019 •

edited

Loading

karampok left a comment

oncilla left a comment

karampok left a comment

oncilla left a comment

karampok left a comment

discovery: Add metrics for itopo and idiscovery #3181

discovery: Add metrics for itopo and idiscovery #3181

Conversation

oncilla commented Sep 24, 2019 • edited Loading

karampok left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

karampok left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

karampok left a comment

Choose a reason for hiding this comment

oncilla commented Sep 24, 2019 •

edited

Loading