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: add kingfisher collect log warnings with scrapyloganalyzer #385

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yolile
Copy link
Member

@yolile yolile commented Jan 3, 2025

ref #29

@yolile
Copy link
Member Author

yolile commented Jan 3, 2025

I'm not sure why gunicorn[setproctitle] and yapw[perf] are being removed when running uv pip compile requirements.in -o requirements.txt

@jpmckinney
Copy link
Member

It's okay - they should be removed.

Comment on lines +112 to +114
logger.warning("%s: %s", self, {scrapy_log.logparser["log_categories"][key]["details"]})
if scrapy_log.error_rate:
logger.warning("%s: crawl error rate was %s", self, {scrapy_log.error_rate})
Copy link
Member

Choose a reason for hiding this comment

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

Log warnings don't generate emails and aren't saved (except temporarily in server log files).

If you want to generate emails, you need to use the error level.

If you want to save the warnings, you need to update self.job.context.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, nevermind, the cron job sets the log level to WARNING, so we will get emails.

Can you check whether error_rate can have a division by zero error when no items were scraped? Ideally with a test in scrapy-log-analyzer.

Also, when the error_rate is non-zero, should the message be more detailed, e.g. print the error messages and/or error counts? https://kingfisher-collect.readthedocs.io/en/latest/logs.html#check-the-crawl-statistics

I think we should also check that scrapy_log.is_finished() is True. (This is different from the Scrapyd job being finished.) The log message can include logparser["finish_reason"]. https://kingfisher-collect.readthedocs.io/en/latest/logs.html#check-the-reason-for-closing-the-spider

In open-contracting/scrapy-log-analyzer#11 we discussed storing item_dropped_count and item_scraped_count in the job context (easier to analyze multiple jobs without having to run logreport manually). If there are already other warnings, these can be logged as well.

I think that's everything from open-contracting/scrapy-log-analyzer#11, but please have a look over again in case I missed an earlier idea.

Copy link
Member Author

@yolile yolile Jan 17, 2025

Choose a reason for hiding this comment

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

Hmm ok, I wasn't sure how much of what we discussed there we wanted to add to the Registry vs to a Kingfisher Collect command. But I guess is easier to have the full info in the Data Registry too

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you check whether error_rate can have a division by zero error when no items were scraped? Ideally with a test in scrapy-log-analyzer.

Do you mean with outputs like these? If so, the error rate is 1 (I will push the test if that is what you meant)

Copy link
Member

Choose a reason for hiding this comment

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

Aha, I guess there is always going to be at least one error.

Yeah, my thinking is it will be faster, operationally, to have the same info directly available in the Registry admin. Once the Collect command is implemented, we can see how to reduce any code duplication at that time.

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