-
Notifications
You must be signed in to change notification settings - Fork 160
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
Introduction de matomo coté serveur #6063
Conversation
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.
C'est cool ! Quelques remarques sur la façon de collecter les données et autres menus détails.
Concernant l'implémentation dans son ensemble, il serait vraiment mieux qu'elle soit asynchrone, afin de ne pas ralentir le chargement des pages. Pour ce faire, plusieurs possibilités :
- créer un thread par requête pour envoyer le ping à Matomo ;
- créer un thread par worker contenant une file d'attente pour envoyer les pings à Matomo ;
- utiliser un paquet Django existant pour 1 ou 2.
Supposant que Django n'exécute qu'une fois le fichier enablematomomiddleware.py
de l'intergiciel, il suffirait de créer un thread là, contenant une file d'attente et une boucle infinie de traitement ou que sais-je ; et de lui passer les requêtes à transmettre (ou juste les infos nécessaire pour éviter de faire vivre trop longtemps l'objet Request
de Django).
Sur Discord, sgble précisait qu'il fallait être prudent :
Peut-être que Gunicorn attend de ses workers qu'ils n'aient pas de long-lived process ou thread. Cas 1 : il les désactive (uwsgi le fait sur option, par exemple), Cas 2 : il laisse faire mais il faut s'assurer qu'ils ne respawn pas les workers de façon à interférer. Par exemple, je sais que là encore uwsgi peut le faire (pour éviter qu'un worker dégénère avec le temps) et je me dis que Gunicorn aussi, peut-être
Quant à lui, artragis indique qu'il a une idée autre ; je le laisse préciser s'il le désire.
exclude_paths = ["/contenus", "/mp"] | ||
# only on get | ||
if request.method == "GET": | ||
for p in exclude_paths: | ||
if request.path.startswith(p): | ||
return response | ||
self.matomo_track(request) |
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.
Tu exlues totalement /contenus
et /mp
, j'imagine pour des raisons de confidentialité. Sur le principe, je suis d'accord, mais ne serais-ce pas plus pertinent de transmettre une autre URL avec uniquement le début du chemin, ou une URL avec les identifiants retirés, dans ces cas là ?
Ça permettrait de savoir à quel point les MPs et l'édition des contenus est utilisée, sans pour autant savoir quoi (ce qui est une bonne chose). Par exemple, au lieu de transmettre /contenus/stats/3819/zeste-de-savoir-30-elpis-lavant-gout-dune-nouvelle-direction/
, on transmettrait /contenus/stats/
.
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.
En fait, je suis parti du principe que "on ne sait jamais" je préfère exclure plutot qu'anonymiser. Au cas ou pour une raison inconnue l'anonymisation ne fonctionne plus correctement. Mais bon, j'avoue que je n'ai pas d'avis tranché.
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.
Ne peut-on point appliquer ce même raisonnement à ton filtre, en soit ?
Avec un code d'une complexité similaire (donc le même niveau de risque), on peut obtenir ce que je propose : il suffit de transmettre l'URL de la liste au lieu de bloquer. Et pour obtenir plus de précision tout en ayant un filet, on peut avoir une liste d'exclusion de ce genre, qui serait traitée dans l'ordre en s'arrêtant au premier qui colle :
exclude_paths = [
# Tout ce qu'on veut voir séparément (pas forcément que ça, c'est pour l'exemple)
"/contenus/stats",
"/contenus/historique",
"/contenus/importer",
"/contenus/tags",
# Tous le reste qui sera groupé dans une seule URL (le filet)
"/contenus",
# On n'a pas ce souci pour les MPs vu que le détail n'importe que peu
"/mp"
]
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.
Du coup, non ?
Merci @artragis pour la mise en background. C'est mergé sur la branche. |
Après quelques atermoiments, j'ai proposé une "fake PR" pour voir si on peut pas réparer la CI. J'ai été obligé de désactiver les stats dans cette config, désolé. Par contre, en contrepartie, je suis repassé à du thread plutôt que du multiprocess. C'est plus léger mais ça reste asynchrone. La seule chose c'est que quand un worker est occupé à envoyer des stats, bah il peut pas prendre des requêtes. Et vice versa. Les stats sont queuées en attendant d'avoir un peu d'espace CPU de libre quand on a une montée en charge des requêtes. |
Si l'envoi est différé, alors il faudrait stocker l'heure exacte de la requête initiale et la passer à Matomo via les paramètres |
J'ai pris en compte le code d'@artragis sur le traitement asynchrone de l'envoi et la remarque de @AmauryCarrade sur l'heure exacte. J'en ai profité pour migrer la page des stats des contenus vers matomo. On perd l'information sur le nombre de session mais c'est tout principalement. Est-ce que la mise à jour de la PR pourraient être déployée sur la beta ? |
Je me suis permis de mettre à jour ta branche par rapport à |
J'ai pris en compte les remarques d'artragis ici. un deploiement en beta (@Situphen , @philippemilink ) ? |
La bêta est à jour. |
zds/middlewares/matomomiddleware.py
Outdated
def _background_process(queue: Queue): | ||
data = queue.get(block=True) | ||
while data: | ||
params = { | ||
"idsite": matomo_site_id, | ||
"action_name": data["r_path"], | ||
"rec": 1, | ||
"apiv": matomo_api_version, | ||
"lang": data["client_accept_language"], | ||
"ua": data["client_user_agent"], | ||
"urlref": data["client_referer"], | ||
"url": data["client_url"], | ||
"h": data["datetime"].hour, | ||
"m": data["datetime"].minute, | ||
"s": data["datetime"].second, | ||
} | ||
requests.get( | ||
matomo_api_url, | ||
params=params, | ||
) | ||
logger.info(f'Matomo tracked this link : {data["client_url"]}') | ||
|
||
data = queue.get(block=True) |
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 demande surtout à @artragis qui a implémenté ça : si j'ai bien compris, la tâche attend qu'il y ait une charge CPU pas trop importante pour traiter les requêtes. Donc il y aura a priori des requêtes traitées par lot. Ne serait-il pas pertinent d'utiliser l'API de traitement par lot de Matomo ?
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.
ça peut être pertinent mais ça augmentera la complexité du code. J'étais juste inconscient que cette API existait.
Remarques des uns et des autres prises en charges (notamment l'envoi de l'IP) et tests corrigé (j'ai mocké complètement l'API dans les tests). Je pense qu'on arrive au bout de cette PR. |
Si tu as le temps de faire un petit rebase ce serait bien ! Je vais mettre à jour la PR sur la bêta :) |
Rebase done. |
Bêta à jour ! |
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.
COmme on a requis ma review, j'ai refait une passe et je n'ai qu'une seule et unique remarque.
Pour la corriger on a deux méthodes : soit on invalide le token côté matomo et tout est gagné, soit on retire ça de notre git
Il faut faire les deux, non ? Le token a été exposé publiquement, il faut donc le changer ! Et oui, dans tous les cas le retirer de Git. |
Je viens d'aller voir les statistiques d'un de mes billets sur la bêta, et les pages contenant les statistiques sont très longues à charger (20,9 secondes), comme si récupérer les infos de Matomo était très long. Sur la prod, le chargement de ces pages est bien plus rapide (1,8 secondes). Je suis le seul à observer ce phénomène ? |
Les logs de la bêta m'indiquent un soucis :
C'est dû à l'utilisation de la fonction Lines 36 to 47 in 9f6e100
|
@philippemilink je n'ai pas observé ça chez moi. Tu reproduis encore ce problème ? |
@Situphen c'est probablement la consultation via munin qui ne passe pas d'entête http et donc django va chercher le nom du host lui même. Je ne vois pas comment on peut faire autrement. |
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.
On m'a demandé une revue, donc acte.
J'ai adressé plusieurs commentaires qui ont été ignorés et qui sont toujours d'actualité, après vérification :
- des statistiques groupées sur certaines URLs plutôt que rien ;
- certains bots semblent passer sur Matomo (vérifié à l'instant sur l'instance de prod de Matomo).
Concernant le second point, ça semble plus subtil que ça car cette fois, on a l'UA — Wget. Donc si quelqu'un aspire l'ensemble du site avec wget (ce qui semble se produire régulièrement ?!), on voit tout dans Matomo. Mais les autres bots plus classiques (Google, etc.) semblent correctement filtrés par Matomo.
personnellement cette solution ne me convainc pas d'autant que je ne vois pas à quoi serviraient ces stats sinon à augmenter le poids de la bdd
ça ressemble à munin : 7 requêtes toutes les 5 minutes pendant 12h ça fait 1008, si on compte que la visite est sensée avoir duré "11 h 45", tu enlèves 2 cycles et tu tombes pile sur... 994 |
Dans mon esprit, à savoir à quel point les outils de rédaction/MP étaient utilisés, mais ce n'est pas forcément indispensable, certes.
Ah oui bien vu, d'autant que les pages visitées sont bien moins diversifiées que ce qu'il me semblait — c'est totalement ça. Du coup, il faudrait ou filtrer Wget côté Matomo, si possible, ou le filtrer avant d'envoyer la requête à Matomo. |
@AmauryCarrade je n'avais pas répondu sur ton commentaire, mais je partage l'avis de @artragis ici :
|
J'aurais dit en sortie. A priori matomo est capable de se rendre compte que c'est lui et pas un autre alors je me dis qu'on peut carrément blacklister sur matomo l'id de wget |
OK pour les deux points. J'approuve le blocage côté Matomo si c'est possible, ça devrait rester son rôle que de filtrer. |
Cette PR permet de faire rentrer matomo (anciennement piwik) comme outil de tracking à la place de google analytics.
Pour la petite histoire, j'ai installé matomo sur un serveur tout frais ( 2CPU , 2GB RAM, 20Go) léger pour pouvoir évaluer s'il peut tenir la route. Si jamais on veut l'utiliser pour zeste de savoir, il suffira de créer un sous domaine de zestedesavoir.com et de le faire pointer sur l'IP du serveur.
NB : que l'on choisisse d'heberger matomo sur une infra zds ou via une offre allechante, la PR reste d'actualité puisque l'url matomo est un paramètre au déploiement du site
Ce que fait la PR
Ce que ne fait pas la PR
Ticket concerné : #6030
Contrôle qualité
Idéalement il faudra déployer cette PR sur la beta pour pouvoir tester convenablement si ça n'impacte pas le rendu du site.