From 3d8419c6c3158a9f74ebec16857bd8bfd3ff13b7 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 4 Oct 2024 14:07:39 +0100 Subject: [PATCH 1/9] feat: Add opportunistic Brotli compression Brotli level 4 and 5 offer comparable or better compression to GZip level 9 (which is our default) with better performance. This patch adds opportunistic Brotli compression at level 4 (to be conservative) when it detects the `brotli` module is available. It also provides some escape hatches through `transport_compression_level` and `transport_compression_algo` experiment configs to fine tune the behavior. In the future, we may want to bump the default level from 4 to 5 for better compression. --- requirements-testing.txt | 1 + sentry_sdk/consts.py | 7 ++++ sentry_sdk/transport.py | 76 +++++++++++++++++++++++++++++++--------- tests/test_transport.py | 26 ++++++++++---- 4 files changed, 87 insertions(+), 23 deletions(-) diff --git a/requirements-testing.txt b/requirements-testing.txt index 0f42d6a7df..dfbd821845 100644 --- a/requirements-testing.txt +++ b/requirements-testing.txt @@ -13,3 +13,4 @@ pysocks socksio httpcore[http2] setuptools +Brotli diff --git a/sentry_sdk/consts.py b/sentry_sdk/consts.py index 9a6c08d0fd..631edd8a83 100644 --- a/sentry_sdk/consts.py +++ b/sentry_sdk/consts.py @@ -18,6 +18,11 @@ class EndpointType(Enum): ENVELOPE = "envelope" +class CompressionAlgo(Enum): + GZIP = "gzip" + BROTLI = "br" + + if TYPE_CHECKING: import sentry_sdk @@ -59,6 +64,8 @@ class EndpointType(Enum): "continuous_profiling_mode": Optional[ContinuousProfilerMode], "otel_powered_performance": Optional[bool], "transport_zlib_compression_level": Optional[int], + "transport_compression_level": Optional[int], + "transport_compression_algo": Optional[CompressionAlgo], "transport_num_pools": Optional[int], "transport_http2": Optional[bool], "enable_metrics": Optional[bool], diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index 7a6b4f07b8..1f5677a9a0 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -10,6 +10,11 @@ from collections import defaultdict from urllib.request import getproxies +try: + import brotli +except ImportError: + brotli = None + import urllib3 import certifi @@ -217,13 +222,6 @@ def __init__( ) # type: DefaultDict[Tuple[EventDataCategory, str], int] self._last_client_report_sent = time.time() - compression_level = options.get("_experiments", {}).get( - "transport_zlib_compression_level" - ) - self._compression_level = ( - 9 if compression_level is None else int(compression_level) - ) - self._pool = self._make_pool( self.parsed_dsn, http_proxy=options["http_proxy"], @@ -237,6 +235,35 @@ def __init__( # Backwards compatibility for deprecated `self.hub_class` attribute self._hub_cls = sentry_sdk.Hub + experiments = options.get("_experiments", {}) + compression_level = experiments.get( + "transport_compression_level", + experiments.get("transport_zlib_compression_level"), + ) + compression_algo = experiments.get( + "transport_compression_algo", "br" if brotli is not None else "gzip" + ) + if compression_algo == "br" and brotli is None: + logger.warning( + "You asked for brotli compression without the Brotli module, falling back to gzip -9" + ) + compression_algo = "gzip" + compression_level = None + + self._compression_algo = compression_algo + if compression_level is not None: + self._compression_level = compression_level + elif compression_algo == "gzip": + self._compression_level = 9 + elif compression_algo == "br": + self._compression_level = 4 + else: + logger.warning( + "Unknown compression algo %s, disabling compression", compression_algo + ) + self._compression_level = 0 + self._compression_algo = None + def record_lost_event( self, reason, # type: str @@ -458,14 +485,7 @@ def _send_envelope( if client_report_item is not None: envelope.items.append(client_report_item) - body = io.BytesIO() - if self._compression_level == 0: - envelope.serialize_into(body) - else: - with gzip.GzipFile( - fileobj=body, mode="w", compresslevel=self._compression_level - ) as f: - envelope.serialize_into(f) + content_encoding, body = self._serialize_envelope(envelope) assert self.parsed_dsn is not None logger.debug( @@ -478,8 +498,8 @@ def _send_envelope( headers = { "Content-Type": "application/x-sentry-envelope", } - if self._compression_level > 0: - headers["Content-Encoding"] = "gzip" + if content_encoding: + headers["Content-Encoding"] = content_encoding self._send_request( body.getvalue(), @@ -489,6 +509,28 @@ def _send_envelope( ) return None + def _serialize_envelope(self, envelope): + # type: (..., Envelope) -> tuple[Optional[str], io.BytesIO] + content_encoding = None + body = io.BytesIO() + if self._compression_level == 0 or self._compression_algo is None: + envelope.serialize_into(body) + else: + content_encoding = self._compression_algo + if self._compression_algo == "br" and brotli is not None: + body.write( + brotli.compress( + envelope.serialize(), quality=self._compression_level + ) + ) + else: # assume gzip as we sanitize the algo value in init + with gzip.GzipFile( + fileobj=body, mode="w", compresslevel=self._compression_level + ) as f: + envelope.serialize_into(f) + + return content_encoding, body + def _get_pool_options(self, ca_certs, cert_file=None, key_file=None): # type: (Optional[Any], Optional[Any], Optional[Any]) -> Dict[str, Any] raise NotImplementedError() diff --git a/tests/test_transport.py b/tests/test_transport.py index 8c69a47c54..078550d3ce 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -9,6 +9,7 @@ from datetime import datetime, timedelta, timezone from unittest import mock +import brotli import pytest from pytest_localserver.http import WSGIServer from werkzeug.wrappers import Request, Response @@ -54,9 +55,13 @@ def __call__(self, environ, start_response): """ request = Request(environ) event = envelope = None - if request.headers.get("content-encoding") == "gzip": + content_encoding = request.headers.get("content-encoding") + if content_encoding == "gzip": rdr = gzip.GzipFile(fileobj=io.BytesIO(request.data)) compressed = True + elif content_encoding == "br": + rdr = io.BytesIO(brotli.decompress(request.data)) + compressed = True else: rdr = io.BytesIO(request.data) compressed = False @@ -117,7 +122,8 @@ def mock_transaction_envelope(span_count): @pytest.mark.parametrize("debug", (True, False)) @pytest.mark.parametrize("client_flush_method", ["close", "flush"]) @pytest.mark.parametrize("use_pickle", (True, False)) -@pytest.mark.parametrize("compression_level", (0, 9)) +@pytest.mark.parametrize("compression_level", (0, 9, None)) +@pytest.mark.parametrize("compression_algo", ("gzip", "br", None)) @pytest.mark.parametrize( "http2", [True, False] if sys.version_info >= (3, 8) else [False] ) @@ -131,14 +137,18 @@ def test_transport_works( client_flush_method, use_pickle, compression_level, + compression_algo, http2, maybe_monkeypatched_threading, ): caplog.set_level(logging.DEBUG) - experiments = { - "transport_zlib_compression_level": compression_level, - } + experiments = {} + if compression_level is not None: + experiments["transport_compression_level"] = compression_level + + if compression_algo is not None: + experiments["transport_compression_algo"] = compression_algo if http2: experiments["transport_http2"] = True @@ -164,7 +174,11 @@ def test_transport_works( out, err = capsys.readouterr() assert not err and not out assert capturing_server.captured - assert capturing_server.captured[0].compressed == (compression_level > 0) + assert ( + capturing_server.captured[0].compressed == (compression_level is None) + or (compression_level > 0) + or (compression_algo is None) + ) assert any("Sending envelope" in record.msg for record in caplog.records) == debug From 75d0275c3bcfebdef0944c8e90f7c351576d5c5d Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 4 Oct 2024 17:55:11 +0100 Subject: [PATCH 2/9] fun with python types --- sentry_sdk/transport.py | 123 ++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 67 deletions(-) diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index 1f5677a9a0..216bc3d99d 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -35,6 +35,7 @@ from typing import List from typing import Mapping from typing import Optional + from typing import Self from typing import Tuple from typing import Type from typing import Union @@ -67,20 +68,16 @@ class Transport(ABC): parsed_dsn = None # type: Optional[Dsn] - def __init__( - self, options=None # type: Optional[Dict[str, Any]] - ): - # type: (...) -> None + def __init__(self, options=None): + # type: (Self, Optional[Dict[str, Any]]) -> None self.options = options if options and options["dsn"] is not None and options["dsn"]: self.parsed_dsn = Dsn(options["dsn"]) else: self.parsed_dsn = None - def capture_event( - self, event # type: Event - ): - # type: (...) -> None + def capture_event(self, event): + # type: (Self, Event) -> None """ DEPRECATED: Please use capture_envelope instead. @@ -99,25 +96,23 @@ def capture_event( self.capture_envelope(envelope) @abstractmethod - def capture_envelope( - self, envelope # type: Envelope - ): - # type: (...) -> None + def capture_envelope(self, envelope): + # type: (Self, Envelope) -> None """ Send an envelope to Sentry. Envelopes are a data container format that can hold any type of data submitted to Sentry. We use it to send all event data (including errors, - transactions, crons checkins, etc.) to Sentry. + transactions, crons check-ins, etc.) to Sentry. """ pass def flush( self, - timeout, # type: float - callback=None, # type: Optional[Any] + timeout, + callback=None, ): - # type: (...) -> None + # type: (Self, float, Optional[Any]) -> None """ Wait `timeout` seconds for the current events to be sent out. @@ -127,7 +122,7 @@ def flush( return None def kill(self): - # type: () -> None + # type: (Self) -> None """ Forcefully kills the transport. @@ -162,11 +157,11 @@ def record_lost_event( return None def is_healthy(self): - # type: () -> bool + # type: (Self) -> bool return True def __del__(self): - # type: () -> None + # type: (Self) -> None try: self.kill() except Exception: @@ -174,7 +169,7 @@ def __del__(self): def _parse_rate_limits(header, now=None): - # type: (Any, Optional[datetime]) -> Iterable[Tuple[Optional[EventDataCategory], datetime]] + # type: (str, Optional[datetime]) -> Iterable[Tuple[Optional[EventDataCategory], datetime]] if now is None: now = datetime.now(timezone.utc) @@ -203,10 +198,8 @@ def _parse_rate_limits(header, now=None): class BaseHttpTransport(Transport): """The base HTTP transport.""" - def __init__( - self, options # type: Dict[str, Any] - ): - # type: (...) -> None + def __init__(self, options): + # type: (Self, Dict[str, Any]) -> None from sentry_sdk.consts import VERSION Transport.__init__(self, options) @@ -299,11 +292,11 @@ def record_lost_event( self._discarded_events[data_category, reason] += quantity def _get_header_value(self, response, header): - # type: (Any, str) -> Optional[str] + # type: (Self, Any, str) -> Optional[str] return response.headers.get(header) def _update_rate_limits(self, response): - # type: (Union[urllib3.BaseHTTPResponse, httpcore.Response]) -> None + # type: (Self, Union[urllib3.BaseHTTPResponse, httpcore.Response]) -> None # new sentries with more rate limit insights. We honor this header # no matter of the status code to update our internal rate limits. @@ -329,12 +322,12 @@ def _update_rate_limits(self, response): def _send_request( self, - body, # type: bytes - headers, # type: Dict[str, str] - endpoint_type=EndpointType.ENVELOPE, # type: EndpointType - envelope=None, # type: Optional[Envelope] + body, + headers, + endpoint_type=EndpointType.ENVELOPE, + envelope=None, ): - # type: (...) -> None + # type: (Self, bytes, Dict[str, str], EndpointType, Optional[Envelope]) -> None def record_loss(reason): # type: (str) -> None @@ -384,12 +377,12 @@ def record_loss(reason): finally: response.close() - def on_dropped_event(self, reason): - # type: (str) -> None + def on_dropped_event(self, _reason): + # type: (Self, str) -> None return None def _fetch_pending_client_report(self, force=False, interval=60): - # type: (bool, int) -> Optional[Item] + # type: (Self, bool, int) -> Optional[Item] if not self.options["send_client_reports"]: return None @@ -420,7 +413,7 @@ def _fetch_pending_client_report(self, force=False, interval=60): ) def _flush_client_reports(self, force=False): - # type: (bool) -> None + # type: (Self, bool) -> 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])) @@ -441,23 +434,21 @@ def _disabled(bucket): return _disabled(category) or _disabled(None) def _is_rate_limited(self): - # type: () -> bool + # type: (Self) -> bool return any( ts > datetime.now(timezone.utc) for ts in self._disabled_until.values() ) def _is_worker_full(self): - # type: () -> bool + # type: (Self) -> bool return self._worker.full() def is_healthy(self): - # type: () -> bool + # type: (Self) -> bool return not (self._is_worker_full() or self._is_rate_limited()) - def _send_envelope( - self, envelope # type: Envelope - ): - # type: (...) -> None + def _send_envelope(self, envelope): + # type: (Self, Envelope) -> None # remove all items from the envelope which are over quota new_items = [] @@ -510,7 +501,7 @@ def _send_envelope( return None def _serialize_envelope(self, envelope): - # type: (..., Envelope) -> tuple[Optional[str], io.BytesIO] + # type: (Self, Envelope) -> tuple[Optional[str], io.BytesIO] content_encoding = None body = io.BytesIO() if self._compression_level == 0 or self._compression_algo is None: @@ -532,11 +523,11 @@ def _serialize_envelope(self, envelope): return content_encoding, body def _get_pool_options(self, ca_certs, cert_file=None, key_file=None): - # type: (Optional[Any], Optional[Any], Optional[Any]) -> Dict[str, Any] + # type: (Self, Optional[Any], Optional[Any], Optional[Any]) -> Dict[str, Any] raise NotImplementedError() def _in_no_proxy(self, parsed_dsn): - # type: (Dsn) -> bool + # type: (Self, Dsn) -> bool no_proxy = getproxies().get("no") if not no_proxy: return False @@ -566,7 +557,7 @@ def _request( body, headers, ): - # type: (str, EndpointType, Any, Mapping[str, str]) -> Union[urllib3.BaseHTTPResponse, httpcore.Response] + # type: (Self, str, EndpointType, Any, Mapping[str, str]) -> Union[urllib3.BaseHTTPResponse, httpcore.Response] raise NotImplementedError() def capture_envelope( @@ -586,10 +577,10 @@ def send_envelope_wrapper(): def flush( self, - timeout, # type: float - callback=None, # type: Optional[Any] + timeout, + callback=None, ): - # type: (...) -> None + # type: (Self, float, Optional[Callable]) -> None logger.debug("Flushing HTTP transport") if timeout > 0: @@ -597,7 +588,7 @@ def flush( self._worker.flush(timeout, callback) def kill(self): - # type: () -> None + # type: (Self) -> None logger.debug("Killing HTTP transport") self._worker.kill() @@ -613,14 +604,14 @@ def _warn_hub_cls(): @property def hub_cls(self): - # type: () -> type[sentry_sdk.Hub] + # type: (Self) -> type[sentry_sdk.Hub] """DEPRECATED: This attribute is deprecated and will be removed in a future release.""" HttpTransport._warn_hub_cls() return self._hub_cls @hub_cls.setter def hub_cls(self, value): - # type: (type[sentry_sdk.Hub]) -> None + # type: (Self, type[sentry_sdk.Hub]) -> None """DEPRECATED: This attribute is deprecated and will be removed in a future release.""" HttpTransport._warn_hub_cls() self._hub_cls = value @@ -631,7 +622,7 @@ class HttpTransport(BaseHttpTransport): _pool: Union[PoolManager, ProxyManager] def _get_pool_options(self, ca_certs, cert_file=None, key_file=None): - # type: (Optional[Any], Optional[Any], Optional[Any]) -> Dict[str, Any] + # type: (Self, Any, Any, Any) -> Dict[str, Any] num_pools = self.options.get("_experiments", {}).get("transport_num_pools") options = { @@ -673,9 +664,9 @@ def _make_pool( parsed_dsn, # type: Dsn http_proxy, # type: Optional[str] https_proxy, # type: Optional[str] - ca_certs, # type: Optional[Any] - cert_file, # type: Optional[Any] - key_file, # type: Optional[Any] + ca_certs, # type: Any + cert_file, # type: Any + key_file, # type: Any proxy_headers, # type: Optional[Dict[str, str]] ): # type: (...) -> Union[PoolManager, ProxyManager] @@ -724,7 +715,7 @@ def _request( body, headers, ): - # type: (str, EndpointType, Any, Mapping[str, str]) -> urllib3.BaseHTTPResponse + # type: (Self, str, EndpointType, Any, Mapping[str, str]) -> urllib3.BaseHTTPResponse return self._pool.request( method, self._auth.get_api_url(endpoint_type), @@ -738,10 +729,8 @@ def _request( except ImportError: # Sorry, no Http2Transport for you class Http2Transport(HttpTransport): - def __init__( - self, options # type: Dict[str, Any] - ): - # type: (...) -> None + def __init__(self, options): + # type: (Self, Dict[str, Any]) -> None super().__init__(options) logger.warning( "You tried to use HTTP2Transport but don't have httpcore[http2] installed. Falling back to HTTPTransport." @@ -758,7 +747,7 @@ class Http2Transport(BaseHttpTransport): # type: ignore ] def _get_header_value(self, response, header): - # type: (httpcore.Response, str) -> Optional[str] + # type: (Self, httpcore.Response, str) -> Optional[str] return next( ( val.decode("ascii") @@ -775,7 +764,7 @@ def _request( body, headers, ): - # type: (str, EndpointType, Any, Mapping[str, str]) -> httpcore.Response + # type: (Self, str, EndpointType, Any, Mapping[str, str]) -> httpcore.Response response = self._pool.request( method, self._auth.get_api_url(endpoint_type), @@ -785,7 +774,7 @@ def _request( return response def _get_pool_options(self, ca_certs, cert_file=None, key_file=None): - # type: (Optional[Any], Optional[Any], Optional[Any]) -> Dict[str, Any] + # type: (Any, Any, Any) -> Dict[str, Any] options = { "http2": True, "retries": 3, @@ -825,9 +814,9 @@ def _make_pool( parsed_dsn, # type: Dsn http_proxy, # type: Optional[str] https_proxy, # type: Optional[str] - ca_certs, # type: Optional[Any] - cert_file, # type: Optional[Any] - key_file, # type: Optional[Any] + ca_certs, # type: Any + cert_file, # type: Any + key_file, # type: Any proxy_headers, # type: Optional[Dict[str, str]] ): # type: (...) -> Union[httpcore.SOCKSProxy, httpcore.HTTPProxy, httpcore.ConnectionPool] From 34ef56247cb32b72893cff3e1f2f774785ac67e0 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 4 Oct 2024 17:56:35 +0100 Subject: [PATCH 3/9] fix asyncio tests --- tests/integrations/aiohttp/test_aiohttp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integrations/aiohttp/test_aiohttp.py b/tests/integrations/aiohttp/test_aiohttp.py index 5b25629a83..a39e316fc2 100644 --- a/tests/integrations/aiohttp/test_aiohttp.py +++ b/tests/integrations/aiohttp/test_aiohttp.py @@ -55,7 +55,7 @@ async def hello(request): assert request["url"] == "http://{host}/".format(host=host) assert request["headers"] == { "Accept": "*/*", - "Accept-Encoding": "gzip, deflate", + "Accept-Encoding": "gzip, deflate, br", "Host": host, "User-Agent": request["headers"]["User-Agent"], "baggage": mock.ANY, From 03b2c2d6acb067dc59915268bb64170f1e37ea95 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 4 Oct 2024 21:18:00 +0100 Subject: [PATCH 4/9] moar type fun --- sentry_sdk/transport.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index 216bc3d99d..bdd73bd9a6 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -11,7 +11,7 @@ from urllib.request import getproxies try: - import brotli + import brotli # type: ignore except ImportError: brotli = None @@ -24,7 +24,7 @@ from sentry_sdk.worker import BackgroundWorker from sentry_sdk.envelope import Envelope, Item, PayloadRef -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, cast if TYPE_CHECKING: from typing import Any @@ -176,10 +176,11 @@ def _parse_rate_limits(header, now=None): for limit in header.split(","): try: parameters = limit.strip().split(":") - retry_after, categories = parameters[:2] + retry_after_val, categories = parameters[:2] - retry_after = now + timedelta(seconds=int(retry_after)) + retry_after = now + timedelta(seconds=int(retry_after_val)) for category in categories and categories.split(";") or (None,): + category = cast(EventDataCategory, category) if category == "metric_bucket": try: namespaces = parameters[4].split(";") @@ -580,7 +581,7 @@ def flush( timeout, callback=None, ): - # type: (Self, float, Optional[Callable]) -> None + # type: (Self, float, Optional[Callable[[int, float], None]]) -> None logger.debug("Flushing HTTP transport") if timeout > 0: From 1c7233b08ffbed363956c8f0ee45f5f2d79dd55e Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 4 Oct 2024 21:33:06 +0100 Subject: [PATCH 5/9] fix aiohttp tests, again --- tests/integrations/aiohttp/test_aiohttp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integrations/aiohttp/test_aiohttp.py b/tests/integrations/aiohttp/test_aiohttp.py index a39e316fc2..cd65e7cdd5 100644 --- a/tests/integrations/aiohttp/test_aiohttp.py +++ b/tests/integrations/aiohttp/test_aiohttp.py @@ -55,7 +55,7 @@ async def hello(request): assert request["url"] == "http://{host}/".format(host=host) assert request["headers"] == { "Accept": "*/*", - "Accept-Encoding": "gzip, deflate, br", + "Accept-Encoding": mock.ANY, "Host": host, "User-Agent": request["headers"]["User-Agent"], "baggage": mock.ANY, From 5d17830276710cbb167b2a33595e64a7606ba56b Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 4 Oct 2024 21:47:38 +0100 Subject: [PATCH 6/9] friggin types and casting --- sentry_sdk/transport.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index bdd73bd9a6..de2baf21da 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -19,6 +19,7 @@ import certifi import sentry_sdk +from sentry_sdk._types import EventDataCategory from sentry_sdk.consts import EndpointType from sentry_sdk.utils import Dsn, logger, capture_internal_exceptions from sentry_sdk.worker import BackgroundWorker @@ -43,7 +44,7 @@ from urllib3.poolmanager import PoolManager from urllib3.poolmanager import ProxyManager - from sentry_sdk._types import Event, EventDataCategory + from sentry_sdk._types import Event KEEP_ALIVE_SOCKET_OPTIONS = [] for option in [ From bec70b95f9c0b7c52441f7c14369db468e93cb69 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Fri, 4 Oct 2024 21:51:39 +0100 Subject: [PATCH 7/9] i give up --- sentry_sdk/transport.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index de2baf21da..879c25d923 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -19,13 +19,12 @@ import certifi import sentry_sdk -from sentry_sdk._types import EventDataCategory from sentry_sdk.consts import EndpointType from sentry_sdk.utils import Dsn, logger, capture_internal_exceptions from sentry_sdk.worker import BackgroundWorker from sentry_sdk.envelope import Envelope, Item, PayloadRef -from typing import TYPE_CHECKING, cast +from typing import TYPE_CHECKING if TYPE_CHECKING: from typing import Any @@ -44,7 +43,7 @@ from urllib3.poolmanager import PoolManager from urllib3.poolmanager import ProxyManager - from sentry_sdk._types import Event + from sentry_sdk._types import Event, EventDataCategory KEEP_ALIVE_SOCKET_OPTIONS = [] for option in [ @@ -181,7 +180,6 @@ def _parse_rate_limits(header, now=None): retry_after = now + timedelta(seconds=int(retry_after_val)) for category in categories and categories.split(";") or (None,): - category = cast(EventDataCategory, category) if category == "metric_bucket": try: namespaces = parameters[4].split(";") @@ -189,10 +187,10 @@ def _parse_rate_limits(header, now=None): namespaces = [] if not namespaces or "custom" in namespaces: - yield category, retry_after + yield category, retry_after # type: ignore else: - yield category, retry_after + yield category, retry_after # type: ignore except (LookupError, ValueError): continue From ba678fde7f176f95e7187fc2803da509350efc69 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Mon, 7 Oct 2024 14:47:31 +0100 Subject: [PATCH 8/9] clarify test condition a bit --- tests/test_transport.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/test_transport.py b/tests/test_transport.py index 078550d3ce..98948218ec 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -174,11 +174,18 @@ def test_transport_works( out, err = capsys.readouterr() assert not err and not out assert capturing_server.captured - assert ( - capturing_server.captured[0].compressed == (compression_level is None) - or (compression_level > 0) - or (compression_algo is None) + should_compress = ( + ( + compression_level is None + ) # default is to compress with bortli if available, gzip otherwise + or ( + compression_level > 0 + ) # setting compression level to 0 means don't compress + or ( + compression_algo is None + ) # if we couldn't resolve to a known algo, we don't compress ) + assert capturing_server.captured[0].compressed == should_compress assert any("Sending envelope" in record.msg for record in caplog.records) == debug From 9ca69c2463625b39327ba72198a8efbbd1c180dc Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Mon, 7 Oct 2024 16:55:02 +0100 Subject: [PATCH 9/9] fix test case and invalid algo fallback logic --- sentry_sdk/transport.py | 28 +++++++++++++++++++--------- tests/test_transport.py | 21 ++++++++++++--------- 2 files changed, 31 insertions(+), 18 deletions(-) diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py index 879c25d923..a43ecabfb6 100644 --- a/sentry_sdk/transport.py +++ b/sentry_sdk/transport.py @@ -234,8 +234,16 @@ def __init__(self, options): experiments.get("transport_zlib_compression_level"), ) compression_algo = experiments.get( - "transport_compression_algo", "br" if brotli is not None else "gzip" + "transport_compression_algo", + ( + "gzip" + # if only compression level is set, assume gzip for backwards compatibility + # if we don't have brotli available, fallback to gzip + if compression_level is not None or brotli is None + else "br" + ), ) + if compression_algo == "br" and brotli is None: logger.warning( "You asked for brotli compression without the Brotli module, falling back to gzip -9" @@ -243,19 +251,21 @@ def __init__(self, options): compression_algo = "gzip" compression_level = None - self._compression_algo = compression_algo - if compression_level is not None: - self._compression_level = compression_level - elif compression_algo == "gzip": - self._compression_level = 9 - elif compression_algo == "br": - self._compression_level = 4 - else: + if compression_algo not in ("br", "gzip"): logger.warning( "Unknown compression algo %s, disabling compression", compression_algo ) self._compression_level = 0 self._compression_algo = None + else: + self._compression_algo = compression_algo + + if compression_level is not None: + self._compression_level = compression_level + elif self._compression_algo == "gzip": + self._compression_level = 9 + elif self._compression_algo == "br": + self._compression_level = 4 def record_lost_event( self, diff --git a/tests/test_transport.py b/tests/test_transport.py index 98948218ec..1c7bc8aac2 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -123,7 +123,7 @@ def mock_transaction_envelope(span_count): @pytest.mark.parametrize("client_flush_method", ["close", "flush"]) @pytest.mark.parametrize("use_pickle", (True, False)) @pytest.mark.parametrize("compression_level", (0, 9, None)) -@pytest.mark.parametrize("compression_algo", ("gzip", "br", None)) +@pytest.mark.parametrize("compression_algo", ("gzip", "br", "", None)) @pytest.mark.parametrize( "http2", [True, False] if sys.version_info >= (3, 8) else [False] ) @@ -175,16 +175,19 @@ def test_transport_works( assert not err and not out assert capturing_server.captured should_compress = ( - ( - compression_level is None - ) # default is to compress with bortli if available, gzip otherwise - or ( - compression_level > 0 - ) # setting compression level to 0 means don't compress + # default is to compress with brotli if available, gzip otherwise + (compression_level is None) or ( - compression_algo is None - ) # if we couldn't resolve to a known algo, we don't compress + # setting compression level to 0 means don't compress + compression_level + > 0 + ) + ) and ( + # if we couldn't resolve to a known algo, we don't compress + compression_algo + != "" ) + assert capturing_server.captured[0].compressed == should_compress assert any("Sending envelope" in record.msg for record in caplog.records) == debug