[slf4j-dev] slf4j i8ln

[Ralph Goers]

I found a few minutes to review the code this evening. There are  
several things I don't care for in this implementation:
1. While doable, plugging in new resolver implementations is kind of a  
pain. You have to create an I18NLogManager, create the custom  
resolvers and presumably new annotation resolvers, add them to the  
CompositeResolvers then add those to the manager. Finally a new  
I18nLoggerFactory has to be created taking the manager and setINSTANCE  
called.
2. The resolvers provide no mechanism for reloading the bundles if  
they are modified. Many environments want 24x7 up time and do not want  
to restart servers just to change message text.
3. The implementation only supports a single Locale - the default.
4. I dislike immensely having the log level in one property file and  
the message text in another. This is very error prone. If you want log  
levels then get rid of the property files and switch to using XML  
where the log level and message can be bound together. Of course, this  
runs the risk that the level might be different from one language to  
another. Another option - which makes far more sense to me - is to not  
even have a LogLevelResolver, just require the log level be defined in  
the enum and just use that.
5. Messages are always resolved in the writeLog method. This precludes  
the option of the message keys being written to a database so the  
message can be retrieved based on the user's locale or passing the  
message to different machines where it can be logged using a Locale  
appropriate for that environment.
6. If you really, really want to use ResourceBundle's then at least  
make Java 6 the minimum version and provide support for  
ResourceBundle.Control. However, since ResourceBundles don't provide  
support for reloading I'd avoid using them at all.
7. It seems very strange to have the LogLevel enum have log methods  
and that writeLog is actually calling them.
8. The callerData is going to be useless since the LogLevel enum is  
just using the standard SLF4J apis. Instead, the Logger needs to  
extend org.slf4j.ext.LoggerWrapper and actually log from that class.
9. The format of the LogMessages enum is:

public enum LogMessages {

	@Error("error message {}") // The log level is bound to Error.
	TEST0001,				

	@Message("waring message {}") // The log level is not specified.
	TEST0002

}

a. why would you want to allow a log level to not be specified? (See  
item 4 above). It is now possible to not specify the level in the  
properties file and not specify it as an annotation. If that happens  
then the level  will be info since that is what is hardcoded in  
I18NLoggerFactory. What is more annoying is that I can call  
logger.warn and generate an info level message.
b. Other than showing that you know how to use annotations I don't see  
what the benefit is over doing something like:

public enum LogMessages {
   private final LogLevel level;
   private final String message;

   public LogMessages(LogLevel lvl, String msg) {
     level = lvl;
     message = msg;
   }

   public LogLevel getLogLevel()  { return level };
   public String getMessage() { return message };

   TEST0001(LogLevel.ERROR, "error message {}");
   TEST0002(LogLevel.WARN, "warning message {}"};

}

I only spent about an hour looking at this so this may not be the  
complete set of issues I might find.

In short, this implementation is not very general purpose as it only  
supports a single use case.

Ralph



On Aug 22, 2009, at 10:32 PM, Takeshi Kondo wrote:

> <slf4j-i18n-1.5.9-SNAPSHOT.jar><slf4j-i18n-1.5.9-SNAPSHOT-sources.jar>
>
> I've developed initial thought of SLF4j's i18n extension.
> It was committed to my branch (http://github.com/takeshi/slf4j/tree/master 
> ).
>
> I've implemented 4 features as follows.
>
> 1. Logger interface using enum.
> @see org.slf4j.i18n.I18NLogger
>
> 2. Extension point to bind log id's enum to log message and level.
> @see org.slf4j.i18n.spi.LogLevelResolver
> @see org.slf4j.i18n.spi.LogMessageFormatResolver
>
> 3. Resolving log message and log level from Annotation associated  
> from log id's enum.
> @see org.slf4j.i18n.impl.AnnotationLogLevelResolver
> @see org.slf4j.i18n.impl.AnnotationMessageFormatResolver
>
> 4. Resolving log message and log level from ResourceBundle  
> associated with log id's enum.
> @see org.slf4j.i18n.impl.ResourceBundleLogLevelResolver
> @see org.slf4j.i18n.impl.ResourceBundleMessageFormatResolver
>
>
> For example:
> ----
> Log Message Definition
> ----
> public enum LogMessages {
>
> 	@Error("error message {}") // log level is bound to Error.
> 	TEST0001,				
>
> 	@Message("waring message {}") // log level is not specified.
> 	TEST0002
>
> }
>
> ----
> Logging
> ----
>
> public static void main(String[] args) {
> 	I18NLogger logger = I18NLoggerFactory.getLogger(LogMain.class);
>
> 	// write to error log , because LogMessages.TEST0001 is bound to  
> Error level.
> 	logger.log(LogMessages.TEST0001, "xxxx");
> 	logger.log(LogMessages.TEST0001, new NullPointerException());
> 	
> 	// write log as error level.
> 	logger.error(LogMessages.TEST0002, "xxxx");
> 	// write log as warn level.
> 	logger.warn(LogMessages.TEST0002, new NullPointerException());
> }
>
>
> Takeshi Kondo
>
> _______________________________________________
> dev mailing list
> dev at slf4j.org
> http://www.slf4j.org/mailman/listinfo/dev