-
Notifications
You must be signed in to change notification settings - Fork 2
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
feature/add_logging_mechanism -> release/v0.4.50 #241
base: release/v0.4.50
Are you sure you want to change the base?
Conversation
…tions with defaults for interx setup
…d interx/block.go
…version printing issues"
…y and maintainability
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.
Function CustomLogger rely on env variable PrintLogs. We use CLI. So it's better pass arg from a flag.
If there is a default behavior we should explicitly show it. In this case it should be:
var printLogs bool = false
We should also handle the error if logger is not initiated correctly as it's crucial part of the app. I would suggest to return error as well. Please keep in mind that the code will run in a container from SCRATCH without actual shell to interact with.
} | ||
} | ||
|
||
func queryBlockByHeightOrHashHandle(rpcAddr string, height string) (interface{}, interface{}, int) { | ||
success, err, statusCode := common.MakeTendermintRPCRequest(rpcAddr, "/block", fmt.Sprintf("height=%s", height)) | ||
|
||
if err != nil { | ||
log.CustomLogger().Error(" `queryBlockByHeightOrHashHandle` failed to execute.", |
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.
On each call you initialize the new logger. Wouldn't it better to init the logger once and share it between packages?
var log = logrus.New() | ||
|
||
// Monitor will continuously collect system information | ||
func Monitor(interval time.Duration) { |
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.
There is no point to track this data within application. We can use dedicated tools from host side to benchmark the performance. This data can't be accurate due to unknown nature of environment where application is running in.
"github.com/sirupsen/logrus" | ||
) | ||
|
||
var logger = logrus.New() |
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.
We should stick to the one logging mechanic. If it's cosmos logger - let's be consistent and proceed with it.
No description provided.