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

NEXUS-5118: Rationalizing logging in bundle #530

Merged
merged 7 commits into from Sep 14, 2012

Conversation

Projects
None yet
4 participants
Contributor

cstamas commented Sep 4, 2012

Logging between Nexus 1.9, 2.0, 2.1 changed a lot, and 2.2 should sort all the user reported problems, like:

  • Jetty and Nexus uses different logging frameworks (Jetty uses ConsoleLogger while Nexus logback in 2.0 for example)
  • duplications of logback libraries in 2.1
    etc

CI:
https://builds.sonatype.org/view/nexus-features/job/nexus-oss-its-feature/407/

cstamas added some commits Aug 2, 2012

@cstamas cstamas NEXUS-5118: Initial imple
Goal is to make "bundle packaging" (which is basically Jetty + Nexus WAR)
use same LogBack instance (as we had problems reported by users in 2.0).

Changes:
* jetty.xml - umarked "server level" logback as hidden to make it really shared
* extracted custom appender into new module nexus-logging-extras-appender that is put top level
* nexus-logging-extras got component implementation for EventTarget
* logback-events.xml changed to use new appender
* bundle assembly changed that exclude all logging relevant JARs from Nexus WAR as they are _provided_ on top level from now on

This is WIP, not yet functional.
349670d
@cstamas cstamas NEXUS-5118: Adding exclusion for top level invluded JAR
Did not stir much water, but Tattletale chokes on this.
18270f5
@cstamas cstamas NEXUS-5118: Made a plexus component for EventTarget 4f87c61
@cstamas cstamas NEXUS-5118: Making the Event Target a SISU component 6d10eba
@cstamas cstamas NEXUS-5118: Fixing the initial imple
The initial implementation was conceived okay, but
there was a problem with Injection happening as the class
loaded up had annotations thrown away by classloader.

Reason for that is that in bundle layout the appender
is on Jetty classloader level (parent of Nexus webapp)
and that CL does not have @Inject. Hence, the fix was to
introduce "special" case for injection when ForwaringAppender
was met, simply doing manual injection on it.
9b6bffb
@cstamas cstamas NEXUS-5118: Unrelated file on my local FS got into the commit by mist…
…ake.
1b90ad3

@peterlynch peterlynch commented on the diff Sep 4, 2012

...main/java/org/sonatype/nexus/logback/EventTarget.java
+ *
+ * Sonatype Nexus (TM) Professional Version is available from Sonatype, Inc. "Sonatype" and "Sonatype Nexus" are trademarks
+ * of Sonatype, Inc. Apache Maven is a trademark of the Apache Software Foundation. M2eclipse is a trademark of the
+ * Eclipse Foundation. All other trademarks are the property of their respective owners.
+ */
+package org.sonatype.nexus.logback;
+
+import ch.qos.logback.classic.spi.ILoggingEvent;
+
+/**
+ * An target that receives logging events from {@link ForwardingAppender}.
+ *
+ * @author cstamas
+ * @since 2.2
+ */
+public interface EventTarget
@peterlynch

peterlynch Sep 4, 2012

Member

If this is all about logging, is there a reason you did not call this LoggingEventTarget?

Member

peterlynch commented Sep 4, 2012

checked out the branch and tried this out a bit - seems to log things correctly - and nexus war is not affected ( still contain logback impl ), so appears all fine.

If there are problems this is solving, are there tests we can write to demonstrate the problems and prove this changes fixes it?

Contributor

cstamas commented Sep 4, 2012

CI passed on both slaves, but new file missed a license header (fixed in c0ee3dd)

Contributor

adreghiciu commented Sep 5, 2012

+1

Contributor

nabcos commented Sep 11, 2012

+1

@cstamas cstamas added a commit that referenced this pull request Sep 14, 2012

@cstamas cstamas Merge pull request #530 from sonatype/nexus-5118
NEXUS-5118: Rationalizing logging in bundle
bd8d01f

@cstamas cstamas merged commit bd8d01f into master Sep 14, 2012

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