On Mon, Jan 9, 2017 at 1:29 PM, 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.. > >>> > 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've drafted my ideas about the Step 3 considering how users can be affected by it in this ticket: - https://bz.apache.org/bugzilla/show_bug.cgi?id=60589 Please take a review. Regards, Woonsan > >> >> >> 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.
