To rollover the crawler logs by day end through settings

[rindhane]

Details of modification are :

• Modified _get_handler method to return TimedRotatingFileHandler
  if LOG_ROTATION variable is set to True in settings.
• This enhancement is for the issue: Crawler logs are cut by day Crawler logs are cut by day #3628
• This modification is considered from this reference
• The input variables of TimedRotatingFileHandler have to provide a dict() to settings parameter "LOG_ROTATION_VALUE"

An example for settings which need to be provided in settings.py are

LOG_ROTATION = True
LOG_ROTATION_VALUE = {
    when = "midnight",
    interval = 1,
    backupCount = 0,
    delay= False,
    utc= False,
    atTime=None,
}

Changes to be committed:
modified: scrapy/utils/log.py

Fixes #3628

[Gallaecio]

I think we should use snake_case for the dictionary keys, and we can probably move the reading/evaluation of some settings later in code, but the changes look fine to me. I think you can proceed with tests and documentation.

[rindhane]

As per feedback made the following changes:

• Information about settings details for log rotation included in the documentation
  modified: docs/topics/logging.rst
  modified: docs/topics/settings.rst
• To use snake_case for variables related to log rotation in settings and accordingly modified _get_handler method to return TimedRotatingFileHandler
  modified: scrapy/utils/log.py
• Tests to confirm attributes and log rotation functionality.
  new file: tests/test_log_rotate.py

[Gallaecio]

In general lines, I wonder if this is the right approach. What if we want to later support size-based log rotation?

Instead of implementing support for a specific log rotation approach, maybe what we need is to look into making it possible for users to write custom log file handling logic, and provide some built-in implementations for users to choose from. Something in line with https://docs.scrapy.org/en/latest/topics/settings.html#std:setting-SPIDER_LOADER_CLASS.

[Gallaecio]

This is probably best suited for your global gitignore instead of Scrapy’s.

[Gallaecio]

You should probably include the other new settings here.

I also wonder if it would make sense to sort these alphabetical, the list is rather large now.

[Gallaecio]

Suggested change

	To have rollover of the logs, :setting:`LOG_FILE_ROTATE` have to be set ``TRUE``. `TimedRotatingFileHandler <https://docs.python.org/3/library/logging.handlers.html#timedrotatingfilehandler>`_ is set as the file handler for the logger.
	To enable time-based log rotation, set the :setting:`LOG_FILE_ROTATE` setting to ``True``.

TimedRotatingFileHandler feels like an implementation detail that users do not need to know at this point.

[Gallaecio]

I think this should be a list. It can be introduced from the paragraph above, with something like “Use the following settings to customize time-based log rotation:”.

[Gallaecio]

Looking at #3628, I see that what was proposed was to cover in the documentation how to implement this, without the need to modify the Scrapy code base at all.

[rindhane]

Looking at #3628, I see that what was proposed was to cover in the documentation how to implement this, without the need to modify the Scrapy code base at all.

@Gallaecio, this last comment has put me into a muddle. As, except the last one, other comments point to improve the previous implementation while the last comment directs to abandon everything and just document this ....

from logging.handlers import TimedRotatingFileHandler
from scrapy.utils.log import configure_logging

logHandler = TimedRotatingFileHandler('crawl.log', when='midnight', interval=1)
configure_logging(install_root_handler=False)
logging.basicConfig(
    format='%(asctime)s [%(name)s] %(levelname)s: %(message)s',
    level=logging.INFO,
    handlers=[logHandler]) 

....into the documentation.

Will you please elaborate again on which path to consider to implement this feature?

[Gallaecio]

Yes, I started reviewing your implementation, and only after that, when re-reading the original issue report, I realized that @kmike had tagged it as a documentation improvement issue.

If the suggested example code does the job, adding it to the documentation may be better in the long term than increasing the complexity of the Scrapy code base.

That said, if you really wish to implement this as a built-in feature, I’m up for reviewing your pull request. But I think we need to make it flexible enough that people won’t take the suggested example code route as soon as they need something a bit different.