chore: better error messages on http client errors #1017
Conversation
|
You can access the deployment of this PR at https://renku-ci-ds-1017.dev.renku.ch |
eikek
force-pushed
the
eikek/fix/connect-errors
branch
from
a9ad077 to
a37c1fd
Compare
| case httpx.RequestError(): | ||
| formatted_exception = errors.BaseError(message=f"Error on remote connection: {exception}") |
There was a problem hiding this comment.
We don't need to create a new class from the httpx client.
| case httpx.RequestError(): | |
| formatted_exception = errors.BaseError(message=f"Error on remote connection: {exception}") | |
| case httpx.HTTPError(): | |
| formatted_exception = errors.BaseError(message=f"Error while performing {exception.request.method} {exception.request.url}: {exception}") |
This way we can log the request method and URL without having to modify the httpx classes.
Reference: https://www.python-httpx.org/exceptions/
There was a problem hiding this comment.
Great, thank you. Somehow missed that page :-|
There was a problem hiding this comment.
Ah now I recalled what I head in mind last week: For making it "least surprise", I wanted this change only to affect the image code. Not using a separate class also means, we get the uri into the error message for every use of the client. Just mentioning it, because potentially there are cases where this is not desired? But I guess it's fine.
There was a problem hiding this comment.
Then we have to manually handle the error.
- By default we say there was an error performing an HTTP request. In the logs and in Sentry we can log the method and URL (and more if needed).
- For endpoints were we know we are not leaking information we can try/catch the exception to give the user the method and URL.
There was a problem hiding this comment.
Yes exactly. So you say we should handle it manually on every case? With this (not very great, I admit) wrapper class, at least it would allow to just replace AsyncClient occurrences, could be more convenient.
There was a problem hiding this comment.
But the client instances could be passed around, which can be hard to track precisely. Explicit handling at the caller site can look redundant but cannot escape to other parts of the code.
Unfortunately, the error message in the cause exception is not much better. So there is now a `HttpClient` extending `AsyncClient` which injects the request url into the error message and re-raises the original error.
eikek
force-pushed
the
eikek/fix/connect-errors
branch
from
afb8a88 to
9607178
Compare
|
|
||
| case httpx.RequestError(): | ||
| req_uri = exception.request.url | ||
| formatted_exception = errors.BaseError(message=f"Error on remote connection to {req_uri}: {exception}") |
There was a problem hiding this comment.
Note: should we also include the method here?
Try for better error message for failing http calls
Unfortunately, the error message in the cause exception is not much
better. So there is now a
HttpClientextendingAsyncClientwhichinjects the request url into the error message and re-raises the
original error.
/deploy