Refactor chunkLog function for better performance by jpty · Pull Request #3288 · jeedom/core

jpty · 2026-04-20T13:45:44Z

Truncates log files only if the file size or row number thresholds are exceeded.
Using uniqid() instead of tempnam() because
- tempnam() crée un fichier temporaire dans /tmp/jeedom en mode 0600 avec comme propriétaire www-data
- Sur mon Jeedom /tmp/jeedom est un répertoire avec le sticky bit (drwxrwxrwt propriétaire root)
- sudo bash -o pipefail -c " tail -c | tail -n > tmpFile ..." n'a pas les droits d'écrire dans tmpFile puisqu'il n'en est pas le propriétaire. Ca donne Permission denied tail: error writing 'standard output': Broken pipe
- uniqid() ne génére qu'un nom de fichier qui est utilisé ensuite dans la commande de tronquage.

Description

Suggested changelog entry

Related issues/external references

Fixes #

Types of changes

Bug fix (non-breaking change which fixes)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
- This change is only breaking for integrators, not for external standards or end-users.
Documentation improvement

PR checklist

I have checked there is no other PR open for the same change.
I have read the [La ligne directrice pour contribuer à ce projet / Contribution guidelines for this project).
I grant the project the right to include and distribute the code under the GNU.
I have added tests to cover my changes.
I have verified that the code complies with the projects coding standards.
[Required for new sniffs] I have added MD documentation for the sniff.

- Truncates log files only if the file size or row number thresholds are exceeded. - Using uniqid() instead of tempnam()

Mips2648 · 2026-04-20T14:33:31Z

Attention de pas casser le format et de respecter les indentations existantes sur le fichier, j'ai modifié

Mips2648 · 2026-04-20T15:06:30Z

Concernant le changement, je trouve que la fonction devient trop complex à lire... et il y a trop de if/else imbriqué
les log en debug sont inutiles à moins d'avoir le log level par défaut en debug et ca alourdi visuellement le code aussi

Le but est la perf, ok mais tu as su mesurer? est-ce que complexifier le code et donc la maintenance vaut le gain de perf?

le wc -l s'est bien mais ca va couter chère si la config permet 100Mo de fichier...

si vous voulez garder le principe pour raison de perf, je veux refacto pour simplifier le code mais je pense que le mieux est l'ennemi du bien

jpty · 2026-04-20T15:45:41Z

Les log::add, c'est pour la phase de debuggage.
Si on les enlève, il n'y a plus aucun else dans la fonction et donc juste un if dans un autre if.

Les mesures que j'ai faites, c'est le temps d'exécution de jeedom::cronDaily sur 166 fichiers de log. 19s avant optimisation, 2s après . Il n'y a qu'une dizaine de fichiers à tronquer par le nombre de lignes. Je suppose que la carte SD me remercie de ne pas réécrire le fichier à l'identique à chaque exécution.

le wc -l s'est bien mais ca va couter chère si la config permet 100Mo de fichier...

Tout dépend ce que l'on appelle cher, sur un fichier de 290Mo, 0.2 seconde:

Il y a peut-être une limite raisonnable à fixer dans la config. Pour le moment, c'est du texte libre. 0 est possible ...
Après les derniers commits, maxSizeLog est bornée entre 1 et 10Mo.
Le comptage des lignes n'est fait que si la taille du log est inférieure à ce chiffre de la config. La valeur par défaut dans la config est 5Mo.

jpty · 2026-04-20T15:49:50Z

Attention de pas casser le format et de respecter les indentations existantes sur le fichier, j'ai modifié

Oui. J'ai vu. Désolé. Je me suis contenté de copier / coller depuis mon éditeur. Mon éditeur convertit toutes les tabulations en 2 espaces dès que je modifie un fichier.
Il y a des conventions d'écriture quelque part ou c'est selon le fichier ?
Je ne sais même pas où trouver comment le fichier que j'édite dans Github est indenté.

Mips2648 · 2026-04-20T18:52:58Z

bon, j'ai poussé une autre version mais j'ai gardé l'idée des circuit-breaker si taille < autorisée && nbr lines < autorisée mais j'ai simplifié
j'ai laissé mes comments justement pour justifier/valider les détails

jpty · 2026-04-20T20:08:17Z

j'ai laissé mes comments justement pour justifier/valider les détails

Pas vu de comments

Mips2648 · 2026-04-21T07:56:35Z

pas dans le code, sur la PR, ici plus haut

jpty · 2026-04-21T14:29:01Z

Bonjour,
Testé cette nuit sur ma "prod" pour avoir des vrais logs à traiter.
Exécution en 3 secondes. 110 fichiers de log et seulement 15 à tronquer. 4 par la taille, 11 par le nombre de lignes.
Et vous ?

Co-authored-by: jpty <1798636+jpty@users.noreply.github.com>

Co-authored-by: kwizer15 <kwizer15@users.noreply.github.com>

Co-authored-by: jpty <1798636+jpty@users.noreply.github.com>

Mips2648

It seems my last review was not submitted or discarded

Mips2648 · 2026-04-20T18:11:32Z

+
+		clearstatcache(true, $rawPath);
+		$fSize = filesize($rawPath);
+		if ($fSize < ($maxLineLog * 50)) { // 50 characters per line


ce n'est pas le role de la fonction de décider de ne pas faire un chunk si le fichier est trop petit
la règle business c'est de limiter en taille et/ou en nombre de ligne; une exception sur la taille est déjà gérée par chunk()

L'exception sur la taille gérée par chunk() est faite sans bornage de maxSizeLog:
if ($_onlyIfSizeExceeded && filesize($path) < (self::getConfig('maxSizeLog') * 1024 * 1024))

chunkLog est aussi appelé dans log::get() fonction deprecated qui ne signale pas qu'elle l'est et pourra donc générer des erreurs quand elle sera appelée mais supprimée.

Mips2648 · 2026-04-20T18:14:30Z

+		}

+		$sudo = system::getCmdSudo();
+		$tmpFile = escapeshellarg(jeedom::getTmpFolder() . '/log_chunk_' . uniqid('', true));


pourquoi concaténer le prefix au lieu de le passer à uniqid() ?
et pourquoi augmenter entropie? on n'est pas dans un truc critique ni à bombarder des milliers de cas par seconde
le but c'était juste d'éviter une collision évidente si 2 chunk arrive en même temps
on peut gagner du temps cpu en gardant la valeur par défaut

Il faut déjà concaténer le /
Pour le temps cpu, 1 million d'exécution d'uniqid() = 1s avec ou sans entropie.
Je n'ai pas modifié le code.

Mips2648 · 2026-04-20T18:16:46Z

+		if ($fSize < $maxBytes) { // Check by nLines
+			$cmd = "wc -l " . escapeshellarg($rawPath);
+			$output = trim(com_shell::execute($cmd));
+			$nLines = (int) explode(' ', $output)[0];


je pense qu'on peut éviter l'array et l'explode avec un wc -l < file

Je suis d'accord. Pas pensé à faire wc -l <
C'est fait dans la version actuelle.

Mips2648 · 2026-04-20T18:48:51Z


+		$sudo = system::getCmdSudo();
+		$tmpFile = escapeshellarg(jeedom::getTmpFolder() . '/log_chunk_' . uniqid('', true));
+		$shellPath = escapeshellarg(realpath($rawPath));


y avait une raison d'ajouter le realpath()?

Juste résoudre une seule fois les ../.. présents dans le rawPath avant de les passer à bash.
J'avais aussi utilisé shellPath dans mes log::add pour plus de lisibilité.
C'est supprimé dans la version actuelle.

Mips2648 · 2026-04-22T08:20:32Z


 		$maxSizeLog = (int)self::getConfig('maxSizeLog', 10);
-		$maxSizeLog = max(1, $maxSizeLog);
+		$maxSizeLog = max(1, min(10, $maxSizeLog));


c'est un changement fonctionnel, pourquoi limiter à 10 ici? il ne faut pas faire ca

/tmp/jeedom a une taille de 256Mo sur la VM de mon NAS.
Sur la Smart debian 12, /tmp est un tmpfs. Taille 955Mo
Dans la config maxSizeLog est du texte libre et tu as écrit plus haut que ca pourrait être 100Mo voir plus.
10 est la limite dans chunkLog avant les modifs.

Mips2648 · 2026-04-22T08:26:34Z


 		$fSize = filesize($rawPath);
-		if ($fSize === false) {
+		if ($fSize === false || $fSize < ($maxLineLog * 50) ) {


pourquoi avoir remis cette ligne?
je suis contre, ce 50 est complétement arbitraire
ca apporte quoi?

Ca sert à ne pas perdre de temps à compter des lignes sur des petits fichiers pour dans le meilleur des cas gagner quelques octets.
com_shell::execute("wc -l ... prend 2ms sur un fichier vide.

Pour les 50, c'est déduit de mes fichiers de log.
Sur mon relevé d'hier et 21Mo de log, la moyenne du nombre de caractères par ligne est 200.
A voir pour augmenter le 50.

Suggested change

if ($fSize === false || $fSize < ($maxLineLog * 50) ) {

if ($fSize === false || $fSize < ($maxLineLog * 80) ) {

Il faut aussi changer filesize en @filesize puisque la vérification de l'existence du fichier n'est plus faite au début et ainsi éviter le PHP Warning dans http.error

Mips2648 · 2026-04-22T08:28:03Z


 			clearstatcache(true, $rawPath);
-			if (file_exists($rawPath) && filesize($rawPath) > (1024 * 1024 * $maxSizeLog)) {
+			if (file_exists($rawPath) && filesize($rawPath) > (10 *1024 *1024)) {


pourquoi remettre en dur 10 au lieu d'utiliser la variable qui calcul déjà ca?

Je trouve anormal de vider le fichier pour quelques octets en trop.
10 est l'ancienne limite de taille.

jpty · 2026-04-23T07:03:40Z

Bonjour,
J'ai écrit plus haut que je n'avais pas vu vos commentaires. Donc plutôt not submitted.

jpty · 2026-04-23T09:07:53Z

Je crois avoir commenté tous vos commentaires. A vous de commenter.

Ce ne sont que des logs. Les daemons continuent d'y écrire. Il est fortement possible que le nombre de lignes soit déjà dépassé dès la fin d'une exécution réussie.

jpty · 2026-04-23T09:25:57Z

+		$maxSizeLog = max(1, min(10, $maxSizeLog));
+		$maxBytes = $maxSizeLog * 1024 * 1024;
+
+		$fSize = filesize($rawPath);


Pour éviter les PHP Warning filesize(): stat failed for

Suggested change

$fSize = filesize($rawPath);

$fSize = @filesize($rawPath);

jpty and others added 3 commits April 20, 2026 15:17

Refactor chunkLog function for better performance

ed6b110

- Truncates log files only if the file size or row number thresholds are exceeded. - Using uniqid() instead of tempnam()

file format

2c5f3bd

log message formatting in chunk method for better readability

5f8980e

kwizer15 approved these changes Apr 20, 2026

View reviewed changes

Comment thread core/class/log.class.php Outdated

Comment thread core/class/log.class.php Outdated

Comment thread core/class/log.class.php Outdated

Refacto optimizaitions

098bcdd

jpty commented Apr 20, 2026

View reviewed changes

Comment thread core/class/log.class.php Outdated

Comment thread core/class/log.class.php Outdated

jpty commented Apr 20, 2026

View reviewed changes

Comment thread core/class/log.class.php

Apply suggestions from code review

ce39eb8

Co-authored-by: jpty <1798636+jpty@users.noreply.github.com>

jpty commented Apr 21, 2026

View reviewed changes

Comment thread core/class/log.class.php Outdated

jpty and others added 2 commits April 22, 2026 00:04

Apply suggestions from code review

c21dac7

Co-authored-by: jpty <1798636+jpty@users.noreply.github.com>

Apply suggestions from code review

ada7bef

Co-authored-by: kwizer15 <kwizer15@users.noreply.github.com>

jpty commented Apr 21, 2026

View reviewed changes

Comment thread core/class/log.class.php Outdated

jpty added 2 commits April 22, 2026 00:50

Apply suggestions from code review

c83c7c7

Co-authored-by: jpty <1798636+jpty@users.noreply.github.com>

Update core/class/log.class.php

e1f3102

Mips2648 reviewed Apr 23, 2026

View reviewed changes

jpty commented Apr 23, 2026

View reviewed changes

	if ($fSize === false \|\| $fSize < ($maxLineLog * 50) ) {
	if ($fSize === false \|\| $fSize < ($maxLineLog * 80) ) {

Conversation

jpty commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry

Related issues/external references

Types of changes

PR checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mips2648 commented Apr 20, 2026

Uh oh!

Mips2648 commented Apr 20, 2026

Uh oh!

jpty commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpty commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mips2648 commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpty commented Apr 20, 2026

Uh oh!

Mips2648 commented Apr 21, 2026

Uh oh!

jpty commented Apr 21, 2026

Uh oh!

Uh oh!

Uh oh!

Mips2648 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpty Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpty Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpty Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpty Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpty Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpty commented Apr 20, 2026 •

edited

Loading

jpty commented Apr 20, 2026 •

edited

Loading

jpty commented Apr 20, 2026 •

edited

Loading

Mips2648 commented Apr 20, 2026 •

edited

Loading

jpty Apr 23, 2026 •

edited

Loading

jpty Apr 23, 2026 •

edited

Loading

jpty Apr 23, 2026 •

edited

Loading

jpty Apr 23, 2026 •

edited

Loading

jpty Apr 23, 2026 •

edited

Loading