4 Replies Latest reply on Aug 22, 2019 10:55 AM by ochaloup

    An approach to substitute e.printStackTrace() for exception traces being printed with the logger

    ochaloup
    ochaloup Aug 21, 2019 10:36 AM

      There was recently a discussion at a pull request about leaving or not leaving e.printStackTrace() calls in the code.

      JBTM-3178 Add warning for AbstractRecord create failure by mmusgrov · Pull Request #1486 · jbosstm/narayana · GitHub

       

      From my perspective use of the calls of Exception.printStackTrace() should be avoided and if possible the code should be cleaned up from parts where bad practices are used. This is one of them (Best Practices | PMD Source Code Analyzer ).

      For me the only valid reason to talk about leaving a bad practice code is because of the backward compatibility. Which I personally do not consider being the case here. It's hard for me to imagine a developer depending on a specific exception trace to be thrown to stderr.

       

      To reference the Tom's opinion on this from the issue is here: JBTM-3178 Add warning for AbstractRecord create failure by mmusgrov · Pull Request #1486 · jbosstm/narayana · GitHub

      Where he says that

      • we have to consider whether making the change outweighs the benefit of users who may be relying on the old behaviour

       

      I'm interested to find and understand what are the possible issues with changing the printStackTrace() for the logger, are there some?

       

      Thank you

      Ondra

        • 1. Re: An approach to substitute e.printStackTrace() for exception traces being printed with the logger
          tomjenkinson
          tomjenkinson Aug 21, 2019 11:13 AM (in response to ochaloup)

          I think it is possible that someone could rely on the current behavior for stack traces. I would be interested to hear from jhalliday what his opinion is on cleaning up the old stack traces and whether that could be a problem for current users?

            • Actions
          • 2. Re: An approach to substitute e.printStackTrace() for exception traces being printed with the logger
            jhalliday
            jhalliday Aug 22, 2019 8:52 AM (in response to tomjenkinson)

            I agree it's bad practice, but I also don't see much upside to putting effort into a change nobody is asking for. It runs the risk of breaking things, in return for what? Stopping PMD from moaning? meh. Difficult to get excited about this one way or the other. I suppose there is an outside chance it will break monitoring infrastructure that's expecting things to be in stderr instead of wherever the logger is redirecting to, but I wouldn't consider that part of the public/supported API anyhow, so I don't think it's unreasonable to change. The closest we've seen before is probably the infamous JBTM-575, but that was programmatic access to the exception, not text access to the stderr/log output.

              • Actions
            • 3. Re: An approach to substitute e.printStackTrace() for exception traces being printed with the logger
              tomjenkinson
              tomjenkinson Aug 22, 2019 9:25 AM (in response to jhalliday)

              Thanks Jonathan.

               

              I think the summary is that there is no need to change a printStackTrace to use a logger but we shouldn't be adding printStackTraces.

                • Actions
              • 4. Re: An approach to substitute e.printStackTrace() for exception traces being printed with the logger
                ochaloup
                ochaloup Aug 22, 2019 10:55 AM (in response to tomjenkinson)

                To be honest I personally don't agree with the summary.

                 

                I understand in in way we are in agreement that from the backward compatibility perspective hardly somebody depends on the stacktrace structure being printed to stderr.

                I don't agree that bad practices should not be fixed. I consider the usage of the e.printStackTraces as a bad practice which should be avoided and if possible it's good to be fixed. If the logger is used instead of the stderr you can configure it in any way you want. You have benefit of flexibility to send the log stream to whatever direction you want. You can say that error messages will go to stderr and any other is discarded. You can redirect your log messages to be send over network to a central storage and you can run a queries over them etc.

                 

                I don't think it's a priority, I don't think there should be some direct effort on fixing these things but I think if somebody works on an issue related to logging of errors then it (and any other bad programming practice) should be fixed.

                  • Actions