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

Concentrateur IRVE dynamique national #3839

Merged
merged 151 commits into from
Jun 6, 2024
Merged

Concentrateur IRVE dynamique national #3839

merged 151 commits into from
Jun 6, 2024

Conversation

thbar
Copy link
Contributor

@thbar thbar commented Mar 25, 2024

Cette PR implémente un concentrateur IRVE dynamique national dans le proxy du PAN.

L'idée est d'être en mesure de fournir, pour les réutilisateurs, une url unique nationale pour disposer des informations de disponibilité des points de charge (en état / hors-service, utilisé ou pas etc) conformément au schéma IRVE dynamique (https://schema.data.gouv.fr/etalab/schema-irve-dynamique/).

L'url sera référencée comme une ressource du PAN.

Cela complète la consolidation IRVE statique déjà en place (https://transport.data.gouv.fr/datasets/fichier-consolide-des-bornes-de-recharge-pour-vehicules-electriques), et permettra de donner aux réutilisateurs une vision complète (emplacements, caractéristiques, et disponibilité des points de charge).

Voir:

Principe général:

  • un nouveau type d' "item" du proxy est introduit dans la configuration, appelé "aggregate".
  • cet "aggregate" dispose de sous-flux, identifiés par une chaîne de caractère, avec leur propre TTL et url cible.
  • le traitement de cet item "aggregate" donne lieu à N sous-requêtes concurrentes, qui sont consolidées dans l'ordre de la configuration de façon déterministe.
  • les champs doivent être (comme le dit la spécification Frictionless) exactement présents et exactement dans l'ordre (je m'appuie dessus pour simplifier le code et le rendre plus efficient, sachant que cette implémentation se traduira par des requêtes potentiellement nombreuses à terme)
  • le schéma suivi est https://schema.data.gouv.fr/etalab/schema-irve-dynamique/ ; une validation temps réel et reformattage à la volée sera possible vu l'architecture mise en place, toutefois on va d'abord aller recruter des flux pour avoir suffisamment de cas d'usages
  • chaque "sous flux" est mis en cache (si on a un retour HTTP formé correctement) dans Cachex
  • le flux global n'est lui pas mis en cache actuellement volontairement, pour le moment en tout cas

Améliorations à prévoir pour le futur

Le code lié à la gestion de Cachex était déjà un peu compliqué, et son usage augmente avec cette PR. Le fait qu'on n'utilise pas une "behaviour" et un isolement fait qu'on utilise de fait réellement Cachex dans chaque test, ce qui introduit des complexités (état partagé).

Ce point sera retravaillé dans une prochaine passe.

On voit également des aspects "GBFS" dans le code du proxy, qu'il faudra déplacer probablement dans shared.

Certains éléments gagneraient à être renommés, mais cela aurait rendu la PR trop complexe, j'ai préféré repousser à plus tard.

Le logging (Logger.info d'ailleurs souvent comme relevé par @ptitfred) pourra être amélioré, et avec lui peut-être un traçage des codes HTTP avec des time-series à un moment.

Comment tester sur prochainement

# flux complet
curl https://proxy.prochainement.transport.data.gouv.fr/resource/consolidation-nationale-irve-dynamique

# avec limite par source pour y voir plus clair
curl "https://proxy.prochainement.transport.data.gouv.fr/resource/consolidation-nationale-irve-dynamique?limit_per_source=1"

# avec l'identifiant de chaque source (origine)
curl "https://proxy.prochainement.transport.data.gouv.fr/resource/consolidation-nationale-irve-dynamique?include_origin=1&limit_per_source=1"

Test en local

Prendre la configuration sur:

Puis:

curl "http://proxy.localhost:5000/resource/consolidation-nationale-irve-dynamique?limit_per_source=1"

@thbar thbar self-assigned this Mar 25, 2024
Also link to #3975 which can lead the maintainer to a problematic database situation.
thbar added 6 commits June 5, 2024 19:18
After verification, `@proxy_requests` is only referenced inside the same file, and only for unsorted guard checks.
The same character is used, but 97800f3 incorrectly conflates telemetry events (metrics) with cache keys.
Copy link
Contributor Author

@thbar thbar left a comment

Choose a reason for hiding this comment

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

Déjà re question de Frédéric sur le script scripts/irve/dynamic-irve.exs:

Je ne suis pas sûr de comprendre l'objet des changements dans le script

Je cherchais à voir la validité des données "remote” sur les fichiers utilisés pour IRVE dynamique (dont j’avais besoin), à débugger pour chercher à comprendre pourquoi data.gouv ne rapportait pas le fichier comme ayant subi la validation, et afficher des stats. Plutôt du script de mise au point de la config utilisée ici, donc.

Par ailleurs - merci pour les nombreux retours à tous les deux, j’ai pu réaliser les améliorations et corrections suivantes (voir diff spécifique c’est plus simple à suivre):

  • ajout de typage sur les paramètres pour que la lecture soit plus aisée
  • ajout de tests sur le parsing de la configuration YAML dans le cas aggregate
  • refactoring pour éviter le passage de fonction get_function en paramètre (trop compliqué et pas clair en maintenance / lecture), au profit d’options du style max_redirects: 2 plus claires
  • ce qui a impliqué le déplacement de la gestion des redirects (nouvellement nécessaire pour le support des urls stables de data gouv, dont on se sert pour les IRVE) d’une couche haute du code vers le wrapper Finch (refactoring)
  • ajout de tests pour le wrapper Finch, qui devenaient nécessaires (les chemins de code 302 réimplémentés dans le wrapper étaient non testés en particulier, et error-prone avec de la récursivité)
  • ajout de Bypass (mini-serveur web de test) pour tester correctement le wrapper Finch
  • correctif d’un bug qui aurait mal compté les requêtes “internal” sur les sources agrégées, avec une solidification des tests associés (🙏 @AntoineAugusti pour la question portant sur le test, qui m'a mis sur la voie)
  • DRY des déclarations de séparateurs de clés : pour le cache
  • clarification du désenregistrement des handlers de télémétrie en début de tests, que je ne comprenais pas (passage de Enum.at(1) à Enum.uniq, qui fait la même chose mais qui fait qu’on ne se pose plus la question en lecture de code).
  • une fois que c’était fait, j’ai pu DRYer @proxy_requestsqui était en double (quoiqu’à l’envers) d’un autre attribut suite à refactoring, sans risque à présent.

Voilà je vais déployer tout ça sur prochainement pour vérifier que rien n'a cassé.

])

headers = @schema_fields
headers = if options[:include_origin], do: headers ++ ["origin"], else: headers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je compte proposer une évolution concertée du schéma IRVE (voir etalab/schema-irve#60) pour ajouter des informations de traçabilité. C'est utile autant sur le statique sur le dynamique. On aura probablement une trace de la forme datagouvfr:$$resource_id$$, et idéalement je chercherais bien à terme à automatiser ou simplifier une partie du travail de remontée "soucis qualité" grâce à ça.

Vu que ce n'est pas encore fait, et que je vise à avoir un jeu "valide par défaut" autant que possible, je le garde en optionnel à ce stade.


rows_stream =
item.feeds
|> Task.async_stream(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avec ce code si le flux général n'est pas requêté pendant plus longtemps que le TTL de l'aggregate la première requête va devoir attendre que l'on requête les n flux pour avoir une réponse.

Oui c'est bien le cas.

Prévois-tu de maintenir un état permanent des différents flux plus tard, même sans trafic ?

Non, en l'état, sans trafic, je priorise d'éviter de créer de la charge ops chez nous et chez les serveurs distants (ça pourra changer si il y a un intérêt fonctionnel).

apps/unlock/lib/aggregate_processor.ex Outdated Show resolved Hide resolved
Process a sub-item (sub-feed of the aggregate item), "safely" returning an empty list
should any error occur.
"""
def process_sub_item(item, %{identifier: origin} = sub_item, options) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Très bonne idée, idem ; j'ai ajouté dans 3d6256a, merci !

apps/unlock/lib/aggregate_processor.ex Show resolved Hide resolved
})
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

La partie ci-dessus a été déplacée et partiellement DRYée dans telemetry.ex

@@ -170,16 +153,8 @@ defmodule Unlock.Controller do
Logger.info("Processing proxy request for identifier #{item.identifier}")

try do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Le code ci-dessous a été refactoré avant réutilisation dans la partie agrégée.


verify!(Unlock.HTTP.Client.Mock)

# more calls should not result in any real query
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Effectivement ! J'ai traité dans 0f197ef, et tout ça a permis de détecter et traiter un bug où les métriques "internal" étaient comptés en trop (hors fonction de cache), voir d696c67. Merci !!

apps/unlock/lib/telemetry.ex Outdated Show resolved Hide resolved
sub_item
# Default to generic-http (typically used for IRVE)
|> Map.put_new("type", "generic-http")
# Use a default 10-second TTL for sub-feeeds, unless specified in the config
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ce point a été corrigé, merci !

@fchabouis
Copy link
Contributor

Bravo pour cette belle PR !

@thbar thbar requested review from AntoineAugusti, ptitfred and a team June 6, 2024 07:05
@thbar
Copy link
Contributor Author

thbar commented Jun 6, 2024

Bravo pour cette belle PR !

Haha merci @fchabouis 😄

Copy link
Member

@AntoineAugusti AntoineAugusti left a comment

Choose a reason for hiding this comment

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

LGTM, bravo pour cette très bonne première version 👏 🔌 🚗

Les améliorations possibles me semblent cohérentes et non critiques pour le moment, il y a besoin d'avoir quelque chose qu'on apprenne et que les réutilisateurs commencent à utiliser.

Copy link
Contributor

@ptitfred ptitfred left a comment

Choose a reason for hiding this comment

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

Dans une vie passée j'aurais dit : c'est tout de même une bien grosse PR et je ne sais pas si on loupe pas des trucs 😬

@thbar
Copy link
Contributor Author

thbar commented Jun 6, 2024

Merci à tous @AntoineAugusti @ptitfred et @fchabouis 👏

LGTM, bravo pour cette très bonne première version 👏 🔌 🚗
Les améliorations possibles me semblent cohérentes et non critiques pour le moment, il y a besoin d'avoir quelque chose qu'on apprenne et que les réutilisateurs commencent à utiliser.

Tout à fait l'idée, on va aller chercher les bons producteurs avec @stephane-pignal et soft-launch tout ça.

Dans une vie passée j'aurais dit : c'est tout de même une bien grosse PR et je ne sais pas si on loupe pas des trucs 😬

C'est gros par rapport à l'habitude, complètement d'accord. Mes prochaines PR seront plus réduites.

@thbar thbar added this pull request to the merge queue Jun 6, 2024
Merged via the queue into master with commit 93e3672 Jun 6, 2024
5 checks passed
@thbar thbar deleted the irve-concentrator-impl branch June 6, 2024 11:30
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.

4 participants