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 Thu, 26 Jan 2017 20:47:00 GMT
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.
>

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