Extra logging & minor unit test for comment #2009 #2160
Conversation
| * @return the stack trace as generated by the exception's | ||
| * {@code printStackTrace(PrintWriter)} method, or an empty String if {@code null} input | ||
| */ | ||
| public static String getStackTrace(final Throwable throwable) { |
There was a problem hiding this comment.
there's already a throwableToString() in StringUtils so can you see if that is sufficient or adjust accordingly. I know StringUtils is not the best place for it - but I don't want too many utils
There was a problem hiding this comment.
Yep that produces the same outcome. Will delete this.
| @@ -598,6 +599,7 @@ private void httpInvokeOnce() { | |||
| long responseTime = endTime - startTime; | |||
| String message = "http call failed after " + responseTime + " milliseconds for url: " + httpRequest.getUrl(); | |||
| logger.error(e.getMessage() + ", " + message); | |||
| logger.error(ExceptionUtils.getStackTrace(e)); | |||
There was a problem hiding this comment.
I'm not in favor of this. this is one of the most common paths in karate and I've tried my best to minimize "log noise" over the last few years. as long as the core message comes through as a one-liner I think we should leave it as is. sorry if that sounds harsh but I really don't want to make this kind of a change without being sure.
if you insist, we can put this into the 1.4.0 series and under DEBUG
There was a problem hiding this comment.
Happy to put it into TRACE or DEBUG (with surrounding if). Reason why I was looking for it is a team that has a weird issue of socket connection reset and after triaging all bits and pieces on the server side it just seems to be unrelated and a client issue (code or machine). Right now we loose visibility of this stacktrace at this catch block and doesn't offer much more information to triage from there.
|
PR comments addressed. Funny enough you had mention in a previous PR for me that function already existed :) Seems like I did exactly the same last year haha. Maybe a future iteration in 1.4.0 could consider rearranging a bit the Utils classes. I understand the need to keep it simpler but maybe by just putting them all under a single package it'll make the class name more meaningful. It could also just be me and the ease of use apache-commons which follows that format. |
joelpramos
changed the title
Description
Thanks for contributing this Pull Request. Make sure that you submit this Pull Request against the
developbranch of this repository, add a brief description, and tag the relevant issue(s) and PR(s) below.