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

Améliore le système de tracking #1009

Merged
merged 34 commits into from
May 9, 2023
Merged

Améliore le système de tracking #1009

merged 34 commits into from
May 9, 2023

Conversation

bjlaa
Copy link
Contributor

@bjlaa bjlaa commented Apr 26, 2023

Mon idée est de tout centraliser dans le TrackerContext, logique et useEffect qui suivent la progression du questionnaire.
Ajoute un système de sauvegarde des évènements envoyé, lié à l'id de simulation.
Corrige le bug de captage d'event sur Plausible.

TODO :

  • centraliser les évènements (chaines de caractères) dans un fichier
  • mettre en place un moyen de lister certains évènements du type trackEvent afin qu'ils puissent être appelés plusieurs fois au besoin
  • tester un implémentation qui ferait usage du store Redux

Edit :
J'ai remplacé TrackingContext par MatomoContext ainsi que créé /analytics/matomo-events.ts où sont centralisés tous nos évènements tracking.
J'ai retiré les évènements 50% de complétion et 90% de complétion pour ajouter un évènement qui est déclenché à chaque fois que l'utilisateur démarre une catégorie. Cela nous donnera des infos plus précises sur la ou les catégories qui peuvent potentiellement poser problème. On pourra affiner dans un second temps sur chaque question pour avoir des métrics très précises en ajoutant un système d'URL unique par question.

@bjlaa bjlaa self-assigned this Apr 26, 2023
@netlify
Copy link

netlify bot commented Apr 26, 2023

Deploy Preview for nosgestesclimat ready!

Name Link
🔨 Latest commit b981f92
🔍 Latest deploy log https://app.netlify.com/sites/nosgestesclimat/deploys/6453c7ce91c35000087af748
😎 Deploy Preview https://deploy-preview-1009--nosgestesclimat.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Apr 26, 2023

Report for the pull request #1009


🌐 Translation status

UI's texts

Language Nb. missing translations Status
en-us Ø ✔️

FAQ's questions

Language Nb. missing translations Status
en-us Ø ✔️

You will find more information about the translation in the dedicated file.

@laem
Copy link
Contributor

laem commented Apr 26, 2023

Si je comprends bien, les événements du funnel de conversion simulation ne sont plus vérifiés dans Redux, mais directement dans le localstorage dans le composant de tracker.

Pourquoi ne pas continuer avec redux, et ajouter la persistance de l'attribut de suivi des events ?

Notamment, j'ai l'impression que ça te fait manipuler cette API localstorage de plus bas niveau que le store redux.

https://github.com/datagir/nosgestesclimat-site/pull/1009/files#diff-844dff7b069f53c99f9d4cef0034b41221b8746b4d2a4f48c9fd738ad716b814R55

Est-ce que ce système de vérification d'event n'empêche pas certains events d'être légitimement envoyés plusieurs fois pour une même utilisateur-simulation ? Tu l'as bien vérifié ?

Finalement, ce que tu fais dans cette PR revient à implémenter nous-mêmes notre propre logiciel d'analytics.

@bjlaa bjlaa marked this pull request as ready for review April 26, 2023 17:34
@bjlaa
Copy link
Contributor Author

bjlaa commented Apr 26, 2023

Si je comprends bien, les événements du funnel de conversion simulation ne sont plus vérifiés dans Redux, mais directement dans le localstorage dans le composant de tracker.
Pourquoi ne pas continuer avec redux, et ajouter la persistance de l'attribut de suivi des events ?

Yes, je suis allé au plus simple. Je pense que ça venait de l'utilisation des useEffect notamment des dépendances qui leurs étaient passés, mais les étapes du funnel n'étaient pas correctement vérifiées. Sachant que j'avais en tête de repenser le système de tracking en l'extrayant des composants je suis parti sur cette idée : un simple hook useState dans un context synchronisé avec la valeur du localstorage pour la persistence. C'est encore en WIP à vrai dire, j'ai passé la PR pour voir si les tests e2e se lançaient bien !

Je suis pas contre l'idée d'utiliser Redux ici je trouve ça un peu plus lourd, et je voulais pouvoir mettre à jour l'objet stocké dans le localStorage depuis le context TrackingContext.

Est-ce que ce système de vérification d'event n'empêche pas certains events d'être légitimement envoyés plusieurs fois pour une même utilisateur-simulation ? Tu l'as bien vérifié ?

Le système que j'ai ajouté est pensé pour filtrer les évènements déjà envoyés, effectivement pour le moment tous les évènements sont filtrés. Je pensais ajouter un array avec les events à envoyer plusieurs fois.

Finalement, ce que tu fais dans cette PR revient à implémenter nous-mêmes notre propre logiciel d'analytics.

Oulà c'est pas du tout mon objectif 😄 on pourra en discuter demain de vive voix mais mes objectifs ici sont :

  • centraliser la logique de tracking dans TrackingContext
  • éviter les doublons pour une même simulation (localstorage et compagnie)
  • centraliser la logique de funnel à un endroit useFunnel

@bjlaa bjlaa marked this pull request as draft April 26, 2023 19:06
@bjlaa
Copy link
Contributor Author

bjlaa commented Apr 26, 2023

J'ai aussi ajouté https://www.npmjs.com/package/eslint-plugin-react-hooks qui permet d'éviter de faire pas mal d'erreurs en utilisant les hooks, je pense que le fait que les events du funnel NGC provenaient d'erreurs dans les dépendances passées aux useEffect dans Conversation notamment

@bjlaa
Copy link
Contributor Author

bjlaa commented Apr 26, 2023

@laem au final je suis bien tenté de stocker les key d'event dans le store redux j'essaie ça demain !

@Clemog
Copy link
Contributor

Clemog commented Apr 27, 2023

J'ai retiré les évènements 50% de complétion et 90% de complétion pour ajouter un évènement qui est déclenché à chaque fois que l'utilisateur démarre une catégorie. Cela nous donnera des infos plus précises sur la ou les catégories qui peuvent potentiellement poser problème. On pourra affiner dans un second temps sur chaque question pour avoir des métrics très précises en ajoutant un système d'URL unique par question.

Je trouve dommage de perdre cet indicateur "numérique" qui permet de donner une idée plus précise de l'avancement plutôt que la catégorie passée ou non .. Si on veut tester la longueur du test, le `progressent suffisant

En revanche, on pourrait changer la manière d'envoyer les events de progress en envoyant un event dès qu'on passe un palier (tous les 5%) qu'en penses-tu ?

Je me demande aussi si les erreurs de complétion ne viennent pas de notre calcul du progress (je n'ai pas vérifié, c'est une hypothèse): si on est à 5/10(50%), on envoie l'event 50%, la 6ème question déclenche d'autres questions, le nombre de questions total passe de 10 à 15 --> on retombe 5/15 (33%), ce qui pourrait potentiellement renvoyer l'event 50% par la suite (mais normalement c'était pas le cas car "event fired')

@bjlaa
Copy link
Contributor Author

bjlaa commented Apr 27, 2023

@Clemog

En revanche, on pourrait changer la manière d'envoyer les events de progress en envoyant un event dès qu'on passe un palier (tous les 5%) qu'en penses-tu ?

J'aime bien l'idée mais comme tu le dis c'est un peu dérangeant qu'on ne puisse pas savoir de manière fiable le pourcentage réel de complétion au cours de la simulation du coup je me demande si c'est une donnée vraiment pertinente. 🤔 Je ne sais pas à quel point le nombre de questions peut varier. Si le delta est minime ça pourrait être intéressant.
Après on avait discuté de la mise en place d'une URL pour chaque question et je comptais me reposer là-dessus, sur les pageview donc, pour avoir une vision fine question par question de la progression, tu crois pas que ça irait dans le même sens que ce tu proposes ?

Dans tous les cas je crois vraiment qu'il faut qu'on crée un nouveau projet matomo pour avoir dès le départ des données propres (ce qui ne nous empêchera pas d'agréger les données de l'ancien avec le nouveau, typiquement le nombre de visites). On pourrait en avoir un sur l'instance betagouv qui est plus musclée à ce qu'il parait - proposition de Chaïb.

En fait je crois qu'il faut que je formalise ma vision pour les stats dans un doc je fais ça ce matin et je vous partage le doc pour qu'on en discute et qu'on se mette d'accord. :)

const [displayExplanations, setDisplayExplanations] = useState(false)

const handleChecked = (e: React.ChangeEvent<HTMLInputElement>) => {
if (e.target.checked) {
setDisplayExplanations(false)
}
onChange?.(e)
tracker.debouncedPush([
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supprimé car ça me semble très spécifique, à moins d'avoir un objectif de mesure clair ça ne sert à rien d'aller autant dans le détail à mon sens 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

On avait un tracker sur tous les clics de checkbox ?

const { eventsSent } = simulation || {}

// Call before sending an event
const checkIfEventAlreadySent = useCallback(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laem @Clemog ici j'empêche l'envoi de plusieurs mêmes évènements. J'ai bien compris (depuis notre point avec Maël) qu'on était dans une logique de session et qu'on laisse matomo faire le tri entre visiteurs uniques et tous les visiteurs mais j'ai quand même un problème avec le fait de balancer autant de requêtes pour un chiffre qui ne me parait pas utile. En fait, je vois mal l'utilité de savoir combien de visiteurs totaux ont déclenché l'évènement "50% de progression" en comparaison avec l'utilité de savoir le nombre de visiteurs uniques pour le même évènement. Je relie ça avec la question de la frugalité : si on peut éviter des requêtes pourquoi ne pas le faire ?

Copy link
Contributor

@Clemog Clemog May 4, 2023

Choose a reason for hiding this comment

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

Je ne comprends pas pourquoi tu veux absolument l'enlever car il a été ajouté pour justement comprendre les résultats élevés des simulations terminées (envoyé, il me semble, via une autre méthode qui n'utilise pas le progress mais je me trompe peut-être) donc pour moi ça reste utile

en comparaison avec l'utilité de savoir le nombre de visiteurs uniques pour le même évènement

C'est à dire ?

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 désolé ces commentaires sont datés, si tu regardes dans Conversation et matomo-events.ts je les ai finalement remis 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

D'acc merci, chaud pour les enlever si on voit qu'on ne s'en sert pas :)

@bjlaa bjlaa marked this pull request as ready for review May 4, 2023 09:26
@cypress
Copy link

cypress bot commented May 4, 2023

Passing run #133 ↗︎

0 23 0 0 Flakiness 0

Details:

Merge b981f92 into 31d4472...
Project: Nos Gestes Climat Commit: 1deeda12a9 ℹ️
Status: Passed Duration: 02:22 💡
Started: May 4, 2023 2:58 PM Ended: May 4, 2023 3:01 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@bjlaa bjlaa linked an issue May 4, 2023 that may be closed by this pull request
@bjlaa
Copy link
Contributor Author

bjlaa commented May 4, 2023

@laem @Clemog @dxb @EmileRolley je prendrais bien une petite review avant de merger :) voir si j'ai pas oublié un truc

@Clemog
Copy link
Contributor

Clemog commented May 4, 2023

je prendrais bien une petite review avant de merger :) voir si j'ai pas oublié un truc

Tu as des fichiers ou features plus spécifiques que tu veux qu'on regarde ?

@bjlaa
Copy link
Contributor Author

bjlaa commented May 9, 2023

@Clemog hmm peut-être le côté logique dans MatomoContext !

@bjlaa
Copy link
Contributor Author

bjlaa commented May 9, 2023

J'ai refait une bonne passe et ça m'a l'air bon ! Je merge ça :)

@bjlaa bjlaa merged commit d719038 into master May 9, 2023
@bjlaa bjlaa deleted the fix-event-triggering branch May 9, 2023 08:33
@Clemog
Copy link
Contributor

Clemog commented May 9, 2023

Trop cool ! Hésite pas à faire un post sur mattermost :)

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.

Faire un troisième audit des événements de parcours de simulation
3 participants