Bug 35907 - Add ability to log all custom report runs with or without query
Summary: Add ability to log all custom report runs with or without query
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-01-25 14:59 UTC by Kyle M Hall
Modified: 2024-05-10 14:46 UTC (History)
2 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:
24.05.00


Attachments
Bug 35907: Add ability to log all custom report runs with or without query (5.45 KB, patch)
2024-01-25 15:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35907: Tidy execute_query (4.39 KB, patch)
2024-01-25 15:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35907: Add ability to log all custom report runs with or without query (5.54 KB, patch)
2024-01-25 17:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35907: Tidy execute_query (3.66 KB, patch)
2024-01-25 17:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35907: Add ability to log all custom report runs with or without query (5.43 KB, patch)
2024-01-25 18:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35907: Tidy execute_query (3.66 KB, patch)
2024-01-25 18:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35907: Add ability to log all custom report runs with or without query (5.48 KB, patch)
2024-02-07 14:29 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35907: Tidy execute_query (3.71 KB, patch)
2024-02-07 14:29 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35907: Add ability to log all custom report runs with or without query (5.57 KB, patch)
2024-05-10 08:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35907: Tidy execute_query (3.80 KB, patch)
2024-05-10 08:06 UTC, Marcel de Rooy
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 2024-01-25 14:59:18 UTC
Because of the way Koha::Logger has been used to log to different categories based on the interface and caller, it can be extremely hard to log all of a particular log statement to one place.

For custom report runs, the category is plack-intranet.C4::Reports::Guided when run from the web interface, cron.C4::Reports::Guided when run from runreport.pl, and plack-intranet.C4::Auth when run from svc/report.

We should add a more standardized report run log, both with and without the full query, so that administrators can log all report runs to a centralized location. If an administrator were to need the "point of entry" for reports, it is easy to include via parameters in PatternLayout.
Comment 1 Kyle M Hall 2024-01-25 15:31:12 UTC
Created attachment 161427 [details] [review]
Bug 35907: Add ability to log all custom report runs with or without query

Because of the way Koha::Logger has been used to log to different categories based on the interface and caller, it can be extremely hard to log all of a particular log statement to one place.

For custom report runs, the category is plack-intranet.C4::Reports::Guided when run from the web interface, cron.C4::Reports::Guided when run from runreport.pl, and plack-intranet.C4::Auth when run from svc/report.

We should add a more standardized report run log, both with and without the full query, so that administrators can log all report runs to a centralized location. If an administrator were to need the "point of entry" for reports, it is easy to include via parameters in PatternLayout.

Test Plan:
1) Apply this patch
2) Modify your log4perl file, add the following:

log4perl.logger.reports.execute.time = INFO, REPORTTIME
log4perl.appender.REPORTTIME=Log::Log4perl::Appender::File
log4perl.appender.REPORTTIME.filename=/tmp/report-time.log
log4perl.appender.REPORTTIME.mode=append
log4perl.appender.REPORTTIME.layout=PatternLayout
log4perl.appender.REPORTTIME.layout.ConversionPattern=[%d] [%p] [%P] %m%n
log4perl.appender.REPORTTIME.utf8=1

log4perl.logger.reports.execute.query = INFO, REPORTQUERY
log4perl.appender.REPORTQUERY=Log::Log4perl::Appender::File
log4perl.appender.REPORTQUERY.filename=/tmp/report-query.log
log4perl.appender.REPORTQUERY.mode=append
log4perl.appender.REPORTQUERY.layout=PatternLayout
log4perl.appender.REPORTQUERY.layout.ConversionPattern=[%d] [%p] [%P] %m%n
log4perl.appender.REPORTQUERY.utf8=1

3) Restart all the things!
4) Run a report somehow:
   CLI: ./misc/cronjobs/runreport.pl 1
   API: /cgi-bin/koha/svc/report?id=1
   Web: /cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Run this report
5) Note the report runs are logged to /tmp/report-time.log and /tmp/report-query.log
Comment 2 Kyle M Hall 2024-01-25 15:31:22 UTC
Created attachment 161429 [details] [review]
Bug 35907: Tidy execute_query
Comment 3 Kyle M Hall 2024-01-25 17:06:34 UTC
Created attachment 161452 [details] [review]
Bug 35907: Add ability to log all custom report runs with or without query

Because of the way Koha::Logger has been used to log to different categories based on the interface and caller, it can be extremely hard to log all of a particular log statement to one place.

For custom report runs, the category is plack-intranet.C4::Reports::Guided when run from the web interface, cron.C4::Reports::Guided when run from runreport.pl, and plack-intranet.C4::Auth when run from svc/report.

We should add a more standardized report run log, both with and without the full query, so that administrators can log all report runs to a centralized location. If an administrator were to need the "point of entry" for reports, it is easy to include via parameters in PatternLayout.

Test Plan:
1) Apply this patch
2) Modify your log4perl file, add the following:

log4perl.logger.reports.execute.time = INFO, REPORTTIME
log4perl.appender.REPORTTIME=Log::Log4perl::Appender::File
log4perl.appender.REPORTTIME.filename=/tmp/report-time.log
log4perl.appender.REPORTTIME.mode=append
log4perl.appender.REPORTTIME.layout=PatternLayout
log4perl.appender.REPORTTIME.layout.ConversionPattern=[%d] [%p] [%P] %m%n
log4perl.appender.REPORTTIME.utf8=1

log4perl.logger.reports.execute.query = INFO, REPORTQUERY
log4perl.appender.REPORTQUERY=Log::Log4perl::Appender::File
log4perl.appender.REPORTQUERY.filename=/tmp/report-query.log
log4perl.appender.REPORTQUERY.mode=append
log4perl.appender.REPORTQUERY.layout=PatternLayout
log4perl.appender.REPORTQUERY.layout.ConversionPattern=[%d] [%p] [%P] %m%n
log4perl.appender.REPORTQUERY.utf8=1

3) Restart all the things!
4) Run a report somehow:
   CLI: ./misc/cronjobs/runreport.pl 1
   API: /cgi-bin/koha/svc/report?id=1
   Web: /cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Run this report
5) Note the report runs are logged to /tmp/report-time.log and /tmp/report-query.log
Comment 4 Kyle M Hall 2024-01-25 17:06:40 UTC
Created attachment 161453 [details] [review]
Bug 35907: Tidy execute_query
Comment 5 Kyle M Hall 2024-01-25 18:38:18 UTC
Created attachment 161458 [details] [review]
Bug 35907: Add ability to log all custom report runs with or without query

Because of the way Koha::Logger has been used to log to different categories based on the interface and caller, it can be extremely hard to log all of a particular log statement to one place.

For custom report runs, the category is plack-intranet.C4::Reports::Guided when run from the web interface, cron.C4::Reports::Guided when run from runreport.pl, and plack-intranet.C4::Auth when run from svc/report.

We should add a more standardized report run log, both with and without the full query, so that administrators can log all report runs to a centralized location. If an administrator were to need the "point of entry" for reports, it is easy to include via parameters in PatternLayout.

Test Plan:
1) Apply this patch
2) Modify your log4perl file, add the following:

log4perl.logger.reports.execute.time = INFO, REPORTTIME
log4perl.appender.REPORTTIME=Log::Log4perl::Appender::File
log4perl.appender.REPORTTIME.filename=/tmp/report-time.log
log4perl.appender.REPORTTIME.mode=append
log4perl.appender.REPORTTIME.layout=PatternLayout
log4perl.appender.REPORTTIME.layout.ConversionPattern=[%d] [%p] [%P] %m%n
log4perl.appender.REPORTTIME.utf8=1

log4perl.logger.reports.execute.query = INFO, REPORTQUERY
log4perl.appender.REPORTQUERY=Log::Log4perl::Appender::File
log4perl.appender.REPORTQUERY.filename=/tmp/report-query.log
log4perl.appender.REPORTQUERY.mode=append
log4perl.appender.REPORTQUERY.layout=PatternLayout
log4perl.appender.REPORTQUERY.layout.ConversionPattern=[%d] [%p] [%P] %m%n
log4perl.appender.REPORTQUERY.utf8=1

3) Restart all the things!
4) Run a report somehow:
   CLI: ./misc/cronjobs/runreport.pl 1
   API: /cgi-bin/koha/svc/report?id=1
   Web: /cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Run this report
5) Note the report runs are logged to /tmp/report-time.log and /tmp/report-query.log
Comment 6 Kyle M Hall 2024-01-25 18:38:24 UTC
Created attachment 161459 [details] [review]
Bug 35907: Tidy execute_query
Comment 7 Brendan Lawlor 2024-02-06 20:04:07 UTC
I tried testing this but couldn't find the log files /tmp/report-time.log and /tmp/report-query.log 

I may be missing a step.
1. Applied the patch
2. Added the provided config to /koha/etc/log4perl.conf
3. restart_all
4. Set all Logging system prefs to 'Log'
5. Ran some reports
6. Check /tmp for report-time.log and report-query.log

There were files in /tmp but none name like that.
Comment 8 Kyle M Hall 2024-02-07 11:50:32 UTC
(In reply to Brendan Lawlor from comment #7)
> I tried testing this but couldn't find the log files /tmp/report-time.log
> and /tmp/report-query.log 
> 
> I may be missing a step.
> 1. Applied the patch
> 2. Added the provided config to /koha/etc/log4perl.conf
> 3. restart_all
> 4. Set all Logging system prefs to 'Log'
> 5. Ran some reports
> 6. Check /tmp for report-time.log and report-query.log
> 
> There were files in /tmp but none name like that.

I just ran through the test plan and I can confirm it's still working as intended! Here is a video demonstration using runreport.pl: https://monosnap.com/file/CcQHcG8kTlJhvOpXxOITdWc5wj5eea
Comment 9 Brendan Lawlor 2024-02-07 14:29:41 UTC
Created attachment 161806 [details] [review]
Bug 35907: Add ability to log all custom report runs with or without query

Because of the way Koha::Logger has been used to log to different categories based on the interface and caller, it can be extremely hard to log all of a particular log statement to one place.

For custom report runs, the category is plack-intranet.C4::Reports::Guided when run from the web interface, cron.C4::Reports::Guided when run from runreport.pl, and plack-intranet.C4::Auth when run from svc/report.

We should add a more standardized report run log, both with and without the full query, so that administrators can log all report runs to a centralized location. If an administrator were to need the "point of entry" for reports, it is easy to include via parameters in PatternLayout.

Test Plan:
1) Apply this patch
2) Modify your log4perl file, add the following:

log4perl.logger.reports.execute.time = INFO, REPORTTIME
log4perl.appender.REPORTTIME=Log::Log4perl::Appender::File
log4perl.appender.REPORTTIME.filename=/tmp/report-time.log
log4perl.appender.REPORTTIME.mode=append
log4perl.appender.REPORTTIME.layout=PatternLayout
log4perl.appender.REPORTTIME.layout.ConversionPattern=[%d] [%p] [%P] %m%n
log4perl.appender.REPORTTIME.utf8=1

log4perl.logger.reports.execute.query = INFO, REPORTQUERY
log4perl.appender.REPORTQUERY=Log::Log4perl::Appender::File
log4perl.appender.REPORTQUERY.filename=/tmp/report-query.log
log4perl.appender.REPORTQUERY.mode=append
log4perl.appender.REPORTQUERY.layout=PatternLayout
log4perl.appender.REPORTQUERY.layout.ConversionPattern=[%d] [%p] [%P] %m%n
log4perl.appender.REPORTQUERY.utf8=1

3) Restart all the things!
4) Run a report somehow:
   CLI: ./misc/cronjobs/runreport.pl 1
   API: /cgi-bin/koha/svc/report?id=1
   Web: /cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Run this report
5) Note the report runs are logged to /tmp/report-time.log and /tmp/report-query.log

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 10 Brendan Lawlor 2024-02-07 14:29:43 UTC
Created attachment 161807 [details] [review]
Bug 35907: Tidy execute_query

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 11 Brendan Lawlor 2024-02-07 14:36:40 UTC
It turned out I didn't edit the correct config file in my first test.

I edited /kohadevbox/koha/etc/log4perl.conf
I guess maybe that is just an example config file. 

Thanks for showing me where the real config file is /etc/koha/sites/kohadev/log4perl.conf
Comment 12 Kyle M Hall 2024-02-08 18:54:17 UTC
(In reply to Brendan Lawlor from comment #11)
> It turned out I didn't edit the correct config file in my first test.
> 
> I edited /kohadevbox/koha/etc/log4perl.conf
> I guess maybe that is just an example config file. 
> 
> Thanks for showing me where the real config file is
> /etc/koha/sites/kohadev/log4perl.conf

Ack! Thanks for the signoff Brendan!
Comment 13 Marcel de Rooy 2024-05-10 08:06:43 UTC
Created attachment 166501 [details] [review]
Bug 35907: Add ability to log all custom report runs with or without query

Because of the way Koha::Logger has been used to log to different categories based on the interface and caller, it can be extremely hard to log all of a particular log statement to one place.

For custom report runs, the category is plack-intranet.C4::Reports::Guided when run from the web interface, cron.C4::Reports::Guided when run from runreport.pl, and plack-intranet.C4::Auth when run from svc/report.

We should add a more standardized report run log, both with and without the full query, so that administrators can log all report runs to a centralized location. If an administrator were to need the "point of entry" for reports, it is easy to include via parameters in PatternLayout.

Test Plan:
1) Apply this patch
2) Modify your log4perl file, add the following:

log4perl.logger.reports.execute.time = INFO, REPORTTIME
log4perl.appender.REPORTTIME=Log::Log4perl::Appender::File
log4perl.appender.REPORTTIME.filename=/tmp/report-time.log
log4perl.appender.REPORTTIME.mode=append
log4perl.appender.REPORTTIME.layout=PatternLayout
log4perl.appender.REPORTTIME.layout.ConversionPattern=[%d] [%p] [%P] %m%n
log4perl.appender.REPORTTIME.utf8=1

log4perl.logger.reports.execute.query = INFO, REPORTQUERY
log4perl.appender.REPORTQUERY=Log::Log4perl::Appender::File
log4perl.appender.REPORTQUERY.filename=/tmp/report-query.log
log4perl.appender.REPORTQUERY.mode=append
log4perl.appender.REPORTQUERY.layout=PatternLayout
log4perl.appender.REPORTQUERY.layout.ConversionPattern=[%d] [%p] [%P] %m%n
log4perl.appender.REPORTQUERY.utf8=1

3) Restart all the things!
4) Run a report somehow:
   CLI: ./misc/cronjobs/runreport.pl 1
   API: /cgi-bin/koha/svc/report?id=1
   Web: /cgi-bin/koha/reports/guided_reports.pl?reports=1&phase=Run this report
5) Note the report runs are logged to /tmp/report-time.log and /tmp/report-query.log

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2024-05-10 08:06:46 UTC
Created attachment 166502 [details] [review]
Bug 35907: Tidy execute_query

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2024-05-10 08:07:00 UTC
(In reply to Kyle M Hall from comment #0)
> Because of the way Koha::Logger has been used to log to different categories
> based on the interface and caller, it can be extremely hard to log all of a
> particular log statement to one place.

Just change your log4perl.conf configuration? And a sysadmin could always concatenate all logs and grep on whatever he wants to find?

This works as expected but feels a bit like a workaround.

+    my $prefix = $params->{prefix} // 1;
I would rather rename this one personally. What you want here is to suppress the plack prefix, not add a custom prefix.
no_plack => 1 or something like that ?

If the log4perl config for reports.execute.query is not found, it seems that Logger just silently ignores logging. This costed me about 15ms per call. Probably 
 acceptable.

Passing QA since it is just a few lines. But hesitantly since our code doesnt get cleaner.
Comment 16 Katrin Fischer 2024-05-10 14:46:58 UTC
Pushed for 24.05!

Well done everyone, thank you!