jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Woonsan Ko <woon...@apache.org>
Subject Re: Want to help with migrating from Apache LogKit to SLF4J
Date Mon, 09 Jan 2017 18:29:41 GMT
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'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
View raw message