Openbravo Issue Tracking System - Openbravo ERP |
| View Issue Details |
|
| ID | Project | Category | View Status | Date Submitted | Last Update |
| 0023534 | Openbravo ERP | Z. Others | public | 2013-04-11 16:12 | 2013-04-18 17:13 |
|
| Reporter | mantaker | |
| Assigned To | AugustoMauch | |
| Priority | high | Severity | major | Reproducibility | N/A |
| Status | new | Resolution | open | |
| Platform | | OS | 5 | OS Version | |
| Product Version | main | |
| Target Version | | Fixed in Version | | |
| Merge Request Status | |
| Review Assigned To | |
| OBNetwork customer | |
| Web browser | |
| Modules | Core |
| Support ticket | |
| Regression level | |
| Regression date | |
| Regression introduced in release | |
| Regression introduced by commit | |
| Triggers an Emergency Pack | No |
|
| Summary | 0023534: slf4j has not been properly used |
| Description | slf4j is a logging abstraction library for various popular logging frameworks. It has been moved to core some 23 months ago.
The project still uses - for eg:
import org.apache.log4j.Logger;
private static final Logger log = Logger.getLogger(OBDal.class);
instead of -
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
private static final Logger logger = LoggerFactory.getLogger(OBDal.class);
This can be easily done by slf4j migrator here at - http://www.slf4j.org/migrator.html [^] |
| Steps To Reproduce | Not Applicable |
| Proposed Solution | |
| Additional Information | |
| Tags | No tags attached. |
| Relationships | |
| Attached Files | |
|
| 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 | bug_revision_view_page.php?bugnote_id=0057865#r4567 |
| 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 | |
|
Notes |
|
|
|
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. |
|
|
|
|
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 [^]
|
|
|
|
|
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 |
|
|
|
|
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. |
|
|
|
|
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, |
|
|
|
|
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 [^] |
|
|
|
|
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, |
|
|
|
|
Yes, that is what I meant. I should have phrased it more clearly, but I see you understood what I meant :)
Best regards, |
|
|
|
|
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, |
|
|
|
|
AugustoMauch - I don't have option to change this issue's status to "feedback" :-) You might have missed to respond to this.
Cheers, |
|