Import des stats mensuelles data.gouv.fr pour les JDDs #3663
Conversation
| import Ecto.Changeset | ||
|
|
||
| typed_schema "dataset_monthly_metrics" do | ||
| belongs_to(:dataset, DB.Dataset, foreign_key: :dataset_datagouv_id, references: :datagouv_id, type: :string) |
There was a problem hiding this comment.
En mode synchro "miroir" je ne suis pas sûr que j'aurais mis la foreign key, justement pour:
- être sûr de toujours pouvoir ingérer la donnée (et analyser le souci après)
- ne pas avoir de risque de suppression automatique lorsqu'un dataset est supprimé (même temporairement)
Je suis plutôt à recommander de la supprimer pour le coup.
There was a problem hiding this comment.
Merci, je retire et je mets des commentaires.
|
|
||
| typed_schema "dataset_monthly_metrics" do | ||
| belongs_to(:dataset, DB.Dataset, foreign_key: :dataset_datagouv_id, references: :datagouv_id, type: :string) | ||
| field(:year_month, :string) |
There was a problem hiding this comment.
Parfois je colle une contrainte sur ce type de colonnes, mais dans l'esprit de ce que j'ai mis au dessus et vu que c'est plutôt un "gros staging" brut, ça me paraît bien de laisser comme ça.
| |> cast(attrs, [:dataset_datagouv_id, :year_month, :metric_name, :count]) | ||
| |> validate_required([:dataset_datagouv_id, :year_month, :metric_name, :count]) | ||
| |> foreign_key_constraint(:dataset_datagouv_id) | ||
| |> validate_format(:year_month, ~r/^2\d{3}-(0?[1-9]|1[012])$/) |
There was a problem hiding this comment.
Et je vois que tu as mis ça après 😄
There was a problem hiding this comment.
Attention à ^ et $, préférer \A et \z. Là ça n'est pas gênant mais après on retrouve des soucis de sécurité ou d'ingestion mal formatée (non détection de lignes multiplies).
Cf:
- https://stackoverflow.com/a/18344295/20302
- https://guides.rubyonrails.org/security.html#regular-expressions (mais on peut retrouver ça dans d'autres langages)
There was a problem hiding this comment.
Intéressant ! J'oublie ça régulièrement, pas hyper dérangeant comme tu dis mais je change pour éviter que ça perdure dans notre codebase.
| end | ||
|
|
||
| def dataset_datagouv_ids do | ||
| DB.Dataset.base_query() |> select([dataset: d], d.datagouv_id) |> DB.Repo.all() |
There was a problem hiding this comment.
Tiens Credo ne force pas à mettre ça sur N lignes ? Intéressant j'aurais cru !
There was a problem hiding this comment.
C'est pas trop long, seulement 84 caractères !
| def import_metrics(dataset_datagouv_id) do | ||
| url = api_url(dataset_datagouv_id) | ||
|
|
||
| case http_client().get(url, []) do |
| {:ok, %Req.Response{status: 200, body: body}} -> | ||
| body | ||
| |> Map.fetch!("data") | ||
| |> Enum.each(fn data -> insert_or_update(data, dataset_datagouv_id) end) |
There was a problem hiding this comment.
On gagnerait à faire un "bulk upsert" avec:
Ca a ses limites sur la taille totale mais là tu ferais juste "un gros call" et ça serait bien plus rapide / léger sur le pool je pense aussi.
There was a problem hiding this comment.
insert_all ne passe pas avec une liste de Ecto.Changeset dans entries_or_query 😢
There was a problem hiding this comment.
Oui tout à fait, il faut se passer de changeset normalement, car l'idée de ce type d'import c'est d'être efficient aussi en mémoire donc de ne pas créer de structure supplémentaire ni de validation.
Si on fait ça il vaut mieux s'appuyer sur des contraintes en base si besoin fort de validation.
| defp insert_or_update( | ||
| %{ | ||
| "metric_month" => metric_month, | ||
| "monthly_visit" => monthly_visit, | ||
| "monthly_download_resource" => monthly_download_resource | ||
| }, |
There was a problem hiding this comment.
L'API donne views + downloads dans un même record. On passe à 2 records (metric_name, count)
| other -> | ||
| Logger.error( | ||
| "metric-api.data.gouv.fr unexpected HTTP response for Dataset##{dataset_datagouv_id}: #{inspect(other)}" | ||
| ) | ||
| end |
There was a problem hiding this comment.
Je me demande s'il faut faire autre chose, genre enregistrer un event dans Sentry
There was a problem hiding this comment.
On pourrait logger un custom metric dans AppSignal à un moment. Mais là le logging + un mot clé filtré dans AppSignal + une alerte éventuelle ça devrait suffire si on veut "être prévenus" !
| dataset_datagouv_id: dataset_datagouv_id, | ||
| year_month: metric_month, | ||
| metric_name: metric_name, | ||
| count: count || 0 |
There was a problem hiding this comment.
J'ai vu un null au moins dans les retours de l'API. Je vais remonter ça.
|
J'avais oublié de soumettre mes commentaires avant la review 🙈 Bon, ça change pas trop. |
|
Merci @thbar ! J'ai pris en compte tes commentaires : pas de contrainte sur la clé étrangère Je ne suis pas passé à |
Pas de souci :-) |
Effectivement comme dit plus haut, mais on peut le faire en se passant de changeset et c'est assez puissant ! Ne t'embête pas forcément sur ce tour effectivement, c'est si on a d'autres cas similaires, voire un peu plus joufflus. |
|
@thbar J'avais regardé et il y a quand même pas mal de problèmes avec |
|
Ce n'est pas spécifique à Ecto pour le coup ; dès que tu fais du insert all / upsert all en volume, tu dois souvent gérer les timestamps un peu à la main (et ça peut être assez vital selon les cas, enfin ça dépend des cas d'usage). Ça a été ajouté assez "récemment" dans ActiveRecord par exemple, et c'est un point que j'aborde souvent avec les gens qui utilisent Kiba (https://github.com/thbar/kiba/wiki/SQL-Bulk-Insert-Upsert-Destination - d'ailleurs je ferais bien d'ajouter ça texto dans la doc). Toutes les colonnes un peu auto-gérées par l'ORM sont un petit peu des citoyens de seconde zone sur ces cas, mais le compromis est un usage mémoire amélioré et une efficience qui permet de gérer des batchs de façon plus rapide. Mais bref, l'idée c'est pas de se lancer du bike-shedding sur cette PR (elle est très bien), juste préciser que ça peut se faire avec Ecto aussi, si on a vraiment besoin à un moment (je l'ai déjà fait à plusieurs moments en tout cas), que c'est un peu moins "intégré" mais que si le bénéfice dépasse l'embêtement c'est puissant. |
Importe les statistiques mensuelles data.gouv.fr pour les JDDs : téléchargements et vues.
Cette PR traite :
dataset_monthly_metrics)Il n'affiche pas ces valeurs, ce travail sera mené avec @etalab/transport-bizdev plus tard. En effet les statistiques de téléchargements provenant de data.gouv.fr ne font sens que pour les données hébergées par data.gouv.fr (ce n'est pas valide pour les ressources qui sont hébergées hors data.gouv.fr).
J'ai exécuté le job d'import en local pour vérifier que l'import fonctionne bien. Le premier import, passant de 0 rows à 15 200 lignes a pris 45s.