0023534: slf4j has not been properly used - Openbravo Issue Tracking System

Notes
(0057851) AugustoMauch (administrator) 2013-04-11 17:21	Hi mantaker, How would this improve the logging? We know that SLF4J is an abstraction that allows users to change the logging system, but in Openbravo we only use Log4j for server side logging. Maybe we could make it a feature request, so if implemented other Openbravo developers can more easily change the logging system.

(0057865) AugustoMauch (administrator) 2013-04-12 12:25 edited on: 2013-04-12 12:25	Hi Mantaker, We now know that slf4j is not only useful as an abstraction, but that it will also improve the performance for disabled logging statements [1]. Thank you very much for your contribution, we will probably end up using the migration tool you suggested to use slf4j properly. Best regards [1] http://www.slf4j.org/faq.html#logging_performance [^]

(0057867) mantaker (reporter) 2013-04-12 12:45	I would have suggested you the same link on performance. Anyway I would surely be happy to see slf4j implemented into openbravo, which would essentially make me not to fork the project just for this. On a side note, why is the slf4j library present inside lib/runtime, when you clearly state that the project uses log4j as the server side logging framework? Is it because it is being used by some other project which openbravo is dependent on? Cheers, Manimaran Selvan

(0057868) AugustoMauch (administrator) 2013-04-12 12:54	The slf4j library was included in lib/runtime because it is needed by the hibernate version we are using. At that time we were not aware of the other advantages provided by slf4j, now that we are we will adapt our code to take advantage of them. Again, thanks for you contribution, it is much appreciated.

(0057870) mantaker (reporter) 2013-04-12 13:02	I understand. The migrator should be fine enough to migrate. Please let me know in case you encounter any issues while integrating with slf4j, I would be happy to help. Cheers,

(0057909) AugustoMauch (administrator) 2013-04-15 15:46	Hi, Unfortunately, we are not going to be able to use slf4j in all instances of Logger in Openbravo. In log4j the method debug(Object object) is defined, but slf4j does not support that method, because the first parameter of all debug methods must be a String [1]. Changing this would be an API change. If someone inherits from a log4j logger and uses the debug(Object object) method, would have his code broken when that logger is changed to slf4j. We are planning to change to slf4j all the private instances of the Logger, and we are going to include in our best practices the use of slf4j, but there are some public Logger instances that we are going to have to keep as log4j to avoid breaking the API. We acknowledge that this means that it is not going to be possible to take advantage of slf4j as an abstraction for other logging libraries, we are sorry about that. [1] http://www.slf4j.org/faq.html#string_or_object [^]

(0057912) mantaker (reporter) 2013-04-15 17:58	Hi, You wrote - "If someone inherits from a log4j logger and uses the debug(Object object) method, would have his code broken when that logger is changed to slf4j." Does it mean? - "If someone inherits from a Openbravo class which holds a public/protected/default logger instance, it would be broken if the user has used debug(Object) in the subclass, as the method will not be available if we change to slf4j Logger" Or am I missing something? Cheers,

(0057913) AugustoMauch (administrator) 2013-04-15 18:04	Yes, that is what I meant. I should have phrased it more clearly, but I see you understood what I meant :) Best regards,

(0057914) mantaker (reporter) 2013-04-15 18:50	I'm sorry. If that is the concern - Is it not a design flaw to have non-private logger instances (subclasses should declare its own logger instance)? Or do you have valid reasons, to have public logger instance? I took a keen look at the code base. There are few places where you people used "public static Logger" declaration. And a lot of sqlc generation where just "static Logger" declaration is present. My suggestion to solve this would be, 1. Change all private Logger instances to slf4j (As you said) 2. Change sqlc generation to have slf4j logger (If that is not possible, follow 3rd point) 3. Add @Deprecated to all the public logger instances - like @Deprecated public static Logger log4j (We could make them private at a later release, or leave it as a legacy code and think of it later) 4. Create a wiki page to enforce the developers the usage of slf4j Logger. 5. Once things get stable, we can think of having logback as the default logger (To leverage the native implementation). Does it make sense? Correct me if this sounds odd. Cheers,

(0058026) mantaker (reporter) 2013-04-18 17:13	AugustoMauch - I don't have option to change this issue's status to "feedback" :-) You might have missed to respond to this. Cheers,

Issue History
Date Modified	Username	Field	Change
2013-04-11 16:12	mantaker	New Issue
2013-04-11 16:12	mantaker	Assigned To	=> dmiguelez
2013-04-11 16:12	mantaker	Modules	=> Core
2013-04-11 16:12	mantaker	Triggers an Emergency Pack	=> No
2013-04-11 17:12	AugustoMauch	Assigned To	dmiguelez => AugustoMauch
2013-04-11 17:21	AugustoMauch	Note Added: 0057851
2013-04-11 17:21	AugustoMauch	Status	new => feedback
2013-04-12 12:17	alostale	Issue Monitored: alostale
2013-04-12 12:25	AugustoMauch	Note Added: 0057865
2013-04-12 12:25	AugustoMauch	Status	feedback => new
2013-04-12 12:25	AugustoMauch	Note Edited: 0057865	View Revisions
2013-04-12 12:45	mantaker	Note Added: 0057867
2013-04-12 12:54	AugustoMauch	Note Added: 0057868
2013-04-12 13:02	mantaker	Note Added: 0057870
2013-04-15 15:01	AugustoMauch	Type	defect => design defect
2013-04-15 15:46	AugustoMauch	Note Added: 0057909
2013-04-15 17:58	mantaker	Note Added: 0057912
2013-04-15 18:04	AugustoMauch	Note Added: 0057913
2013-04-15 18:50	mantaker	Note Added: 0057914
2013-04-18 17:13	mantaker	Note Added: 0058026

Relationships [ Relation Graph ] [ Dependency Graph ]