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 Sun, 05 Feb 2017 14:26:11 GMT
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.

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