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

Fix httplib #3820

Merged
merged 1 commit into from
Nov 28, 2024
Merged

Fix httplib #3820

Show file tree
Hide file tree
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
53 changes: 30 additions & 23 deletions sentry_sdk/integrations/opentelemetry/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member Author

Choose a reason for hiding this comment

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

this logic is a mess, I will remove description completely later.
Also this function messes up order of precedence of op and name/desc if explicitly set.

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)

Expand Down
3 changes: 2 additions & 1 deletion sentry_sdk/integrations/stdlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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

Expand Down
10 changes: 6 additions & 4 deletions tests/integrations/httpx/test_httpx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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"
)


Expand Down
157 changes: 72 additions & 85 deletions tests/integrations/stdlib/test_httplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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()
Copy link
Member Author

Choose a reason for hiding this comment

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

removed the mock in favor of the fixture above

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()
Expand All @@ -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()
Expand All @@ -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(
Expand Down Expand Up @@ -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 = {
Copy link
Member Author

Choose a reason for hiding this comment

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

this was not necessary for this test at all

"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
Expand All @@ -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()

Expand All @@ -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:
Expand All @@ -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"
Loading