Camel
  1. Camel
  2. CAMEL-3759

After switching to slf4j, we can get rid of the 'isTraceEnabled', 'isDebugEnabled' and 'isInfoEnabled' statements

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.6.0
    • Fix Version/s: 2.8.0
    • Component/s: None
    • Labels:
      None

      Description

      we can get rid of the 'isTraceEnabled', 'isDebugEnabled' and 'isInfoEnabled' statements with slf4j and use

      logger.debug("Temperature set to {}. Old temperature was {}.", t, oldT);
      

      instead

      christian-muellers-macbook-pro:camel cmueller$ egrep -r 'isTraceEnabled|isDebugEnabled|isInfoEnabled]' . | wc -l
      1485

        Issue Links

          Activity

          Hide
          Claus Ibsen added a comment -

          You can only use at most 2 placeholders like that. If you need 3 or more, you have to do it the old fashion way.
          Unfortunately sfl4j doesn't support varargs.

          Show
          Claus Ibsen added a comment - You can only use at most 2 placeholders like that. If you need 3 or more, you have to do it the old fashion way. Unfortunately sfl4j doesn't support varargs.
          Hide
          Christian Müller added a comment -

          slfj4 also has the following interface:

          public void info(String format, Object[] argArray);
          

          With this it's possible to get rid of the isXXXEnabled() statements.

          I also found some log statements like this one:

          if (log.isInfoEnabled()) {
              log.info("Loaded " + typeMappings.size() + " type converters in " + TimeUtils.printDuration(watch.stop()));
          }
          

          which we could refactor to

          log.info("Loaded {} type converters in {}"), typeMappings.size(), TimeUtils.printDuration(watch.stop());
          

          but I assume "TimeUtils.printDuration(watch.stop())" is an expensive operation which should only be executed if we want log this statement. Therefore I would refactor some similar parts of the code to

          if (log.isInfoEnabled()) {
              log.info("Loaded {} type converters in {}"), typeMappings.size(), TimeUtils.printDuration(watch.stop());
          }
          
          Show
          Christian Müller added a comment - slfj4 also has the following interface: public void info( String format, Object [] argArray); With this it's possible to get rid of the isXXXEnabled() statements. I also found some log statements like this one: if (log.isInfoEnabled()) { log.info( "Loaded " + typeMappings.size() + " type converters in " + TimeUtils.printDuration(watch.stop())); } which we could refactor to log.info( "Loaded {} type converters in {}" ), typeMappings.size(), TimeUtils.printDuration(watch.stop()); but I assume "TimeUtils.printDuration(watch.stop())" is an expensive operation which should only be executed if we want log this statement. Therefore I would refactor some similar parts of the code to if (log.isInfoEnabled()) { log.info( "Loaded {} type converters in {}" ), typeMappings.size(), TimeUtils.printDuration(watch.stop()); }
          Hide
          Christian Müller added a comment -

          Committed r1084858
          removed the unnecessary isInfoEnabled() statements

          Show
          Christian Müller added a comment - Committed r1084858 removed the unnecessary isInfoEnabled() statements
          Hide
          Christian Müller added a comment -

          Committed r1085587
          removed the unnecessary isTraceEnabled() statements in camel-core

          Show
          Christian Müller added a comment - Committed r1085587 removed the unnecessary isTraceEnabled() statements in camel-core
          Hide
          Christian Müller added a comment -

          Committed r1086028
          removed the unnecessary isTraceEnabled() statements in components

          Show
          Christian Müller added a comment - Committed r1086028 removed the unnecessary isTraceEnabled() statements in components
          Hide
          Christian Müller added a comment -

          Committed r1089960
          removed the unnecessary isDebugEnabled() statements in camel-core

          Show
          Christian Müller added a comment - Committed r1089960 removed the unnecessary isDebugEnabled() statements in camel-core
          Hide
          Claus Ibsen added a comment -

          Christian you cannot just do this everywhere. The single line is only possible if the parameters is not to be computed or creating new objects.

          So the ones who creates a new Object[] for the parameters etc, should still be guarded with the isDebugEnabled etc.
          Also some of the operations on the parameters may take time to compute, so its really not always a good idea.

          IMHO the isXXX should be used when the parameters are not simple and if they invoke operations to get data (and the operation is not a simple getter, the compiler can inline).

          Show
          Claus Ibsen added a comment - Christian you cannot just do this everywhere. The single line is only possible if the parameters is not to be computed or creating new objects. So the ones who creates a new Object[] for the parameters etc, should still be guarded with the isDebugEnabled etc. Also some of the operations on the parameters may take time to compute, so its really not always a good idea. IMHO the isXXX should be used when the parameters are not simple and if they invoke operations to get data (and the operation is not a simple getter, the compiler can inline).
          Hide
          Christian Müller added a comment -

          That's exactly what I want/did. I will review my changes. May be I was to optimistic by some getters or ...

          Show
          Christian Müller added a comment - That's exactly what I want/did. I will review my changes. May be I was to optimistic by some getters or ...
          Hide
          Claus Ibsen added a comment -

          Christian is there any more work on this?

          Show
          Claus Ibsen added a comment - Christian is there any more work on this?
          Hide
          Christian Müller added a comment -

          Unfortunately yes. At present I work on to remove the unnecessary isDebugEnabled() in the camel components. Afterwards I have to review my first commits as you suggested. I think I can finish this work until end of next week (5/29/2011).

          Show
          Christian Müller added a comment - Unfortunately yes. At present I work on to remove the unnecessary isDebugEnabled() in the camel components. Afterwards I have to review my first commits as you suggested. I think I can finish this work until end of next week (5/29/2011).
          Hide
          Christian Müller added a comment -

          Committed r1134194
          removed the unnecessary isDebugEnabled() statements in components
          Have to go through my previous commits and check they

          Show
          Christian Müller added a comment - Committed r1134194 removed the unnecessary isDebugEnabled() statements in components Have to go through my previous commits and check they
          Hide
          Claus Ibsen added a comment -

          Christian can you look into this, as we would like to be ready to cut Camel 2.8 later this month.

          Show
          Claus Ibsen added a comment - Christian can you look into this, as we would like to be ready to cut Camel 2.8 later this month.

            People

            • Assignee:
              Christian Müller
              Reporter:
              Christian Müller
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development