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 Mon, 30 Jan 2017 19:47:00 GMT
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/log4j/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.

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