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

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2c99a8
Add logging at main API endpoints (bulk ingest, msearch)
autata May 10, 2024
3f01a85
fix formatting
autata May 10, 2024
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
1 change: 1 addition & 0 deletions astra/src/main/java/com/slack/astra/bulkIngestApi/BulkIngestApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

return HttpResponse.of(future);
}

Expand Down
32 changes: 19 additions & 13 deletions astra/src/main/java/com/slack/astra/elasticsearchApi/ElasticsearchApiService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,24 +92,30 @@ public HttpResponse clusterMetadata() {
@Post
@Blocking
@Path("/_msearch")
public HttpResponse multiSearch(String postBody) throws Exception {
LOG.debug("Search request: {}", postBody);
public HttpResponse multiSearch(String postBody) {

CurrentTraceContext currentTraceContext = Tracing.current().currentTraceContext();
try (var scope = new StructuredTaskScope<EsSearchResponse>()) {
List<StructuredTaskScope.Subtask<EsSearchResponse>> requestSubtasks =
try {
LOG.debug("Search request: {}", postBody);

CurrentTraceContext currentTraceContext = Tracing.current().currentTraceContext();
try (var scope = new StructuredTaskScope<EsSearchResponse>()) {
List<StructuredTaskScope.Subtask<EsSearchResponse>> requestSubtasks =
openSearchRequest.parseHttpPostBody(postBody).stream()
.map((request) -> scope.fork(currentTraceContext.wrap(() -> doSearch(request))))
.toList();
.map((request) -> scope.fork(currentTraceContext.wrap(() -> doSearch(request))))
.toList();

scope.join();
SearchResponseMetadata responseMetadata =
scope.join();
SearchResponseMetadata responseMetadata =
new SearchResponseMetadata(
0,
requestSubtasks.stream().map(StructuredTaskScope.Subtask::get).toList(),
Map.of("traceId", getTraceId()));
return HttpResponse.of(
0,
requestSubtasks.stream().map(StructuredTaskScope.Subtask::get).toList(),
Map.of("traceId", getTraceId()));
return HttpResponse.of(
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:

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.

return HttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR);
}
}

Expand Down
Loading