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
Created attachment 21436 [details] Path to manage Loggers per JMX
I forgot to mention that I would like to contribute this patch under the Apache License, Version 2.0.
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.
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.
(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.
(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?
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.
Awaiting newer patch.
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.
Created attachment 23287 [details] RollbackableLoggerManager#destroy now calls super.destroy
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.
*** Bug 36860 has been marked as a duplicate of this bug. ***