Refactor logger #3924

balamurugana · 2017-03-18T03:50:48Z

This patch fixes below

Previously fatalIf() never writes log other than first logging target.
quiet flag is not honored to show progress messages other than startup messages.
Removes console package usage for progress messages.

Description

Motivation and Context

How Has This Been Tested?

make test
manual

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2017-03-18T03:50:50Z

@balamurugana, thanks for your PR! By analyzing the history of the files in this pull request, we identified @harshavardhana, @krishnasrinivas and @abperiasamy to be potential reviewers.

codecov-io · 2017-03-18T04:06:23Z

Codecov Report

Merging #3924 into master will increase coverage by 0.17%.
The diff coverage is 64.17%.

@@            Coverage Diff             @@
##           master    #3924      +/-   ##
==========================================
+ Coverage   68.44%   68.61%   +0.17%     
==========================================
  Files         173      173              
  Lines       23382    23407      +25     
==========================================
+ Hits        16003    16060      +57     
+ Misses       6317     6292      -25     
+ Partials     1062     1055       -7

Impacted Files	Coverage Δ
cmd/prepare-storage.go	`62.06% <0%> (ø)`	⬆️
cmd/gateway-main.go	`0% <0%> (ø)`	⬆️
cmd/namespace-lock.go	`73.61% <100%> (ø)`	⬆️
cmd/config-v16.go	`85.22% <100%> (-0.15%)`	⬇️
cmd/credential.go	`92.45% <100%> (+31.88%)`	⬆️
cmd/config-dir.go	`100% <100%> (+6.81%)`	⬆️
cmd/config-migrate.go	`60.19% <100%> (+0.08%)`	⬆️
cmd/update-main.go	`53.59% <16.66%> (+1.98%)`	⬆️
cmd/file-logger.go	`16.66% <16.66%> (ø)`
cmd/console-logger.go	`57.14% <57.14%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11e15f9...729bc0d. Read the comment docs.

vadmeste · 2017-03-20T18:24:35Z

cmd/config-v15.go

@@ -261,8 +259,11 @@ func validateConfig() error {
 	}

 	// Validate logger field
-	if err := srvCfg.Logger.Validate(); err != nil {
-		return err
+	if srvCfg.Logger != nil {


Why not using the srvCfg.Logger.Validate() style here ?

Do you want below?

func (l *loggers) Validate() (err error) { if l != nil { fileLogger := srvCfg.Logger.GetFile() if fileLogger.Enable && fileLogger.Filename == "" { err := errors.New("Missing filename for enabled file logger") } } return err }

?

@balamurugana yes, 👍

vadmeste · 2017-03-20T18:26:44Z

cmd/file-logger.go

+	}
+
+	if _, err = logger.file.Write(msgBytes); err != nil {
+		return nil


Do you mean 'return err' here ?

Yes. Fixing it.

vadmeste · 2017-03-20T18:31:05Z

cmd/log-level.go

+
+// IsHigher - check whether given log level is higher or not.
+func (level LogLevel) IsHigher(l logrus.Level) bool {
+	return l > level.Level


Debug > Fatal ?

Please add a comment if yes

harshavardhana · 2017-03-21T08:37:26Z

any reviews here @donatello ?

donatello · 2017-03-21T08:57:00Z

any reviews here @donatello ?

Reviewing now.

donatello · 2017-03-21T09:02:10Z

cmd/gateway-main.go

@@ -141,12 +142,9 @@ func gatewayMain(ctx *cli.Context) {
 	// default to "us-east-1"
 	err := newGatewayConfig(accessKey, secretKey, "us-east-1")
 	if err != nil {
-		console.Fatalf("Unable to initialize gateway config. Error: %s", err)
+		fatalIf(err, "Unable to initialize gateway config")
 	}


If block not needed here. The fatalIf() can replace the if block.

donatello · 2017-03-21T09:02:31Z

cmd/gateway-main.go

@@ -155,7 +153,7 @@ func gatewayMain(ctx *cli.Context) {

 	newObject, err := newGatewayLayer(backendType, accessKey, secretKey)
 	if err != nil {
-		console.Fatalf("Unable to initialize gateway layer. Error: %s", err)
+		fatalIf(err, "Unable to initialize gateway layer")


If block not needed here. The fatalIf() can replace the if block.

donatello · 2017-03-21T09:03:49Z

cmd/gateway-main.go

@@ -193,7 +191,7 @@ func gatewayMain(ctx *cli.Context) {
 			cert, key = getPublicCertFile(), getPrivateKeyFile()
 		}
 		if aerr := apiServer.ListenAndServe(cert, key); aerr != nil {
-			console.Fatalf("Failed to start minio server. Error: %s\n", aerr)
+			fatalIf(err, "Failed to start minio server")
 		}


It should be aerr instead of err in fatalIf, and this block can be rewritten in fewer lines as:

aerr := apiServer.ListenAndServe(cert, key) fatalIf(aerr, "Failed to start minio server")

donatello · 2017-03-21T09:41:31Z

cmd/file-logger.go

+
+	str += ":" + logger.Filename
+	return str
+}


This function is better implemented with Sprintf, than by so many concatenations:

enableStr := "enabled" if !logger.Enable { enableStr = "disabled" } return fmt.Sprintf("file:%s:%s:%s", enableStr, logger.Level.String(), logger.Filename)

Arguably this is more readable.

donatello · 2017-03-21T09:53:44Z

cmd/logger.go

+// Fire - log entry handler to save logs.
+func (log *Logger) Fire(entry *logrus.Entry) (err error) {
+	if err = log.consoleTarget.Fire(entry); err != nil {
+		log.Printf("Unable to log to console target. %s\n", err)


Shouldn't we return here, since we found an error?

Otherwise, the next for block over-writes the error we found here.

Its OK to print the error to the console and continue. Returning error is not helpful, but required to work with logrus interface compatibility.

donatello · 2017-03-21T11:02:50Z

cmd/console-logger.go

+	}
+
+	str += ":" + logger.Level.String()
+	return str


This part can also be improved using Sprintf.

donatello

LGTM - one last place remaining where string concatenation can be improved with Sprintf.

This patch fixes below * Previously fatalIf() never writes log other than first logging target. * quiet flag is not honored to show progress messages other than startup messages. * Removes console package usage for progress messages.

harshavardhana requested review from vadmeste and donatello March 20, 2017 17:53

harshavardhana added the dev-review label Mar 20, 2017

vadmeste reviewed Mar 20, 2017

View reviewed changes

balamurugana force-pushed the clean-logger branch 2 times, most recently from c073ec2 to 02ffe07 Compare March 21, 2017 00:51

donatello reviewed Mar 21, 2017

View reviewed changes

balamurugana force-pushed the clean-logger branch from 02ffe07 to 6ce0e10 Compare March 21, 2017 10:23

donatello reviewed Mar 21, 2017

View reviewed changes

donatello approved these changes Mar 21, 2017

View reviewed changes

balamurugana force-pushed the clean-logger branch 5 times, most recently from 5f0bf89 to 4aa12ce Compare March 23, 2017 16:13

Refactor logger

729bc0d

This patch fixes below * Previously fatalIf() never writes log other than first logging target. * quiet flag is not honored to show progress messages other than startup messages. * Removes console package usage for progress messages.

balamurugana force-pushed the clean-logger branch from 4aa12ce to 729bc0d Compare March 23, 2017 16:22

harshavardhana approved these changes Mar 23, 2017

View reviewed changes

harshavardhana merged commit d3cb79a into minio:master Mar 23, 2017

balamurugana deleted the clean-logger branch July 14, 2017 05:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor logger #3924

Refactor logger #3924

balamurugana commented Mar 18, 2017

mention-bot commented Mar 18, 2017

codecov-io commented Mar 18, 2017 •

edited

Loading

vadmeste Mar 20, 2017

balamurugana Mar 21, 2017 •

edited

Loading

vadmeste Mar 21, 2017

vadmeste Mar 20, 2017

balamurugana Mar 21, 2017

vadmeste Mar 20, 2017

balamurugana Mar 21, 2017

harshavardhana commented Mar 21, 2017

donatello commented Mar 21, 2017

donatello Mar 21, 2017

balamurugana Mar 21, 2017

donatello Mar 21, 2017

balamurugana Mar 21, 2017

donatello Mar 21, 2017

balamurugana Mar 21, 2017

donatello Mar 21, 2017

balamurugana Mar 21, 2017

donatello Mar 21, 2017

balamurugana Mar 21, 2017

donatello Mar 21, 2017

donatello Mar 21, 2017

balamurugana Mar 21, 2017

donatello left a comment

Refactor logger #3924

Refactor logger #3924

Conversation

balamurugana commented Mar 18, 2017

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mention-bot commented Mar 18, 2017

codecov-io commented Mar 18, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

balamurugana Mar 21, 2017 • 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

harshavardhana commented Mar 21, 2017

donatello commented Mar 21, 2017

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

donatello left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 18, 2017 •

edited

Loading

balamurugana Mar 21, 2017 •

edited

Loading