Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: stats API (stats.bitswap and stats.repo) #1198

Merged
merged 4 commits into from
Feb 15, 2018
Merged

feat: stats API (stats.bitswap and stats.repo) #1198

merged 4 commits into from
Feb 15, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jan 29, 2018

Right now, js-ipfs doesn't implement any of the STATS spec and it is required to make IPFS Desktop work with js-ipfs.

Prerequisites and related PRs

ToDos

  • bitswap.stat must implement the SPEC
    • HTTP API
    • Core
    • CLI
  • stats.bitswap just points to bitswap.stat
    • HTTP API
    • Core
    • CLI
  • repo.stat must implement the SPEC
    • HTTP API
    • Core
    • CLI
  • stats.repo just points to repo.stat
    • HTTP API
    • Core
    • CLI

Not Fundamental, but also done in this PR

  • /api/v0/repo/version
  • ipfs repo version
  • ipfs.repo.version()

Closes #1197
Closes #1116

@ghost ghost assigned hacdias Jan 29, 2018
@ghost ghost added the status/in-progress In progress label Jan 29, 2018
@hacdias hacdias changed the title Implement Stats API [WIP] Implement Stats API Jan 29, 2018
@hacdias hacdias mentioned this pull request Jan 30, 2018
@hacdias
Copy link
Member Author

hacdias commented Jan 30, 2018

Now I need to develop some kind of bandwidth metrics and I already know it is related to libp2p. I saw that with go-ipfs there is this library that was developed to take out metrics.

I was wondering if any of you (@diasdavid @dignifiedquire @pgte) could point me to the right direction since you are more into this project than I😄

@pgte
Copy link
Contributor

pgte commented Jan 30, 2018

Regarding bandwidth, you'll have to look into js-ipfs-bitswap.
There are two metrics: dataReceived and dataSent (bytes). But this only gives you a cumulative measure.

If you want per second, you can use a moving average. You have moving averages for 1, 5 and 15 minutes (this is configurable if you want to add a new interval).

There are no API docs (there is an issue about this somewhere), but here are the tests for this functionality: https://github.com/ipfs/js-ipfs-bitswap/blob/feat/benchmarks/test/bitswap-stats.js#L137-L145

Please tell me if you need more pointers.

@hacdias
Copy link
Member Author

hacdias commented Jan 30, 2018

Thanks @pgte!

Another question: when we have this on the API how should we do the response on the HTTP API? I tried this:

    const stream = Readable()
    reply(stream)

    stream.push(Buffer.from(JSON.stringify({
      TotalIn: stat.totalIn,
      TotalOut: stat.totalOut,
      RateIn: stat.rateIn,
      RateOut: stat.rateOut,
    })))

but it didn't work

@hacdias
Copy link
Member Author

hacdias commented Jan 30, 2018

@pgte could you take a look here? It seems to always print 0 for every field. And how should I get the stats for specific peers and protocols?

Ref.: Go implementation

@pgte
Copy link
Contributor

pgte commented Jan 30, 2018 via email

@hacdias
Copy link
Member Author

hacdias commented Jan 30, 2018

@pgte after digging into the source code I noticed we are using two funny special header x-stream-output and x-chunked-output. Didn't notice 😄

@pgte
Copy link
Contributor

pgte commented Jan 31, 2018

@hacdias are you still getting problems with the stats?

Regarding peer-specific stats: currently, js-ipfs-bitswap doesn't keep stats per peer.
It should be easy to add it. I'd be extra careful in garbage collection (removing stats for peers that are no longer relevant to us)..

@hacdias
Copy link
Member Author

hacdias commented Jan 31, 2018

@pgte the stats always have zero values:

bw: promisify((options, callback) => {
  if (typeof options === 'function') {
    callback = options
    options = {}
  }

  options = Object.assign({
    peer: '',
    proto: '',
    poll: false,
    interval: '1s'
  }, options)

  let stats

  if (options.peer) {
    // TODO: stats for a specific peer
  } else if (options.proto) {
    // TODO: stats for a specific proto
  } else {
    const stat = self._bitswap.stat()
    const snapshot = stat.snapshot
    const movingAverages = stat.movingAverages

    stats = {
      totalIn: snapshot.dataReceived,
      totalOut: snapshot.dataSent,
      rateIn: movingAverages.dataReceived[60000].movingAverage() / 60, // Bytes per second
      rateOut: movingAverages.dataSent[60000].movingAverage() / 60 // Bytes per second
    }
  }

  callback(null, stats)
})

That code always gives me a 0 on each key.

@pgte
Copy link
Contributor

pgte commented Jan 31, 2018

@hacdias this may be a timing issue.
Stats updates are throttled to, by default, a max of 1 seconds. This is configurable, by the way, but the stats are always updated in bulk to minimize interference with particular operations.

I don't know if it helps, but, like exemplified in the test, the stat object emits an update event once they're updated, so maybe you can subscribe to that before replying? I suggest making some experiments with that..

@pgte
Copy link
Contributor

pgte commented Jan 31, 2018

and, of course, opening an issue on js-ipfs-bitswap (preferably with a reproducible problem if you can :) ) if you're not able to get values.

@hacdias
Copy link
Member Author

hacdias commented Jan 31, 2018

@pgte I just added this:

image

to the place where bitswap is initialized. It seems that it never gets called (at least waiting 10 mins). I'm running js-ipfs directly with node src/cli/bin.js daemon.

@pgte
Copy link
Contributor

pgte commented Feb 1, 2018

@hacdias I think you'll have to enable stats like this.

@pgte
Copy link
Contributor

pgte commented Feb 1, 2018

... which I think may have to become a default setting in js-ipfs.
Or you could change the default in js-ipfs-bitswap if you don't want to add to the config clutter.

@pgte
Copy link
Contributor

pgte commented Feb 1, 2018

I will also be needing per-peer stats because of ipfs-inactive/dynamic-data-and-capabilities#3 (comment).

I opened bitswap issue here: ipfs/js-ipfs-bitswap#165

@hacdias could you state your requirements there?

@pgte
Copy link
Contributor

pgte commented Feb 2, 2018

@hacdias any luck enabling stats?

@hacdias
Copy link
Member Author

hacdias commented Feb 2, 2018

@pgte not really, they are always 0.

@pgte
Copy link
Contributor

pgte commented Feb 2, 2018

@hacdias are the stats tests passing?:

$ mocha test/bitswap-stats.js

They are for me, and they test for > 0 values.

(also, keep in mind that values are Big: https://github.com/MikeMcl/big.js#readme

@pgte
Copy link
Contributor

pgte commented Feb 2, 2018

Took a stab at documenting current stats API:
ipfs/js-ipfs-bitswap@4b27d36

Hope this + tests help.

@hacdias
Copy link
Member Author

hacdias commented Feb 7, 2018

Since libp2p/js-libp2p#157 might take a bit, I suggest to move Stats.Bandwidth to another PR and finish Stats.Bitswap and Stats.Repo here.

@daviddias
Copy link
Member

@hacdias js-ipfs-api was released. You should be able to finish this PR now :)

@hacdias
Copy link
Member Author

hacdias commented Feb 14, 2018

@diasdavid there is still one thing missing: we need to merge ipfs-inactive/interface-js-ipfs-core#216 in order to finish this PR.

@daviddias
Copy link
Member

merging #1216 first and then back at this one :)

@hacdias
Copy link
Member Author

hacdias commented Feb 14, 2018

OKay, as soon as you merge it, I'll finish this PR, but I'll still need ipfs-inactive/interface-js-ipfs-core#216 to be merged.

@daviddias
Copy link
Member

@hacdias do it like this:

  1. Rebase master onto this PR
  2. Run tests locally and guarantee that everything passes
  3. Ping me with everything ready so that I can merge and release and update the dep here in one go

@daviddias daviddias changed the title Implement Stats API (stats.bitswap and stats.repo) feat: stats API (stats.bitswap and stats.repo) Feb 15, 2018
- [repo](https://github.com/ipfs/interface-ipfs-core/tree/master/SPEC/)
- `ipfs.repo.init`
- `ipfs.repo.version`
- `ipfs.repo.gc` (not implemented, yet!)
Copy link
Member

Choose a reason for hiding this comment

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

Why move these to the Graph section??

Copy link
Member

Choose a reason for hiding this comment

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

I would agree if moved to the Node Management Section

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's on the Node Management section

Copy link
Member

Choose a reason for hiding this comment

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

nvm, I got fooled twice by GH interface. all good :)

wantlist [${stats.wantlist.length} keys]
${stats.wantlist.join('\n ')}
partners [${stats.peers.length}]
${stats.peers.join('\n ')}`)
})
}
}
Copy link
Member

Choose a reason for hiding this comment

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

After Rebase, enable the CLI Bitswap test stat

Copy link
Member Author

Choose a reason for hiding this comment

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

Done and it works!!

if (!self.isOnline()) {
throw new Error(OFFLINE_ERROR)
callback(new Error(OFFLINE_ERROR))
Copy link
Member

Choose a reason for hiding this comment

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

This needs two things:

  • Be wrapped with a setImmediate from the async module
  • Have a return, otherwise the rest of the function will be executed.

self._peerInfoBook
Object.assign({
statsEnabled: true
}, self._peerInfoBook)
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 weird. Are you saying that in order to enable stats on Bitswap there is a variable on PeerBook that needs to be true? This Object Assign is also probably messing with the fact that PeerBook is a Object from Class and not an Object Literal. //cc @pgte

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 need to have the stats enabled and this was the best way I found to do that.

Copy link
Member

Choose a reason for hiding this comment

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

@pgte could you comment here given that you implemented stats on Bitswap

Copy link
Member

Choose a reason for hiding this comment

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

I see. the thing is that bitswap doesn't need access to the PeerInfoBook at all. You can just pass the object options.

@hacdias hacdias force-pushed the stats-api branch 3 times, most recently from b8cd672 to bb5eece Compare February 15, 2018 10:02
@hacdias
Copy link
Member Author

hacdias commented Feb 15, 2018

@diasdavid the related tests to this PR seem to be working here 😄

daviddias
daviddias previously approved these changes Feb 15, 2018
Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Thank you @hacdias <3

@daviddias daviddias merged commit 905bdc0 into master Feb 15, 2018
@daviddias daviddias deleted the stats-api branch February 15, 2018 11:02
@ghost ghost removed the status/in-progress In progress label Feb 15, 2018
@daviddias daviddias mentioned this pull request Feb 17, 2018
JonKrone pushed a commit to JonKrone/js-ipfs that referenced this pull request Feb 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants