views:

191

answers:

3

I have recently inherited a J2EE Struts web app that was written back in 2002. There is no logging in the application other than the odd System.out.println().
I have added log4j logging so that I can write out some info to the console, but I'm concerned on how best to approach this. Any suggestions, tips, best practices would be welcome as I don't want to spend too much time adding logging to every method or class (or trying to find what places where logging would be best - ie. bad code blocks/defects).
My current approach is to just add some logging to the few classes that I have been looking at to understand the code, but are there a few key places where I can add logging to maximize my use of adding log4j?

Edit:
Progress update:
I extended the Struts (v.1) ExceptionHandler and configured the struts-config.xml to use my CustomExceptionHandler instead of the Struts version. Then by overriding the execute() method, I added some logic to log the exception using log4j. See below:

public class CustomExceptionHandler extends ExceptionHandler {

 static Logger logger = Logger.getLogger(CustomExceptionHandler.class);

 public ActionForward execute(Exception ex, ExceptionConfig ae, ActionMapping mapping, ActionForm formInstance, HttpServletRequest request, HttpServletResponse response) throws ServletException {
  logException(ex);
  return super.execute(ex, ae, mapping, formInstance, request, response);
 }

 private void logException(Throwable thr) {
   // Add code here to log the exception appropiately
   // i.e. logger.error("Exception is: " + ex.getMessage());
}

The struts-config.xml file had to be updated as well:

<global-exceptions>
<exception key="" type="java.lang.Throwable" handler="com.mycompany.CustomExceptionHandler" />
</global-exceptions>

Now I can be sure that I will always appropiately log any exceptions.
For the exceptions that were getting eaten, I wrap them in unchecked exceptions so that they make it to the top:

} catch (Exception e) {
//nothing here
}

Changed to:

} catch (Exception e) {
 throw new RuntimeException(e);
}
+1  A: 

The most important thing is, I would set up a Struts exception handler in the struts-config.xml to catch everything that was thrown from the actions and log it. Then I'd check all the exception handling to see what exceptions are getting eaten, written to stdout, or otherwise not making it into the log, and make changes so that every exception that can't be usefully handled gets propagated (wrapped in unchecked exceptions if necessary) so that it makes it to the exception handler.

Besides that, I'd do like you describe, adding logging to the parts where it is directly helpful. As long as exceptions are getting thrown and making their way to the exception handler there shouldn't be much more to do.

Nathan Hughes
@Nathan - With regards to your first suggestion, do you mean this: <global-exceptions> <exception key="" type="java.lang.Throwable" handler="com.mycompany.CustomHandler" /> </global-exceptions> . CustomHandler overwrites the execute method. In the execute method I added a method logException(Throwable t){} then call the super.execute() after I log the exception.
Ruepen
yeah something like that. there is a StrutsExceptionHandler that is part of the struts-1 distribution, the only thing wrong with it is it logs everything as DEBUG.
Nathan Hughes
@Nathan: When you say "wrapped in unchecked exceptions if necessary", do you mean the following:catch (IOException e){throw new RuntimeException(e);} catch (Exception e) {throw new RuntimeExcpetion (e);}I see in the code that there is a lot of the following:catch (Exception e) {//nothing here}
Ruepen
Yes, you're seeing exceptions get eaten. Your snippet passing the original exception into the RuntimeException's constructor makes sure the original information isn't lost.
Nathan Hughes
A: 

If the only thing that is going to System.out (System.err) is the old logging and you are looking for a simple solution, you could always redirect System.out to a wrapper class around a logger in the startup of your app's lifecycle:

public class LoggerStream extends PrintStream {
  Logger legacyLogger = Logger.getLogger("app.legacyLogger");

  ...

  public void println(String s){
    legacyLogger.log(s);
  }
}

overriding any other methods like print, etc. It might not generate the prettiest output, but you could use it as a "quick-fix" start and then replace the legacy logging with more appropriate logging as you find items of greater interest.

M. Jessup
@M.Jessup - Thanks for the suggestion, but the System.out seems to have been added back in 2005 when someone needed to see some output in the classes that they were fixing. It was not pervasively used for logging. I hadn't considered a wrapper class, but I'll remember that for next time.
Ruepen
A: 

Are you familiar with aspect oriented programming? This can be a good way to add generalized logging to your application. Here is a good post about simplying your logging with AspectJ .

In struts 2 you can also handle some of your logging with interceptors, but since this project was started in 2002, I am assuming you are using Struts 1?

dbyrne
Yes it is using Struts 1. And thanks for the suggestion of using AspectJ. I will take a look at the link and post some feedback.
Ruepen
@dbyrne: I reviewed AspectJ, but because there is an approval process for any new code implementations, I will not be able to go through with this option. At least for now. Thanks.
Ruepen
@Ruepen: I subscribe to the "its easier to ask forgiveness than permission" philosophy ;)
dbyrne
@dbyrne: Sounds like a good philosophy, but I'm kinda new here and things are on lockdown. Put it this way, I `have` to use IE6 here, nothing else allowed (w/o super special permission).
Ruepen