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.

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

show full HTTP response on failure (Feat/1605) #1895

Open
wants to merge 9 commits into
base: devel
Choose a base branch
from
Prev Previous commit
Next Next commit
Logs full response also on rest_api HTTP errors
willi-mueller committed Sep 28, 2024
commit 139c2862a5f0164d7f8a33a1ffc337db2c783688
9 changes: 8 additions & 1 deletion dlt/sources/helpers/requests/session.py
Original file line number Diff line number Diff line change
@@ -60,9 +60,16 @@ def request(self, *args, **kwargs): # type: ignore[no-untyped-def,no-redef]

def send(self, request: PreparedRequest, **kwargs: Any) -> Response:
kwargs.setdefault("timeout", self.timeout)
request.hooks["response"].append(self._log_full_response_if_error)
self._attach_log_error_hook(request)
return super().send(request, **kwargs)

def _attach_log_error_hook(self, request: PreparedRequest) -> None:
if "response" in request.hooks:
# ensure that the logging is called before any other handler, such as raise_for_status()
request.hooks["response"].insert(0, self._log_full_response_if_error)
else:
request.hooks["response"] = [self._log_full_response_if_error]

def _log_full_response_if_error(self, response: Response, *args: Any, **kwargs: Any) -> None:
if 400 <= response.status_code < 500:
http_error_msg = (
41 changes: 39 additions & 2 deletions tests/sources/rest_api/integration/test_offline.py
Original file line number Diff line number Diff line change
@@ -2,7 +2,9 @@
from unittest import mock

import pytest
from requests import Request, Response, Session
from requests import Request, Response, Session as BaseSession

from dlt.sources.helpers.requests import Session

import dlt
from dlt.common import pendulum
@@ -370,7 +372,7 @@ def test_posts_with_inremental_date_conversion(mock_api_server) -> None:


def test_multiple_response_actions_on_every_response(mock_api_server, mocker):
class CustomSession(Session):
class CustomSession(BaseSession):
pass

my_session = CustomSession()
@@ -432,3 +434,38 @@ def post_list():
for i in range(3):
_, kwargs = mock_paginate.call_args_list[i]
assert kwargs["path"] == f"posts/{i}/comments"


def test_logs_full_response_on_http_error(mock_api_server, mocker):

my_session = Session()
mocked_send = mocker.spy(my_session, "_log_full_response_if_error")
logger_spy = mocker.spy(dlt.common.logger, "error")

source = rest_api_source(
{
"client": {
"base_url": "https://api.example.com",
"session": my_session,
},
"resources": [
{
"name": "crash",
"endpoint": {
"path": "/posts/2/some_details_404",
}

},
],
}
)

with pytest.raises(dlt.extract.exceptions.ResourceExtractionError):
list(source.with_resources("crash"))

mocked_send.assert_called_once()
logger_spy.assert_called_once_with('404 Client Error: None for url: https://api.example.com/posts/2/some_details_404. Full response: {"error":"Post not found"}')

assert mocked_send.call_args[0][0].status_code == 404
assert mocked_send.call_args[0][0].text == '{"error":"Post not found"}'
assert mocked_send.call_args[0][0].url == "https://api.example.com/posts/2/some_details_404"