Bug 46426 - Implement commons-logging interfaces natively in log4j
Summary: Implement commons-logging interfaces natively in log4j
Status: ASSIGNED
Alias: None
Product: Log4j - Now in Jira
Classification: Unclassified
Component: Appender (show other bugs)
Version: 1.2
Hardware: PC Linux
: P2 normal
Target Milestone: ---
Assignee: log4j-dev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-20 23:33 UTC by Henning Schmiedehausen
Modified: 2009-12-13 11:04 UTC (History)
0 users



Attachments
Patch to implement the commons-logging interfaces natively in log4j (12.35 KB, patch)
2008-12-20 23:33 UTC, Henning Schmiedehausen
Details | Diff
Patch to implement the commons-logging interfaces natively in log4j (13.55 KB, patch)
2008-12-21 00:12 UTC, Henning Schmiedehausen
Details | Diff
Patch to implement the commons-logging interfaces natively in log4j (13.08 KB, patch)
2008-12-21 01:44 UTC, Henning Schmiedehausen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Henning Schmiedehausen 2008-12-20 23:33:59 UTC
Created attachment 23041 [details]
Patch to implement the commons-logging interfaces natively in log4j

With the ongoing discussion on whether or not to implement slf4j inside log4j, it makes IMHO much more sense to integrate the "other" popular logging component at the ASF directly into log4j.

The most popular pattern in libraries is to use either slf + logging framework or commons-logging + logging framework. In the case of "log4j" as logging framework, this is slf4j-log4j + log4j or commons-logging + log4j. 

The ongoing discussion tries to alleviate the first situation by integrating slf4j into log4j, thus allowing log4j as a direct replacement for all components that currently use slf4j + logging framework in the log4j case (drop log4j into the libs folder, remove all other slf4j components and logging frameworks. Done).

This patch aims at doing the same thing with commons-logging. If this patch gets applied by log4j, log4j itself is a drop-in replacement for "commons-logging + log framework", redirecting all log messages from components using commons-logging directly to log4j. The combo commons-logging + log4j is probably the most popular combination.

The widespread critique of commons-logging which tries to select its logging framework at runtime through a combination of reflection and configuration, which led to the design of slf4j could also be silenced as this is a non-nonsense concrete implementation of the classes. 

The patch itself is minimal; the included LogFactory class is purely designed to serve the most common patterns of commons-logging usage, which is 

private static final Log log = LogFactory.getLog(classname.class);
private static final Log log = LogFactory.getLog("mylogger");
protected final Log log = LogFactory.getLog(this.getClass());
protected final Log log = LogFactory.getLog("mylogger");

these four patterns are the vast majority of all commons-logging uses.

Please apply the attached patch. This bug report is not meant as a joke.

Thanks
    Henning
Comment 1 Henning Schmiedehausen 2008-12-21 00:12:48 UTC
Created attachment 23042 [details]
Patch to implement the commons-logging interfaces natively in log4j

Bundle plugin must export the interfaces.
Comment 2 Henning Schmiedehausen 2008-12-21 01:44:52 UTC
Created attachment 23043 [details]
Patch to implement the commons-logging interfaces natively in log4j

This improves the patch by adding the org.apache.commons.logging package to the private packages section of the MANIFEST, thus keeping this version of log4j to be OSGI compatible.
Comment 3 Ceki Gulcu 2008-12-22 06:51:46 UTC
Hello Henning,

Thank you for your proposal. Given that your proposal binds commons-logging's discovery mechanism with log4j, I think in addition to log4j committers, the Apache commons project committers should also be involved in deciding whether to apply this patch or not, as it is likely to have an impact on them as well.

I would also like to note several years ago, prior to the advent of SLF4J, applying your patch would have been inconceivable. 
Comment 4 Curt Arnold 2009-12-12 15:20:26 UTC
Sorry I missed the bug report last year.  Maybe that it was 7 days before my wedding might explain my mind being otherwise occupied.   Just ran across it doing a routine bug sweep.  Surprised that it didn't get more traction when it was reported.

The patch has a couple of different aspects,

1. Adding Logger.IsErrorEnabled, Logger.IsFatalEnabled, Logger.IsWarnEnabled

Innocuous additional methods on a concrete class.

2. Declaring the Logger implements org.apache.commons.logging.Log and bundling o.a.c.l.Log

I'm uneasy about adding the dependency on Logger without providing the class, don't want to add a new dependency if people are upgrading for unrelated reasons.  However, providing a separate class file would seem likely to result in the potential for mismatched if o.a.log4j.Logger is cast to a o.a.c.Log from a different class loader.

3. Providing a concrete implementation of LogFactory.

I'm uneasy to a log4j.jar that is bundled with some component, but not actually used might disrupt a generic LogFactory that is dispatching to some other logging framework.

My gut is it might be better for log4j to conditionally implement o.a.common.Log is it available, but not provide it.   Maybe:

o.a.l.Logger gets the three extra methods.  
Create another class (CLogger for the rest of the discussion) that extends o.a.l.Logger and implements Log.  
Default log4j factory would attempt to create the CLogger's but if they encounter a ClassNotFoundException, they'd create plain Loggers.
The Common Logging adapter for log4j could do an instanceof on the loggers returned from log4j and if they supported Log then pass them directly to the caller, otherwise wrap them as previously.
A hard-coded LoggerFactory wired to log4j could be provided as an extra jar.


I'd love for you to update the report with your current thoughts.
Comment 5 Henning Schmiedehausen 2009-12-13 11:04:36 UTC
Hi Curt,

the main idea of this patch is to start bridging the gap between the various "log<xyz>" implementations. commons-logging was the wrong thing at the right time; these days it seems that slf4j fills the gap nicely. 

This patch is mainly an exercise in "you can do it, if you really want", not something that should go in tomorrow.

Thanks for considering it.

-h

(In reply to comment #4)
> Sorry I missed the bug report last year.  Maybe that it was 7 days before my
> wedding might explain my mind being otherwise occupied.   Just ran across it
> doing a routine bug sweep.  Surprised that it didn't get more traction when it
> was reported.
> 
> The patch has a couple of different aspects,
> 
> 1. Adding Logger.IsErrorEnabled, Logger.IsFatalEnabled, Logger.IsWarnEnabled
> 
> Innocuous additional methods on a concrete class.
> 
> 2. Declaring the Logger implements org.apache.commons.logging.Log and bundling
> o.a.c.l.Log
> 
> I'm uneasy about adding the dependency on Logger without providing the class,
> don't want to add a new dependency if people are upgrading for unrelated
> reasons.  However, providing a separate class file would seem likely to result
> in the potential for mismatched if o.a.log4j.Logger is cast to a o.a.c.Log from
> a different class loader.
> 
> 3. Providing a concrete implementation of LogFactory.
> 
> I'm uneasy to a log4j.jar that is bundled with some component, but not actually
> used might disrupt a generic LogFactory that is dispatching to some other
> logging framework.
> 
> My gut is it might be better for log4j to conditionally implement
> o.a.common.Log is it available, but not provide it.   Maybe:
> 
> o.a.l.Logger gets the three extra methods.  
> Create another class (CLogger for the rest of the discussion) that extends
> o.a.l.Logger and implements Log.  
> Default log4j factory would attempt to create the CLogger's but if they
> encounter a ClassNotFoundException, they'd create plain Loggers.
> The Common Logging adapter for log4j could do an instanceof on the loggers
> returned from log4j and if they supported Log then pass them directly to the
> caller, otherwise wrap them as previously.
> A hard-coded LoggerFactory wired to log4j could be provided as an extra jar.
> 
> 
> I'd love for you to update the report with your current thoughts.