On 01/10/2017 09:57 PM, sebb wrote: > On 9 January 2017 at 18:29, Woonsan Ko <[email protected]> wrote: >> On Fri, Jan 6, 2017 at 3:23 PM, Philippe Mouawad >> <[email protected]> wrote: >>> On Wednesday, January 4, 2017, sebb <[email protected]> wrote: >>> >>>> On 3 January 2017 at 20:59, Woonsan Ko <[email protected] <javascript:;>> >>>> wrote: >>>>> On Tue, Jan 3, 2017 at 2:32 PM, Felix Schumacher >>>>> <[email protected] <javascript:;>> wrote: >>>>>> Am 02.01.2017 um 21:31 schrieb Philippe Mouawad: >>>>>>> On Monday, January 2, 2017, Woonsan Ko <[email protected] >>>> <javascript:;>> wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> I'd like to help with migrating from Apache LogKit to SLF4J [1], and >>>>>>>> so I've been reading the current logging implementation with logkit, >>>>>>>> avalon-framework and excalibur-logger. >>>>>>> Thanks for your proposal >>>>>> +1 >>>>>>> >>>>>>>> From my understanding, maybe we can take the following approach: >>>>>>>> - Since SLF4J API doesn't provide a logging implementation or binding >>>>>>>> by itself, we need to choose one at least such as log4j2 [2] or >>>>>>>> logback. [3] IMHO, log4j2 binding provided by Apache Logging services >>>>>>>> project could be a good default binding option. >>>>>>> >>>>>>> +1 >>>>>> Well, I would start with what we have, which is a working binding for >>>> SLF4J. >>>>> It seems more important to migrate each logger usages to use slf4j >>>>> logger in each class than replacing logging framework in the first >>>>> step. So, we can keep the current logkit binding in the first step. >>>>> That prioritization makes sense to me. >>>>> >>>>>>> >>>>>>>> - By the default binding such as log4j2, I mean JMeter should be >>>>>>>> bundled with log4j2 library and its binding library as well as a >>>>>>>> default configuration file (e.g, log4j2.xml), by default. It seems >>>>>>>> neater to separate the logging configuration file from >>>>>>>> jmeter.properties with simply following its default auto-resolving >>>>>>>> conventions such as log4j2.xml [4] or logback.xml [5] to me. >>>>>>> +1 >>>>>> I am +-0 to any decision right now. >>>>> This can be revisited with separate ticket after the first step done. >>>>> >>>>>>> >>>>>>>> - It seems like each Logger instance is created as a static member by >>>>>>>> `LoggingManager.getLoggerForXXX()' method(s). I suppose all of those >>>>>>>> should be replaced by simply using slf4j logger factory as done in >>>>>>>> AbstractSampleConsumer.java. >>>>>>> Yes >>>>>>> >>>>>>>> - It might be even better if each logging line is more optimized by >>>>>>>> taking advantage of slf4j logging methods (e.g, message format >>>>>>>> arguments and throwable argument). >>>>>>> Yes >>>>>> Plus, if we use formatted messages with arguments, the need for if >>>>>> (log-is-enabled) statements might be gone for simple cases. >>>>> Yes. >>>>> >>>>>>>> - After all migrated, the old logkit and some other related unused >>>>>>>> libraries should be gone. >>>>>>> >>>>>>> A possible option to avoid breaking too many existing third party >>>> plugins >>>>>>> would be to embed in source code current logkit logger factory and >>>> logger >>>>>>> so that it delegates to slf4j. >>>>>>> We would drop avalon, logkit and excalibur jars. >>>>> Good point. This can be revisited in the later step. >>>>> >>>>>>> >>>>>>> >>>>>>>> Am I in the right track? Any advice or thoughts? >>>>>>> >>>>>>> please wait for other team members to answer before starting code. >>>>>>> Give a week . >>>>>> I would start slowly and contribute drop by drop first, to see if you >>>> are >>>>>> going in the right direction. >>>>> You're right. >>>>> Maybe we can split the steps (with separate bz tickets) like the >>>>> following (ordered by priority): >>>>> Step 1: Replace logkit loggers with slf4j ones with keeping the >>>>> current logkit binding solution. >>>>> Step 2: Optimize logging statements. e.g, message format args, >>>>> throwable args, unnecessary if-enabled-logging in simple ones, etc. >> I've created two tickets for the Step 1 and 2: >> - https://bz.apache.org/bugzilla/show_bug.cgi?id=60564 >> - https://bz.apache.org/bugzilla/show_bug.cgi?id=60565 >> >> Each looks straightforward. I'd create separate PRs for each bz ticket. >> But, there's one thing tricky: some classes have public constructor or >> methods requiring logkit Logger, such as: >> - o.a.jmeter.engine.ClientJMeterEngine.tidyRMI(Logger) >> - o.a.jmeter.util.BeanShellInterpreter.BeanShellInterpreter(String, Logger) >> - o.a.jmeter.protocol.jms.Utils.close(MessageConsumer, Logger) >> - o.a.jmeter.protocol.jms.Utils.close(Session, Logger) >> - o.a.jmeter.protocol.jms.Utils.close(Connection, Logger) >> - o.a.jmeter.protocol.jms.Utils.close(MessageProducer, Logger) >> >> I think we have the following options for those: >> (a) Don't change those for backward compatibility, but we need a >> wrapper to wrap slf4j logger as logkit logger. >> (b) Change those to use slf4j logger, breaking bc. >> (c) or sometimes (a) and sometimes (b)? >> >> What do you think? (a) seems safer to me.. > It's obviously safer, but AFAICT it negates the point of migrating? > > I'm not sure JMeter has ever promised BC for all classes. > AFAICR these are mainly or entirely classes that are internal to > JMeter, with the possible exception of jms.Utils. > Is it likely that any 3rd party code would need to use these classes directly? >From my knowledge, no plugins use these specific classes/methods listed above. FYI Typical logging approach in 3rd party plugins is limited to: import org.apache.jorphan.logging.LoggingManager;
import org.apache.log.Logger; private static final Logger log = LoggingManager.getLoggerForClass(); >>>>> Step 3: Drop avalon, logkit and excalibur with backward compatibility >>>>> for 3rd party modules. (This step would require discussions about >>>>> which logging framework/configuration can be used/changed later.) >>>> I still think it's unnecessary. >>> I don't share your point. >>> I think we need to remove the old attic dependencies and use a more up to >>> date framework. >>> Besides some like log4j2 have really important perf improvements. >>> This will also let us reduce code size. >>> >>> >>>> However, the most important aspect is that users are able to use the >>>> logging system to debug problems. >>>> This means that there needs to be updated documentation on how to use >>>> the configuration options. >>>> >>>> I would start with the user-facing items. >>>> i.e documentation >>> +1 >>> >>>> and any user-configuration such as menu options. >>> +0 what exact menu item ? the one that allows settings log level on element >>> ? >>> >>>> Getting the documentation done is critical to the process. >>>> Doing it first should help tease out any so far unforseen issues. >>> +1 >> I will try to figure out what's to be done from end users' perspective >> with some draft possibly. >> At the moment, it seems to have a menu (Option / Log Viewer) only in >> UI which probably reads the configuration for where to load the logs >> from. We will need to figure out how to keep this feature without any >> problem at least if we use log4j2 later, for instance. Anyway, I think >> we can consider this after the step 1 and 2 are done. >> Other UI improvement (e.g, setting log configuration, level, etc) seem >> to be a separate enhancement to me, not necessarily part of this slf4j >> migration. >> >>> >>> I'd also suggest short to medium PRs to avoid conflict if we take some time >>> to integrate them. >> Yes. I'd ask for reviews with the first PR for a smaller set (e.g, >> src/protocol/java/**) first in each bz ticket. If okay, continue with >> the next PR(s) that can include the rest. >> >> Regards, >> >> Woonsan >> >>>>> Regards, >>>>> >>>>> Woonsan >>>>> >>>>>> Regards, >>>>>> Felix >>>>>> >>>>>>>> Kind regards, >>>>>>>> >>>>>>>> Woonsan >>>>>>>> >>>>>>>> [1] >>>>>>>> https://helpwanted.apache.org/task.html? >>>> ad91cbf270c711a1c6aa0e67180309 >>>>>>>> d282c81e02 >>>>>>>> [2] https://logging.apache.org/log4j/2.0/log4j-slf4j-impl/index.html >>>>>>>> [3] http://www.slf4j.org/manual.html >>>>>>>> [4] https://logging.apache.org/log4j/2.x/manual/ >>>>>>>> configuration.html#Automatic_Configuration >>>>>>>> [5] http://logback.qos.ch/manual/configuration.html#auto_ >>>> configuration >>> >>> -- >>> Cordialement. >>> Philippe Mouawad.
