[Koha-bugs] [Bug 15253] Add Koha::Logger based logging for SIP2

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15253

--- Comment #8 from Olli-Antti Kivilahti <olli-antti.kivilahti at jns.fi> ---
(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.