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

Unnecessary allocations in ZerologWrapper #14850

Closed
jhernandezb opened this issue Jan 31, 2023 · 6 comments
Closed

Unnecessary allocations in ZerologWrapper #14850

jhernandezb opened this issue Jan 31, 2023 · 6 comments

Comments

@jhernandezb
Copy link

jhernandezb commented Jan 31, 2023

Summary of Bug

While debugging a tendermint performance issue (see informalsystems/tendermint#4) I noticed that even if the log level in config.toml is set to error only, the Debug method in https://github.com/public-awesome/cosmos-sdk/blob/release/v0.45.x/server/logger.go#L33 appears consistently in profiles with a significant amount, and this is due to getLogFields methods being evaluated regardless of log level.

Screenshot 2023-01-30 at 6 49 43 PM

Screenshot 2023-01-30 at 6 49 47 PM

Debug is used in different code paths of tendermint adding up some unnecessary computations.

I'm currently running the same node but with an extra check as in https://github.com/public-awesome/cosmos-sdk/blob/3a862e03d4177ccff3927cac2d2a5aaf36d5b367/server/logger.go#L46-L51

Which seems to help, although not sure if the extra check it's actually worth the troubles but CPU did decrease in some % after applying the patch.

Profile comparison
Screenshot 2023-01-30 at 7 19 29 PM

Version

0.45.11

Steps to Reproduce

Run any node with log level set to error

@tac0turtle
Copy link
Member

This logger was deleted actually. We use Tendermint's logger now with filtering.

@alexanderbez
Copy link
Contributor

Yeah it's a shame really. Zerolog is so fast. We just couldn't use it the way it was meant to be used because Tendermint's logger interface is very limited.

@tac0turtle
Copy link
Member

how is the interface limiting? we swapped back cause there was no filtering and logs became impossible to read

@alexanderbez
Copy link
Contributor

alexanderbez commented Feb 1, 2023

how is the interface limiting? we swapped back cause there was no filtering and logs became impossible to read

Because zerolog provides structured typed logging -- that's the entire point and reason why it's so efficient. Tendermint's interface does not, as it only allows you to provide any pairs. We were trying to put Ferrari decals on a Toyota.

@tac0turtle
Copy link
Member

Okay I have a plan on how to get use there.

In regards to this issue it should actually live in comet. Looking at the profile the logger is from us but the log is comets. It is most likely because they are logging an interface which causes zero log to use reflection. I ran into this a few times on comet. Let's close this and open an issue on tendermint to stop logging interfaces.

@tac0turtle tac0turtle closed this as not planned Won't fix, can't repro, duplicate, stale Feb 6, 2023
@alexanderbez
Copy link
Contributor

Awesome. Looking forward to seeing what your idea is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants