Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Summary: Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward com...
Status: RESOLVED FIXED
Alias: None
Product: JMeter
Classification: Unclassified
Component: Main (show other bugs)
Version: 3.1
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords: PatchAvailable
Depends on:
Blocks: 60664
  Show dependency tree
 
Reported: 2017-01-15 16:34 UTC by Woonsan Ko
Modified: 2017-02-16 20:06 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Woonsan Ko 2017-01-15 16:34:07 UTC
First, I think the goals are as follows:

# Goals

G1. Drop dependencies on logkit, exclalibur and logkit.
G2. Allow logging configuration with other popular logging framework configuration such as log4j2. log4j2 is the primary one to support.
  (It should be able to support logback, but that's out of scope in this story. That can be part of other new story later.)
G3. Keep the existing UI functionalities.
G4. Keep the backward compatibility for 3rd party plugin modules.
G5. Ensure all the code use slf4j for logging (exception: usages of logkit logger interface for backward compatibility). 

Second, let me try to list all the existing logging related features of JMeter

# Current Logging Related Features

CF1. Users are able to configure log file, logging format, log levels for the combined root logger or for each category in the JMeter configuration properties file or command line arguments (`-j' for log file and `-L' for log level).
CF2. Logs by libraries using Commons Logging are passed to the JMeter logging (logkit) by JMeter's Commons Logging implementation.
CF3. Advanced Excalibur logging configuration is possible through `log_config' property in JMeter configuration properties.
CF3. Logs Viewer UI Menu can be turned on to receive logging event and show in the UI panel (org.apache.jmeter.gui.LoggerPanel). Note this is not reading the log file directly, but receives and renders log events currently propagated by the underlying logging framework at runtime only.
CF4. For libraries using log4j API directly, log4j system property is set with `log4j.conf' configuration properties by default, but the logs through log4j is not used by JMeter, it's simply appended to a separate log file.

Since the goals include removing dependencies on logkit, excalibur and avalon, and introducing log4j2 logging framework, I'd like to propose the following in the feature level.

# Proposal in Feature Level

PF1. Users are able to configure log file, logging format, log levels, etc. for combined root logger and for each logger in a separate log4j2 configuration file, ./log4j2.xml or bin/log4j2.xml.
PF2. Users are able to change the log4j2 configuration by passing the standard log4j2 system property (e.g, `-Dlog4j2.configuration=file:/var/conf/log4j2-pt1.xml').
PF3. Logs Viewer UI Menu keeps the same functionality as it does.
PF4. All the logging related configuration in JMeter configuration properties will be dropped. Also, the logging related command line arguments (`-j' for log file and `-L' for log level) will be dropped.
PF5. Advanced Excalibur logging configuration support will be dropped.
PF6. JMeter Commons Logging provision will be dropped. Instead jcl-over-slf4j jar dependency will be provided for the same feature.
PF7. log4j support through `log4j.configuration' system property and `log4j.conf' will be dropped. Instead Log4j 2's log4j-1.2-api.jar [1] will be provided for backward compatibility.

# Candidate solutions
- I'll describe solution ideas in comments.

-----
[1] https://logging.apache.org/log4j/2.x/manual/migration.html
Comment 1 Woonsan Ko 2017-01-15 16:54:12 UTC
My solution ideas (roughly):
- NewDriver.java can determine (default) log4j2 configuration file location (e.g, first ./log4j2.xml, second bin/log4j2.xml) if log4j2 configuration system property is not explicitly set by users.
- Add jcl-over-slf4j jar and Log4j 2's log4j-1.2-api.jar dependencies for Commons Logging and Log4j using libraries.
- Remove logkit, excalibur and avalon dependencies.
- Fork logkit Logger interface with a wrapper class for slf4j logger for backward compatibility in jorphan. So we can remove logkit dependencies in maven pom as well.
- Change LoggingManager only for using wrappers for slf4j. Drop logging initialization.
- Remove all the logging related JMeter configuration properties and command line arguments support.
- Provide logging event listener for the Logs Viewer UI feature).
  (This can be a log4j2 listener implementation since I don't see any listener support by slf4j itself (yet).)
Comment 2 Woonsan Ko 2017-01-15 18:47:04 UTC
(In reply to Woonsan Ko from comment #1)

> - Provide logging event listener for the Logs Viewer UI feature).
>   (This can be a log4j2 listener implementation since I don't see any
> listener support by slf4j itself (yet).)

One solution is to provide a custom log4j2 Appender implementation, as the most minimal direct log4j2 (runtime) dependency, for the Logs Viewer UI. So, users are supposed to configure this custom appender and reference in to root logger to keep the same functionality. The custom appender can possibly propagate events with the logging events.
Comment 3 Philippe Mouawad 2017-01-15 22:11:55 UTC
Thanks a lot for this full and great analysis !

I am ok with all points.
Wait a bit for other committers feedback before starting work.

Thank you

Regards
Comment 4 Woonsan Ko 2017-01-20 06:14:28 UTC
I've created an initial pull request. This is not ready for merging, but only for initial review on the direction:
- https://github.com/apache/jmeter/pull/254

This PR contains only the following at the moment:
- Removing dependencies on logkit, excalibur and avalon.
- Forking some logkit classes in jorphan.
- NewDriver.java setting log4j2 configuration file location system property.
- Add jcl-over-slf4j, log4j-1.2-api, jcl-over-slf4j and log4j2 jars.
- Update LoggingManager to return slf4j logger wrapper.

It doesn't include a custom appender for the UI and unit test fixes yet. Also requires cleanups.

But it seems working at runtime with log4j2 now in this steps:
$ git clone -b feature/bz60589-1 https://github.com/woonsan/jmeter.git
$ cd jmeter
$ ant download_jars
$ ant
$ cd bin
$ ./jmeter
Now, I see jmeter.log being added by log4j2 via logkit fork Logger (wrapping slf4j logger) / LoggingManager.

Please review the PR and share your thoughts on the direction.

P.S. The original logkit Logger is a class (not an interface) with final methods. So, I decided to remove the final modifier and add abstract modifier for overridable methods (for wrapper impl). And I kept unsupportable methods as NOP impl.
Comment 5 Philippe Mouawad 2017-01-21 08:11:19 UTC
(In reply to Woonsan Ko from comment #4)
> I've created an initial pull request. This is not ready for merging, but
> only for initial review on the direction:
> - https://github.com/apache/jmeter/pull/254
> 
> This PR contains only the following at the moment:
> - Removing dependencies on logkit, excalibur and avalon.
> - Forking some logkit classes in jorphan.
> - NewDriver.java setting log4j2 configuration file location system property.
> - Add jcl-over-slf4j, log4j-1.2-api, jcl-over-slf4j and log4j2 jars.
> - Update LoggingManager to return slf4j logger wrapper.
> 
> It doesn't include a custom appender for the UI and unit test fixes yet.
> Also requires cleanups.
> 
> But it seems working at runtime with log4j2 now in this steps:
> $ git clone -b feature/bz60589-1 https://github.com/woonsan/jmeter.git
> $ cd jmeter
> $ ant download_jars
> $ ant
> $ cd bin
> $ ./jmeter
> Now, I see jmeter.log being added by log4j2 via logkit fork Logger (wrapping
> slf4j logger) / LoggingManager.
> 
> Please review the PR and share your thoughts on the direction.
> 
> P.S. The original logkit Logger is a class (not an interface) with final
> methods. So, I decided to remove the final modifier and add abstract
> modifier for overridable methods (for wrapper impl). And I kept
> unsupportable methods as NOP impl.

The direction looks good to me.
Thanks for your work.

I suppose last step is having:
- custom appender for the UI 
- The ability to set debug level through the menu
- Fix Test Units

Regards
Comment 6 Woonsan Ko 2017-01-26 08:22:30 UTC
Hi,

I think the PR is now ready for review:
- https://github.com/apache/jmeter/pull/254

Here's what I did with this PR in summary:
- Removing dependencies on logkit, excalibur, avalon and commons-logging.
- Adding slf4j and log4j dependencies.
- Forking minimal logkit interfaces/classes in jorphan module.
- NewDriver.java setting system properties for log4j2 configuration file location and the log file path (referenced by system property lookup in log4j2.xml files).
- Migrating logging properties (in jmeter*.properties) to log4j2*.xml files with equivalent comments and examples.
- Remove logkit initialization in LoggingManager and update it to return slf4j logger wrapper simply.
- Fixing some unit tests accordingly as logkit LogTarget is not available in unit testing any more.
- Implement LoggerPanel based on new logging mechanism for full backward compatibility.
- Keep supporting -j command line option to allow log file setting. Almost full backward compatibility. 'LAST' value is dropped since this option handling happens before GUI initialized.
- Keep supporting -L command line option to allow log level setting. Full backward compatibility.
- Introduce -i command line option to allow to change log4j2 configuration file location by command line argument as well as the default log4j2 system property (log4j.configurationFile).
- Minimal updates in getting started documentation.
Comment 7 Woonsan Ko 2017-01-26 08:29:49 UTC
(In reply to Philippe Mouawad from comment #5)

> I suppose last step is having:
> - custom appender for the UI 

It was added and configured in the default log4j2*.xml files.

> - The ability to set debug level through the menu

I'm not sure if we want to do this with this ticket because the UI menu to change log level looks like a new improvement, not part of migration.
The PR includes backward compatible option support to change log level (`-L DEBUG' for example). I'd personally like to improve the UI menu for log level setting in a new ticket.

> - Fix Test Units

Yes, everything seems to be fixed.

Thanks,

Woonsan
Comment 8 Woonsan Ko 2017-01-30 16:03:01 UTC
(In reply to Woonsan Ko from comment #7)
> (In reply to Philippe Mouawad from comment #5)

> > - The ability to set debug level through the menu
> 
> I'm not sure if we want to do this with this ticket because the UI menu to
> change log level looks like a new improvement, not part of migration.
> The PR includes backward compatible option support to change log level (`-L
> DEBUG' for example). I'd personally like to improve the UI menu for log
> level setting in a new ticket.
I've created a new ticket for the UI menu to allow log level changes:
- https://bz.apache.org/bugzilla/show_bug.cgi?id=60664

Woonsan
Comment 9 Philippe Mouawad 2017-02-05 13:42:40 UTC
Author: pmouawad
Date: Sun Feb  5 13:42:22 2017
New Revision: 1781756

URL: http://svn.apache.org/viewvc?rev=1781756&view=rev
Log:
Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Part 1 of PR #254
Contributed by Woonsan Ko

Bugzilla Id: 60589

Added:
    jmeter/trunk/src/core/org/apache/jmeter/gui/logging/
    jmeter/trunk/src/core/org/apache/jmeter/gui/logging/GuiLogEventAppender.java   (with props)
    jmeter/trunk/src/core/org/apache/jmeter/gui/logging/GuiLogEventBus.java   (with props)
    jmeter/trunk/src/core/org/apache/jmeter/gui/logging/GuiLogEventListener.java   (with props)
    jmeter/trunk/src/core/org/apache/jmeter/gui/logging/LogEventObject.java   (with props)
Modified:
    jmeter/trunk/src/core/org/apache/jmeter/JMeter.java
    jmeter/trunk/src/core/org/apache/jmeter/NewDriver.java
    jmeter/trunk/src/core/org/apache/jmeter/gui/GuiPackage.java
    jmeter/trunk/src/core/org/apache/jmeter/gui/LoggerPanel.java
    jmeter/trunk/src/core/org/apache/jmeter/gui/MainFrame.java
    jmeter/trunk/src/core/org/apache/jmeter/gui/action/What.java
    jmeter/trunk/src/core/org/apache/jmeter/samplers/SampleResult.java
    jmeter/trunk/src/core/org/apache/jmeter/util/JMeterUtils.java
Comment 10 Philippe Mouawad 2017-02-05 13:43:43 UTC
Author: pmouawad
Date: Sun Feb  5 13:43:13 2017
New Revision: 1781757

URL: http://svn.apache.org/viewvc?rev=1781757&view=rev
Log:
Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Part 2 of PR #254
Contributed by Woonsan Ko

Bugzilla Id: 60589

Added:
    jmeter/trunk/src/jorphan/org/apache/jorphan/logging/Slf4jLogkitLogger.java   (with props)
    jmeter/trunk/src/jorphan/org/apache/log/
    jmeter/trunk/src/jorphan/org/apache/log/ContextMap.java   (with props)
    jmeter/trunk/src/jorphan/org/apache/log/LogEvent.java   (with props)
    jmeter/trunk/src/jorphan/org/apache/log/LogTarget.java   (with props)
    jmeter/trunk/src/jorphan/org/apache/log/Logger.java   (with props)
    jmeter/trunk/src/jorphan/org/apache/log/Priority.java   (with props)
Modified:
    jmeter/trunk/src/jorphan/org/apache/jorphan/logging/LoggingManager.java
Comment 11 Philippe Mouawad 2017-02-05 13:44:29 UTC
Author: pmouawad
Date: Sun Feb  5 13:44:09 2017
New Revision: 1781758

URL: http://svn.apache.org/viewvc?rev=1781758&view=rev
Log:
Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Part 3 of PR #254
Contributed by Woonsan Ko

Bugzilla Id: 60589

Modified:
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/HttpMirrorServer.java
Comment 12 Philippe Mouawad 2017-02-05 13:45:46 UTC
Author: pmouawad
Date: Sun Feb  5 13:45:14 2017
New Revision: 1781759

URL: http://svn.apache.org/viewvc?rev=1781759&view=rev
Log:
Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Part 4 of PR #254
Contributed by Woonsan Ko

Bugzilla Id: 60589

Added:
    jmeter/trunk/test/src/org/apache/jmeter/gui/logging/
    jmeter/trunk/test/src/org/apache/jmeter/gui/logging/TestGuiLogEventAppender.java   (with props)
    jmeter/trunk/test/src/org/apache/jmeter/util/LogRecord.java   (with props)
    jmeter/trunk/test/src/org/apache/jmeter/util/LogRecordingDelegatingLogger.java   (with props)
Modified:
    jmeter/trunk/test/src/org/apache/jmeter/JMeterVersionTest.java
    jmeter/trunk/test/src/org/apache/jmeter/samplers/TestSampleResult.java
    jmeter/trunk/test/src/org/apache/jorphan/reflect/TestFunctor.java
    jmeter/trunk/test/src/org/apache/jorphan/test/AllTests.java
Comment 13 Philippe Mouawad 2017-02-05 13:48:01 UTC
Author: pmouawad
Date: Sun Feb  5 13:46:09 2017
New Revision: 1781760

URL: http://svn.apache.org/viewvc?rev=1781760&view=rev
Log:
Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Part 5 of PR #254
Contributed by Woonsan Ko

Bugzilla Id: 60589

Added:
    jmeter/trunk/bin/testfiles/log4j2-batch.xml   (with props)
Removed:
    jmeter/trunk/bin/log4j.conf
    jmeter/trunk/bin/logkit.xml
Modified:
    jmeter/trunk/bin/jmeter.properties
    jmeter/trunk/bin/mirror-server.cmd
    jmeter/trunk/bin/mirror-server.sh
    jmeter/trunk/bin/testfiles/jmeter-batch.properties
    jmeter/trunk/bin/user.properties
Comment 14 Philippe Mouawad 2017-02-05 13:52:40 UTC
Author: pmouawad
Date: Sun Feb  5 13:49:29 2017
New Revision: 1781761

URL: http://svn.apache.org/viewvc?rev=1781761&view=rev
Log:
Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Part 6 of PR #254
Contributed by Woonsan Ko

Bugzilla Id: 60589

Removed:
    jmeter/trunk/res/maven/ApacheJMeter_slf4j_logkit.pom

Author: pmouawad
Date: Sun Feb  5 13:51:56 2017
New Revision: 1781762

URL: http://svn.apache.org/viewvc?rev=1781762&view=rev
Log:
Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Part 7 of PR #254
Contributed by Woonsan Ko

Removed:
    jmeter/trunk/src/slf4j-logkit/
Comment 15 Philippe Mouawad 2017-02-05 13:56:45 UTC
Author: pmouawad
Date: Sun Feb  5 13:55:33 2017
New Revision: 1781763

URL: http://svn.apache.org/viewvc?rev=1781763&view=rev
Log:
Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Part 8 of PR 254
This closes #254
Contributed by Woonsan Ko

Bugzilla Id: 60589

Added:
    jmeter/trunk/licenses/bin/jcl-over-slf4j-1.7.22.txt   (with props)
    jmeter/trunk/licenses/bin/slf4j-ext-1.7.22.txt   (with props)
Modified:
    jmeter/trunk/LICENSE
    jmeter/trunk/build.properties
    jmeter/trunk/build.xml
    jmeter/trunk/eclipse.classpath
    jmeter/trunk/lib/   (props changed)
    jmeter/trunk/lib/aareadme.txt
    jmeter/trunk/licenses/bin/README.txt
    jmeter/trunk/res/maven/ApacheJMeter_parent.pom
    jmeter/trunk/xdocs/changes.xml


Author: pmouawad
Date: Sun Feb  5 13:56:05 2017
New Revision: 1781764

URL: http://svn.apache.org/viewvc?rev=1781764&view=rev
Log:
Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Remove mention of commons-logging in docs
Bugzilla Id: 60589

Modified:
    jmeter/trunk/xdocs/usermanual/get-started.xml
Comment 16 Philippe Mouawad 2017-02-05 14:10:19 UTC
Author: pmouawad
Date: Sun Feb  5 14:09:25 2017
New Revision: 1781765

URL: http://svn.apache.org/viewvc?rev=1781765&view=rev
Log:
Bug 60589 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Part 9 of PR 254
Add missing log4j2.xml under svn
Contributed by Woonsan Ko

Bugzilla Id: 60589

Added:
    jmeter/trunk/bin/log4j2.xml   (with props)