Projet

Général

Profil

Development #24327

alertes peu utiles sur les pépins de workflow

Ajouté par Thomas Noël il y a presque 6 ans. Mis à jour il y a plus de 5 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
-
Version cible:
-
Début:
07 juin 2018
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:

Description

Suite à #23894 on reçoit pas mal d'alertes sur des "trous" dans les workflows, ie des sauts qui n'ont plus de cible, parce que le statut correspondant a été supprimé.

Mais c'est pas très pertinent, parce que :
  • ces alertes n'ont aucun contexte (juste le nom du workflow, mais pas le tenant)
  • et surtout ces alertes n'arrivent pas à la bonne personne (le développeur du workflow)
  • et notamment elles ne sont pas dans la colonne "erreur" du workflow

A priori un passage en get_publisher().notify_of_exception... pourrait déjà rentre l'affaire plus claire.


Fichiers

Révisions associées

Révision 30732d5b (diff)
Ajouté par Thomas Noël il y a presque 6 ans

misc: only log missing statuses as functional errors (#24327)

Révision e6175685 (diff)
Ajouté par Thomas Noël il y a presque 6 ans

tests: do not check mail message on logged errors backoffice (#24327)

Révision 9a86adf1 (diff)
Ajouté par Thomas Noël il y a presque 6 ans

workflows: use format strings on logged error message (i18n) (#24327)

Historique

#1

Mis à jour par Frédéric Péters il y a presque 6 ans

ces alertes n'ont aucun contexte (juste le nom du workflow, mais pas le tenant)
et surtout ces alertes n'arrivent pas à la bonne personne (le développeur du workflow)

Oui et c'est pour moi à cause du logging de Django, j'ai créé #24331.

et notamment elles ne sont pas dans la colonne "erreur" du workflow

Oui ça manque.

A priori un passage en get_publisher().notify_of_exception... pourrait déjà rentre l'affaire plus claire.

Je serais plutôt pour passer directement par LoggedError, que ça apparaisse uniquement dans l'UI.

#2

Mis à jour par Thomas Noël il y a presque 6 ans

Pour étude/discussion, ce patch qui permet de faire des notifications directemetn liées à un formdata :

get_publisher().notify_of_exception(sys.exc_info(), formdata=formdata)

Dans le cas où le formdata n'est pas None la notification consiste pour l'instant à ajouter une LoggedError (plus tard ça pourrait envoyer un mail aux gens concernés par le formulaire et/ou le workflow, ou tout autre moyen de notifier un pépin).

Le patch joint utilise la possibilité dans un seul cas, qui est la détection d'un saut raté (l'objet du présent ticket).

Manquent les tests, parce que je veux bien d'abord un avis avant de m'y lancer.

#3

Mis à jour par Frédéric Péters il y a presque 6 ans

J'aurais préféré ne pas passer du tout par publisher, plutôt :

-           get_publisher().get_app_logger().error(
+           LoggedError.record(..., formdata=formdata)

Et qu'on ait une restructuration côté LoggedError, dans ce genre :

class LoggedError(...):
    @classmethod
    def record(cls, error_summary, plain_error_msg, formdata=None, formdef=None, workflow=None):
    def record(cls, error_summary, plain_error_msg, publisher=None, formdata=None):
        error = cls()
        error.summary = error_summary
        error.traceback = plain_error_msg
        error.first_occurence_timestamp = datetime.datetime.now()
        error.formdata_id = formdata.id if formdata else None
        ...

     @classmethod
     def record_exception(cls, error_summary, plain_error_msg, publisher):
         ... récup de formdata/formdef/workflow via publisher.substitutions.get_context_variables()
         cls.record(publisher.substitutions.get_context_variables, formdata=... etc.)
#4

Mis à jour par Thomas Noël il y a presque 6 ans

Frédéric Péters a écrit :

J'aurais préféré ne pas passer du tout par publisher, plutôt : (...)

Oui je pensais prendre ce chemin, mais «plain_error_msg» doit être généré via la méthode "privée" publisher._generate_plaintext_error, ça donnait pas un truc joli (à mon goût). Et j'étais dans l'idée que get_publisher().notify_of_exception(formdata=truc) pourrait effectivement, plus tard, notifier et pas juste enregistrer l'erreur.

(je regarde ce que je peux faire quand même)

#5

Mis à jour par Frédéric Péters il y a presque 6 ans

On pourrait avoir plain_error_msg à None, non ? La trace qui fait que finalement ça touche un statut inconnu, on s'en fout un peu. (mais on pourrait par contre ajouter de manière précise le statut et l'action, pour avoir un lien direct).

#6

Mis à jour par Thomas Noël il y a presque 6 ans

Frédéric Péters a écrit :

On pourrait avoir plain_error_msg à None, non ? La trace qui fait que finalement ça touche un statut inconnu, on s'en fout un peu. (mais on pourrait par contre ajouter de manière précise le statut et l'action, pour avoir un lien direct).

Ah, vu comme ça, oui. Reste que pour les autres logs à venir (plantages sur conditions ou expressions) il faudrait quand même garder trace du contexte du calcul (ie les variables, qu'on a déjà au moment de ces calculs, donc pas besoin de get_publisher). Je regarde comme ça.

#7

Mis à jour par Thomas Noël il y a presque 6 ans

Voici donc une version qui ne fait appel qu'à LoggedError.record. On enregistre quand c'est transmis le statut et l'action en jeu. Il n'y a pas toujours de traceback. Le résumé pourra être traduit.

#8

Mis à jour par Thomas Noël il y a presque 6 ans

  • Assigné à Thomas Noël supprimé
  • Patch proposed changé de Oui à Non

Ne passe pas les tests, rewrite en cours.

#9

Mis à jour par Frédéric Péters il y a presque 6 ans

(ok, pendant qu'ils tournaient chez moi, j'écrivais  : un peu dommage d'avoir à la fois formdata et formdata_id comme paramètres, mais détail, ack. (mais pas fan du message, plutôt genre "misc: only log missing statuses as functional errors (#...)")).

#10

Mis à jour par Thomas Noël il y a presque 6 ans

Fix des tests, retrait du formdata_id au profit du seul formdata, suppression de la mention "cette erreur a été mailée à ..." dans le backoffice.

#11

Mis à jour par Frédéric Péters il y a presque 6 ans

Hop, ack.

#12

Mis à jour par Thomas Noël il y a presque 6 ans

  • Statut changé de En cours à Résolu (à déployer)
commit 30732d5b12e9b6e70f0d97d5583d3fe6927357e6
Author: Thomas NOEL <tnoel@entrouvert.com>
Date:   Thu Jun 7 12:16:20 2018 +0200

    misc: only log missing statuses as functional errors (#24327)

#13

Mis à jour par Thomas Noël il y a presque 6 ans

Quelques maladresses...

commit 9a86adf174bb70583b0d553a4a9899485d48233b
Author: Thomas NOEL <tnoel@entrouvert.com>
Date:   Thu Jun 7 18:05:13 2018 +0200

    workflows: use format strings on logged error message (i18n) (#24327)

commit e61756856129c164024cdb7bb7474e4832b16c71
Author: Thomas NOEL <tnoel@entrouvert.com>
Date:   Thu Jun 7 18:02:35 2018 +0200

    tests: do not check mail message on logged errors backoffice (#24327)

#14

Mis à jour par Frédéric Péters il y a plus de 5 ans

  • Statut changé de Résolu (à déployer) à Solution déployée

Formats disponibles : Atom PDF