Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules #4226

Closed
asfimport opened this issue Jan 15, 2017 · 16 comments

Comments

@asfimport
Copy link
Collaborator

Woonsan Ko (Bug 60589):
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

OS: All

Blocks:

@asfimport
Copy link
Collaborator Author

Woonsan Ko (migrated from Bugzilla):
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).)

@asfimport
Copy link
Collaborator Author

Woonsan Ko (migrated from Bugzilla):
(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.

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
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

@asfimport
Copy link
Collaborator Author

Woonsan Ko (migrated from Bugzilla):
I've created an initial pull request. This is not ready for merging, but only for initial review on the direction:

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.

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
(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:

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

@asfimport
Copy link
Collaborator Author

Woonsan Ko (migrated from Bugzilla):
Hi,

I think the PR is now ready for review:

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.

@asfimport
Copy link
Collaborator Author

Woonsan Ko (migrated from Bugzilla):
(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

@asfimport
Copy link
Collaborator Author

Woonsan Ko (migrated from Bugzilla):
(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:

Woonsan

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
Author: pmouawad
Date: Sun Feb 5 13:42:22 2017
New Revision: 1781756

URL: http://svn.apache.org/viewvc?rev=1781756&view=rev
Log:
#4226 - 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

#4226

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

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
Author: pmouawad
Date: Sun Feb 5 13:43:13 2017
New Revision: 1781757

URL: http://svn.apache.org/viewvc?rev=1781757&view=rev
Log:
#4226 - 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

#4226

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

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
Author: pmouawad
Date: Sun Feb 5 13:44:09 2017
New Revision: 1781758

URL: http://svn.apache.org/viewvc?rev=1781758&view=rev
Log:
#4226 - 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

#4226

Modified:
jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/HttpMirrorServer.java

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
Author: pmouawad
Date: Sun Feb 5 13:45:14 2017
New Revision: 1781759

URL: http://svn.apache.org/viewvc?rev=1781759&view=rev
Log:
#4226 - 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

#4226

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

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
Author: pmouawad
Date: Sun Feb 5 13:46:09 2017
New Revision: 1781760

URL: http://svn.apache.org/viewvc?rev=1781760&view=rev
Log:
#4226 - 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

#4226

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

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
Author: pmouawad
Date: Sun Feb 5 13:49:29 2017
New Revision: 1781761

URL: http://svn.apache.org/viewvc?rev=1781761&view=rev
Log:
#4226 - 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

#4226

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:
#4226 - 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/

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
Author: pmouawad
Date: Sun Feb 5 13:55:33 2017
New Revision: 1781763

URL: http://svn.apache.org/viewvc?rev=1781763&view=rev
Log:
#4226 - 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

#4226

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:
#4226 - Migrate LogKit to SLF4J - Drop avalon, logkit and excalibur with backward compatibility for 3rd party modules
Remove mention of commons-logging in docs
#4226

Modified:
jmeter/trunk/xdocs/usermanual/get-started.xml

@asfimport
Copy link
Collaborator Author

@pmouawad (migrated from Bugzilla):
Author: pmouawad
Date: Sun Feb 5 14:09:25 2017
New Revision: 1781765

URL: http://svn.apache.org/viewvc?rev=1781765&view=rev
Log:
#4226 - 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

#4226

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant