Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metrics: introduce go-metrics-interface #3189

Merged
merged 10 commits into from
Sep 20, 2016
Merged

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Sep 5, 2016

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

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

func arcCached(bs Blockstore, lruSize int) (*arccache, error) {
func newARCCachedBS(bs Blockstore, ctx context.Context, lruSize int) (*arccache, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 6, 2016

Updated.

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

go func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@whyrusleeping
Copy link
Member

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
Copy link
Member Author

Kubuxu commented Sep 6, 2016

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
Copy link
Member

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
Copy link
Member Author

Kubuxu commented Sep 7, 2016

Wait a sec.

@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 7, 2016

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@Kubuxu Kubuxu Sep 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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 [email protected]
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
.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Kubuxu Kubuxu force-pushed the feat/metrics/interface branch 2 times, most recently from 94917e2 to 7f77cc5 Compare September 8, 2016 09:33
@@ -359,8 +360,7 @@ func daemonFunc(req cmds.Request, res cmds.Response) {
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was enableCollectChecks removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was removed from API and enabled by default.

@whyrusleeping
Copy link
Member

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
Copy link
Member Author

Kubuxu commented Sep 9, 2016

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

@whyrusleeping
Copy link
Member

@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

@whyrusleeping whyrusleeping added need/author-input Needs input from the original author and removed need/review Needs a review labels Sep 9, 2016
@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 12, 2016

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 Kubuxu added need/review Needs a review and removed need/author-input Needs input from the original author labels Sep 12, 2016
@Kubuxu Kubuxu force-pushed the feat/metrics/interface branch 2 times, most recently from b5c89d3 to 184b72c Compare September 12, 2016 18:00
@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 12, 2016

That was one hello of a rebase.

We really need to do something with this.

@whyrusleeping
Copy link
Member

whyrusleeping commented Sep 12, 2016

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:

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
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 <[email protected]>
so the context is first one

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 13, 2016

Rebased it once more

@whyrusleeping whyrusleeping added the status/in-progress In progress label Sep 14, 2016
@whyrusleeping
Copy link
Member

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

@whyrusleeping whyrusleeping merged commit f23cd5c into master Sep 20, 2016
@whyrusleeping whyrusleeping deleted the feat/metrics/interface branch September 20, 2016 01:02
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Sep 20, 2016
@ghost ghost mentioned this pull request Dec 23, 2016
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
metrics: introduce go-metrics-interface

This commit was moved from ipfs/kubo@f23cd5c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants