[WIP]Add New Monitors to track Requests Success Rate #454
Conversation
shafiq-muhammad
changed the title
There was a problem hiding this comment.
Thanks Muhammad for these changes!
I pointed some few things to update, but these monitors are nearly done already :)
I'll be tagging @vineettomar06 for any further feedback about these monitors, and also so they can serve as a base for the new 429 error monitors.
|
|
||
| threshold = self.crawler.settings.getint(SPIDERMON_MAX_SPM_ERROR_RATIO, -1) | ||
| if threshold < 0: | ||
| return |
There was a problem hiding this comment.
suggestion: I think in this case we should notify that the monitor is not configured. Either via a regular log message, or by skipping the test with a message saying that SPIDERMON_MAX_SPM_ERROR_RATIO is not set.
Otherwise this monitor will be marked as "OK", when it didn't really run, which could make debugging harder.
|
|
||
| errors = self.stats[self.stat_name] | ||
| requests = self.stats["zyte_smartproxy/request"] | ||
| ratio = errors / requests |
There was a problem hiding this comment.
todo: We should handle the case where requests is zero, otherwise this will throw a ZeroDivisionError.
We could consider that case to have a ratio of zero, or we can skip it otherwise.
| @@ -297,6 +299,59 @@ def test_minimum_successful_requests(self): | |||
| self.assertGreaterEqual(requests, threshold, msg=msg) | |||
|
|
|||
|
|
|||
| @monitors.name("Zyte API Error Ratio monitor") | |||
| class ZyteAPIErrorRatioMonitor(Monitor, BaseStatMonitor): | |||
There was a problem hiding this comment.
todo: We'll have to add unitttests for the new monitor checks and fix the CI checks.
|
Ah, didn't notice these were issued here instead of the common monitors repo. We might want to submit them to the common monitors repo for the time being @shafiq-muhammad, while we finalize the repo with the Zyte product monitors. |
Added following monitors to test requests success rate