New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow the opportunity to customize your logging factory #996

Merged
merged 1 commit into from May 8, 2015

Conversation

Projects
None yet
6 participants
@carlo-rtr
Member

carlo-rtr commented Apr 16, 2015

No description provided.

@jplock

This comment has been minimized.

Member

jplock commented Apr 17, 2015

Should we also try and address #987 or keep that as a separate change?

@carlo-rtr

This comment has been minimized.

Member

carlo-rtr commented Apr 17, 2015

That's a good point, let's get that sorted. I'll add my 2 cents.

Btw, I probably should say that there was much debate on whether this should be configurable. See #567

My feeling is that we should only maintain the default logger. Making it flexible doesn't feel like it's going to add much over head on our maintenance. If someone is really hell bent on using xml configuration, they'll find a way.

@arteam

This comment has been minimized.

Member

arteam commented Apr 26, 2015

After #987 that issue need to rebased.

Also we need to think abount the need of bootstrapping logging in Application, if it's configured externally.

@carlo-rtr carlo-rtr force-pushed the carlo-rtr:discoverable_logging branch 2 times, most recently from 84d60ef to 1281b2a Apr 27, 2015

@carlo-rtr

This comment has been minimized.

Member

carlo-rtr commented Apr 27, 2015

I've rebased and added some new code.

  • documented the bootstrap logging code
  • made logging bootstrap only have effect on first run
  • perform bootstrap logging init in constructor of Application in favor of static block
@carlo-rtr

This comment has been minimized.

Member

carlo-rtr commented Apr 27, 2015

@arteam I missed the #987 comment. I'll wait until your code is back on master, then I'll pull it down and push the rebased version.

@carlo-rtr carlo-rtr referenced this pull request Apr 27, 2015

Merged

Run tests in parallel #1012

@carlo-rtr carlo-rtr force-pushed the carlo-rtr:discoverable_logging branch from 1281b2a to 7012fe2 May 7, 2015

@carlo-rtr carlo-rtr removed the needs code label May 7, 2015

@carlo-rtr

This comment has been minimized.

Member

carlo-rtr commented May 7, 2015

@jplock @arteam should be all set for review

@jplock

This comment has been minimized.

Member

jplock commented May 7, 2015

LGTM! I'll let @arteam review as well.

import java.util.concurrent.locks.ReentrantLock;
/**
* Utililty class to configure logging before the dropwizard yml

This comment has been minimized.

@arteam

arteam May 7, 2015

Member

Nit: s/Utililty/Utility

* strategy has been applied.
* <p/>
* N.B. The methods in this class have run once semantics,
* multiple calls are impodent

This comment has been minimized.

@arteam

arteam May 7, 2015

Member

Nit: s/impodent/idempotent, if this word was meant.

@GuardedBy("boostrappingLock")
private static boolean bootstrapped = false;
private static Lock bootstrappingLock = new ReentrantLock();

This comment has been minimized.

@arteam

arteam May 7, 2015

Member

Could we me make the lock final? I think it's better to always lock on final fields as a safety measure.
Moreover, this field is not mutated.

*/
public class BootstrapLogging {
@GuardedBy("boostrappingLock")

This comment has been minimized.

@arteam

arteam May 7, 2015

Member

Nit: s/boostrappingLock/bootstrappingLock

bootstrappingLock.lock();
try {
if (!bootstrapped) {

This comment has been minimized.

@arteam

arteam May 7, 2015

Member

Maybe better to revert the condition? If logging is bootstrapped, we can return early.
Advantage is a reduction of a big if block with identation.

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
public class LoggingUtil {

This comment has been minimized.

@arteam

arteam May 7, 2015

Member

I wonder should we make this class a package protected or public... I mean, do we want its methods be visible for user logger factories or should they be closed as a part of internal Dropwizard logging mechanism? I'm more apt to to the latter. What is your opinion on this?

This comment has been minimized.

@carlo-rtr

carlo-rtr May 8, 2015

Member

I was thinking it should be public because so that people implementing their own logging factories have a way to get the LoggerContext in a thread safe way and have a utility for hijacking jdk logging.

@GuardedBy("julHijackingLock")
private static boolean julHijacked = false;
private static Lock julHijackingLock = new ReentrantLock();

This comment has been minimized.

@arteam

arteam May 7, 2015

Member

The lock could be final, because we don't mutate it.

@arteam

This comment has been minimized.

Member

arteam commented May 7, 2015

Apart from some nitpicks, this PR looks accurate to me. Well done, @carlo-rtr.

@carlo-rtr

This comment has been minimized.

Member

carlo-rtr commented May 8, 2015

I really wish I could spell :)

discoverable logging
* make jdk log hijacking run once
* make bootstrap run once, and pull into a Utility class
* LoggingFactory a discoverable an interface
* Old LoggingFactory becomes DefaultLoggingFactory

@carlo-rtr carlo-rtr force-pushed the carlo-rtr:discoverable_logging branch from 7012fe2 to 847275a May 8, 2015

@carlo-rtr

This comment has been minimized.

Member

carlo-rtr commented May 8, 2015

@arteam fixed the typos, made the locks final, and inverted the condition. Thanks for looking

arteam added a commit that referenced this pull request May 8, 2015

Merge pull request #996 from carlo-rtr/discoverable_logging
allow the opportunity to customize your logging factory

@arteam arteam merged commit c4d98e8 into dropwizard:master May 8, 2015

@jplock jplock added this to the 0.9.0 milestone May 8, 2015

@dknc

This comment has been minimized.

dknc commented on 847275a Jul 31, 2016

The method BootstrapLogging.bootstrap() is called in the constructor of Application class make sub class of Application not able to bootstrap logging in different log level other than Level.WARN, which make some INFO /DEBUG logs cannot be written to configured log appenders at the very early stage of application initialization.

This comment has been minimized.

Member

joschi replied Jul 31, 2016

@dknc Please create an issue for that if you think that this is a problem that should be fixed.

@coveralls

This comment has been minimized.

coveralls commented Dec 8, 2016

Coverage Status

Changes Unknown when pulling 847275a on carlo-rtr:discoverable_logging into ** on dropwizard:master**.

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