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

Add logging at main API endpoints (bulk ingest, msearch) when 500 is returned #924

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

autata
Copy link
Contributor

@autata autata commented May 10, 2024

Summary

Add 2 more places to log errors where astra returns a 500. Without this, nothing is logged in these cases when astra encounters an internal server error. I picked error level because other places where a 500 is returned, those are also logged as errors. These may not be as severe and more implementation choices. I'm curious what level the reviewers would prefer here.

In the case of request must contain only 1 unique index, debug may be a more suitable log level as there could be a lot of these and error logs could be spammy.

For multisearch query, an example where this is triggered would be shown in the PR: #780. (Null pointer exception due to json deserialization logic) Potentially, this could be better suited as a warning log?

Requirements

@@ -81,6 +81,7 @@ public HttpResponse addDocument(String bulkRequest) {
BulkIngestResponse response =
new BulkIngestResponse(0, 0, "request must contain only 1 unique index");
future.complete(HttpResponse.ofJson(INTERNAL_SERVER_ERROR, response));
LOG.error("request must contain only 1 unique index");
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is a good add.

HttpStatus.OK, MediaType.JSON_UTF_8, JsonUtil.writeAsString(responseMetadata));
}
} catch (Exception e) {
LOG.error("Error fulfilling request for multisearch query", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should already be logging these, as we've enabled Armeria request logging here:

private void initializeLogging() {
serverBuilder.decorator(
LoggingService.builder()
// Not logging any successful response, say prom scraping /metrics every 30 seconds at
// INFO
.successfulResponseLogLevel(LogLevel.DEBUG)
.failureResponseLogLevel(LogLevel.ERROR)
// Remove the content to prevent blowing up the logs
.responseContentSanitizer((ctx, content) -> "truncated")
// Remove all headers to be sure we aren't leaking any auth/cookie info
.requestHeadersSanitizer((ctx, headers) -> DefaultHttpHeaders.EMPTY_HEADERS)
.newDecorator());
}

This should automatically log all successful and failed requests at the log levels indicated. This does make use of responseContentSanitizer which has since been deprecated in Armeria.

I'm not sure if this currently logs out the stack trace, but instead of adding try/catches to the individual endpoints I think that looking into the LoggingService decorator would be a better approach.

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

Successfully merging this pull request may close these issues.

None yet

2 participants