Hi, Anybody had a chance to review PR ? Thanks On Mon, Jan 30, 2017 at 8:47 PM, Philippe Mouawad < [email protected]> wrote:
> 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/log >>> 4j/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. > > > -- Cordialement. Philippe Mouawad.
