Added structured logging to systemd journal. #7115
Conversation
|
Obligatory "I wonder how to test this". I'm also not sure if enabling this if and only if |
scrapy/utils/log.py
Outdated
| handler = logging.FileHandler(filename, mode=mode, encoding=encoding) | ||
| elif settings.getbool("LOG_ENABLED"): | ||
| handler = logging.StreamHandler() | ||
| # Use systemd journal handler if running under systemd |
There was a problem hiding this comment.
I don’t think we should do this automatically, I think this should be opt-in through a new setting.
There was a problem hiding this comment.
Something like this?
# OPT-IN: Use systemd JournalHandler only if explicitly enabled
if (
settings.getbool("LOG_TO_SYSTEMD", False)
and SYSTEMD_JOURNAL_AVAILABLE
):
handler = systemd.journal.JournalHandler()
else:
handler = logging.StreamHandler()
|
|
Please check it now. |
docs/topics/logging.rst
Outdated
| * :setting:`LOG_DATEFORMAT` | ||
| * :setting:`LOG_STDOUT` | ||
| * :setting:`LOG_SHORT_NAMES` | ||
| * :setting:`LOG_TO_SYSTEMD` |
There was a problem hiding this comment.
I think LOG_SYSTEMD or LOG_SYSTEMD_ENABLED would be more in line with the naming of other Scrapy settings.
docs/topics/logging.rst
Outdated
| If :setting:`LOG_TO_SYSTEMD` is set to ``True``, Scrapy will send logs to the | ||
| systemd journal using ``systemd.journal.JournalHandler`` as its logging handler. | ||
| This requires the external Python package ``systemd`` to be installed (minimum | ||
| version 234 recommended). You can install it via pip:: | ||
|
|
||
| pip install systemd | ||
|
|
||
| This feature is opt-in and disabled by default. When enabled, Scrapy will raise an | ||
| ``ImportError`` if the ``systemd`` package is not found. Use this setting when | ||
| running Scrapy under systemd to utilize ``journalctl`` for log management and | ||
| filtering. |
There was a problem hiding this comment.
You can have a minor reference here, but the setting documentation should live in the settings.rst page, with a section of its own properly labeled so that :setting:`FOO` actually links there. Consider running tox -e docs to check how your docs render.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7115 +/- ##
==========================================
- Coverage 91.43% 91.41% -0.03%
==========================================
Files 165 165
Lines 12644 12649 +5
Branches 1621 1622 +1
==========================================
+ Hits 11561 11563 +2
- Misses 810 812 +2
- Partials 273 274 +1
|
|
Anything else I should do? |
|
@AliAlimohammadi at the very least you should fix the CI failures and make sure you run these checks locally before pushing. |
|
But we wanted to raise an exception. Unless you want me to gracefully raise an exception in try block? |
|
@Gallaecio Can you please let me know how exactly to handle the exception? |
It doesn't look like you understood what I said, do you need some clarifications? |
|
The docs CI reports: You should run Also, you should use pre-commit. |
|
Can you check it now? |
tox.ini
Outdated
| deps = | ||
| {[testenv:extra-deps]deps} | ||
| pylint==4.0.2 | ||
| systend-python; sys-platform=="linux" |
scrapy/utils/log.py
Outdated
| handler = logging.StreamHandler() | ||
| if settings.getbool("LOG_SYSTEMD", False): | ||
| # Opt-in systemd journal logging, will raise if systemd.journal is missing | ||
| import systemd.journal |
There was a problem hiding this comment.
It doesn't look like you've started using pre-commit.
There was a problem hiding this comment.
I used pre-commit and it passed without an issue.
There was a problem hiding this comment.
I don't know what else to do.
There was a problem hiding this comment.
Then it's likely that you've used it incorrectly.
There was a problem hiding this comment.
I fixed it. There was a typo.
|
Have you tested these changes? If so, can you please describe how? |
3 participants
Fixed the issue #4858.