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

Switch log library to Zap #216

Closed
liclac opened this issue May 11, 2017 · 5 comments
Closed

Switch log library to Zap #216

liclac opened this issue May 11, 2017 · 5 comments
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 refactor

Comments

@liclac
Copy link
Contributor

liclac commented May 11, 2017

https:/uber-go/zap

According to my tests, it is significantly faster than logrus, to the point where logging in hot paths would even be okay.

@na-- na-- added refactor evaluation needed proposal needs to be validated or tested before fully implementing it in k6 labels Oct 3, 2018
@thinkerou
Copy link
Contributor

@liclac @na-- this switch still needed? I recently am learning k6 and want to contribute some code for more learn it. thanks!

@mstoykov
Copy link
Contributor

mstoykov commented May 13, 2020

I don't think I have ever noticed logrus in any of my profiling of k6.

And while more speed has never hurt anyone, in a lot of places the problem with logging is that it's done using the global logger instead of local one, which will require a lot more refactoring. I think we have made big strides fixing this in the last year and maybe we are nearly there, as me grepping in new-schedulers branch gives me

$ rg 'logrus\.(Info|Warn|Error|Log|Print)\w*\('
stats/cloud/collector.go
133:            logrus.Warn("K6CLOUD_TOKEN is deprecated and will be removed. Use K6_CLOUD_TOKEN instead.")

lib/executor/execution_config_shortcuts.go
121:                    logrus.Warnf(
128:                    logrus.Warnf("`stages` was explicitly set to an empty value, running the script with 1 iteration in 1 VU")
132:                    logrus.Warnf("`execution` was explicitly set to an empty value, running the script with 1 iteration in 1 VU")

Although I am not certain I am grepping for everything possible :) Edit - if I add With I found way more - mostly in the collector/output code where I would expect them :)

After this.. changing logging libraries could possibly be an interesting question, although again it will need to be profiled and see if it actually even have a difference to make :)

I myself haven't used zap so I can't give any personal opinion on whether there are other concerns around moving to it ...

@na-- na-- removed the help wanted label May 13, 2020
@liclac
Copy link
Contributor Author

liclac commented May 13, 2020

When I wrote this, there weren't any log calls in the VU hot path at all; I don't know if that's changed, but I was curious if we could get it fast enough to be able to log errors and warnings in there - plus debug logging if it's turned on, using Checked Entries: https://godoc.org/go.uber.org/zap#Logger.Check

@thinkerou
Copy link
Contributor

@mstoykov @liclac thank you reply, got it.

@na--
Copy link
Member

na-- commented Mar 7, 2023

Closing this in favor of #2958

@na-- na-- closed this as completed Mar 7, 2023
@na-- na-- closed this as not planned Won't fix, can't repro, duplicate, stale Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 refactor
Projects
None yet
Development

No branches or pull requests

4 participants