Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Improve Jira logging #5351

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1618d8f
no stack trace for known errors
Galactus22625 Jan 23, 2025
f7fa348
add else block for error handling in jirarestclient
Galactus22625 Jan 23, 2025
813d53e
print partition progress streate as well
Galactus22625 Jan 23, 2025
7edf388
me when i address comments or something
Galactus22625 Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions ...rce/src/main/java/org/opensearch/dataprepper/plugins/source/jira/rest/JiraRestClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,19 +119,21 @@ private <T> ResponseEntity<T> invokeRestApi(URI uri, Class<T> responseType) thro
} catch (HttpClientErrorException ex) {
Copy link
Member

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.

Copy link
Member Author

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

HttpStatus statusCode = ex.getStatusCode();
String statusMessage = ex.getMessage();
log.error("An exception has occurred while getting response from Jira search API {}", ex.getMessage());
log.error("An exception has occurred while getting response from Jira search API with statusCode {} and error message: {}", statusCode, statusMessage);
if (statusCode == HttpStatus.FORBIDDEN) {
throw new UnAuthorizedException(statusMessage);
} else if (statusCode == HttpStatus.UNAUTHORIZED) {
log.error(NOISY, "Token expired. We will try to renew the tokens now", ex);
log.error("Token expired. We will try to renew the tokens now.");
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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be noisy?

Copy link
Member Author

@Galactus22625 Galactus22625 Jan 24, 2025 •
edited
Loading

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

} else {
log.error(NOISY, "Exception: ", ex);
Copy link
Member

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);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

}
try {
Thread.sleep((long) RETRY_ATTEMPT_SLEEP_TIME.get(retryCount) * sleepTimeMultiplier);
} catch (InterruptedException e) {
throw new RuntimeException("Sleep in the retry attempt got interrupted", e);
throw new RuntimeException("Sleep in the retry attempt got interrupted.");
}
}
retryCount++;
Expand Down
2 changes: 1 addition & 1 deletion ...c/main/java/org/opensearch/dataprepper/plugins/source/jira/rest/auth/JiraOauthConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public String getJiraAccountCloudId() {
if (e.getRawStatusCode() == HttpStatus.UNAUTHORIZED.value()) {
renewCredentials();
}
log.error("Error occurred while accessing resources: ", e);
log.error("Error occurred while accessing resources. Status code: {}. Error message: {}", e.getStatusCode(), e.getMessage());
}
}
throw new UnAuthorizedException(String.format("Access token expired. Unable to renew even after %s attempts", RETRY_ATTEMPT));
Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/org/opensearch/dataprepper/plugins/source/jira/utils/AddressValidation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.error(INVALID_URL + " : {}", url);
log.error("{}: {}", INVALID_URL, url);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

throw new BadRequestException(e.getMessage(), e);
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...rch/dataprepper/plugins/source/source_crawler/coordination/partition/LeaderPartition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}. Partition progress state string: {}.", e.getMessage(), serializedPartitionProgressState);
return null;
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...rch/dataprepper/plugins/source/source_crawler/coordination/scheduler/WorkerScheduler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public void run() {
try {
Thread.sleep(RETRY_BACKOFF_ON_EXCEPTION_MILLIS);
} catch (InterruptedException ex) {
log.warn("Thread interrupted while waiting to retry", ex);
log.warn("Thread interrupted while waiting to retry due to {}", ex.getMessage());
}
}
}
Expand Down
Loading