Support for CustomLog

[juriad]

I started to implement support for @CustomLog annotation as discussed in #843.

This PR proposes the new annotation and how it can be configured. This is work in progress; I haven't tested it at all yet. I'd like to discuss the design, my approach, and the code style.

[ghost]

Hi all, this is a great addition and that we could use on our project. When could someone take a look at this?

[rzwitserloot]

Apologies for the late reply.

This is heading in the right direction:

IdentifierName

Your addition to the config system with java identifiers (IdentifierName) is an excellent idea. If you can find your way into fixing up all uses of java identifiers in the config system, that'd be great. If you don't have the time, let us know and we'll try to fix it up before merging the lot.

However... my preferred suggestion for how to tackle this means you can't use IdentifierName for it.

@CustomLog

We've discussed an alternative take on this feature where you define a custom annotation as being the equivalent of, say, @Slf4j, but this would require a kind of dynamism lombok doesn't currently have. So, going with @CustomLog, as you've done, has our preference.

Purpose of the feature

We're a bit conflicted on the purpose of @CustomLog. There are 2 main reasons to use it:

1. There's a more or less publicly available log framework out there which isn't already supported by one of the logger annotations in the lombok.extern package, and @CustomLog is faster than asking us to add it.

2. You've built your own logging framework as an inside job; this framework is not public enough to feasibly take the route of asking us to explicitly support it via lombok.extern. @CustomLog is the only way.

Even for option 1, but especially for option 2, we can dictate that your logging factory needs to fit a certain signature. This way, we can reduce the config requirements considerably.

Here are the settings I can imagine. Given:

private static final LoggerType log = LoggerFactoryType.loggerCreatorMethod(parameters);

The configurable aspects are:

1. LoggerType
2. LoggerFactoryType
3. loggerCreatorMethod
4. preference as to whether loggers should be static or non-static
5. If topic is provided in the annotation: [1] error out, [2] pass just the topic, [3] pass the self-class as parameter 1, and the topic as parameter 2, [4] pass the self-class.toString() as string param, and the topic as parameter 2, [5] like 3 but the params reversed, [6] like 4 but the params reversed.
6. If there's no topic, same question, but it's simpler here: [1] pass nothing, [2] pass the self-class as parameter, [3] pass the self-class.toString() as parameter.

That's a lot of configuring and you've implemented most of this. However, if we go with the mindset that it's okay to require a wrapper method, then we can do something much simpler. The only 3 configurable entities that are left that seem relevant, is the logger type, the factory type, and the factory method name. We can dictate that the parameters will always be the self-type first as java.lang.Class object, and the topic second, being the empty string if none was provided. It is on the @CustomLog user to ensure that the configuration they've provided leads to a static method that has these properties.

We can provide all this configuration in a single config key. It would have 2 forms; the simple and the complex form.

The simple form: One fully qualified type name + 1 method name, separated by a dot, no spaces.

customLog.declaration = com.foo.MyLogger.createLogger

complex form: Like the simple form, but prefixed to it one fully qualified type, with a single space as separator.

customLog.declaration = com.foo.MyLogger bar.baz.mystuff.MyLoggerFactory.create

The first parameter is the log type, the second is a combination of log factory type + create method. If you do not specify the log type, it is constructed by lopping off the final dot and all characters after it (so, in the simple case above, it'd be com.foo.MyLogger.

[rzwitserloot]

NB: One downside to the above proposal is that use of custom logger which does not allow topics would result in a runtime-only exception (in that the glue method would throw if topic was anything but empty), whereas if it's all configurable, lombok could generate an error sooner, but this seems like a fair price to pay. Having more than one config key does not play well with the cascading nature of the config system. Imagine you set 'fail if topic is set' in your project root, and then in some subdir of your project root you write a lombok config that sets the logger factory method to use (to a different logger that DOES support topics), but fails to reset the 'fail if topic is set' flag. By having only 1 configkey, this problem is avoided.

[juriad]

IdentifierName

I will be happy to apply it to all places and will create a separate PR for it. There are only two other places anyway (marked TODO by my PR).

@CustomLog

I represent option 2 (we have a custom semantic logging framework) but understand and embrace the need for generality. My idea is to be able to cover all lombok-supported logging annotations.

I agree that the configuration must be simpler, ideally expressed as a single property for the cascading config to work well. I didn't (and wouldn't) enjoy writing 5 different config keys. :-)
I would name the configuration key lombok.log.custom.declaration if you don't mind as I would like to keep it underneath lombok.log next to lombok.log.custom.flagUsage.

Even though we could add the wrapper method, I would like to extend the configuration a bit as it won't make things much harder. The value is still based on your idea:

1. optional LoggerType (if omitted it will be the same as LoggerFactoryType) followed by space
2. mandatory LoggerFactoryType followed by dot
3. mandatory loggerCreatorMethod
4. at least one parameter declaration.
   Parameter declaration is a possibly empty comma separated list of TYPE, NAME, TOPIC wrapped in parentheses, no spaces allowed.

Examples showing different settings for parameters:

com.foo.MyLogger bar.baz.mystuff.MyLoggerFactory.create() // instead of NONE
com.foo.MyLogger bar.baz.mystuff.MyLoggerFactory.create(TYPE)
com.foo.MyLogger bar.baz.mystuff.MyLoggerFactory.create(NAME)
com.foo.MyLogger bar.baz.mystuff.MyLoggerFactory.create(TOPIC) // topic mandatory
com.foo.MyLogger bar.baz.mystuff.MyLoggerFactory.create(TYPE,TOPIC) // topic mandatory
com.foo.MyLogger bar.baz.mystuff.MyLoggerFactory.create()(TOPIC) // topic optional
com.foo.MyLogger bar.baz.mystuff.MyLoggerFactory.create(TYPE)(TYPE,TOPIC) // topic optional
com.foo.MyLogger bar.baz.mystuff.MyLoggerFactory.create()(TOPIC,NAME) // topic optional, provided with class name (crazy setting)
com.foo.MyLogger bar.baz.mystuff.MyLoggerFactory.create(NAME,TYPE)(TOPIC,TOPIC) // even crazier

If topic is set, the parameter declaration with TOPIC is used; otherwise the parameter declaration without TOPIC is used. It is an error for more parameter declarations to match. This forces either one or two parameter declarations.
Note that this generalizes the list of 6, resp. 3 options which you listed above and realizes the desired compile-time error.

Performance

Is there anything wrong with my approach from performance perspective? I assume that programming annotation processor requires stricter rules. Can I afford to use java.util.regex.Pattern to parse the config?

[rzwitserloot]

This looks great to me.

If you treat the configkey as a string and parse it out in the customlogger handler with a regexp, I don't think we'd run into any performance issues there.

[sbartram]

I tested the latest from ataccama:master and it worked fine for me. The one downside is that the IntelliJ plugin will need to be updated since it didn't recognize my custom logger.

[juriad]

@rzwitserloot, I would also like to add support to the IDEA plugin. Have you had time to review my implementation? Do you expect the current proposal to change considerably?

[rzwitserloot]

Gonna look at this now.

[rzwitserloot]

Just a random question @juriad : I noticed you indented the empty lines. Whilst git recommends against this, the way both Roel and I use editors, we like doing this, and it is indeed the lombok 'house style'. This might be the first time I noticed a PR that got this right. Did you notice we do it this way and you copied the style, or, is it also how you write code?

[rzwitserloot]

I've reviewed your code, made some very minor fixes, and updated the docs + changelog.

However, I noticed you haven't added your name to the AUTHORS file yet. For legal purposes we do require it (as part of doing that, you're saying that you're okay with your code being integrated into lombok, as per the lombok open source license, and that you wrote it / have full and permanent permission to relinquish the copyright like that).

@juriad can you review my docs, and add your name to the AUTHORS file?

See https://github.com/rzwitserloot/lombok/tree/customlog

[sbartram]

@juriad any progress on this?

[juriad]

@sbartram
I reviewed the changes, added my name, found some bugs regarding configuration; there is a PR #2136