Fix httplib (#3820)

Author: sl0thentr0py

sentry_sdk/integrations/opentelemetry/utils.py

@@ -161,36 +161,43 @@ def span_data_for_http_method(span):
     # type: (ReadableSpan) -> OtelExtractedSpanData
     span_attributes = span.attributes or {}
 
-    op = "http"
+    op = span_attributes.get(SentrySpanAttribute.OP)
+    if op is None:
+        op = "http"
 
-    if span.kind == SpanKind.SERVER:
-        op += ".server"
-    elif span.kind == SpanKind.CLIENT:
-        op += ".client"
+        if span.kind == SpanKind.SERVER:
+            op += ".server"
+        elif span.kind == SpanKind.CLIENT:
+            op += ".client"
 
     http_method = span_attributes.get(SpanAttributes.HTTP_METHOD)
     route = span_attributes.get(SpanAttributes.HTTP_ROUTE)
     target = span_attributes.get(SpanAttributes.HTTP_TARGET)
     peer_name = span_attributes.get(SpanAttributes.NET_PEER_NAME)
 
-    description = f"{http_method}"
-
-    if route:
-        description = f"{http_method} {route}"
-    elif target:
-        description = f"{http_method} {target}"
-    elif peer_name:
-        description = f"{http_method} {peer_name}"
-    else:
-        url = span_attributes.get(SpanAttributes.HTTP_URL)
-        url = cast("Optional[str]", url)
-
-        if url:
-            parsed_url = urlparse(url)
-            url = "{}://{}{}".format(
-                parsed_url.scheme, parsed_url.netloc, parsed_url.path
-            )
-            description = f"{http_method} {url}"
+    # TODO-neel-potel remove description completely
+    description = span_attributes.get(
+        SentrySpanAttribute.DESCRIPTION
+    ) or span_attributes.get(SentrySpanAttribute.NAME)
+    if description is None:
+        description = f"{http_method}"
+
+        if route:
+            description = f"{http_method} {route}"
+        elif target:
+            description = f"{http_method} {target}"
+        elif peer_name:
+            description = f"{http_method} {peer_name}"
+        else:
+            url = span_attributes.get(SpanAttributes.HTTP_URL)
+            url = cast("Optional[str]", url)
+
+            if url:
+                parsed_url = urlparse(url)
+                url = "{}://{}{}".format(
+                    parsed_url.scheme, parsed_url.netloc, parsed_url.path
+                )
+                description = f"{http_method} {url}"
 
     status, http_status = extract_span_status(span)
 

sentry_sdk/integrations/stdlib.py

@@ -96,6 +96,7 @@ def putrequest(self, method, url, *args, **kwargs):
             origin="auto.http.stdlib.httplib",
             only_if_parent=True,
         )
+        span.__enter__()
 
         data = {
             SPANDATA.HTTP_METHOD: method,
@@ -152,7 +153,7 @@ def getresponse(self, *args, **kwargs):
             span.set_http_status(int(rv.status))
             span.set_data("reason", rv.reason)
         finally:
-            span.finish()
+            span.__exit__(None, None, None)
 
         return rv
 

tests/integrations/httpx/test_httpx.py

@@ -101,7 +101,10 @@ def test_outgoing_trace_headers(sentry_init, httpx_client, capture_envelopes):
     (httpx.Client(), httpx.AsyncClient()),
 )
 def test_outgoing_trace_headers_append_to_baggage(
-    sentry_init, httpx_client, capture_envelopes, SortedBaggage,  # noqa: N803
+    sentry_init,
+    httpx_client,
+    capture_envelopes,
+    SortedBaggage,  # noqa: N803
 ):
     sentry_init(
         traces_sample_rate=1.0,
@@ -137,9 +140,8 @@ def test_outgoing_trace_headers_append_to_baggage(
         parent_span_id=request_span["span_id"],
         sampled=1,
     )
-    assert (
-        response.request.headers["baggage"]
-        == SortedBaggage(f"custom=data,sentry-trace_id={trace_id},sentry-environment=production,sentry-release=d08ebdb9309e1b004c6f52202de58a09c2268e42,sentry-transaction=/interactions/other-dogs/new-dog,sentry-sample_rate=1.0,sentry-sampled=true")
+    assert response.request.headers["baggage"] == SortedBaggage(
+        f"custom=data,sentry-trace_id={trace_id},sentry-environment=production,sentry-release=d08ebdb9309e1b004c6f52202de58a09c2268e42,sentry-transaction=/interactions/other-dogs/new-dog,sentry-sample_rate=1.0,sentry-sampled=true"
     )
 
 

tests/integrations/stdlib/test_httplib.py

@@ -6,7 +6,7 @@
 
 import pytest
 
-from sentry_sdk import capture_message, start_transaction
+from sentry_sdk import capture_message, start_span, continue_trace, isolation_scope
 from sentry_sdk.consts import MATCH_ALL, SPANDATA
 from sentry_sdk.tracing import Transaction
 from sentry_sdk.integrations.stdlib import StdlibIntegration
@@ -16,6 +16,31 @@
 PORT = create_mock_http_server()
 
 
+@pytest.fixture
+def capture_request_headers(monkeypatch):
+    """
+    HTTPConnection.send is passed a string containing (among other things)
+    the headers on the request. Mock it so we can check the headers.
+    """
+
+    def inner(do_send=True):
+        request_headers = {}
+        old_send = HTTPConnection.send
+
+        def patched_send(self, data):
+            for line in data.decode("utf-8").split("\r\n")[1:]:
+                if line:
+                    key, val = line.split(": ")
+                    request_headers[key] = val
+            if do_send:
+                old_send(self, data)
+
+        monkeypatch.setattr(HTTPConnection, "send", patched_send)
+        return request_headers
+
+    return inner
+
+
 def test_crumb_capture(sentry_init, capture_events):
     sentry_init(integrations=[StdlibIntegration()])
     events = capture_events()
@@ -79,7 +104,7 @@ def test_empty_realurl(sentry_init):
     """
 
     sentry_init(dsn="")
-    HTTPConnection("example.com", port=443).putrequest("POST", None)
+    HTTPConnection("localhost", PORT).putrequest("POST", None)
 
 
 def test_httplib_misuse(sentry_init, capture_events, request):
@@ -131,40 +156,28 @@ def test_httplib_misuse(sentry_init, capture_events, request):
     )
 
 
-def test_outgoing_trace_headers(sentry_init, monkeypatch, capture_envelopes):
-    # HTTPSConnection.send is passed a string containing (among other things)
-    # the headers on the request. Mock it so we can check the headers, and also
-    # so it doesn't try to actually talk to the internet.
-    mock_send = mock.Mock()
-    monkeypatch.setattr(HTTPSConnection, "send", mock_send)
-
+def test_outgoing_trace_headers(
+    sentry_init, capture_envelopes, capture_request_headers, SortedBaggage
+):
     sentry_init(traces_sample_rate=1.0)
-
     envelopes = capture_envelopes()
+    request_headers = capture_request_headers()
 
     headers = {}
+    headers["sentry-trace"] = "771a43a4192642f0b136d5159a501700-1234567890abcdef-1"
     headers["baggage"] = (
         "other-vendor-value-1=foo;bar;baz, sentry-trace_id=771a43a4192642f0b136d5159a501700, "
-        "sentry-public_key=49d0f7386ad645858ae85020e393bef3, sentry-sample_rate=0.01337, "
+        "sentry-public_key=49d0f7386ad645858ae85020e393bef3, "
+        "sentry-sampled=true, sentry-sample_rate=0.01337, "
         "sentry-user_id=Am%C3%A9lie, other-vendor-value-2=foo;bar;"
     )
 
-    transaction = Transaction.continue_from_headers(headers)
-
-    with start_transaction(
-        transaction=transaction,
-        name="/interactions/other-dogs/new-dog",
-        op="greeting.sniff",
-        trace_id="12312012123120121231201212312012",
-    ) as transaction:
-        HTTPSConnection("www.squirrelchasers.com").request("GET", "/top-chasers")
-
-        (request_str,) = mock_send.call_args[0]
-        request_headers = {}
-        for line in request_str.decode("utf-8").split("\r\n")[1:]:
-            if line:
-                key, val = line.split(": ")
-                request_headers[key] = val
+    with isolation_scope():
+        with continue_trace(headers):
+            with start_span(name="/interactions/other-dogs/new-dog"):
+                conn = HTTPConnection("localhost", PORT)
+                conn.request("GET", "/top-chasers")
+                conn.getresponse()
 
     (envelope,) = envelopes
     transaction = envelope.get_transaction_event()
@@ -180,38 +193,30 @@ def test_outgoing_trace_headers(sentry_init, monkeypatch, capture_envelopes):
     expected_outgoing_baggage = (
         "sentry-trace_id=771a43a4192642f0b136d5159a501700,"
         "sentry-public_key=49d0f7386ad645858ae85020e393bef3,"
+        "sentry-sampled=true,"
         "sentry-sample_rate=0.01337,"
         "sentry-user_id=Am%C3%A9lie"
     )
 
-    assert request_headers["baggage"] == expected_outgoing_baggage
+    assert request_headers["baggage"] == SortedBaggage(expected_outgoing_baggage)
 
 
-def test_outgoing_trace_headers_head_sdk(sentry_init, monkeypatch, capture_envelopes):
-    # HTTPSConnection.send is passed a string containing (among other things)
-    # the headers on the request. Mock it so we can check the headers, and also
-    # so it doesn't try to actually talk to the internet.
-    mock_send = mock.Mock()
-    monkeypatch.setattr(HTTPSConnection, "send", mock_send)
-
+def test_outgoing_trace_headers_head_sdk(
+    sentry_init, monkeypatch, capture_request_headers, capture_envelopes, SortedBaggage
+):
     # make sure transaction is always sampled
     monkeypatch.setattr(random, "random", lambda: 0.1)
 
     sentry_init(traces_sample_rate=0.5, release="foo")
-
     envelopes = capture_envelopes()
+    request_headers = capture_request_headers()
 
-    transaction = Transaction.continue_from_headers({})
-
-    with start_transaction(transaction=transaction, name="Head SDK tx") as transaction:
-        HTTPSConnection("www.squirrelchasers.com").request("GET", "/top-chasers")
-
-        (request_str,) = mock_send.call_args[0]
-        request_headers = {}
-        for line in request_str.decode("utf-8").split("\r\n")[1:]:
-            if line:
-                key, val = line.split(": ")
-                request_headers[key] = val
+    with isolation_scope():
+        with continue_trace({}):
+            with start_span(name="Head SDK tx") as root_span:
+                conn = HTTPConnection("localhost", PORT)
+                conn.request("GET", "/top-chasers")
+                conn.getresponse()
 
     (envelope,) = envelopes
     transaction = envelope.get_transaction_event()
@@ -225,14 +230,15 @@ def test_outgoing_trace_headers_head_sdk(sentry_init, monkeypatch, capture_envel
     assert request_headers["sentry-trace"] == expected_sentry_trace
 
     expected_outgoing_baggage = (
-        "sentry-trace_id=%s,"
+        f"sentry-trace_id={root_span.trace_id},"
         "sentry-environment=production,"
         "sentry-release=foo,"
         "sentry-sample_rate=0.5,"
-        "sentry-sampled=%s"
-    ) % (transaction.trace_id, "true" if transaction.sampled else "false")
+        "sentry-sampled=true,"
+        "sentry-transaction=Head%20SDK%20tx"
+    )
 
-    assert request_headers["baggage"] == expected_outgoing_baggage
+    assert request_headers["baggage"] == SortedBaggage(expected_outgoing_baggage)
 
 
 @pytest.mark.parametrize(
@@ -295,42 +301,23 @@ def test_outgoing_trace_headers_head_sdk(sentry_init, monkeypatch, capture_envel
     ],
 )
 def test_option_trace_propagation_targets(
-    sentry_init, monkeypatch, trace_propagation_targets, host, path, trace_propagated
+    sentry_init,
+    capture_request_headers,
+    trace_propagation_targets,
+    host,
+    path,
+    trace_propagated,
 ):
-    # HTTPSConnection.send is passed a string containing (among other things)
-    # the headers on the request. Mock it so we can check the headers, and also
-    # so it doesn't try to actually talk to the internet.
-    mock_send = mock.Mock()
-    monkeypatch.setattr(HTTPSConnection, "send", mock_send)
-
     sentry_init(
         trace_propagation_targets=trace_propagation_targets,
         traces_sample_rate=1.0,
     )
 
-    headers = {
-        "baggage": (
-            "sentry-trace_id=771a43a4192642f0b136d5159a501700, "
-            "sentry-public_key=49d0f7386ad645858ae85020e393bef3, sentry-sample_rate=0.01337, "
-        )
-    }
-
-    transaction = Transaction.continue_from_headers(headers)
-
-    with start_transaction(
-        transaction=transaction,
-        name="/interactions/other-dogs/new-dog",
-        op="greeting.sniff",
-        trace_id="12312012123120121231201212312012",
-    ) as transaction:
-        HTTPSConnection(host).request("GET", path)
+    request_headers = capture_request_headers(do_send=False)
 
-        (request_str,) = mock_send.call_args[0]
-        request_headers = {}
-        for line in request_str.decode("utf-8").split("\r\n")[1:]:
-            if line:
-                key, val = line.split(": ")
-                request_headers[key] = val
+    with start_span(name="foo"):
+        HTTPSConnection(host).request("GET", path)
+        # don't invoke getresponse to avoid actual network traffic
 
         if trace_propagated:
             assert "sentry-trace" in request_headers
@@ -344,8 +331,8 @@ def test_span_origin(sentry_init, capture_events):
     sentry_init(traces_sample_rate=1.0, debug=True)
     events = capture_events()
 
-    with start_transaction(name="foo"):
-        conn = HTTPSConnection("example.com")
+    with start_span(name="foo"):
+        conn = HTTPConnection("localhost", PORT)
         conn.request("GET", "/foo")
         conn.getresponse()
 
@@ -364,9 +351,9 @@ def test_http_timeout(monkeypatch, sentry_init, capture_envelopes):
 
     envelopes = capture_envelopes()
 
-    with start_transaction(op="op", name="name"):
+    with start_span(op="op", name="name"):
         try:
-            conn = HTTPSConnection("www.squirrelchasers.com")
+            conn = HTTPConnection("localhost", PORT)
             conn.request("GET", "/top-chasers")
             conn.getresponse()
         except Exception:
@@ -385,4 +372,4 @@ def test_http_timeout(monkeypatch, sentry_init, capture_envelopes):
 
     span = transaction["spans"][0]
     assert span["op"] == "http.client"
-    assert span["description"] == "GET https://www.squirrelchasers.com/top-chasers"
+    assert span["description"] == f"GET http://localhost:{PORT}/top-chasers"