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

Call to Ads API doesn't throw error on failure. #675

Open
JBarti opened this issue Jun 24, 2024 · 1 comment
Open

Call to Ads API doesn't throw error on failure. #675

JBarti opened this issue Jun 24, 2024 · 1 comment

Comments

@JBarti
Copy link

JBarti commented Jun 24, 2024

While trying to gather all adcreatives from a customer ad account I managed to trigger an unexpected error in the Ads API. The library handles this response incorrectly by not raising an error even though the response status is 400.

Details

  1. Try to retrieve all adcreatives from a single ad account, together with all the existing fields, with the response limit set to 100 (the account has more than a thousand adcreatives).
  2. After around 10 pages are retrieved facebook returns the body in HTML format with a status code of 400:
<!DOCTYPE html>
<html lang="en" id="facebook">
  <head>
    <title>Facebook | Error</title>
    <meta charset="utf-8">
    <meta http-equiv="cache-control" content="no-cache">
    <meta http-equiv="cache-control" content="no-store">
    <meta http-equiv="cache-control" content="max-age=0">
    <meta http-equiv="expires" content="-1">
    <meta http-equiv="pragma" content="no-cache">
    <meta name="robots" content="noindex,nofollow">
    <style>
      html, body {
        color: #141823;
        background-color: #e9eaed;
        font-family: Helvetica, Lucida Grande, Arial,
                     Tahoma, Verdana, sans-serif;
        margin: 0;
        padding: 0;
        text-align: center;
      }

      #header {
        height: 30px;
        padding-bottom: 10px;
        padding-top: 10px;
        text-align: center;
      }

      #icon {
        width: 30px;
      }

      h1 {
        font-size: 18px;
      }

      p {
        font-size: 13px;
      }

      #footer {
        border-top: 1px solid #ddd;
        color: #9197a3;
        font-size: 12px;
        padding: 5px 8px 6px 0;
      }
    </style>
  </head>
  <body>
    <div id="header">
      <a href="//www.facebook.com/">
        <img id="icon" src="//static.facebook.com/images/logos/facebook_2x.png" />
      </a>
    </div>
    <div id="core">
      <h1 id="sorry">Sorry, something went wrong.</h1>
      <p id="promise">
        We're working on it and we'll get it fixed as soon as we can.
      </p>
      <p id="back-link">
        <a id="back" href="//www.facebook.com/">Go Back</a>
      </p>
    … [message truncated due to size]
  1. The is_success method in the facebook_business.api module fails to identify this response as a failure and returns True.

Code

The code provided encounters this issue when accessing the /act_<account_id>/adcreatives endpoint. Here's a breakdown of what the code does:

  1. API Call: It makes a request to a specified path of the Facebook Ads API.
  2. Response Parsing: It parses the API response using a Pydantic model.
  3. Pagination Handling: If the response is paginated, it repeats the entire process using the paging.next URL.
    This approach ensures that the full data for a single edge is retrieved.
def _call_fb_api_with_retry(api: FacebookAdsApi, method: str, path: str, params: dict, max_retries: int = 3) -> FacebookResponse:
    """Call the Facebook API with retries."""
    for i in range(max_retries):
        try:
            return api.call(method=method, path=path, params=params)
        except Exception as e:
            logger.error(e)
            if i == max_retries - 1:
                raise e
            logger.info(f"Retrying {method} {path} with params {params} (attempt {i + 1}/{max_retries})")

    raise Exception("This should never happen")


def _get_fb_data(
    api: FacebookAdsApi,
    validation_model: Type[BaseModel],
    endpoint: str,
    params: dict,
    limit: int = 1000
) -> Generator[list[BaseModel], Any, Any]:
    """Get data from the Facebook API and yield the data in chunks."""
    base_url = "https://graph.facebook.com/v20.0"
    path = f"{base_url}{endpoint}"

    while path:
        resp = _call_fb_api_with_retry(
            api=api,
            method="GET",
            path=path,
            params={
                **params,
                "limit": limit,
                "time_increment": 1,
            },
        )

        raw_data = resp.json()

        if not raw_data:
            logger.info("No data for endpoint ", endpoint)
            return

        try:
            fb_body_model = validation_model(**raw_data)
        except Exception as e:
            logger.error(e)
            logging.error("Status Code: %s" % resp.status())
            logging.error("Error: %s" % resp.error())
            logging.error("Body: %s" % resp.body())
            raise e

        if isinstance(fb_body_model, EdgeRequestResponseBody):
            path = fb_body_model.paging.next if fb_body_model.paging else None
            logger.info("Next page: %s" % path)
            yield fb_body_model.data
        else:
            path = None
            yield [fb_body_model]
@PacificGilly
Copy link

PacificGilly commented Jun 25, 2024

We've experienced the exact same issue on occasion as the status code isn't being acted on, even 5xx codes.

Think the simplest and cleanest solution might be to implement a proper Retry strategy directly at the low-level API request call being made:

response = self._session.requests.request(
method,
path,
params=params,
headers=headers,
files=files,
timeout=self._session.timeout
)
else:
response = self._session.requests.request(
method,
path,
data=params,
headers=headers,
files=files,
timeout=self._session.timeout

Potential Fix

from urllib3 import Retry
from requests import Session()
from requests.adapters import HTTPAdapter


DEFAULT_RETRIES = 5  # Allows configuration.
DEFAULT_BACKOFF_FACTOR = 0.5  # Time doubles between API calls.


session = Session()
retry = Retry(
    total=DEFAULT_RETRIES,
    # Matches `settings.RETRY_HTTP_CODES` in lyst/checkout.
    status_forcelist=[
        HTTP_408_REQUEST_TIMEOUT,
        HTTP_429_TOO_MANY_REQUESTS,
        HTTP_500_INTERNAL_SERVER_ERROR,
        HTTP_503_SERVICE_UNAVAILABLE,
        HTTP_504_GATEWAY_TIMEOUT,
    ],
    backoff_factor=DEFAULT_BACKOFF_FACTOR,
    respect_retry_after_header=True,
)
adapter = HTTPAdapter(max_retries=retry)
# Multiple `mount`s shouldn't be an issue since it's just updating a dict key.
# Preferably declare them by descending prefix length as per below.
session.mount("https://", adapter)
session.mount("http://", adapter)

Happy to raise a PR if useful

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

No branches or pull requests

2 participants