jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@gmail.com>
Subject Re: Want to help with migrating from Apache LogKit to SLF4J
Date Sat, 11 Feb 2017 08:38:02 GMT
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 <woonsan@apache.org> wrote:
>>>>
>>>>> On Mon, Jan 9, 2017 at 1:29 PM, Woonsan Ko <woonsan@apache.org>
wrote:
>>>>> > On Fri, Jan 6, 2017 at 3:23 PM, Philippe Mouawad
>>>>> > <philippe.mouawad@gmail.com> wrote:
>>>>> >> On Wednesday, January 4, 2017, sebb <sebbaz@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.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message