Changes after doing a review

Author: ericapisani

sentry_sdk/integrations/aiohttp.py

@@ -4,26 +4,28 @@
 
 import sentry_sdk
 from sentry_sdk.api import continue_trace
-from sentry_sdk.consts import OP, SPANSTATUS, SPANDATA
+from sentry_sdk.consts import OP, SPANDATA, SPANSTATUS
 from sentry_sdk.integrations import (
     _DEFAULT_FAILED_REQUEST_STATUS_CODES,
-    _check_minimum_version,
-    Integration,
     DidNotEnable,
+    Integration,
+    _check_minimum_version,
 )
-from sentry_sdk.integrations.logging import ignore_logger
-from sentry_sdk.scope import should_send_default_pii
-from sentry_sdk.sessions import track_session
 from sentry_sdk.integrations._wsgi_common import (
     _filter_headers,
     request_body_within_bounds,
 )
+from sentry_sdk.integrations.logging import ignore_logger
+from sentry_sdk.scope import should_send_default_pii
+from sentry_sdk.sessions import track_session
+from sentry_sdk.traces import (
+    SOURCE_FOR_STYLE as SEGMENT_SOURCE_FOR_STYLE,
+)
 from sentry_sdk.traces import (
     NoOpStreamedSpan,
     SegmentSource,
     SpanStatus,
     StreamedSpan,
-    SOURCE_FOR_STYLE as SEGMENT_SOURCE_FOR_STYLE,
 )
 from sentry_sdk.tracing import (
     BAGGAGE_HEADER_NAME,
@@ -36,6 +38,10 @@
     should_propagate_trace,
 )
 from sentry_sdk.utils import (
+    CONTEXTVARS_ERROR_MESSAGE,
+    HAS_REAL_CONTEXTVARS,
+    SENSITIVE_DATA_SUBSTITUTE,
+    AnnotatedValue,
     capture_internal_exceptions,
     ensure_integration_enabled,
     event_from_exception,
@@ -44,39 +50,31 @@
     parse_version,
     reraise,
     transaction_from_function,
-    HAS_REAL_CONTEXTVARS,
-    CONTEXTVARS_ERROR_MESSAGE,
-    SENSITIVE_DATA_SUBSTITUTE,
-    AnnotatedValue,
 )
 
 try:
     import asyncio
 
-    from aiohttp import __version__ as AIOHTTP_VERSION
     from aiohttp import ClientSession, TraceConfig
+    from aiohttp import __version__ as AIOHTTP_VERSION
     from aiohttp.web import Application, HTTPException, UrlDispatcher
 except ImportError:
     raise DidNotEnable("AIOHTTP not installed")
 
 from typing import TYPE_CHECKING
 
 if TYPE_CHECKING:
-    from aiohttp.web_request import Request
-    from aiohttp.web_urldispatcher import UrlMappingMatchInfo
-    from aiohttp import TraceRequestStartParams, TraceRequestEndParams
-
     from collections.abc import Set
     from types import SimpleNamespace
-    from typing import Any
-    from typing import ContextManager
-    from typing import Optional
-    from typing import Tuple
-    from typing import Union
+    from typing import Any, ContextManager, Optional, Tuple, Union
+
+    from aiohttp import TraceRequestEndParams, TraceRequestStartParams
+    from aiohttp.web_request import Request
+    from aiohttp.web_urldispatcher import UrlMappingMatchInfo
 
+    from sentry_sdk._types import Attributes, Event, EventProcessor
     from sentry_sdk.tracing import Span
     from sentry_sdk.utils import ExcInfo
-    from sentry_sdk._types import Attributes, Event, EventProcessor
 
 
 TRANSACTION_STYLE_VALUES = ("handler_name", "method_and_path_pattern")
@@ -145,22 +143,24 @@ async def sentry_app_handle(
                             {"aiohttp_request": request}
                         )
 
-                        header_dict: "dict[str, Any]" = {}
+                        header_attributes: "dict[str, Any]" = {}
                         for header, header_value in _filter_headers(
                             headers, use_annotated_value=False
                         ).items():
-                            header_dict[f"http.request.header.{header.lower()}"] = (
+                            header_attributes[
+                                f"http.request.header.{header.lower()}"
+                            ] = (
                                 # header_value will always be a string because we set `use_annotated_value` to false above
                                 header_value
                             )
 
-                        url_query = (
+                        url_query_attribute = (
                             {"url.query": request.query_string}
                             if request.query_string
                             else {}
                         )
 
-                        client_address = (
+                        client_address_attributes = (
                             {
                                 "client.address": request.remote,
                                 "user.ip_address": request.remote,
@@ -180,9 +180,9 @@ async def sentry_app_handle(
                                 "url.full": "%s://%s%s"
                                 % (request.scheme, request.host, request.path),
                                 "http.request.method": request.method,
-                                **url_query,
-                                **client_address,
-                                **header_dict,
+                                **url_query_attribute,
+                                **client_address_attributes,
+                                **header_attributes,
                             },
                         )
                     else:
@@ -212,8 +212,8 @@ async def sentry_app_handle(
                                         "http.response.status_code", e.status_code
                                     )
 
-                                    # There are a number of sub-exceptions that should have an "ok" status, but will
-                                    # actually have a status of error once the `raise` happens below
+                                    # There are a number of sub-exceptions that should have an "ok" status,
+                                    # but will actually have a status of error once the `raise` happens below
                                     # and we pass through the `should_be_treated_as_error` method invoked
                                     # within a span's `__exit__` method.
                                     #
@@ -224,6 +224,8 @@ async def sentry_app_handle(
                                     # approach as well, so this matches what we do today.
                                     span.status = SpanStatus.ERROR.value
                                 else:
+                                    # Since a NoOpStreamedSpan can end up here, we have to guard against it
+                                    # so this only gets set in the legacy transaction approach.
                                     if not isinstance(span, NoOpStreamedSpan):
                                         span.set_http_status(e.status_code)
 

tests/integrations/aiohttp/test_aiohttp.py

@@ -1,31 +1,33 @@
-import os
-import datetime
 import asyncio
+import datetime
 import json
-
+import os
 from contextlib import suppress
 from unittest import mock
 
 import pytest
-
 from aiohttp import web
 from aiohttp.client import ServerDisconnectedError
-from aiohttp.web_request import Request
 from aiohttp.web_exceptions import (
+    HTTPBadRequest,
     HTTPInternalServerError,
     HTTPNetworkAuthenticationRequired,
-    HTTPBadRequest,
     HTTPNotFound,
     HTTPUnavailableForLegalReasons,
 )
+from aiohttp.web_request import Request
 
 import sentry_sdk
 from sentry_sdk import capture_message, start_transaction
+from sentry_sdk._types import OVER_SIZE_LIMIT_SUBSTITUTE
 from sentry_sdk.consts import SPANDATA
-from sentry_sdk.integrations.aiohttp import AioHttpIntegration, create_trace_config
+from sentry_sdk.integrations.aiohttp import (
+    BODY_NOT_READ_MESSAGE,
+    AioHttpIntegration,
+    create_trace_config,
+)
 from sentry_sdk.utils import SENSITIVE_DATA_SUBSTITUTE
 from tests.conftest import ApproxDict
-from sentry_sdk._types import OVER_SIZE_LIMIT_SUBSTITUTE
 
 
 @pytest.mark.asyncio
@@ -1317,17 +1319,15 @@ async def hello(request):
     # test the segment is the aiohttp_client's outgoing http.client span (the
     # test client is itself sentry-instrumented). In production with separate
     # processes, the server span is the segment; the assertion is the same.
-    segments = [item.payload for item in items if item.payload.get("is_segment")]
-    assert len(segments) == 1
-    assert segments[0]["attributes"]["http.request.body.data"] == json.dumps(body)
+    segment = items.pop().payload
+    assert segment["is_segment"] is True
+    assert segment["attributes"]["http.request.body.data"] == json.dumps(body)
 
 
 @pytest.mark.asyncio
 async def test_request_body_not_read_span_streaming(
     sentry_init, aiohttp_client, capture_items
 ):
-    from sentry_sdk.integrations.aiohttp import BODY_NOT_READ_MESSAGE
-
     sentry_init(
         integrations=[AioHttpIntegration()],
         traces_sample_rate=1.0,
@@ -1349,9 +1349,9 @@ async def hello(request):
 
     sentry_sdk.flush()
 
-    segments = [item.payload for item in items if item.payload.get("is_segment")]
-    assert len(segments) == 1
-    assert segments[0]["attributes"]["http.request.body.data"] == BODY_NOT_READ_MESSAGE
+    segment = items.pop().payload
+    assert segment["is_segment"] is True
+    assert segment["attributes"]["http.request.body.data"] == BODY_NOT_READ_MESSAGE
 
 
 @pytest.mark.asyncio
@@ -1381,12 +1381,9 @@ async def hello(request):
 
     sentry_sdk.flush()
 
-    segments = [item.payload for item in items if item.payload.get("is_segment")]
-    assert len(segments) == 1
-    assert (
-        segments[0]["attributes"]["http.request.body.data"]
-        == OVER_SIZE_LIMIT_SUBSTITUTE
-    )
+    segment = items.pop().payload
+    assert segment["is_segment"] is True
+    assert segment["attributes"]["http.request.body.data"] == OVER_SIZE_LIMIT_SUBSTITUTE
 
 
 @pytest.mark.asyncio
@@ -1619,12 +1616,13 @@ async def hello(request):
     assert inner_client_span["attributes"]["http.request.method"] == "GET"
     assert inner_client_span["attributes"]["http.response.status_code"] == 200
     assert inner_client_span["attributes"]["url.query"] == "foo=bar"
+    assert inner_client_span["status"] == "ok"
+
     url_full = inner_client_span["attributes"]["url.full"]
     # parse_url() splits the URL — url.full is the base URL only, with the
     # query string captured separately on url.query.
     assert url_full.startswith("http://127.0.0.1:")
     assert url_full.endswith("/")
-    assert inner_client_span["status"] == "ok"
 
 
 @pytest.mark.asyncio
@@ -1656,6 +1654,7 @@ async def handler(request):
 
     assert client_span["is_segment"] is True
     assert client_span["attributes"]["sentry.op"] == "http.client"
+    assert client_span["name"].startswith("GET http://127.0.0.1:")
     assert resp.request_info.headers[
         "sentry-trace"
     ] == "{trace_id}-{span_id}-{sampled}".format(