Skip to content
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
29 changes: 29 additions & 0 deletions sentry_sdk/integrations/_asgi_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from typing import Union
from typing_extensions import Literal

from sentry_sdk._types import Attributes
from sentry_sdk.utils import AnnotatedValue


Expand Down Expand Up @@ -105,3 +106,31 @@
request_data["env"] = {"REMOTE_ADDR": _get_ip(asgi_scope)}

return request_data


def _get_request_attributes(asgi_scope: "Any") -> "dict[str, Any]":
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is an attributes based copy of _get_request_data just above

"""
Return attributes related to the HTTP request from the ASGI scope.
"""
attributes: "Attributes" = {}

ty = asgi_scope["type"]
if ty in ("http", "websocket"):
if asgi_scope.get("method"):
attributes["http.request.method"] = asgi_scope["method"].upper()

headers = _filter_headers(_get_headers(asgi_scope))
for header, value in headers.items():
attributes[f"http.request.header.{header.lower()}"] = value

Check warning on line 124 in sentry_sdk/integrations/_asgi_common.py

View check run for this annotation

@sentry/warden / warden: find-bugs

AnnotatedValue objects from _filter_headers are not properly handled in attributes

_get_request_attributes iterates over headers from _filter_headers and directly assigns values to attributes. When should_send_default_pii() returns False, _filter_headers returns AnnotatedValue objects for sensitive headers (cookies, authorization, etc.). These objects are passed to set_attribute(), which calls format_attribute() -> safe_repr(), resulting in unhelpful string representations like '<AnnotatedValue object>' instead of proper redaction handling. While no actual PII is leaked (AnnotatedValue.value is empty), the behavior is inconsistent with _get_request_data and produces meaningless attribute values.

attributes["http.query"] = _get_query(asgi_scope)

attributes["url.full"] = _get_url(
asgi_scope, "http" if ty == "http" else "ws", headers.get("host")
)

client = asgi_scope.get("client")
if client and should_send_default_pii():
attributes["client.address"] = _get_ip(asgi_scope)

return attributes
60 changes: 58 additions & 2 deletions sentry_sdk/integrations/asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from sentry_sdk.consts import OP
from sentry_sdk.integrations._asgi_common import (
_get_headers,
_get_request_attributes,
_get_request_data,
_get_url,
)
Expand All @@ -23,7 +24,11 @@
nullcontext,
)
from sentry_sdk.sessions import track_session
from sentry_sdk.traces import StreamedSpan
from sentry_sdk.traces import (
StreamedSpan,
SegmentSource,
SOURCE_FOR_STYLE as SEGMENT_SOURCE_FOR_STYLE,
)
from sentry_sdk.tracing import (
SOURCE_FOR_STYLE,
Transaction,
Expand All @@ -40,6 +45,7 @@
_get_installed_modules,
reraise,
capture_internal_exceptions,
qualname_from_function,
)

from typing import TYPE_CHECKING
Expand Down Expand Up @@ -235,7 +241,7 @@
transaction_source, "value", transaction_source
),
"sentry.origin": self.span_origin,
"asgi.type": ty,
"network.protocol.name": ty,

Check warning on line 244 in sentry_sdk/integrations/asgi.py

View check run for this annotation

@sentry/warden / warden: find-bugs

Attribute name mismatch between code and test

The code sets `network.protocol.name` as the attribute key, but the test `test_capture_transaction` in `tests/integrations/asgi/test_asgi.py` line 217 expects the attribute to be `http.request.protocol.name`. This will cause the test to fail when span streaming is enabled, as the test assertion `span["attributes"]["http.request.protocol.name"] == "http"` will not find the expected key.
}

if ty in ("http", "websocket"):
Expand Down Expand Up @@ -301,6 +307,9 @@
else nullcontext()
)

for attribute, value in _get_request_attributes(scope).items():
sentry_scope.set_attribute(attribute, value)

with span_ctx as span:
try:

Expand Down Expand Up @@ -336,6 +345,7 @@
return await self.app(
scope, receive, _sentry_wrapped_send
)

except Exception as exc:
suppress_chained_exceptions = (
sentry_sdk.get_client()
Expand All @@ -350,6 +360,15 @@
with capture_internal_exceptions():
self._capture_request_exception(exc)
reraise(*exc_info)

finally:
with capture_internal_exceptions():
name, source = self._get_segment_name_and_source(
self.transaction_style, scope
)
if isinstance(span, StreamedSpan):
span.name = name
span.set_attribute("sentry.span.source", source)
finally:
_asgi_middleware_applied.set(False)

Expand Down Expand Up @@ -424,3 +443,40 @@
return name, source

return name, source

def _get_segment_name_and_source(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a copy of _get_transaction_name_and_source above, just adapted for segments

self: "SentryAsgiMiddleware", segment_style: str, asgi_scope: "Any"
) -> "Tuple[str, str]":
name = None
source = SEGMENT_SOURCE_FOR_STYLE[segment_style].value
ty = asgi_scope.get("type")

if segment_style == "endpoint":
endpoint = asgi_scope.get("endpoint")
# Webframeworks like Starlette mutate the ASGI env once routing is
# done, which is sometime after the request has started. If we have
# an endpoint, overwrite our generic transaction name.
if endpoint:
name = qualname_from_function(endpoint) or ""
else:
name = _get_url(asgi_scope, "http" if ty == "http" else "ws", host=None)
source = SegmentSource.URL.value

elif segment_style == "url":
# FastAPI includes the route object in the scope to let Sentry extract the
# path from it for the transaction name
route = asgi_scope.get("route")
if route:
path = getattr(route, "path", None)
if path is not None:
name = path
else:
name = _get_url(asgi_scope, "http" if ty == "http" else "ws", host=None)
source = SegmentSource.URL.value

if name is None:
name = _DEFAULT_TRANSACTION_NAME
source = SegmentSource.ROUTE.value
return name, source

return name, source
23 changes: 23 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -1212,6 +1212,29 @@
client.set_cookie(key, value)


def envelopes_to_spans(envelopes):
res: "list[dict[str, Any]]" = []
for envelope in envelopes:
for item in envelope.items:
if item.type == "span":
for span_json in item.payload.json["items"]:
span = {
"start_timestamp": span_json["start_timestamp"],
"end_timestamp": span_json.get("end_timestamp"),
"trace_id": span_json["trace_id"],
"span_id": span_json["span_id"],
"name": span_json["name"],
"status": span_json["status"],
"is_segment": span_json["is_segment"],
"parent_span_id": span_json.get("parent_span_id"),
"attributes": {
k: v["value"] for (k, v) in span_json["attributes"].items()
},

Check warning on line 1232 in tests/conftest.py

View check run for this annotation

@sentry/warden / warden: code-review

envelopes_to_spans raises KeyError when span has no attributes

The function directly accesses `span_json["attributes"]` which will raise a `KeyError` if the span has no attributes. Looking at `_span_batcher.py:116-119`, the `attributes` field is only serialized when `item._attributes` is truthy - spans without attributes won't have this key in the JSON. This will cause test failures if any span being parsed lacks attributes.

Check warning on line 1232 in tests/conftest.py

View check run for this annotation

@sentry/warden / warden: find-bugs

KeyError when span has no attributes

The `envelopes_to_spans` function directly accesses `span_json["attributes"]` without checking if the key exists. In `_span_batcher.py` line 116, the `attributes` field is only included in the transport format when `item._attributes` is truthy (non-empty). If a span has no attributes set, `span_json["attributes"]` will not exist, causing a `KeyError` when the function tries to iterate over it.
}
res.append(span)
return res


@contextmanager
def patch_start_tracing_child(
fake_transaction_is_none: bool = False,
Expand Down
80 changes: 63 additions & 17 deletions tests/integrations/asgi/test_asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from sentry_sdk.tracing import TransactionSource
from sentry_sdk.integrations._asgi_common import _get_ip, _get_headers
from sentry_sdk.integrations.asgi import SentryAsgiMiddleware, _looks_like_asgi3
from tests.conftest import envelopes_to_spans

from async_asgi_testclient import TestClient

Expand Down Expand Up @@ -164,34 +165,79 @@


@pytest.mark.asyncio
@pytest.mark.parametrize(
("span_streaming", "send_default_pii"),
[[False, True], [False, True]],

Check failure on line 170 in tests/integrations/asgi/test_asgi.py

View check run for this annotation

@sentry/warden / warden: code-review

Test parameterization duplicates same values, never tests span_streaming=True code path

The `@pytest.mark.parametrize` decorator specifies `[[False, True], [False, True]]` - two identical test cases. This means `span_streaming` is always `False`, so the entire `if span_streaming:` branch (lines 198-222) that validates the new span streaming functionality is never executed. The PR's stated purpose is to migrate away from event processors in span-first mode, but this test doesn't actually test that functionality.

Check failure on line 170 in tests/integrations/asgi/test_asgi.py

View check run for this annotation

@sentry/warden / warden: find-bugs

Test parameterization has duplicate values - span_streaming path never tested

The parametrize decorator on line 170 uses `[[False, True], [False, True]]` which creates two identical test cases with `span_streaming=False, send_default_pii=True`. This means the new span streaming code path (lines 198-222) will never be executed. The intended parameterization should test all combinations: `[[False, False], [False, True], [True, False], [True, True]]` or at least include `span_streaming=True` cases.
)
async def test_capture_transaction(
sentry_init,
asgi3_app,
capture_events,
capture_envelopes,
span_streaming,
send_default_pii,
):
sentry_init(send_default_pii=True, traces_sample_rate=1.0)
sentry_init(
send_default_pii=send_default_pii,
traces_sample_rate=1.0,
_experiments={
"trace_lifecycle": "stream" if span_streaming else "static",
},
)
app = SentryAsgiMiddleware(asgi3_app)

async with TestClient(app) as client:
events = capture_events()
if span_streaming:
envelopes = capture_envelopes()
else:
events = capture_events()
await client.get("/some_url?somevalue=123")

(transaction_event,) = events
sentry_sdk.flush()

assert transaction_event["type"] == "transaction"
assert transaction_event["transaction"] == "/some_url"
assert transaction_event["transaction_info"] == {"source": "url"}
assert transaction_event["contexts"]["trace"]["op"] == "http.server"
assert transaction_event["request"] == {
"headers": {
"host": "localhost",
"remote-addr": "127.0.0.1",
"user-agent": "ASGI-Test-Client",
},
"method": "GET",
"query_string": "somevalue=123",
"url": "http://localhost/some_url",
}
if span_streaming:
spans = envelopes_to_spans(envelopes)
assert len(spans) == 1
(span,) = spans

assert span["is_segment"] is True
assert span["name"] == "/some_url"

assert span["attributes"]["sentry.span.source"] == "url"
assert span["attributes"]["sentry.op"] == "http.server"

if send_default_pii:
assert span["attributes"]["client.address"] == "127.0.0.1"
else:
assert "client.address" not in span["attributes"]

assert span["attributes"]["http.request.method"] == "GET"
assert span["attributes"]["url.full"] == "http://localhost/some_url"
assert span["attributes"]["http.query"] == "somevalue=123"
assert span["attributes"]["http.request.protocol.name"] == "http"
assert span["attributes"]["http.request.header.host"] == "localhost"
assert span["attributes"]["http.request.header.remote-addr"] == "127.0.0.1"
assert (
span["attributes"]["http.request.header.user-agent"] == "ASGI-Test-Client"
)

else:
(transaction_event,) = events

assert transaction_event["type"] == "transaction"
assert transaction_event["transaction"] == "/some_url"
assert transaction_event["transaction_info"] == {"source": "url"}
assert transaction_event["contexts"]["trace"]["op"] == "http.server"
assert transaction_event["request"] == {
"headers": {
"host": "localhost",
"remote-addr": "127.0.0.1",
"user-agent": "ASGI-Test-Client",
},
"method": "GET",
"query_string": "somevalue=123",
"url": "http://localhost/some_url",
}


@pytest.mark.asyncio
Expand Down
26 changes: 2 additions & 24 deletions tests/tracing/test_span_streaming.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,43 +2,21 @@
import re
import sys
import time
from typing import Any
from unittest import mock

import pytest

import sentry_sdk
from sentry_sdk.profiler.continuous_profiler import get_profiler_id
from sentry_sdk.traces import NoOpStreamedSpan, SpanStatus, StreamedSpan
from tests.conftest import envelopes_to_spans


minimum_python_38 = pytest.mark.skipif(
sys.version_info < (3, 8), reason="Asyncio tests need Python >= 3.8"
)


def envelopes_to_spans(envelopes):
res: "list[dict[str, Any]]" = []
for envelope in envelopes:
for item in envelope.items:
if item.type == "span":
for span_json in item.payload.json["items"]:
span = {
"start_timestamp": span_json["start_timestamp"],
"end_timestamp": span_json.get("end_timestamp"),
"trace_id": span_json["trace_id"],
"span_id": span_json["span_id"],
"name": span_json["name"],
"status": span_json["status"],
"is_segment": span_json["is_segment"],
"parent_span_id": span_json.get("parent_span_id"),
"attributes": {
k: v["value"] for (k, v) in span_json["attributes"].items()
},
}
res.append(span)
return res


def test_start_span(sentry_init, capture_envelopes):
sentry_init(
traces_sample_rate=1.0,
Expand Down
Loading