DataChecker : adapte erreur vers Sentry

AntoineAugusti commented 2 months ago

Fixes #4178

Symptome

Un warning est visible lors de l'exécution des tests, provenant de Transport.DataChecker

[warning] Event dropped due to being a duplicate of a previously-captured event.

Pourquoi

Le warning est visible depuis peu, parce que est Sentry en test mode en test (commit https://github.com/etalab/transport-site/commit/344a4d0ddd3daa895de62719cb245ef063c1310c), avant c'était juste silencieux. Mais le warning est intéressant, il dit qu'avec la conf/le code actuel seul le premier event aurait été envoyé. Donc en cas d'erreur d'un appel sur data.gouv.fr on aurait eu 1 seul event comportant le dataset ID et pas les suivants. Le code d'envoi vers Sentry visé est là depuis de nombreux mois.

Que faire

Il semble qu'il est souhaitable d'envoyer plusieurs événements vers Sentry afin de disposer des différents datagouv_id des jeux de données. Il faut donc faire en sorte que l'événement ne soit pas considéré comme un doublon par Sentry.

Documentation de Sentry

Le code qui n'envoie pas un événement en cas de doublon dans le SDK Elixir Sentry est le suivant

https://github.com/getsentry/sentry-elixir/blob/b8e40d793a7d45f6d1a1965120ce3a586793ce69/lib/sentry/client.ex#L107-L119

Sentry.Dedupe.insert a le code suivant

https://github.com/getsentry/sentry-elixir/blob/b8e40d793a7d45f6d1a1965120ce3a586793ce69/lib/sentry/dedupe.ex#L18-L27

Le hash d'un événement est calculé de la façon suivante

https://github.com/getsentry/sentry-elixir/blob/b8e40d793a7d45f6d1a1965120ce3a586793ce69/lib/sentry/event.ex#L479-L489

Il se repose sur l'exception, le message, le level et la fingerprint. Mais pas extra, qui comporte le datagouv_id.

Fingerprint permet de grouper des événements.

All events have a fingerprint. Events with the same fingerprint are grouped together into an issue.

Décision

Avec ces éléments, je choisis de spécifier une fingerprint et de faire varier le message pour grouper les événements et ne pas être considéré comme un doublon.

Correction

Le warning n'est plus visible dans l'exécution des tests après le changement effectué.

ptitfred commented 2 months ago

Ca fixe en effet le warning en lançant le test du Transport.DataChecker.

J'observe également le warning pour le test apps/transport/test/transport_web/controllers/resource_controller_test.exs. Ca m'avait échappé en logguant l'issue. J'imagine que c'est en appelant le Datagouvfr.Client et notamment la fonction maybe_report_error. Penses-tu judicieux de le fixer de façon similaire ?

AntoineAugusti commented 2 months ago

@ptitfred Merci de ta vigilance ! L'autre erreur était causée par un doublon de Ecto.NoResultsError (chercher un modèle qui n'existe pas en DB, puis une page web 404) qui était envoyé 2 fois.

Cet événement est normalement ignoré par https://github.com/etalab/transport-site/blob/master/apps/shared/lib/sentry_exception_filter.ex mais la configuration Sentry en test n'avait pas ce filtre.

J'ai mis dans la config commune :sentry tout ce qui n'était pas spécifique à la prod pour que Sentry en test utilise ceci aussi.

etalab / transport-site