Remplace ActiveRecord.strict_loading par Prosopite pour détecter les requêtes N+1
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 moden_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 !