metrics: introduce go-metrics-interface #3189

Kubuxu · 2016-09-05T15:56:49Z

It makes introducing metrics much easier.

The interface itself doesn't depend on prometheus, that means it is very light and without it (or other implementation) acts as a general noop.

Importing go-metrics-prometheus injects prometheus implementation.

Usage:
There are three most improtant functions in go-metrics-interface:

metrics.New(name, helptext) - creates new metrics with given name and helptext
metrics.NewCtx(ctx, name, helptext) - creates new metrics with given name and helptext in scope defined in the context
metric.CtxSubScope(ctx, name) - returns context that is a subscope of scope defined in current ctx

As you can see it uses context.Value for passing the scope name of the metrics. In my opinion it is one of the better solutions as context is already required in many places, and it introduction in constructors chains isn't bad either.

Current problem I have is that goprocess doesn't have something like context.Value so passing scopes through goprocess is something I haven't figured out yet but it can be done later.

//cc @whyrusleeping @lgierth

whyrusleeping · 2016-09-05T23:47:24Z

blocks/blockstore/arc_cache.go

 }

-func arcCached(bs Blockstore, lruSize int) (*arccache, error) {
+func newARCCachedBS(bs Blockstore, ctx context.Context, lruSize int) (*arccache, error) {


convention is to place the context first, i think that even though the blockstore is the primary object we should still respect that.

Kubuxu · 2016-09-06T06:04:25Z

Updated.

whyrusleeping · 2016-09-06T18:26:28Z

blocks/blockstore/bloom_cache.go

 	bc.Invalidate()
 	go bc.Rebuild(ctx)
-
+	go func() {


If there a quick way to check if we even have metrics enabled? It would be nice if metrics were turned off, this goroutine wouldnt exist.

Yeah, had the same idea, I will add method to go-metrics-interface to check if there was some impl injected.

might even be cool to wrap this sort of behaviour in a metrics.Periodic type method.

whyrusleeping · 2016-09-06T18:56:36Z

This LGTM. I'm definitely on board with doing things this way.

Some notes for future metrics to add (maybe i should make this an issue?)

Bitswap wantlist size
Number of stored provider records
Number of each dht rpc being made
Number of diagnostics service messages handled

Kubuxu · 2016-09-06T19:11:24Z

Number of each dht rpc being made

This option could really use other feature of prometheus: labels, but for that I will have to figure out how to wrap it nicely first.

whyrusleeping · 2016-09-07T16:19:51Z

Alright, so i'm gonna merge this. But since there are a few tweaks we want to make to the interface, lets hold off on pushing it into packages outside this repo (to avoid the pain of massive amounts of updating)

Kubuxu · 2016-09-07T16:32:00Z

Wait a sec.

Kubuxu · 2016-09-07T16:34:31Z

Updated to not run bloom fillrate collector when metrics aren't active.

whyrusleeping · 2016-09-07T18:57:24Z

blocks/blockstore/bloom_cache.go

-				return
-			case <-t.C:
-				fill.Set(bc.bloom.FillRatio())
+	if metrics.Active() {


Is this some sort of global? That feels very wrong to me. Why arent we checking if the context has metrics injected into it and using that as a way to tell if we have metrics enabled?

Context is used to define full name of the metrics.

This checks if there is metrics collector injected into go-metrics-interface.
The collector is global.

ooooh.... hrm.. I misunderstood how this all worked then. I was under the impression we were avoiding globals entirely by using the contexts to pass metrics down through to where they would be collected.

This is still okay, Its just less testable and composable than i was hoping.

We could do that, but then we won't be able to use metrics from libp2p as the goprocess doesn't support WithValue/Value.

in general: don't use statics. If you run multiple IPFS daemons, the
metrics may clash. If the lib FORCES you to use a global, then add a
guaranteed-unique prefix. Ideally with a local variable that internally
uses the global, threaded through with the context.

but I don't care enough about this case to force it, if it's way easier
and safe to have thousands of IPFS daemons in the same process, and nothing
will clash, fine.

On Wed, Sep 7, 2016 at 9:10 PM Jeromy Johnson notifications@github.com
wrote:

In blocks/blockstore/bloom_cache.go
#3189 (comment):

bc.Invalidate() go bc.Rebuild(ctx)

go func() {

<-bc.rebuildChan

t := time.NewTicker(1 \* time.Minute)

for {

select {

case <-ctx.Done():

t.Stop()

return

case <-t.C:

fill.Set(bc.bloom.FillRatio())

if metrics.Active() {

This is still okay, Its just less testable and composable than i was
hoping.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/ipfs/go-ipfs/pull/3189/files/0fa592196ed0f0429ef5a734e4efc17e2f8e0cb9..bb405f316a3819a4dba1d972dca8a547ea80fcb0#r77885221,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIcoRleigomIjiQWwSzrGwiXL5YAXpYks5qnwxCgaJpZM4J1L0L
.

Metrics you get will be a bit meaningless (as they will be combined) but we install the collector in cmd/ipfs/daemon.go so someone using ipfs as a lib can use different collector.

I can also make ipfs use subscope if there is one passed to context in which NewNode then you will be able to pass scope in which given IPFS nodes should be created.

whyrusleeping · 2016-09-08T13:26:16Z

cmd/ipfs/daemon.go

@@ -359,8 +360,7 @@ func daemonFunc(req cmds.Request, res cmds.Response) {
 	}

 	// initialize metrics collector
-	prometheus.MustRegisterOrGet(&corehttp.IpfsNodeCollector{Node: node})
-	prometheus.EnableCollectChecks(true)


why was enableCollectChecks removed?

It was removed from API and enabled by default.

whyrusleeping · 2016-09-08T13:27:29Z

Where do we actually fill the interface? I see the old prometheus stuff... but i think i'm missing where we actually add prometheus as a collector for your metrics package

Kubuxu · 2016-09-09T06:44:26Z

https://github.com/ipfs/go-ipfs/pull/3189/files#diff-54f2c4a5481de30b8ccb59ad5b6588b3R31 I am using init() method of go-metrics-prometheus package.

whyrusleeping · 2016-09-09T18:21:26Z

@Kubuxu i'm not sure i'm comfortable with injecting things that way... It should be an explicit thing we do in an init function of whatever code we import it from.

also need rebasing here

Kubuxu · 2016-09-12T11:05:49Z

The single purpose of go-metrics-prometheus is to be injected into go-metrics-interface, this pacakge doesn't export any other symbols.

I will change it.

Kubuxu · 2016-09-12T18:01:48Z

That was one hello of a rebase.

We really need to do something with this.

whyrusleeping · 2016-09-12T18:10:15Z

I'm not super on board with the way this does metrics... but since it is an improvement on the current situation, we can merge it in.

Some action items to move forward after this:

fix goprocess to allow context Values to be passed through to derived contexts Pass context values through to derived contexts jbenet/goprocess#16
switch away from using a 'global' metrics registration and inject metrics collection into the top level metrics. Pass metrics collection through the root context of an ipfs node #3212
Add flags/config vars to disable/enable metrics collection Metrics control flags #3213
Add more metrics!

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

ARC cache is influenced by requests and bloom isn't This means that if bloom is able to remove some requests caching them in ARC is pointless. License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

so the context is first one License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

Kubuxu · 2016-09-13T12:00:46Z

Rebased it once more

whyrusleeping · 2016-09-20T01:02:06Z

Alright, lets get this in. Thanks @Kubuxu! Better metrics for everyone!

metrics: introduce go-metrics-interface This commit was moved from ipfs/kubo@f23cd5c

Kubuxu added the status/in-progress In progress label Sep 5, 2016

Kubuxu force-pushed the feat/metrics/interface branch from 6aff024 to b5fd41d Compare September 5, 2016 16:15

Kubuxu added need/review Needs a review and removed status/in-progress In progress labels Sep 5, 2016

whyrusleeping reviewed Sep 5, 2016
View reviewed changes

Kubuxu force-pushed the feat/metrics/interface branch from 47d568c to 98bf4c4 Compare September 6, 2016 08:10

whyrusleeping reviewed Sep 6, 2016
View reviewed changes

whyrusleeping reviewed Sep 7, 2016
View reviewed changes

Kubuxu force-pushed the feat/metrics/interface branch 2 times, most recently from 94917e2 to 7f77cc5 Compare September 8, 2016 09:33

whyrusleeping reviewed Sep 8, 2016
View reviewed changes

whyrusleeping assigned Kubuxu Sep 9, 2016

whyrusleeping added need/author-input Needs input from the original author and removed need/review Needs a review labels Sep 9, 2016

Kubuxu added need/review Needs a review and removed need/author-input Needs input from the original author labels Sep 12, 2016

Kubuxu force-pushed the feat/metrics/interface branch 2 times, most recently from b5c89d3 to 184b72c Compare September 12, 2016 18:00

whyrusleeping mentioned this pull request Sep 12, 2016

Pass metrics collection through the root context of an ipfs node #3212

Open

Kubuxu added 10 commits September 13, 2016 14:00

metrics: update prometheus

a2bb6e8

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

metrics: add hit counter for ARC and bloom caches

3251c29

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

blockstore: move ARC cache below the bloom cache

66a031f

ARC cache is influenced by requests and bloom isn't This means that if bloom is able to remove some requests caching them in ARC is pointless. License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

blockstore: change order of newARCCachedBS parmaeters

d1237f1

so the context is first one License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

blockstore: update bbloom

95dca1e

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

blockstore: add Bloom fill ratio metric

5f0be2e

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

metrics: fix peer number metric in offline mode

7faa324

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

deps: update go-metrics-interface and -prometheus to 0.1.2

a5899a4

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

metrics: do not run bloom fillrate collector when metrics are inactive

c7d8966

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

metrics: do explicit metrics injection

9cbeffa

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

Kubuxu force-pushed the feat/metrics/interface branch from 184b72c to 9cbeffa Compare September 13, 2016 12:00

whyrusleeping added the status/in-progress In progress label Sep 14, 2016

whyrusleeping merged commit f23cd5c into master Sep 20, 2016

whyrusleeping deleted the feat/metrics/interface branch September 20, 2016 01:02

whyrusleeping removed the status/in-progress In progress label Sep 20, 2016

ghost mentioned this pull request Dec 23, 2016

Code Review v0.4.4...v0.4.5 #3534

Closed

hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023

Merge pull request ipfs/kubo#3189 from ipfs/feat/metrics/interface

8c75ffd

metrics: introduce go-metrics-interface This commit was moved from ipfs/kubo@f23cd5c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metrics: introduce go-metrics-interface #3189

metrics: introduce go-metrics-interface #3189

Kubuxu commented Sep 5, 2016

whyrusleeping Sep 5, 2016

Kubuxu commented Sep 6, 2016

whyrusleeping Sep 6, 2016

Kubuxu Sep 6, 2016

whyrusleeping Sep 6, 2016

whyrusleeping commented Sep 6, 2016

Kubuxu commented Sep 6, 2016

whyrusleeping commented Sep 7, 2016

Kubuxu commented Sep 7, 2016

Kubuxu commented Sep 7, 2016

whyrusleeping Sep 7, 2016

Kubuxu Sep 7, 2016 •

edited

Loading

whyrusleeping Sep 7, 2016

whyrusleeping Sep 7, 2016

Kubuxu Sep 8, 2016

jbenet Sep 8, 2016

Kubuxu Sep 9, 2016

whyrusleeping Sep 8, 2016

Kubuxu Sep 9, 2016

whyrusleeping commented Sep 8, 2016

Kubuxu commented Sep 9, 2016

whyrusleeping commented Sep 9, 2016

Kubuxu commented Sep 12, 2016

Kubuxu commented Sep 12, 2016

whyrusleeping commented Sep 12, 2016 •

edited

Loading

Kubuxu commented Sep 13, 2016

whyrusleeping commented Sep 20, 2016

metrics: introduce go-metrics-interface #3189

metrics: introduce go-metrics-interface #3189

Conversation

Kubuxu commented Sep 5, 2016

Choose a reason for hiding this comment

Kubuxu commented Sep 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Sep 6, 2016

Kubuxu commented Sep 6, 2016

whyrusleeping commented Sep 7, 2016

Kubuxu commented Sep 7, 2016

Kubuxu commented Sep 7, 2016

Choose a reason for hiding this comment

Kubuxu Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Sep 8, 2016

Kubuxu commented Sep 9, 2016

whyrusleeping commented Sep 9, 2016

Kubuxu commented Sep 12, 2016

Kubuxu commented Sep 12, 2016

whyrusleeping commented Sep 12, 2016 • edited Loading

Kubuxu commented Sep 13, 2016

whyrusleeping commented Sep 20, 2016

Kubuxu Sep 7, 2016 •

edited

Loading

whyrusleeping commented Sep 12, 2016 •

edited

Loading