feat(breaking change): stats.bwReadableStream and stats.bwPullStream #211

hacdias · 2018-01-30T21:59:48Z

More details on ipfs-inactive/js-ipfs-http-client#686

This also closes #212

hacdias · 2018-01-31T08:10:29Z

Needed for ipfs/js-ipfs#1198 /cc @diasdavid 😄

victorb · 2018-01-31T08:17:08Z

@hacdias this should be a argument to the function that changes if it just returns the values once or if you get a stream back, rather than just having one of them.

The different signatures would be something like this:

ipfs.stats.bw((err, oneTimeRes) => {})

or

ipfs.stats.bw({poll: true}, () => {err, stream})

hacdias · 2018-01-31T08:19:16Z

Sure! Makes sense. Will change that @victorbjelkholm

hacdias · 2018-01-31T08:36:53Z

@victorbjelkholm done 😄

victorb · 2018-01-31T08:40:28Z

SPEC/STATS.md

-`callback` must follow `function (err, stats) {}` signature, where `err` is an error if the operation was not successful. `stats` is an Object containing the following keys:
+If `poll` is `true`, then `callback` must follow `function (err, stream) {}` signature, where `err` is an error if the operation was not successful and `stream` is a Readable Stream where you can listen to the event `data` with a listener that must follow `function (stat) {}` signature.
+
+Otherwise, `callback` must follow `function (err, stat) {}` signature, where `err` is an error and `stat` is an Object containing the following keys:


We should define what the values are as well, I think the values are defined in bytes, but unsure.

? The stat thing on the stream?

If so, it is an object like the other one. That's why I left them with the same name and explained in the end.

Yeah, that stat object. We say which keys are on the object, but not what the values are.

Problem is not understanding that stat is used in both cases, but what the values are. Are they bytes? Are they kilobytes?

I'll add that!

victorb · 2018-01-31T08:41:19Z

js/src/stats.js

+        expect(err).to.not.exist()
+        expect(res).to.exist()
+
+        res.once('data', (data) => {


We should have a way of closing the stream as well, for when you're done polling for stats

@victorbjelkholm probably stream.destroy() is our best shot

Added res.destroy() in the end.

victorb · 2018-01-31T08:43:14Z

js/src/stats.js

+        expect(res).to.exist()
+
+        res.once('data', (data) => {
+          expect(data).to.have.a.property('totalIn')


This expectation only checks that the property exists, which I don't think is good enough. These test cases would still pass even if totalIn is set to for example false, undefined or even null, which is not correct.

Noticed that the two test cases are the same as well. One should test with {poll: true} and the other one without passing any extra args.

We have 4 test cases: 2 for callbacks and two for promises. One of those two has {poll: true} and the other has no args.

Right now, those values are strings. Should I convert them to something like big.js?

Added a try catch to check if they are numbers using big.js.

victorb · 2018-01-31T08:46:49Z

js/src/stats.js

+          done()
+        })
+      }).catch(err => {
+        expect(err).to.not.exist()


can just do .catch(done) instead as done with a error set will fail the test case

Since you are using Promises, you can remove the done and just make sure that you return the promise. The way it is now you don't get any benefit from using the Promise syntax over Callbacks

daviddias · 2018-01-31T08:52:45Z

js/src/stats.js

+          done()
+        })
+      }).catch(err => {
+        expect(err).to.not.exist()


Since you are using Promises, you can remove the done and just make sure that you return the promise. The way it is now you don't get any benefit from using the Promise syntax over Callbacks

daviddias · 2018-01-31T08:53:28Z

SPEC/STATS.md

@@ -25,7 +25,11 @@ Where:
  - `poll` is used to print bandwidth at an interval.
  - `interval` is the time interval to wait between updating output, if `poll` is true.

-`callback` must follow `function (err, stats) {}` signature, where `err` is an error if the operation was not successful. `stats` is an Object containing the following keys:
+If `poll` is `true`, then `callback` must follow `function (err, stream) {}` signature, where `err` is an error if the operation was not successful and `stream` is a Readable Stream where you can listen to the event `data` with a listener that must follow `function (stat) {}` signature.


Why ReadableStream and not pull-stream?

I can use a pull-stream, but I didn't find a way to do this with that package...

@diasdavid Also, js-ipfs-api doesn't seem to be using pull streams. at least the package isn't a dependency.

And how can we close a pull-stream? Their docs say nothing about it. The Readable Stream seems to be more straightforward and easy to use. You just have to do .on('data', fn).

hacdias · 2018-01-31T11:23:34Z

@victorbjelkholm I made some updates so this also fixes #212

hacdias · 2018-01-31T11:31:10Z

@diasdavid, @victorbjelkholm and I discussed about using Readable Stream or Pull Stream and he pointed me to ipfs/js-ipfs#557 (comment). Then we noticed that it doesn't make much sense to have three different functions for this API since it is really simple.

Right now, I have made the implementation to use Readable Stream here. Do you think we should use pull-stream? If so, why and could you point me out some places to look to see how I should implement this because their docs didn't help me much.

daviddias · 2018-01-31T22:56:04Z

@hacdias there are some more bits that are important for you to get familiar with. Glad that you found ipfs/js-ipfs#557, make sure to also:

read through Node.js Streams vs pull-streams vs ReactiveJS ipfs/js-ipfs#362
Watch https://www.youtube.com/watch?v=g5ewQEuXjsQ
read http://dominictarr.com/post/145135293917/history-of-streams
read http://dominictarr.com/post/149248845122/pull-streams-pull-streams-are-a-very-simple
see how we support multiple streams interfaces at https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md
understand the memory implications of https://gist.github.com/dominictarr/33042821928753986138942d0fdcefed
get up to speed on https://github.com/ipfs/community/issues/278 and learn how this kind of polymorphism can have a severe perf impact.

Then we noticed that it doesn't make much sense to have three different functions for this API since it is really simple.

I feel that the argument that you are trying to make is that "this is not a very used function and so a simple thing will though", which can be fair, however, it is important to understand the future implications and make decisions that will enable users to migrate without having to follow a changing interface. Two cery good threads for you to read: https://unix.stackexchange.com/questions/235335/the-linux-kernel-breaking-user-space & https://felipec.wordpress.com/2013/10/07/the-linux-way/

Let's follow up once you went through all of these materials :)

victorb · 2018-02-01T08:50:38Z

js/src/repo.js

+        expect(isNumber(res.repoSize)).to.eql(true)
+        expect(isNumber(res.storageMax)).to.eql(true)
+        expect(res.repoPath).to.be.a('string')
+        expect(res.version).to.be.a('string')


This is still not valid as expect('').to.be.a('string') would pass the tests.

victorb · 2018-02-01T08:51:13Z

js/src/stats.js

+
+        expect(isNumber(res.provideBufLen)).to.eql(true)
+        expect(res.wantlist).to.be.an('array')
+        expect(res.peers).to.be.an('array')


Would we consider this test to be correct if res.peers was a empty array?

Yes and it might be empty in some cases.

victorb · 2018-02-01T08:51:51Z

js/src/repo.js

 chai.use(dirtyChai)

+const isNumber = (n) => {


This is actually checking if n is a BigInt, not Number, which is a built-in type in JavaScript

victorb · 2018-02-01T08:52:54Z

js/src/repo.js

-        expect(stat).to.have.property('repoPath')
-        expect(stat).to.have.property('version')
-        expect(stat).to.have.property('storageMax')
+        expect(res).to.exist()


These assertions are repeated all over the tests. We should extract the assertions into a function and pass in res instead.

const expectIsStat = (stat) => { // Make all the assertions once here }

victorb · 2018-02-01T08:54:04Z

SPEC/STATS.md

+
+`stat` is, in both cases, an Object containing the following keys:
+
+- `totalIn` - is a string containing a uint64 in bytes.


These are now Big values rather than strings.

daviddias · 2018-02-05T08:21:48Z

SPEC/STATS.md

@@ -25,12 +25,16 @@ Where:
  - `poll` is used to print bandwidth at an interval.
  - `interval` is the time interval to wait between updating output, if `poll` is true.

-`callback` must follow `function (err, stats) {}` signature, where `err` is an error if the operation was not successful. `stats` is an Object containing the following keys:
+If `poll` is `true`, then `callback` must follow `function (err, stream) {}` signature, where `err` is an error if the operation was not successful and `stream` is a Pull Stream where you can listen to the event `data` with a listener that must follow `function (stat) {}` signature.


@hacdias this wasn't necessarily what I was hoping for here. Follow the example of https://github.com/ipfs/interface-ipfs-core/blob/master/SPEC/FILES.md#files-api and create two api calls.

ipfs.stats.bw - single requests

ipfs.stats.bwPoll - return a stream

bwPoll returns which kind of stream?

ipfs.stats.bwPollPullStream

ipfs.stats.bwPollReadableStream

🙂

I'd just suggest to take the 'Poll' out of the equation and use this three functions:

stats.bw

stats.bwPullStream

stats.bwReadableStream

What do you think @diasdavid?

I suggest to structure the SPEC like that instead of creating different entries for each function and add examples for each of them. In FILES spec, we have:

Which is not accurate: we have one API endpoint which is cat. The other functions should be called variants or something like that and not separated in the SPEC. Why? Duplicated information (on the parameters for example) and then we have that GO - WIP thing which may make users think there will be catPullStream or [insert your command here]ReadableStream on Go. That's something I'd like to work on after finishing this Pull Request and make this interface documentation a little better. What do you think @diasdavid?

@hacdias let's avoid painting the 🚲🏡 in this PR. tl;dr; they are top level functions, they should be listed in the table of contents and have urls to it.

hacdias · 2018-02-06T14:29:49Z

@diasdavid @victorbjelkholm could you please take a look at the SPEC I wrote? If you approve it, I can start implementing it on js-ipfs-api and create the tests here 😄

daviddias · 2018-02-06T18:40:46Z

@hacdias go for it :)

hacdias · 2018-02-07T08:06:40Z

I think this is ready @diasdavid :)

ghost assigned hacdias Jan 30, 2018

ghost added the in progress label Jan 30, 2018

hacdias mentioned this pull request Jan 30, 2018

feat(breaking change): use stream on stats.bw ipfs-inactive/js-ipfs-http-client#686

Merged

hacdias requested review from dignifiedquire and daviddias January 30, 2018 22:10

hacdias mentioned this pull request Jan 30, 2018

feat: stats API (stats.bitswap and stats.repo) ipfs/js-ipfs#1198

Merged

34 tasks

hacdias force-pushed the stats-bw branch 3 times, most recently from c9aedbf to 17aebbf Compare January 31, 2018 08:34

victorb reviewed Jan 31, 2018

View reviewed changes

daviddias suggested changes Jan 31, 2018

View reviewed changes

victorb reviewed Feb 1, 2018

View reviewed changes

daviddias suggested changes Feb 5, 2018

View reviewed changes

hacdias force-pushed the stats-bw branch 2 times, most recently from 4432dc9 to c76eab4 Compare February 6, 2018 14:29

feat: add stats.bwPullStream and stats.bwReadableStream

3ecff7f

hacdias force-pushed the stats-bw branch from 5967ec5 to 3ecff7f Compare February 7, 2018 08:05

hacdias changed the title ~~feat(breaking change): use stream on stats.bw~~ feat(breaking change): stats.bwReadableStream and stats.bwPullStream Feb 7, 2018

daviddias approved these changes Feb 7, 2018

View reviewed changes

daviddias merged commit 4421eb2 into master Feb 7, 2018

daviddias deleted the stats-bw branch February 7, 2018 09:42

ghost removed the in progress label Feb 7, 2018


		`stat` is, in both cases, an Object containing the following keys:

		- `totalIn` - is a string containing a uint64 in bytes.

feat(breaking change): stats.bwReadableStream and stats.bwPullStream #211

feat(breaking change): stats.bwReadableStream and stats.bwPullStream #211

Conversation

hacdias commented Jan 30, 2018 • edited Loading

hacdias commented Jan 31, 2018

victorb commented Jan 31, 2018

hacdias commented Jan 31, 2018

hacdias commented Jan 31, 2018

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

hacdias Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacdias commented Jan 31, 2018

hacdias commented Jan 31, 2018

daviddias commented Jan 31, 2018

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 Feb 1, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacdias commented Feb 6, 2018 • edited Loading

daviddias commented Feb 6, 2018

hacdias commented Feb 7, 2018

hacdias commented Jan 30, 2018 •

edited

Loading

hacdias Jan 31, 2018 •

edited

Loading

victorb Feb 1, 2018 •

edited

Loading

hacdias commented Feb 6, 2018 •

edited

Loading