Skip to content

Investigate how to improve error responses back to client #296

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

Closed
kevin-bates opened this issue Apr 4, 2018 · 4 comments
Closed

Investigate how to improve error responses back to client #296

kevin-bates opened this issue Apr 4, 2018 · 4 comments
Assignees
Milestone

Comments

@kevin-bates
Copy link
Member

When the EG server encounters an exception, primarily during a kernel's startup, it will often times produce a contextual message along with the traceback in the log file. However, all the notebook client gets (behind the Kernel Error button) is a local traceback and the canned reason that is associated with the HTTP error code (e.g., 500: Server Error).

We should look into the error response plumbing to see if there are any ways we can propagate, at a minimum, the EG's contextual message and, if possible, its traceback as well.

@lresende
Copy link
Member

lresende commented Apr 4, 2018

This is probably a sign of not properly catching the error which is generating the HTTP 500, if not, one way I handled this in a different stack was to send additional headers with the error details, but this would involve the client knowing about these headers.

@kevin-bates
Copy link
Member Author

@lresende hmm - the contextual messages I'm referring to are produced from error handlers, but perhaps they themselves should be re-throwing a more HTTP-like error and reason? We looked into this a while back and I think its more involved than that. Sounds like you're volunteering here - thanks! 😃
We don't want to make clients be updated, although if we had to modify NB2KG to make this happen that would address a majority of clients.

Here's some code that, when it occurs, comes back with 403 (good), but a generic message (bad):

    def _raise_authorization_error(self, kernel_username, differentiator_clause):
        kernel_name = self.kernel_manager.kernel_spec.display_name
        kernel_clause = " '{}'.".format(kernel_name) if kernel_name is not None else "s."
        error_message = "User '{}' is {} to start kernel{} " \
                        "Ensure KERNEL_USERNAME is set to an appropriate value and retry the request.". \
            format(kernel_username, differentiator_clause, kernel_clause)
        self.log.error(error_message)
        raise tornado.web.HTTPError(403, error_message)

It would be great to get the message to flow as well.

@kevin-bates
Copy link
Member Author

@lresende - I can get the custom error message to flow back to the caller if both of the following are done:

  • a kwargs parameter containing a reason keyword is initialized and passed to the HTTPError constructor...
        error_args = {'reason': error_message}
        raise tornado.web.HTTPError(403, **error_args)

and

  • a custom status code is used (e.g., 4033). If the status code is a built-in code (e.g., 403), then the built-in reason is used (e.g., Forbidden).

Unless I'm missing something, we'd need to define a range of error codes for the various scenarios in which we'd want custom messages.

@kevin-bates
Copy link
Member Author

kevin-bates commented Apr 23, 2018

Correction: Revisiting this situation (after discussing with @lresende and @sanjay-saxena) there are two changes required, but things are much better.

  1. You still must pass the keyword 'reason' containing the customer message, but the dictionary overhead is not necessary (still learning python 😀 ):
        raise tornado.web.HTTPError(403, reason=error_message)
  1. But, any status code can be provided so long as the status code is set with a reason named argument:
        self.set_status(status_code, reason=reply['reason'])

Unfortunately, the latter requires the additional named argument to be added to Kernel Gateway's JSONErrorsMixin. However, once added, we can then use registered or unregistered status codes and the custom message will flow to the client. I will be opening an issue in Kernel Gateway that references this issue.

In the meantime, we can move forward with refactoring based on the first item. Please note though, that until Kernel Gateway is updated with the second item, we must retain the built-in status codes, else tornado's set_status() call will raise a ValueError. That enforcement of built-in status codes is lifted if the reason named argument is provided. Also, prior to the JKG update, we'll continue to see the default built-in message on the client.

kevin-bates added a commit to kevin-bates/enterprise_gateway that referenced this issue Apr 23, 2018
This PR also requires [Kernel Gateway PR 284](jupyter-server/kernel_gateway#284)

In absence of that PR, the default message corresponding to the status code will
be returned to the caller.  So this PR is essentially preparation to have custom
error messages returned.

Fixes jupyter-server#296
kevin-bates added a commit to kevin-bates/enterprise_gateway that referenced this issue Apr 23, 2018
This PR also requires [Kernel Gateway PR 284](jupyter-server/kernel_gateway#284)

In absence of that PR, the default message corresponding to the status code will
be returned to the caller.  So this PR is essentially preparation to have custom
error messages returned.

Fixes jupyter-server#296
@kevin-bates kevin-bates self-assigned this Apr 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants