frjaeger220 / google-guice

Automatically exported from code.google.com/p/google-guice
Apache License 2.0
0 stars 0 forks source link

Move Logger binding out to LoggerModule, so users can bind their own Loggers #269

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
Currently Guice binds a Logger for you, and won't let you bind your own. 
Instead, we should ask the 
user to specify a LoggerModule. If they want a different one, they're free to 
choose their own.

Original issue reported on code.google.com by limpbizkit on 3 Nov 2008 at 11:41

GoogleCodeExporter commented 9 years ago
A quick hack that works till that fix is released, that won't just give you 
context
information about the injectee, but also any additional information (like the 
logging
channel).
Using method injection and a simple interceptor ;)

V=========== Sample Usage ================V

private Logger logger;

@Inject
@LogFactory
@LogChannel("Module1LoggingChannel")
@SuppressWarnings("unused")
private void setLogger(Provider<LoggerFactory> loggerFactory, Logger logger)
{
  this.logger = logger;
}

private void someMethod()
{
  logger.info("i've been called, and the logger have been injected correctly !");
}

^======== END Sample Usage ==============^

V=========== LoggingModule ===============V

public class LoggerModule extends AbstractModule {

  @Override
  protected void configure() {

    bind(Logger.class).toInstance(
        new LoggerFactory().getExceptionThrowingLogger()); // so that if injection
isn't performed, this logger will produce an exception

    bindInterceptor(Matchers.any(), Matchers
        .annotatedWith(LogFactory.class), new MethodInterceptor() {

      @SuppressWarnings("unchecked")
      @Override
      public Object invoke(MethodInvocation invocation) throws Throwable {
        Provider<LoggerFactory> provider = (Provider<LoggerFactory>) invocation
            .getArguments()[0]; // injected by guice

        String channel = null;
        LogChannel lc = invocation.getMethod().getAnnotation(
            LogChannel.class);
        if (lc != null)
          channel = lc.value();

        Class clazz = invocation.getMethod().getDeclaringClass();

        Logger logger = provider.get().getLogger(clazz, channel);

        invocation.getArguments()[1] = logger; // override the exception-throwing
logger injected by guice !

        return invocation.proceed();
      }
    });
  }
}
^=========== END LoggingModule ===========^

It works and everything but adds the burden of adding that dummy method to every
class you want to log from.

Original comment by mohammad...@gmail.com on 18 Apr 2009 at 11:33

GoogleCodeExporter commented 9 years ago

Original comment by limpbizkit on 26 Apr 2009 at 9:49

GoogleCodeExporter commented 9 years ago
Example patch attached.  API:

  <T> Binder.bindLogger(Class<T>, LoggerProvider<? extends T>)
  LoggerProvider<T> {
    T get(String className);
  }

Original comment by sberlin on 21 Feb 2011 at 8:30

Attachments:

GoogleCodeExporter commented 9 years ago
Looks like a useful addition - it would also avoid the need for Issue 492 
(slf4j support)

Original comment by mccu...@gmail.com on 22 Feb 2011 at 12:10

GoogleCodeExporter commented 9 years ago
Guice special-cases java.util.logging.Logger because it can and because it's 
part of the JDK.  Adding a bindLogger call could be abused, and is more or less 
an unnecessary addition to the API because the same thing can be done in other 
ways with the existing API.

Some potential solutions without changing the API:
  * Use an @InjectLogger approach (http://code.google.com/p/google-guice/wiki/CustomInjections)
  * Use something like Slf4jModule from Sitebricks (http://code.google.com/p/google-sitebricks/source/browse/trunk/slf4j/src/main/java/com/google/inject/slf4j/Slf4jModule.java)
  * Inject your LogFactory instead of a Logger, if really want constructor injection.
  * Don't inject your loggers.

Original comment by sberlin on 27 Feb 2011 at 8:22

GoogleCodeExporter commented 9 years ago
@Sam Sorry for waking this zombie up, but your workarounds are not really 
satisfying for cases where Guice is used to inject a class during unit testing. 
Asking all the users to forego Logger injection and force them to inject some 
custom logging interface/factory is incompatible with the desire of a 
injection-testing framework to act transparently on top of existing code. At 
the same time, it's important to be able to mock-out or replace logging code 
during unit testing.

For a concrete example, I'm running into this problem with Jukito:
  http://code.google.com/p/jukito/issues/detail?id=9

IMHO, non-rebindable built-in bindings pose a very important limitation for a 
use case (unit testing) that DI is supposed to specifically help resolve. 
Therefore I would really love to get some hook inside of Guice to switch the 
bound Logger. It doesn't have to be easy to do, but it really does sound 
impossible to do without some internal support from your side.

Also, I'm happy to work on a patch if you think it's appropriate.

Original comment by philippe.beaudoin on 17 Mar 2011 at 4:53

GoogleCodeExporter commented 9 years ago
No update on this?

Original comment by philippe.beaudoin on 25 May 2011 at 2:30

GoogleCodeExporter commented 9 years ago
It's closed as WontFix.

Original comment by sberlin on 25 May 2011 at 2:32

GoogleCodeExporter commented 9 years ago
I know. I was still hoping to have a discussion on it, following my March 17th 
comment...

Original comment by philippe.beaudoin on 25 May 2011 at 2:40

GoogleCodeExporter commented 9 years ago
Ah, sorry, missed that.  To be honest, I don't really see this changing.  It's 
dead simple to workaround.  Inject your LogFactory & construct your logger.  I 
don't see how this makes testing more difficult -- there's just a layer of 
indirection to account for (and it's easy to write a one-stop solution for 
that).

Original comment by sberlin on 25 May 2011 at 4:49

GoogleCodeExporter commented 9 years ago
I guess I find it painful because I can't accommodate my users who are 
injecting Logger in their classes and would like to test the logging behavior 
or their app. Instead of just being able to say "let's mock the Logger" I have 
to instruct them to change their code... IMHO, it makes auto-binding the Logger 
relatively useless.

Original comment by philippe.beaudoin on 25 May 2011 at 5:01

GoogleCodeExporter commented 9 years ago
Yes, the existing autobinding of Logger is a misfeature, IMO.

Original comment by sberlin on 25 May 2011 at 5:09

GoogleCodeExporter commented 9 years ago
Ok, it helps to know you agree. Now I'll be able to tell my users: "Sam thinks 
it's a misfeature, stop injecting that Logger!" ;)

Meanwhile, I'll keep an eye out for any move you make in that area. (Some 
mechanism for unbinding Logger, or anything like that...)

Original comment by philippe.beaudoin on 25 May 2011 at 5:17