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

improved: move level check from entry to logger and bail out faster #154

Merged
merged 1 commit into from
Mar 20, 2015

Conversation

imkira
Copy link
Contributor

@imkira imkira commented Mar 19, 2015

Hi there.

Thank you for logrus, it's a wonderful logger.
I was benchmarking loggers, as mentioned in https://github.com/imkira/go-loggers-bench
and I found I could make logrus bail-out faster (but more importantly, lesser allocations) when the baseline log-level is higher than the level being logged.
This is a modest attempt to improve it.

I didn't do it in this PR, but I also tried to do the same with WithFields, but given the current API it is very difficult to do it.
My idea was to invert

log.WithFields(...).Info(...)

and allow something like:

log.Info(...).WithFields(...)

or alternatively something like

log.WithLevel(Info).WithFields(...).Log(...)

Using this kind of API it would be possible to return "blackhole" entries/loggers when the level does not satisfy the level condition.
What do you think of it?

@@ -110,6 +110,10 @@ func (entry *Entry) Debug(args ...interface{}) {
}
}

func (entry *Entry) debug(args ...interface{}) {
entry.log(DebugLevel, fmt.Sprint(args...))
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these methods only added to avoid another integer comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I could remove them and call the original (publicly exported) ones. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think this extra code is necessary but I'm happy to accept the other part of the patch. Please rebase when you do that as well so it's just one commit. Thanks!

@imkira
Copy link
Contributor Author

imkira commented Mar 20, 2015

@sirupsen done. Let me know if its good now

sirupsen added a commit that referenced this pull request Mar 20, 2015
improved: move level check from entry to logger and bail out faster
@sirupsen sirupsen merged commit 3fc34d0 into sirupsen:master Mar 20, 2015
@sirupsen
Copy link
Owner

Thank you! It'll be in 0.7.1

@sirupsen
Copy link
Owner

You should re-run the benchmarks :) I'll happily accept other performance patches like this that don't break the API.

@imkira
Copy link
Contributor Author

imkira commented Mar 20, 2015

Thanks!
https://github.com/imkira/go-loggers-bench
Updated. Still sucks for JSON though but let's think this later.

devopstaku pushed a commit to devopstaku/logrus that referenced this pull request Aug 9, 2016
improved: move level check from entry to logger and bail out faster
cgxxv pushed a commit to cgxxv/logrus that referenced this pull request Mar 25, 2022
improved: move level check from entry to logger and bail out faster
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

Successfully merging this pull request may close these issues.

None yet

2 participants