chamilo / chamilo-lms

Chamilo is a learning management system focused on ease of use and accessibility
https://chamilo.org
GNU General Public License v3.0
798 stars 480 forks source link

Problème au sein de l'API ajax New Scorm. #1685

Open dbm-abernard opened 7 years ago

dbm-abernard commented 7 years ago

[Bug #8515 sur la plate-forme support.chamilo.org]

Je pense que nous avons détecté un problème au sein de l'API ajax New Scorm.

Les performances se dégradent drastiquement lors de l'appel : chamilo/main/newscorm/lp_ajax_save_item.php nous avons des temps de réponse pouvant aller jusqu'à plus de 60s

En analysant il s'avère que c'est $myLP->get_complete_items_count() qui provoque le ralentissement. (learnpath::get_complete_items_count, learnpathItem::get_status)

N'y a t-il pas un problème de conception sur cette boucle effectuant une requête en base sur tous les items du parcours ? La table c_lp_item_view pouvant être très conséquente > 500 000 ligne ne faut il pas revoir ce fonctionnement ?

Pouvez vous nous confirmer qu'il y a bien un défaut sur ce point ?

Nous avons pour la méthode status_is désactivé les requêtes via le flag de la methode get_status $currentStatus = $this->get_status(true); => $currentStatus = $this->get_status(false); ce qui nous a permis de revenir à des temps de réponse corrects.

Quel impact cela peut-il avoir sur le calcul des statistiques ? Quel(s) autre(s) réglage(s) ou action pouvons nous mener pour retrouver un niveau de temps de réponse acceptable ?

tests effectués sur la version 1.11.2

Mis à jour par Yannick Warnier il y a 3 jours Bonjour Michèle,

Nous transitons doucement vers https://github.com/chamilo/chamilo-lms/issues pour le rapport d'erreurs/améliorations. Il vaut donc mieux enregistrer ceci à cet endroit.

Avant de jeter un oeil plus en détail sur ce que tu indiques (qui m'a l'air intéressant et exploitable), je trouve un peu perturbant que tu rapportes des tests sur 1.11.2 dans le répertoire main/newscorm/ alors que celui-ci a changé de nom dans 1.11.0 vers main/lp/ Pourrais-tu dès lors me confirmer la version, histoire de ne pas travailler inutilement?

Réponse Merci pour ce retour, concerant le nom du répertoire effectivement nous vous avons communiqué celui de la version 1.10.8 (version sur laquelle nous avons initiallement détecté le problème)

Nous vous confirmons que nous avons bien mené les mêmes tests sur la version 1.11.2. Nous reproduisons l'anomalie pour cette version (la montée de version ne corrige pas le défaut).

Je prends note du changement de plate-forme pour le rapport d'erreurs/améliorations.

dbm-abernard commented 7 years ago

Bonjour @ywarnier Avez-vous vu notre réponse concernant la version testée ?

Merci

ywarnier commented 7 years ago

Après révision du code, je comprends qu'il y ait préoccupation du côté de l'efficacité, mais je ne suis pas certain que le problème soit là (plutôt que du côté de l'optimisation de la base de données).

En effet, s'il est vrai que get_status() fait une requête à la base de données, celle-ci se fait en utilisant les champs id, c_id et view_count. Le champ c_id est indexé, réduisant considérablement l'effort de requête. view_count ne l'est pas mais est généralement très limité (il ne change que si on active le multi-view au niveau du parcours). Enfin, le champ "id" (l'id du lp_item_view), lui, devrait être indexé, mais ne l'est pas, en conséquence d'une transition que nous faisons entre ces versions (1.9->2.0) entre une combinaison unique de c_id + id et un nouvel ID unique entre tous les cours, iid.

En changeant vers iid (mais pas au niveau de cette requête apparemment), nous avons donc changé l'index de la table (de id vers iid), sans pour autant changer la requête, ce qui est une erreur de notre part.

Je suggère donc de commencer par tester cette très simple modification dans get_status() et de m'indiquer si cela améliore suffisamment la situation:

                $sql = "SELECT status FROM $table
                        WHERE
                            c_id = $course_id AND
                            id = '".$this->db_item_view_id."' AND
                            view_count = '" . $this->get_attempt_id()."'";

vers

                $sql = "SELECT status FROM $table
                        WHERE
                            iid = '".$this->db_item_view_id."' AND
                            view_count = '" . $this->get_attempt_id()."'";

Techniquement, je pense qu'on pourrait se passer également de la partie view_count, mais ça reste à tester:

                $sql = "SELECT status FROM $table
                        WHERE
                            iid = ".$this->db_item_view_id;
ywarnier commented 7 years ago

Le problème d'appeler get_status(false), ce que nous faisions auparavant, c'est que les mises à jour en SCORM sont générées par des appels AJAX asynchrones. Ils peuvent donc se produire à n'importe quel moment, et donc jusqu'au dernier moment avant de consulter get_status(). Si on utilise get_status(false), on court le risque que le statut ait été modifié entretemps, et que le résultat à afficher pour le progrès actuel soit incorrect. C'est la seule influence que ça a, mais certains de nos clients nous avaient fait noter qu'elle était importante.

ywarnier commented 7 years ago

S'il n'est pas possible de faire les tests correspondants d'ici 3-4 jours, je passerai la tâche dans la liste prévue pour 2.0. En attendant, on sait qu'il y a différents moyens d'optimiser

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.