Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 365302

Summary: jmx mbeans for logging need to be enhanced
Product: [RT] Jetty Reporter: Thomas Becker <tbecker>
Component: serverAssignee: Thomas Becker <tbecker>
Status: RESOLVED FIXED QA Contact:
Severity: enhancement    
Priority: P3 CC: gregw, jetty-inbox, simone.bordet
Version: unspecified   
Target Milestone: 7.5.x   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
proposed patch
none
jetty-jmx.xml preconfig
none
first commit - code format
none
second commit - actual patch none

Description Thomas Becker CLA 2011-12-01 07:09:08 EST 
Build Identifier: 

We need to improve the naming of the loggers and provide operations to set loglevels and addd more loggers at runtime. E.g. it should be possible to add a new logger for a specific package/class and change it's loglevel via a jmx operation.

Currently we've an mbean per stderr logger.

It'll be better to have:

- A single mbean containing a String[] of all loggers
- operation to add a new logger
- operation to change loglevels of existing loggers

Reproducible: Always
Comment 1 Thomas Becker CLA 2011-12-04 12:00:07 EST 
Created attachment 207887 [details]
proposed patch

Patch adds a MBean for configuring Loggers as described above.
Comment 2 Thomas Becker CLA 2011-12-05 04:16:46 EST 
Created attachment 207904 [details]
jetty-jmx.xml preconfig

Second patch attached for jetty-jmx.xml configuration changes.
Comment 3 Greg Wilkins CLA 2012-01-08 21:51:41 EST 
Thomas,

There are a few things that I'd like changed in this.

Firstly the MBean should be implemented in the same pattern as our other MBeans: Just create the Logger object and an mbean properties file and let the ObjectMBean do the rest (this will also provide some doco in the mbean descriptor).

You then only need to add the Logger instance to the Jetty server container for it's mbean to be auto generated.

I also don't like the logger listener stuff.   I don't think it is necessary, and the Logging object just pull state from the Logs rather than have the state pushed to it.   It is an infrequent event to query the Logging mbean, so efficiency is less important than simplicity.

Also, there is a lot of reformatting in the patches.  Generally speaking I don't like reformat commits - they just screw up history.   Are you  using the format from svn+ssh://dev.eclipse.org/svnroot/rt/org.eclipse.jetty/admin/jetty-eclipse-java-format.xml     If so, there should be very little reformatting.  Even if there is reformatting needed, I'd much rather have a clean history than "correct" spaces around an = etc.
Comment 4 Thomas Becker CLA 2012-01-09 04:27:43 EST 
Ok, will refactor accordingly.
Comment 5 Thomas Becker CLA 2012-01-11 08:40:56 EST 
Created attachment 209309 [details]
first commit - code format
Comment 6 Thomas Becker CLA 2012-01-11 08:41:47 EST 
Created attachment 209310 [details]
second commit - actual patch

Hi Greg,

as requested I've started from scratch using ObjectMBean. Patch attached.

Cheers,
Thomas
Comment 7 Greg Wilkins CLA 2012-01-11 20:43:34 EST 
Thomas,

there are still problems with this.

firstly the __logger map needs to be concurrent.  Also we already have a __logger map in StdErrLog, so perhaps we can avoid duplicate code.

there is also a problem with the fact that we have a Logger.getLogger(name) API that creates logs directly - this will bypass the logic you have in Log.getLogger(String).  Generally it is better to build such data structures on create rather than lookup.

finally, after your patch is applied, the NamedLogTest fails.


I'll have a look at these issues now.
Comment 8 Greg Wilkins CLA 2012-01-12 04:41:30 EST 
I have created an AbstractLogger that implements getLogger(String) with a pattern that ensure atomic creation of missing loggers.
Comment 9 Thomas Becker CLA 2012-01-12 09:44:39 EST 
Reviewed the changes. Marking issue as resolved.
Comment 10 Greg Wilkins CLA 2012-01-12 21:20:48 EST 
any joy working out where the empty StdErrLog mbean is coming from?
Comment 11 Greg Wilkins CLA 2012-01-12 22:01:04 EST 
I found the StdErrLog mbeans being registered as managed beans of the ProxyServlet.  Removed those and also removed the empty mbean properties files for StdErrLog and Slf4JLog
Comment 12 Thomas Becker CLA 2012-01-13 04:29:30 EST 
Oh sorry, I knew about the ProxyServlet mbean for StdErrLog as I was confused about it as well and searched the root of it, but I was offline already yesterday evening. :(