Skip to content
This repository was archived by the owner on Aug 14, 2025. It is now read-only.

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

Open
asmodat wants to merge 9 commits intorelease/v0.4.50from
feature/add_logging_mechanism
Open

feature/add_logging_mechanism -> release/v0.4.50#241
asmodat wants to merge 9 commits intorelease/v0.4.50from
feature/add_logging_mechanism

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.

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants