Add Event Logging to Add Command #4465

frrist · 2017-12-07T20:52:26Z

License: MIT
Signed-off-by: ForrestWeston forrest@protocol.ai

Stebalien

LGTM.

My only worry would be performance but I'm pretty sure this won't be the bottleneck. However, would you mind running a small local before and after benchmark? E.g., add the go-ipfs git repo to an empty repo before and after (and then maybe try adding a large file).

whyrusleeping · 2017-12-07T22:13:27Z

We really need benchmarks...

frrist · 2017-12-07T22:27:32Z

@Stebalien
time ipfs add -r go-ipfs
Before Changes (bbdbd0a):

0.10user 0.07system 0:04.57elapsed 3%CPU (0avgtext+0avgdata 29340maxresident)k
0inputs+0outputs (0major+2198minor)pagefaults 0swaps
0.12user 0.06system 0:04.87elapsed 3%CPU (0avgtext+0avgdata 28816maxresident)k
0inputs+0outputs (0major+2237minor)pagefaults 0swaps
0.15user 0.03system 0:04.82elapsed 3%CPU (0avgtext+0avgdata 28732maxresident)k
0inputs+0outputs (0major+2274minor)pagefaults 0swaps

After Changes (d84e5a2):

0.09user 0.08system 0:04.83elapsed 3%CPU (0avgtext+0avgdata 29112maxresident)k
0inputs+0outputs (0major+2204minor)pagefaults 0swaps
0.12user 0.05system 0:04.70elapsed 3%CPU (0avgtext+0avgdata 26488maxresident)k
0inputs+0outputs (0major+2231minor)pagefaults 0swaps
0.12user 0.06system 0:04.65elapsed 3%CPU (0avgtext+0avgdata 26372maxresident)k
0inputs+0outputs (0major+2235minor)pagefaults 0swaps

whyrusleeping · 2017-12-07T23:30:01Z

Cool, doesnt appear to be any noticeable difference there.

whyrusleeping · 2017-12-07T23:30:26Z

Could you post an image here of the tracing output that this generates?

frrist · 2017-12-08T00:30:51Z

Sorry it got cut off a bit, this on is from the above benchmarks.
add-go-ipfs

This is one from my bin/ dir
https://ipfs.io/ipfs/QmUMJCmHRQ2svFjWjXfiC5y5XCHG2qj4TvA4Jbz7XKFvaE/Selection_008.png

Also, its not shown in the screenshots, but you can view the tags (like filename) if you click the spans

frrist · 2017-12-08T00:58:21Z

core/coreunix/add.go

@@ -153,112 +153,148 @@ func (adder Adder) add(reader io.Reader) (node.Node, error) {
 }

 func (adder *Adder) RootNode() (node.Node, error) {


Does this make sense as well? It feels cleaner, it compiles and the tests pass.

diff --git a/core/coreunix/add.go b/core/coreunix/add.go index 516b76d44..090ecee27 100644 --- a/core/coreunix/add.go +++ b/core/coreunix/add.go @@ -152,40 +152,42 @@ func (adder Adder) add(reader io.Reader) (node.Node, error) { return balanced.BalancedLayout(params.New(chnk)) } -func (adder *Adder) RootNode() (node.Node, error) { +func (adder *Adder) RootNode() (root node.Node, err error) { eip := log.EventBegin(adder.ctx, "RootNode") - defer eip.Done() + defer func() { + if root != nil { + eip.Append(root.Cid()) + } + if err != nil { + eip.SetError(err) + } + eip.Done() + }() // for memoizing - if adder.root != nil { - eip.Append(adder.root.Cid()) - return adder.root, nil + root = adder.root + if root != nil { + return root, nil } mr, err := adder.mfsRoot() if err != nil { - eip.SetError(err) return nil, err } - root, err := mr.GetValue().GetNode() + root, err = mr.GetValue().GetNode() if err != nil { - eip.SetError(err) return nil, err } // if not wrapping, AND one root file, use that hash as root. if !adder.Wrap && len(root.Links()) == 1 { - nd, err := root.Links()[0].GetNode(adder.ctx, adder.dagService) + root, err = root.Links()[0].GetNode(adder.ctx, adder.dagService) if err != nil { - eip.SetError(err) return nil, err } - - root = nd } adder.root = root - eip.Append(root.Cid()) - return root, err + return }

I'm fine with naming the return values for the purpose of accessing them from within the defer statement. However, I wouldn't use the implicit return feature (IMO, it was a mistake). How about:

diff --git a/core/coreunix/add.go b/core/coreunix/add.go index 516b76d44..090ecee27 100644 --- a/core/coreunix/add.go +++ b/core/coreunix/add.go @@ -152,37 +152,38 @@ func (adder Adder) add(reader io.Reader) (node.Node, error) { return balanced.BalancedLayout(params.New(chnk)) } -func (adder *Adder) RootNode() (node.Node, error) { +func (adder *Adder) RootNode() (root node.Node, err error) { eip := log.EventBegin(adder.ctx, "RootNode") - defer eip.Done() + defer func() { + if root != nil { + eip.Append(root.Cid()) + } + if err != nil { + eip.SetError(err) + } + eip.Done() + }() // for memoizing - if adder.root != nil { - eip.Append(adder.root.Cid()) - return adder.root, nil + if addr.root != nil { + return addr.root, nil } mr, err := adder.mfsRoot() if err != nil { - eip.SetError(err) return nil, err } - root, err := mr.GetValue().GetNode() + root, err = mr.GetValue().GetNode() if err != nil { - eip.SetError(err) return nil, err } // if not wrapping, AND one root file, use that hash as root. if !adder.Wrap && len(root.Links()) == 1 { - nd, err := root.Links()[0].GetNode(adder.ctx, adder.dagService) + root, err = root.Links()[0].GetNode(adder.ctx, adder.dagService) if err != nil { - eip.SetError(err) return nil, err } - - root = nd } adder.root = root - eip.Append(root.Cid()) return root, err }

agreed, the implicit return thing was a terrible choice by the go team

Alright, I can make these changes on the rest of the methods here too then. I am not sure if I should make that a separate commit or rebase. What would you prefer?

either way, i'm not picky. Though I do appreciate smaller numbers of commits

frrist · 2017-12-12T23:53:38Z

blockservice/blockservice.go

 	return getBlocks(ctx, ks, s.blockstore, s.exchange)
 }

 func getBlocks(ctx context.Context, ks []*cid.Cid, bs blockstore.Blockstore, f exchange.Fetcher) <-chan blocks.Block {
 	out := make(chan blocks.Block)
 	go func() {
+		defer logging.MaybeFinishEvent(ctx)


~~This PR needs to go first for this functionality.~~

frrist · 2018-01-18T23:12:12Z

Note: blocked until changes in go-log are bubbled up - CI will fail till then

Stebalien · 2018-01-31T04:54:18Z

Note: blocked until changes in go-log are bubbled up - CI will fail till then

Now that everything else has been unblocked, what exact changes do you still need (if any)?

frrist · 2018-01-31T15:25:10Z

@Stebalien we will need to gx publish the changes in go-log and update the version that go-ipfs uses (currently using 1.2.0).

The changes in go-log include instrumenting it with opentracing, as well as adding the DoneWithErr method and MaybeFinishEvent method (basically all the changes made to go-log in prs 8 - 16). I can start that process now.

CI fails because the aforementioned methods are missing from go-log and this branch uses them.

Changes Needed:
ipfs/go-log#17
#4637

License: MIT Signed-off-by: ForrestWeston <forrest@protocol.ai>

Named return params and defer func to complete event License: MIT Signed-off-by: ForrestWeston <forrest@protocol.ai>

Contextual logging for async methods License: MIT Signed-off-by: ForrestWeston <forrest@protocol.ai>

frrist · 2019-01-22T18:24:09Z

Closing, will reopen when this becomes a priority -- I currently lack the bandwidth to push this forward.

frrist requested review from Stebalien, whyrusleeping and Kubuxu December 7, 2017 20:52

ghost assigned frrist Dec 7, 2017

ghost added the status/in-progress In progress label Dec 7, 2017

Stebalien approved these changes Dec 7, 2017

View reviewed changes

frrist commented Dec 8, 2017

View reviewed changes

frrist force-pushed the feat/coreunix/event-log branch from 2483a55 to 2822474 Compare December 11, 2017 20:37

frrist commented Dec 12, 2017

View reviewed changes

frrist force-pushed the feat/coreunix/event-log branch from 7ffb1d3 to 5ff4e7a Compare December 12, 2017 23:56

frrist force-pushed the feat/coreunix/event-log branch 3 times, most recently from 1f8003a to aef5ab7 Compare December 21, 2017 23:01

frrist force-pushed the feat/coreunix/event-log branch 2 times, most recently from bd44804 to 28aa9e2 Compare January 9, 2018 23:15

frrist force-pushed the feat/coreunix/event-log branch 2 times, most recently from 9949fba to 3134df7 Compare January 18, 2018 23:10

frrist force-pushed the feat/coreunix/event-log branch 2 times, most recently from 5cdbf8d to 0215e8f Compare January 26, 2018 18:46

frrist force-pushed the feat/coreunix/event-log branch from 0215e8f to ddc37ae Compare January 31, 2018 15:40

frrist and others added 2 commits February 28, 2018 13:29

Add Event Logging to Add Command

7cf285a

License: MIT Signed-off-by: ForrestWeston <forrest@protocol.ai>

Add Benchmark to Recursive Add

b5d780d

License: MIT Signed-off-by: ForrestWeston <forrest@protocol.ai>

frrist force-pushed the feat/coreunix/event-log branch from ddc37ae to 0426dd1 Compare February 28, 2018 21:37

frrist and others added 2 commits February 28, 2018 13:49

Standardize the way we log events in resolver

cbc584e

Named return params and defer func to complete event License: MIT Signed-off-by: ForrestWeston <forrest@protocol.ai>

Add event logging to blockservce get methods

00d0582

Contextual logging for async methods License: MIT Signed-off-by: ForrestWeston <forrest@protocol.ai>

frrist force-pushed the feat/coreunix/event-log branch from 0426dd1 to 00d0582 Compare February 28, 2018 21:50

frrist closed this Jan 22, 2019

ghost removed the status/in-progress In progress label Jan 22, 2019

hacdias deleted the feat/coreunix/event-log branch May 9, 2023 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Event Logging to Add Command #4465

Add Event Logging to Add Command #4465

frrist commented Dec 7, 2017

Stebalien left a comment

whyrusleeping commented Dec 7, 2017

frrist commented Dec 7, 2017 •

edited

Loading

whyrusleeping commented Dec 7, 2017

whyrusleeping commented Dec 7, 2017

frrist commented Dec 8, 2017 •

edited

Loading

frrist Dec 8, 2017

Stebalien Dec 8, 2017 •

edited

Loading

whyrusleeping Dec 8, 2017

frrist Dec 9, 2017

whyrusleeping Dec 9, 2017

frrist Dec 12, 2017 •

edited

Loading

frrist commented Jan 18, 2018 •

edited

Loading

Stebalien commented Jan 31, 2018

frrist commented Jan 31, 2018 •

edited

Loading

frrist commented Jan 22, 2019

		@@ -153,112 +153,148 @@ func (adder Adder) add(reader io.Reader) (node.Node, error) {
		}

		func (adder *Adder) RootNode() (node.Node, error) {

Add Event Logging to Add Command #4465

Add Event Logging to Add Command #4465

Conversation

frrist commented Dec 7, 2017

Stebalien left a comment

Choose a reason for hiding this comment

whyrusleeping commented Dec 7, 2017

frrist commented Dec 7, 2017 • edited Loading

whyrusleeping commented Dec 7, 2017

whyrusleeping commented Dec 7, 2017

frrist commented Dec 8, 2017 • edited Loading

frrist Dec 8, 2017

Choose a reason for hiding this comment

Stebalien Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

whyrusleeping Dec 8, 2017

Choose a reason for hiding this comment

frrist Dec 9, 2017

Choose a reason for hiding this comment

whyrusleeping Dec 9, 2017

Choose a reason for hiding this comment

frrist Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

frrist commented Jan 18, 2018 • edited Loading

Stebalien commented Jan 31, 2018

frrist commented Jan 31, 2018 • edited Loading

frrist commented Jan 22, 2019

frrist commented Dec 7, 2017 •

edited

Loading

frrist commented Dec 8, 2017 •

edited

Loading

Stebalien Dec 8, 2017 •

edited

Loading

frrist Dec 12, 2017 •

edited

Loading

frrist commented Jan 18, 2018 •

edited

Loading

frrist commented Jan 31, 2018 •

edited

Loading