Hello, I'll be merging this feature by end of week unless there is a NO GO from somebody. Regards Philippe
On Thu, Jan 26, 2017 at 9:47 PM, Philippe Mouawad < [email protected]> wrote: > Hi All, > Woonsan finalized PR 254. > I reviewed it, it looks ok for me. > > In order to avoid upcoming conflicts (PR concerns 40 files), it would be > nice if somebody else (or more) could review it so that it can be merged > in short terms, before release of 3.2. > > What's your thoughts ? > Thanks > Regards > > > On Sun, Jan 15, 2017 at 5:57 PM, Woonsan Ko <[email protected]> wrote: > >> 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. >> > > > > > -- Cordialement. Philippe Mouawad.
