Bug 44308 - [Patch] JMX component for managing Logger configurations
Summary: [Patch] JMX component for managing Logger configurations
Status: NEW
Alias: None
Product: Log4j - Now in Jira
Classification: Unclassified
Component: Other (show other bugs)
Version: 1.2
Hardware: All All
: P2 enhancement
Target Milestone: ---
Assignee: log4j-dev
URL:
Keywords: PatchAvailable
: 36860 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-01-28 06:17 UTC by Stefan Fleiter
Modified: 2009-10-10 10:25 UTC (History)
1 user (show)



Attachments
Path to manage Loggers per JMX (21.50 KB, patch)
2008-01-28 06:19 UTC, Stefan Fleiter
Details | Diff
Refactored patch to provide an AbstractModelMBean base class (26.59 KB, patch)
2008-01-29 02:31 UTC, Stefan Fleiter
Details | Diff
Patch including RollbackableLoggerManagerModelMBean (30.81 KB, patch)
2009-02-21 14:04 UTC, Stefan Fleiter
Details | Diff
RollbackableLoggerManager#destroy now calls super.destroy (30.84 KB, patch)
2009-02-21 14:15 UTC, Stefan Fleiter
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Fleiter 2008-01-28 06:17:55 UTC
Log4j comes with HierarchyDynamicMBean for managing Log4j by means of JMX.

This class has the following restrictions:
- It can not be registered more than once at the same MBean server, this
  is a critical restriction in J2EE Environments.
- Only loggers specified in the Log4j configuration file can be configured.
- The code is overly complex.

Since these restrictions are by design I have implemented a more straight 
forward solution to this problem.

LoggerManager:             Class with Methods for managing loggers
LoggerManagerModelMBean:   Model MBean which makes LoggerManager methods
                           available per JMX
RollbackableLoggerManager: Extension to LoggerManager which records intial state
                           for all changed loggers and offers a method to
                           roll them back
Comment 1 Stefan Fleiter 2008-01-28 06:19:00 UTC
Created attachment 21436 [details]
Path to manage Loggers per JMX
Comment 2 Stefan Fleiter 2008-01-28 06:22:03 UTC
I forgot to mention that I would like to contribute this patch under the
Apache License, Version 2.0.
Comment 3 Stefan Fleiter 2008-01-29 02:31:45 UTC
Created attachment 21442 [details]
Refactored patch to provide an AbstractModelMBean base class

I have refactored the previous patch to provide a AbstractModelMBean base
class.
The code is more clean and concerns are separated.
The primary goal of the refactoring was to make LoggerManagerModelMBean easily
extendable if anybody has further needs in this direction.
Comment 4 Paul Smith 2008-02-07 18:35:53 UTC
Ok, I _finally_ had time to sit down and review this.  It does look quite
substantial and generally well thought out.

Here are my comments:

RollbackableLoggerManager
	Ordering of rollback, should this be stack based instead?
	Be nice to indicate the size of the rollback state (number of undos) via jmx
	Operation to rollback just the most recent?
	
LoggerManager
	getLevelByName - stylistic, but should that method declare the RuntimeException?
	since this class is designed for extension, should some thought be made towards
making some of the methods final? (Effective Java)
	setRootLogLevel - declaring throwing RuntimeException?
	getConfiguredLoggers - is there an off-by-1 error there in th efor loop with
the ++i instead of i++  ?
	
LoggerManagerModelMBean
	+        try {
	+            getDelegate().getClass().getMethod("rollbackLogConfiguration", null);
	+            modelMBeanOperations.add(rollbackLogConfiguration);
	+        } catch (NoSuchMethodException expected) {
	+            // nothing to do
	+        }
	This seems a bit of a hack! :)   Given RollbackableLoggerManager extends from
LoggerManager, perhaps the same thought could be done to having a second
RollbackableLoggerManagerModelMBean that has a doPostRegisterOtherSchtuff()
style methods.
	
	
	
Comment 5 Stefan Fleiter 2008-02-08 01:55:38 UTC
(In reply to comment #4)

> RollbackableLoggerManager
> 	Ordering of rollback, should this be stack based instead?

The intention of Rollback was only to get Log4j in the state it was initially
configured. Since Log4j itself does not offer such an operation I implemented
this myself.
The order of the modifications of the levels of the logger instances is
not important for this purpose since Log4j determines the effective Level
with every logging action.
Since we do not set the effective level (which is not even possible) the order
of the undo operations does not matter.

> 	Be nice to indicate the size of the rollback state (number of undos)
>       via jmx Operation to rollback just the most recent?

I can add a method to show the size of the rollback information, no problem.
The undo information can be smaller than the modifications made
since only for the first change of a logger its' initial level is saved.
I do not know if there is an advantage of doing partial undo.
The purpose of rollback was to have the possibility to change the log levels
of production machines for a debugging session and to have a single operation
which can undo all changes to get it in the same state as the other nodes.

Partial undo would require a complete changed implementation of the 
RollbackableLoggerManager since I do only save the initial states.
One could do this with a MultiMap but I do not see a usecase for partial undo.
What usecase did you think of?
 	
> LoggerManager
> 	getLevelByName - stylistic, but should that method declare the
>       RuntimeException?

It's only for documentation purposes, if you prefer not to declare 
RuntimeExceptions I have no problems removing those declarations.

> 	since this class is designed for extension, should some thought be made 
>       towards making some of the methods final? (Effective Java)

What methods have you thought of?
When I start to think about this I could rename doSetLogLevel
to setLogLevelInternal and call a doSetLogLevelInternal template method.
Then one could make setLogLevelInternal final.
But even that is difficult because the extending class could wish to veto
the modification.
I am not a big fan of the final keyword because there always is somebody
who wants to do something with your class you have never thought of.

But if you feel strongly for making some method(s) final I will do this.

> 	setRootLogLevel - declaring throwing RuntimeException?

See above, no problem removing the declaration.

> 	getConfiguredLoggers - is there an off-by-1 error there in the for loop 
>       with the ++i instead of i++  ?

I hope I understand you correctly, but Java language specification 3 states
explicitly under 14.14.1.2, that the update part of the for statement (third
part in parentheses) is executed after the body/statement.
Therefor ++i and i++ have the same effect.
I got used to prefer pre-increment to post-increment if there is no difference
in their effects when programming C++.
So this can be changed but I do not see the mentioned off by one bug.


> LoggerManagerModelMBean
> 	+        try {
> 	+            getDelegate().getClass().getMethod("rollbackLogConfiguration",
null);
> 	+            modelMBeanOperations.add(rollbackLogConfiguration);
> 	+        } catch (NoSuchMethodException expected) {
> 	+            // nothing to do
> 	+        }
> 	This seems a bit of a hack! :)   Given RollbackableLoggerManager extends 
>       from LoggerManager, perhaps the same thought could be done to having a
>       second RollbackableLoggerManagerModelMBean that has a 
>       doPostRegisterOtherSchtuff() style methods.

You got me there. :-)
That really is a hack.
RollbackableLoggerManagerModelMBean could easily override
createModelMBeanOperationInfos of LoggerManagerModelMBean which would
not register that method.

Thanks a lot for your kind review.
Comment 6 Paul Smith 2008-03-13 19:11:47 UTC
(In reply to comment #5)
> 
> > LoggerManagerModelMBean
> > 	+        try {
> > 	+            getDelegate().getClass().getMethod("rollbackLogConfiguration",
> null);
> > 	+            modelMBeanOperations.add(rollbackLogConfiguration);
> > 	+        } catch (NoSuchMethodException expected) {
> > 	+            // nothing to do
> > 	+        }
> > 	This seems a bit of a hack! :)   Given RollbackableLoggerManager extends 
> >       from LoggerManager, perhaps the same thought could be done to having a
> >       second RollbackableLoggerManagerModelMBean that has a 
> >       doPostRegisterOtherSchtuff() style methods.
> 
> You got me there. :-)
> That really is a hack.
> RollbackableLoggerManagerModelMBean could easily override
> createModelMBeanOperationInfos of LoggerManagerModelMBean which would
> not register that method.
> 
> Thanks a lot for your kind review.

Stefan, do you a newer patch available that addresses this point?  I'd be up for applying this if this particular item was addressed.

If you were also interested, 

https://issues.apache.org/bugzilla/show_bug.cgi?id=40246

May well also fit within this scope (and possibly easy to do?).  I couldn't get my local application to do the deRegister, perhaps you would have more success?




Comment 7 Stefan Fleiter 2008-03-24 15:17:57 UTC
I will generate a new patch in week 14, sorry but my time is limited at the moment.
Becuase of this I do not think that I will have the time to look at bug 40246.
Comment 8 Thorbjørn Ravn Andersen 2008-08-02 14:05:55 UTC
Awaiting newer patch.
Comment 9 Stefan Fleiter 2009-02-21 14:04:40 UTC
Created attachment 23286 [details]
Patch including RollbackableLoggerManagerModelMBean

This patch provides both a RollbackableLoggerManagerModelMBean class and the
possibility to rollback the undo stack step by step.
Comment 10 Stefan Fleiter 2009-02-21 14:15:06 UTC
Created attachment 23287 [details]
RollbackableLoggerManager#destroy now calls super.destroy
Comment 11 Stefan Fleiter 2009-02-21 14:21:04 UTC
Sorry it took so long.
The newest patch provides RollbackableLoggerManagerModelMBean as
asked for in comment 6 and even the possibility to rollback step by
step through the loglevel history.

Please tell me if anything is still missing.
I hope to be able to react faster tis time.
Comment 12 Curt Arnold 2009-10-10 10:25:56 UTC
*** Bug 36860 has been marked as a duplicate of this bug. ***