Feat(alerts): add alert on mattermost in case of cron failure #280

hlecuyer · 2024-08-28T15:32:24Z

No description provided.

vperron

Quelques petits commentaires ! Et perso je ne l'appellerais pas run_management_command.sh ; c'etait pour les emplois spécifiquement parce que eux ils wrappent django-admin qui lance des... "management commands" Django.

Nous on a plutot un ... execute_and_notify.sh ?

vperron · 2024-08-29T08:57:41Z

api/run_management_command.sh

+#!/bin/sh
+set -u
+
+# Execute the command and capture standard output and error output


attention aux commentaires misleadings ou inutiles.

$@ & : on sait que ça va executer la commande en detached

... par contre je ne vois pas comment cela capturerait les sorties ?

Commentaires obsoletes

vperron · 2024-08-29T08:57:48Z

api/run_management_command.sh

@@ -0,0 +1,21 @@
+#!/bin/sh
+set -u


Pour quoi faire ?

c'est sense rendre le script plus robuste. Ca genere une erreur en cas d'utilisation de variables non definit. Pas indispensable, mais pas incompatible.

Pourquoi pas, mais bon a priori a part les variables d'environnement (pas sûr qu'elles soient prises par -u ? A vérifier !) on les définit toutes en interne les variables bash donc ça va.

Ca aurait pu être intéressant si par exemple on était capables de crasher si $MATTERMOST_WEBHOOK_URL est non défini mais bon, dans ce cas on aura pas de notification non plus ^^ Scalingo va juste rester silencieux comme d'hab ! Donc so far, je vois pas trop à quoi ça servira dans le fond.

Les var d'env sont prise par le -u :)

Je fais souvent des choses par mimetisme (peut etre une habitude perdre). Les script sh commence souvent par ca. On peut le virer pour etre un peu plus yagni. J'ai pas d'avis fort la dessus.

Ca ne changera rien au schmilblick final mais bon, gardons le ça ne pose pas de souci !

vperron · 2024-08-29T08:58:11Z

api/run_management_command.sh

+
+wait $command_pid
+
+exit_code=$?  # Capture the exit code of the previous command


je pense que là aussi, vu le nom de la variable, on peut se passer du commentaire ^

vperron · 2024-08-29T08:59:05Z

api/run_management_command.sh

+
+exit_code=$?  # Capture the exit code of the previous command
+
+# Check if the exit code indicates an error (not equal to 0)


pareil ici :)
https://blog.codinghorror.com/code-tells-you-how-comments-tell-you-why/

vperron · 2024-08-29T09:02:54Z

api/run_management_command.sh

+    # Execute the curl command in case of an error
+    escaped_command="in command '$@'"
+    echo "Error with exit code $exit_code $escaped_command, sending message to Mattermost"
+    curl -i -X POST -H "Content-Type: application/json" -d "{\"text\": \"Error with exit code $exit_code $escaped_command:  \nLogs: scalingo --region osc-secnum-fr1 --app data-inclusion-api-prod logs --lines 1000 -F $CONTAINER -f\"}" $MATTERMOST_HOOK


nice :)

Je raffinerais encore un poil plus:

Error $exit_code with command : `$escaped_command`

(note les `` qui seront formatés par Mattermost)

Ensuite pour la commande de logs, je me demande pourquoi le -f ou les 1000 lignes ? Et pareil, je le mettrais bien dans des backticks pour que ça soit chouli dans Mattermost.

-f pour la navigation
1000 ligne c'est pour eviter d'en avoir trop
Ok pour les ``

Yes mais:

le -f te donne juste la "suite" de la commande or elle est déjà crashée a priori, donc pas de nouvelles lignes n'arriveront ?

1000 lignes : ok, pourquoi pas !

vperron · 2024-08-29T09:04:54Z

api/run_management_command.sh

+# Check if the exit code indicates an error (not equal to 0)
+if [ $exit_code -ne 0 ] && [ "$ENV" = "prod" ]; then
+    # Execute the curl command in case of an error
+    escaped_command="in command '$@'"


C'est pour éviter d'envoyer des trucs bizarres dans Mattermost c'est ça ? Il s'est passé des choses bizarres ? En tout cas je recommande le backtick plus que les guillemets, cf. plus bas.

J'ai galerer avec le format, j'avais des erreur chelou de caracter non escaped, je vais essayer de les remettre

Hmmm, intéressant, je veux bien voir dans quels cas du coup le format rend mattermost pas content, c'est une bonne info

Si tu as des soucis de caractères non échappés, potentiellement je te recommande d'utilise $* au lieu de $@.

"$@" (donc mis dans une chaine de caractères) devient une liste d'arguments séparés. "$*" devient un seul paramètre, qui sont tous les arguments du script ($1, $2, ...) concaténés en un seul.

vperron · 2024-08-29T09:36:02Z

api/Dockerfile

 RUN chmod +x docker-entrypoint.sh
+RUN chmod +x run_management_command.sh


vperron · 2024-08-29T09:36:12Z

api/cron.json

@@ -1,15 +1,15 @@
 {
    "jobs": [
        {
-            "command": "30 * * * * TQDM_DISABLE=1 test $ENV = 'prod' && data-inclusion-api load_inclusion_data",
+            "command": "30 * * * * TQDM_DISABLE=1 test $ENV = 'prod' && sh run_management_command.sh data-inclusion-api load_inclusion_data",


vperron · 2024-08-29T09:36:42Z

api/execute_and_notify.sh

+
+exit_code=$?  
+if [ $exit_code -ne 0 ] && [ "$ENV" = "prod" ]; then
+    # Execute the curl command in case of an error


tjs inutile je pense :) surtout avec le print qui vient expliquer exactement ça juste en dessous

vperron · 2024-08-29T09:39:53Z

api/cron.json

            "size": "XL"
        },
        {
-            "command": "30 * * * * TQDM_DISABLE=1 test $ENV != 'prod' && data-inclusion-api load_inclusion_data",
+            "command": "30 * * * * TQDM_DISABLE=1 test $ENV != 'prod' && sh run_management_command.sh data-inclusion-api load_inclusion_data",


pas nécessaire de le mettre en staging du coup ? puisque à l'intérieur du script on hardcode "prod" deux fois ^^

vperron

LVGTM !

(tu peux virer le sh xxxx.sh puisque a priori, tu as:

un shbang
les droits d'execution appliqués via chmod dans le Dockerfile

Vérifie quand meme que le fichier est executable dans le repository pour que Scalingo puisse le lancer sans pb

…in case of failure

hlecuyer requested a review from vmttn as a code owner August 28, 2024 15:32

hlecuyer temporarily deployed to staging August 28, 2024 15:36 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 28, 2024 15:36 — with GitHub Actions Failure

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch from accd390 to 9e6d584 Compare August 28, 2024 15:44

hlecuyer temporarily deployed to staging August 28, 2024 15:48 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 28, 2024 15:48 — with GitHub Actions Failure

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch from 9e6d584 to 1e46fc1 Compare August 28, 2024 15:52

hlecuyer temporarily deployed to staging August 28, 2024 15:56 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 28, 2024 15:56 — with GitHub Actions Failure

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch from 1e46fc1 to ea07e5a Compare August 28, 2024 16:04

hlecuyer temporarily deployed to staging August 28, 2024 16:07 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 28, 2024 16:07 — with GitHub Actions Failure

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch from ea07e5a to db7271e Compare August 28, 2024 16:11

hlecuyer temporarily deployed to staging August 28, 2024 16:15 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 28, 2024 16:15 — with GitHub Actions Failure

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch 2 times, most recently from a9d3559 to 467a3f7 Compare August 29, 2024 08:26

hlecuyer temporarily deployed to staging August 29, 2024 08:30 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 29, 2024 08:30 — with GitHub Actions Failure

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch 3 times, most recently from 87f9466 to 67ec49e Compare August 29, 2024 08:47

hlecuyer temporarily deployed to staging August 29, 2024 08:51 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 29, 2024 08:51 — with GitHub Actions Failure

vperron reviewed Aug 29, 2024

View reviewed changes

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch from 31059b9 to f7cbe37 Compare August 29, 2024 09:35

vperron reviewed Aug 29, 2024

View reviewed changes

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch 2 times, most recently from 111ac63 to d5ae3bc Compare August 29, 2024 09:43

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch from d5ae3bc to bbef4b3 Compare August 29, 2024 09:45

hlecuyer temporarily deployed to staging August 29, 2024 09:48 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 29, 2024 09:48 — with GitHub Actions Failure

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch from bbef4b3 to 9ef3a7e Compare August 29, 2024 09:49

hlecuyer temporarily deployed to staging August 29, 2024 09:53 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 29, 2024 09:53 — with GitHub Actions Failure

vperron approved these changes Aug 29, 2024

View reviewed changes

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch 2 times, most recently from 0ca81da to c40c683 Compare August 29, 2024 12:17

hlecuyer temporarily deployed to staging August 29, 2024 12:22 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 29, 2024 12:22 — with GitHub Actions Failure

feat(alert): add wrapper on cron command to send alert on mattermost …

1748f10

…in case of failure

hlecuyer force-pushed the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch from c40c683 to 1748f10 Compare August 29, 2024 12:32

hlecuyer temporarily deployed to staging August 29, 2024 12:35 — with GitHub Actions Inactive

hlecuyer had a problem deploying to prod August 29, 2024 12:35 — with GitHub Actions Failure

hlecuyer merged commit f7c2c2d into main Aug 29, 2024
8 of 9 checks passed

hlecuyer deleted the hlecuyer/feat/alerts/add-wrapper-to-send-alert-in-mattermost branch August 29, 2024 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat(alerts): add alert on mattermost in case of cron failure #280

Feat(alerts): add alert on mattermost in case of cron failure #280

hlecuyer commented Aug 28, 2024

vperron left a comment

vperron Aug 29, 2024

hlecuyer Aug 29, 2024

vperron Aug 29, 2024

hlecuyer Aug 29, 2024

vperron Aug 29, 2024

hlecuyer Aug 29, 2024

hlecuyer Aug 29, 2024

vperron Aug 29, 2024

vperron Aug 29, 2024

vperron Aug 29, 2024

vperron Aug 29, 2024

hlecuyer Aug 29, 2024

vperron Aug 29, 2024

vperron Aug 29, 2024

hlecuyer Aug 29, 2024

vperron Aug 29, 2024

vperron Aug 29, 2024

vperron Aug 29, 2024

vperron Aug 29, 2024

vperron Aug 29, 2024

vperron Aug 29, 2024

vperron left a comment


		wait $command_pid

		exit_code=$? # Capture the exit code of the previous command


		exit_code=$? # Capture the exit code of the previous command

		# Check if the exit code indicates an error (not equal to 0)

		RUN chmod +x docker-entrypoint.sh
		RUN chmod +x run_management_command.sh

		@@ -0,0 +1,21 @@
		#!/bin/sh
		set -u

Feat(alerts): add alert on mattermost in case of cron failure #280

Feat(alerts): add alert on mattermost in case of cron failure #280

Conversation

hlecuyer commented Aug 28, 2024

vperron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vperron left a comment

Choose a reason for hiding this comment