(wip): No more separate record_lost_transaction

Author: szokeasaurusrex

sentry_sdk/client.py

@@ -454,14 +454,15 @@ def _prepare_event(
             # one of the event/error processors returned None
             if event_ is None:
                 if self.transport:
+                    self.transport.record_lost_event(
+                        "event_processor",
+                        data_category=("transaction" if is_transaction else "error"),
+                    )
                     if is_transaction:
-                        self.transport.record_lost_transaction(
-                            "event_processor", len(event.get("spans", []))
-                        )
-                    else:
                         self.transport.record_lost_event(
                             "event_processor",
-                            data_category="error",
+                            data_category="span",
+                            quantity=spans_before + 1,  # +1 for the transaction itself
                         )
                 return None
 
@@ -559,8 +560,13 @@ def _prepare_event(
             if new_event is None:
                 logger.info("before send transaction dropped event")
                 if self.transport:
-                    self.transport.record_lost_transaction(
-                        reason="before_send", span_count=len(event.get("spans", []))
+                    self.transport.record_lost_event(
+                        reason="before_send", data_category="transaction"
+                    )
+                    self.transport.record_lost_event(
+                        reason="before_send",
+                        data_category="span",
+                        quantity=spans_before + 1,  # +1 for the transaction itself
                     )
             else:
                 spans_delta = spans_before - len(new_event.get("spans", []))

sentry_sdk/tracing.py

@@ -854,9 +854,10 @@ def finish(self, hub=None, end_timestamp=None):
                 else:
                     reason = "sample_rate"
 
-                # No spans are discarded here because they were never recorded in the first place
-                client.transport.record_lost_transaction(reason, span_count=0)
+                client.transport.record_lost_event(reason, data_category="transaction")
 
+                # Only one span (the transaction itself) is discarded, since we did not record any spans here.
+                client.transport.record_lost_event(reason, data_category="span")
             return None
 
         if not self.name:

sentry_sdk/transport.py

@@ -144,22 +144,12 @@ def record_lost_event(
         extracted from the item, and the values passed for
         data_category and quantity are ignored.
 
-        data_category="transaction" should use record_lost_transaction,
-        instead. Items containing a transaction are okay to pass, since
-        we can get the number of spans from the transaction.
-        """
-        return None
-
-    def record_lost_transaction(
-        self,
-        reason,  # type: str
-        span_count,  # type: int
-    ):
-        # type: (...) -> None
-        """This increments a counter for loss of a transaction and the transaction's spans.
-
-        The span_count should only include the number of spans contained in the transaction, EXCLUDING the transaction
-        itself.
+        When recording a lost transaction via data_category="transaction",
+        the calling code should also record the lost spans via this method.
+        When recording lost spans, `quantity` should be set to the number
+        of contained spans, plus one for the transaction itself. When
+        passing an Item containing a transaction via the `item` parameter,
+        this method automatically records the lost spans.
         """
         return None
 
@@ -254,49 +244,28 @@ def record_lost_event(
         if not self.options["send_client_reports"]:
             return
 
-        if data_category == "transaction":
-            warnings.warn(
-                "Use record_lost_transaction for transactions to ensure lost spans are counted.",
-                stacklevel=2,
-            )
-
         if item is not None:
             data_category = item.data_category
+            quantity = 1  # If an item is provided, we always count it as 1 (except for attachments, handled below).
 
             if data_category == "transaction":
-                # Handle transactions through record_lost_transaction.
+                # Also record the lost spans
                 event = item.get_transaction_event() or {}
-                span_count = len(event.get("spans") or [])
-                self.record_lost_transaction(reason, span_count)
-                return
+
+                # +1 for the transaction itself
+                span_count = len(event.get("spans") or []) + 1
+                self.record_lost_event(reason, "span", quantity=span_count)
 
             elif data_category == "attachment":
                 # quantity of 0 is actually 1 as we do not want to count
                 # empty attachments as actually empty.
                 quantity = len(item.get_bytes()) or 1
 
-            else:
-                # Any other item data category should be counted as 1, since the one item corresponds to one event.
-                quantity = 1
-
         elif data_category is None:
             raise TypeError("data category not provided")
 
         self._discarded_events[data_category, reason] += quantity
 
-    def record_lost_transaction(
-        self,
-        reason,  # type: str
-        span_count,  # type: int
-    ):  # type: (...) -> None
-        if not self.options["send_client_reports"]:
-            return
-
-        self._discarded_events["transaction", reason] += 1
-        self._discarded_events["span", reason] += (
-            span_count + 1
-        )  # Also count the transaction itself
-
     def _update_rate_limits(self, response):
         # type: (urllib3.BaseHTTPResponse) -> None
 

tests/conftest.py

@@ -253,8 +253,8 @@ def inner():
         calls = []
         test_client = sentry_sdk.get_client()
 
-        def record_lost_event(reason, data_category=None, item=None):
-            calls.append((reason, data_category, item))
+        def record_lost_event(reason, data_category=None, item=None, *, quantity=1):
+            calls.append((reason, data_category, item, quantity))
 
         monkeypatch.setattr(
             test_client.transport, "record_lost_event", record_lost_event

tests/profiler/test_transaction_profiler.py

@@ -162,7 +162,7 @@ def test_profiles_sample_rate(
     elif profile_count:
         assert record_lost_event_calls == []
     else:
-        assert record_lost_event_calls == [("sample_rate", "profile", None)]
+        assert record_lost_event_calls == [("sample_rate", "profile", None, 1)]
 
 
 @pytest.mark.parametrize(
@@ -231,7 +231,7 @@ def test_profiles_sampler(
     if profile_count:
         assert record_lost_event_calls == []
     else:
-        assert record_lost_event_calls == [("sample_rate", "profile", None)]
+        assert record_lost_event_calls == [("sample_rate", "profile", None, 1)]
 
 
 def test_minimum_unique_samples_required(
@@ -260,7 +260,7 @@ def test_minimum_unique_samples_required(
     # because we dont leave any time for the profiler to
     # take any samples, it should be not be sent
     assert len(items["profile"]) == 0
-    assert record_lost_event_calls == [("insufficient_data", "profile", None)]
+    assert record_lost_event_calls == [("insufficient_data", "profile", None, 1)]
 
 
 @pytest.mark.forked

tests/test_basics.py

@@ -570,7 +570,7 @@ def test_dedupe_event_processor_drop_records_client_report(
 
     assert event["level"] == "error"
     assert "exception" in event
-    assert lost_event_call == ("event_processor", "error", None)
+    assert lost_event_call == ("event_processor", "error", None, 1)
 
 
 def test_event_processor_drop_records_client_report(
@@ -602,8 +602,9 @@ def foo(event, hint):
         # Using Counter because order of record_lost_event calls does not matter
         assert Counter(record_lost_event_calls) == Counter(
             [
-                ("event_processor", "error", None),
-                ("event_processor", "transaction", None),
+                ("event_processor", "error", None, 1),
+                ("event_processor", "transaction", None, 1),
+                ("event_processor", "span", None, 1),
             ]
         )
 

tests/test_monitor.py

@@ -1,4 +1,5 @@
 import random
+from collections import Counter
 from unittest import mock
 
 import sentry_sdk
@@ -79,7 +80,12 @@ def test_transaction_uses_downsampled_rate(
         assert transaction.sampled is False
         assert transaction.sample_rate == 0.5
 
-    assert record_lost_event_calls == [("backpressure", "transaction", None)]
+    assert Counter(record_lost_event_calls) == Counter(
+        [
+            ("backpressure", "transaction", None, 1),
+            ("backpressure", "span", None, 1),
+        ]
+    )
 
 
 def test_monitor_no_thread_on_shutdown_no_errors(sentry_init):