Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: adding zerolog logging to cosmovisor #10217
Changes from 7 commits
8eaff82
355d129
3d6277b
fdfb7e2
699d70b
aea872a
24526ad
b9c48b1
1a74a9f
6ed2ff8
50a8a55
87a06cc
b93a594
d53deb2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 calllogger := 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.
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?