Produits Entr'ouvert » Passerelle

Est-ce qu'on en profite pour ajouter un support asynchrone général dans passerelle ? L'idée de base ce serait au niveau de la ressource on met un flag "queue_on_error = True" et en cas d'erreur on créé un objet queue avec le contenu du POST et on se débrouille pour qu'une tâche cron rejoue ça, parce que des connecteurs "fire and forget" on n'aura pas que celui là. Plus tard on pourra faire évoluer ça pour permettre une notification de w.c.s. quand ça finit par passer.

Un modèle comme cela:

class Job:
   ressource = FK(ContentType)
   endpoint = string 
   data = json : None
   result = json : None
   status = enum(TODO, ERROR, UNRECOVERABLE_ERROR, DONE) : TODO
   tries = int : 0
   next_try = datetime : None
   time_between_tries = int : 60
   exponential_retry_timeout_factor = decimal : 1
   max_tries = int : 

C'est vaguement inspiré du système de job que j'ai dans zoo.

À noter aussi #3090 et #12469.

Il faudrait que ça soit tout le temps asynchrone, et qu'il y ait plusieurs tentatives sur l'envoi des messages en cas d'échec.

Il y a un job.parameters qui permet de ne pas créer de modèle.

Oui, je l'ai utilisé dans le premier patch.
Ensuite on peut relancer les Jobs avec SkipJob, mais j'ai l'impression qu'alors on ne peut pas garder trace des erreurs.

Nicolas Roche a écrit :

Oui, je l'ai utilisé dans le premier patch.
Ensuite on peut relancer les Jobs avec SkipJob, mais j'ai l'impression qu'alors on ne peut pas alors garder traces des erreurs.

• pour les récupérables tu les logs et tu raises SkipJob,
• pour les non récupérables tu les raises et ce sera enregistré dans le job par handle_job_error()

En v1 ça me parait bien suffisant, il suffira de chercher un numéro de téléphone dans les logs pour savoir ce qui lui est arrivé (pourvu que ce soit loggé correctement).

• différencier dans GenericViewJobsConnectorView les jobs qui n'ont jamais été exécutés des jobs en retry (visible via creation_timestamp != update_timestamp),
• trouver un moyen de passer un ProxyLogger particulier à la méthode exécutée construit via resource.log.context(job_id=job.id) pour pouvoir afficher les logs d'un job en particulier (via @ResourceLog.objects.filter(extra__job_id=job.id)).

Mais c'est un travail plus global à penser pour bénéficier à tous les connecteurs.

¹ Ici l'absence d'accès à l'objet job depuis la méthode appelée rend les choses simples et empêche de faire des trucs sioux ; mais c'est vrai que pour gérer une politique de retry un peu complexe ça nous limite (genre essayer 10 fois, en attendant 5, puis 10, 30, 60, 120, 240, etc.. minutes, sans savoir quand on a commencé ou combien de fois on a essayé c'est pas simple). On pourrait prévoir un attribute nees_job = True ajouté à la méthode et dans ce cas on lui passe le job en premier paramètre.

Un truc que tu peux faire aussi c'est alloué un natural_id à tes jobs pour pouvoir retrouver les jobs qui étaient ensembles et plus tard pouvoir récupérer leur statut groupé; natural_id que tu peux renvoyer dans ta réponse à l'appelant.

De manière pas générique, à travailler avec un modèle dédié, il y a le modèle SMSLog introduit dans #39651 qui pourrait être étendu, avec les informations de contenu/destinataire de message, et statut de l'envoi, on pourrait alors avoir les choses découpées :

• job qui reçoit l'id d'un enregistrement "SMSLog", tente l'envoi,
• cron hourly qui prend tous les SMSLog pas réussis à être transmis, vieux entre 5 minutes et ?, et retente,
• sur succès d'envoi les infos destinataire et contenu du message seraient vidées.

Pour natural_id j'aurai juste mis un str(uuid.uuid4()) là on va avoir le même id pour un même envoi (et encore ça dépendra de l'ordre des numéros dans destinations, enfin ça se discute).

0002 me semble dupliquer tout le code d'envoi.

Aussi je notais plus haut :

Il faudrait que ça soit tout le temps asynchrone [...]

Je voulais dire par là que l'endpoint send devait être asynchrone, qu'il ne devait pas y avoir de nouvel endpoint.

Très basiquement, quand j'ai réévoqué ce ticket, je pensais juste à zéro modif côté connecteurs, quelque chose de global, approchant ceci :

@@ -962,9 +962,15 @@ class SMSResource(BaseResource):
         logging.info('sending message %r to %r with sending number %r',
                      data['message'], data['to'], data['from'])
         stop = not bool('nostop' in request.GET)
-        result = {'data': self.send_msg(data['message'], data['from'], data['to'], stop=stop)}
+        self.add_job('send_job', stop=stop, **data)
+        return {'err': 0}
+
+    def send_job(self, **kwargs):
+        try:
+            self.send_msg(**kwargs)
+        except whatever:
+            # deal with it
+            return
         SMSLog.objects.create(appname=self.get_connector_slug(), slug=self.slug)
-        return result

qui correspond assez à ton premier patch; modulo mon commentaire "tout le temps async".

Et puis je ne comprends pas comment ce commentaire "tout le temps async" amène à la production de la nouvelle série, qui commence à modifier partout, ajouter des modèles, etc. et une discussion qui part là-dessus, alors que non, c'est à la simplicité d'origine qu'il faut retourner.

Désolé, c'est moi qui ai mal interprété.

Mon commentaire été lié au sujet du ticket :

Il faudrait gérer une file des messages, que l'envoi se passe de manière asynchrone.

Mon commentaire n'étais vraiment pas (du tout) clair.

Je précise que ce patch ne gère pas l'envoie d'un lot de messages de façon asynchrone.

• il n'y a pas de reprise sur exception
• les messages ne sont pas envoyés de façon unitaire

J'ai sur-interprété ta réponse avec la pensée implicite que les SMS devaient être envoyés de façon unitaire : si l'on envoie tous les SMS (un même texte peut être adressé à plusieurs destinataires via un unique appel au endpoint), et que seul le n-ième SMS plante, alors on va renvoyer tous les autres SMS une seconde fois.

Je suis pour ne pas vouloir en faire trop, abysse, déjà 17 commentaires, etc. Limitons ce ticket à l'envoi aynchrone, qui n'offrira pas plus de gestion d'erreurs que la situation présente.

On verra ensuite dans un autre ticket pour s'occuper de la gestion des erreurs, là on pourra avoir la question "quid si un message dans une série échoue ?", là on pourra s'interroger sur le moment ou les moments où tenter la réexpédition, etc.

• J'ai du inventer une erreur libellée "connection timed-out" pour les (...)

Parce que tu négliges "On verra ensuite dans un autre ticket pour s'occuper de la gestion des erreurs.". Laisse ça à un autre ticket, où ça pourra être approché spécifiquement, où on pourra se dire qu'il y a à uniformiser des bouts entre connecteurs plutôt qu'essayer de matcher des bouts de chaines.

J'ai du retirer 2 tests sur "parameters.responses_max_size" parce que le 'body' n'est pas logué dans les Jobs.

C'est peut-être un problème en soit, par quelle mécanique celui-ci devrait être loggué, pourquoi ne l'est-il pas ici, ça peut faire l'objet d'un ticket séparé, etc.

Est-ce qu'il faut que je réécrive test_api_access.py en me basant sur un autre connecteur ?

J'aurais volontiers pointé que la gestion des accès ne devrait pas changer.

Mais à regarder le patch, elle ne change pas, il y a juste zéro raison à la présence d'un "test_logged_requests_and_responses_max_size" dans test_api_access.py.

Ça me semble plutôt relever de tests/test_proxylogger.py.

J'ai noté dans #42230 qu'il ne me semble pas opportun. Et k'ai noté ici, plus haut, "On verra ensuite dans un autre ticket pour s'occuper de la gestion des erreurs.".

Vraiment : d'abord l'envoi asynchrone, ensuite la gestion des erreurs.

#21465#note-18

    def send_job(self, *args, **kwargs):
        try:
            self.send_msg(**kwargs)
        except APIError:
            raise  # not recoverable

Ça rend l'erreur silencieuse, faut pas.