[coldbox:4226] LogBox Log Level inconsistencies

Geez, I won’t shut up, will I? :slight_smile: At least this time I have answers and not just questions.

Luis, line 76 of LogBoxConfig.cfc reads as such:
appender(argumentCollection=logBoxDSL.appenders[key]);

However, I believe it should look like this:
appender(argumentCollection=convertLevels(logBoxDSL.appenders[key]));

The LogBox levelMin and levelMax settings are supposed to allow for a numeric representation OR the English word. The rest of the loadDataDSL and parseAndLoad methods use this.logLevels.lookupAsInt() and/or convertLevels() to change the word versions to the numeric version. Line 76, however, does not do the conversion which causes an error for a configuration like so:

appenders =
{
logFile =
{
class=“logbox.system.logging.appenders.AsyncRollingFileAppender”,
levelMin = “ERROR”,
levelMax = “DEBUG”
}
}

This also got me thinking. Let’s say you like to use the Programmatic Approach where you call the methods of the config object directly like so:

props = {filePath=expandPath("/coldbox/testing/cases/logging/tmp"),autoExpand=false,fileMaxArchives=1,fileMaxSize=3};
config.appender(name=‘MyAsyncFile’,class=“logbox.system.logging.appenders.AsyncRollingFileAppender”,properties=props,levelMin=“ERROR”,levelMax=“INFO”);

This method suffers from the same problem since the config.appender() method requires an int be passed in and errors on the word “ERROR” or “INFO”. Now, the obvious solution here is that the programmatic approach (as opposed to the XML and Configuration CFC approach) can simply access your pseudo-statics in LogLevels like config.logLevels.INFO, config.logLevels.DEBUG, etc. However, I’m not sure that it is exceedingly obvious or necessary for the different configuration methods to accept different inputs.

As a solution, you could modify the “appender”, “root”, and “category” methods in LogBoxConfig to accept a string for the levelMin and levelMax arguments, and then run lookupAsInt() at that point. Since all three configuration methods all flow through this point, you wouldn’t have to convert any of the levels at the XML or DSL level as they would get caught here at this one singular location.

And, sorry to do this, but the more code I read, the more little things I notice. The “category” method and “root” method run levelChecks() to ensure levelMin and levelMax are valid integers. However, “appender” does not run levelChecks() which means I can pass in a levelMax of 50 and it will not catch it like it will for a category.

And speaking of the levelCheck() method in LogLevels.cfc, I don’t think it works.
return ( arguments.level gte this.MINLEVEL OR arguments.level lte this.MAXLEVEL );
I’m pretty certain this statement will ALWAYS return true. I think the OR should be an AND.

Ok, two more items and I promise I’ll quit for tonight.

Firstly, the log levels appear to be completely ignored for appenders. They make it into instance.config in LogBox.cfc’s configure() method, and they are passed into registerAppender(), which in turn sends them in the argumentCollection to the init for the appender. However, unless the concrete class does something with them, the abstract base appender class’s init ONLY uses the name, properties, and custom layout arguments. levelMin and levelMax fall on deaf ears and every appender ends up with the default max and min set at the top of the component regardless of what I put in the config.

And finally, this last item applies potentially to how you might implement the prior bug as well as how the Logger.cfc component is written. Both AbstractAppender.cfc and Logger.cfc’s setLevelMax() and setLevelMin() methods enforce a bit of validation to ensure that the min level is actually less or equal to the max and vice versa. However, in the init for Logger.cfc you bypass the setters and simply set the arguments directly into the instance struct. This means that maxs and mins set after the fact will have to obey the rules, but the initial config data doesn’t have to. The simple answer seemed to have the init method use the proper setters, but unfortunately that opened a Pandora’s box of problems. Firstly Logger.cfc’s level setters error on init because they require this.logLevels to exist and logLevels isn’t set in by LogBox until the Logger is initted. If I modify LogBox.cfc to change oRoot’s creation in “configure” and oLogger’s creation in “getLogger” to run the init AFTER logLevels has been injected, than the first level setter called still errors because you defaulted instance.levelMin and instance.levelMax to non-numeric values at the top of the component.

I recommend Logger.cfc be changed to work like the AbstractAppender where this.logLevels is created internally as soon as the component is instantiated and the instance.levelMin and instance.levelMax is immediately set to this.logLevels.FATAL and this.logLevels.DEBUG. This will allow your max and min setters to work in init so they can enforce their validation rules.

Ok, I’m done. Really, I am. Let me know if you need any clarification. I didn’t mean for this to be a brain dump, but the more code I read the more issues I found. I tested all my suggestions on my install and they seem to work fine.
Keep up the awesome work Luis. I’m loving LogBox!

~Brad

Hi Brad,

Thanks, first point was logged as a ticket and fixed on SVN now.

This method suffers from the same problem since the config.appender() method requires an int be passed in and errors on the word “ERROR” or “INFO”. Now, the obvious solution here is that the programmatic approach (as opposed to the XML and Configuration CFC approach) can simply access your pseudo-statics in LogLevels like config.logLevels.INFO, config.logLevels.DEBUG, etc. However, I’m not sure that it is exceedingly obvious or necessary for the different configuration methods to accept different inputs.

Yes, totally right on. It is awesome what a pair of fresh eyes does :slight_smile: I have logged it and fixed on svn. and also updated the tests.

And speaking of the levelCheck() method in LogLevels.cfc, I don’t think it works.
return ( arguments.level gte this.MINLEVEL OR arguments.level lte this.MAXLEVEL );
I’m pretty certain this statement will ALWAYS return true. I think the OR should be an AND.

Right on Brad. I have fixed it and updated the unit tests for this.

Appender not doing level checks
Again, right on and fixed it.

irstly, the log levels appear to be completely ignored for appenders. They make it into instance.config in LogBox.cfc’s configure() method, and they are passed into registerAppender(), which in turn sends them in the argumentCollection to the init for the appender. However, unless the concrete class does something with them, the abstract base appender class’s init ONLY uses the name, properties, and custom layout arguments. levelMin and levelMax fall on deaf ears and every appender ends up with the default max and min set at the top of the component regardless of what I put in the config.

I just saw this luis slaps forehead, I know added it to the constructor like it should, thanks!

As for the last point, yes, tottaly right, I updated the code to now be consistent throughout. THanks Brad, you single handedly made a complete dot release!! WOOT!!

Luis F. Majano
President
Ortus Solutions, Corp

ColdBox Platform: http://www.coldbox.org
Linked In: http://www.linkedin.com/pub/3/731/483
Blog: http://www.luismajano.com
IECFUG Manager: http://www.iecfug.com