-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Jira logging #5351
base: main
Are you sure you want to change the base?
Improve Jira logging #5351
Conversation
Signed-off-by: Maxwell Brown <[email protected]>
Signed-off-by: Maxwell Brown <[email protected]>
@@ -75,7 +75,7 @@ public LeaderProgressState convertToPartitionState(final String serializedPartit | |||
try { | |||
return objectMapper.readValue(serializedPartitionProgressState, LeaderProgressState.class); | |||
} catch (final JsonProcessingException e) { | |||
LOG.error("Unable to convert string to partition progress state class ", e); | |||
LOG.error("Unable to convert string to partition progress state class due to {}", e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we print the String partition progress state as well here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Signed-off-by: Maxwell Brown <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this clean up
@@ -38,7 +38,7 @@ public static InetAddress getInetAddress(String url) { | |||
try { | |||
return InetAddress.getByName(new URL(url).getHost()); | |||
} catch (UnknownHostException | MalformedURLException e) { | |||
log.error(INVALID_URL, e); | |||
log.error(INVALID_URL + " : {}", url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.error(INVALID_URL + " : {}", url); | |
log.error("{}: {}", INVALID_URL, url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
authConfig.renewCredentials(); | ||
} else if (statusCode == HttpStatus.TOO_MANY_REQUESTS) { | ||
log.error(NOISY, "Hitting API rate limit. Backing off with sleep timer.", ex); | ||
log.error("Hitting API rate limit. Backing off with sleep timer."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be noisy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its not needed because it's only one line and retries are capped at 6
log.error(NOISY, "Hitting API rate limit. Backing off with sleep timer.", ex); | ||
log.error("Hitting API rate limit. Backing off with sleep timer."); | ||
} else { | ||
log.error(NOISY, "Exception: ", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide a more useful exception line here. Also, we know that this is an HTTP client status code when we get here. It is just one we don't know.
For example:
log.error(NOISY, "Received an unexpected status code when requesting from Jira. statusCode={}", statusCode);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -119,19 +119,21 @@ private <T> ResponseEntity<T> invokeRestApi(URI uri, Class<T> responseType) thro | |||
} catch (HttpClientErrorException ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if another type of exception is thrown here? This will throw it back. Do we have other error handling for that?
Also, we appear to not have error handling for 5xx errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added error logging for unknown exception, added error handling for some 5xx errors
Signed-off-by: Maxwell Brown <[email protected]>
Description
prevent over logging stack traces in jira source
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.