Skip Menu |

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Log-Any CPAN distribution.

Maintainer(s)' notes

DO NOT FILE TICKETS HERE

Instead, file tickets on Github here → Log Any Issues.

Report information
The Basics
Id: 80448
Status: resolved
Priority: 0/
Queue: Log-Any
People
Owner: Nobody in particular
Requestors: bohica [...] ntlworld.com
Cc:
AdminCc:
Bug Information
Severity: Wishlist
Broken in: 0.14
Fixed in: (no value)

History Show all quoted text
  Sun Oct 28 08:47:26 2012 bohica [...] ntlworld.com - Ticket created
Subject: log functions don't accept closures
I'm not sure if you'd class this a bug or an enhancement. I use Log::Log4perl all the time and had a request to change DBIx::Log4perl to DBIx::LogAny. However, I've hit a problem as a lot of the code does: $log->debug(sub {something_expensive}); e.g. $log->debug(sub {Dumper($x)}); which results in the log file like this: [Sun Oct 28 12:21:21 2012] CODE(0x2986758) Log::Log4perl allows the closure and expands it. It is useful as the expression does not get evaled if logging is disabled. See http://search.cpan.org/~mschilli/Log-Log4perl-1.39/lib/Log/Log4perl.pm#Penalties Would you consider making this work. If you don't the only way around this I see is to put is_debug etc around all of these which I'd rather not do because a) there is a lot of them b) it will penalise DBIx::Log4perl which shares the same code base. BTW, 2 other observations: 1) in the pod on cpan, the links in Log::Any::Adapter below "Adapters in this distribution" don't work. 2) you probably think it is obvious but I did a set('File', 'x.log') and got an error saying autoflush could not be found via IO::File. It might be worth mentioning you need to use IO::File before setting File (at least in Log::Any::Adapter::File). This might have been slightly more obvious if (1) was not broken as one of the links I guess should be to IO::File or Log::Any::Adapter::File. Martin -- Martin J. Evans Wetherby, UK
  Sat Nov 24 07:40:39 2012 JSWARTZ [...] cpan.org - Correspondence added
This seemed reasonable, unfortunately I realized it will be difficult to add now, since every Log::Any adapter defines its own debug(), info(), etc. methods. There is no easy way for Log::Any::Adapter to capture control of logging methods and check for a closure. To me it seems like $log->debug(...) if $log->is_debug(); $log->debug(sub { ... }); have the same performance in the case that debug logging is off - for each one it is a single method call. In fact the first is slightly faster since you don't have to check the ref of the argument. And you can cache $log->is_debug at the beginning of a method for even better performance, e.g. my $is_debug = $log->is_debug(); ... $log->debug(...) if $is_debug(); ... $log->debug(...) if $is_debug(); So I don't understand your point about b) penalizing - am I missing something? As far as a), the effort required, if you point me to a git repo I'd be happy to make a patch that changes all your closure calls to is_debug calls. :) On Sun Oct 28 08:47:26 2012, MJEVANS wrote: Show quoted text
> I'm not sure if you'd class this a bug or an enhancement. I use > Log::Log4perl all the time and had a request to change DBIx::Log4perl > to > DBIx::LogAny. However, I've hit a problem as a lot of the code does: > > $log->debug(sub {something_expensive}); > > e.g. > > $log->debug(sub {Dumper($x)}); > > which results in the log file like this: > > [Sun Oct 28 12:21:21 2012] CODE(0x2986758) > > Log::Log4perl allows the closure and expands it. It is useful as the > expression does not get evaled if logging is disabled. > > See > http://search.cpan.org/~mschilli/Log-Log4perl- > 1.39/lib/Log/Log4perl.pm#Penalties > > Would you consider making this work. If you don't the only way around > this I see is to put is_debug etc around all of these which I'd rather > not do because a) there is a lot of them b) it will penalise > DBIx::Log4perl which shares the same code base. > > BTW, 2 other observations: > > 1) in the pod on cpan, the links in Log::Any::Adapter below "Adapters > in > this distribution" don't work. > 2) you probably think it is obvious but I did a set('File', 'x.log') > and > got an error saying autoflush could not be found via IO::File. It > might > be worth mentioning you need to use IO::File before setting File (at > least in Log::Any::Adapter::File). This might have been slightly more > obvious if (1) was not broken as one of the links I guess should be to > IO::File or Log::Any::Adapter::File. > > Martin
  Sat Nov 24 07:40:40 2012 The RT System itself - Status changed from 'new' to 'open'
  Wed Nov 28 09:21:51 2012 bohica [...] ntlworld.com - Correspondence added
On Sat Nov 24 07:40:39 2012, JSWARTZ wrote: Show quoted text
> This seemed reasonable, unfortunately I realized it will be difficult to > add now, since every Log::Any adapter defines its own debug(), info(), > etc. methods. There is no easy way for Log::Any::Adapter to capture > control of logging methods and check for a closure.
Fair enough. Show quoted text
> To me it seems like > > $log->debug(...) if $log->is_debug(); > $log->debug(sub { ... }); > > have the same performance in the case that debug logging is off - for > each one it is a single method call. In fact the first is slightly > faster since you don't have to check the ref of the argument. And you > can cache $log->is_debug at the beginning of a method for even better > performance, e.g. > > my $is_debug = $log->is_debug(); > ... > $log->debug(...) if $is_debug(); > ... > $log->debug(...) if $is_debug(); > > So I don't understand your point about b) penalizing - am I missing > something?
It was a penalty if you did not add "if $log->xxx()" and xxx logging was not enabled as Perl would eval your sub first, pass it to Log::Log4perl and it would not log it as level xxx was not in use. I've changed it by adding if tests on all log calls. Show quoted text
> As far as a), the effort required, if you point me to a git repo I'd be > happy to make a patch that changes all your closure calls to is_debug > calls. :)
Kind offer, thanks, but I've done it. BTW, I also put the caller_depth stuff from Log::Log4perl back in by using Module::Loaded to see if Log::Log4perl was already loaded. Thanks for looking at this. Problem sorted. Martin -- Martin J. Evans Wetherby, UK
  Wed Nov 28 09:25:17 2012 swartz [...] pobox.com - Correspondence added
Subject: Re: [rt.cpan.org #80448] log functions don't accept closures
Date: Wed, 28 Nov 2012 06:25:02 -0800
To: bug-Log-Any [...] rt.cpan.org
From: Jonathan Swartz <swartz [...] pobox.com>
Great! On Nov 28, 2012, at 6:21 AM, Martin J Evans via RT wrote: Show quoted text
> Queue: Log-Any > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=80448 > > > On Sat Nov 24 07:40:39 2012, JSWARTZ wrote:
>> This seemed reasonable, unfortunately I realized it will be difficult to >> add now, since every Log::Any adapter defines its own debug(), info(), >> etc. methods. There is no easy way for Log::Any::Adapter to capture >> control of logging methods and check for a closure.
> > Fair enough. >
>> To me it seems like >> >> $log->debug(...) if $log->is_debug(); >> $log->debug(sub { ... }); >> >> have the same performance in the case that debug logging is off - for >> each one it is a single method call. In fact the first is slightly >> faster since you don't have to check the ref of the argument. And you >> can cache $log->is_debug at the beginning of a method for even better >> performance, e.g. >> >> my $is_debug = $log->is_debug(); >> ... >> $log->debug(...) if $is_debug(); >> ... >> $log->debug(...) if $is_debug(); >> >> So I don't understand your point about b) penalizing - am I missing >> something?
> > It was a penalty if you did not add "if $log->xxx()" and xxx logging was > not enabled as Perl would eval your sub first, pass it to Log::Log4perl > and it would not log it as level xxx was not in use. > > I've changed it by adding if tests on all log calls. >
>> As far as a), the effort required, if you point me to a git repo I'd be >> happy to make a patch that changes all your closure calls to is_debug >> calls. :)
> > Kind offer, thanks, but I've done it. > > BTW, I also put the caller_depth stuff from Log::Log4perl back in by > using Module::Loaded to see if Log::Log4perl was already loaded. > > Thanks for looking at this. Problem sorted. > > Martin > -- > Martin J. Evans > Wetherby, UK
  Fri Nov 30 14:06:30 2012 bohica [...] ntlworld.com - Correspondence added
By all means close/reject this rt now. Martin -- Martin J. Evans Wetherby, UK
  Wed Jul 10 18:08:12 2013 dagolden [...] cpan.org - Correspondence added
On Sat Nov 24 07:40:39 2012, JSWARTZ wrote: Show quoted text
> This seemed reasonable, unfortunately I realized it will be difficult to > add now, since every Log::Any adapter defines its own debug(), info(), > etc. methods. There is no easy way for Log::Any::Adapter to capture > control of logging methods and check for a closure.
Looking at the code, I think you could implement this as part of the ______f() methods. If the "format" argument is a coderef, call it with the arguments, possibly protected by an is_level check. $log->debugf(sub{ ...expensive... }); If you think of "f" as "filter" instead of "format", it fits conceptually. David
  Wed Aug 21 23:33:02 2013 dagolden [...] cpan.org - Severity Normal changed to Wishlist
  Fri Jan 09 13:31:42 2015 dagolden [...] cpan.org - Correspondence added
Ticket migrated to Github: https://github.com/dagolden/Log-Any/issues/17
  Fri Jan 09 13:31:43 2015 dagolden [...] cpan.org - Status changed from 'open' to 'resolved'