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(#3628): add rotating log handler to support log rotation #5815

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yushao2
Copy link

@yushao2 yushao2 commented Jan 30, 2023

Background

Opening this because PR #4844 is stale

Refer to #3628 for more context -- this PR adds the option to rotate logs based on size. Rotation can be configured using 2 settings:

  • LOG_MAX_BYTES -- when configured, logs will automatically rotate when it approaches the set size (in bytes)
  • LOG_BACKUP_COUNT -- number of backup / rotated logs to keep on disk. If not set and LOG_MAX_BYTES the logs will just be cleared when it approaches the set size.

Some design consideration:
I found out from the docs that when LOG_MAX_BYTES and LOG_BACKUP_COUNT is set to 0, essentially RotatingFileHandler behaves like a regular FileHandler See: here

Closes #3628, closes #4844.

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Merging #5815 (b2649dd) into master (e9e1034) will increase coverage by 46.30%.
The diff coverage is 100.00%.

❗ Current head b2649dd differs from pull request most recent head 0f0bc85. Consider uploading reports for the commit 0f0bc85 to get more accurate results

@@             Coverage Diff             @@
##           master    #5815       +/-   ##
===========================================
+ Coverage   42.36%   88.66%   +46.30%     
===========================================
  Files         162      162               
  Lines       11206    11209        +3     
  Branches     1825     1825               
===========================================
+ Hits         4747     9939     +5192     
+ Misses       6103      986     -5117     
+ Partials      356      284       -72     
Impacted Files Coverage Δ
scrapy/utils/log.py 89.89% <100.00%> (+49.27%) ⬆️

... and 140 files with indirect coverage changes

scrapy/utils/log.py Outdated Show resolved Hide resolved
@wRAR
Copy link
Member

wRAR commented Jan 30, 2023

The comment from the original PR about it needing a test applies here too.

scrapy/utils/log.py Outdated Show resolved Hide resolved
@yushao2
Copy link
Author

yushao2 commented Jan 30, 2023

@wRAR I have no idea how to write unittests for the logging, so i pretty much copied from cpython's example and shoehorned it to work. I hope that's fine
https://github.com/python/cpython/blob/main/Lib/test/test_logging.py

@yushao2 yushao2 requested a review from wRAR January 30, 2023 16:57
@yushao2
Copy link
Author

yushao2 commented Feb 1, 2023

seems like checks are failing on 3.7 & 3.8, give me some time while i set up a 3.7 env and run tests locally. it seems like manual trigger of workflows are needed given that i'm a new contributor so let me try to sort this out

@wRAR wRAR marked this pull request as draft March 4, 2023 17:17
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.

Crawler logs are cut by day
2 participants