Skip to content

fix(scrapy): make logging configuration idempotent#954

Open
vdusek wants to merge 4 commits into
masterfrom
fix/scrapy-idempotent-logging
Open

fix(scrapy): make logging configuration idempotent#954
vdusek wants to merge 4 commits into
masterfrom
fix/scrapy-idempotent-logging

Conversation

@vdusek

@vdusek vdusek commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

initialize_logging re-wrapped scrapy.utils.log.configure_logging on every call, stacking wrappers when it ran more than once. The monkey-patch is now installed at most once and reads the current handler and level from a module-level state dict, so repeated initialization stays correct without nesting wrappers.

Part of splitting the larger Scrapy integration fix (fix/scrapy-integration) into reviewable pieces.

@vdusek vdusek added adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. labels Jun 9, 2026
@vdusek vdusek self-assigned this Jun 9, 2026
@github-actions github-actions Bot added this to the 142nd sprint - Tooling team milestone Jun 9, 2026
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 14.28571% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.84%. Comparing base (a87e8d1) to head (93d494e).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/apify/scrapy/_logging_config.py 14.28% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #954      +/-   ##
==========================================
- Coverage   90.00%   89.84%   -0.17%     
==========================================
  Files          49       49              
  Lines        3053     3062       +9     
==========================================
+ Hits         2748     2751       +3     
- Misses        305      311       +6     
Flag Coverage Δ
e2e 35.56% <0.00%> (-0.11%) ⬇️
integration 56.72% <0.00%> (-0.14%) ⬇️
unit 78.54% <14.28%> (-0.17%) ⬇️

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vdusek vdusek requested a review from Pijukatel June 9, 2026 11:03
@vdusek vdusek marked this pull request as ready for review June 9, 2026 11:03
logger.propagate = False


def _configure_all_loggers() -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am missing some context, but can't we skip the module-level dict and do it like this?

@lru_cache
def _configure_all_loggers(handler, level):
    if handler is None:
        return
    for logger_name in [None, *_ALL_LOGGERS]:
        _configure_logger(logger_name, level, handler)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we can't do that - the _configure_all_loggers needs to run more than once.

The expected flow of the Scrapy Actors is something like this:

  1. We configure logging (our handler + levels).
  2. We do some setup, during which things may already log.
  3. Scrapy's own setup runs and overrides our logging configuration.
  4. We re-apply our configuration on top to override it back.

So the current code is the correct way.

@vdusek vdusek requested a review from Pijukatel June 9, 2026 14:16
@vdusek

vdusek commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

@Pijukatel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants