0
\$\begingroup\$

I was trying to wrap a Logback logger in order to provide some handy methods and already defined default keys of the logged json output and I came up with something like this.

Do you spot any problem with it, or do you have any suggestion?

public class Log {

    private Logger logger = LoggerFactory.getLogger("jsonLogger");

    private String msg;
    private long start;
    private Map<String, Object> kvs;

    private Log(String msg) {
        this.msg = msg;
        this.start = System.currentTimeMillis();
        this.kvs = new HashMap<>();
    }

    public static Log msg(String msg) {
        return new Log(msg);
    }

    public Log kv(String key, Object value) {
        kvs.put(key, value);
        return this;
    }

    public void log() {
        kvs.put("elapsed", System.currentTimeMillis() - start);
        List<StructuredArgument> args = new ArrayList<>();
        for (String k : kvs.keySet()) {
            args.add(StructuredArguments.kv(k, kvs.get(k)));
        }
        logger.info(msg, args.toArray());
    }

}

In this way you can log just a message, custom key-values and have a way to keep track of the execution between a function:

// just a message
Log.msg("my message").log();

// message with custom kv or complex objects
Log.msg("my message").kv("foo", "bar").kv("arr", new String[]{"a", "b", "c"}).log();

// elapsed time
Log logger = Log.msg("my message").kv("foo", "bar");
Thread.sleep(1240);
logger.log();
\$\endgroup\$
-1
\$\begingroup\$

i do not think this is really a good idea (sorry to say that)...

why is is not a good idea? it's breaking the concept of segregaton of concerns:

  1. a Logger is responsible for logging
  2. a Profiler is responsible for measuring execution time
  3. a Logger is NOT responsible for keeping track of items (who is responsible for cleaning these entries?)

another issue i see is that you create a new logger whenever you want to write something into you log, i don't see any reason for that...

what would i suggest?

create a timesheet

public class Timesheet{

   private final Map<TimeStamp, KeyValue kv> entries;
   private final Logger logger;

    public Timesheet(Logger logger){
        this.logger = logger
        entries= ... 
    }

    public Timesheet kv(Object key, Object value) {
        TimeStamp timestamp = new TimeStamp();
        entries.put(timestamp , new KeyValue(key, value));
        log.debug("adding key/value {}/{} at {}", key, value, timestamp );
        return this;
    }

    public Timesheet msg(Object msg) {
        logger.debug(msg);
        return this;
    }

    public Timesheet elapsed() {
        logger.debug("time elapsed {}", calculateTimeElapsed());
        return this;
    }

}

not all is implemented here, it should just girve you an idea of how you could do it...

Timesheet timesheet = new Timesheet(LoggerFactory.getLogger("jsonLogger")); //now availible for all classes
timesheet.msg("hello");
timesheet.kv("foo", "bar");
Thread.sleep(1234);
timesheet.elapsed();

some final words...

i think it is not a good idea of using KeyValue pairs - if you know what kind of entries these are, give them a proper name!

i think if you would have choosen a more compliant name (Log is really confusing) your idea would not have been so bad...

\$\endgroup\$
  • \$\begingroup\$ I beg to differ. 1) this isn't violated, it's still logging 2) OP added a profiler via their own extensions 3) it's not keeping any items but OP's extensions. The API might not be the prettiest one but in general there is nothing wrong with it. I use similar extensions myself. Lastly i think it is not a good idea of using KeyValue pairs this is also not true because you cannot know the names of the fields. A logger must be pretty flexible so using a dictionary was the right choice. \$\endgroup\$ – t3chb0t Jan 7 at 10:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.