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 getsentry/develop#551 and getsentry/develop#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 <anton.pirker@sentry.io>

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