-
Notifications
You must be signed in to change notification settings - Fork 310
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
Added error propagation to gateway_request function #1233
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #1233 +/- ##
==========================================
- Coverage 80.47% 80.35% -0.13%
==========================================
Files 68 68
Lines 8252 8261 +9
Branches 1599 1600 +1
==========================================
- Hits 6641 6638 -3
- Misses 1189 1200 +11
- Partials 422 423 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
These changes look good - thank you @broden-wanner!
Can we also backport this to 1.x branch? |
@meeseeksdev please backport to 1.x |
…equest function
…equest function) (#1234)Co-authored-by: Broden Wanner <broden.wanner@outlook.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Steven Silvester <steven.silvester@ieee.org> * Backport PR #1233: Added error propagation to gateway_request function * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * lint --------- Co-authored-by: Broden Wanner <broden.wanner@outlook.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Steven Silvester <steven.silvester@ieee.org>
Hi @blink1073 - Is there a chance we could get a 1.x release that contains this change? |
Thank you @blink1073! |
Thank you @kevin-bates and @blink1073! Much appreciated! |
Summary
Feature for #1231. With this change, error messages from Enterprise Gateway will be propagated through Jupyter Server to the frontend. Looking through the Enterprise Gateway code, it seems that the JSON payload returned should only contain a
reason
field or amessage
field (or both possibly). See the code for thelog_and_raise
method of the processproxy here where thereason
field is populated, and also see the swagger spec that lists onlyreason
andmessage
as possible properties. However, in my own testing, I've noticed themessage
andreason
fields might not be in the body of the error (it's usually only one of them), so I've added logic to account for the fields missing or non-JSON payloads being returned. See relevant discussion on the Jupyter community forum here.Screenshots
The following screenshots show the before and after error messages displayed on Jupyter lab when kernel creation times out.
Before
Error message in text:
HTTP 500: Internal Server Error (Error attempting to connect to Gateway server url 'https://mygatewayurl'. Ensure gateway url is valid and the Gateway instance is running.)
After
Error message:
HTTP 500: Internal Server Error (Error from Gateway: [Internal Server Error] KernelID: '094f5160-163a-4025-94cf-5d8922808dd3' launch timeout due to: Waited too long (40.0s) to get connection file. Ensure gateway url is valid and the Gateway instance is running.)
Tests
No test cases were modified, and it doesn't seem that any have failed. I noticed that the
gateway_request
function is mocked in testing and doesn't seem to be directly used in the tests. Let me know if you'd like any tests to be added for this error propagation.