-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: adding zerolog logging to cosmovisor #10217
Conversation
should we use the tendermint logger interface to be consistent with app.go? |
|
||
var Logger zerolog.Logger | ||
|
||
func SetupLogging() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not meaning to block here, but is there a reason to use a global here instead of just returning a logger instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean to have a function which returns a private instance? Or putting it in a context?
I think this is fine for the moment - it's still better than using fmt.Print*
. Logger is not something that it's going to change during the runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spoo-bar - since nobody is blocking, feel free to add automerge
unless you want to expand in this thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not sure what you mean by private, but I mean just an instance, instead of relying on a global. So instead of requiring a SetupLogger
call, you just call logger := CreateLogger()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexanderbez creating an instance would require logger
to be passed around to each function.
having a global makes it easy to call it from anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
creating an instance would require logger to be passed around to each function.
Not really, as the caller would would just call the function when it needs a logger (this is what SDK modules do BTW).
Also, globals are generally the root of all evil. Avoid them if possible (in most contexts).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay.
But it would still require frequent reinitialization all over. Idk if having it has a global for a small component like cosmovisor is all that bad, is it?
|
||
var Logger zerolog.Logger | ||
|
||
func SetupLogging() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean to have a function which returns a private instance? Or putting it in a context?
I think this is fine for the moment - it's still better than using fmt.Print*
. Logger is not something that it's going to change during the runtime.
@marbar3778 Using Zerolog directly, there are some really nice APIs we can use directly. Using the tendermint logger doesn't bring anything extra except consistency. |
Codecov Report
@@ Coverage Diff @@
## master #10217 +/- ##
==========================================
- Coverage 63.65% 63.65% -0.01%
==========================================
Files 573 573
Lines 53761 53761
==========================================
- Hits 34222 34221 -1
- Misses 17590 17591 +1
Partials 1949 1949
|
Description
Closes: #10058
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change