Description
Marc Véron
2015-03-22 12:47:23 UTC
Created attachment 37082 [details] [review] Bug 13889 - Add information about cron jobs to system log This patch adds information about cron jobs performed and make it viewable under Home > Tools > Logs ("Browse system logs") To test: Apply patch - Got to system preferences and set 'CronjobLog' to: [Log] information from cron jobs. - Run some cron jobs - Go to Home > Tools > Logs - Verify that you have a selection 'Cron jobs' in drop-down 'Module'. Select it with Action "All" and Submit. - Output should show Dae/time and info about Cron jobs Created attachment 37325 [details] [review] Bug 13889 - Add information about cron jobs to system log This patch adds information about cron jobs performed and make it viewable under Home > Tools > Logs ("Browse system logs") To test: Apply patch - Got to system preferences and set 'CronjobLog' to: [Log] information from cron jobs. - Run some cron jobs - Go to Home > Tools > Logs - Verify that you have a selection 'Cron jobs' in drop-down 'Module'. Select it with Action "All" and Submit. - Output should show Dae/time and info about Cron jobs Rebased to work on top of Bug 6911 (conflict in viewlog.tt) /MV I wasn't able to apply this patch on master: fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt). Created attachment 37679 [details] [review] Bug 13889 - Add information about cron jobs to system log This patch adds information about cron jobs performed and make it viewable under Home > Tools > Logs ("Browse system logs") To test: Apply patch - Got to system preferences and set 'CronjobLog' to: [Log] information from cron jobs. - Run some cron jobs - Go to Home > Tools > Logs - Verify that you have a selection 'Cron jobs' in drop-down 'Module'. Select it with Action "All" and Submit. - Output should show Dae/time and info about Cron jobs Rebased to work on top of Bug 6911 (conflict in viewlog.tt) /MV Rebased after applying patch for Bug 6911 /MV Conflicts resolved: misc/cronjobs/overdue_notices.pl misc/cronjobs/cleanup_database.pl (In reply to Frédéric Demians from comment #3) > I wasn't able to apply this patch on master: > > fatal: sha1 information is lacking or useless > (koha-tmpl/intranet-tmpl/prog/en/modules/tools/viewlog.tt). Hi Frédéric, thanks for testing. The patch depends on Bug 6911 (has changes to viewlog.tt), my patch should apply on top of this bug. I rebased 2 more conflicts after applying 6911. Re-tested applying, works for me on current master. (In reply to Marc Véron from comment #5) > The patch depends on Bug 6911 (has changes to viewlog.tt), my patch should > apply on top of this bug. > I rebased 2 more conflicts after applying 6911. > Re-tested applying, works for me on current master. Works for me also. Your patch add a new syspref in .pref file. I think you still have to add it in the DB also: http://wiki.koha-community.org/wiki/System_Preferences#Adding_a_new_system_preference Created attachment 37707 [details] [review] [Follow-up]Bug 13889 Add syspref to sysprefs.sql and updatedatabase.pl This patch adds the syspref CronjobLog to sysprefs.sql and updatedatabase.pl Created attachment 37711 [details] [review] Bug 13889 - Add information about cron jobs to system log This patch adds information about cron jobs performed and make it viewable under Home > Tools > Logs ("Browse system logs") To test: Apply patch - Got to system preferences and set 'CronjobLog' to: [Log] information from cron jobs. - Run some cron jobs - Go to Home > Tools > Logs - Verify that you have a selection 'Cron jobs' in drop-down 'Module'. Select it with Action "All" and Submit. - Output should show Date/time and info about Cron jobs Rebased to work on top of Bug 6911 (conflict in viewlog.tt) /MV Rebased after applying patch for Bug 6911 /MV Conflicts resolved: misc/cronjobs/overdue_notices.pl misc/cronjobs/cleanup_database.pl Signed-off-by: Frederic Demians <f.demians@tamil.fr> - Merge both patches, and fix updatedatabase.pl - Works as described. Provide intersting feedback from cronjob scripts. The syspref CronjobLog is created, but is not used. Instead the cronjob logs are always written. I'm not sure if this is a bad thing. However, either then system preference needs to be removed, or utilized. Created attachment 38116 [details] [review] Bug 13889 - Add information about cron jobs to system log This patch adds information about cron jobs performed and make it viewable under Home > Tools > Logs ("Browse system logs") To test: Apply patch - Got to system preferences and set 'CronjobLog' to: [Log] information from cron jobs. - Run some cron jobs - Go to Home > Tools > Logs - Verify that you have a selection 'Cron jobs' in drop-down 'Module'. Select it with Action "All" and Submit. - Output should show Date/time and info about Cron jobs Rebased to work on top of Bug 6911 (conflict in viewlog.tt) /MV Rebased after applying patch for Bug 6911 /MV Conflicts resolved: misc/cronjobs/overdue_notices.pl misc/cronjobs/cleanup_database.pl Signed-off-by: Frederic Demians <f.demians@tamil.fr> - Merge both patches, and fix updatedatabase.pl - Works as described. Provide intersting feedback from cronjob scripts. -- Modified version taking in account syspref CronJobLog. Handling simplified by introducing a convenience sub cronlogaction in C4/Log.pm /MV Comment on attachment 38116 [details] [review] Bug 13889 - Add information about cron jobs to system log Review of attachment 38116 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/sysprefs.sql @@ +474,5 @@ > ('XSLTDetailsDisplay','default','','Enable XSL stylesheet control over details page display on intranet','Free'), > ('XSLTResultsDisplay','default','','Enable XSL stylesheet control over results page display on intranet','Free'), > ('z3950AuthorAuthFields','701,702,700',NULL,'Define the MARC biblio fields for Personal Name Authorities to fill biblio.author','free'), > +('z3950NormalizeAuthor','0','','If ON, Personal Name Authorities will replace authors in biblio.author','YesNo'), > +('CronjobLog','0',NULL,'If ON, log information from cron jobs.','YesNo') This should be placed alphabetically. Comment on attachment 38116 [details] [review] Bug 13889 - Add information about cron jobs to system log Review of attachment 38116 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/updatedatabase.pl @@ +10039,5 @@ > +if ( CheckVersion($DBversion) ) { > + $dbh->do("INSERT IGNORE INTO `systempreferences` (variable,value,explanation,options,type) VALUES ('CronjobLog','0',NULL,'If ON, log information from cron jobs.','YesNo');"); > + print "Upgrade to $DBversion done (Add CronjobLog syspref)\n"; > + SetVersion ($DBversion); > +} This new. Why not do the new atomic update way? http://wiki.koha-community.org/wiki/Database_updates Koha QA tools fails related to my comment #11. And a tab character on line 37 of C4/Log.pm Created attachment 38183 [details] [review] Bug 13889 - Add information about cron jobs to system log This patch adds information about cron jobs performed and make it viewable under Home > Tools > Logs ("Browse system logs") To test: Apply patch - Got to system preferences and set 'CronjobLog' to: [Log] information from cron jobs. - Run some cron jobs - Go to Home > Tools > Logs - Verify that you have a selection 'Cron jobs' in drop-down 'Module'. Select it with Action "All" and Submit. - Output should show Date/time and info about Cron jobs Rebased to work on top of Bug 6911 (conflict in viewlog.tt) /MV Rebased after applying patch for Bug 6911 /MV Conflicts resolved: misc/cronjobs/overdue_notices.pl misc/cronjobs/cleanup_database.pl Signed-off-by: Frederic Demians <f.demians@tamil.fr> - Merge both patches, and fix updatedatabase.pl - Works as described. Provide intersting feedback from cronjob scripts. -- Modified version taking in account syspref CronJobLog. Handling simplified by introducing a convenience sub cronlogaction in C4/Log.pm /MV Amended to take in account comments #11, #12, #13 /MV http://bugs.koha-community.org/show_bug.cgi?id=13899 Reset to 'Needs Signoff'. Created attachment 38227 [details] [review] Bug 13889 - Add information about cron jobs to system log This patch adds information about cron jobs performed and make it viewable under Home > Tools > Logs ("Browse system logs") To test: Apply patch - Got to system preferences and set 'CronjobLog' to: [Log] information from cron jobs. - Run some cron jobs - Go to Home > Tools > Logs - Verify that you have a selection 'Cron jobs' in drop-down 'Module'. Select it with Action "All" and Submit. - Output should show Date/time and info about Cron jobs Rebased to work on top of Bug 6911 (conflict in viewlog.tt) /MV Rebased after applying patch for Bug 6911 /MV Conflicts resolved: misc/cronjobs/overdue_notices.pl misc/cronjobs/cleanup_database.pl Signed-off-by: Frederic Demians <f.demians@tamil.fr> - Merge both patches, and fix updatedatabase.pl - Works as described. Provide intersting feedback from cronjob scripts. -- Modified version taking in account syspref CronJobLog. Handling simplified by introducing a convenience sub cronlogaction in C4/Log.pm /MV Amended to take in account comments #11, #12, #13 /MV http://bugs.koha-community.org/show_bug.cgi?id=13899 Created attachment 38228 [details] [review] Bug 13889: Added Test Coverage Previous commit was lacking test coverage of new function. TEST PLAN --------- 1) Apply all the patches 2) prove -v t/db_dependent/Log.t -- should be success. 3) koha qa test tools Created attachment 38229 [details] [review] Bug 13889 - Add information about cron jobs to system log This patch adds information about cron jobs performed and make it viewable under Home > Tools > Logs ("Browse system logs") To test: Apply patch - Got to system preferences and set 'CronjobLog' to: [Log] information from cron jobs. - Run some cron jobs - Go to Home > Tools > Logs - Verify that you have a selection 'Cron jobs' in drop-down 'Module'. Select it with Action "All" and Submit. - Output should show Date/time and info about Cron jobs Rebased to work on top of Bug 6911 (conflict in viewlog.tt) /MV Rebased after applying patch for Bug 6911 /MV Conflicts resolved: misc/cronjobs/overdue_notices.pl misc/cronjobs/cleanup_database.pl Signed-off-by: Frederic Demians <f.demians@tamil.fr> - Merge both patches, and fix updatedatabase.pl - Works as described. Provide intersting feedback from cronjob scripts. -- Modified version taking in account syspref CronJobLog. Handling simplified by introducing a convenience sub cronlogaction in C4/Log.pm /MV Amended to take in account comments #11, #12, #13 /MV http://bugs.koha-community.org/show_bug.cgi?id=13899 Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 38230 [details] [review] Bug 13889: Added Test Coverage Previous commit was lacking test coverage of new function. TEST PLAN --------- 1) Apply all the patches 2) prove -v t/db_dependent/Log.t -- should be success. 3) koha qa test tools Leaving as 'Needs Signoff', because my test coverage addition can't be signed off by me. Also, I ran the purge suggestions cronjob to test the logging. I did not run all the scripts, though I did look at all of them. They were all the same change: use C4::Log, and trigger a cronlogaction() after a successful start. Created attachment 38236 [details] [review] [Signed-off] Bug 13889: Added Test Coverage Previous commit was lacking test coverage of new function. TEST PLAN --------- 1) Apply all the patches 2) prove -v t/db_dependent/Log.t -- should be success. 3) koha qa test tools Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 38239 [details] [review] Bug 13889: Log with parameters This patch proposes to log the cronjob filepath with the parameters. On this way, we will get even more information. Note that the @ARGV is modified by GetOptions and have to be put before. (In reply to Jonathan Druart from comment #22) > Created attachment 38239 [details] [review] [review] > Bug 13889: Log with parameters Marc, What do you think of this patch? (In reply to Jonathan Druart from comment #23) > (In reply to Jonathan Druart from comment #22) > > Created attachment 38239 [details] [review] [review] [review] > > Bug 13889: Log with parameters > > Marc, What do you think of this patch? -- I know I'm not Marc Véron, but I thought I'd chime in my like of that kind of tweak with the parameters. However, you also moved it before a successful start. If we have it before a successful start, then where will we record the difference between the successful start or failure to start? A failure to log when the cronjoblog is on, is an implied failure to start. And if we distinguish between those two, why not a successful run as well? There's no status column. Just some thoughts. (In reply to Jonathan Druart from comment #23) > (In reply to Jonathan Druart from comment #22) > > Created attachment 38239 [details] [review] [review] [review] > > Bug 13889: Log with parameters > > Marc, What do you think of this patch? Jonatan, I like the idea. I was thinking about adding some more detailed information to the action log before and I planned to expand it after this patch set is pushed. First consideration: With the patch, I have the following problem: Call of cronlogaction() without param does not display the path: 2015-04-21 15:24:09 0 Cron jobs Run 0 Call of cronlogaction(join ' ', $0, @ARGV) displays the path and the params: 2015-04-21 15:24:09 0 Cron jobs Run 0 misc/cronjobs/overdue_notices.pl -n -csv IMO cronlogaction() (without param) should behave as before. Second consideration: I think it would be better tho have a string as param and to prepend it with caller(0))[1] or whatever it needs to display the script's name. (That is what I was planning as a next step.) With a string param it is up to the logscript's author to decide what to communicate in the logs (in a human readable form), e.g. not only the params but their defaults as well if no params are passed. That brings me to the third consideration: The original idea is that the log is written after all of the preconditions. If the script dies before doing anything "real", no log is written. Otherwise we have an entry in the logs for a cronjob that was called but did not do anything, and users think that everything is OK. With the string param mentioned above the author of the log file then can use logging for more sophisticated things (if necessary), e.g. - cronlogaction("start") - do the preconditions and log some information if it dies: cronlogaction("fail: ..reason...)" - cronlogaction("execute with: ...intersting params / defaults...") - cronlogaction("success") or cronlogaction("success: ...interesting results...") As a minimum, cronlogaction should be called as soon the script does the real work (that's what my patches to the cronjobs do at the moment), in the sense of "Hi, I was here and did something" So I propose to add the string parameter and, for this first step, leave the calls in the cron scripts as is. What do you think? Created attachment 38283 [details] [review] [Follow-up] Provide parameter $infos in cronlogaction() (Alternative for patch Bug 13889: Log with parameters) To test: - Apply patches interactive and skip following patch: Bug 13889: Log with parameters - Tweak a cron job in order to call cronlogaction twice, one with a param at the beginning of the cron ob, e.g. cronlogaction("This is my message") and one without param. - Run this cron job. Result: You should have two entries in the log, one with and one without additional information after the path and name of the calling script. Created attachment 38406 [details] [review] [PASSED QA] Bug 13889 - Add information about cron jobs to system log This patch adds information about cron jobs performed and make it viewable under Home > Tools > Logs ("Browse system logs") To test: Apply patch - Got to system preferences and set 'CronjobLog' to: [Log] information from cron jobs. - Run some cron jobs - Go to Home > Tools > Logs - Verify that you have a selection 'Cron jobs' in drop-down 'Module'. Select it with Action "All" and Submit. - Output should show Date/time and info about Cron jobs Rebased to work on top of Bug 6911 (conflict in viewlog.tt) /MV Rebased after applying patch for Bug 6911 /MV Conflicts resolved: misc/cronjobs/overdue_notices.pl misc/cronjobs/cleanup_database.pl Signed-off-by: Frederic Demians <f.demians@tamil.fr> - Merge both patches, and fix updatedatabase.pl - Works as described. Provide intersting feedback from cronjob scripts. -- Modified version taking in account syspref CronJobLog. Handling simplified by introducing a convenience sub cronlogaction in C4/Log.pm /MV Amended to take in account comments #11, #12, #13 /MV http://bugs.koha-community.org/show_bug.cgi?id=13899 Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38407 [details] [review] [PASSED QA] Bug 13889: Added Test Coverage Previous commit was lacking test coverage of new function. TEST PLAN --------- 1) Apply all the patches 2) prove -v t/db_dependent/Log.t -- should be success. 3) koha qa test tools Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38408 [details] [review] [PASSED QA] Provide parameter $infos in cronlogaction() (Alternative for patch Bug 13889: Log with parameters) To test: - Apply patches interactive and skip following patch: Bug 13889: Log with parameters - Tweak a cron job in order to call cronlogaction twice, one with a param at the beginning of the cron ob, e.g. cronlogaction("This is my message") and one without param. - Run this cron job. Result: You should have two entries in the log, one with and one without additional information after the path and name of the calling script. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 38409 [details] [review] Bug 13889 [QA Followup] - Supress warning, set object column to undef instead of 0 Kyle, Have you seen the discussion before? What's your opinion on it? See from comment 22 Patches pushed to master. Good job Marc! |