API particulier : Respecter la fréquence d’appels et les workers web#4983
API particulier : Respecter la fréquence d’appels et les workers web#4983francoisfreitag merged 1 commit intomasterfrom
Conversation
francoisfreitag
force-pushed
the
ff/rate-limits
branch
from
70c0ef5 to
49bc79e
Compare
francoisfreitag
force-pushed
the
ff/rate-limits
branch
from
49bc79e to
61d719c
Compare
francoisfreitag
force-pushed
the
ff/rate-limits
branch
from
61d719c to
47acda2
Compare
| certify_criteria(self) | ||
| except httpx.HTTPError: | ||
| logger.info("Could not certify criteria synchronously.", exc_info=True) | ||
| async_certify_criteria(self._meta.model_name, self.pk) |
There was a problem hiding this comment.
Toute autre erreur (y compris d'autres exceptions de httpx qui ne sont pas sous HTTPError) va casser et faire échouer la transition accept. Est-ce vraiment souhaité ?
Est-ce que l'on ne rajouterait pas un except générique avec un logger.exception pour fail safe mais quand même avoir une trace ?
There was a problem hiding this comment.
Toutes les exceptions de httpx héritent de HTTPError: https://www.python-httpx.org/exceptions/
On peut effectivement laisser un except générique pour être sûrs de ne pas bloquer une embauche pour un problème d’intégration partenaire externe (genre format de réponse de l’API qui a changé). 👍
On doit pouvoir laisser au niveau info, car on va réessayer, et l’erreur remontera à ce moment là ?
There was a problem hiding this comment.
Est-ce pertinent de réessayer pour ces cas de figure ?
There was a problem hiding this comment.
Démarrer une tâche asynchrone pour réessayer me semble pertinent : une erreur réseau devrait être résolue et l’app devrait pouvoir interroger l’API particulier au prochain essai.
Pour la politique de ré-essai côté tâche asynchrone, j’ai fait un truc assez simple. À voir si on complexifie.
francoisfreitag
force-pushed
the
ff/rate-limits
branch
3 times, most recently
from
58d8db6 to
0be3bbf
Compare
francoisfreitag
force-pushed
the
ff/rate-limits
branch
10 times, most recently
from
214ab46 to
0a544d5
Compare
itou/eligibility/tasks.py
Outdated
| data = api_particulier.revenu_solidarite_active(client, job_seeker) | ||
| except httpx.HTTPStatusError as exc: | ||
| criterion.data_returned_by_api = exc.response.json() | ||
| logger.error("%d: %s", exc.response.status_code, criterion.data_returned_by_api) |
There was a problem hiding this comment.
Est-ce qu'on ne voudrait pas un peu plus de contexte dans ces logs ?
There was a problem hiding this comment.
Genre la PK du diag/critère d’éligibilité ?
There was a problem hiding this comment.
Oui avec un peu de texte genre Error certifying criterion=Model[123] - code=500 data={} et éviter des lignes de logs du genre 500: {} 😅
francoisfreitag
force-pushed
the
ff/rate-limits
branch
2 times, most recently
from
9c2dd0f to
0ef3ed9
Compare
itou/eligibility/tasks.py
Outdated
| # The data provider for API particulier returned a 429. | ||
| # Re-raising will cause huey to retry after a long retry_delay, | ||
| # let’s hope the data provider rate limit has been reset by then. | ||
| raise |
There was a problem hiding this comment.
Ne pourrait-on pas considérer qu'il s'agit d'une 429 sous couverture et lever une RetryTask également ?
There was a problem hiding this comment.
Si, tout à fait. Le problème est de savoir quand retry, car l’API ne nous communique pas les instructions du fournisseur de données. Du reste, si j’ai bien compris, le fournisseur de données ne communique pas non plus ses instructions ré-essai. 🙈
Dans le doute, je me suis dit qu’on pouvait laisser le mécanisme générique (attendre 10 minutes) entrer en jeu.
On peut aussi RetryTask qui a l’avantage de ne pas changer le nombre de retry de la tâche, et est peut-être plus clair. On peut fixer le délai à 10 minutes pour le moment, et on le fera évoluer si besoin. 👍
Move API calls to an async task, to avoid holding up web workers with sleep. Having tenacity retry during the request/response cycle was very risky, as a few concurrent certifications with errors (e.g. downtime on the API) could bring down all workers for multiple seconds. Implement a retry policy based on indications from the API. `test_service_unavailable` was updated to use a payload matching what the API actually returns and not what the OpenAPI specification describes.
francoisfreitag
force-pushed
the
ff/rate-limits
branch
from
0ef3ed9 to
6893b40
Compare
|
J’ai ajouté du contexte au message de log, et fait une |
4 participants
🤔 Pourquoi ?
Être un bon citoyen, éviter de tabasser l’API des copains.
Éviter de faire attendre des workers web, car on peut vite saturer notre bande passante (DoS). Par exemple, si l’API se met à répondre avec des 500, chaque worker qui essaye de certifier un critère sera bloqué pour 3 * 2 = 6s, à attendre et réessayer. En augmentant le nombre de critères certifiés, on peut assez vite faire tomber le service, où tous les workers sont en train de
time.sleep(2)(les fainéants !).🍰 Comment ?
Suivre les indications de l’API quant à la fréquence des appels.
Ne faire que le premier essai de certification en synchrone. En cas d’échec, démarrer une tâche asynchrone qui effectuera la vérification.