Skip to content

Commit

Permalink
Add better default messages
Browse files Browse the repository at this point in the history 
Signed-off-by: Chase Engelbrecht <[email protected]>
  • Loading branch information
@engechas
engechas committed Aug 4, 2023
1 parent b389de2 commit c70ac58
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 27 deletions.
3 changes: 1 addition & 2 deletions .../armeria-common/src/main/java/org/opensearch/dataprepper/GrpcRequestExceptionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

public class GrpcRequestExceptionHandler implements GrpcStatusFunction {
static final String ARMERIA_REQUEST_TIMEOUT_MESSAGE = "Timeout waiting for request to be served. This is usually due to the buffer being full.";
static final String DEFAULT_MESSAGE = "";

public static final String REQUEST_TIMEOUTS = "requestTimeouts";
public static final String BAD_REQUESTS = "badRequests";
Expand Down Expand Up @@ -72,7 +71,7 @@ private Status createStatus(final Throwable e, final Status status) {
if (e instanceof RequestTimeoutException) {
message = ARMERIA_REQUEST_TIMEOUT_MESSAGE;
} else {
message = e.getMessage() == null ? DEFAULT_MESSAGE : e.getMessage();
message = e.getMessage() == null ? status.getCode().name() : e.getMessage();
}

return status.withDescription(message);
Expand Down
19 changes: 9 additions & 10 deletions .../armeria-common/src/main/java/org/opensearch/dataprepper/HttpRequestExceptionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,10 @@
import io.micrometer.core.instrument.Counter;

import java.io.IOException;
import java.util.Objects;
import java.util.concurrent.TimeoutException;

public class HttpRequestExceptionHandler implements ExceptionHandlerFunction {
static final String ARMERIA_REQUEST_TIMEOUT_MESSAGE = "Timeout waiting for request to be served. This is usually due to the buffer being full.";
static final String DEFAULT_MESSAGE = "";

public static final String REQUEST_TIMEOUTS = "requestTimeouts";
public static final String BAD_REQUESTS = "badRequests";
Expand All @@ -43,29 +41,30 @@ public HttpRequestExceptionHandler(final PluginMetrics pluginMetrics) {

@Override
public HttpResponse handleException(final ServiceRequestContext ctx, final HttpRequest req, final Throwable cause) {
final HttpStatus status = handleException(cause);
final String message;
if (cause instanceof RequestTimeoutException) {
message = ARMERIA_REQUEST_TIMEOUT_MESSAGE;
} else {
message = cause.getMessage() == null ? DEFAULT_MESSAGE : cause.getMessage();
message = cause.getMessage() == null ? status.reasonPhrase() : cause.getMessage();
}

return handleException(cause, message);
return HttpResponse.of(status, MediaType.ANY_TYPE, message);
}

private HttpResponse handleException(final Throwable e, final String message) {
Objects.requireNonNull(message);
private HttpStatus handleException(final Throwable e) {
if (e instanceof IOException) {
badRequestsCounter.increment();
return HttpResponse.of(HttpStatus.BAD_REQUEST, MediaType.ANY_TYPE, message);
return HttpStatus.BAD_REQUEST;
} else if (e instanceof TimeoutException || e instanceof RequestTimeoutException) {
requestTimeoutsCounter.increment();
return HttpResponse.of(HttpStatus.REQUEST_TIMEOUT, MediaType.ANY_TYPE, message);
return HttpStatus.REQUEST_TIMEOUT;
} else if (e instanceof SizeOverflowException) {
requestsTooLargeCounter.increment();
return HttpResponse.of(HttpStatus.REQUEST_ENTITY_TOO_LARGE, MediaType.ANY_TYPE, message);
return HttpStatus.REQUEST_ENTITY_TOO_LARGE;
}

internalServerErrorCounter.increment();
return HttpResponse.of(HttpStatus.INTERNAL_SERVER_ERROR, MediaType.ANY_TYPE, message);
return HttpStatus.INTERNAL_SERVER_ERROR;
}
}
11 changes: 5 additions & 6 deletions ...eria-common/src/test/java/org/opensearch/dataprepper/GrpcRequestExceptionHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.opensearch.dataprepper.GrpcRequestExceptionHandler.ARMERIA_REQUEST_TIMEOUT_MESSAGE;
import static org.opensearch.dataprepper.GrpcRequestExceptionHandler.DEFAULT_MESSAGE;

@ExtendWith(MockitoExtension.class)
public class GrpcRequestExceptionHandlerTest {
Expand Down Expand Up @@ -76,7 +75,7 @@ public void testHandleBadRequestException() {

final Status noMessageStatus = grpcRequestExceptionHandler.apply(requestContext, badRequestExceptionNoMessage, metadata);
assertThat(noMessageStatus.getCode(), equalTo(Status.Code.INVALID_ARGUMENT));
assertThat(noMessageStatus.getDescription(), equalTo(DEFAULT_MESSAGE));
assertThat(noMessageStatus.getDescription(), equalTo(Status.Code.INVALID_ARGUMENT.name()));

final Status messageStatus = grpcRequestExceptionHandler.apply(requestContext, badRequestExceptionWithMessage, metadata);
assertThat(messageStatus.getCode(), equalTo(Status.Code.INVALID_ARGUMENT));
Expand All @@ -93,7 +92,7 @@ public void testHandleTimeoutException() {

final Status noMessageStatus = grpcRequestExceptionHandler.apply(requestContext, timeoutExceptionNoMessage, metadata);
assertThat(noMessageStatus.getCode(), equalTo(Status.Code.RESOURCE_EXHAUSTED));
assertThat(noMessageStatus.getDescription(), equalTo(DEFAULT_MESSAGE));
assertThat(noMessageStatus.getDescription(), equalTo(Status.Code.RESOURCE_EXHAUSTED.name()));

final Status messageStatus = grpcRequestExceptionHandler.apply(requestContext, timeoutExceptionWithMessage, metadata);
assertThat(messageStatus.getCode(), equalTo(Status.Code.RESOURCE_EXHAUSTED));
Expand Down Expand Up @@ -121,7 +120,7 @@ public void testHandleSizeOverflowException() {

final Status noMessageStatus = grpcRequestExceptionHandler.apply(requestContext, sizeOverflowExceptionNoMessage, metadata);
assertThat(noMessageStatus.getCode(), equalTo(Status.Code.RESOURCE_EXHAUSTED));
assertThat(noMessageStatus.getDescription(), equalTo(DEFAULT_MESSAGE));
assertThat(noMessageStatus.getDescription(), equalTo(Status.Code.RESOURCE_EXHAUSTED.name()));

final Status messageStatus = grpcRequestExceptionHandler.apply(requestContext, sizeOverflowExceptionWithMessage, metadata);
assertThat(messageStatus.getCode(), equalTo(Status.Code.RESOURCE_EXHAUSTED));
Expand All @@ -138,7 +137,7 @@ public void testHandleRequestCancelledException() {

final Status noMessageStatus = grpcRequestExceptionHandler.apply(requestContext, requestCancelledExceptionNoMessage, metadata);
assertThat(noMessageStatus.getCode(), equalTo(Status.Code.CANCELLED));
assertThat(noMessageStatus.getDescription(), equalTo(DEFAULT_MESSAGE));
assertThat(noMessageStatus.getDescription(), equalTo(Status.Code.CANCELLED.name()));

final Status messageStatus = grpcRequestExceptionHandler.apply(requestContext, requestCancelledExceptionWithMessage, metadata);
assertThat(messageStatus.getCode(), equalTo(Status.Code.CANCELLED));
Expand All @@ -155,7 +154,7 @@ public void testHandleInternalServerException() {

final Status noMessageStatus = grpcRequestExceptionHandler.apply(requestContext, runtimeExceptionNoMessage, metadata);
assertThat(noMessageStatus.getCode(), equalTo(Status.Code.INTERNAL));
assertThat(noMessageStatus.getDescription(), equalTo(DEFAULT_MESSAGE));
assertThat(noMessageStatus.getDescription(), equalTo(Status.Code.INTERNAL.name()));

final Status messageStatus = grpcRequestExceptionHandler.apply(requestContext, runtimeExceptionWithMessage, metadata);
assertThat(messageStatus.getCode(), equalTo(Status.Code.INTERNAL));
Expand Down
17 changes: 8 additions & 9 deletions ...eria-common/src/test/java/org/opensearch/dataprepper/HttpRequestExceptionHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.opensearch.dataprepper.HttpRequestExceptionHandler.ARMERIA_REQUEST_TIMEOUT_MESSAGE;
import static org.opensearch.dataprepper.HttpRequestExceptionHandler.DEFAULT_MESSAGE;

@ExtendWith(MockitoExtension.class)
class HttpRequestExceptionHandlerTest {
Expand Down Expand Up @@ -79,7 +78,7 @@ public void testHandleIOException() throws ExecutionException, InterruptedExcept
// Then
AggregatedHttpResponse aggregatedHttpResponse = httpResponse.aggregate().get();
assertEquals(HttpStatus.BAD_REQUEST, aggregatedHttpResponse.status());
assertEquals(DEFAULT_MESSAGE, aggregatedHttpResponse.contentUtf8());
assertEquals(HttpStatus.BAD_REQUEST.reasonPhrase(), aggregatedHttpResponse.contentUtf8());

// When
httpResponse = httpRequestExceptionHandler.handleException(serviceRequestContext, httpRequest, testExceptionWithMessage);
Expand All @@ -106,7 +105,7 @@ public void testHandleTimeoutException() throws ExecutionException, InterruptedE
// Then
AggregatedHttpResponse aggregatedHttpResponse = httpResponse.aggregate().get();
assertEquals(HttpStatus.REQUEST_TIMEOUT, aggregatedHttpResponse.status());
assertEquals(DEFAULT_MESSAGE, aggregatedHttpResponse.contentUtf8());
assertEquals(HttpStatus.REQUEST_TIMEOUT.reasonPhrase(), aggregatedHttpResponse.contentUtf8());

// When
httpResponse = httpRequestExceptionHandler.handleException(serviceRequestContext, httpRequest, testExceptionWithMessage);
Expand Down Expand Up @@ -140,17 +139,17 @@ public void testHandleArmeriaTimeoutException() throws ExecutionException, Inter
@Test
public void testHandleSizeOverflowException() throws ExecutionException, InterruptedException {
// Prepare
final SizeOverflowException testExceptionEmptyMessage = new SizeOverflowException("");
final SizeOverflowException testExceptionNoMessage = new SizeOverflowException(null);
final String testMessage = "test exception message";
final SizeOverflowException testExceptionWithMessage = new SizeOverflowException(testMessage);

// When
HttpResponse httpResponse = httpRequestExceptionHandler.handleException(serviceRequestContext, httpRequest, testExceptionEmptyMessage);
HttpResponse httpResponse = httpRequestExceptionHandler.handleException(serviceRequestContext, httpRequest, testExceptionNoMessage);

// Then
AggregatedHttpResponse aggregatedHttpResponse = httpResponse.aggregate().get();
assertEquals(HttpStatus.REQUEST_ENTITY_TOO_LARGE, aggregatedHttpResponse.status());
assertEquals(DEFAULT_MESSAGE, aggregatedHttpResponse.contentUtf8());
assertEquals(HttpStatus.REQUEST_ENTITY_TOO_LARGE.reasonPhrase(), aggregatedHttpResponse.contentUtf8());

// When
httpResponse = httpRequestExceptionHandler.handleException(serviceRequestContext, httpRequest, testExceptionWithMessage);
Expand All @@ -167,17 +166,17 @@ public void testHandleSizeOverflowException() throws ExecutionException, Interru
@Test
public void testHandleUnknownException() throws ExecutionException, InterruptedException {
// Prepare
final UnknownException testExceptionEmptyMessage = new UnknownException("");
final UnknownException testExceptionNoMessage = new UnknownException(null);
final String testMessage = "test exception message";
final UnknownException testExceptionWithMessage = new UnknownException(testMessage);

// When
HttpResponse httpResponse = httpRequestExceptionHandler.handleException(serviceRequestContext, httpRequest, testExceptionEmptyMessage);
HttpResponse httpResponse = httpRequestExceptionHandler.handleException(serviceRequestContext, httpRequest, testExceptionNoMessage);

// Then
AggregatedHttpResponse aggregatedHttpResponse = httpResponse.aggregate().get();
assertEquals(HttpStatus.INTERNAL_SERVER_ERROR, aggregatedHttpResponse.status());
assertEquals(DEFAULT_MESSAGE, aggregatedHttpResponse.contentUtf8());
assertEquals(HttpStatus.INTERNAL_SERVER_ERROR.reasonPhrase(), aggregatedHttpResponse.contentUtf8());

// When
httpResponse = httpRequestExceptionHandler.handleException(serviceRequestContext, httpRequest, testExceptionWithMessage);
Expand Down

0 comments on commit c70ac58

Please sign in to comment.