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

[ENHANCEMENT] Improves Error Messaging in Responses #283

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions common/exceptions/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Shared Exceptions Code for the Rest API

This package contains shared code that is used to handle exceptions
from REST requests to mojo frames and back.


## Building

This is a standard Java gradle project and is build as usual by `./gradlew build`.
9 changes: 9 additions & 0 deletions common/exceptions/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
apply from: project(":").file('gradle/java.gradle')

dependencies {
implementation group: 'org.slf4j', name: 'slf4j-api'
}

test {
useJUnitPlatform()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package ai.h2o.mojos.deploy.common.exceptions;

public class ScoringException extends RuntimeException {

public ScoringException(String message) {
super(message);
}

public ScoringException(String message, Throwable cause) {
super(message, cause);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package ai.h2o.mojos.deploy.common.exceptions;

public class ScoringShapleyException extends RuntimeException {

public ScoringShapleyException(String message, Throwable cause) {
super(message, cause);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ public ScoreResponse score(ScoreRequest request) {
} catch (Exception e) {
log.info("Failed shapley contribution due to: {}", e.getMessage());
log.debug(" - failure cause: ", e);
Copy link
Member

Choose a reason for hiding this comment

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

side note: generally, suggestion is to either log or throw, but not both. eventually, whatever part of the code catches all bubbled up exceptions would do the logging. And with all nested exceptions simply tacking on their own messages (e.g., throw new Illegal....Exception("Failed to ...", e)), it all gets wrapped up into a single object that can be logged by one catcher.

Else, error/warning/etc. messages are scattered across logs (often separated by regular, healthy, logs) instead of in a single, trackable, block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure that can be moved, I tried to minimize my impact to the logs in general as much as possible. The goal was to actually give the response something useful that can be used as opposed to always sending 400 and nothing else.

return response;
}
return response;
}
Expand Down
1 change: 1 addition & 0 deletions local-rest-scorer/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ plugins {
apply from: project(":").file('gradle/java.gradle')

dependencies {
implementation project(':common:exceptions')
implementation project(':common:rest-spring-api')
implementation project(':common:transform')
implementation group: 'ai.h2o', name: 'h2o-genmodel'
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package ai.h2o.mojos.deploy.local.rest.controller;

import ai.h2o.mojos.deploy.common.exceptions.ScoringException;
import ai.h2o.mojos.deploy.common.exceptions.ScoringShapleyException;
import ai.h2o.mojos.deploy.common.rest.api.ModelApi;
import ai.h2o.mojos.deploy.common.rest.model.CapabilityType;
import ai.h2o.mojos.deploy.common.rest.model.ContributionRequest;
Expand All @@ -18,7 +20,6 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Controller;

Expand Down Expand Up @@ -72,29 +73,28 @@ public ResponseEntity<ScoreResponse> getScore(ScoreRequest request) {
ScoreResponse scoreResponse = scorer.score(request);
return ResponseEntity.ok(scoreResponse);
} catch (Exception e) {
log.info("Failed scoring request: {}, due to: {}", request, e.getMessage());
log.debug(" - failure cause: ", e);
return ResponseEntity.badRequest().build();
String message = String.format(
"Failed scoring request: %s, due to: %s", request, e.getMessage());
throw new ScoringException(message, e);
}
}

@Override
public ResponseEntity<ScoreResponse> getScoreByFile(String file) {
if (Strings.isNullOrEmpty(file)) {
log.info("Request is missing a valid CSV file path");
return ResponseEntity.badRequest().build();
throw new ScoringException("Request is missing a valid CSV file path");
}
try {
log.info("Got scoring request for CSV");
return ResponseEntity.ok(scorer.scoreCsv(file));
} catch (IOException e) {
log.info("Failed loading CSV file: {}, due to: {}", file, e.getMessage());
log.debug(" - failure cause: ", e);
return ResponseEntity.badRequest().build();
String message = String.format(
"Failed loading CSV file: %s, due to: %s", file, e.getMessage());
throw new ScoringException(message, e);
nkpng2k marked this conversation as resolved.
Show resolved Hide resolved
} catch (Exception e) {
log.info("Failed scoring CSV file: {}, due to: {}", file, e.getMessage());
log.debug(" - failure cause: ", e);
return ResponseEntity.badRequest().build();
String message = String.format(
"Failed scoring CSV file: %s, due to: %s", file, e.getMessage());
throw new ScoringException(message, e);
}
}

Expand All @@ -106,13 +106,15 @@ public ResponseEntity<ContributionResponse> getContribution(
ContributionResponse contributionResponse
= scorer.computeContribution(request);
return ResponseEntity.ok(contributionResponse);
} catch (UnsupportedOperationException e) {
log.info("Unsupported operation due to: {}", e.getMessage());
return ResponseEntity.status(HttpStatus.NOT_IMPLEMENTED).build();
} catch (IllegalArgumentException e) {
String message = String.format("Unsupported operation due to: %s", e.getMessage());
throw new ScoringShapleyException(message, e);
Comment on lines +109 to +111
Copy link
Member

Choose a reason for hiding this comment

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

When catching IllegalArgumentException (thrown by computeContribution() because of user error), should the message be changed from Unsupported...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't throw Unsupported... anywhere anymore, so it didn't make sense to keep it.

} catch (Exception e) {
log.info("Failed shapley contribution request due to: {}", e.getMessage());
log.debug(" - failure cause: ", e);
return ResponseEntity.badRequest().build();
String message = String.format(
"Failed shapley contribution request: %s, due to: %s",
request,
e.getMessage());
throw new ScoringShapleyException(message, e);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package ai.h2o.mojos.deploy.local.rest.exception;

import ai.h2o.mojos.deploy.common.exceptions.ScoringException;
import ai.h2o.mojos.deploy.common.exceptions.ScoringShapleyException;
import ai.h2o.mojos.deploy.local.rest.controller.ModelsApiController;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.ControllerAdvice;
import org.springframework.web.bind.annotation.ExceptionHandler;
import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;

@ControllerAdvice
public class ModelsApiExceptionController extends ResponseEntityExceptionHandler {
nkpng2k marked this conversation as resolved.
Show resolved Hide resolved

private static final Logger log = LoggerFactory.getLogger(ModelsApiController.class);

@ExceptionHandler(value = ScoringException.class)
protected ResponseEntity<Object> handleScoringException(ScoringException scoringException) {
log.info(scoringException.getMessage());
log.debug(" - failure cause: ", scoringException);
return new ResponseEntity<>(scoringException.getMessage(), HttpStatus.BAD_REQUEST);
}
Comment on lines +23 to +24
Copy link
Member

Choose a reason for hiding this comment

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

Not an issue with the PR, pointing out something I just now noticed:

A 4xx (client error) is being returned every time even when the process fails because of the backend (e.g., CSV file issue, mojo lib, etc.) (which should be 5xx). This was the case even before this PR, so no regression being introduced, or anything, of course.

Pointing out for if 4xx are fixed to 5xx (where correct) at some point, this class may need slight changes as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true this is a good point. I stuck with the error codes we were already sending.


@ExceptionHandler(value = ScoringShapleyException.class)
protected ResponseEntity<Object> handleScoringShapleyException(
ScoringShapleyException scoringShapleyException) {
log.info(scoringShapleyException.getMessage());
log.debug(" - failure cause: ", scoringShapleyException);
return new ResponseEntity<>(scoringShapleyException.getMessage(), HttpStatus.BAD_REQUEST);
}
}
1 change: 1 addition & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
rootProject.name = 'dai-deployment-templates'
include 'aws-lambda-scorer:lambda-template'
include 'aws-lambda-scorer:terraform-recipe'
include 'common:exceptions'
include 'common:jdbc'
include 'common:rest-java-model'
include 'common:rest-jdbc-spring-api'
Expand Down