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

[PR #7240/315ae903 backport][3.9] Reduce access log overhead when logging is disabled #7254

Conversation

patchback[bot]
Copy link
Contributor

@patchback patchback bot commented Apr 16, 2023

This is a backport of PR #7240 as merged into master (315ae90).

What do these changes do?

If the logger was disabled the log message would be formatted and thrown away. Avoid formatting the log message when logging is not enabled at info level.

In testing this represented up to 12% of the run time of serving a static file.

Are there changes in behavior for the user?

No

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

<!-- Thank you for your contribution! -->

## What do these changes do?

If the logger was disabled the log message would be formatted and thrown
away. Avoid formatting the log message when logging is not enabled at
info level.

In testing this represented up to 12% of the run time of serving a
static file.

## Are there changes in behavior for the user?

No

## Related issue number

<!-- Are there any issues opened that will be resolved by merging this
change? -->

## Checklist

- [x] I think the code is well written
- [x] Unit tests for the changes exist
- [ ] Documentation reflects the changes
- [ ] If you provide code modification, please add yourself to
`CONTRIBUTORS.txt`
  * The format is &lt;Name&gt; &lt;Surname&gt;.
  * Please keep alphabetical order, the file is sorted by names.
- [x] Add a new news fragment into the `CHANGES` folder
  * name it `<issue_id>.<type>` for example (588.bugfix)
* if you don't have an `issue_id` change it to the pr id after creating
the pr
  * ensure type is one of the following:
    * `.feature`: Signifying a new feature.
    * `.bugfix`: Signifying a bug fix.
    * `.doc`: Signifying a documentation improvement.
    * `.removal`: Signifying a deprecation or removal of public API.
* `.misc`: A ticket has been closed, but it is not of interest to users.
* Make sure to use full sentences with correct case and punctuation, for
example: "Fix issue with non-ascii contents in doctest text files."

---------

Co-authored-by: Sviatoslav Sydorenko <[email protected]>
Co-authored-by: Sam Bull <[email protected]>
(cherry picked from commit 315ae90)
@patchback patchback bot requested a review from asvetlov as a code owner April 16, 2023 15:42
@Dreamsorcerer Dreamsorcerer enabled auto-merge (squash) April 16, 2023 15:43
@codecov
Copy link

codecov bot commented Apr 16, 2023

Codecov Report

Merging #7254 (6acd801) into 3.9 (586778f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              3.9    #7254      +/-   ##
==========================================
- Coverage   97.32%   97.31%   -0.01%     
==========================================
  Files         108      108              
  Lines       31587    31596       +9     
  Branches     4012     4013       +1     
==========================================
+ Hits        30741    30748       +7     
- Misses        640      641       +1     
- Partials      206      207       +1     
Flag Coverage Δ
CI-GHA 97.21% <100.00%> (-0.01%) ⬇️
OS-Linux 96.89% <100.00%> (+<0.01%) ⬆️
OS-Windows 94.48% <100.00%> (-0.01%) ⬇️
OS-macOS 96.46% <100.00%> (-0.01%) ⬇️
Py-3.10.10 ?
Py-3.10.11 96.94% <100.00%> (?)
Py-3.11.0 96.39% <100.00%> (+<0.01%) ⬆️
Py-3.7.16 96.69% <100.00%> (+<0.01%) ⬆️
Py-3.7.9 94.32% <100.00%> (-0.01%) ⬇️
Py-3.8.10 94.25% <100.00%> (-0.01%) ⬇️
Py-3.8.16 96.61% <100.00%> (+<0.01%) ⬆️
Py-3.9.13 94.26% <100.00%> (-0.01%) ⬇️
Py-3.9.16 96.63% <100.00%> (-0.01%) ⬇️
Py-pypy7.3.11 94.04% <100.00%> (+<0.01%) ⬆️
VM-macos 96.46% <100.00%> (-0.01%) ⬇️
VM-ubuntu 96.89% <100.00%> (+<0.01%) ⬆️
VM-windows 94.48% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
aiohttp/web_log.py 100.00% <100.00%> (ø)
tests/test_web_log.py 98.26% <100.00%> (+0.11%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Dreamsorcerer Dreamsorcerer merged commit cdc8b29 into 3.9 Apr 16, 2023
@Dreamsorcerer Dreamsorcerer deleted the patchback/backports/3.9/315ae9037438351b48a712734173020374e5ff33/pr-7240 branch April 16, 2023 16:15
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