Set level based on status code for HTTP client breadcrumbs (#4004)

- add logic to `maybe_create_breadcrumbs_from_span` to set the `level`
of the breadcrumb to `warning` for the client error range (4xx) and to
`error` for server errors (5xx)
- add functionality to the simple HTTP server that we use in some tests
to respond with a specific error code
- we were (and are) still "using" `responses` in multiple places, but
they're not actually active (the `activate` decorator is missing) and
we're making actual requests outside -- we should clean this up
- we also can't use `responses` for stdlib/requests tests since they
patch something that we patch
- add `httpx`, `stdlib`, `requests`, `aiohttp` tests for the new
behavior
- restrict the `requests` tests to 3.7+ since in 3.6, the span is
finished before the HTTP status is set for some reason...

Closes #4000

Author: sentrivana

sentry_sdk/tracing_utils.py

@@ -156,13 +156,27 @@ def record_sql_queries(
 
 def maybe_create_breadcrumbs_from_span(scope, span):
     # type: (sentry_sdk.Scope, sentry_sdk.tracing.Span) -> None
-
     if span.op == OP.DB_REDIS:
         scope.add_breadcrumb(
             message=span.description, type="redis", category="redis", data=span._tags
         )
+
     elif span.op == OP.HTTP_CLIENT:
-        scope.add_breadcrumb(type="http", category="httplib", data=span._data)
+        level = None
+        status_code = span._data.get(SPANDATA.HTTP_STATUS_CODE)
+        if status_code:
+            if 500 <= status_code <= 599:
+                level = "error"
+            elif 400 <= status_code <= 499:
+                level = "warning"
+
+        if level:
+            scope.add_breadcrumb(
+                type="http", category="httplib", data=span._data, level=level
+            )
+        else:
+            scope.add_breadcrumb(type="http", category="httplib", data=span._data)
+
     elif span.op == "subprocess":
         scope.add_breadcrumb(
             type="subprocess",

tests/conftest.py

@@ -587,8 +587,14 @@ def suppress_deprecation_warnings():
 
 class MockServerRequestHandler(BaseHTTPRequestHandler):
     def do_GET(self):  # noqa: N802
-        # Process an HTTP GET request and return a response with an HTTP 200 status.
-        self.send_response(200)
+        # Process an HTTP GET request and return a response.
+        # If the path ends with /status/<number>, return status code <number>.
+        # Otherwise return a 200 response.
+        code = 200
+        if "/status/" in self.path:
+            code = int(self.path[-3:])
+
+        self.send_response(code)
         self.end_headers()
         return
 

tests/integrations/aiohttp/test_aiohttp.py

@@ -525,6 +525,61 @@ async def handler(request):
         )
 
 
+@pytest.mark.parametrize(
+    "status_code,level",
+    [
+        (200, None),
+        (301, None),
+        (403, "warning"),
+        (405, "warning"),
+        (500, "error"),
+    ],
+)
+@pytest.mark.asyncio
+async def test_crumb_capture_client_error(
+    sentry_init,
+    aiohttp_raw_server,
+    aiohttp_client,
+    event_loop,
+    capture_events,
+    status_code,
+    level,
+):
+    sentry_init(integrations=[AioHttpIntegration()])
+
+    async def handler(request):
+        return web.Response(status=status_code)
+
+    raw_server = await aiohttp_raw_server(handler)
+
+    with start_transaction():
+        events = capture_events()
+
+        client = await aiohttp_client(raw_server)
+        resp = await client.get("/")
+        assert resp.status == status_code
+        capture_message("Testing!")
+
+        (event,) = events
+
+        crumb = event["breadcrumbs"]["values"][0]
+        assert crumb["type"] == "http"
+        if level is None:
+            assert "level" not in crumb
+        else:
+            assert crumb["level"] == level
+        assert crumb["category"] == "httplib"
+        assert crumb["data"] == ApproxDict(
+            {
+                "url": "http://127.0.0.1:{}/".format(raw_server.port),
+                "http.fragment": "",
+                "http.method": "GET",
+                "http.query": "",
+                "http.response.status_code": status_code,
+            }
+        )
+
+
 @pytest.mark.asyncio
 async def test_outgoing_trace_headers(sentry_init, aiohttp_raw_server, aiohttp_client):
     sentry_init(

tests/integrations/httpx/test_httpx.py

@@ -57,6 +57,64 @@ def before_breadcrumb(crumb, hint):
         )
 
 
+@pytest.mark.parametrize(
+    "httpx_client",
+    (httpx.Client(), httpx.AsyncClient()),
+)
+@pytest.mark.parametrize(
+    "status_code,level",
+    [
+        (200, None),
+        (301, None),
+        (403, "warning"),
+        (405, "warning"),
+        (500, "error"),
+    ],
+)
+def test_crumb_capture_client_error(
+    sentry_init, capture_events, httpx_client, httpx_mock, status_code, level
+):
+    httpx_mock.add_response(status_code=status_code)
+
+    sentry_init(integrations=[HttpxIntegration()])
+
+    url = "http://example.com/"
+
+    with start_transaction():
+        events = capture_events()
+
+        if asyncio.iscoroutinefunction(httpx_client.get):
+            response = asyncio.get_event_loop().run_until_complete(
+                httpx_client.get(url)
+            )
+        else:
+            response = httpx_client.get(url)
+
+        assert response.status_code == status_code
+        capture_message("Testing!")
+
+        (event,) = events
+
+        crumb = event["breadcrumbs"]["values"][0]
+        assert crumb["type"] == "http"
+        assert crumb["category"] == "httplib"
+
+        if level is None:
+            assert "level" not in crumb
+        else:
+            assert crumb["level"] == level
+
+        assert crumb["data"] == ApproxDict(
+            {
+                "url": url,
+                SPANDATA.HTTP_METHOD: "GET",
+                SPANDATA.HTTP_FRAGMENT: "",
+                SPANDATA.HTTP_QUERY: "",
+                SPANDATA.HTTP_STATUS_CODE: status_code,
+            }
+        )
+
+
 @pytest.mark.parametrize(
     "httpx_client",
     (httpx.Client(), httpx.AsyncClient()),

tests/integrations/requests/test_requests.py

@@ -1,30 +1,77 @@
+import sys
 from unittest import mock
 
 import pytest
 import requests
-import responses
 
 from sentry_sdk import capture_message
 from sentry_sdk.consts import SPANDATA
 from sentry_sdk.integrations.stdlib import StdlibIntegration
-from tests.conftest import ApproxDict
+from tests.conftest import ApproxDict, create_mock_http_server
+
+PORT = create_mock_http_server()
 
 
 def test_crumb_capture(sentry_init, capture_events):
     sentry_init(integrations=[StdlibIntegration()])
+    events = capture_events()
 
-    url = "http://example.com/"
-    responses.add(responses.GET, url, status=200)
+    url = f"http://localhost:{PORT}/hello-world"  # noqa:E231
+    response = requests.get(url)
+    capture_message("Testing!")
+
+    (event,) = events
+    (crumb,) = event["breadcrumbs"]["values"]
+    assert crumb["type"] == "http"
+    assert crumb["category"] == "httplib"
+    assert crumb["data"] == ApproxDict(
+        {
+            "url": url,
+            SPANDATA.HTTP_METHOD: "GET",
+            SPANDATA.HTTP_FRAGMENT: "",
+            SPANDATA.HTTP_QUERY: "",
+            SPANDATA.HTTP_STATUS_CODE: response.status_code,
+            "reason": response.reason,
+        }
+    )
+
+
+@pytest.mark.skipif(
+    sys.version_info < (3, 7),
+    reason="The response status is not set on the span early enough in 3.6",
+)
+@pytest.mark.parametrize(
+    "status_code,level",
+    [
+        (200, None),
+        (301, None),
+        (403, "warning"),
+        (405, "warning"),
+        (500, "error"),
+    ],
+)
+def test_crumb_capture_client_error(sentry_init, capture_events, status_code, level):
+    sentry_init(integrations=[StdlibIntegration()])
 
     events = capture_events()
 
+    url = f"http://localhost:{PORT}/status/{status_code}"  # noqa:E231
     response = requests.get(url)
+
+    assert response.status_code == status_code
+
     capture_message("Testing!")
 
     (event,) = events
     (crumb,) = event["breadcrumbs"]["values"]
     assert crumb["type"] == "http"
     assert crumb["category"] == "httplib"
+
+    if level is None:
+        assert "level" not in crumb
+    else:
+        assert crumb["level"] == level
+
     assert crumb["data"] == ApproxDict(
         {
             "url": url,
@@ -41,11 +88,10 @@ def test_crumb_capture(sentry_init, capture_events):
 def test_omit_url_data_if_parsing_fails(sentry_init, capture_events):
     sentry_init(integrations=[StdlibIntegration()])
 
-    url = "https://example.com"
-    responses.add(responses.GET, url, status=200)
-
     events = capture_events()
 
+    url = f"http://localhost:{PORT}/ok"  # noqa:E231
+
     with mock.patch(
         "sentry_sdk.integrations.stdlib.parse_url",
         side_effect=ValueError,
@@ -63,7 +109,6 @@ def test_omit_url_data_if_parsing_fails(sentry_init, capture_events):
             # no url related data
         }
     )
-
     assert "url" not in event["breadcrumbs"]["values"][0]["data"]
     assert SPANDATA.HTTP_FRAGMENT not in event["breadcrumbs"]["values"][0]["data"]
     assert SPANDATA.HTTP_QUERY not in event["breadcrumbs"]["values"][0]["data"]

tests/integrations/stdlib/test_httplib.py

@@ -1,6 +1,7 @@
 import random
 from http.client import HTTPConnection, HTTPSConnection
 from socket import SocketIO
+from urllib.error import HTTPError
 from urllib.request import urlopen
 from unittest import mock
 
@@ -42,6 +43,50 @@ def test_crumb_capture(sentry_init, capture_events):
     )
 
 
+@pytest.mark.parametrize(
+    "status_code,level",
+    [
+        (200, None),
+        (301, None),
+        (403, "warning"),
+        (405, "warning"),
+        (500, "error"),
+    ],
+)
+def test_crumb_capture_client_error(sentry_init, capture_events, status_code, level):
+    sentry_init(integrations=[StdlibIntegration()])
+    events = capture_events()
+
+    url = f"http://localhost:{PORT}/status/{status_code}"  # noqa:E231
+    try:
+        urlopen(url)
+    except HTTPError:
+        pass
+
+    capture_message("Testing!")
+
+    (event,) = events
+    (crumb,) = event["breadcrumbs"]["values"]
+
+    assert crumb["type"] == "http"
+    assert crumb["category"] == "httplib"
+
+    if level is None:
+        assert "level" not in crumb
+    else:
+        assert crumb["level"] == level
+
+    assert crumb["data"] == ApproxDict(
+        {
+            "url": url,
+            SPANDATA.HTTP_METHOD: "GET",
+            SPANDATA.HTTP_STATUS_CODE: status_code,
+            SPANDATA.HTTP_FRAGMENT: "",
+            SPANDATA.HTTP_QUERY: "",
+        }
+    )
+
+
 def test_crumb_capture_hint(sentry_init, capture_events):
     def before_breadcrumb(crumb, hint):
         crumb["data"]["extra"] = "foo"