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

feat(logging): skip unnecessary log message constructions #2886

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

jacobowitz
Copy link
Contributor

When processing data requests (which is a quite frequent task) we construct some log messages in every case for debug logging purpose. This happens even when debug logging is disabled. I think that is wasteful and should be avoided.
The easiest way to avoid this is to check for the log level and only construct the log message if necessary. This is also recommended in the Python documentation.
The idea for this came up in this pr: #2865

I only changed it for a few cases in this PR which happen quite often and should inflict the most costs for us. Other cases (Initialization and control messages) should not happen often enough that its worth to introduce this check there, I think.

@jacobowitz jacobowitz requested a review from JoanFM July 8, 2021 10:29
@jacobowitz jacobowitz requested a review from a team as a code owner July 8, 2021 10:29
@jina-bot jina-bot added size/S area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality component/logging component/peapod labels Jul 8, 2021
@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #2886 (c0f73c1) into master (a673d27) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2886      +/-   ##
==========================================
+ Coverage   88.07%   88.15%   +0.07%     
==========================================
  Files         138      138              
  Lines        9505     9508       +3     
==========================================
+ Hits         8372     8382      +10     
+ Misses       1133     1126       -7     
Flag Coverage Δ
daemon 43.43% <84.61%> (-0.01%) ⬇️
jina 88.10% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jina/logging/logger.py 93.47% <100.00%> (+0.07%) ⬆️
jina/peapods/runtimes/gateway/prefetch.py 93.15% <100.00%> (+0.09%) ⬆️
jina/peapods/runtimes/zmq/zed.py 93.36% <100.00%> (+0.02%) ⬆️
jina/peapods/runtimes/jinad/__init__.py 95.58% <0.00%> (-1.48%) ⬇️
jina/peapods/runtimes/jinad/client.py 86.39% <0.00%> (+1.18%) ⬆️
jina/peapods/pods/compound.py 91.54% <0.00%> (+1.40%) ⬆️
jina/peapods/peas/__init__.py 98.23% <0.00%> (+2.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a673d27...c0f73c1. Read the comment docs.

JoanFM
JoanFM previously approved these changes Jul 8, 2021
@github-actions
Copy link

github-actions bot commented Jul 8, 2021

Latency summary

Current PR yields:

  • 🐎🐎 index QPS at 1983, delta to last 1 avg.: +10%
  • 🐎🐎 query QPS at 43, delta to last 1 avg.: +7%
  • 😶 import jina within 0.2007s, delta to last 1 avg.: +0%

Breakdown

Version Index QPS Query QPS Import Time (s)
current 1983 43 0.2007
2.0.4 1798 40 0.2019

Backed by latency-tracking. Further commits will update this comment.

@JoanFM JoanFM merged commit 9ace745 into master Jul 8, 2021
@JoanFM JoanFM deleted the feat-speed-up-logging branch July 8, 2021 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality component/logging component/peapod size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants