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.

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

OpenSearchSink - Enhance logs to include index name and last exception information #4841

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

kkondaka
Copy link
Collaborator

Description

OpenSearchSink -
Enhance logs to include index name where applicable
and add last exception information when giving up after retrying

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • [ X] 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.

graytaylor0
graytaylor0 previously approved these changes Aug 16, 2024
@graytaylor0
Copy link
Member

Build and DCO checks are failing

kkondaka and others added 3 commits August 16, 2024 19:21
Signed-off-by: Krishna Kondaka <[email protected]>
graytaylor0
graytaylor0 previously approved these changes Aug 22, 2024
@@ -209,7 +209,7 @@ public void execute(final AccumulatingBulkRequest bulkRequest) throws Interrupte
request = operationResponse.getBulkRequest();
exceptionMessage = operationResponse.getExceptionMessage();
if (delayMillis < 0) {
RuntimeException e = new RuntimeException(String.format("Index : %s, Number of retries reached the limit of max retries (configured value %d. Last exception message: %s)", maxRetries, exceptionMessage));
Copy link
Member

Choose a reason for hiding this comment

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

Why get rid of the index name here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@graytaylor0 I did not get rid of it. It is not available. I have mistakenly put it there in the previous revision.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Index is available at each operation level. Not at bulk operation level.

@graytaylor0 graytaylor0 dismissed their stale review August 22, 2024 17:08

Build is failing due to failing tests in BulkRetryStrategy

Signed-off-by: Krishna Kondaka <[email protected]>
Copy link
Collaborator

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

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

Is it possible for the exception message to contain sensitive information? In that case, I think the marker should be set in the log statements.

@kkondaka
Copy link
Collaborator Author

@KarstenSchnitter most of the changes are about adding index name. There is one place where exception message is printed. But this is no different from what we already do.

@kkondaka kkondaka merged commit 891b228 into opensearch-project:main Sep 27, 2024
68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants