From 29692eb4e30c905968d5cb22d36dd5d62d434b58 Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Thu, 14 Apr 2022 07:44:50 +0200 Subject: [PATCH 1/5] Update session also for non sampled events and change filter order We have already merged https://github.com/getsentry/sentry-python/pull/1390 where we moved `ignore_errors` before sampling. Background for the order change: We want to update the session for dropped events in case the event is dropped by sampling. Events dropped by other mechanisms should not update the session. See https://github.com/getsentry/develop/pull/551 Now we would like to discuss if we can simply move sampling after `before_send` and `event_processor` and update the session right before sampling. What are implications of changing this? * How does this affect session count and session crash rate? * Will this have a negative effect on performance as `before_send` and `event_processor` will now be executed for every event instead of only being executed for sampled events? Developers may have fine tuned their sample rate for a good performance tradeoff and now we change. Also developers can supply their own implementations for both `before_send` and `event_processor` on some SDKs so we have no way of predicting performance I'm afraid. * We are uncertain why a developer chose to drop an event in `before_send` and `event_processor`: ** Was it because they want to ignore the event - then it shouldn't update the session ** Or was it to save quota - then it should update the session Please feel free to optimize the code this is just to start the discussion. --- sentry_sdk/client.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index 15cd94c3a1..02ac95cadd 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -260,6 +260,12 @@ def _should_capture( if ignored_by_config_option: return False + return True + + def _should_sample( + self, + event, # type: Event + ): not_in_sample_rate = ( self.options["sample_rate"] < 1.0 and random.random() >= self.options["sample_rate"] @@ -270,7 +276,7 @@ def _should_capture( self.transport.record_lost_event("sample_rate", data_category="error") return False - + return True def _update_session_from_event( @@ -348,6 +354,9 @@ def capture_event( session = scope._session if scope else None if session: self._update_session_from_event(session, event) + + if not self._should_sample(event): + return None attachments = hint.get("attachments") is_transaction = event_opt.get("type") == "transaction" From 5edb6340ea570b51b4e3b141f09a5fe263d9e37a Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Thu, 14 Apr 2022 08:46:50 +0200 Subject: [PATCH 2/5] Update client.py --- 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 02ac95cadd..a5bee0b454 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -261,7 +261,7 @@ def _should_capture( return False return True - + def _should_sample( self, event, # type: Event @@ -276,7 +276,7 @@ def _should_sample( self.transport.record_lost_event("sample_rate", data_category="error") return False - + return True def _update_session_from_event( @@ -354,7 +354,7 @@ def capture_event( session = scope._session if scope else None if session: self._update_session_from_event(session, event) - + if not self._should_sample(event): return None From a3bc9725dc4862e5af253d2b527f5b55f805153c Mon Sep 17 00:00:00 2001 From: Alexander Dinauer Date: Thu, 14 Apr 2022 08:49:46 +0200 Subject: [PATCH 3/5] Specify return type --- sentry_sdk/client.py | 1 + 1 file changed, 1 insertion(+) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index a5bee0b454..0caa362b2d 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -266,6 +266,7 @@ def _should_sample( self, event, # type: Event ): + # type: (...) -> bool not_in_sample_rate = ( self.options["sample_rate"] < 1.0 and random.random() >= self.options["sample_rate"] From 3b5fdeff4a761bca6842f2e889e9625b1af8f265 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Wed, 20 Apr 2022 13:43:17 +0200 Subject: [PATCH 4/5] New sample function is only for errors --- sentry_sdk/client.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index 0caa362b2d..cda9b063cf 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -356,11 +356,12 @@ def capture_event( if session: self._update_session_from_event(session, event) - if not self._should_sample(event): + is_transaction = event_opt.get("type") == "transaction" + + if not is_transaction and not self._should_sample_error(event): return None attachments = hint.get("attachments") - is_transaction = event_opt.get("type") == "transaction" # this is outside of the `if` immediately below because even if we don't # use the value, we want to make sure we remove it before the event is From d4cb745a607f1f74ef10a6ebc4c7256a38271d18 Mon Sep 17 00:00:00 2001 From: Neel Shah Date: Wed, 20 Apr 2022 14:01:49 +0200 Subject: [PATCH 5/5] Learn how to commit --- sentry_sdk/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index cda9b063cf..628cb00ee3 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -262,7 +262,7 @@ def _should_capture( return True - def _should_sample( + def _should_sample_error( self, event, # type: Event ):