From 08aa510b56e513bf251ffb68042131fb3969cafb Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 12 Apr 2022 08:05:26 +0200 Subject: [PATCH 1/3] Change ordering of event drop mechanisms As requested by @mitsuhiko this PR shall serve as basis for discussing the ordering of event drop mechanisms and its implications. We are planning for `sample_rate` to update the session counts despite dropping an event (see https://github.com/getsentry/develop/pull/551 and https://github.com/getsentry/develop/issues/537). Without changing the order of filtering mechanisms this would mean any event dropped by `sample_rate` would update the session even if it would be dropped by `ignore_errors` which should not update the session counts when dropping an event. By changing the order we would first drop `ignored_errors` and only then check `sample_rate`, so session counts would not be affected in the case mentioned before. The same reasoning could probably be applied to `event_processor` and `before_send` but we don't know why a developer decided to drop an event there. Was it because they don't care about the event (then session should not be updated) or to save quota (session should be updated)? Also these may be more expensive in terms of performance (developers can provide their own implementations for both of those on some SDKs). So moving them before `sample_rate` would execute `before_send` and `event_processor` for every event instead of only doing it for the sampled events. --- sentry_sdk/client.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index efc8799c00..5623a51e75 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -253,6 +253,9 @@ def _should_capture( if scope is not None and not scope._should_capture: return False + if self._is_ignored_error(event, hint): + return False + if ( self.options["sample_rate"] < 1.0 and random.random() >= self.options["sample_rate"] @@ -262,9 +265,6 @@ def _should_capture( self.transport.record_lost_event("sample_rate", data_category="error") return False - if self._is_ignored_error(event, hint): - return False - return True def _update_session_from_event( From 9289474a61f714d6c0f315f51f583b96d1491e87 Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 12 Apr 2022 11:01:35 +0200 Subject: [PATCH 2/3] ref(sampling): Made code more readable --- sentry_sdk/client.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index 5623a51e75..9cdc9f5b9e 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -246,23 +246,28 @@ def _should_capture( scope=None, # type: Optional[Scope] ): # type: (...) -> bool - if event.get("type") == "transaction": - # Transactions are sampled independent of error events. + # Transactions are sampled independent of error events. + is_transaction = event.get("type") == "transaction" + if is_transaction: return True - if scope is not None and not scope._should_capture: + ignoring_prevents_recursion = scope is not None and not scope._should_capture + if ignoring_prevents_recursion: return False - if self._is_ignored_error(event, hint): + ignored_by_config_option = self._is_ignored_error(event, hint) + if ignored_by_config_option: return False - if ( + not_in_sample_rate = ( self.options["sample_rate"] < 1.0 and random.random() >= self.options["sample_rate"] - ): - # record a lost event if we did not sample this. + ) + if not_in_sample_rate: + # because we will not sample this event, record a "lost event". if self.transport: self.transport.record_lost_event("sample_rate", data_category="error") + return False return True From de226fd79b34cd32b3ffb631268b614be272fe5f Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 12 Apr 2022 11:05:24 +0200 Subject: [PATCH 3/3] ref(sampling): Made code more readable --- sentry_sdk/client.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index 9cdc9f5b9e..15cd94c3a1 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -224,17 +224,18 @@ def _is_ignored_error(self, event, hint): if exc_info is None: return False - type_name = get_type_name(exc_info[0]) - full_name = "%s.%s" % (exc_info[0].__module__, type_name) + error = exc_info[0] + error_type_name = get_type_name(exc_info[0]) + error_full_name = "%s.%s" % (exc_info[0].__module__, error_type_name) - for errcls in self.options["ignore_errors"]: + for ignored_error in self.options["ignore_errors"]: # String types are matched against the type name in the # exception only - if isinstance(errcls, string_types): - if errcls == full_name or errcls == type_name: + if isinstance(ignored_error, string_types): + if ignored_error == error_full_name or ignored_error == error_type_name: return True else: - if issubclass(exc_info[0], errcls): + if issubclass(error, ignored_error): return True return False