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

Fix displayed lines in log #2546

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

BadWolf42
Copy link
Contributor

Proposed change

As per bug report here: https://community.jeedom.com/t/niveau-de-logs/124440

Type of change

  • Bugfix (non breaking change)

Test check

No test done for this PR!
This should be tested first.

Documentation

N/A

@zoic21
Copy link
Contributor

zoic21 commented Apr 18, 2024

Bonjour,
Je suis pas sur la car ca limite le nombre de ligne renvoyé par la fonction d'affichage. Pour la limitation du nombre de ligne dans les logs c'est la https://github.com/jeedom/core/blob/alpha/core/class/jeedom.class.php#L1223 qui passe une fois par jour d'ou le faite que maintenant on peut avoir plus de ligne dans les logs. Après il faut peut être la faire tourner toute les heures.

@BadWolf42
Copy link
Contributor Author

Oui, l'ancienne fonction d'affichage tronquait les fichiers de log à chaque affichage :

public static function get($_log, $_begin, $_nbLines) {
$path = (!file_exists($_log) || !is_file($_log)) ? self::getPathToLog($_log) : $_log;
if (!file_exists($path)) {
return false;
}
self::chunkLog($path);

Et cela faisait (selon moi) beaucoup d'écritures disque pour pas grand chose. Lorsque j'ai écrit la fonction log::getDelta(), j'ai supprimé ce mécanisme et préféré ignorer les ligne au delà des 4000 dernières (valeur statique) :

* @param int $_max Max number of returned lines (default 4000)

Initialement dans log::get(), le fait que le log était tronqué permettait de ne renvoyer que le nombre max de lignes définies dans la config (cf log::getConfig('maxLineLog')).
Ok, cette purge n'a lieu maintenant qu'une fois par jour, mais avec ce PR (#2546) l'affichage respecte le nombre de lignes à garder selon le paramètre de config de Jeedom et plus arbitrairement les 4000 dernières.

Garder tous les logs du jour permet d'avoir plus d'historique et de pouvoir demander à un utilisateur, s'il y a besoin de plus de logs pour du début, de transférer le fichier de log complet, simplement le téléchargeant.

Après il faut peut être la faire tourner toute les heures.

Effectivement, i'l faut s'assurer que la taille des logs ne dépassent pas un certain maximum (par ex 3Mo par fichier de log). Peut-être que ça devrait être le travail du check de santé (cf #2438) et qu'il devrait tourner toutes les heures ?

@Mips2648
Copy link
Collaborator

Je me permet de donner mon avis, c'est juste un avis:
je suis aligné avec le fait que la fonction qui renvoi le contenu respecte ce qui est configuré: ca permet de ne pas se soucier si le fichier a bien été tronqué ou pas et surtout si on a vmt bcp de log il arrive actuellement que ca pose problème coté client

donc en bref: je suis pour ce PR

@zoic21 zoic21 requested a review from an0Nym0us63 April 19, 2024 09:06
@zoic21
Copy link
Contributor

zoic21 commented Apr 19, 2024

Ok effectivement j'ai pas pensé qu'en téléchargeant on aura effectivement le fichier complet donc c'est bon pour moi.

@Sekiro-kost Sekiro-kost self-requested a review April 22, 2024 07:39
@Sekiro-kost Sekiro-kost merged commit bfb8b13 into jeedom:alpha Apr 22, 2024
4 checks passed
@BadWolf42 BadWolf42 deleted the fix-displayed-log-line-count branch April 22, 2024 07:51
@BadWolf42
Copy link
Contributor Author

Attention @zoic21, @Sekiro-kost : je n'ai pas testé ce PR.
Même si le changement est assez simple, je suppose que vous avez testé son bon fonctionnement ?

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.

4 participants