[Client] Improve HTTPError Log

[animeshk08]

This commit improves HTTPError log by
logging the text in the of the error
response.

Fixes: #671

Signed-off-by: Animesh Kumar [email protected]

[animeshk08]

Some improved logs:
RocketChat:

[2020-05-27 17:45:00,514] - Sir Perceval is on his quest.
[2020-05-27 17:45:01,982] - Fetching messages of channel: testapichannel from date: 1970-01-01 00:00:00+00:00
[2020-05-27 17:45:05,864] - HTTPError: {"status":"error","message":"You must be logged in to do this."}

Pagure:

[2020-05-27 17:46:58,705] - Sir Perceval is on his quest.
[2020-05-27 17:47:02,878] - HTTPError: {
  "error": "Issue tracker disabled",
  "error_code": "ETRACKERDISABLED"
}

Gitter:

[2020-05-27 17:52:39,561] - Sir Perceval is on his quest.
[2020-05-27 17:52:43,696] - HTTPError: {"error":"Unauthorized"}

[coveralls]

Coverage increased (+0.0003%) to 97.274% when pulling 6fc4440 on animeshk08:client-patch into 48eff53 on chaoss:master.

[sduenas]

raise_for_status can only raise HTTError exceptions?

[animeshk08]

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

[sduenas]

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

[animeshk08]

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.

[sduenas]

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.

[animeshk08]

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.

[sduenas]

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.