Bug 8190 - Add a logging module to Koha
Summary: Add a logging module to Koha
Status: RESOLVED DUPLICATE of bug 14167
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-05 13:52 UTC by Jonathan Druart
Modified: 2015-07-21 14:58 UTC (History)
12 users (show)

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


Attachments
Bug 8190: C4::Logger, Logging module (12.60 KB, patch)
2012-06-05 13:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (12.70 KB, patch)
2012-07-19 08:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: C4::Logger, Logging module (13.20 KB, patch)
2012-09-15 05:44 UTC, Mason James
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (13.18 KB, patch)
2012-09-17 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Followup Logger: FIX perlcritic (1.86 KB, patch)
2012-09-17 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Followup Logger: FIX perlcritic (1.97 KB, patch)
2012-09-17 10:42 UTC, Mason James
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (13.29 KB, patch)
2012-09-17 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190: Followup Logger: FIX perlcritic (2.00 KB, patch)
2012-09-17 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190 - Followup - Add cached logger, output messages to template (8.76 KB, patch)
2012-09-17 15:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190 - Followup - Add cached logger, output messages to template (12.29 KB, patch)
2012-09-17 17:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8190: Koha::Utils::Logger, Logging module (13.30 KB, patch)
2012-10-09 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8190: Followup Logger: FIX perlcritic (2.00 KB, patch)
2012-10-09 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190 - Followup - Add cached logger, output messages to template (12.38 KB, patch)
2012-10-09 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190 - Followup - Add cached logger, output messages to template (11.77 KB, patch)
2012-10-09 13:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (18.45 KB, patch)
2012-11-28 16:41 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (18.75 KB, patch)
2012-11-28 16:43 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8190: Follow Add some POD (6.14 KB, patch)
2012-11-29 13:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Follow Add some POD (6.14 KB, patch)
2012-11-29 13:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (18.84 KB, patch)
2012-12-07 14:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190: Follow Add some POD (6.19 KB, patch)
2012-12-07 14:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190: raise an error if the Log::LogLite module is not installed (1.00 KB, patch)
2012-12-27 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: FIX Unit test (1.12 KB, patch)
2012-12-27 14:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (18.88 KB, patch)
2012-12-28 19:25 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8190: Follow Add some POD (6.19 KB, patch)
2012-12-28 19:29 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8190: raise an error if the Log::LogLite module is not installed (1.00 KB, patch)
2012-12-28 19:29 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8190: FIX Unit test (1.12 KB, patch)
2012-12-28 19:29 UTC, Chris Cormack
Details | Diff | Splinter Review
DO NOT USE - Partial rewrite of Logger (8.95 KB, patch)
2012-12-28 22:51 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 8190: QA Followup (5.52 KB, patch)
2013-01-03 11:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: raise an error if the Log::LogLite module is not installed (1.06 KB, patch)
2013-01-18 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190: FIX Unit test (1.17 KB, patch)
2013-01-18 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190: QA Followup (5.57 KB, patch)
2013-01-18 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (18.78 KB, patch)
2013-03-19 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Follow Add some POD (6.19 KB, patch)
2013-03-19 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: raise an error if the Log::LogLite module is not installed (1.06 KB, patch)
2013-03-19 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: FIX Unit test (1.17 KB, patch)
2013-03-19 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: QA Followup (5.58 KB, patch)
2013-03-19 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (18.66 KB, patch)
2013-06-10 07:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: Follow Add some POD (6.23 KB, patch)
2013-06-10 07:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: raise an error if the Log::LogLite module is not installed (1.10 KB, patch)
2013-06-10 07:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: FIX Unit test (1.21 KB, patch)
2013-06-10 07:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: QA Followup (5.61 KB, patch)
2013-06-10 07:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190 - Add a logging module to Koha - Followup - Add html embedded logs to OPAC (2.73 KB, patch)
2013-06-10 12:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8190: Add the 'alert' level. (1.79 KB, patch)
2013-06-10 12:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (18.72 KB, patch)
2013-06-12 11:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: Follow Add some POD (6.29 KB, patch)
2013-06-12 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: raise an error if the Log::LogLite module is not installed (1.16 KB, patch)
2013-06-12 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: FIX Unit test (1.26 KB, patch)
2013-06-12 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: QA Followup (5.67 KB, patch)
2013-06-12 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190 - Add a logging module to Koha - Followup - Add html embedded logs to OPAC Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (2.83 KB, patch)
2013-06-12 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: Add the 'alert' level. (1.86 KB, patch)
2013-06-12 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: Followup for opac-search (1.70 KB, patch)
2013-06-12 11:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (18.00 KB, patch)
2013-06-12 12:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: Follow Add some POD (6.29 KB, patch)
2013-06-12 12:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: raise an error if the Log::LogLite module is not installed (1.16 KB, patch)
2013-06-12 12:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: FIX Unit test (1.26 KB, patch)
2013-06-12 12:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: QA Followup (5.67 KB, patch)
2013-06-12 12:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: Followup - Add html embedded logs to OPAC (2.79 KB, patch)
2013-06-12 12:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: Add the 'alert' level. (1.86 KB, patch)
2013-06-12 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: Followup for opac-search (1.70 KB, patch)
2013-06-12 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8190: FIX description and values for the LogLevel pref (2.78 KB, patch)
2013-06-12 13:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Koha::Utils::Logger, Logging module (18.44 KB, patch)
2014-12-31 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Follow Add some POD (6.25 KB, patch)
2014-12-31 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: raise an error if the Log::LogLite module is not installed (1.11 KB, patch)
2014-12-31 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: FIX Unit test (1.22 KB, patch)
2014-12-31 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: QA Followup (5.64 KB, patch)
2014-12-31 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Followup - Add html embedded logs to OPAC (3.01 KB, patch)
2014-12-31 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Add the 'alert' level. (1.84 KB, patch)
2014-12-31 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: Followup for opac-search (1.70 KB, patch)
2014-12-31 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8190: FIX description and values for the LogLevel pref (3.21 KB, patch)
2014-12-31 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2012-06-05 13:52:17 UTC
I would be great to specify a log level (to differentiate production and development environment for example).
Comment 1 Jonathan Druart 2012-06-05 13:55:48 UTC Comment hidden (obsolete)
Comment 2 Dobrica Pavlinusic 2012-06-05 15:01:09 UTC
Should we also wrap warn(s) to emit debug output like this?

$SIG{__WARN__} = sub {
        C4::Logger->new->debug( @_ );
}       

Right now every warn in code makes patch fail QA (I know that first 
hand ;-) and it seems to me it's easier to just put warn instead of

C4::Logger->new->debug( "this used to be warn" );
Comment 3 Jared Camins-Esakov 2012-06-15 17:33:18 UTC
I had a thought about this. Would it be possible to have the logging module serve dual purposes, for both debug logging and for action_logs logging?
Comment 4 Chris Cormack 2012-06-15 19:57:32 UTC
Perhaps also take a look at Log4Perl 
http://search.cpan.org/~mschilli/Log-Log4perl-1.37/lib/Log/Log4perl.pm
Comment 5 Jonathan Druart 2012-07-19 08:13:40 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2012-07-19 08:20:38 UTC
(In reply to comment #2)
> Right now every warn in code makes patch fail QA (I know that first 
> hand ;-) and it seems to me it's easier to just put warn instead of
> 
> C4::Logger->new->debug( "this used to be warn" );

Just $log->debug("my debug message");
In fact, a lot of warn in Koha are not a debug message. I think each warn have to be replaced with his right level.
But your are right, we can have a thought about this.
(A simple method is to map into your favorite editor the string "warn" with a call to the debug method of Logger ;)


(In reply to comment #3)
> I had a thought about this. Would it be possible to have the logging module
> serve dual purposes, for both debug logging and for action_logs logging?
Hum, I had already this idea and I think it is not a good one :)
First, it would take a lot of disk space. To process the results, we will have to grep a huge number of lines of data.
Then, we oblige users to keep all of their Koha log. We will depend of an other source of data (currently, we only have the DB).
Imo processing time would be very increased.

(In reply to comment #4)
> Perhaps also take a look at Log4Perl 
> http://search.cpan.org/~mschilli/Log-Log4perl-1.37/lib/Log/Log4perl.pm
I had a look at several perl logging modules. My conclusion is LogLite is the easier for what we need into Koha.
In any case, since we have a Koha::Utils::Logger used in Koha, it is easy to replace this module with another ;)
Comment 7 Mason James 2012-09-15 05:44:45 UTC Comment hidden (obsolete)
Comment 8 Mason James 2012-09-15 05:48:48 UTC
> Signed-off-by: Mason James <mtj@kohaaloha.com>

1/installed loglite package
 $ sudo apt-get install liblog-loglite-perl 

2/ ran test successfully
 $ LOG="/tmp/t1.log" perl  t/Logger.t
1..15
ok 1 - use Koha::Utils::Logger;
ok 2 - Check LogLevel syspref
ok 3 - Check $log is undef
ok 4 - Check $log is not undef
ok 5 - check error string with level 3
ok 6 - check normal string with level 3
ok 7 - check error string with level 5
ok 8 - check normal string with level 5
ok 9 - check calledby string with level 5
ok 10 - check WARN string with dump
ok 11 - check VAR1 string with dump
ok 12 - check values aaaaa string with dump
ok 13 - check log level return
ok 14 - check error string with level 3
ok 15 - check normal string with level 3

Jonathon. looks good, signing-off...
Comment 9 Mason James 2012-09-15 05:51:59 UTC
> Jonathan. looks good, signing-off...

but, perlcritic errors...

Koha/Utils/Logger.pm                                                     FAIL
 Loop iterator is not lexical at line 168, column 5. See page 108 of PBP.
t/Logger.t                                                               FAIL
 Two-argument "open" used at line 56, column 1. See page 207 of PBP.
 Bareword file handle opened at line 80, column 5. See pages 202,204 of PBP.
 Bareword file handle opened at line 69, column 5. See pages 202,204 of PBP.
Comment 10 Mason James 2012-09-15 06:12:19 UTC
> Prerequisite:
> - set an environment variable LOG in your virtual host:
>     SetEnv LOG /home/koha/var/log/opac.log
> - set a write flag for www-data on this file

hmm, just a little suggestion, too...

change SetEnv from LOG, to KOHA_LOG?
Comment 11 Jonathan Druart 2012-09-17 09:29:36 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2012-09-17 09:29:46 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2012-09-17 09:30:55 UTC
First patch replace LOG with KOHA_LOG
Second patch fixes perlcritic errors
Comment 14 Mason James 2012-09-17 10:42:15 UTC Comment hidden (obsolete)
Comment 15 Mason James 2012-09-17 10:43:28 UTC
(In reply to comment #9)
> > Jonathan. looks good, signing-off...
> 
> but, perlcritic errors...

errors fixed, signing-off
Comment 16 Kyle M Hall 2012-09-17 15:55:37 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2012-09-17 15:55:48 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2012-09-17 15:56:06 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2012-09-17 15:58:56 UTC
(In reply to comment #18)
> Created attachment 12284 [details] [review]
> Bug 8190 - Followup - Add cached logger, output messages to template
> 
> * Add C4::Context->logger
> * Embed logged messageds to a comment in the template html

I forget to add that I ran perltidy on Logger.pm. I think this followup could be very useful. It may be better to have it controlled by a system preference, rather than always on though.
Comment 20 Kyle M Hall 2012-09-17 17:54:35 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2012-09-18 08:17:21 UTC
Hi Kyle,
Thanks for your followup !
2 small remarks about your patch:
* you commited mainpage.pl unfortunately
* the added block of code in Logger::write is not perltidied

Else all seems good but we have to pull together on how we will use the logger.
Your idea to add a global var in C4::Context is what I had at the beginning of my work. But after thinking, on this way, we canot change the loglevel for 1 script ! And it is a regression (indeed currently we can do that, changing the DEBUG var).

My idea was to add 
  use C4::Logger qw/$log/;
  $log = C4::Logger->new({level => C4::Context->preference("LogLevel")});
on each page. Like that we could change the log level easily.
Comment 22 Jared Camins-Esakov 2012-10-06 04:16:16 UTC
Changing target version so this bug report will show up in the list of 3.12-targeted features and doesn't get lost in the shuffle during feature freeze.
Comment 23 Kyle M Hall 2012-10-09 13:11:22 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2012-10-09 13:11:40 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2012-10-09 13:11:58 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2012-10-09 13:19:39 UTC
(In reply to comment #21)
> Hi Kyle,
> Thanks for your followup !
> 2 small remarks about your patch:
> * you commited mainpage.pl unfortunately
Fixed!

> * the added block of code in Logger::write is not perltidied
Fixed!
 
> Else all seems good but we have to pull together on how we will use the
> logger.

I think it would be simple enough to replace all existing warns with the logger. I can volunteer some time to do this.

> Your idea to add a global var in C4::Context is what I had at the beginning
> of my work. But after thinking, on this way, we canot change the loglevel
> for 1 script ! And it is a regression (indeed currently we can do that,
> changing the DEBUG var).

Would it be possible to add support for the DEBUG variable to the logger? 

> My idea was to add 
>   use C4::Logger qw/$log/;
>   $log = C4::Logger->new({level => C4::Context->preference("LogLevel")});
> on each page. Like that we could change the log level easily.

That would work, but it would be nice to follow the DRY principle. However, if you think it is not possible to place it in C4::Context, then I will believe you.
Comment 27 Kyle M Hall 2012-10-09 13:34:18 UTC Comment hidden (obsolete)
Comment 28 Paul Poulain 2012-11-28 16:40:59 UTC
QA comment:
 * I was enthusiastic about setting this patch to passed QA, because that's a large enhancement.
 * passes koha-qa.pl, except for a warning about merge marker in t/Logger.t that is irrelevant
 * the new Koha/Utils/Logger.pm has no PODDOC at all. there must be one (that should be easy to write)
 * I really love the  LogToHtmlComments addition, it will be very userful
 * i've updated the bug title (for future 3.12 release notes)

Please provide a follow-up to my signed-off & squashed patch with a POD and i'll be more than happy to pass QA on it !
Comment 29 Paul Poulain 2012-11-28 16:41:21 UTC Comment hidden (obsolete)
Comment 30 Paul Poulain 2012-11-28 16:43:08 UTC Comment hidden (obsolete)
Comment 31 Paul Poulain 2012-11-28 16:44:07 UTC
(In reply to comment #30)
Forget patch updated in comment 29, I forgot to squash the updatedatabase changes:
> Bug 8190 follow-up moving updatedatabase part at the right place

the comment 30 patch is OK !
Comment 32 Jonathan Druart 2012-11-29 13:04:14 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2012-11-29 13:19:19 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2012-12-07 14:31:53 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2012-12-07 14:32:12 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2012-12-27 13:10:46 UTC
I have started testing this. I notice, that when you have liblog-loglite-perl not installed, Koha is unusable. And instead of giving you an error indicating what's going wrong, it explodes with:

Internal Server Error

The server encountered an internal error or misconfiguration and was unable to complete your request.

I think this will cause a lot of questions - can we make this fail a bit more gracefully?
Comment 37 Jonathan Druart 2012-12-27 13:24:59 UTC
(In reply to comment #36)
> I have started testing this. I notice, that when you have
> liblog-loglite-perl not installed, Koha is unusable. And instead of giving
> you an error indicating what's going wrong, it explodes with:
> 
> Internal Server Error
> 
> The server encountered an internal error or misconfiguration and was unable
> to complete your request.
> 
> I think this will cause a lot of questions - can we make this fail a bit
> more gracefully?

I think this is the case for all non installed package.
Comment 38 Katrin Fischer 2012-12-27 13:35:40 UTC
Often the perl error message is more helpful, indicating that there is a missing dependency. I think the way this explodes now is going to make us answer lots of questions.
Comment 39 Jonathan Druart 2012-12-27 14:11:22 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2012-12-27 14:12:59 UTC
The patch displays a specific message if the module Koha::Utils::Logger raises an error.
It is not a clean way but I don't know how to do this differently.
Comment 41 Katrin Fischer 2012-12-27 14:27:59 UTC
Hi Jonathan,

thx for looking into this.

I have continued testing and found another problem. When you don't have the new environment variable set, the test suite fails. The tests need to pass without additional 'developer configuration'.

Can you please change the test? Maybe you can set the environment variable in the test and use a writeable temporary file.

I am going set 'Failed QA' for this reason - please switch back to Signed Off when the tests pass. 

There is also a little typo (Logguer) when you run the test with prove t/Logger.t:
| You must call this test with a KOHA_LOG env var like: |
| KOHA_LOG="/tmp/t1.log" prove t/Logguer.t      |

I was also wondering about the 7 levels - do we have some more documentation how the different levels are supposed to be used? Maybe this is a standard Perl/Logging thing I am not aware of, but I think some more pointers in the POD or on a wiki page would be really helpful before we start using this all over Koha.
Comment 42 Jonathan Druart 2012-12-27 14:38:22 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2012-12-27 14:54:21 UTC
(In reply to comment #41)
> I was also wondering about the 7 levels - do we have some more documentation
> how the different levels are supposed to be used? Maybe this is a standard
> Perl/Logging thing I am not aware of, but I think some more pointers in the
> POD or on a wiki page would be really helpful before we start using this all
> over Koha.

It is not easy to deal correctly with each level.
debug, warning and error are easy. But how to differentiate an error from a critical message?
The choice is made from personal point of view and can vary for each kind of bug. It is not easy to do an exhaustive wiki page for such stuff.
Comment 44 Jonathan Druart 2012-12-27 15:28:53 UTC
A quick start:
http://wiki.koha-community.org/wiki/Logger
Comment 45 Katrin Fischer 2012-12-27 15:31:33 UTC
Hi Jonathan, thx for your work - still testing. :)
Comment 46 Jared Camins-Esakov 2012-12-27 15:32:09 UTC
(In reply to comment #44)
> A quick start:
> http://wiki.koha-community.org/wiki/Logger

It seems to me that ->debug calls *should* be included in code committed to Master. Isn't the entire point of the logger that it's possible to change your log level so that you can get all the debug information/all the warnings/info and errors/just errors/just critical errors?
Comment 47 Jonathan Druart 2012-12-27 16:21:23 UTC
(In reply to comment #46)
> (In reply to comment #44)
> > A quick start:
> > http://wiki.koha-community.org/wiki/Logger
> 
> It seems to me that ->debug calls *should* be included in code committed to
> Master. Isn't the entire point of the logger that it's possible to change
> your log level so that you can get all the debug information/all the
> warnings/info and errors/just errors/just critical errors?

Yes maybe. But developpers must be aware that too many debug message are not relevant because logs would become too big and then unreadable.
Comment 48 Katrin Fischer 2012-12-27 17:29:23 UTC
I can't get debug to show up. My LogLevel is set to the highest level 7. I verified this is also the value in the systempreferences table.

I added following code to the mainpage.pl file:
use Koha::Utils::Logger qw/$log/;
$log = Koha::Utils::Logger->new;

$log->unusable("This is an unusable thing");
$log->critical("This is critical!");
$log->error("This is an error !");
$log->warning("This is only a warning.");
$log->normal("This nothing to worry about.");
$log->info("This is an information");
$log->debug("This is a debug message");

With LogToHtmlComments set to Embed I can see following in the HTML source code of the main page:

<!-- LOG MESSAGES
[Thu Dec 27 17:54:44 2012] UNUS  : This is an unusable thing
[Thu Dec 27 17:54:44 2012] CRIT  : This is critical!
[Thu Dec 27 17:54:44 2012] ERROR : This is an error !
[Thu Dec 27 17:54:44 2012] WARN  : This is only a warning.
[Thu Dec 27 17:54:44 2012] NORMAL: This nothing to worry about.
[Thu Dec 27 17:54:44 2012] INFO  : This is an information

-->

My log files shows:
[27/12/2012 17:54:44] UNUS  : This is an unusable thing
[27/12/2012 17:54:44] CRIT  : This is critical!
[27/12/2012 17:54:44] ERROR : This is an error !
[27/12/2012 17:54:44] WARN  : This is only a warning.
[27/12/2012 17:54:44] NORMAL: This nothing to worry about.
[27/12/2012 17:54:44] INFO  : This is an information

My test plan so far was:

1) Apply patches and try to access Koha.
  Verify useful error message is shown.
2) Install new dependency: Log::LogLite
  sudo apt-get install liblog-loglite-perl
3) Run database update
  Verify database update works correctly
4) Run t/Logger.t
  Verify all tests pass
5) Do some searches in OPAC.
  Verify Koha does not explode and all is acting normal.
  Verify debug message is not showing up in HTML source.
  Look for <!-- LOG MESSAGES
  Verify nothing has changed and no messages show up.
6) Set environment variables in your Apache configuration files
  I added following line to the OPAC and Intranet <virtualhost> sections:
  SetEnv KOHA_LOG "/home/<user>/koha-dev/var/log/opac.log"
  SetEnv KOHA_LOG "/home/<user>/koha-dev/var/log/intranet.log"
  Make sure log file is writeable for your Apache user.
7) Repeat 5).
  Check that the searches show up in the OPAC log file.
8) Change one of the intranet files and add some messages/warns etc. 
  to be logged.
9) Set LogToHtmlComments to 'Embed' and check output in log file
  and for the debug messages in the source code.
  Looks for <!-- LOG MESSAGES

ALSO: With bug 7167 pushed, the update database part on this should be rewritten.
Comment 49 Jonathan Druart 2012-12-28 12:36:45 UTC
Hi Katrin,

Yes it is normal.
Try with : 
  my $log = C4::Context->logger;

the constructor Koha::Utils::Logger->new takes a level parameter. If it does not exist, the normal level is used.

C4::Context->logger uses the level from the syspref LogLevel.
Comment 50 Chris Cormack 2012-12-28 19:25:34 UTC Comment hidden (obsolete)
Comment 51 Chris Cormack 2012-12-28 19:29:10 UTC Comment hidden (obsolete)
Comment 52 Chris Cormack 2012-12-28 19:29:23 UTC Comment hidden (obsolete)
Comment 53 Chris Cormack 2012-12-28 19:29:36 UTC Comment hidden (obsolete)
Comment 54 Chris Cormack 2012-12-28 19:32:37 UTC
Patches rebased so they all apply on master.

Tests pass. However it is currently not plack safe, follow up to come.
Comment 55 Chris Cormack 2012-12-28 22:50:52 UTC
I started trying to tidy it, but didn't get super far, attached is the patch, for interest sake.

I might try another approach if time allows.
Comment 56 Chris Cormack 2012-12-28 22:51:26 UTC Comment hidden (obsolete)
Comment 57 Jared Camins-Esakov 2012-12-29 03:08:58 UTC
I think I favor Log4Perl over LogLite, since it allows us to cut down dramatically on the amount of code that we are maintaining. However, I do not have a strong opinion on that.

There are five issues that absolutely must be fixed:
1) The ->debug() level needs to work.
2) Private variable need to be eliminated from the module level and the code made thread-safe.
3) The unit test must be changed to not require the database.
4) The $log package variable should not be in Koha::Utils::Logger. Any static configuration should be handled entirely in C4::Context.
5) If Koha::Utils::Logger is object-oriented, it must not be an Exporter.
Comment 58 Chris Cormack 2012-12-29 07:15:45 UTC
If we are going to always access the logger object via C4::Context, we could do this all without the need of a new module.

if in C4::Context we added something very similar to

use Log::Log4perl qw(:levels get_logger);

my $layout_class   = "Log::Log4perl::Layout::PatternLayout";
my $layout_pattern = "[%d] %p - %M - %m - %T%n";

my $config = <<"END";
      log4perl.logger = INFO, Logfile, Screen
      
      log4perl.appender.Logfile = Log::Log4perl::Appender::File
      log4perl.appender.Logfile.filename = $ENV{KOHA_LOG}
      log4perl.appender.Logfile.mode=append
      log4perl.appender.Logfile.layout = $layout_class
      log4perl.appender.Logfile.layout.ConversionPattern=$layout_pattern
      log4perl.appender.Screen  = Log::Log4perl::Appender::Screen
      log4perl.appender.Screen.layout = $layout_class
      log4perl.appender.Screen.layout.ConversionPattern = $layout_pattern
END

Log::Log4perl->init( \$config );

and then in there a sub

sub logger
{
    my $self = shift;

    if ( defined( $context->{"logger"} ) ) {
        return $context->{"logger"};
    }

    $context->{"logger"} = Log::Log4perl->get_logger('C4::Context');
    $context->{"logger"}->level(C4::Context->preference("LogLevel"));

    return $context->{"logger"};
}

We would have a logger logging to stderr and to the file defined. We would need to change our numeric loglevels to strings, like FATAL, WARN etc but that is easily done. And if we did want our 2 extra ones, we can add these 2 lines

 # set up our custom log leves CRIT and NORMAL

        Log::Log4perl::Logger::create_custom_level( 'CRIT', 'FATAL');
        Log::Log4perl::Logger::create_custom_level( 'NORMAL', 'WARN' );

Before we do the init.
Comment 59 Jared Camins-Esakov 2012-12-29 13:32:42 UTC
(In reply to comment #58)
> If we are going to always access the logger object via C4::Context, we could
> do this all without the need of a new module.

That seems reasonable to me. Is there a downside to that method of handling the logging?
Comment 60 Chris Cormack 2012-12-29 18:22:23 UTC
(In reply to comment #59)
> (In reply to comment #58)
> > If we are going to always access the logger object via C4::Context, we could
> > do this all without the need of a new module.
> 
> That seems reasonable to me. Is there a downside to that method of handling
> the logging?

Hmm I think when working with Koha you always need a context object, so that having a logging object seems good to me. If you did want to log without context (no access to db or conf)  you would have to make your own object. 
If you want to provide a dump mechanism like the original patch you can define a filter in the configuration to do that. 

I may have missed something though
Comment 61 Jonathan Druart 2013-01-03 11:47:48 UTC Comment hidden (obsolete)
Comment 62 Jonathan Druart 2013-01-03 11:48:20 UTC
(In reply to comment #57)
> There are five issues that absolutely must be fixed:
> 1) The ->debug() level needs to work.

It has always worked, see comment #49

> 2) Private variable need to be eliminated from the module level and the code
> made thread-safe.

Done, tested with Plack

> 3) The unit test must be changed to not require the database.

Done, removed the C4::Context requirement

> 4) The $log package variable should not be in Koha::Utils::Logger. Any
> static configuration should be handled entirely in C4::Context.
> 5) If Koha::Utils::Logger is object-oriented, it must not be an Exporter.

Done
Comment 63 Kyle M Hall 2013-01-18 15:29:12 UTC Comment hidden (obsolete)
Comment 64 Kyle M Hall 2013-01-18 15:29:29 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2013-01-18 15:29:47 UTC Comment hidden (obsolete)
Comment 66 Kyle M Hall 2013-01-18 15:31:23 UTC
Comment on attachment 14324 [details] [review]
DO NOT USE - Partial rewrite of Logger

Obsoleting this patch for now as the rest are signed off and this one isn't meant to be used.
Comment 67 Paul Poulain 2013-03-19 16:34:27 UTC
Joubu, I can't apply those patches, I get a :

fatal: sha1 information is lacking or useless (C4/Context.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 8190: raise an error if the Log::LogLite module is not installed

on the 3rd patch. Please fix & set back to signed off.
Comment 68 Jonathan Druart 2013-03-19 16:40:19 UTC Comment hidden (obsolete)
Comment 69 Jonathan Druart 2013-03-19 16:40:25 UTC Comment hidden (obsolete)
Comment 70 Jonathan Druart 2013-03-19 16:40:29 UTC Comment hidden (obsolete)
Comment 71 Jonathan Druart 2013-03-19 16:40:32 UTC Comment hidden (obsolete)
Comment 72 Jonathan Druart 2013-03-19 16:40:37 UTC Comment hidden (obsolete)
Comment 73 Marcel de Rooy 2013-06-10 07:35:15 UTC Comment hidden (obsolete)
Comment 74 Marcel de Rooy 2013-06-10 07:35:20 UTC Comment hidden (obsolete)
Comment 75 Marcel de Rooy 2013-06-10 07:35:24 UTC Comment hidden (obsolete)
Comment 76 Marcel de Rooy 2013-06-10 07:35:27 UTC Comment hidden (obsolete)
Comment 77 Marcel de Rooy 2013-06-10 07:35:32 UTC Comment hidden (obsolete)
Comment 78 Marcel de Rooy 2013-06-10 07:37:58 UTC
Trivial rebase.
Comment 79 Marcel de Rooy 2013-06-10 08:47:45 UTC
QA Comment:
Looks quite promising! Some questions on details:

You (almost) follow common levels like:
       LOG_EMERG      system is unusable
       LOG_ALERT      action must be taken immediately
       LOG_CRIT       critical conditions
       LOG_ERR        error conditions
       LOG_WARNING    warning conditions
       LOG_NOTICE     normal, but significant, condition
       LOG_INFO       informational message
       LOG_DEBUG      debug-level message
Why no alert level? Note that LogLite also supports 8 levels (0=unusable, 1=close to unusable)
Displaying 1-Unusable is not intuitively clear (in the prefs)? 

If you include html comments in staff doc-head-open, why not in opac side? 
Note that you add a test in opac-search (info statement).

Is the eval require+die for Koha::Utils::Logger in the third patch really an improvement?

I am no packaging expert :) But is adding this module in install_misc/debian.packages enough? 

Changing status to reflect need for clarification (or adjustment).
Comment 80 Jonathan Druart 2013-06-10 12:43:27 UTC
Hi Marcel, thanks for your comments!

(In reply to comment #79)

> Why no alert level? Note that LogLite also supports 8 levels (0=unusable,
> 1=close to unusable)

I will propose a new patch adding the "alert" level.

> Displaying 1-Unusable is not intuitively clear (in the prefs)? 

I created a wiki page, maybe should we add a link to this page in the syspref description?

> If you include html comments in staff doc-head-open, why not in opac side? 
> Note that you add a test in opac-search (info statement).

Kyle wrote this part (patch in comment 27), so I don't really why but I think it could be a security issue to display the Koha logs at the OPAC.

> Is the eval require+die for Koha::Utils::Logger in the third patch really an
> improvement?

It was asked by Katrin in order to avoid an "internal server error" (see comment 36).

> 
> I am no packaging expert :) But is adding this module in
> install_misc/debian.packages enough? 

I think so, but not really sure.

Switch back to "Signed Off"
Comment 81 Kyle M Hall 2013-06-10 12:47:43 UTC Comment hidden (obsolete)
Comment 82 Jonathan Druart 2013-06-10 12:49:12 UTC Comment hidden (obsolete)
Comment 83 Marcel de Rooy 2013-06-10 13:30:48 UTC
(In reply to comment #81)
> Created attachment 18819 [details] [review] [review]
> Bug 8190 - Add a logging module to Koha - Followup - Add html embedded logs
> to OPAC

I understand the changes for opac and using the Koha plugin logic.
But I still cannot see the log message in the opac document. (Just empty for opac-search)
Are you sure that the statement Logger.get_messages works this way?
I tried logger lc too without results.
It seems not to work..
Comment 84 Kyle M Hall 2013-06-10 15:41:03 UTC
Did you add any logging code to an opac script to test it? I just added this to my opac-main.pl to test:

C4::Context->logger->debug("This is a debug message");
C4::Context->logger->error("This is an error !");

With the default error level only the latter message will display in the embedded logs comment.

(In reply to comment #83)
> (In reply to comment #81)
> > Created attachment 18819 [details] [review] [review] [review]
> > Bug 8190 - Add a logging module to Koha - Followup - Add html embedded logs
> > to OPAC
> 
> I understand the changes for opac and using the Koha plugin logic.
> But I still cannot see the log message in the opac document. (Just empty for
> opac-search)
> Are you sure that the statement Logger.get_messages works this way?
> I tried logger lc too without results.
> It seems not to work..
Comment 85 Marcel de Rooy 2013-06-12 11:38:09 UTC
(In reply to comment #84)
> Did you add any logging code to an opac script to test it? I just added this
> to my opac-main.pl to test:
> 
> C4::Context->logger->debug("This is a debug message");
> C4::Context->logger->error("This is an error !");

Found what happened:
In opac-search Jonathan creates a logger object in opac-search (that's the one I was looking for). But C4/Auth also initialized a logger ALREADY. So the second logger got my message, but the template asked it to the first logger and got nothing. Still following?

Will send a small followup for this.
Comment 86 Marcel de Rooy 2013-06-12 11:43:24 UTC Comment hidden (obsolete)
Comment 87 Marcel de Rooy 2013-06-12 11:44:12 UTC Comment hidden (obsolete)
Comment 88 Marcel de Rooy 2013-06-12 11:44:16 UTC Comment hidden (obsolete)
Comment 89 Marcel de Rooy 2013-06-12 11:44:20 UTC Comment hidden (obsolete)
Comment 90 Marcel de Rooy 2013-06-12 11:44:24 UTC Comment hidden (obsolete)
Comment 91 Marcel de Rooy 2013-06-12 11:44:29 UTC Comment hidden (obsolete)
Comment 92 Marcel de Rooy 2013-06-12 11:44:33 UTC Comment hidden (obsolete)
Comment 93 Marcel de Rooy 2013-06-12 11:44:37 UTC Comment hidden (obsolete)
Comment 94 Marcel de Rooy 2013-06-12 12:04:31 UTC
Some QA observations:

The first patch commit message should preferably be edited.
Since we initialize the logger in Auth.pm, you do not need:
use Koha::Utils::Logger qw/$log/;
$log = Koha::Utils::Logger->new;
Just say: $log= C4::Context->logger;
(This was actually the reason for the last followup.)
Will take the liberty now to edit these comments. Forgive me for that..

The first patch seems to delete a SetVersion line in updatedatabase for an older dbrev (3.9.0.45?). This is a blocker. Will amend this one if nothing more follows.

The hardcoded 5 in Koha::Utils::Logger in new routine is not very nice :) Strange, because it was a constant in earlier patch than patch 5.

Thanks for adding the alert level. When looking trough the code I still noticed a mention of 7 levels instead of 8 now.

Another set of patches coming..
Comment 95 Marcel de Rooy 2013-06-12 12:19:44 UTC Comment hidden (obsolete)
Comment 96 Marcel de Rooy 2013-06-12 12:21:07 UTC Comment hidden (obsolete)
Comment 97 Marcel de Rooy 2013-06-12 12:21:12 UTC Comment hidden (obsolete)
Comment 98 Marcel de Rooy 2013-06-12 12:21:16 UTC Comment hidden (obsolete)
Comment 99 Marcel de Rooy 2013-06-12 12:21:20 UTC Comment hidden (obsolete)
Comment 100 Marcel de Rooy 2013-06-12 12:22:34 UTC Comment hidden (obsolete)
Comment 101 Marcel de Rooy 2013-06-12 12:23:34 UTC Comment hidden (obsolete)
Comment 102 Marcel de Rooy 2013-06-12 12:23:40 UTC Comment hidden (obsolete)
Comment 103 Marcel de Rooy 2013-06-12 12:32:40 UTC
'final' comment for RM:
Code looks good to me now. (See QA comments before.)
I am wondering if we should by default put the level to 5 (normal) at install or upgrade. When someone does not want to use it, we could set it to zero (emerge or unusable level) and have around zero messages.
Robin should still have a look for the packaging stuff.
Jonathan will probably send a few trivial followups for housekeeping (e.g. correcting 7->8 levels; also in pref desc in db).

Passed QA
Comment 104 Jonathan Druart 2013-06-12 13:02:38 UTC Comment hidden (obsolete)
Comment 105 Jonathan Druart 2013-06-12 13:04:03 UTC
(In reply to comment #95)
> Amended the patch for a updatedatabase problem. Edited the commit message.

Thanks for that Marcel!
Comment 106 Galen Charlton 2013-06-12 14:41:51 UTC
(In reply to comment #6)
> (In reply to comment #4)
> > Perhaps also take a look at Log4Perl 
> > http://search.cpan.org/~mschilli/Log-Log4perl-1.37/lib/Log/Log4perl.pm
> I had a look at several perl logging modules. My conclusion is LogLite is
> the easier for what we need into Koha.

That begs a question, though -- what do we need in Koha?

One part of the answer to that question, for me, and I suspect for most folks who administer more than one Koha database or who administer services in addition to Koha, is syslog support.  Log::LogLite doesn't provide such support, whereas Log4Perl does in addition to supporting appending to traditional log files.

Also, Chris in comment 58 all but proposed a counter-patch.  Would the original patch submitter respond to that comment, please?
Comment 107 Jonathan Druart 2013-06-12 15:07:55 UTC
(In reply to comment #106)
> That begs a question, though -- what do we need in Koha?
> 
> One part of the answer to that question, for me, and I suspect for most
> folks who administer more than one Koha database or who administer services
> in addition to Koha, is syslog support.  Log::LogLite doesn't provide such
> support, whereas Log4Perl does in addition to supporting appending to
> traditional log files.
> 
> Also, Chris in comment 58 all but proposed a counter-patch.  Would the
> original patch submitter respond to that comment, please?

This development was written 1 year ago for BibLibre branches. We need a logger with different levels (like many softwares).
I provided a simple module which does what we want.
Since this is a simple class, it could be rewritten easily by someone having a better idea.
The patch proposed by Chris was not working but it could be the second step to this development.
Comment 108 Chris Cormack 2013-06-12 19:38:36 UTC
(In reply to comment #107)
> (In reply to comment #106)
> > That begs a question, though -- what do we need in Koha?
> > 
> > One part of the answer to that question, for me, and I suspect for most
> > folks who administer more than one Koha database or who administer services
> > in addition to Koha, is syslog support.  Log::LogLite doesn't provide such
> > support, whereas Log4Perl does in addition to supporting appending to
> > traditional log files.
> > 
> > Also, Chris in comment 58 all but proposed a counter-patch.  Would the
> > original patch submitter respond to that comment, please?
> 
> This development was written 1 year ago for BibLibre branches. We need a
> logger with different levels (like many softwares).
> I provided a simple module which does what we want.
> Since this is a simple class, it could be rewritten easily by someone having
> a better idea.
> The patch proposed by Chris was not working but it could be the second step
> to this development.

The point of my counter proposal was that we do not need another module, we can add some lines to context.pm using Log4perl and not have another module to maintain ourselves.

Pushing this now as it is, would mean we would just have to remove it again if we wanted to do it the Log4perl way. IE if people think that is a good idea, then I can write a working patch for that, however if this is pushed, I won't. 
I also won't be happy that we have yet another module we have to maintain instead.

I maintain we dont need a new logging module, we just need to add logging to C4::Context.

I don't like suggesting people abandon the work they have done, but I have real reservations about adding yet another module we have to maintain. The fact that it is a class doesn't really affect this point at all
My point was you don't need another module, we can simply do 

use Log::Log4perl qw(:levels get_logger);

my $layout_class   = "Log::Log4perl::Layout::PatternLayout";
my $layout_pattern = "[%d] %p - %M - %m - %T%n";

my $config = <<"END";
      log4perl.logger = INFO, Logfile, Screen
      
      log4perl.appender.Logfile = Log::Log4perl::Appender::File
      log4perl.appender.Logfile.filename = $ENV{KOHA_LOG}
      log4perl.appender.Logfile.mode=append
      log4perl.appender.Logfile.layout = $layout_class
      log4perl.appender.Logfile.layout.ConversionPattern=$layout_pattern
      log4perl.appender.Screen  = Log::Log4perl::Appender::Screen
      log4perl.appender.Screen.layout = $layout_class
      log4perl.appender.Screen.layout.ConversionPattern = $layout_pattern
END

Log::Log4perl->init( \$config );

and then in there a sub

sub logger
{
    my $self = shift;

    if ( defined( $context->{"logger"} ) ) {
        return $context->{"logger"};
    }

    $context->{"logger"} = Log::Log4perl->get_logger('C4::Context');
    $context->{"logger"}->level(C4::Context->preference("LogLevel"));

    return $context->{"logger"};
}

(Or something very similar in C4::Context)

Then anywhere we want to log C4::Context->logger() gives us a logger
Comment 109 Jonathan Druart 2013-06-13 08:58:01 UTC
(In reply to comment #108)

Hi Chris,

> The point of my counter proposal was that we do not need another module, we
> can add some lines to context.pm using Log4perl and not have another module
> to maintain ourselves.

Humm, I don't think it is a good argument. A module with unit tests is easiest to maintain than a module like C4::Context (~1000l) :)

> Pushing this now as it is, would mean we would just have to remove it again
> if we wanted to do it the Log4perl way. IE if people think that is a good
> idea, then I can write a working patch for that, however if this is pushed,
> I won't. 
> I also won't be happy that we have yet another module we have to maintain
> instead.
> 
> I maintain we dont need a new logging module, we just need to add logging to
> C4::Context.

There is no problem to propose an alternative working patch, but don't loose sight of the fact that the module we propose here does something else more than just logging:
- a called by trace is displayed for some levels
- a get_messages method allows to return all logged messages
- messages are thrown to the stdout if the logger is not defined (ie. no filepatch defined or a bad configuration somewhere)
- a parameter allows to dump a variable (using Data::Dumper or something else. e.g. $logger->debug($hashref, "dump_me");)
Comment 110 Marcel de Rooy 2013-06-13 09:44:32 UTC
(In reply to comment #109)
> (In reply to comment #108)
> > The point of my counter proposal was that we do not need another module, we
> > can add some lines to context.pm using Log4perl and not have another module
> > to maintain ourselves.
> 
> Humm, I don't think it is a good argument. A module with unit tests is
> easiest to maintain than a module like C4::Context (~1000l) :)

Chris, I think that Jonathan has a point here. Maintaining a small and clear coded module will not be a pain. The maintenance problems will not be in this kind of module, but in complex modules and scripts dealing with circulation among others.

But if Log4Perl is a better choice, it sounds like that it could replace LogLite rather easy?
Comment 111 Chris Cormack 2013-06-13 09:46:17 UTC
(In reply to comment #109)
> (In reply to comment #108)
> 
> Hi Chris,
> 
> > The point of my counter proposal was that we do not need another module, we
> > can add some lines to context.pm using Log4perl and not have another module
> > to maintain ourselves.
> 
> Humm, I don't think it is a good argument. A module with unit tests is
> easiest to maintain than a module like C4::Context (~1000l) :)
> 

We still have to maintain C4::Context, and now a new module, it feels hugely like reinventing the wheel for the 13th time and I still don't like it. We probably will have to agree to disagree on this.
Comment 112 Jonathan Druart 2014-12-31 12:04:45 UTC
Created attachment 34837 [details] [review]
Bug 8190: Koha::Utils::Logger, Logging module

Use the logger like:
$logger= C4::Context->logger;
$logger->debug("This is a debug message");
$logger->info("This is an information");
$logger->error("This is an error !");

The Logger constructor can take an hash reference with "file" and
"level" to define a filepath or a log level.

For a log level >= warning, a call stack is printed.

Prerequisite:
- set an environment variable KOHA_LOG in your virtual host:
    SetEnv KOHA_LOG /home/koha/var/log/opac.log
- set a write flag for www-data on this file

Please have a look at t/Logger.t for more details.

Includes (apparently from squashed patches):
Bug 8190: Followup Logger: FIX perlcritic
Bug 8190 - Followup - Add cached logger, output messages to template
* Add C4::Context->logger
* Embed logged messages to a comment in the template html,
  controlled by the system preference LogToHtmlComments\
* Add both new system preferences to sysprefs.sql
Bug 8190 follow-up moving updatedatabase part at the right place

Signed-off-by: Mason James <mtj@kohaaloha.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended the patch for a updatedatabase problem. Edited the commit message.
Comment 113 Jonathan Druart 2014-12-31 12:04:50 UTC
Created attachment 34838 [details] [review]
Bug 8190: Follow Add some POD

Add POD and prefix private methods with _

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 114 Jonathan Druart 2014-12-31 12:04:54 UTC
Created attachment 34839 [details] [review]
Bug 8190: raise an error if the Log::LogLite module is not installed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 115 Jonathan Druart 2014-12-31 12:04:58 UTC
Created attachment 34840 [details] [review]
Bug 8190: FIX Unit test

Set the KOHA_LOG environment variable in the test

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 116 Jonathan Druart 2014-12-31 12:05:03 UTC
Created attachment 34841 [details] [review]
Bug 8190: QA Followup

- Remove the private variables
- Remove the use of C4::Context in the UT
- Remove the Exporter

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 117 Jonathan Druart 2014-12-31 12:05:08 UTC
Created attachment 34842 [details] [review]
Bug 8190: Followup - Add html embedded logs to OPAC

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 118 Jonathan Druart 2014-12-31 12:05:12 UTC
Created attachment 34843 [details] [review]
Bug 8190: Add the 'alert' level.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 119 Jonathan Druart 2014-12-31 12:05:17 UTC
Created attachment 34844 [details] [review]
Bug 8190: Followup for opac-search

Script should not create a second logger object, but use the one that the Auth
module already made.
Works fine now. Only question is: Should we leave it here. This is somewhat of
a debug statement.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 120 Jonathan Druart 2014-12-31 12:05:21 UTC
Created attachment 34845 [details] [review]
Bug 8190: FIX description and values for the LogLevel pref
Comment 121 Jonathan Druart 2014-12-31 12:06:08 UTC
Patches rebased.
Comment 122 Martin Renvoize 2015-06-17 07:43:57 UTC
I'm in general preferring the simplicity of Bug 14167 for now. Lets go with that and get this much needed feature into Koha!
Comment 123 Tomás Cohen Arazi 2015-07-21 14:58:38 UTC

*** This bug has been marked as a duplicate of bug 14167 ***