What about being able to inject a Logger?

[agoncal]

Description

Loggers are everywhere and Quarkus even supports several ones. So why not being able to inject a logger?

This issue was created from a discussion in Zulip.

Implementation ideas

Today we use static loggers:

private static final Logger LOG = Logger.getLogger(ExampleResource.class);

But we could also be able to inject a logger:

@Inject
Logger LOG;

Or even use a qualifier and remove the @Inject (using the same style of @ConfigProperty) and have:

@Log
Logger LOG;

As @mkouba mentions on Zulip, we could have a producer but "the problem with this approach is that the bean must be @dependent in order to be able to access the injection point metadata:

class Loggers {
    @Produces
   Logger getLogger(InjectionPoint injectionPoint) {
        return Logger.getLogger( injectionPoint.getMember().getDeclaringClass().getSimpleName() );
    }
}

There is also a performance impact as injection happens at runtime. So "we could cache the loggers to avoid the perf penalty"

[Ladicek]

FYI I did a little experiment with simplifying logging using bytecode transformation. Basically, transform static method invocations like Log.info(...) to instance method invocations on a generated private static final Logger quarkusSyntheticLogger = Logger.getLogger(... class name ...).

The experiment is here: https://github.com/Ladicek/quarkus/commits/logging-experiment It's crude, because it scans all classes in the application (!) and the bytecode transformation only works properly for methods with up to 2 arguments. But it works :-)

I guess it could be done properly (perhaps limit the transformation to classes with some annotation?), but I didn't want to spend too much time on it.

[Ladicek]

Ah now I know where I've seen this idea! I think @FroMage proposed basically the same thing a while ago?

[FroMage]

Haha, yes!!! And it depends on smallrye/jandex#85 to be efficient.

[FroMage]

It's #10929 but from what you describe it's the same thing, so +1

[Ladicek]

I found a constant pool scanning utility in the bytecode transformation code, so I used it, but agree it would be even better to have that info in Jandex.

[mkouba]

So here's a complete solution for "injectable logger": https://github.com/mkouba/quarkus/tree/injectable-logger

@ApplicationScoped
class SimpleBean {

   @Inject
   Logger log; // org.jboss.logging.Logger.getLogger(SimpleBean.class)

   @LoggerName("shared")
   Logger shared; // org.jboss.logging.Logger.getLogger("shared")
}

The impl is very straightforward (and lacks any transformation magic) but of course there are some drawbacks (mentioned below).

pros:

• no transformation needed
• intuitive usage in CDI beans
• easy way to use a shared logger (with name other than FQCN)
• loggers are cached so no perf penalty when injecting short-lived beans

cons:

• cannot be used in static methods (which are not very common in beans but still!)
• requires injection

I'm not sure whether it would make sense to provide both the CDI-based and the static-transformation solutions...

[agoncal]

@mkouba what about having both (injection and used in static method) ? If we change the naming so it "looks" the same, we could have:

@ApplicationScoped
class SimpleBean {

   @Log
   Logger log; // org.jboss.logging.Logger.getLogger(SimpleBean.class)

   @Log("shared")
   Logger shared; // org.jboss.logging.Logger.getLogger("shared")

  static {
    Log.info(); // The solution of @Ladicek 
  }
}

Of course it's complitely different things, but because @Log and Log have the same name, it feels the same.

[mkouba]

Of course it's complitely different things, but because @log and Log have the same name, it feels the same.

Maybe. I can update my branch to reflect this proposal.

[mkouba]

Updated. You can do @Inject Logger, @Log Logger and @Log("myApp") Logger. And of course it works for constructor/setter injection as well:

@ApplicationScoped
class SimpleBean {

   private final Logger log; // org.jboss.logging.Logger.getLogger(SimpleBean.class)

   SimpleBean(Logger log) {
    this.log = log;
   }
}

[agoncal]

Beautiful !

[FroMage]

I found a constant pool scanning utility in the bytecode transformation code, so I used it, but agree it would be even better to have that info in Jandex.

@Ladicek It's not really scanning in the sense that you have to scan every class to find the ones that have the constant, which is why I did this PR. If we can merge it, we can implement this by not scanning every class so it's reasonably efficient.

Otherwise +1 on having an annotation to get a logger injected, but @agoncal we can't use the same class name for the annotation and the logger utility with static methods. Which means we need to find two names that go hand in hand.

@Ladicek we should continue discussion of the static methods on #10929

[Ladicek]

Yea I agree we need to figure out nice names. Because I totally want the "simplified" calls to be Log.info, because that needs to be very close to what people otherwise do, which is log.info :-)

[mkouba]

we can't use the same class name for the annotation and the logger utility with static methods. Which means we need to find two names that go hand in hand.

That's a good point. Hm, @Logger Logger is probably out of game. For FQCN-based logger we don't need a qualifier at all (simply @Inject Logger) but we need one for a logger with custom name. @javax.inject.Named cannot be easily used, mainly because its value() is binding and so we would have to create a bean for every Logger injection point, also @Named qualifier is not "counted" in CDI when it comes to @Default (more interesting reading in the spec)... and there are few other consequences. So maybe something like @LogName? Alternatively, we could use LOG for the logger utility class ;-).

[FroMage]

Indeed @Logger Logger is out, but as you mention, @Inject Logger is enough.

Can we assume that the named variant is a lot less common? All the examples I've seen use the current class name for the category.

If it is indeed less common, then @LogName seems fine, and make it auto-inject so you don't need it and @Inject both.

[mkouba]

Can we assume that the named variant is a lot less common?

Well, it's very like a lot less common if you use FQCN-based loggers everywhere. However, if you e.g. use interface-based loggers it's common to define certain parts of your app and group the log messages (but then you would probably not need an injected logger ;-).

If it is indeed less common, then @LogName seems fine, and make it auto-inject so you don't need it and @Inject both.

This woks for qualifiers out of the box in quarkus ;-).

[agoncal]

I quite like the LOG idea actually ;o) Its purpose is for static content, so why not having it UPPER case ? @Log and LOG.... I wonder if it feels natural though.

As for the other choice, maybe that's an "English" bias, but for me "log" is the trace in it self, as "logger" is the all thing. So for me @LoggerName is more natural than @LogName

[FroMage]

I think Log and LOG are bad UX because users will be confused by the completion and trying to guess what they do and how they differ.

I did wonder about using @LogName which is an abbreviation and is generally bad UX. But now that you mention it, we can totally justify @LoggerName and Log.info because Log is not an abbreviation here but an action: "log this at info level".

[mkouba]

Ok, then @Inject Logger log and @LoggerName("foo") Logger log it is... I'm going to send a PR shortly.