Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

BufferingLog FQCN support and a Logback implementation #8

Merged
merged 6 commits into from Nov 7, 2011

Conversation

Projects
None yet
3 participants
Contributor

chrisdolan commented Oct 26, 2011

Sorry to put to commits in the same pull request... I can break them up if needed.

  1. FQCN for BufferingLog - this one is simple. Early log messages (before the pax-logging-service is activated) were being annotated with the wrong origin class.

  2. Logback support - this one is big and has a few details that need consideration.
    a) it includes two monkey-patched Logback classes to fix some places where Logback was not flexible enough to hook in OSGi details. I've submitted both patches to Logback and one has been accepted.
    b) I duplicated the four SPI classes. Surely there's a better way, but it's beyond my Maven-fu to copy them from pax-logging-server dynamically. Maybe they should really be in pax-logging-api?
    c) my unit tests use EasyMock instead of JMock. Hopefully not a big deal?
    d) my code uses FindBugs @nonnull annotations. These are not retained to runtime, so hopefully not a problem?
    e) I've copied and modified a handful of classes in org.ops4j.pax.logging.service.internal. Perhaps these should be refactored into a shared service base package to reduce code duplication?

Thanks for your consideration. A very similar version of this code has been running at my employer for the last 3 months (I cleaned up and changed package names for this submission)

Contributor

chrisdolan commented Oct 26, 2011

Oh, another point. I've also specified Logback 0.9.29 in the pom. Logback 0.9.30 has been out for a month, but I haven't tested it yet so I left this submission as 0.9.29 for now.

Owner

niclash commented Oct 26, 2011

Just one comment; Please go through and make the License headers in
each source file accurate. I.e. where it is copied from
pax-logging-service, and I am likely listed as Copyright owner, add
your name if you have made changes. And for new files, copy and place
yourself as the Copyright owner.

I am pleased to see this come into fruition, although I suspect it is
a fairly 'rigid' implementation and I had bigger plans to fully take
advantage of Logback's better SPI. But I gave up on that, so no
complaints...

On Wed, Oct 26, 2011 at 12:20 PM, Chris Dolan
reply@reply.github.com
wrote:

Sorry to put to commits in the same pull request... I can break them up if needed.

 1) FQCN for BufferingLog - this one is simple. Early log messages (before the pax-logging-service is activated) were being annotated with the wrong origin class.

 2) Logback support - this one is big and has a few details that need consideration.
   a) it includes two monkey-patched Logback classes to fix some places where Logback was not flexible enough to hook in OSGi details. I've submitted both patches to Logback and one has been accepted.
   b) I duplicated the four SPI classes. Surely there's a better way, but it's beyond my Maven-fu to copy them from pax-logging-server dynamically. Maybe they should really be in pax-logging-api?
   c) my unit tests use EasyMock instead of JMock. Hopefully not a big deal?
   d) my code uses FindBugs @nonnull annotations. These are not retained to runtime, so hopefully not a problem?
   e) I've copied and modified a handful of classes in org.ops4j.pax.logging.service.internal. Perhaps these should be refactored into a shared service base package to reduce code duplication?

Thanks for your consideration. A very similar version of this code has been running at my employer for the last 3 months (I cleaned up and changed package names for this submission)

You can merge this Pull Request by running:

 git pull https://github.com/chrisdolan/org.ops4j.pax.logging master

Or you can view, comment on it, or merge it online at:

 #8

-- Commit Summary --

  • Add FQCN support to BufferingLog. Otherwise early log messages may get attributed to the wrong class
  • add a version of the service that uses Logback instead of Log4J

-- File Changes --

M pax-logging-api/src/main/java/org/ops4j/pax/logging/internal/BufferingLog.java (87)
A pax-logging-logback/LICENSE.txt (227)
A pax-logging-logback/NOTICE.txt (6)
A pax-logging-logback/osgi.bnd (47)
A pax-logging-logback/pom.xml (158)
A pax-logging-logback/src/main/java/ch/qos/logback/classic/spi/LoggingEvent.java (338)
A pax-logging-logback/src/main/java/ch/qos/logback/classic/spi/PackagingDataCalculator.java (288)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/appender/PaxAppenderDelegate.java (70)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/Activator.java (180)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/FrameworkHandler.java (182)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/JdkHandler.java (105)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/LogEntryImpl.java (89)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/LogReaderServiceImpl.java (110)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/PaxAppenderProxy.java (60)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/PaxEventHandler.java (13)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/PaxLevelForLogback.java (70)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/PaxLocationInfoForLogback.java (52)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/PaxLoggerImpl.java (224)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/PaxLoggingEventForLogback.java (95)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/logback/internal/PaxLoggingServiceImpl.java (428)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/spi/PaxAppender.java (30)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/spi/PaxLevel.java (51)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/spi/PaxLocationInfo.java (53)
A pax-logging-logback/src/main/java/org/ops4j/pax/logging/spi/PaxLoggingEvent.java (45)
A pax-logging-logback/src/test/java/org/ops4j/pax/logging/logback/internal/JdkHandlerTest.java (77)
A pax-logging-logback/src/test/java/org/ops4j/pax/logging/logback/internal/PaxAppenderDelegateTest.java (69)
A pax-logging-logback/src/test/java/org/ops4j/pax/logging/logback/internal/PaxLoggerImplTest.java (148)
A pax-logging-logback/src/test/java/org/ops4j/pax/logging/logback/internal/PaxLoggingServiceImplTest.java (118)
M pom.xml (15)

-- Patch Links --

 https://github.com/ops4j/org.ops4j.pax.logging/pull/8.patch
 https://github.com/ops4j/org.ops4j.pax.logging/pull/8.diff

Reply to this email directly or view it on GitHub:
#8

Niclas Hedhman, Software Developer
http://www.qi4j.org - New Energy for Java

I live here; http://tinyurl.com/3xugrbk
I work here; http://tinyurl.com/6a2pl4j
I relax here; http://tinyurl.com/2cgsug

Owner

anpieber commented Oct 26, 2011

boah! I cant believe it... finally someone really stepped up and added logback support :-) I'vent done any intensive tests with the implementation but only have given it a short look; from my point of view pls add the according jira's and push it to the master; if we discover any problems we can still revert the patch (which is the ops4j mentality anyway) --> +1

Contributor

chrisdolan commented Oct 26, 2011

I'll do that after I address Niclas' recommendations, thanks.

An implementation question: I used "org.ops4j.pax.logging" as the
configuration PID, which means that the logback service shares a .cfg file
with the log4j service. I thought that was fine because the only config
keys that my implementation uses are
"org.ops4j.pax.logging.logback.config.file" which is the path to the
logback.xml file, and "pax.logging.entries.size" which has the same
meaning as the log4j version.

Is that an acceptable decision?

Chris

boah! I cant believe it... finally someone really stepped up and added
logback support :-) I'vent done any intensive tests with the
implementation but only have given it a short look; from my point of view
pls add the according jira's and push it to the master; if we discover any
problems we can still revert the patch (which is the ops4j mentality
anyway) --> +1

Reply to this email directly or view it on GitHub:
#8 (comment)

Owner

niclash commented Oct 26, 2011

Sounds ok to me.

On Wed, Oct 26, 2011 at 9:02 PM, Chris Dolan
reply@reply.github.com
wrote:

An implementation question: I used "org.ops4j.pax.logging" as the
configuration PID, which means that the logback service shares a .cfg file
with the log4j service. I thought that was fine because the only config
keys that my implementation uses are
"org.ops4j.pax.logging.logback.config.file" which is the path to the
logback.xml file, and "pax.logging.entries.size" which has the same
meaning as the log4j version.

Contributor

chrisdolan commented Nov 7, 2011

I added http://team.ops4j.org/browse/PAXLOGGING-117 to describe this new addition.

@chrisdolan chrisdolan added a commit that referenced this pull request Nov 7, 2011

@chrisdolan chrisdolan Merge pull request #8 from chrisdolan/master
A Logback implementation and supporting changes
53d75b5

@chrisdolan chrisdolan merged commit 53d75b5 into ops4j:master Nov 7, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment