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

Merged
merged 11 commits into from
Jan 30, 2025
Merged

Improve Jira logging #5351

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 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
e9f389e
remove unused metric
Galactus22625 Jan 25, 2025
a8b00f1
ensure hosts is list of length 1
Galactus22625 Jan 25, 2025
9a7c8f0
change error to info log
Galactus22625 Jan 29, 2025
275309a
NOISY flag for logs
Galactus22625 Jan 29, 2025
ff1dce8
sleep timer logging and secret refresh error and logging changes
Galactus22625 Jan 30, 2025
70411df
address comments
Galactus22625 Jan 30, 2025
aa7947f
fix refresh because test failing
Galactus22625 Jan 30, 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
2 changes: 1 addition & 1 deletion ...jira-source/src/main/java/org/opensearch/dataprepper/plugins/source/jira/JiraService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ private StringBuilder createIssueFilterCriteria(JiraSourceConfig configuration,
.collect(Collectors.joining(DELIMITER, PREFIX, SUFFIX)))
.append(CLOSING_ROUND_BRACKET);
}
log.error("Created issue filter criteria JiraQl query: {}", jiraQl);
log.info("Created issue filter criteria JiraQl query: {}", jiraQl);
return jiraQl;
}

Expand Down
6 changes: 6 additions & 0 deletions ...source/src/main/java/org/opensearch/dataprepper/plugins/source/jira/JiraSourceConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import com.fasterxml.jackson.annotation.JsonProperty;
import jakarta.validation.Valid;
import jakarta.validation.constraints.AssertTrue;
import lombok.Getter;
import org.opensearch.dataprepper.plugins.source.jira.configuration.AuthenticationConfig;
import org.opensearch.dataprepper.plugins.source.jira.configuration.FilterConfig;
Expand All @@ -31,6 +32,11 @@ public class JiraSourceConfig implements CrawlerSourceConfig {
@JsonProperty("hosts")
private List<String> hosts;

@AssertTrue(message = "Jira hosts must be a list of length 1")
boolean isValidHosts() {
return hosts != null && hosts.size() == 1;
}

/**
* Authentication Config to Access Jira
*/
Expand Down
20 changes: 12 additions & 8 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 @@ -51,13 +51,11 @@ public class JiraRestClient {
public static final List<Integer> RETRY_ATTEMPT_SLEEP_TIME = List.of(1, 2, 5, 10, 20, 40);
private static final String TICKET_FETCH_LATENCY_TIMER = "ticketFetchLatency";
private static final String SEARCH_CALL_LATENCY_TIMER = "searchCallLatency";
private static final String PROJECTS_FETCH_LATENCY_TIMER = "projectFetchLatency";
private static final String ISSUES_REQUESTED = "issuesRequested";
private final RestTemplate restTemplate;
private final JiraAuthConfig authConfig;
private final Timer ticketFetchLatencyTimer;
private final Timer searchCallLatencyTimer;
private final Timer projectFetchLatencyTimer;
private final Counter issuesRequestedCounter;
private final PluginMetrics jiraPluginMetrics = PluginMetrics.fromNames("jiraRestClient", "aws");
private int sleepTimeMultiplier = 1000;
Expand All @@ -68,8 +66,6 @@ public JiraRestClient(RestTemplate restTemplate, JiraAuthConfig authConfig) {

ticketFetchLatencyTimer = jiraPluginMetrics.timer(TICKET_FETCH_LATENCY_TIMER);
searchCallLatencyTimer = jiraPluginMetrics.timer(SEARCH_CALL_LATENCY_TIMER);
projectFetchLatencyTimer = jiraPluginMetrics.timer(PROJECTS_FETCH_LATENCY_TIMER);

issuesRequestedCounter = jiraPluginMetrics.counter(ISSUES_REQUESTED);
}

Expand Down Expand Up @@ -119,20 +115,28 @@ 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

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, will make noisy

} else if (statusCode == HttpStatus.SERVICE_UNAVAILABLE) {
log.error("Service unavailable. Will retry after backing off with sleep timer.");
} else if (statusCode == HttpStatus.GATEWAY_TIMEOUT) {
log.error("Gateway timeout. Will retry after backing off with sleep timer.");
} else {
log.error(NOISY, "Received an unexpected status code {} response from Jira.", statusCode, ex);
}
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.");
}
} catch (Exception ex) {
log.error(NOISY, "An exception has occurred while getting a response from the Jira search API", ex);
}
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);
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