Skip to content
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

Add @Logger(factory=MyLoggerFactory.class) #843

Open
lombokissues opened this issue Jul 14, 2015 · 23 comments
Open

Add @Logger(factory=MyLoggerFactory.class) #843

lombokissues opened this issue Jul 14, 2015 · 23 comments

Comments

@lombokissues
Copy link

Migrated from Google Code (issue 808)

@lombokissues
Copy link
Author

👤 Robot.Ninja.Sausage   🕗 May 01, 2015 at 20:00 UTC

I would like a new logging annotation that could accept a factory class as a parameter, and would then generate the log field.

For example,
import com.mycompany.MyLoggerFactory;

@ Logger(factory=MyLoggerFactory.class)
public class MyClass {
public static void doStuff() {
log.info("some stuff");
}
}

would become
import com.mycompany.MyLoggerFactory;

public class MyClass {
private static final java.util.logging.Logger log = com.mycompany.MyLoggerFactory.getLogger(MyClass.class);

public static void doStuff() {
    log.info("some stuff");
}

}

This would make it convenient to add custom loggers, in particular loggers that had custom methods on them that would allow for logging structured data.

@lombokissues
Copy link
Author

👤 Robot.Ninja.Sausage   🕗 May 27, 2015 at 19:22 UTC

I just noticed that the output example is wrong. It should be

import com.mycompany.MyLoggerFactory;

public class MyClass {
private static final MyLogger log = com.mycompany.MyLoggerFactory.getLogger(MyClass.class);

public static void doStuff() {
    log.info("some stuff");
}

}

where MyLogger is the return type of getLogger.

@lombokissues
Copy link
Author

End of migration

@sir4ur0n
Copy link

I'm actually surprised nobody else upvoted this feature. It would be quite useful when you write a wrapper around SLF4J or any other logger, to have business-oriented log methods (e.g. you create a logger method sensitiveOperation(BusinessContext context, User user, OperationEnum operation), or to use Supplier<String> for SLF4J.

@rspilker
Copy link
Collaborator

The statement "when you write a wrapper around SLF4J" doesn't really make sense since it is a facade. It is already a very extendable system.

@Maaartinus
Copy link
Contributor

@rspilker It does. While SLF4J is a facade, it's purpose is to interact with logback and other loggers. IMHO placing another facade over it makes sense. Especially, logger.sensitiveOperation(BusinessContext context, User user, OperationEnum operation) won't ever compile for logger.getClass() == org.slf4j.Logger.class. That said, I know I don't know much about SLF4J yet, but I'm thinking about a facade, too.

@sir4ur0n
Copy link

sir4ur0n commented Jun 19, 2017

@rspilker I probably wasn't explicit enough, sorry about that :/
SLF4J is a great logging facade, but a generic one.
A good practice is to add an application "facade" on top of your logger, to ensure meaningful logs, and/or enforce more typing.

Concrete example: for all your performance logs, you may write log.info("Time to execute order: " + time); at one code place, and log.info("Profile creation|" + time); at another, which means if you want later to parse all perf logs (e.g. in an ELK), you need to go through all your logs again to ensure the syntax is always the same.
Worse, at some place you may have logged as DEBUG, another as INFO for the same kind of performance.

The idea instead is to create a "facade" logger on top of SLF4J, which would look like this:

// In custom log facade called BusinessLogger.java
public void perf(Supplier<String> measurementDescription, double duration) {
  if (logger.isInfoEnabled()) {
    logger.info(measurementDescription.get() + "|" + duration);
  }
}

// In business class
private static final BusinessLogger businessLogger = BusinessLoggerFactory.get(MyClass.class);

// And then you use in your business code
businessLogger.perf(() -> "Order for client " + clientId + " in country " + country, transactionTime);

Benefits:

  • You don't need the isXXXEnabled saving everywhere, only in the business logger (less code cluttering).
  • You can use Suppliers (i.e. Java 8 lambdas) to save any heavy object building.
  • You ensure all your logs of same type (e.g. here, performance logs) use the same syntax (i.e. here if tomorrow I need to change my perf log format to adapt to Logstash, I need to modify it only in one place; same benefit if tomorrow I want to switch my perf logs to DEBUG).
  • You can "encourage" developers to use similar logs for similar meanings.

There are other benefits (like a BusinessContext) but these are the main ones I see in our day-to-day life in my company.

If we could pass to an (existing or new) Lombok annotation a factory class, then we could benefit from all the Lombok work to inject the field, and also benefit from application business logger.

The example from Robot.Ninja.Sausage looked neat:

@Logger(factory=BusinessLoggerFactory.class)
MyClass {}

would become

MyClass {
  private static final BusinessLogger log = com.mycompany.BusinessLoggerFactory.getLogger(MyClass.class);
}

Obviously I have no idea if this is a huge dev or not in Lombok, but I definitely see how many persons could benefit from it.

@nikowitt
Copy link

nikowitt commented Oct 6, 2017

I exactly need the same feature as e.g. SLF4J does not support Java 8 specific features like the Supplier functional interface that makes lazy log evaluation much easier, e.g.

Logger.error(java.util.function.Supplier<String>)

@sir4ur0n
Copy link

sir4ur0n commented Nov 4, 2017

Hi @rspilker,

What's the status on this issue please? Do you accept to add this feature?
If yes, I may try to implement it, I took a look at the code already, it seems feasible.
Bonus questions if it's ok to go this way:

  • What about the new annotation? Here's a first draft:
package lombok.extern.custom;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Retention(RetentionPolicy.SOURCE)
@Target(ElementType.TYPE)
public @interface CustomLogger {
    /** @return The category of the constructed Logger. By default, it will use the type where the annotation is placed. */
    String topic() default "";

    /** @return Mandatory logger factory class */
    Class<?> factory();
    
    /** @return Mandatory logger class */
    Class<?> logger();

    /** @return The factory method to retrieve the logger. */
    String methodName() default "getLogger";
}

In particular, 2 classes must be passed to this annotation: the logger factory, and the resulting logger, since they are often different.
The factory method is often getLogger so we keep this default value, but allow to override it.

  • What about lombok.eclipse.handlers.HandleLog#processAnnotation and lombok.javac.handlers.HandleLog#processAnnotation signatures? Currently they take the optional topic as last parameter.
  1. Should we add the 3 new parameters (factory, logger, factory method) to this method? And for all other handlers, we would pass null values.
public static void processAnnotation(LoggingFramework framework, AnnotationValues<?> annotation, JavacNode annotationNode, String loggerTopic, Class<?> factory, Class<?>logger, String factoryMethod)
  1. Same scenario but instead of invoking processAnnotation with 3 null parameters, we keep a processAnnotation without those parameters, which invokes the new one with null values (keeps handlers cleaner).
public static void processAnnotation(LoggingFramework framework, AnnotationValues<?> annotation, JavacNode annotationNode, String loggerTopic) {
  processAnnotation(framework, annotation, annotationNode, loggerTopic, null, null, null);
}
  • Besides the HandleLog classes and obviously the new annotation CustomLogger, I identified the following classes to impact, do you see anything else?
    • lombok.ConfigurationKeys: add a ConfigurationKey<FlagUsageType> LOG_CUSTOMLOGGER_FLAG_USAGE
    • Add 3 test files LoggerCustomLogger in ./test/transform/resource/{before|after-delombok|after-ecj} with the 3 scenarios of other loggers (FQN, import, import + topic) + new scenarios implied by the custom logger (with or without factory method).
    • All other logger annotations, since each javadoc refers to all other logger annotations via a @see annotation.
    • ./website/templates/features/log/html to document this new feature

What's your opinion?

@Maaartinus
Copy link
Contributor

@sir4ur0n I guess, configuration is more important than annotation arguments as you probably want to nearly always use the same logger of yours. You probably want both.

FWIW, I'm using an Eclipse shortcut generating

private static final MyLogger logger = MyLoggerFactory.newLogger();

where the topic gets obtained via new Exception().getStackTrace()[1] (it's only once per class, so it's cheap). Unlike when being explicit, it doesn't break when the class gets renamed. It's not much longer than the annotation. ;)

@sir4ur0n
Copy link

sir4ur0n commented Nov 4, 2017

@Maaartinus Good point on the Lombok configuration, this probably makes more sense.
Thus the annotation attributes would always be optional.

As for the trick to retrieve the class name, I'm not sure I understand the point. Lombok loggers already retrieve the class name, so there's no problem with that.

@Maaartinus
Copy link
Contributor

As for the trick to retrieve the class name, I'm not sure I understand the point. Lombok loggers already retrieve the class name, so there's no problem with that.

@sir4ur0n Indeed, Lombok loggers are fine in this respect. Only manually declared loggers like

private static final Logger logger = LoggerFactory.getLogger(nameOfThedeclaringClass);

suffer from this problem. No idea why I mentioned that.

@projectlombok projectlombok deleted a comment from anotherwally Apr 9, 2018
@projectlombok projectlombok deleted a comment from sir4ur0n Apr 9, 2018
@rzwitserloot
Copy link
Collaborator

It should SOLELY be configured via lombok.config; spewing a gigantic annotation with all the bells and whistles about how your custom logger works is just piling on even more boilerplate, that doesn't seem useful.

However, what if you want to mix em up? I know it's rare, but now presumably there's @CustomLogger which would require a configuration, but what if you want different custom loggers?

About using closures for 'efficiency': On most JVMs I doubt that's a solid idea. At any rate, we could add a lombok feature where any log.trace(...) statement is replaced with if (log.isTraceEnabled()) log.trace(...), if there's really a need for that. However, I bet the java community is going to move to log libs which can deal with closures, java will make them efficient, and idiomatic java becomes log.trace(() -> expensiveOperation());

@sir4ur0n
Copy link

Hi @rzwitserloot I'm not sure I understand why you closed this issue?

The only reason why it would be useful to configure it in the Java annotation is so users create their own annotations (e.g. @MyAmazingLogger) which would configure the generic @CustomLogger a certain way, and that way each application may create several overrides of @CustomLogger (maybe one for perf, one for business, one for tech, etc).
And for each the field name could be different.

@ruiruige
Copy link

ruiruige commented Jul 3, 2018

sorry to comment on a closed issue, but I have the same requirements like:

@Slf4j(loggerClass= MyLogger.class)
public class MyClass

or the param is a loggerFactory

Becasue sometimes we really can not use the default one.

@jyeary
Copy link

jyeary commented Oct 8, 2018

We have the same kind of requirement. We have a wrapped logger from ESAPI for Log4J. It would be nice to be able to use an annotation like @ESAPILog4J. However that is a lot less flexible than being able to use @CustomLogger(loggerClass=ESAPILogger.class) or @CustomLogger(factory=ESAPILogger.class) which is like the original request.

@rspilker
Copy link
Collaborator

rspilker commented Oct 8, 2018

  1. Presumably, you have a lot of calls to create a specific logger. So you don't want to do a lot of configuration in each annotation, otherwise you would be better of just creating the logger yourself. So you want some configuration, probably in lombok.config.

  2. This configuration should specify the fully qualified name of the type of the field, The fully qualified name of the logger factory, The name of the factory method. And possibly if the factory method needs a String parameter or a class parameter. But if the other ones are configurable, you could create a static method anywhere to do any conversion.

@jyeary
Copy link

jyeary commented Oct 8, 2018

@rspilker Yes, I agree it could be handled someplace like lombok.config. I am just looking for a means of providing a custom logger regardless of the more intimate details. If we are in agreement on the idea for a custom logger, the details of configuration should match the existing mechanisms as much as possible, or use a mechanism in place for that specific logger. The ESAPI configuration handles the configuration of the logger in this case.

@rspilker
Copy link
Collaborator

rspilker commented Oct 8, 2018

If someone wants to pick this up, we would accept pull requests. Before starting on this, please contact me and/or Reinier on how to approach this.

Typically we start with a design, using before and after code. We can move those designs to the tests directory afterwards.

It does not need to be in lombok.extern, since there should not be any external libraries involved.

Currently, I think @lombok.CustomLog would be a good name.

@rspilker
Copy link
Collaborator

rspilker commented Oct 8, 2018

Regarding parameters for the factory, I think an enum can be used:

  • String
  • Type
  • None

@littlefisher666
Copy link

I need this feature, too.

@Diarsid
Copy link

Diarsid commented May 6, 2019

Hi all.

I also need this feature.
Basically I came to it when our application became clusterized. It has several instances deployed in the same tomcat and we would like to distinguish which line is written by which instance.

Let's assume we have class MyClass that writes some data into log on line 70. Usually we would see something like

[INFO] MyClass:70 - data
[INFO] MyClass:70 - data
[INFO] MyClass:70 - data
[INFO] MyClass:70 - data

From this log it is not possible to understand which node created which line. I have an id of every node in the cluster and what I want is to write some prefix with this id like

[INFO] [node:1] MyClass:70 - data
[INFO] [node:2] MyClass:70 - data
[INFO] [node:1] MyClass:70 - data
[INFO] [node:2] MyClass:70 - data

I can achieve it easily by custom slf4j logger wrapper like this:

public class NodeAwareLoggerWrapper implements Logger {

    private final int nodeId;
    private final Logger wrappedLogger;

    public NodeAwareLoggerWrapper(int nodeId, Logger wrappedLogger) {...}
}

but it requires manual logger creation because it is not possible with lombok annotation.
It would be really nice to have a custom logger extension in lombok.

@iherasymenko
Copy link

@Diarsid have you considered https://logback.qos.ch/manual/mdc.html ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests