Skip to content

feat: Added optional args to logger.log to format string only when log level applies #706

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

Merged
merged 3 commits into from
Oct 5, 2021

Conversation

zashraf1985
Copy link
Contributor

@zashraf1985 zashraf1985 commented Oct 4, 2021

Summary

Message formatting is performance heavy. The logger takes formatted message as a parameter, which means message formatting is being done for all types of logs even in cases when the log level does not apply. This PR adds arguments array to the log function to carry token values. Logger already has the functionality to sprintf based on extra args if the log level applies. This will increase performance in cases where log level is restricted to more conservative ones such as ERROR.

Test plan

  • All Unit tests pass
  • Manually tested thoroughly with optimizely-sdk package. A follow up PR for the main package will be created after this is merged and released.

@zashraf1985 zashraf1985 requested a review from a team as a code owner October 4, 2021 23:21
@zashraf1985 zashraf1985 removed their assignment Oct 4, 2021
@coveralls
Copy link

coveralls commented Oct 4, 2021

Coverage Status

Coverage remained the same at 96.993% when pulling d3e83b0 on zeeshan/logger-add-function into f33af13 on master.

@zashraf1985 zashraf1985 changed the title feat: Added optional formatting function to logger feat: Added optional args to logger to delay formatting Oct 5, 2021
@zashraf1985 zashraf1985 changed the title feat: Added optional args to logger to delay formatting feat: Added optional args to logger.log to format string only when log level applies Oct 5, 2021
@yavorona yavorona self-requested a review October 5, 2021 16:21
Copy link
Contributor

@yavorona yavorona left a comment

Choose a reason for hiding this comment

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

LGTM!

@zashraf1985 zashraf1985 merged commit 4f37397 into master Oct 5, 2021
@zashraf1985 zashraf1985 deleted the zeeshan/logger-add-function branch October 5, 2021 17:30
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.

3 participants