feat: debug logging #195

richardschneider · 2018-01-29T02:32:49Z

Just some simple debug statements to figure out what is happening.

dryajov · 2018-01-29T02:40:38Z

@richardschneider thanks! We have to be careful to scope debug output correctly and only use that scope when debugging, otherwise it will turn ipfs logging, which will break our stdout/stderr parsing.

richardschneider · 2018-01-29T02:58:54Z

@dryajov not sure what you are saying?

These are debug statements for ipfsd-ctl. it has nothing to do with issue #193

dryajov

LGTM besides the comment below

dryajov · 2018-01-30T04:44:04Z

package.json

    "detect-node": "^2.0.3",
    "hapi": "^16.6.2",
    "hat": "0.0.3",
+    "ipfs-api": "^17.3.0",


this needs to be moved to devDependencies

@dryajov why devDep and not dep?

@dryajov Doesn't ipfsd-ctl need ipfs-api to control the remote daemon?

@richardschneider it does, but we've had issues bundling deps like that before (it was ipfs proper in that case, but it applies to ipfs-api as well).

Is it a circular reference, both need each other?

Why does it apply to ipfs-api as well? ipfs caused issues because it brought in some native dependencies that were causing trouble for ipfs-desktop. It was ok to move ipfs to devDep because we moved it to be injected by the user (the other option being go-ipfs-dep). However, ipfs-api is always used and doesn't make sense that a user needs to inject as well.

IMO, we should not bundle neither ipfs not ipfs-api and let the consumer bundle their own, because we can't guarantee which version is going to be ultimately used by ipfsd-ctl in either case.

However, ipfs-api is always used and doesn't make sense that a user needs to inject as well.

So is js-ipfs, the consuming project might have a different version bundled already.

I'm fine with bundling ipfs-api, I don't think it's going to cause any issues, but allowing the consuming project use their own, IMO gives it more control. It's not a big issue either way, because the consumer can alway explicitly use their own ipfs-api version with IpfsApi(ipfsd.apiAddr).

Hey guys, I just want some debug logging. Please make a decision.

I'm fine including it 👍

I'm fine with bundling ipfs-api, I don't think it's going to cause any issues, but allowing the consuming project use their own, IMO gives it more control.

What kind of control do you expect? ipfs-api is the client library to talk with daemons.

If I understand correctly, the only reason why we don't pack in ipfs and npm-go-ipfs-dep by default is due to issues with electron, not because of control.

richardschneider · 2018-01-31T09:59:34Z

@diasdavid are you happy to merge?

daviddias · 2018-01-31T23:30:06Z

@richardschneider I'm now worried that the lack including ipfs-api here was enabling the tests on js-ipfs-api to pass because of how require works. Could you double check and test that js-ipfs, js-ipfs-api and ipfs/interop tests pass with this version?

richardschneider · 2018-01-31T23:39:58Z

@diasdavid CI tests are failing all over the place.

Running js-ipfs, js-ipfs-api and ipfs-interop will take most of my day and then most likely yield false positive results. If the code base can not handle adding a few debug statements into a module, then we have very serious issues.

I am adding this PR, so I can figure out why tests are running so slow. Blaming Travis or sprinkling this.timeout is just not working.

dryajov · 2018-02-01T00:51:21Z

@richardschneider could you point to what's failing?

@diasdavid in js-ipfs-api we use the current js-ipfs-api from src/ and instantiate it explicitly using IpfsApi(ipfsd.apiAddr). js-ipfs and interop would pick up the latest ipfs-api from their dependencies. Not sure how this would have affected it, specially with later versions of npm doing ~~explicit~~ implicit dedup on those dependencies.

richardschneider · 2018-02-01T01:03:20Z

@dryajov See

Different repos and CIs with different types of random errors.

dryajov · 2018-02-01T01:08:04Z

@richardschneider I don't think they are related to this changes, I've seen them come up randomly at different times.

https://circleci.com/gh/ipfs/js-ipfs-api/1655?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link - I believe its been mentioned somewhere else might be due to webpack.

https://travis-ci.org/ipfs/js-ipfs/jobs/335828903 - has been a consistent random error on travis that pops up now and then, restarting the build fixes it. I'm sure there is a race condition somewhere that might be causing that and we should definitely investigate, but it's been around for a while and predates using ipfsd-ctl.

dryajov · 2018-02-01T01:18:11Z

To clarify, and make sure we're on the same page.

ipfsd-ctl was shipped with ipfs-api as a devDependency in hopes it would cause less conflicts and allow consuming projects use the latests available ipfs-api to them without having to update ipsd-ctl constantly
js-ipfs is also a devDependency, however the initial reason for moving it there was due to the native dependencies that were affecting electron builds, however, we have the same side effect as with ipfs-api - i.e. we can use the latest js-ipfs in consuming projects without having to ship a new versions of ipfsd-ctl every time

The concerns are that, shipping ipfs-api as a devDependency could affect testing across the different projects.

I don't believe that it's the case, because:

in js-ipfs-api, ipfsd-ctl uses the ipfs-api from its src/ dir, by explicitly instantiating it with IpfsApi(ipfsd.apiAddr)
in the all other projects, it should default to using the version of js-ipfs and ipfs-api declared in their dependencies

I must be missing something and I would love to see where the issues might lie.

daviddias · 2018-02-01T01:27:07Z

@dryajov you are missing all the projects that use ipfsd-ctl to spawn go-ipfs daemons that are not js-ipfs nor js-ipfs-api.

dryajov · 2018-02-01T01:46:51Z

@dryajov you are missing all the projects that use ipfsd-ctl to spawn go-ipfs daemons that are not js-ipfs nor js-ipfs-api.

Fair point, thanks for bringing that up. As I said, I'm fine moving ipfs-api back to dependencies.

I'm not clear on why it would affect existing tests in js-ipfs and interop though?

daviddias · 2018-02-01T01:54:58Z

I'm not clear on why it would affect existing tests in js-ipfs and interop though?

I'm not saying that it will. I just want us to be diligent and make sure that we test the dependents when we make breaking changes. I've seen multiple times PRs that "ready" to later see quick fixes all over when things start failing on the dependents. I know it is a cumbersome process and ideally CI would handle all of this, but until we have the tooling it has to be done manually.

dryajov · 2018-02-01T01:57:58Z

@diasdavid got you! Thanks for clarifying. Agree on being diligent - point taken.

richardschneider · 2018-02-02T18:48:49Z

@dryajov @diasdavid any progress?

richardschneider · 2018-02-03T04:02:08Z

@diasdavid could you just merge this (not release to NPM). I really need this to test some speedups.

Pretty please with a cherry on the top.

daviddias · 2018-02-06T05:48:08Z

@richardschneider what would merge to master enable you to do that on a branch it wouldn't?

daviddias

I actually do not see why this can't be merged

feat: debug logging

617e030

ghost assigned richardschneider Jan 29, 2018

ghost added the status/in-progress In progress label Jan 29, 2018

dryajov mentioned this pull request Jan 29, 2018

Debug logging #193

Closed

richardschneider mentioned this pull request Jan 29, 2018

Make spawning super fast! #190

Closed

richardschneider requested a review from daviddias January 29, 2018 19:09

daviddias requested a review from dryajov January 30, 2018 04:42

dryajov suggested changes Jan 30, 2018

View reviewed changes

dryajov approved these changes Jan 31, 2018

View reviewed changes

daviddias approved these changes Feb 6, 2018

View reviewed changes

Merge branch 'master' into debug-logging

9aaa6ef

ghost assigned daviddias Feb 6, 2018

daviddias merged commit 86b0ab0 into master Feb 6, 2018

ghost removed the status/in-progress In progress label Feb 6, 2018

daviddias deleted the debug-logging branch February 6, 2018 05:51

dryajov mentioned this pull request May 22, 2018

feat: bring your own API client #257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: debug logging #195

feat: debug logging #195

richardschneider commented Jan 29, 2018

dryajov commented Jan 29, 2018

richardschneider commented Jan 29, 2018

dryajov left a comment

dryajov Jan 30, 2018

daviddias Jan 30, 2018

richardschneider Jan 30, 2018

dryajov Jan 30, 2018

richardschneider Jan 30, 2018

daviddias Jan 30, 2018

dryajov Jan 31, 2018

richardschneider Jan 31, 2018

dryajov Jan 31, 2018

daviddias Jan 31, 2018

richardschneider commented Jan 31, 2018

daviddias commented Jan 31, 2018

richardschneider commented Jan 31, 2018

dryajov commented Feb 1, 2018 •

edited

Loading

richardschneider commented Feb 1, 2018

dryajov commented Feb 1, 2018

dryajov commented Feb 1, 2018

daviddias commented Feb 1, 2018

dryajov commented Feb 1, 2018

daviddias commented Feb 1, 2018 •

edited

Loading

dryajov commented Feb 1, 2018

richardschneider commented Feb 2, 2018

richardschneider commented Feb 3, 2018

daviddias commented Feb 6, 2018

daviddias left a comment

feat: debug logging #195

feat: debug logging #195

Conversation

richardschneider commented Jan 29, 2018

dryajov commented Jan 29, 2018

richardschneider commented Jan 29, 2018

dryajov 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

richardschneider commented Jan 31, 2018

daviddias commented Jan 31, 2018

richardschneider commented Jan 31, 2018

dryajov commented Feb 1, 2018 • edited Loading

richardschneider commented Feb 1, 2018

dryajov commented Feb 1, 2018

dryajov commented Feb 1, 2018

daviddias commented Feb 1, 2018

dryajov commented Feb 1, 2018

daviddias commented Feb 1, 2018 • edited Loading

dryajov commented Feb 1, 2018

richardschneider commented Feb 2, 2018

richardschneider commented Feb 3, 2018

daviddias commented Feb 6, 2018

daviddias left a comment

Choose a reason for hiding this comment

dryajov commented Feb 1, 2018 •

edited

Loading

daviddias commented Feb 1, 2018 •

edited

Loading