Logging and profiling - automatic logging through annotations

I am working with a little project intended to allow applications to benefit from aspect oriented programming. The first aspect is logging, which is already working. However I want this code to be more performatic and cleaner.

Whole project

The basic goals is to annotate methods - private/public static or instance - with and Custom annotation @LoggableObjects. This annotation has options to define level of log - how much information to log (only args values, profiling information).

Since it uses reflection in many steps, I am afraid I might have missed something. I would appreciate if anyone reviewed my code.

This code processes the annotations and instruments the methods. How can the method aroundObjects be better executed? Is this a valid idea?

```
package org.greentea.aspect.log;

import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.Signature;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Aspect;
import org.greentea.aspect.log.annotation.LoggableObject;

/**
* LoggableObjects are either Methods or Constructors that migh be logged
* The information logged is the method signature, its arguments and execution
* time
*
* The level of verbosity, and information might be configurable through the use
* of the annotation @LoggableObjet
*
* @author Filipe Gonzaga Miranda
*/
@Aspect
public class LoggableObjects {

PluggableLogger pluggableLogger;

public static ConcurrentMap, PluggableLogger> cachedLoggers = new ConcurrentHashMap<>();

static{
cachedLoggers.putIfAbsent(DefaultPluggableLoggerIfNotInjected.class, new DefaultPluggableLoggerIfNotInjected());
}

/**
* Captures the Annotations {@link LoggableObjects}
*
* And applies the logic to decid

I know little about AspectJ, but have a number of more basic points to call out in your code.

Hygiene

• cachedLoggers should be private static final and not public static. Do you really want anyone to be able to replace your instance?

• PluggableLogger pluggableLogger is an instance field, but it should be in the method only (perhaps just called logger). As things are at the moment, you risk a race condition when the method is called concurrently.... you could be overwriting the logger part-way through one call, with another call.

• throws Throwable .... really? you should always narrow your exceptions down more than that.

• throw new AssertionError(...) ... really? Why not just an IllegalArgumentException?

Performance

• The Pattern.compile(...) should be a private-static-final field. The patterns are thread-safe, and designed to be reused.

• Instead of a ConcurrentMap I would consider a thread-local HashMap. This would require benchmarking to resolve, but, it could be significantly better at memory management, especially when there's only the default plugin used.

• You can do the isDisabled check before getting the declaring class, and method name

-
You jump through hoops getting the clazzPluggLogg ..:

Class clazzPluggLogg = loggObject.pluggableLoggerClass();

if(clazzPluggLogg != DefaultPluggableLoggerIfNotInjected.class){
    if(cachedLoggers.containsKey(clazzPluggLogg)){
        pluggableLogger = cachedLoggers.get(clazzPluggLogg);
    }else{
        pluggableLogger = clazzPluggLogg.newInstance();
        cachedLoggers.putIfAbsent(clazzPluggLogg, pluggableLogger);
    }
}else{
    pluggableLogger = cachedLoggers.get(clazzPluggLogg);
}

this can be done instead as:

Class clazzPluggLogg = loggObject.pluggableLoggerClass();
pluggableLogger = cachedLoggers.computeIfAbsent(clazzPluggLogg, k -> k.newInstance());

The above will lock the map only once, not multiple times.

StackExchange Code Review Q#92959, answer score: 2