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

feature/add_logging_mechanism -> release/v0.4.50 #241

Open
wants to merge 9 commits into
base: release/v0.4.50
Choose a base branch
from

Conversation

asmodat
Copy link
Contributor

@asmodat asmodat commented Nov 29, 2024

No description provided.

Copy link
Collaborator

@MrLutik MrLutik left a 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.",
Copy link
Collaborator

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) {
Copy link
Collaborator

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()
Copy link
Collaborator

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.

@golnar-boosty golnar-boosty requested a review from MrLutik January 27, 2025 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants