Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace log with logrus #52

Merged
merged 3 commits into from
Mar 18, 2022
Merged

replace log with logrus #52

merged 3 commits into from
Mar 18, 2022

Conversation

glightfoot
Copy link
Contributor

This replaces the std lib log with logrus to enable structured logging and levels. I attempted to guess what levels things should live at, as well as moved some things to structured fields, and changed the default output to stderr.

I can't get the tests to run correctly on my machine as they hang forever, but it seems to work.

Fixes #46

@kruskall
Copy link

kruskall commented Mar 7, 2022

I think zap would be a better/faster alternative

See sirupsen/logrus@4ecd9a6

@glightfoot
Copy link
Contributor Author

While zap is definitely faster, this only has a few calls and the performance difference is negligible. Logrus is in maintenance mode, but is stable and shouldn't need any updates. In this case, I think it makes for a simpler swap, but I opened #55 to show what it would look like with zap.

@glightfoot
Copy link
Contributor Author

Here's some example output (all to stderr):

$ dist/gci print pkg/gci/format.go --debug >/dev/null 
DEBUG[2022-03-10T14:31:49-08:00] Loaded file                                   file=pkg/gci/format.go
DEBUG[2022-03-10T14:31:49-08:00] Parsed imports in file                        imports="[\"bytes\" \"fmt\" \"strings\" \"github.com/daixiang0/gci/pkg/constants\" \"github.com/daixiang0/gci/pkg/gci/imports\" \"github.com/daixiang0/gci/pkg/gci/sections\" \"github.com/daixiang0/gci/pkg/gci/specificity\" \"github.com/sirupsen/logrus\"]"
DEBUG[2022-03-10T14:31:49-08:00] Matched import to section                     import="\"bytes\"" section=Standard
DEBUG[2022-03-10T14:31:49-08:00] Matched import to section                     import="\"fmt\"" section=Standard
DEBUG[2022-03-10T14:31:49-08:00] Matched import to section                     import="\"strings\"" section=Standard
DEBUG[2022-03-10T14:31:49-08:00] Matched import to section                     import="\"github.com/daixiang0/gci/pkg/constants\"" section=Default
DEBUG[2022-03-10T14:31:49-08:00] Matched import to section                     import="\"github.com/daixiang0/gci/pkg/gci/imports\"" section=Default
DEBUG[2022-03-10T14:31:49-08:00] Matched import to section                     import="\"github.com/daixiang0/gci/pkg/gci/sections\"" section=Default
DEBUG[2022-03-10T14:31:49-08:00] Matched import to section                     import="\"github.com/daixiang0/gci/pkg/gci/specificity\"" section=Default
DEBUG[2022-03-10T14:31:49-08:00] Matched import to section                     import="\"github.com/sirupsen/logrus\"" section=Default
DEBUG[2022-03-10T14:31:49-08:00] Formatting section with imports               imports="[\"bytes\" \"fmt\" \"strings\"]" section=Standard
DEBUG[2022-03-10T14:31:49-08:00] Formatting section with imports               imports="[\"github.com/daixiang0/gci/pkg/constants\" \"github.com/daixiang0/gci/pkg/gci/imports\" \"github.com/daixiang0/gci/pkg/gci/sections\" \"github.com/daixiang0/gci/pkg/gci/specificity\" \"github.com/sirupsen/logrus\"]" section=Default

@glightfoot
Copy link
Contributor Author

With the pretty print funcs, the output looks like this:

$ dist/gci print pkg/gci/format.go --debug  >/dev/null 
DEBUG[2022-03-10T14:49:50-08:00] Loaded file                                   file=pkg/gci/format.go
DEBUG[2022-03-10T14:49:50-08:00] Parsed imports in file                        imports="[bytes fmt strings github.com/daixiang0/gci/pkg/constants github.com/daixiang0/gci/pkg/gci/imports github.com/daixiang0/gci/pkg/gci/sections github.com/daixiang0/gci/pkg/gci/specificity github.com/sirupsen/logrus]"
DEBUG[2022-03-10T14:49:50-08:00] Matched import to section                     import=bytes section=Standard
DEBUG[2022-03-10T14:49:50-08:00] Matched import to section                     import=fmt section=Standard
DEBUG[2022-03-10T14:49:50-08:00] Matched import to section                     import=strings section=Standard
DEBUG[2022-03-10T14:49:50-08:00] Matched import to section                     import=github.com/daixiang0/gci/pkg/constants section=Default
DEBUG[2022-03-10T14:49:50-08:00] Matched import to section                     import=github.com/daixiang0/gci/pkg/gci/imports section=Default
DEBUG[2022-03-10T14:49:50-08:00] Matched import to section                     import=github.com/daixiang0/gci/pkg/gci/sections section=Default
DEBUG[2022-03-10T14:49:50-08:00] Matched import to section                     import=github.com/daixiang0/gci/pkg/gci/specificity section=Default
DEBUG[2022-03-10T14:49:50-08:00] Matched import to section                     import=github.com/sirupsen/logrus section=Default
DEBUG[2022-03-10T14:49:50-08:00] Formatting section with imports               imports="[bytes fmt strings]" section=Standard
DEBUG[2022-03-10T14:49:50-08:00] Formatting section with imports               imports="[github.com/daixiang0/gci/pkg/constants github.com/daixiang0/gci/pkg/gci/imports github.com/daixiang0/gci/pkg/gci/sections github.com/daixiang0/gci/pkg/gci/specificity github.com/sirupsen/logrus]" section=Default

@@ -28,6 +29,9 @@ func (e *Executor) newGciCommand(use, short, long string, aliases []string, stdI
if err != nil {
return err
}
if *debug {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we can support more levels more than debug.

@daixiang0
Copy link
Owner

Looks good overall, logrus is enough for now.

@glightfoot glightfoot marked this pull request as ready for review March 11, 2022 14:29
@glightfoot glightfoot force-pushed the logging branch 2 times, most recently from 9e1653d to 4d897de Compare March 16, 2022 15:50
Signed-off-by: Greg Lightfoot <glightfoot3@gmail.com>
Signed-off-by: Greg Lightfoot <glightfoot3@gmail.com>
Signed-off-by: Greg Lightfoot <glightfoot3@gmail.com>
@daixiang0 daixiang0 merged commit b301814 into daixiang0:master Mar 18, 2022
@daixiang0
Copy link
Owner

@glightfoot thanks for your contribution!

@ldez
Copy link
Contributor

ldez commented Mar 18, 2022

Hello, the problems with logrus are:

  • a large dependencies graph
  • when using gci as a library, because the logger is a singleton, it can be complex to setup.

In 99% of cases when you create a library, the init function must be prohibited.

It's better to create an abstraction around the logger with an interface and set up the logger in the main function instead of an init function.

daixiang0 added a commit that referenced this pull request Mar 18, 2022
daixiang0 added a commit that referenced this pull request Mar 18, 2022
@glightfoot glightfoot deleted the logging branch March 18, 2022 15:17
daixiang0 added a commit that referenced this pull request May 30, 2022
This reverts commit b301814.

Signed-off-by: Loong Dai <loong.dai@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GCI is way too verbose
4 participants