Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pipeline) : Lancer et snapshotter le modèle de monitoring #267

Merged
merged 5 commits into from
Aug 13, 2024

Conversation

vperron
Copy link
Contributor

@vperron vperron commented Aug 6, 2024

Tout est dans le titre :)

@vperron vperron self-assigned this Aug 6, 2024
@vperron vperron requested a review from vmttn as a code owner August 6, 2024 09:22
@vperron vperron changed the title feat(pipeline) : Lancer et snapshotter le modèle de monotoring feat(pipeline) : Lancer et snapshotter le modèle de monitoring Aug 6, 2024
@vperron
Copy link
Contributor Author

vperron commented Aug 6, 2024

@vmttn je me demande si:

  • je ne devrais pas merger celle-ci avant celle sur le geocodage Améliorer le géocodage de nos adresses #266
  • au passage y mettre un score supplémentaire sur le nombre de lignes de marts avec un score supérieur à 0.8 ? ou autre ?

Ca permettra de voir l'évolution, sachant que la première sera trompeuse: aujourd'hui on a beaucoup de scores au-delà de 0.8 pour des matchs foncièrement erronés (cf commentaire du second commit de cette PR)

pipeline/dags/import_data_inclusion_api.py Outdated Show resolved Hide resolved
pipeline/dbt/snapshots/quality/snps_quality__stats.sql Outdated Show resolved Hide resolved
pipeline/dbt/snapshots/quality/snps_quality__stats.sql Outdated Show resolved Hide resolved
pipeline/dags/import_data_inclusion_api.py Outdated Show resolved Hide resolved
@vmttn
Copy link
Contributor

vmttn commented Aug 9, 2024

je ne devrais pas merger celle-ci avant celle sur le geocodage
au passage y mettre un score supplémentaire sur le nombre de lignes de marts avec un score supérieur à 0.8 ? ou autre ?

Ca permettra de voir l'évolution, sachant que la première sera trompeuse: aujourd'hui on a beaucoup de scores au-delà de 0.8 pour des matchs foncièrement erronés (cf commentaire du second commit de cette PR)

spontanément je serais plutot d'avis de ne pas ajouter cette info. On peut merger et en reparler ensuite si ça te va ? Ca fait déja beaucoup

@vperron
Copy link
Contributor Author

vperron commented Aug 12, 2024

OK pour ton dernier commentaire, ça me simplifie la vie ^^

Comment on lines 106 to 108
# Don't snapshot though if the initial API import failed, which
# would result in `build_source_stats` to be skipped.
# In that case there is nothing to be snapshotted.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm pas certain d'avoir bien compris, mais si import_data_inclusion_api fail, alors build_source_stats et snapshot_source_stats seront dans l'état upstream_failed non ?

Copy link
Contributor

Choose a reason for hiding this comment

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

et finalement all_done suffirait ici ?

Copy link
Contributor

Choose a reason for hiding this comment

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

bref je te laisse voir si il faut changer qqchose

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non, je l'ai testé. Quand import_data_inclusion_api fail:

  • build_source_stats passe en SKIPPED
  • et donc on veut savoir si on lance le snapshot ou pas
    Mon idée est que dans ce cas précis, non; les stats n'auront donc pas été créées et lancer le snapshot est inutile ou dangereux, si la manière de le créer change par exemple.

Par contre si les tests des stats ont échoué (par exemple, une source entière est vide) on veut quand meme snapshotter.

Au final, ça donne "tout sauf si le précédent a skipped".

Mais on peut décider ALL_DONE et snapshotter en permanence.
Meme question pour les stats : est-ce qu'on les build indépendamment de l'import ? Mon avis est "plutot non" parce que ça peut donner une image fausse de la donnée, mais peut etre que du point de vue d'Antoine c'est mieux d'avoir toujours "quelque chose".

Copy link
Contributor

Choose a reason for hiding this comment

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

ok clair

Attention je pars loin :

Actuellement il n'y a pas de tests sur le model de qualité pour vérifier qu'une source entière n'est pas vide, correct ?

Je m'interroge sur la nature des data_tests qu'on souhaiterait définir. On souhaite que ce tableau nous permette de suivre l'état des sources et donc éventuellement de mettre en évidence des problèmes. Par exemple le fait d'avoir une source vide. Donc finalement le fait d'avoir une source vide est valide au regard du rôle de ce tableau.

Si on veut alerter sur un pb type source vide / valeur en deça d'un seuil, il faudrait "presque" un deuxième model qui dépend du premier, matérialisée comme une vue, sur lesquels on défini ces tests spécifiquement. (Je fais l'hypothese qu'on veut utiliser ce mecanisme test_dbt x callback airflow pour alerter, mais il reste l'option metabase qui donne une certaine indépendance à l'équipe produit)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aujourd'hui, j'ai défini des tests très basiques qui vérifient que on ne "perd" pas une source quelque part.

cf. ici : https://github.com/gip-inclusion/data-inclusion/blob/main/pipeline/dbt/models/intermediate/quality/_quality_models.yml#L51-L79

(d'ailleurs, ils vont trop loin car je ne voulais pas tester le count_contact et count_adresses, on a des sources qui n'en ont pas et c'est "normal".

Du coup ce que j'en comprends pour l'instant:

  • je retire tous les data tests actuels, on verra plus tard dans metabase ou dans un second model spécialisé
  • je mets les triggers en ALL_DONE pour le nuild et le snapshot de stats pour qu'on ait toujours des données, meme si parfois elles peuvent etre difficiles à interpréter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai mis un dernier commit qui reflète cela

@vperron
Copy link
Contributor Author

vperron commented Aug 12, 2024

cf. ici, apparemment on lance quand meme déjà les stats 🤷 or, rien ne sélectionne explicitement le modèle en question et il n'est pas une dépendance des marts. Ou j'ai mal vu un truc ?

To mimic the behavior found with all the models.
The table and its snapshots are ran after every API import.
It's "overkill" but very simple and will keep the data up-to-date if we
ever have more hourly changes in the future.

Example output:

data-inclusion=# SELECT id,date_day,source,stream,count_raw,count_stg,count_int,count_api,dbt_updated_at FROM snapshots.snps_quality__stats ORDER BY id, date_day;
                 id                  |  date_day  |        source         |         stream         | count_raw | count_stg | count_int | count_api | dbt_updated_at
-------------------------------------+------------+-----------------------+------------------------+-----------+-----------+-----------+-----------+----------------
 action_logement-services            | 2024-08-06 | action_logement       | services               | 26        | 23        | 2760      | 2760      | 2024-08-06
 action_logement-services            | 2024-08-07 | action_logement       | services               | 26        | 23        | 2760      | 2760      | 2024-08-07
 action_logement-structures          | 2024-08-06 | action_logement       | structures             | 123       | 120       | 120       | 120       | 2024-08-06
 action_logement-structures          | 2024-08-07 | action_logement       | structures             | 123       | 120       | 120       | 120       | 2024-08-07
 agefiph-services                    | 2024-08-06 | agefiph               | services               | 31        | 31        | 27        | 27        | 2024-08-06
 agefiph-services                    | 2024-08-07 | agefiph               | services               | 31        | 31        | 27        | 27        | 2024-08-07
 cd35-organisations                  | 2024-08-06 | cd35                  | organisations          | 3545      | 3545      | 3545      | 3540      | 2024-08-06
 cd35-organisations                  | 2024-08-07 | cd35                  | organisations          | 3545      | 3545      | 3545      | 3540      | 2024-08-07
 cd72-services                       | 2024-08-06 | cd72                  | services               | 474       | 463       | 463       | 0         | 2024-08-06
 cd72-services                       | 2024-08-07 | cd72                  | services               | 474       | 463       | 463       | 0         | 2024-08-07
 cd72-structures                     | 2024-08-06 | cd72                  | structures             | 217       | 217       | 217       | 457       | 2024-08-06
 cd72-structures                     | 2024-08-07 | cd72                  | structures             | 217       | 217       | 217       | 457       | 2024-08-07
 data_inclusion-services             | 2024-08-06 | data_inclusion        | services               | 47        | 44        | 44        | 44        | 2024-08-06
 data_inclusion-services             | 2024-08-07 | data_inclusion        | services               | 47        | 44        | 44        | 44        | 2024-08-07
 data_inclusion-structures           | 2024-08-06 | data_inclusion        | structures             | 22        | 19        | 19        | 19        | 2024-08-06
 data_inclusion-structures           | 2024-08-07 | data_inclusion        | structures             | 22        | 19        | 19        | 19        | 2024-08-07
 dora-services                       | 2024-08-06 | dora                  | services               | 17717     | 11707     | 11707     | 11034     | 2024-08-06
 dora-services                       | 2024-08-07 | dora                  | services               | 17717     | 11707     | 11707     | 11034     | 2024-08-07
 dora-structures                     | 2024-08-06 | dora                  | structures             | 8554      | 8554      | 8554      | 8538      | 2024-08-06
 dora-structures                     | 2024-08-07 | dora                  | structures             | 8554      | 8554      | 8554      | 8538      | 2024-08-07

No day-over-day change here as I'm using the same database between 2 runs.
We'll test and get notified about the data quality another time.
It's going to all change after Valentin's geocoding revamp anyway.

Still I don't get why they get run.
@vmttn vmttn merged commit 41ed26a into main Aug 13, 2024
8 of 9 checks passed
@vmttn vmttn deleted the vperron/snapshot-monitoring branch August 13, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants