observe traffic and expose statistics #243

pgte · 2018-02-16T17:41:37Z

Required because of libp2p/js-libp2p#157

victorb · 2018-02-20T13:19:53Z

test/stats.node.js

@@ -82,19 +82,47 @@ describe('Stats', () => {
    })
  })

-  it('waits a bit', (done) => setTimeout(done, 1900))
+  it('waits a bit', (done) => setTimeout(done, 1000))


A test case that just waits? Should fix the tests to not depend on the ordering...

@victorbjelkholm good call! Fixed by moving the preparation code to a before hook.

@pgte better to get rid of the after/before hooks we have and have function calls in the test cases that returns what you need. If all tests where shaped like this, we can run tests in parallel and just run one isolated test case if wanted.

In my case I need something to run once, not for every test.
In this case, I want to setup some switches, exchange some protocol-specific data, wait for the stats to compute, and then I can infer all the statistics are there.
@victorbjelkholm Any recommendations?

@victorbjelkholm changed to isolated tests for stats, would love your feedback.

victorb · 2018-02-20T15:45:13Z

test/stats.node.js

+      const peerA = infos[0]
+      const peerB = infos[1]
+
+      peerA.multiaddrs.add('/ip4/127.0.0.1/tcp/9001')


This is gonna conflict if we run two test cases at the same time, is it possible to set the port to 0 and then later get it (if you need it) instead? Think you're not actually using the port in the test, so should work fine to be set to 0.

@victorbjelkholm good call, fixed!

victorb

Tests LGTM 👍

hacdias

Looks Awesome 😄

daviddias

@pgte really solid work here (and in record time!) 👏🏽Made some comments in what I think is a regression (protocolMuxing) and request for more tests.

daviddias · 2018-02-21T08:25:07Z

README.md

@@ -30,6 +30,7 @@ libp2p-switch is used by [libp2p](https://github.com/libp2p/js-libp2p) but it ca
  - [`switch.stop(callback)`](#swarmclosecallback)
  - [`switch.connection`](#connection)
  - [Internal Transports API](#transports)
+  - [`switch.stats`](#stats-api)


move this above the Internal Transports line.

daviddias · 2018-02-21T08:26:06Z

README.md

@@ -51,7 +52,7 @@ libp2p-switch is used by [libp2p](https://github.com/libp2p/js-libp2p) but it ca
 ```JavaScript
 const switch = require('libp2p-switch')

-const sw = new switch(peerInfo [, peerBook])
+const sw = new switch(peerInfo [, peerBook [, options]])


This makes it look like options only exists in the context of passing peerBook. What about changing this to just a single options object and assert on its properties?

@diasdavid can do this, but will be a breaking API change..

Wait, I see that a peerBook is always required. The only optional argument should be options.

daviddias · 2018-02-21T08:27:08Z

README.md

@@ -223,6 +224,106 @@ To avoid the confusion between connection, stream, transport, and other names th
 - connection - something that implements the transversal expectations of a stream between two peers, including the benefits of using a stream plus having a way to do half duplex, full duplex
 - transport - something that as a dial/listen interface and return objs that implement a connection interface

+### Stats API


Do you see this all RED too?

Mind checking what went wrong here?

daviddias · 2018-02-21T08:27:36Z

README.md

+Should return a stats snapshot, which is an object containing the following keys and respective values:
+
+* dataSent: amount of bytes sent, [Big](https://github.com/MikeMcl/big.js#readme) number
+* dataReceived: amount of bytes received, [Big](https://github.com/MikeMcl/big.js#readme) number


s/*/-/g for consistency

daviddias · 2018-02-21T08:28:12Z

README.md

+
+Returns an array containing the tags (string) for each observed transport.
+
+##### `switch.stats.forTransports(transportTag).snapshot`


nickpick s/forTransports/transport/g (singular and no need the word for, I think)

daviddias · 2018-02-21T08:30:41Z

src/connection.js


        // If identify is enabled
        //   1. overload getPeerInfo
        //   2. call getPeerInfo
        //   3. add this conn to the pool
        if (swarm.identify) {
          // overload peerInfo to use Identify instead
+          const setPeerInfo = conn.setPeerInfo


Isn't there an overload of setPeerInfo? I'm afraid that this might cause issues in the future. Is it needed?

daviddias · 2018-02-21T08:32:33Z

src/connection.js

@@ -120,7 +123,7 @@ module.exports = function connection (swarm) {
      swarm.handle(tag, (protocol, conn) => {
        const myId = swarm._peerInfo.id
        const secure = encrypt(myId, conn, undefined, () => {
-          protocolMuxer(swarm.protocols, secure)
+          swarm.protocolMuxer(tag)(secure)


Something is not correct here. Doing swarm.protocolMuxer(tag) is saying "After you get the conn encrypted, you can encrypt again". It should be for all the Protocols being handled and not just for one.

Note: I'm still reading through the PR and you might have changed something else.

It's the exact same behaviour as before, with the exception we're tracking the protocol tag for accounting purposes..

daviddias · 2018-02-21T08:34:28Z

src/protocol-muxer.js

+    Object.keys(protocols).forEach((protocol) => {
+      if (!protocol) {
+        return
+      }


Still digesting the code but it seems that there is an assumption that all the switch.handle happen in the beginning and so you are locking the list of protocols we support.

The protocols is an object, which can get mutated later as the user adds more protocols.

Let's not get https://github.com/libp2p/js-libp2p-switch/issues/239 even harder to fix :)

@diasdavid again, this was the same behaviour as before: switch.protocols also contained a mutable object with the protocol handlers... I'm just storing the protocols on that closure, but it's still a JS object that will get mutated, same as before..

Got it. Makes sense now thanks for explaining :)

daviddias · 2018-02-21T08:36:29Z

src/stats/index.js

+    forTransport: (transport) => transportStats.get(transport),
+    protocols: () => Array.from(protocolStats.keys()),
+    forProtocol: (protocol) => protocolStats.get(protocol)
+  }


How does one "clear" previous stats from a peer?

daviddias · 2018-02-21T08:38:07Z

test/stats.node.js

+      teardown(switches, done)
+    })
+  })
+})


We need tests for:

Clearing out previous stats from a peer (Rep mechanisms will need a reset)

Check what happens when a peer disconnects and connects again

Check when a peer disconnects and connects again from a different transport

@diasdavid Currently, peer disconnections imply losing the stats for that peer.
If you think that it's important to keep them around, we should impose a time limit so that I eventually becomes garbage collected...
@diasdavid thoughts?

Perhaps tracking the sessions and having them in a LRU cache?

@diasdavid good call, but I'm not going to put all of the sessions there, only once a peer disconnects..

pgte · 2018-02-22T15:04:55Z

I still have one bug to squash that I discovered this morning:
The base protocol (tcp, wbrtc, etc.) is not being accounted for because the peerInfo never resolves on those connections. I'm still trying to track this down..

pgte · 2018-02-22T18:02:27Z

last described issue should be fixed by libp2p/js-libp2p-secio#98

…ic from global traffic

pgte · 2018-02-22T18:50:14Z

@diasdavid everything you asked for (apart from the protocolMuxing regression you described, which I think is not a regression, I argued above) should be done. :)

hacdias · 2018-02-26T18:15:04Z

Hello @pgte! 😄 When is this PR expected to be finished?

pgte · 2018-02-26T18:26:47Z

I think I’m all done here.

…

On 26 Feb 2018, 18:15 +0000, Henrique Dias ***@***.***>, wrote: Hello @pgte! 😄 When is this PR expected to be finished? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

daviddias

@pgte this looks great!! :) Thanks for kicking ass!

ghost assigned pgte Feb 16, 2018

ghost added the in progress label Feb 16, 2018

starting to observe

494d7fd

pgte force-pushed the feat/observe branch from e0d175d to 494d7fd Compare February 16, 2018 17:45

pgte added 9 commits February 19, 2018 11:03

WIP: stats: observer to stats

758cf2a

stats have transport now

0afd916

stats: peer list and transport list

0f5bfaa

stats: stop

20d6934

trying to set peer info

7210efc

propagating peer info

0eb8c26

stats: first tests

84e766b

stats: transports, protocols and peers

fa6e377

stats: transport-specific stats

a1d9014

victorb reviewed Feb 20, 2018

View reviewed changes

pgte added 5 commits February 20, 2018 14:24

stats: not relying on the order / existence of tests

3d0ad42

stats: peer-specific stats

7ee4fc5

setPeerInfo only if there's a peerInfo

6ea2061

stats: tests now are isolated

e692fa2

stats: tests: removed console.log

905680a

victorb reviewed Feb 20, 2018

View reviewed changes

stats: tests not hardcoding the port

f6db42d

victorb approved these changes Feb 20, 2018

View reviewed changes

pgte added 2 commits February 20, 2018 16:36

stats: documented API

ae88175

removed console log

39f01af

pgte changed the title ~~WIP: observe traffic~~ observe traffic and expose statistics Feb 20, 2018

pgte requested review from daviddias and hacdias February 20, 2018 16:40

hacdias approved these changes Feb 20, 2018

View reviewed changes

hacdias mentioned this pull request Feb 20, 2018

feat: .stats.bw* - Bandwidth Stats ipfs/js-ipfs#1230

Merged

9 tasks

daviddias suggested changes Feb 21, 2018

View reviewed changes

pgte added 2 commits February 21, 2018 09:20

stats: emit update event every time stats get updated

84b00f9

docs: moved stats api place in readme

eb2cf59

daviddias mentioned this pull request Feb 21, 2018

Connection Closing ipfs/js-ipfs#962

Closed

pgte added 5 commits February 21, 2018 11:46

docs: fixed typo

fb0d209

setting the peer info directly without having to cache the method

a2cd482

docs: consistency for list markdown

2ed0cf8

docs: dcumented options

5b3cb3b

stats: retaning old peers in case they reconnect

6c0ff32

pgte mentioned this pull request Feb 22, 2018

fix: cascading connection from encrypted connection to parent connection libp2p/js-libp2p-secio#98

Merged

stats: fix: now accounts for transport, and separates protocol-specif…

efc7c92

…ic from global traffic

pgte mentioned this pull request Feb 23, 2018

Expose switch stats libp2p/js-libp2p#168

Closed

daviddias approved these changes Feb 27, 2018

View reviewed changes

daviddias merged commit a4521dd into master Feb 27, 2018

daviddias deleted the feat/observe branch February 27, 2018 12:22

ghost removed the in progress label Feb 27, 2018


		Returns an array containing the tags (string) for each observed transport.

		##### `switch.stats.forTransports(transportTag).snapshot`

observe traffic and expose statistics #243

observe traffic and expose statistics #243

Conversation

pgte commented Feb 16, 2018 • 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

victorb left a comment

Choose a reason for hiding this comment

hacdias left a comment

Choose a reason for hiding this comment

daviddias left a 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

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

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

pgte commented Feb 22, 2018

pgte commented Feb 22, 2018

pgte commented Feb 22, 2018

hacdias commented Feb 26, 2018

pgte commented Feb 26, 2018 via email

daviddias left a comment

Choose a reason for hiding this comment

pgte commented Feb 16, 2018 •

edited

Loading