Skip to content

Conversation

@tonial
Copy link
Contributor

@tonial tonial commented Sep 2, 2024

🤔 Pourquoi ?

Pour avoir le bon nombre de membres.
Pour les organisations, il est beaucoup trop coûteux de faire une annotation, donc je suis resté sur un calcul python avec un prefretch plus restrictif. (j'ai arrêté après 300 orga exportées sur 8600 en plus d'une minute...)

populate_companies

avant après
1 91.56 s 48.58 s
2 111.66 s 50.99 s
3 80.79 s 49.30 s
moyenne 94.67 s 49.62 s -47%

populate_organizations

avant après
1 72.67 s 70.05 s
2 73.06 s 71.34 s
3 79.18 s 91.29 s
moyenne 74.97 s 77.56 s +3%

Il faut éviter les annotations sur les memberships / members qui sont très coûteuse (aucune idée de pourquoi 🤷)

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?

🏝️ Comment tester

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

@tonial tonial requested a review from rsebille September 2, 2024 04:53
@tonial tonial self-assigned this Sep 2, 2024
@tonial tonial added the modifié Modifié dans le changelog. label Sep 2, 2024
@tonial tonial force-pushed the alaurent/stats branch 2 times, most recently from f61a4b1 to 29e4720 Compare September 2, 2024 08:37
def get_first_membership_join_date(memberships):
memberships = list(memberships.all())
# We have to do all this in python to benefit from prefetch_related.
# Prefetched memberships are sorted by -joined_at
Copy link
Contributor

Choose a reason for hiding this comment

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

Le order_by() pourrais être modifié ou supprimé que personne irai voir dans cette fonction, donc pas super à l'aise d'avoir cette supposition.
Par contre le tri est effectivement coûteux pour rien, ce qu'on cherche au final c'est min(joined_at) donc soit le faire en python ici, ou alors peut-être essayer en forçant une sous-requête avec un .annotate(date_inscription=SubQuery(...)).
Le coup de la sous requête pourrais d'ailleurs peut-être aussi être essayer pour le active_memberships plutôt que prefetch 🤷, y a moyen que ça soit pas pire niveaux perf et ça nous éviterais du code 🤷.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alors de manière assez incompréhensible, quand j'ai essayé de faire un annotate à la place, c'était plus long (pour les entreprises) ou infiniment trop long (pour les organisations).

Je vais ré-essayer d'écrire les annotations pour essayer de comprendre ce qui pose problème.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ce que tu me dis me choque pas pour un .annotate(date_inscription=Min("members__joined_at")) car il va y avoir un JOIN de la mort avec les utilisateurs, mais un .annotate(date_inscription=SubQuery(CompanyMembership.objects.active().filter(company=OuterRef("pk").annotate(min=Min("joined_at").values("min")))) (pas sûr des parenthèses mais tu vois l'idée 😁) devrait je pense plutôt bien s'en sortir et ne pas être dépendant des autres annotations ou de la requête de base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

first_membership_join_date=Subquery(
    CompanyMembership.objects.filter(company_id=OuterRef("id"))
    .values("company_id")
    .annotate(min=Min("joined_at"))
    .values("min")
),
active_membership_count=Subquery(
     CompanyMembership.objects.filter(company_id=OuterRef("id"))
     .active()
     .values("company_id")
     .annotate(count=Count("pk"))
     .values("count")
),
last_login=Subquery(
    CompanyMembership.objects.filter(company_id=OuterRef("id"))
    .values("company_id")
    .annotate(last_login=Max("user__last_login"))
    .values("last_login")
),

Ca fonctionne, mais c'est beaucoup plus long (similaire à l'annotation directe)

Je laisse comme c'est ici du coup :)

@tonial tonial requested a review from rsebille September 3, 2024 08:44
@tonial tonial force-pushed the alaurent/stats branch 2 times, most recently from 6a5125c to 716a752 Compare September 3, 2024 09:08
@tonial tonial added this pull request to the merge queue Sep 3, 2024
Merged via the queue into master with commit e2c1c34 Sep 3, 2024
@tonial tonial deleted the alaurent/stats branch September 3, 2024 11:01
@francoisfreitag francoisfreitag changed the title Statistiques: Correction du nombre de members dans les exports Statistiques: Correction du nombre de membres dans les exports Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

modifié Modifié dans le changelog.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants