You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by Philippe Mouawad <ph...@gmail.com> on 2017/02/04 21:15:17 UTC
Re: Want to help with migrating from Apache LogKit to SLF4J
Hi,
Anybody had a chance to review PR ?
Thanks
On Mon, Jan 30, 2017 at 8:47 PM, Philippe Mouawad <
philippe.mouawad@gmail.com> 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 <
> philippe.mouawad@gmail.com> 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 <wo...@apache.org> wrote:
>>
>>> On Mon, Jan 9, 2017 at 1:29 PM, Woonsan Ko <wo...@apache.org> wrote:
>>> > On Fri, Jan 6, 2017 at 3:23 PM, Philippe Mouawad
>>> > <ph...@gmail.com> wrote:
>>> >> On Wednesday, January 4, 2017, sebb <se...@gmail.com> wrote:
>>> >>
>>> >>> On 3 January 2017 at 20:59, Woonsan Ko <woonsan@apache.org
>>> <javascript:;>>
>>> >>> wrote:
>>> >>> > On Tue, Jan 3, 2017 at 2:32 PM, Felix Schumacher
>>> >>> > <felix.schumacher@internetallee.de <javascript:;>> wrote:
>>> >>> >> Am 02.01.2017 um 21:31 schrieb Philippe Mouawad:
>>> >>> >>>
>>> >>> >>> On Monday, January 2, 2017, Woonsan Ko <woonsan@apache.org
>>> >>> <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.
Re: Want to help with migrating from Apache LogKit to SLF4J
Posted by Philippe Mouawad <ph...@gmail.com>.
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 <philippe.mouawad@gmail.com
> 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 <
> philippe.mouawad@gmail.com> wrote:
>
>> Hi,
>> Anybody had a chance to review PR ?
>> Thanks
>>
>> On Mon, Jan 30, 2017 at 8:47 PM, Philippe Mouawad <
>> philippe.mouawad@gmail.com> 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 <
>>> philippe.mouawad@gmail.com> 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 <wo...@apache.org> wrote:
>>>>
>>>>> On Mon, Jan 9, 2017 at 1:29 PM, Woonsan Ko <wo...@apache.org> wrote:
>>>>> > On Fri, Jan 6, 2017 at 3:23 PM, Philippe Mouawad
>>>>> > <ph...@gmail.com> wrote:
>>>>> >> On Wednesday, January 4, 2017, sebb <se...@gmail.com> wrote:
>>>>> >>
>>>>> >>> On 3 January 2017 at 20:59, Woonsan Ko <woonsan@apache.org
>>>>> <javascript:;>>
>>>>> >>> wrote:
>>>>> >>> > On Tue, Jan 3, 2017 at 2:32 PM, Felix Schumacher
>>>>> >>> > <felix.schumacher@internetallee.de <javascript:;>> wrote:
>>>>> >>> >> Am 02.01.2017 um 21:31 schrieb Philippe Mouawad:
>>>>> >>> >>>
>>>>> >>> >>> On Monday, January 2, 2017, Woonsan Ko <woonsan@apache.org
>>>>> >>> <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.
Re: Want to help with migrating from Apache LogKit to SLF4J
Posted by Philippe Mouawad <ph...@gmail.com>.
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 <
philippe.mouawad@gmail.com> wrote:
> Hi,
> Anybody had a chance to review PR ?
> Thanks
>
> On Mon, Jan 30, 2017 at 8:47 PM, Philippe Mouawad <
> philippe.mouawad@gmail.com> 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 <
>> philippe.mouawad@gmail.com> 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 <wo...@apache.org> wrote:
>>>
>>>> On Mon, Jan 9, 2017 at 1:29 PM, Woonsan Ko <wo...@apache.org> wrote:
>>>> > On Fri, Jan 6, 2017 at 3:23 PM, Philippe Mouawad
>>>> > <ph...@gmail.com> wrote:
>>>> >> On Wednesday, January 4, 2017, sebb <se...@gmail.com> wrote:
>>>> >>
>>>> >>> On 3 January 2017 at 20:59, Woonsan Ko <woonsan@apache.org
>>>> <javascript:;>>
>>>> >>> wrote:
>>>> >>> > On Tue, Jan 3, 2017 at 2:32 PM, Felix Schumacher
>>>> >>> > <felix.schumacher@internetallee.de <javascript:;>> wrote:
>>>> >>> >> Am 02.01.2017 um 21:31 schrieb Philippe Mouawad:
>>>> >>> >>>
>>>> >>> >>> On Monday, January 2, 2017, Woonsan Ko <woonsan@apache.org
>>>> >>> <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.