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 6 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
18 changes: 10 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,22 +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 with statusCode {} and error message: {}", statusCode, statusMessage);
log.error(NOISY, "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("Token expired. We will try to renew the tokens now.");
log.error(NOISY, "Token expired. We will try to renew the tokens now.");
Copy link
Member

Choose a reason for hiding this comment

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

I think we are expecting this to happen right? So every hour (or whatever the token refresh time is) we hit this. If this is the case, it really should be a warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

changing to log.warn

authConfig.renewCredentials();
} else if (statusCode == HttpStatus.TOO_MANY_REQUESTS) {
log.error("Hitting API rate limit. Backing off with sleep timer.");
log.error(NOISY, "Hitting API rate limit. Backing off with sleep timer for {} seconds.", RETRY_ATTEMPT_SLEEP_TIME.get(retryCount));
Copy link
Member

Choose a reason for hiding this comment

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

You could consolidate these.

else if(statusCode == HttpStatus.SERVICE_UNAVAILABLE || statusCode == GATEWAY_TIMEOUT ...) {
  log.error(NOISY, "Received {}. Will retry after backing off with sleep timer for {} seconds.", statusCode, RETRY_ATTEMPT_SLEEP_TIME.get(retryCount));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

consolidated

} else if (statusCode == HttpStatus.SERVICE_UNAVAILABLE) {
log.error(NOISY, "Service unavailable. Will retry after backing off with sleep timer for {} seconds.", RETRY_ATTEMPT_SLEEP_TIME.get(retryCount));
} else if (statusCode == HttpStatus.GATEWAY_TIMEOUT) {
log.error(NOISY, "Gateway timeout. Will retry after backing off with sleep timer for {} seconds.", RETRY_ATTEMPT_SLEEP_TIME.get(retryCount));
} else {
log.error(NOISY, "Exception: ", ex);
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.");
}
} 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: 2 additions & 0 deletions ...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 @@ -153,6 +153,8 @@ public void renewCredentials() {
this.accessToken = (String) oauth2Config.getAccessToken().getValue();
this.refreshToken = (String) oauth2Config.getRefreshToken().getValue();
this.expireTime = Instant.now().plusSeconds(10);
log.info("Access Token and Refresh Token pair is now refreshed. Corresponding Secret store key updated.");
return;
}
throw new RuntimeException("Failed to renew access token message:" + ex.getMessage(), ex);
}
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 + " : {}", url);
log.error("{}: {}", INVALID_URL, url);
throw new BadRequestException(e.getMessage(), e);
}
}
Expand Down
Loading