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

Jump to bottom

Improve Jira logging #5351

Merged

dlvenable merged 11 commits into opensearch-project:main from Galactus22625:jira-overlogging

Jan 30, 2025

Member

Galactus22625 commented Jan 23, 2025

Description

prevent over logging stack traces in jira source

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

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.


          no stack trace for known errors

1618d8f

Signed-off-by: Maxwell Brown <[email protected]>

Galactus22625 requested review from sb2k16, chenqi0805, engechas, san81, srikanthjg, graytaylor0, dinujoh, kkondaka, KarstenSchnitter, dlvenable and oeyh as code owners

January 23, 2025 20:17


          add else block for error handling in jirarestclient

f7fa348

Signed-off-by: Maxwell Brown <[email protected]>

san81 reviewed

View reviewed changes

...search/dataprepper/plugins/source/source_crawler/coordination/partition/LeaderPartition.java Outdated

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

Collaborator

san81 Jan 23, 2025

Can we print the String partition progress state as well here?

Member Author

Galactus22625 Jan 23, 2025

ok


          print partition progress streate as well

813d53e

Signed-off-by: Maxwell Brown <[email protected]>

san81 previously approved these changes

View reviewed changes

Collaborator

san81 left a comment

Thank you for this clean up

dlvenable requested changes

View reviewed changes

...ce/src/main/java/org/opensearch/dataprepper/plugins/source/jira/utils/AddressValidation.java Outdated

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

Member

dlvenable Jan 23, 2025

Suggested change

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

Member Author

Galactus22625 Jan 24, 2025

ok

...source/src/main/java/org/opensearch/dataprepper/plugins/source/jira/rest/JiraRestClient.java Outdated

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

Member

dlvenable Jan 23, 2025

Should this be noisy?

Member Author

Galactus22625 Jan 24, 2025 •

edited

Loading

I think its not needed because it's only one line and retries are capped at 6

Member Author

Galactus22625 Jan 29, 2025

Actually, will make noisy

...source/src/main/java/org/opensearch/dataprepper/plugins/source/jira/rest/JiraRestClient.java Outdated

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

Member

dlvenable Jan 23, 2025

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

Member Author

Galactus22625 Jan 24, 2025

ok

...source/src/main/java/org/opensearch/dataprepper/plugins/source/jira/rest/JiraRestClient.java

		@@ -119,19 +119,21 @@ private <T> ResponseEntity<T> invokeRestApi(URI uri, Class<T> responseType) thro
		} catch (HttpClientErrorException ex) {

Member

dlvenable Jan 23, 2025

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.

Member Author

Galactus22625 Jan 24, 2025

added error logging for unknown exception, added error handling for some 5xx errors


          me when i address comments or something

7edf388

Signed-off-by: Maxwell Brown <[email protected]>

Galactus22625 dismissed san81’s stale review via

7edf388

January 24, 2025 01:21

Galactus22625 requested a review from dlvenable

January 24, 2025 01:21

san81 previously approved these changes

View reviewed changes

Galactus22625 added 2 commits

January 24, 2025 16:42


          remove unused metric

e9f389e

Signed-off-by: Maxwell Brown <[email protected]>


          ensure hosts is list of length 1

a8b00f1

Signed-off-by: Maxwell Brown <[email protected]>

Galactus22625 dismissed san81’s stale review via

a8b00f1

January 25, 2025 00:42

san81 previously approved these changes

View reviewed changes


          change error to info log

9a7c8f0

Signed-off-by: Maxwell Brown <[email protected]>

Galactus22625 dismissed san81’s stale review via

9a7c8f0

January 29, 2025 22:15

san81 previously approved these changes

View reviewed changes


          NOISY flag for logs

275309a

Signed-off-by: Maxwell Brown <[email protected]>

Galactus22625 dismissed san81’s stale review via

275309a

January 29, 2025 22:54


          sleep timer logging and secret refresh error and logging changes

ff1dce8

Signed-off-by: Maxwell Brown <[email protected]>

dlvenable reviewed

View reviewed changes

...source/src/main/java/org/opensearch/dataprepper/plugins/source/jira/rest/JiraRestClient.java Outdated

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

Member

dlvenable Jan 30, 2025

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.

Member Author

Galactus22625 Jan 30, 2025

changing to log.warn

dlvenable reviewed

View reviewed changes

...source/src/main/java/org/opensearch/dataprepper/plugins/source/jira/rest/JiraRestClient.java Outdated

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

Member

dlvenable Jan 30, 2025

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

Member Author

Galactus22625 Jan 30, 2025

consolidated


          address comments

70411df

Signed-off-by: Maxwell Brown <[email protected]>

dlvenable previously approved these changes

View reviewed changes

san81 previously approved these changes

View reviewed changes


          fix refresh because test failing

aa7947f

Signed-off-by: Maxwell Brown <[email protected]>

Galactus22625 dismissed stale reviews from san81 and dlvenable via

aa7947f

January 30, 2025 18:02

dlvenable approved these changes

View reviewed changes

dlvenable merged commit 414a7c9 into opensearch-project:main

43 of 47 checks passed

Galactus22625 deleted the jira-overlogging branch

January 30, 2025 19:47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dlvenable dlvenable approved these changes

san81 san81 left review comments

sb2k16 Awaiting requested review from sb2k16 sb2k16 is a code owner

chenqi0805 Awaiting requested review from chenqi0805 chenqi0805 is a code owner

engechas Awaiting requested review from engechas engechas is a code owner

srikanthjg Awaiting requested review from srikanthjg srikanthjg is a code owner

graytaylor0 Awaiting requested review from graytaylor0 graytaylor0 is a code owner

dinujoh Awaiting requested review from dinujoh dinujoh is a code owner

kkondaka Awaiting requested review from kkondaka kkondaka is a code owner

KarstenSchnitter Awaiting requested review from KarstenSchnitter KarstenSchnitter is a code owner

oeyh Awaiting requested review from oeyh oeyh is a code owner

Labels

None yet