From 91436cdc582d1ea38e1a6280553b23f3a6d14cc7 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Tue, 12 Apr 2022 13:30:45 +0200 Subject: [PATCH] Change ordering of event drop mechanisms (#1390) * 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. Co-authored-by: Anton Pirker --- sentry_sdk/client.py | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index efc8799c00..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 @@ -246,23 +247,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 ( + ignored_by_config_option = self._is_ignored_error(event, hint) + if ignored_by_config_option: + return False + + 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 - if self._is_ignored_error(event, hint): return False return True