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.
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
Before you implement the Log4perl logic we should first see if the upstream will accept a patch for it.
(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
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
(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
(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
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
worked 5 hours
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.
(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 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
[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
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
5 hours worked, added logging to the extension webservice.pl and to the actual WebService interface + testing + logging added to Bugzilla/Mailer.pm
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
patch is not committed to cvs. Closing bug
changing bug to MODIFIED as it hasn't been out yet in a milestone