Bug 30984 - Action logs should log the cronjob script name that generated the given log
Summary: Action logs should log the cronjob script name that generated the given log
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-17 10:57 UTC by Kyle M Hall
Modified: 2023-06-08 22:26 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00,22.05.06,21.11.12


Attachments
Bug 30984 - Action logs should log the cronjob script name that generated the given log (4.03 KB, patch)
2022-06-17 11:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30984 - Action logs should log the cronjob script name that generated the given log (4.09 KB, patch)
2022-06-17 14:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30984: Action logs should log the cronjob script name that generated the given log (4.19 KB, patch)
2022-07-29 06:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30984: (QA follow-up) Ternary formatting style (1.01 KB, patch)
2022-07-29 06:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30984: (QA follow-up) Switch from mediumtext to tinytext (2.11 KB, patch)
2022-07-29 10:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30984: (QA follow-up) Switch from mediumtext to varchar(255) (2.35 KB, patch)
2022-07-29 11:03 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2022-06-17 10:57:19 UTC
When something is changed by a cronjob, and that entity is logged via action logs, we can know what changed, and that it was via a cronjob, but we cannot necessarily know which cronjob made that change. The closest we can come is to find the action logs for the cronjob module which ran before the change which is by no means reliable assuming the CronLog is even enabled.

We should add a new column to action logs to store the name of the script ran for any action logs where the interface is "cron".
Comment 1 Kyle M Hall 2022-06-17 11:05:27 UTC
Created attachment 136252 [details] [review]
Bug 30984 - Action logs should log the cronjob script name that generated the given log

When something is changed by a cronjob, and that entity is logged via action logs, we can know what changed, and that it was via a cronjob, but we cannot necessarily know which cronjob made that change. The closest we can come is to find the action logs for the cronjob module which ran before the change which is by no means reliable assuming the CronLog is even enabled.

We should add a new column to action logs to store the name of the script ran for any action logs where the interface is "cron".

Test plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Enable all the Log/Logging sysprefs
4) Run some cronjobs that will generate action logs
5) Note the new action_logs column "script" contains the filename of the
   cronjob that caused the change.
Comment 2 Martin Renvoize 2022-06-17 14:10:01 UTC
Created attachment 136288 [details] [review]
Bug 30984 - Action logs should log the cronjob script name that generated the given log

When something is changed by a cronjob, and that entity is logged via action logs, we can know what changed, and that it was via a cronjob, but we cannot necessarily know which cronjob made that change. The closest we can come is to find the action logs for the cronjob module which ran before the change which is by no means reliable assuming the CronLog is even enabled.

We should add a new column to action logs to store the name of the script ran for any action logs where the interface is "cron".

Test plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Enable all the Log/Logging sysprefs
4) Run some cronjobs that will generate action logs
5) Note the new action_logs column "script" contains the filename of the
   cronjob that caused the change.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2022-06-17 14:12:31 UTC
Nice improvement in logging.. I initially thought we'd see it passed in and part of Koha::Script rather than being populated from within C4::Log.. but I'm ambivalent as to where it should live.  I wonder about background tasks, whether we'd want to record those as 'scripts' too somewhere along the line..

Signing off anyway as it works as described.
Comment 4 Marcel de Rooy 2022-07-29 06:14:38 UTC
Processing additional checks

        * Commit title does not start with 'Bug XXXXX: ' - 5dd5449223
Comment 5 Marcel de Rooy 2022-07-29 06:41:59 UTC
Created attachment 138243 [details] [review]
Bug 30984: Action logs should log the cronjob script name that generated the given log

When something is changed by a cronjob, and that entity is logged via action logs, we can know what changed, and that it was via a cronjob, but we cannot necessarily know which cronjob made that change. The closest we can come is to find the action logs for the cronjob module which ran before the change which is by no means reliable assuming the CronLog is even enabled.

We should add a new column to action logs to store the name of the script ran for any action logs where the interface is "cron".

Test plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Enable all the Log/Logging sysprefs
4) Run some cronjobs that will generate action logs
5) Note the new action_logs column "script" contains the filename of the
   cronjob that caused the change.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2022-07-29 06:42:03 UTC
Created attachment 138244 [details] [review]
Bug 30984: (QA follow-up) Ternary formatting style

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2022-07-29 06:44:12 UTC
Just mentioning here that mediumtext is not really needed for the script name; tinytext would suffice (256).
Comment 8 Kyle M Hall 2022-07-29 10:53:32 UTC
Created attachment 138294 [details] [review]
Bug 30984: (QA follow-up) Switch from mediumtext to tinytext
Comment 9 Kyle M Hall 2022-07-29 11:03:00 UTC
Created attachment 138295 [details] [review]
Bug 30984: (QA follow-up) Switch from mediumtext to varchar(255)

My research indeicates that tinytext of less than 40 bytes uses
about the same storage as a varchar(255). Also, using
a TEXT field in a complex query may create a temp table.
varchar fields are also more efficient for sorting on.
Comment 10 Tomás Cohen Arazi 2022-08-01 12:31:26 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 11 Lucas Gass 2022-09-29 15:32:18 UTC
Backported to 22.05.x for 22.05.06
Comment 12 Victor Grousset/tuxayo 2022-10-13 16:02:07 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.
Comment 13 Arthur Suzuki 2022-10-22 22:08:27 UTC
Thanks!

Pushed to 21.11 for 21.11.12