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.

Sign up for GitHub

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

[Client] Improve HTTPError Log #672

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+2 −1
Open

[Client] Improve HTTPError Log #672

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion perceval/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,11 @@ def _fetch_from_remote(self, url, payload, headers, method, stream, auth):

try:
response.raise_for_status()
Copy link
Member

Choose a reason for hiding this comment

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

raise_for_status can only raise HTTError exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I went through the source and checked it only raises HTTPError

Copy link
Member

Choose a reason for hiding this comment

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

Ok. In any case, response.text attribute returns a dictionary in the examples you wrote, not a text. What's the reason of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the response.text of an API is always a dict, whether it is an error or not, usually, we handle it in specific backends where the dict key can be mapped. But since different backends have different response formats we cannot do that in the client. Also, I don't think we really need to parse the dict as this only serves a log message, which can be used as an additional aid while debugging.

Copy link
Member

Choose a reason for hiding this comment

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

Then, shouldn't it be better if the backends are the ones that manage this error and print it if necessary? From your examples I think they make sense to print them but probably not that way. If there are other errors the problem is we don't have the context of which url was failing, for example.

Copy link
Contributor Author

@animeshk08 animeshk08 May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, @sduenas I had given this a thought before following the current approach. If we try to extract the error message in the backend, a problem which may arise is that for some backends the error JSON which is returned is not always uniform(for example, for some errors gitter may return {"error":<error>} format and in other cases, it may return some other format). Currently, I don't have an exhaustive list of which backends show this behaviour, but I have seen this happen. Also, usually, there is not much documentation about this, and, if such behaviour happens, the purpose of this patch is defeated.

If there are other errors the problem is we don't have the context of which url was failing, for example.

This info is always available. For eg, a full log or Gitter would look like:

[2020-05-29 17:05:14,356] - Sir Perceval is on his quest.
[2020-05-29 17:05:17,877] - HTTPError: {"error":"Unauthorized"}
Traceback (most recent call last):
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/backend.py", line 618, in run
    for item in big.items:
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/backend.py", line 794, in __fetch
    raise e
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/backend.py", line 788, in __fetch
    for item in items:
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/backend.py", line 226, in fetch
    for item in self.fetch_items(category, **kwargs):
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/backends/core/gitter.py", line 146, in fetch_items
    self.room_id = self.client.get_room_id(room)
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/backends/core/gitter.py", line 327, in get_room_id
    rooms = self.fetch(path)
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/backends/core/gitter.py", line 301, in fetch
    response = super().fetch(url, payload, headers=headers)
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/client.py", line 143, in fetch
    response = self._fetch_from_remote(url, payload, headers, method, stream, auth)
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/client.py", line 187, in _fetch_from_remote
    raise e
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/client.py", line 181, in _fetch_from_remote
    response.raise_for_status()
  File "/home/animesh/.local/lib/python3.6/site-packages/requests/models.py", line 940, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 401 Client Error: Unauthorized for url: https://api.gitter.im/v1/rooms

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/animesh/percevallastest/grimoirelab-perceval/bin/perceval", line 169, in <module>
    main()
  File "/home/animesh/percevallastest/grimoirelab-perceval/bin/perceval", line 115, in main
    cmd.run()
  File "/home/animesh/percevallastest/grimoirelab-perceval/perceval/backend.py", line 628, in run
    raise RuntimeError(str(e))
RuntimeError: 401 Client Error: Unauthorized for url: https://api.gitter.im/v1/rooms

As you can see the URL info is always available, as is any other info which was previously returned by perceval.

Copy link
Member

Choose a reason for hiding this comment

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

Then, shouldn't it be better to set this message as debug?

I don't want to be too picky with this :). Just trying to figure out the best way to show these errors and problems with perceval to follow the same approach everywhere. Sometimes I feel we just write errors but that they don't have any context and it's hard to figure out what happened and where.

Sorry for my late response.

except Exception as e:
except requests.exceptions.HTTPError as e:
if self.archive:
url, headers, payload = self.sanitize_for_archive(url, headers, payload)
self.archive.store(url, payload, headers, e)
logger.error("HTTPError: " + e.response.text)
raise e

if self.archive:
Expand Down