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

dlog: Defer sprintf to the final logger. #28

Merged
merged 2 commits into from
Aug 16, 2021
Merged

Conversation

josecv
Copy link
Contributor

@josecv josecv commented Aug 10, 2021

This is intended to preserve CPU cycles that would otherwise be spent
formatting messages that will never be logged (e.g. formatting debug
messages when the log level is info)

Signed-off-by: Jose Cortes josecortes@datawire.io

@josecv josecv requested a review from LukeShu August 10, 2021 16:53
@josecv
Copy link
Contributor Author

josecv commented Aug 10, 2021

This comes after i discovered that telepresence's root daemon can spend a lot of time formatting debug statements even when the log level is set to info. For large enough requests this has a considerable performance penalty
image

This is intended to preserve CPU cycles that would otherwise be spent
formatting messages that will never be logged (e.g. formatting debug
messages when the log level is info)

Signed-off-by: Jose Cortes <josecortes@datawire.io>
@josecv josecv force-pushed the josecv/dlog-performance branch 3 times, most recently from 059f1f9 to 5c7357f Compare August 11, 2021 21:28
Signed-off-by: Jose Cortes <josecortes@datawire.io>
Copy link

@donnyyung donnyyung left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work.

@josecv josecv merged commit 5127e9c into master Aug 16, 2021
@josecv josecv deleted the josecv/dlog-performance branch August 16, 2021 14:10
LukeShu added a commit that referenced this pull request May 24, 2022
dlog: Fix issue where the fast-logging broke backward compat

#28 changed the `Logger`
interface, breaking any existing implementors of the interface.
That's not OK.

So revert that interface (and add a comment about how it's frozen),
and add an opt-in `OptimizedLogger` interface for Jose's
optimizations.
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.

2 participants