https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15253
--- Comment #8 from Olli-Antti Kivilahti <[email protected]> --- (In reply to Kyle M Hall from comment #5) > (In reply to Colin Campbell from comment #4) > > Is this a bit cumbersome? what about starting by passing Log4perl to the > > Net::Server config it already supports it its the Koha calls to syslog that > > stop us enabling it. Coud we not then just call up logger rather than > > passing $self->server about all the time which looks a bit clumsy > > I'm not sure I understand what your saying. Could you expand on this a bit? > > Thanks! ::My Code review about this feature:: I think we should use only one way of logging in Koha. If we have a separate way of defining and calling loggers in Net::Server, then in C4::* , Koha::* then we have Mojo::Log in Mojolicious Server, it gets hard to work with all of them. We should use Koha::Logger with everything. If you don't happen to like the extra securities Kyle imnplemented in Koha::Logger you can easily write your own drop-in replacement for it and Koha won't know the difference, especially since the feature is so well regression tested. Passing the server as a constructor parameter is smart. The other alternative is to define and make sure that the global package-level parameter $currentServer (or equivalent) has its status maintained in all parts of the SIP-server lifecycle. This might get cumbersome and will lead to hard-to-track bugs in the future. This way is explicit and improves the usability of the logging output. ...however... Looking at http://search.cpan.org/~mschilli/Log-Log4perl-1.47/lib/Log/Log4perl/Layout/PatternLayout.pm#DESCRIPTION and especially "Mapped Diagnostic Context" http://search.cpan.org/~mschilli/Log-Log4perl-1.47/lib/Log/Log4perl.pm#Mapped_Diagnostic_Context_%28MDC%29 ...makes me think that you no longer need to pass the server-handle around to log the mentioned sip2-request data elements. You can just define the MDC when you start processing the transaction (or when you log-in or however the SIP-server worked). The added benefit of using MDC is that you can configure whether or not you want to display the userid or institution or whatnot. This is a must for this kind of generic information. There is only one reasonable way to deal with this issue, and that is to use the MDC and set it when you decide which userid/institution/whatnot is being logged, or you can set the MDC every time you want to log from the server-handle. With MDC the ConversionPattern would look crudely something like this: log4perl.appender.SIP.layout.ConversionPattern=[%d] [%p] [%X{userid}] [%X{institution}] %m %l %n I can imagine in the future there may be other purposes for the server-handle and it is nice to have it accessible. However passing these references around leaves a dreadful possibility of accidentally stumbling upon a circular reference :) "Bug 15253 - Log stderr via Koha::Logger as well" is VERY GOOD <3 <3 <3 Also this feature has the title "Bug 15253 - Add Koha::Logger based logging for SIP2" which doesn't imply passing server as a parameter everywhere and quite frankly in light of this (new for me) MDC workaround, I see no reason to introduce that server-handle passing change here. Also messages are logged twice: 1st with $self->{server}->{logger}->debug(); 2nd syslog("LOG_DEBUG", ...); Log::Log4perl best practice is to invoke it like this: $self->{server}->{logger}->debug() or syslog("LOG_DEBUG", ...); I don't see any reason why we should preserve the old logging code in the SIP2-server at all. Koha::Logger is the way to go! But looks like that is fixed in "Bug 15253 - Remove use of syslog". Also there is a complete lack of tests. You can take a look at Bug 16302 and Bug 16304 for a pretty good testing system for Koha::Logger. Summary: A much needed change, but needs to be simplified a lot using MDC. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
