Bug 406301 - 3.20: Log4perl support (keys added to data/params as well) Bugzilla/RH.pm
3.20: Log4perl support (keys added to data/params as well) Bugzilla/RH.pm
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: Bugzilla General (Show other bugs)
3.2
All Linux
high Severity medium (vote)
: ---
: ---
Assigned To: Noura El hawary
: Reopened
Depends On:
Blocks: 406201
  Show dependency treegraph
 
Reported: 2007-11-30 11:58 EST by David Lawrence
Modified: 2013-06-24 00:18 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-22 02:36:00 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
v1 for log4perl logging code added to BZ 3.2 (15.86 KB, patch)
2008-03-31 02:32 EDT, Noura El hawary
dkl: review-
Details | Diff
v2 for log4perl logging code added to BZ 3.2 (36.66 KB, patch)
2008-04-01 08:27 EDT, Noura El hawary
dkl: review+
Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Mozilla Foundation 426463 None None None Never

  None (edit)
Description David Lawrence 2007-11-30 11:58:34 EST
Description:
Different parts of the Bugzilla code has debug statements added that use Log4perl to write to /tmp/bz.log. These are added to give the ability to see where performance bottlenecks exist as well as to give statistics on how often different code paths are executed.

Function Requirements:
Needs keys added to data/params. Bugzilla.pm Bugzilla/RH.pm. $logger code added to process_bug.cgi, show_bug.cgi, xmlrpc.cgi, Bugzilla/BugMail.pm, Bugzilla/RPC/*.pm, and others.
Comment 1 Noura El hawary 2007-12-19 13:11:21 EST
LOC estimation:

1- first we need to add textual parameter in data/params that will hold the
log4perl configuration file ,, can be done easily in 3.2 , ((6 LOC)) for
log4perl configuration will be added to data/params and ,, The definitions for
that param in 2.18 is in defparams.pl however this file doesn't exist in
bugzilla 3 ,, defparams.pl is replaced with Bugzilla/Config/*.pm modules ,,, I
would say the suitable place for the log4perl config param  would be
Bugzilla/Config/Admin.pm as this param is for bugzilla administrators use to
check bugzilla performance and should be controlled by them this will need about
((6 LOC)),, also the log4perl configuration will be put into a constant in
Bugzilla/Constants.pm ((6 LOC)

Then few lines of code will be added to Bugzilla.pm to create logger subroutine
about (( 7 LOC ))  ,, New module Bugzilla/RH.pm will be added that include will
initalize log4perl using the config params,, currently RH.pm in 2_18 will not
work under 3.2 because of mod_perl ,, We will have to find a way to get it to
work , so we can not use the current LOC in 2.18 RH.pm to estimate  what will it
be in 3.2 so I will use for that FP

LOC for Bugzilla/RH.pm

Inputs : log4perl Config param,, log4perl config constant = 2
Outputs : log message, load log4perl config file failure warnings = 2
Inquiries = 0
Internal Logical files: Constants.pm and data/params = 2
External Logical files : log4perl = 1

FP (total count) = (3*2)+(4*2)+(7*2)+(5*1) = 33

FP = 33 * 1.11 = 37 

LOC (for RH.pm) = 37 * 60 = 2220


Then we need to spread the logger statements in the Bugzilla code, basically we
can use current number of these statements in 2.18 for the estimation ,,
currently in the code we have : 216 LOC for  logger statements 


So in total :

TOTAL LOC = 6 + 6 + 6 + 7 + 2220 + 216 = 2461
Comment 2 Kevin Baker 2007-12-29 13:27:51 EST
Before you implement the Log4perl logic we should first see if the upstream 
will accept a patch for it. 
Comment 3 Noura El hawary 2008-03-27 02:06:32 EDT
(In reply to comment #2)
> Before you implement the Log4perl logic we should first see if the upstream 
> will accept a patch for it. 

Hi Kevin,,

I talked to Max Kanat about upstream accepting patch for log4perl , this was his
answer:

<Noura> in redhat bugzilla 2.18 to add logging to bugzilla
<Noura> so we can track performace 
<mkanat> Noura: That seems a little overkill, it's pretty easy to see Bugzilla's
performance.
<mkanat> Noura: But I've implemented a logging mechanism once for a client, too.
<Noura> mkanat: yeah what was it using?
<mkanat> Oh, just some custom code that put things in the DB.
<Noura> mkanat: so do you think that you would accept patch for logging ?
<Noura> or that is not really prefered?
<mkanat> Noura: Um, I don't see a need for it, right now.

so what shall we do?

Noura
Comment 4 Noura El hawary 2008-03-28 12:32:09 EDT
Hi Dave,,

Can you please give me some guidance here on how to work around the BEGIN block
gets executed only once with mod_perl for our log4perl patch.

and as we discussed before , did we agree at the end to implement the whole
thing as extension and hooks?

Thanks,
Noura
Comment 5 David Lawrence 2008-03-30 21:40:01 EDT
(In reply to comment #4)
> Hi Dave,,
> 
> Can you please give me some guidance here on how to work around the BEGIN block
> gets executed only once with mod_perl for our log4perl patch.
> 
> and as we discussed before , did we agree at the end to implement the whole
> thing as extension and hooks?
> 
> Thanks,
> Noura

Since we decided that we would need to put Bugzilla::Hook::process() code in may
places throughout the code anyway we would not do this as an extension. We will
just put the $logger->debug() lines in the appropriate places similar to 2.18.

As for the BEGIN {} block conversion for mod_perl, just place the code that is
normally in RH.pm in the BEGIN, inside of the init_page() in Bugzilla.pm. This
gets executed in Bugzilla.pm itself if not in a mod_perl environment or by
mod_perl.pl script when inside of a mod_perl environment.

And the code that is in the END {} block in RH.pm will go in the _cleanup()
function in Bugzilla.pm.

You will still need to add the sub logger {} function as well to Bugzilla.pm so
the caller can get the logger handle when needed using Bugzilla->logger. You
should be able to move the function from 2.18 to 3.2 as is.

Dave 
Comment 6 Noura El hawary 2008-03-30 21:57:14 EDT
(In reply to comment #5)
> (In reply to comment #4)
> > Hi Dave,,
> > 
> > Can you please give me some guidance here on how to work around the BEGIN block
> > gets executed only once with mod_perl for our log4perl patch.
> > 
> > and as we discussed before , did we agree at the end to implement the whole
> > thing as extension and hooks?
> > 
> > Thanks,
> > Noura
> 
> Since we decided that we would need to put Bugzilla::Hook::process() code in may
> places throughout the code anyway we would not do this as an extension. We will
> just put the $logger->debug() lines in the appropriate places similar to 2.18.
> 
> As for the BEGIN {} block conversion for mod_perl, just place the code that is
> normally in RH.pm in the BEGIN, inside of the init_page() in Bugzilla.pm. This
> gets executed in Bugzilla.pm itself if not in a mod_perl environment or by
> mod_perl.pl script when inside of a mod_perl environment.
> 
> And the code that is in the END {} block in RH.pm will go in the _cleanup()
> function in Bugzilla.pm.
> 
> You will still need to add the sub logger {} function as well to Bugzilla.pm so
> the caller can get the logger handle when needed using Bugzilla->logger. You
> should be able to move the function from 2.18 to 3.2 as is.
> 
> Dave 

Great Thanks Dave :) ,, this will make it very easy for me :)

Noura
Comment 7 Noura El hawary 2008-03-31 02:32:08 EDT
Created attachment 299688 [details]
v1 for log4perl logging code added to BZ 3.2 

Attached is a patch that will apply log4perl logging to bugzilla 3.2,

Difference between this patch and current patch in 2.18:
1- as Dave Suggested the code that was in Bugzilla::RH is now in Bugzilla.pm
the BEGIN code in is Bugzilla::init_page and the END code is in
Bugzilla::_clean_up()

2- as the log4perl_config param is accessed using Bugzilla->params as the
function Bugzilla::Config::Param() doesn't exist anymore in 3.2

3- the log4perl_config param definition I put it under Bugzilla::Config::Admin
as defparams.pl doesn't exist anymore in 3.2,, also I chose Admin.pm as I think
this param is for bugzilla admin and sysadmin to manage. so it will appear in
the parameters section under Administrative Policies, any objections?

4- log4perl_config param description is kept separate to its definition it is
now in the template template/en/default/admin/params/admin.html.tmpl .

5- I haven't added yet logging for WebService modules, shall I add logging in
both the WebService modules and also the xmlrpc functions in the compat_xmlrpc
extention webservice.pl ?

6- Also I didn add logging yet for sendmail.

the patch doesn't include data/params , please add to the data/params:

	   'log4perl_config' => 'log4perl.rootLogger=DEBUG, LOGFILE
log4perl.appender.LOGFILE=Log::Log4perl::Appender::File
log4perl.appender.LOGFILE.filename=/tmp/bz.log
log4perl.appender.LOGFILE.mode=append
log4perl.appender.LOGFILE.layout=PatternLayout
log4perl.appender.LOGFILE.layout.ConversionPattern=[%d.%r] [%H;%P;%X{script}]
[%p] [%X{user_login}:%X{http_x_forwarded_for}:%X{remote_addr}] [%l] - %m%n
',


Please let me know if there are any more suggestions for the logging.

Thanks,
Noura
Comment 8 Noura El hawary 2008-03-31 02:33:56 EDT
worked 5 hours
Comment 9 Noura El hawary 2008-03-31 02:48:39 EDT
Patch is now applied to bugdev.devel.redhat.com so you can test it there. also
you can see the /tmp/bz.log there generated. 
Comment 10 David Lawrence 2008-03-31 15:00:57 EDT
(In reply to comment #7)
> Difference between this patch and current patch in 2.18:
> 1- as Dave Suggested the code that was in Bugzilla::RH is now in Bugzilla.pm
> the BEGIN code in is Bugzilla::init_page and the END code is in
> Bugzilla::_clean_up()
> 

Looks good.

> 2- as the log4perl_config param is accessed using Bugzilla->params as the
> function Bugzilla::Config::Param() doesn't exist anymore in 3.2
> 

Sure

> 3- the log4perl_config param definition I put it under Bugzilla::Config::Admin
> as defparams.pl doesn't exist anymore in 3.2,, also I chose Admin.pm as I think
> this param is for bugzilla admin and sysadmin to manage. so it will appear in
> the parameters section under Administrative Policies, any objections?
> 

I think that is the best place for it.

> 4- log4perl_config param description is kept separate to its definition it is
> now in the template template/en/default/admin/params/admin.html.tmpl .
> 

Yep.

> 5- I haven't added yet logging for WebService modules, shall I add logging in
> both the WebService modules and also the xmlrpc functions in the compat_xmlrpc
> extention webservice.pl ?
>

Yes, please do. Bottom line is any logging we have in the current
rh_bugzilla_2_18 tree now, after the last scale back, we want to duplicate where
possible in the rh_bugzilla_3 tree. We can scale back further later on if we
need to.

> 6- Also I didn add logging yet for sendmail.
> 

Please do also.

Thanks
Dave
Comment 11 David Lawrence 2008-03-31 15:04:50 EDT
Comment on attachment 299688 [details]
v1 for log4perl logging code added to BZ 3.2 

>Index: Bugzilla.pm
>===================================================================
>+# REDHAT EXTENSION BEGIN 406301
>+# add Log4perl support 
>+use Log::Log4perl;
>+use Log::Log4perl::MDC;
>+use Time::HiRes;
>+use Bugzilla::Constants;

Bugzilla::Constans already being imported above so no need to do so here.

>@@ -282,7 +323,17 @@ sub login {
>     else {
>         $class->set_user($authenticated_user);
>     }
>-    
>+
>+    # REDHAT EXTENSION BEGIN 406301
>+    # Log4perl support 
>+    # code these in the log layout pattern as %X{<key>}
>+    Log::Log4perl::MDC->put( user_id => $class->user->id );
>+    Log::Log4perl::MDC->put( user_name => $class->user->name );
>+    Log::Log4perl::MDC->put( user_login => $class->user->login );
>+    # REDHAT EXTENSION END 406301
>+
>+
>+ 

Nit-pick: Only need one line of white space here.


>@@ -62,7 +63,17 @@ sub get_param_list {
>    name => 'supportwatchers',
>    type => 'b',
>    default => 0
>-  } );
>+  },
>+
>+  # REDHAT EXTENSION BEGIN 406301
>+  # Log4perl param      
>+  {
>+  name => 'log4perl_config',
>+  type => 'l',
>+  default => Bugzilla::Constants::LOG4PERL_DEFAULT_CONFIG, 
>+  }
>+  # REDHAT EXTENSION END 406301

Nit-Pick: please add one extra space in front of the hash keys similar to the
other param sections.

Dave
Comment 12 David Lawrence 2008-03-31 15:44:51 EDT
[dkl@localhost bugzilla-redhat-log]$ perl t/001compile.t 

ok 101 - Bugzilla/WebService/Component.pm
not ok 102 - Bugzilla/WebService/Bug.pm --ERROR
#   Failed test 'Bugzilla/WebService/Bug.pm --ERROR'
#   at t/001compile.t line 112.
Can't locate object method "logger" via package "Bugzilla" at
Bugzilla/BugMail.pm line 69.
Compilation failed in require at Bugzilla/WebService/Bug.pm line 32.
BEGIN failed--compilation aborted at Bugzilla/WebService/Bug.pm line 32.
ok 103 - Bugzilla/WebService/User.pm

You will need to "use Bugzilla;" in either Bugzilla/BugMail.pm or
Bugzilla/WebService/Bug.pm.

Dave
Comment 13 Noura El hawary 2008-04-01 08:27:44 EDT
Created attachment 299881 [details]
v2 for log4perl logging code added to BZ 3.2 

Thanks for the review Dave.. new patch for log4perl is attached , with fixes
you suggested , also it includes logging added to the extention webservice.pl
and all the xmlrpc functions under WebService/*.pm , and logging added to
Bugzilla/Mailer.pm.

Please review and let me know what you think.

Noura
Comment 14 Noura El hawary 2008-04-01 08:30:02 EDT
5 hours worked, added logging to the extension webservice.pl and to the actual
WebService interface + testing + logging added to Bugzilla/Mailer.pm
Comment 15 David Lawrence 2008-04-01 10:39:27 EDT
Comment on attachment 299881 [details]
v2 for log4perl logging code added to BZ 3.2 

Looks good Noura. Feel free to check in if Kevin doesnt see anything wrong.

Dave
Comment 16 Noura El hawary 2008-04-01 20:59:56 EDT
patch is not committed to cvs. Closing bug
Comment 17 Noura El hawary 2008-04-15 00:11:32 EDT
changing bug to MODIFIED as it hasn't been out yet in a milestone

Note You need to log in before you can comment on or make changes to this bug.