-
Notifications
You must be signed in to change notification settings - Fork 30
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
Conversation
@@ -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"); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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:
astra/astra/src/main/java/com/slack/astra/server/ArmeriaService.java
Lines 72 to 84 in 2d7c87f
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.
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 3 days. |
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