From cfed85d313fd804fd0b47761bd5aece9199e04a6 Mon Sep 17 00:00:00 2001 From: srothh Date: Fri, 11 Jul 2025 10:49:35 +0200 Subject: [PATCH 01/10] ref(transport): Added shared sync/async transport superclass and created a sync transport HTTP subclass Moved shared sync/async logic into a new superclass (HttpTransportCore), and moved sync transport specific code into a new subclass(BaseSyncHttpTransport), from which the current transport implementations inherit Fixes GH-4568 --- sentry_sdk/client.py | 4 +- sentry_sdk/transport.py | 151 ++++++++++++++++++++++++---------------- 2 files changed, 92 insertions(+), 63 deletions(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index 98553d8993..917701f39c 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -25,7 +25,7 @@ ) from sentry_sdk.serializer import serialize from sentry_sdk.tracing import trace -from sentry_sdk.transport import BaseHttpTransport, make_transport +from sentry_sdk.transport import HttpTransportCore, make_transport from sentry_sdk.consts import ( SPANDATA, DEFAULT_MAX_VALUE_LENGTH, @@ -406,7 +406,7 @@ def _capture_envelope(envelope: Envelope) -> None: self.monitor or self.log_batcher or has_profiling_enabled(self.options) - or isinstance(self.transport, BaseHttpTransport) + or isinstance(self.transport, HttpTransportCore) ): # If we have anything on that could spawn a background thread, we # need to check if it's safe to use them. diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index ac7a8c3522..3a0d4ec991 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -162,7 +162,7 @@ def _parse_rate_limits( continue -class BaseHttpTransport(Transport): +class HttpTransportCore(Transport): """The base HTTP transport.""" TIMEOUT = 30 # seconds @@ -286,12 +286,8 @@ def _update_rate_limits( seconds=retry_after ) - def _send_request( - self: Self, - body: bytes, - headers: Dict[str, str], - endpoint_type: EndpointType = EndpointType.ENVELOPE, - envelope: Optional[Envelope] = None, + def _handle_request_error( + self: Self, envelope: Optional[Envelope], loss_reason: str = "network" ) -> None: def record_loss(reason: str) -> None: if envelope is None: @@ -300,45 +296,45 @@ def record_loss(reason: str) -> None: for item in envelope.items: self.record_lost_event(reason, item=item) + self.on_dropped_event(loss_reason) + record_loss("network_error") + + def _handle_response( + self: Self, + response: Union[urllib3.BaseHTTPResponse, httpcore.Response], + envelope: Optional[Envelope], + ) -> None: + self._update_rate_limits(response) + + if response.status == 429: + # if we hit a 429. Something was rate limited but we already + # acted on this in `self._update_rate_limits`. Note that we + # do not want to record event loss here as we will have recorded + # an outcome in relay already. + self.on_dropped_event("status_429") + pass + + elif response.status >= 300 or response.status < 200: + logger.error( + "Unexpected status code: %s (body: %s)", + response.status, + getattr(response, "data", getattr(response, "content", None)), + ) + self._handle_request_error( + envelope=envelope, loss_reason="status_{}".format(response.status) + ) + + def _update_headers( + self: Self, + headers: Dict[str, str], + ) -> None: + headers.update( { "User-Agent": str(self._auth.client), "X-Sentry-Auth": str(self._auth.to_header()), } ) - try: - response = self._request( - "POST", - endpoint_type, - body, - headers, - ) - except Exception: - self.on_dropped_event("network") - record_loss("network_error") - raise - - try: - self._update_rate_limits(response) - - if response.status == 429: - # if we hit a 429. Something was rate limited but we already - # acted on this in `self._update_rate_limits`. Note that we - # do not want to record event loss here as we will have recorded - # an outcome in relay already. - self.on_dropped_event("status_429") - pass - - elif response.status >= 300 or response.status < 200: - logger.error( - "Unexpected status code: %s (body: %s)", - response.status, - getattr(response, "data", getattr(response, "content", None)), - ) - self.on_dropped_event("status_{}".format(response.status)) - record_loss("network_error") - finally: - response.close() def on_dropped_event(self: Self, _reason: str) -> None: return None @@ -375,11 +371,6 @@ def _fetch_pending_client_report( type="client_report", ) - def _flush_client_reports(self: Self, force: bool = False) -> None: - client_report = self._fetch_pending_client_report(force=force, interval=60) - if client_report is not None: - self.capture_envelope(Envelope(items=[client_report])) - def _check_disabled(self: Self, category: EventDataCategory) -> bool: def _disabled(bucket: Optional[EventDataCategory]) -> bool: ts = self._disabled_until.get(bucket) @@ -398,9 +389,9 @@ def _is_worker_full(self: Self) -> bool: def is_healthy(self: Self) -> bool: return not (self._is_worker_full() or self._is_rate_limited()) - def _send_envelope(self: Self, envelope: Envelope) -> None: - - # remove all items from the envelope which are over quota + def _prepare_envelope( + self: Self, envelope: Envelope + ) -> Optional[Tuple[Envelope, io.BytesIO, Dict[str, str]]]: new_items = [] for item in envelope.items: if self._check_disabled(item.data_category): @@ -442,13 +433,7 @@ def _send_envelope(self: Self, envelope: Envelope) -> None: if content_encoding: headers["Content-Encoding"] = content_encoding - self._send_request( - body.getvalue(), - headers=headers, - endpoint_type=EndpointType.ENVELOPE, - envelope=envelope, - ) - return None + return envelope, body, headers def _serialize_envelope( self: Self, envelope: Envelope @@ -506,6 +491,54 @@ def _request( ) -> Union[urllib3.BaseHTTPResponse, httpcore.Response]: raise NotImplementedError() + def kill(self: Self) -> None: + logger.debug("Killing HTTP transport") + self._worker.kill() + + +class BaseSyncHttpTransport(HttpTransportCore): + + def _send_envelope(self: Self, envelope: Envelope) -> None: + _prepared_envelope = self._prepare_envelope(envelope) + if _prepared_envelope is None: # TODO: check this behaviour in detail + return None + envelope, body, headers = _prepared_envelope + self._send_request( + body.getvalue(), + headers=headers, + endpoint_type=EndpointType.ENVELOPE, + envelope=envelope, + ) + return None + + def _send_request( + self: Self, + body: bytes, + headers: Dict[str, str], + endpoint_type: EndpointType, + envelope: Optional[Envelope], + ) -> None: + self._update_headers(headers) + try: + response = self._request( + "POST", + endpoint_type, + body, + headers, + ) + except Exception: + self._handle_request_error(envelope=envelope, loss_reason="network") + raise + try: + self._handle_response(response=response, envelope=envelope) + finally: + response.close() + + def _flush_client_reports(self: Self, force: bool = False) -> None: + client_report = self._fetch_pending_client_report(force=force, interval=60) + if client_report is not None: + self.capture_envelope(Envelope(items=[client_report])) + def capture_envelope(self: Self, envelope: Envelope) -> None: def send_envelope_wrapper() -> None: with capture_internal_exceptions(): @@ -528,12 +561,8 @@ def flush( self._worker.submit(lambda: self._flush_client_reports(force=True)) self._worker.flush(timeout, callback) - def kill(self: Self) -> None: - logger.debug("Killing HTTP transport") - self._worker.kill() - -class HttpTransport(BaseHttpTransport): +class HttpTransport(BaseSyncHttpTransport): if TYPE_CHECKING: _pool: Union[PoolManager, ProxyManager] @@ -650,7 +679,7 @@ def __init__(self: Self, options: Dict[str, Any]) -> None: else: - class Http2Transport(BaseHttpTransport): # type: ignore + class Http2Transport(BaseSyncHttpTransport): # type: ignore """The HTTP2 transport based on httpcore.""" TIMEOUT = 15 From 3fbb6da7ba7e1a967b8c7b32e4b68cbcdbd22cd0 Mon Sep 17 00:00:00 2001 From: srothh Date: Fri, 11 Jul 2025 11:30:29 +0200 Subject: [PATCH 02/10] ref(transport) Removed Todo and reverted class name change Removed an unnecessary TODO message and reverted a class name change for BaseHTTPTransport. GH-4568 --- sentry_sdk/client.py | 4 ++-- sentry_sdk/transport.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index 917701f39c..98553d8993 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -25,7 +25,7 @@ ) from sentry_sdk.serializer import serialize from sentry_sdk.tracing import trace -from sentry_sdk.transport import HttpTransportCore, make_transport +from sentry_sdk.transport import BaseHttpTransport, make_transport from sentry_sdk.consts import ( SPANDATA, DEFAULT_MAX_VALUE_LENGTH, @@ -406,7 +406,7 @@ def _capture_envelope(envelope: Envelope) -> None: self.monitor or self.log_batcher or has_profiling_enabled(self.options) - or isinstance(self.transport, HttpTransportCore) + or isinstance(self.transport, BaseHttpTransport) ): # If we have anything on that could spawn a background thread, we # need to check if it's safe to use them. diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index 3a0d4ec991..89651f355b 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -162,7 +162,7 @@ def _parse_rate_limits( continue -class HttpTransportCore(Transport): +class BaseHttpTransport(Transport): """The base HTTP transport.""" TIMEOUT = 30 # seconds @@ -496,11 +496,11 @@ def kill(self: Self) -> None: self._worker.kill() -class BaseSyncHttpTransport(HttpTransportCore): +class BaseSyncHttpTransport(BaseHttpTransport): def _send_envelope(self: Self, envelope: Envelope) -> None: _prepared_envelope = self._prepare_envelope(envelope) - if _prepared_envelope is None: # TODO: check this behaviour in detail + if _prepared_envelope is None: return None envelope, body, headers = _prepared_envelope self._send_request( From 2f2cfdff85b3673a2532c864fbe01c0470b87f0f Mon Sep 17 00:00:00 2001 From: srothh Date: Fri, 11 Jul 2025 12:34:28 +0200 Subject: [PATCH 03/10] test(transport): Add test for HTTP error status handling Adds test coverage for the error handling path when HTTP requests return error status codes. GH-4568 --- tests/test_transport.py | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/tests/test_transport.py b/tests/test_transport.py index 7e0cc6383c..76db2b8e82 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -619,9 +619,9 @@ def test_record_lost_event_transaction_item(capturing_server, make_client, span_ transport.record_lost_event(reason="test", item=transaction_item) client.flush() - (captured,) = capturing_server.captured # Should only be one envelope + (captured,) = capturing_server.captured envelope = captured.envelope - (item,) = envelope.items # Envelope should only have one item + (item,) = envelope.items assert item.type == "client_report" @@ -641,3 +641,38 @@ def test_record_lost_event_transaction_item(capturing_server, make_client, span_ "reason": "test", "quantity": span_count + 1, } in discarded_events + + +def test_handle_unexpected_status_invokes_handle_request_error( + make_client, monkeypatch +): + client = make_client() + transport = client.transport + + monkeypatch.setattr(transport._worker, "submit", lambda fn: fn() or True) + + def stub_request(method, endpoint, body=None, headers=None): + class MockResponse: + def __init__(self): + self.status = 500 # Integer + self.data = b"server error" + self.headers = {} + + def close(self): + pass + + return MockResponse() + + monkeypatch.setattr(transport, "_request", stub_request) + + seen = [] + monkeypatch.setattr( + transport, + "_handle_request_error", + lambda envelope, loss_reason: seen.append(loss_reason), + ) + + client.capture_event({"message": "test"}) + client.flush() + + assert seen == ["status_500"] From 356e905d315f5679105393eaf2a79f455c29aa16 Mon Sep 17 00:00:00 2001 From: srothh Date: Fri, 11 Jul 2025 14:48:30 +0200 Subject: [PATCH 04/10] test(transport): Restore accidentally removed comments Restore comments accidentally removed during a previous commit. --- tests/test_transport.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_transport.py b/tests/test_transport.py index 76db2b8e82..bd87728962 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -619,9 +619,9 @@ def test_record_lost_event_transaction_item(capturing_server, make_client, span_ transport.record_lost_event(reason="test", item=transaction_item) client.flush() - (captured,) = capturing_server.captured + (captured,) = capturing_server.captured # Should only be one envelope envelope = captured.envelope - (item,) = envelope.items + (item,) = envelope.items # Envelope should only have one item assert item.type == "client_report" From 781f7d22e859fce3bf12646e17b74fa8af9dbcb3 Mon Sep 17 00:00:00 2001 From: srothh Date: Mon, 14 Jul 2025 09:27:57 +0200 Subject: [PATCH 05/10] ref(transport) Refactor class names to reflect previous functionality Refactored class names such that BaseHttpTransport now has the same functionality as before the hierarchy refactor GH-4568 --- sentry_sdk/client.py | 4 ++-- sentry_sdk/transport.py | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index 98553d8993..917701f39c 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -25,7 +25,7 @@ ) from sentry_sdk.serializer import serialize from sentry_sdk.tracing import trace -from sentry_sdk.transport import BaseHttpTransport, make_transport +from sentry_sdk.transport import HttpTransportCore, make_transport from sentry_sdk.consts import ( SPANDATA, DEFAULT_MAX_VALUE_LENGTH, @@ -406,7 +406,7 @@ def _capture_envelope(envelope: Envelope) -> None: self.monitor or self.log_batcher or has_profiling_enabled(self.options) - or isinstance(self.transport, BaseHttpTransport) + or isinstance(self.transport, HttpTransportCore) ): # If we have anything on that could spawn a background thread, we # need to check if it's safe to use them. diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index 89651f355b..e0e9694b90 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -162,7 +162,7 @@ def _parse_rate_limits( continue -class BaseHttpTransport(Transport): +class HttpTransportCore(Transport): """The base HTTP transport.""" TIMEOUT = 30 # seconds @@ -496,7 +496,7 @@ def kill(self: Self) -> None: self._worker.kill() -class BaseSyncHttpTransport(BaseHttpTransport): +class BaseHttpTransport(HttpTransportCore): def _send_envelope(self: Self, envelope: Envelope) -> None: _prepared_envelope = self._prepare_envelope(envelope) @@ -562,7 +562,7 @@ def flush( self._worker.flush(timeout, callback) -class HttpTransport(BaseSyncHttpTransport): +class HttpTransport(BaseHttpTransport): if TYPE_CHECKING: _pool: Union[PoolManager, ProxyManager] @@ -679,7 +679,7 @@ def __init__(self: Self, options: Dict[str, Any]) -> None: else: - class Http2Transport(BaseSyncHttpTransport): # type: ignore + class Http2Transport(BaseHttpTransport): # type: ignore """The HTTP2 transport based on httpcore.""" TIMEOUT = 15 From 2135cd6fcc0ab9e007b88af546f53dc0c002719f Mon Sep 17 00:00:00 2001 From: srothh Date: Thu, 17 Jul 2025 11:48:11 +0200 Subject: [PATCH 06/10] ref(transport): Add flush_async in the Transport abc Add a new flush_async method in the Transport ABC. This is needed for the async transport, as calling it from the client while preserving execution order in close will require flush to be a coroutine, not a function. GH-4568 --- sentry_sdk/transport.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index e0e9694b90..60c6ae794a 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -107,6 +107,19 @@ def flush( """ return None + async def flush_async( + self: Self, + timeout: float, + callback: Optional[Any] = None, + ) -> None: + """ + Send out current events within `timeout` seconds. This method needs to be awaited for blocking behavior. + + The default implementation is a no-op, since this method may only be relevant to some transports. + Subclasses should override this method if necessary. + """ + return None + def kill(self: Self) -> None: """ Forcefully kills the transport. From 7916ab24e18fefa0bdf1aa59c0ed21bd32cfc276 Mon Sep 17 00:00:00 2001 From: srothh Date: Thu, 17 Jul 2025 12:33:26 +0200 Subject: [PATCH 07/10] ref(transport): Move flush_async from ABC Move flush_async down to the specific async transport subclass. This makes more sense anyway, as this will only be required by the async transport. If more async transports are expected, another shared superclass can be created. GH-4568 --- sentry_sdk/transport.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index 60c6ae794a..e0e9694b90 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -107,19 +107,6 @@ def flush( """ return None - async def flush_async( - self: Self, - timeout: float, - callback: Optional[Any] = None, - ) -> None: - """ - Send out current events within `timeout` seconds. This method needs to be awaited for blocking behavior. - - The default implementation is a no-op, since this method may only be relevant to some transports. - Subclasses should override this method if necessary. - """ - return None - def kill(self: Self) -> None: """ Forcefully kills the transport. From cb3c2a9e276c285d7f93f09fbca4f91ae9730f50 Mon Sep 17 00:00:00 2001 From: srothh Date: Wed, 23 Jul 2025 15:51:58 +0200 Subject: [PATCH 08/10] ref(transport): add async type annotations to HTTPTransportCore Add necessary type annotations to the core HttpTransport to accomodate for async transport. GH-4568 --- sentry_sdk/transport.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index e0e9694b90..c9aa3e15ea 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -479,6 +479,9 @@ def _make_pool( httpcore.SOCKSProxy, httpcore.HTTPProxy, httpcore.ConnectionPool, + httpcore.AsyncSOCKSProxy, + httpcore.AsyncHTTPProxy, + httpcore.AsyncConnectionPool, ]: raise NotImplementedError() From 5fd43178c5700bc21c3a73cff2578ac7786d4df3 Mon Sep 17 00:00:00 2001 From: srothh Date: Thu, 24 Jul 2025 14:21:17 +0200 Subject: [PATCH 09/10] ref(transport): Refactor transport class to restore former comments and cleaner return paths GH-4568 --- sentry_sdk/transport.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index c9aa3e15ea..d612028250 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -163,7 +163,7 @@ def _parse_rate_limits( class HttpTransportCore(Transport): - """The base HTTP transport.""" + """Shared base class for sync and async transports.""" TIMEOUT = 30 # seconds @@ -392,6 +392,8 @@ def is_healthy(self: Self) -> bool: def _prepare_envelope( self: Self, envelope: Envelope ) -> Optional[Tuple[Envelope, io.BytesIO, Dict[str, str]]]: + + # remove all items from the envelope which are over quota new_items = [] for item in envelope.items: if self._check_disabled(item.data_category): @@ -500,18 +502,18 @@ def kill(self: Self) -> None: class BaseHttpTransport(HttpTransportCore): + """The base HTTP transport.""" def _send_envelope(self: Self, envelope: Envelope) -> None: _prepared_envelope = self._prepare_envelope(envelope) - if _prepared_envelope is None: - return None - envelope, body, headers = _prepared_envelope - self._send_request( - body.getvalue(), - headers=headers, - endpoint_type=EndpointType.ENVELOPE, - envelope=envelope, - ) + if _prepared_envelope is not None: + envelope, body, headers = _prepared_envelope + self._send_request( + body.getvalue(), + headers=headers, + endpoint_type=EndpointType.ENVELOPE, + envelope=envelope, + ) return None def _send_request( From 4e56e5c2348d7235f40002e82f58ed948f8a3fbe Mon Sep 17 00:00:00 2001 From: srothh Date: Thu, 24 Jul 2025 14:28:44 +0200 Subject: [PATCH 10/10] ref(transport): Add test for record_loss GH-4568 --- tests/test_transport.py | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/test_transport.py b/tests/test_transport.py index bd87728962..e612bfcaa5 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -676,3 +676,44 @@ def close(self): client.flush() assert seen == ["status_500"] + + +def test_handle_request_error_basic_coverage(make_client, monkeypatch): + client = make_client() + transport = client.transport + + monkeypatch.setattr(transport._worker, "submit", lambda fn: fn() or True) + + # Track method calls + calls = [] + + def mock_on_dropped_event(reason): + calls.append(("on_dropped_event", reason)) + + def mock_record_lost_event(reason, data_category=None, item=None): + calls.append(("record_lost_event", reason, data_category, item)) + + monkeypatch.setattr(transport, "on_dropped_event", mock_on_dropped_event) + monkeypatch.setattr(transport, "record_lost_event", mock_record_lost_event) + + # Test case 1: envelope is None + transport._handle_request_error(envelope=None, loss_reason="test_reason") + + assert len(calls) == 2 + assert calls[0] == ("on_dropped_event", "test_reason") + assert calls[1] == ("record_lost_event", "network_error", "error", None) + + # Reset + calls.clear() + + # Test case 2: envelope with items + envelope = Envelope() + envelope.add_item(mock.MagicMock()) # Simple mock item + envelope.add_item(mock.MagicMock()) # Another mock item + + transport._handle_request_error(envelope=envelope, loss_reason="connection_error") + + assert len(calls) == 3 + assert calls[0] == ("on_dropped_event", "connection_error") + assert calls[1][0:2] == ("record_lost_event", "network_error") + assert calls[2][0:2] == ("record_lost_event", "network_error")