Hello, I'm happy to announce that thanks to the huge work of Woonsan Ko, we've now completed the migration to SLF4J/LOG4J2.
Big thanks to Woonsan ! for the quality of his work and the amount of involvement on this. Regards Philippe On Sun, Feb 5, 2017 at 3:26 PM, Philippe Mouawad <[email protected] > wrote: > Thanks Felix for your review. > I've merged the PR with only modifications related to build and > exclusions/cleanup in build. > I've not taken into account the remarks on PR, feel free to commit and > review. > I've cleaned up aaareadme but did not update it yet. > > Regards > Philippe > > On Sat, Feb 4, 2017 at 10:15 PM, Philippe Mouawad < > [email protected]> wrote: > >> 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. >> >> >> > > > -- > Cordialement. > Philippe Mouawad. > > > -- Cordialement. Philippe Mouawad.
