I favor providing a detailed log message that includes the Throwable as an
argument.
I have seen code like the following in Geode:
} catch (Exception exception) {
logger.error(exception.getMessage());
}
...but I consider that to be a naive anti-pattern for informing the user
that a problem occurred. For one thing, getMessage() is not guaranteed to
have a non-null value even for Exceptions thrown by the JDK. For example,
NullPointerExceptions thrown by the JDK usually (if not always) have a null
message and the result in the Geode log is:
null
...which is clearly not useful and is very confusing. It actually just
looks like a bug.
I think any error-handling that logs an exception should include a log
message with at least some basic context about what was attempted that
resulted in the exception:
logger.error("Attempt to perform operation foo resulted in {}",
exception);
The above will then use exception.toString() for {} which provides much
better detail to the user about what problem actually occurred.
On Tue, May 28, 2019 at 10:50 AM Bruce Schuchardt <bschucha...@pivotal.io>
wrote:
> In some ways I agree with Owen - it's always better to have more
> information about a problem. I can recall seeing blank error messages
> or "null" on many occasions.
>
> In other ways I agree with Dan and Anthony that this needs to not be a
> global search&replace, but I think that's mostly because our alert
> system is intertwined with our logging system. An operator wouldn't
> know what do make of "UnknownHostException" in an alert but someone
> doing forensic analysis might want to see that exception name in a log
> file.
>
> On 5/28/19 10:03 AM, Anthony Baker wrote:
> > In the example you provided, I don’t agree that adding the exception
> class name creates a better user experience.
> >
> > Anthony
> >
> >
> >> On May 25, 2019, at 6:39 PM, Owen Nichols <onich...@pivotal.io> wrote:
> >>
> >> Here’s an example of a message that was logged before Jack’s change:
> >>
> >> l192.168.99.1: nodename nor servname provided, or not known
> >>
> >> Here’s what it will look like now with .toString() instead of
> .getMessage():
> >>
> >> java.net.UnknownHostException: l192.168.99.1: nodename nor servname
> provided, or not known
> >>
> >>
> >> I cannot think of a single good reason to ever print an exception’s
> message stripped of all context. As Jack noted, many exceptions don’t even
> have a message; the key information is the class of the exception itself.
> Even if you aren’t catching Exception but rather some very specific
> subclass, code should never make assumptions about the text of an exception
> (see PR#3616 <https://github.com/apache/geode/pull/3616> for a recent
> example).
> >>
> >>
> >> @jinmei There is no built-in method in java to capture the entire
> stacktrace as a string. Exception::toString() just concatenates the
> exception class name and message (i.e. the first line only of a full
> stacktrace)
> >>
> >>
> >>> On May 24, 2019, at 3:37 PM, Dan Smith <dsm...@pivotal.io> wrote:
> >>>
> >>> I think the right thing to do in those 100+ cases may be different in
> each
> >>> case, a blanket search and replace might not be the best idea.
> >>>
> >>> -Dan
> >>>
> >>>
> >>>
> >>> On Fri, May 24, 2019 at 3:05 PM Jinmei Liao <jil...@pivotal.io> wrote:
> >>>
> >>>> does exception.toString() print out the entire stack trace? If so, I
> don't
> >>>> think we should use that to replace exception.getMessage().
> >>>>
> >>>> On Fri, May 24, 2019 at 1:18 PM Jack Weissburg <jweissb...@pivotal.io
> >
> >>>> wrote:
> >>>>
> >>>>> Hi All,
> >>>>>
> >>>>> Owen and I investigated a strange error message caused because Geode
> >>>>> previously handled an error exception with exception.getMessage()
> instead
> >>>>> of
> >>>>> exception.toString().
> >>>>>
> >>>>> The issue is that the exception message from exception.getMessage()
> could
> >>>>> be unhelpful or empty. Instead, we should return the whole exception
> via
> >>>>> exception.toString().
> >>>>>
> >>>>> exception.getMessage()is used widely throughout the Geode code base
> (100+
> >>>>> times) and could be prone to cause more issues similar to
> >>>>> https://issues.apache.org/jira/browse/GEODE-6796
> >>>>> I would like to change the remaining instances of
> >>>> exception.getMessage()to
> >>>>> avoid future issues of this variety.
> >>>>>
> >>>>> Thoughts? Comments?
> >>>>>
> >>>>> Best,
> >>>>> Jack Weissburg
> >>>>>
> >>>>
> >>>> --
> >>>> Cheers
> >>>>
> >>>> Jinmei
> >>>>
>
