[improvement] Include TraceID in the message for HTTPError #21
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Python errors are, more than any others, printed out to interactive consoles rather than logged with context. Including the traceid in the printout is going to help a bunch in debugging issues in interactive workflows.
ahggns
reviewed
Oct 16, 2018
| detail.get('errorName', 'UnknownError'), | ||
| detail.get('message', 'No Message'), | ||
| e.response.headers.get('X-B3-TraceId', 'No TraceId'), |
There was a problem hiding this comment.
e.response.headers is guaranteed to exist?
There was a problem hiding this comment.
ugh no I guess response can be None sometimes
JacekLach
commented
Oct 16, 2018
| @@ -81,12 +81,14 @@ def _request(self, *args, **kwargs): | |||
| detail = e.response.json() | |||
| except ValueError: | |||
| detail = {'message': e.response.content} | |||
| detail['traceId'] = e.response.headers.get('X-B3-TraceId'), | |||
| else: | |||
| detail = {} | |||
There was a problem hiding this comment.
I kinda feel like if response is None, we should just reraise the old error (e)
There was a problem hiding this comment.
I agree we should just pass along the original HttpError in that case.
I think it might be nice to wrap the HTTPError in a way that python (3.x) can understand a little better (i.e. use from future.utils import raise_from), and perhaps have a new ConjureHttpError type that has a reference to its cause as well as attributes for each of the details that can be present in an httpremoting failure response.
ahggns
pushed a commit
that referenced
this pull request
Oct 17, 2018
…or details (#22) <!-- PR title should start with '[fix]', '[improvement]' or '[break]' if this PR would cause a patch, minor or major SemVer bump. Omit the prefix if this PR doesn't warrant a standalone release. --> ## Before this PR <!-- Describe the problem you encountered with the current state of the world (or link to an issue) and why it's important to fix now. --> `HTTPError` response content was parsed hoping to find `SerializableError` contents, though didn't extract all contents and relied on deprecated fields such as message. The original `HTTPError` and a message were then wrapped in a new `HTTPError` and reraised. ## After this PR <!-- Describe at a high-level why this approach is better. --> 1. The original `HTTPError` is wrapped in a way (`raise_from`) that is understandable to Python 3's improved traceback functionality. 2. The new `ConjureHTTPError` can be separately detected as such while maintaining a reference to its wrapped `cause`. 3. The new `ConjureHTTPError` contains attributes to access fields from `SerializableError`, as well as the `X-B3-TraceId` from response headers when possible. <!-- Reference any existing GitHub issues, e.g. 'fixes #000' or 'relevant to #000' --> See also #21.
|
Closing in favor of #22 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Python errors are, more than any others, printed out to interactive consoles rather than logged with context.
Including the traceid in the printout is going to help a bunch in debugging issues in interactive workflows.