-
Notifications
You must be signed in to change notification settings - Fork 31
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
Conversation
Also link to #3975 which can lead the maintainer to a problematic database situation.
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.
There was a problem hiding this 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 stylemax_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_requests
qui é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 |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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).
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 |
There was a problem hiding this comment.
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 !
}) | ||
end | ||
end | ||
|
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apps/unlock/lib/config.ex
Outdated
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 |
There was a problem hiding this comment.
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 !
Bravo pour cette belle PR ! |
Haha merci @fchabouis 😄 |
There was a problem hiding this 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.
There was a problem hiding this 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 😬
Merci à tous @AntoineAugusti @ptitfred et @fchabouis 👏
Tout à fait l'idée, on va aller chercher les bons producteurs avec @stephane-pignal et soft-launch tout ça.
C'est gros par rapport à l'habitude, complètement d'accord. Mes prochaines PR seront plus réduites. |
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:
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
Test en local
Prendre la configuration sur:
Puis: