Cleanup logging 1 #1359

rht · 2015-06-12T00:25:27Z

Remove prefixlog (totally unused) and go-logging (backend of util/log).
Now there are only util/log, eventlog, logrus left (util/log and eventlog use logrus as backend).
Side-effect: the logs don't have shortfile (because logrus doesn't use it), and the formatting is less flexible than go-logging. The former needs a fork, the latter only needs a custom formatter.
#593

jbenet · 2015-06-12T02:59:35Z

util/log.go

@@ -3,23 +3,21 @@ package util
 import (
 	"os"

-	logging "github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/whyrusleeping/go-logging"
+	"github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/Sirupsen/logrus"


@whyrusleeping had you switched to go-logging on purpose?

i was under the assumption that we were using go-logging.

@whyrusleeping do you want to keep go-logging, or is logrus ok? (i dont care either way)

@jbenet irc logs and past issues say logrus is the preferred backend. Some of the logs are even in pure logrus. It's just that eventlog heavy logging needs to be toned down somehow.

The argument for go-logging is that it is more lightweight than logrus and more compatible with standard logging extra features (e.g. logrus's setflags feature is still pending).

keep go-logging is euphemism for kill logrus (as this will be the course of action written in the commit message, not the former)

@rht at this point, you know more about them to make a better decision on the two. I'd say take the path of least resistance if theyre about the same.

rht · 2015-06-14T07:59:49Z

"completely API compatible with the standard library logger" but doesn't have stdlib SetFlags is indeed surprising. sirupsen/logrus#63

whyrusleeping · 2015-06-16T22:58:42Z

@rht, status update here?

jbenet · 2015-06-16T23:41:35Z

@whyrusleeping said:

@rht at this point, you know more about them to make a better decision on the two. I'd say take the path of least resistance if theyre about the same.

so let's go with whatever you think is best, @rht. is that logrus + the changes here?

rht · 2015-06-17T03:03:10Z

@whyrusleeping There is a very specific error that happens here when I switch Critical{,f} with Fatalf{,f}. I'm bisecting on my previous commit (now that gpe is not running). The err is erring.

Yes it is logrus + changes.

jbenet · 2015-06-17T07:14:10Z

@rht is the problem these:

17:36:47.598 CRITI   boguskey: TestBogusPrivateKey.Sign -- this better be a test! key.go:54
FAIL    github.com/ipfs/go-ipfs/test/integration    11.337s

??

it's because in logrus, log.Fatalf calls os.Exit(1) after logging:

// Calls os.Exit(1) after logging
log.Fatal("Bye.")
// Calls panic() after logging
log.Panic("I'm bailing.")
https://github.com/Sirupsen/logrus#level-logging

Our Critical does not cause that sort of failure, so we can probably just change all occurrences of log.Critical{,f} to log.Error{,f}

rht · 2015-06-17T07:56:19Z

(I c, I did try changing to error, but not to all files...). So Fatal will be used when in the lines there are explicit exits.

rht · 2015-06-17T14:44:35Z

Test passes on the commit "Replace Critical with Error". The latest fail is only on the linux.

rht · 2015-06-17T15:55:11Z

Will close #592, #1217. Cleanup logging 2 will be for #593.

jbenet · 2015-06-18T00:28:10Z

exchange/bitswap/wantmanager.go

@@ -96,7 +96,7 @@ func (pm *WantManager) SendBlock(ctx context.Context, env *engine.Envelope) {
 	log.Infof("Sending block %s to %s", env.Peer, env.Block)
 	err := pm.network.SendMessage(ctx, env.Peer, msg)
 	if err != nil {
-		log.Noticef("sendblock error: %s", err)
+		log.Warningf("sendblock error: %s", err)


this should probably just be log.Infof -- warnings are things the user may need to be conscious about

jbenet · 2015-06-18T00:29:31Z

last couple changes. otherwise, this LGTM!

jbenet · 2015-06-18T00:30:04Z

@rht, make sure to rebase on latest master. we might merge @whyrusleeping's #1382 if it gets finished first.

License: MIT Signed-off-by: rht <[email protected]>

Except when there is an explicit os.Exit(1) after the Critical line, then replace with Fatal{,f}. golang's log and logrus already call os.Exit(1) by default with Fatal. License: MIT Signed-off-by: rht <[email protected]>

And substitute the lines using Notice{,f} with Info{,f} License: MIT Signed-off-by: rht <[email protected]>

License: MIT Signed-off-by: rht <[email protected]>

and exchange/bitswap/testutils.go License: MIT Signed-off-by: rht <[email protected]>

rht · 2015-06-18T03:04:39Z

Fixed + rebased

jbenet · 2015-06-18T11:47:26Z

@rht this LGTM! and 👍 on all the go linting.

Cleanup logging 1

jbenet · 2015-06-18T11:49:19Z

thanks @rht!

rht · 2015-06-18T12:39:08Z

The golint was an excuse to rerun the travis.

jbenet added the backlog label Jun 12, 2015

jbenet reviewed Jun 12, 2015
View reviewed changes

rht force-pushed the cleanup-logging branch from 68b72a8 to 5c53fda Compare June 14, 2015 07:57

rht force-pushed the cleanup-logging branch 10 times, most recently from 8af084f to bc0183f Compare June 16, 2015 17:29

rht force-pushed the cleanup-logging branch 8 times, most recently from f9e4205 to 20b87d1 Compare June 17, 2015 14:23

rht force-pushed the cleanup-logging branch from 20b87d1 to c3acb21 Compare June 17, 2015 16:04

jbenet reviewed Jun 18, 2015
View reviewed changes

rht added 7 commits June 18, 2015 10:03

Decimate prefixlog

6bc9086

License: MIT Signed-off-by: rht <[email protected]>

Replace Critical{,f} with Error{,f}

78b6cc5

Except when there is an explicit os.Exit(1) after the Critical line, then replace with Fatal{,f}. golang's log and logrus already call os.Exit(1) by default with Fatal. License: MIT Signed-off-by: rht <[email protected]>

Remove Notice{,f} logging interface

4b086ef

And substitute the lines using Notice{,f} with Info{,f} License: MIT Signed-off-by: rht <[email protected]>

Add panic as loglevel in log config

84f974e

License: MIT Signed-off-by: rht <[email protected]>

Decimate go-logging & replace with logrus backend

9ceca98

License: MIT Signed-off-by: rht <[email protected]>

Remove github.com/whyrusleeping/go-logging Godeps

2e9ec3d

License: MIT Signed-off-by: rht <[email protected]>

golint util/, thirdparty/

18f5fce

and exchange/bitswap/testutils.go License: MIT Signed-off-by: rht <[email protected]>

rht force-pushed the cleanup-logging branch from c3acb21 to 18f5fce Compare June 18, 2015 03:04

jbenet added a commit that referenced this pull request Jun 18, 2015

Merge pull request #1359 from rht/cleanup-logging

152247d

Cleanup logging 1

jbenet merged commit 152247d into ipfs:master Jun 18, 2015

jbenet removed the backlog label Jun 18, 2015

This was referenced Jun 18, 2015

Prefix log filename unhelpful #592

Closed

logs that dont get printed still render #1217

Closed

jbenet mentioned this pull request Jun 18, 2015

Sprint L ipfs/team-mgmt#13

Closed

55 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup logging 1 #1359

Cleanup logging 1 #1359

rht commented Jun 12, 2015

jbenet Jun 12, 2015

whyrusleeping Jun 15, 2015

jbenet Jun 16, 2015

rht Jun 16, 2015

rht Jun 16, 2015

whyrusleeping Jun 16, 2015

rht commented Jun 14, 2015

whyrusleeping commented Jun 16, 2015

jbenet commented Jun 16, 2015

rht commented Jun 17, 2015

jbenet commented Jun 17, 2015

rht commented Jun 17, 2015

rht commented Jun 17, 2015

rht commented Jun 17, 2015

jbenet Jun 18, 2015

jbenet commented Jun 18, 2015

jbenet commented Jun 18, 2015

rht commented Jun 18, 2015

jbenet commented Jun 18, 2015

jbenet commented Jun 18, 2015

rht commented Jun 18, 2015

Cleanup logging 1 #1359

Cleanup logging 1 #1359

Conversation

rht commented Jun 12, 2015

jbenet Jun 12, 2015

Choose a reason for hiding this comment

whyrusleeping Jun 15, 2015

Choose a reason for hiding this comment

jbenet Jun 16, 2015

Choose a reason for hiding this comment

rht Jun 16, 2015

Choose a reason for hiding this comment

rht Jun 16, 2015

Choose a reason for hiding this comment

whyrusleeping Jun 16, 2015

Choose a reason for hiding this comment

rht commented Jun 14, 2015

whyrusleeping commented Jun 16, 2015

jbenet commented Jun 16, 2015

rht commented Jun 17, 2015

jbenet commented Jun 17, 2015

rht commented Jun 17, 2015

rht commented Jun 17, 2015

rht commented Jun 17, 2015

jbenet Jun 18, 2015

Choose a reason for hiding this comment

jbenet commented Jun 18, 2015

jbenet commented Jun 18, 2015

rht commented Jun 18, 2015

jbenet commented Jun 18, 2015

jbenet commented Jun 18, 2015

rht commented Jun 18, 2015