Skip to content
This repository has been archived by the owner on Aug 5, 2024. It is now read-only.

Cron: handle error from OVH when creating email #378

Merged
merged 16 commits into from
Jan 8, 2021

Conversation

polomarcus
Copy link
Contributor

@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 6, 2021 16:29 Inactive
@polomarcus polomarcus changed the title Cron: handle error from OVH when creating email WIP: Cron: handle error from OVH when creating email Jan 6, 2021
@polomarcus polomarcus changed the title WIP: Cron: handle error from OVH when creating email WIP : Cron: handle error from OVH when creating email Jan 6, 2021
@eml-trm eml-trm marked this pull request as draft January 6, 2021 16:49
@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 8, 2021 10:50 Inactive
@LucasCharrier LucasCharrier linked an issue Jan 8, 2021 that may be closed by this pull request
@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 8, 2021 11:25 Inactive
@polomarcus polomarcus changed the title WIP : Cron: handle error from OVH when creating email Cron: handle error from OVH when creating email Jan 8, 2021
@eml-trm eml-trm marked this pull request as ready for review January 8, 2021 13:20
@eml-trm eml-trm requested a review from jdauphant January 8, 2021 13:20
tests/test-user.js Outdated Show resolved Hide resolved
tests/test-user.js Outdated Show resolved Hide resolved
tests/test-user.js Outdated Show resolved Hide resolved
tests/test-user.js Outdated Show resolved Hide resolved
@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 8, 2021 14:15 Inactive
@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 8, 2021 14:36 Inactive
@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 8, 2021 15:27 Inactive
@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 8, 2021 15:51 Inactive
@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 8, 2021 16:02 Inactive
@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 8, 2021 16:07 Inactive
tests/test-user.js Outdated Show resolved Hide resolved
tests/test-user.js Outdated Show resolved Hide resolved
tests/test-user.js Outdated Show resolved Hide resolved
tests/test-user.js Outdated Show resolved Hide resolved
tests/test-user.js Outdated Show resolved Hide resolved
LucasCharrier and others added 4 commits January 8, 2021 17:30
Co-authored-by: Alejandro M Guillén <alejandro@amguillen.dev>
Co-authored-by: Alejandro M Guillén <alejandro@amguillen.dev>
Co-authored-by: Alejandro M Guillén <alejandro@amguillen.dev>
Co-authored-by: Alejandro M Guillén <alejandro@amguillen.dev>
@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 8, 2021 16:31 Inactive
@jdauphant jdauphant temporarily deployed to secretariat-fix-372-ovh-nmximn January 8, 2021 16:36 Inactive
@LucasCharrier LucasCharrier merged commit 4549b24 into master Jan 8, 2021
@LucasCharrier LucasCharrier deleted the fix/#372-ovh-cron branch January 8, 2021 16:39
Comment on lines +33 to +37
return await Promise.all(emailCreationTasks);
} catch (err) {
console.error(err);
return Promise.resolve();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si le job crash en continue, on aura un point d'alerte ?

@jdauphant
Copy link
Member

jdauphant commented Jan 8, 2021

Je ne suis pas sûr que ça résolve le problème.
On a OVH qui renvoit une erreur 500, qu'est-ce qu'on fait ?

Le comportement avant la PR

C'est de crasher -> l'app ne peut pas fonctionner.
Scalingo nous alerte et relance le job -> ça redémarre et on est au courant

La solution adopté dans la PR

Déplacer l'erreur dans les logs (ce n'est pas mettre un peu la poussière sous le tapis de log ?)
Il n'y a plus d'alerte, le job est arrété silencieusement -> Je vois une régression ( Après je me trompe peut être vous avez mis en place une veille des logs entre vous pour vérifier ^^)

J'aurais plutôt vu, comme solutions :

  • On ne change rien, l'erreur n'est pas assez fréquente, il y a un système de relance par Scalingo. On peut en profiter pour signaler l'erreur au support OVH
    Ou
  • On relance le call en échec (plutôt sur les GET) et on lance une exception uniquement au 2 ou 3 ème essai sur l'appel

My 2 cents

@alemangui
Copy link
Collaborator

@jdauphant en effet, le 500 d'OVH n'est pas permanent, ça arrive de temps en temps et disparaît dans l'itération suivante.

Par contre, je ne suis pas sur que le crash du cron soit souhaitable dans ce cas. Ama, la tâche de cron doit pouvoir échouer gracieusement dans ce cas et logger l'erreur sans que le processus lance une exception non gérée (et par conséquent envoie un email aux admins Scalingo). Sauf erreur de ma part dans le cron du marrainage il y a une approche similaire.

Je serais plutôt dans le camp de soit faire de relances ou de mettre en place un système de suivi de logs. On avait discuté d'un Sentry dans #143, ce serait peut être l'occasion de s'y attaquer ?

@jdauphant
Copy link
Member

On n'a pas le sentry pour le moment, on dépend des rapports de crashs de scalingo. Ça n'aurait pas du être fait en premier ? Une fois mis en place, cette PR fait plus de sens.
Est-ce que sentry marche aussi dans ce cas là en JS, tu sais comment ça marche ? ça parse le log de sortie ?

On pourrait rendre le job plus robuste sur cette erreur en particulier en traitant les erreurs aussi au bon niveau pour le moment.

@alemangui
Copy link
Collaborator

Ça marche, on pourra faire des modifs dans cet enroit (ou faire un revert) le temps que Sentry soit en place.

Il y a moyen de capturer le console.error dans Sentry une fois que ça sera mis en place via le CaptureConsole, qui pourrait être configuré pour capturer seulement la méthode error, quelque chose comme :

Sentry.init({
    integrations: [
        new CaptureConsole({
            levels: ['error']
        })
    ]
})

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash du cron lors d'une erreur OVH
5 participants