Skip to content

Remplace ActiveRecord.strict_loading par Prosopite pour détecter les requêtes N+1

Pierre de La Morinerie requested to merge n-plus-one into main

Le problème

Jusque ici, on utilisait le réglage ActiveRecord.strict_loading pour détecter les requêtes SQL N+1.

Mais ça pose plusieurs problèmes :

  • Beaucoup de faux positifs : à chaque fois qu'on fait une requête qui est juste une +1 (territory.row), Rails nous en empêche, et demande à ce qu'on préload l'association.
  • Beaucoup de faux négatifs : Rails ne détecte pas beaucoup de N+1 qui sont pourtant aujourd'hui faites dans l'application.
  • [Edit @n-b] Par ailleurs, ActiveRecord.strict_loading ne fonctionne pas réellement pour le mode n_plus_one_only, ou du moins pas de la façon qu’on peut imaginer. On avait essayé d’y faire quelque chose, cf !217 (merged) et !327 (merged).

La solution

Cette PR remplace la détection de N+1 par la gem prosopite.

Le mécanisme : si deux requêtes SQL identiques ont la même stacktrace, alors c'est une N+1. Simple, basique.

Ça a trouvé un certain nombre de N+1 présentes dans le code, mais que Rails ne détectait pas. Certains sont bénignes (voire presque désirables, comme des N+1 par layer). D'autres sont corrigées par cette PR. Certaines sont ignorées pour l'instant, et demanderont un refactoring plus tard.

Détails d'implémentation

  • Portée : les N+1 sont détectées dans le code exécuté par des contrôleurs (via un middleware inséré dans la stack Rack). Ça concerne dont essentiellement les tests systèmes. La détection de N+1 dans des jobs n'est pas encore activée (plus tard).
  • Reporting : En développement et dans les tests, les N+1 jettent une exception direct. En production, le problème est juste remonté dans les logs.
  • Issue de secours : On peut ignorer temporairement une requête N+1 en l'entourant de Prosopite.pause do … end.

Éléments à refactorer plus tard

  • Le bouton "Centrer la carte sur ce calque" fait une N+1 par layer (on met la bounding box en dur). Il faudra à terme le remplacer par une implem asynchrone.
  • Le pied-de-table des layers fait une N+1 en calculant la somme des layers. On voudra sans doute grouper ça dans le SQL.

Relecture

Si vous avez des commentaires, je prends volontiers. Y'a p-ê des N+1 qu'on devrait retravailler tout de suite, ou d'autres où il faut une autre approche. Le SQL n'est vraiment pas mon fort, alors je prends les retours !

Edited by Nicolas Bouilleaud

Merge request reports