Code to Review Customer Ticket #16
Conversation
WalkthroughAdds logging and error-handling to SOPSRequest.post, including early return on JSON serialization errors and clearer retry logging. Updates get_settings to include only JSON‑serializable spider settings, logging excluded keys and introducing a helper for serializability checks. Changes
Sequence Diagram(s)sequenceDiagram autonumber participant Caller participant SOPSRequest participant Logger participant HTTP as HTTP Client Caller->>SOPSRequest: post(url, payload, retries) rect rgba(230,245,255,0.6) note over SOPSRequest,Logger: New: Early JSON serialization validation/logging SOPSRequest->>SOPSRequest: json.dumps(payload) alt JSON not serializable SOPSRequest-->>Logger: error("JSON serialization failed") SOPSRequest-->>Caller: return None else JSON ok loop attempts SOPSRequest->>HTTP: POST alt Connection error SOPSRequest-->>Logger: warn("Connection error on attempt i") else Other unexpected error SOPSRequest-->>Logger: error("Unexpected error on attempt i") else Success HTTP-->>SOPSRequest: Response SOPSRequest-->>Caller: return Response end end end end sequenceDiagram autonumber participant Spider participant Model as get_settings participant Logger Spider->>Model: get_settings(spider) rect rgba(240,255,240,0.6) note over Model,Logger: New: Filter settings by JSON serializability Model->>Model: Iterate spider.settings alt value JSON-serializable Model->>Model: include in spider_settings else not serializable Model->>Model: collect key in exclusion list end opt any exclusions Model-->>Logger: warn("Excluded non-serializable settings: ...") end Model-->>Spider: spider_settings (filtered) end Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scrapeops_scrapy/core/api.py (1)
201-203: Prefer a module-level logger over per-call setup.Initialize
logger = logging.getLogger(__name__)once at module scope and reuse it to avoid repeated lookups and ease testing.Example (outside this range):
# At top of module import logging logger = logging.getLogger(__name__)Then remove these lines from post().
- import logging - logger = logging.getLogger(__name__)scrapeops_scrapy/core/model.py (2)
219-221: Move logger to module scope.Define
logger = logging.getLogger(__name__)at the top and reuse here to avoid per-call setup.Example (outside this range):
# top of module import logging logger = logging.getLogger(__name__)Then remove these lines from get_settings().
- import logging - logger = logging.getLogger(__name__)
248-255: Minor: prefer try/except/else for clarity.Return True in an
else:block to make the happy path explicit.- try: - json.dumps(value) - return True - except (TypeError, ValueError): - return False + try: + json.dumps(value) + except (TypeError, ValueError): + return False + else: + return True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scrapeops_scrapy/core/api.py(2 hunks)scrapeops_scrapy/core/model.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scrapeops_scrapy/core/api.py (2)
scrapeops_scrapy/normalizer/proxies.py (3)
ProxyNormalizer(11-197)unknown_proxy_scheme(92-95)get_proxy_scheme(85-89)scrapeops_scrapy/exceptions.py (1)
ScrapeOpsAPIResponseError(13-16)
🪛 Ruff (0.13.3)
scrapeops_scrapy/core/model.py
253-253: Consider moving this statement to an else block
(TRY300)
scrapeops_scrapy/core/api.py
220-226: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
236-236: Do not catch blind exception: Exception
(BLE001)
237-237: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (2)
scrapeops_scrapy/core/model.py (1)
227-247: Good safeguard: filter to JSON‑serializable settings and warn once.This prevents payload failures and guides users to the exclusion list. LGTM.
scrapeops_scrapy/core/api.py (1)
200-210: SOPSRequest.post never receives non-None files All callsites either omit the files parameter or explicitly pass files=None, so json+files combination cannot occur.
| import logging | ||
| logger = logging.getLogger(__name__) | ||
| |
There was a problem hiding this comment.
Differentiate serialization vs response JSON decode errors; narrow exceptions and include tracebacks.
Catching (TypeError, ValueError) here also swallows response.json() decode failures, causing premature no-retry. Handle json.JSONDecodeError separately (retry), keep early-return only for payload serialization errors, add Timeout/RequestException, and use logger.exception for tracebacks.
Apply:
@@ - import logging - logger = logging.getLogger(__name__) + import logging + import json + logger = logging.getLogger(__name__) @@ - except (TypeError, ValueError) as json_error: - # These are JSON serialization errors - don't retry, log and return immediately - logger.error( - "ScrapeOps: Unable to send monitoring data due to non-serializable settings. " - "Some of your Scrapy settings contain functions or other objects that cannot be " - "converted to JSON format. To resolve this, add the problematic setting names " - "to SCRAPEOPS_SETTINGS_EXCLUSION_LIST in your settings.py file. " - f"Error details: {json_error}" - ) - error = json_error - return None, str(error) # Don't retry on serialization errors + except json.JSONDecodeError as parse_error: + # Invalid JSON in response from server → retry + logger.warning(f"ScrapeOps: Invalid JSON response (attempt {attempt+1}): {parse_error}") + error = parse_error + continue + except (TypeError, ValueError) as serialization_error: + # Payload JSON serialization error → don't retry + logger.exception( + "ScrapeOps: Unable to send monitoring data due to non-serializable settings. " + "Add the problematic setting names to SCRAPEOPS_SETTINGS_EXCLUSION_LIST in your settings.py. " + f"Error details: {serialization_error}" + ) + return None, str(serialization_error) + except requests.exceptions.Timeout as e: + logger.warning(f"ScrapeOps: Timeout (attempt {attempt+1}): {e}") + error = e + continue except requests.exceptions.ConnectionError as e: logger.warning(f"ScrapeOps: Connection error (attempt {attempt+1}): {e}") error = e continue except ScrapeOpsAPIResponseError as e: error = e continue - except Exception as e: - logger.error(f"ScrapeOps: Unexpected error (attempt {attempt+1}): {e}") - error = e - continue + except requests.exceptions.RequestException as e: + logger.exception(f"ScrapeOps: Request error (attempt {attempt+1}): {e}") + error = e + continueAlso consider converting the error log at Line 237 to logger.exception for tracebacks (addressed above). Based on static analysis hints.
Also applies to: 218-239
🤖 Prompt for AI Agents
In scrapeops_scrapy/core/api.py around lines 201-203 and 218-239, narrow broad exception handling so payload serialization errors (TypeError, ValueError) remain an early-return, but handle response.json() failures (json.JSONDecodeError) separately and treat them as retryable (do not early-return); also add explicit catches for requests.Timeout and requests.RequestException where appropriate and treat them as retryable. Replace simple logger.error calls (notably around line 237) with logger.exception to include tracebacks, and ensure json (for JSONDecodeError) and requests exceptions are imported/used accordingly. Ensure each except block only handles the intended error class and logs context+traceback via logger.exception where debugging info is needed. 1 participant
Summary by CodeRabbit