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

Improve on logging #1809

Merged
merged 2 commits into from
Jan 5, 2018
Merged

Improve on logging #1809

merged 2 commits into from
Jan 5, 2018

Conversation

repeatedly
Copy link
Member

@repeatedly repeatedly commented Dec 30, 2017

  1. Improve Log#on_xxx API performance with yield.
  2. Old implementation is error prune. API is log.on_info { ... } so error should be thrown when block is not passed.
    I searched the plugin code and no log.on_xxx without block code so it doesn't break existing plugins.
  3. Use on_trace API in buffer. trace level is rare on the production so frequent object creation for trace logging is not good.

@repeatedly repeatedly added enhancement Feature request or improve operations v1 labels Dec 30, 2017
@repeatedly repeatedly requested a review from mururu December 30, 2017 18:26
Copy link
Member

@mururu mururu left a comment

Choose a reason for hiding this comment

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

For 3, my understanding is that we can avoid evaluation for the arguments of log.trace by using log.on_trace. At the same time, we also don't have to create unnecessary Proc object because we use yield even if we use log.on_trace. So it shows better performance, right?

@repeatedly
Copy link
Member Author

Exactly

@mururu
Copy link
Member

mururu commented Jan 5, 2018

Thanks. Looks good!

@repeatedly repeatedly merged commit ce6cda0 into master Jan 5, 2018
@repeatedly repeatedly deleted the improve-on-logging branch January 5, 2018 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request or improve operations v1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants