Repository: getsentry/sentry-python

Issue:
Improve `record_lost_event` parameter documentation 
`data_category` and `item` are mutually exclusive; only one should be provided at a time. 

https://github.com/getsentry/sentry-python/blob/31efa62c90e5b88c6c15b55f6908a25133d65958/sentry_sdk/transport.py#L133-L142

Discussion:
szokeasaurusrex: Marked as draft, still need to test this
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/getsentry/sentry-python/pull/3244?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) Report
Attention: Patch coverage is `75.00000%` with `5 lines` in your changes missing coverage. Please review.
> Project coverage is 78.84%. Comparing base [(`460e508`)](https://app.codecov.io/gh/getsentry/sentry-python/commit/460e50896e3ffbcf411a961f1b6531e3d791a568?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) to head [(`1cc0144`)](https://app.codecov.io/gh/getsentry/sentry-python/commit/1cc01442cd447562017adecfc50a6a3c0172788d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry).

<details><summary>Additional details and impacted files</summary>


```diff
@@                                    Coverage Diff                                     @@
##           szokeasaurusrex/improve-transport-data-category-typing    #3244      +/-   ##
==========================================================================================
- Coverage                                                   79.33%   78.84%   -0.49%     
==========================================================================================
  Files                                                         132      132              
  Lines                                                       14218    14234      +16     
  Branches                                                     2985     2989       +4     
==========================================================================================
- Hits                                                        11280    11223      -57     
- Misses                                                       2092     2165      +73     
  Partials                                                      846      846              
```

| [Files](https://app.codecov.io/gh/getsentry/sentry-python/pull/3244?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | Coverage Δ | |
|---|---|---|
| [sentry\_sdk/client.py](https://app.codecov.io/gh/getsentry/sentry-python/pull/3244?src=pr&el=tree&filepath=sentry_sdk%2Fclient.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-c2VudHJ5X3Nkay9jbGllbnQucHk=) | `74.57% <100.00%> (-4.75%)` | :arrow_down: |
| [sentry\_sdk/tracing.py](https://app.codecov.io/gh/getsentry/sentry-python/pull/3244?src=pr&el=tree&filepath=sentry_sdk%2Ftracing.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-c2VudHJ5X3Nkay90cmFjaW5nLnB5) | `73.54% <100.00%> (ø)` | |
| [sentry\_sdk/transport.py](https://app.codecov.io/gh/getsentry/sentry-python/pull/3244?src=pr&el=tree&filepath=sentry_sdk%2Ftransport.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-c2VudHJ5X3Nkay90cmFuc3BvcnQucHk=) | `77.45% <66.66%> (-2.69%)` | :arrow_down: |

... and [11 files with indirect coverage changes](https://app.codecov.io/gh/getsentry/sentry-python/pull/3244/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry)

</details>
szokeasaurusrex: > We need to keep track of the amount of spans prior before_send_transaction and any event processors and calculate the delta afterwards, so we can report eventually dropped spans as well.

@cleptric, does this mean that we also need to report dropped spans in the `before_send_transaction` even if the transaction gets sent to Sentry?

For example, in the situation below, do we report any dropped spans to Sentry?

<img width="894" alt="Transaction diagram" src="https://github.com/getsentry/sentry-python/assets/7881302/ce2dcabc-b2e8-4a44-9b85-946e9ec03767">


And for clarity, in the following situation where the transaction gets dropped after `before_send`/`before_send_transaction`, e.g. in the `Transport`, how many spans do we report as dropped, 4 or 6?

<img width="873" alt="Transaction diagram-2" src="https://github.com/getsentry/sentry-python/assets/7881302/e98830b0-7326-42db-9ca0-976c66adab7c">
cleptric: The first example reports two dropped spans, the second example reports six dropped spans and one dropped transaction.

Patch:
diff --git a/sentry_sdk/_types.py b/sentry_sdk/_types.py
index bd229977..14fa8d08 100644
--- a/sentry_sdk/_types.py
+++ b/sentry_sdk/_types.py
@@ -155,6 +155,7 @@ if TYPE_CHECKING:
         "profile_chunk",
         "metric_bucket",
         "monitor",
+        "span",
     ]
     SessionStatus = Literal["ok", "exited", "crashed", "abnormal"]
 
diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py
index 07cd3902..f93aa935 100644
--- a/sentry_sdk/client.py
+++ b/sentry_sdk/client.py
@@ -448,6 +448,7 @@ class _Client(BaseClient):
 
         if scope is not None:
             is_transaction = event.get("type") == "transaction"
+            spans_before = len(event.get("spans", []))
             event_ = scope.apply_to_event(event, hint, self.options)
 
             # one of the event/error processors returned None
@@ -457,10 +458,22 @@ class _Client(BaseClient):
                         "event_processor",
                         data_category=("transaction" if is_transaction else "error"),
                     )
+                    if is_transaction:
+                        self.transport.record_lost_event(
+                            "event_processor",
+                            data_category="span",
+                            quantity=spans_before + 1,  # +1 for the transaction itself
+                        )
                 return None
 
             event = event_
 
+            spans_delta = spans_before - len(event.get("spans", []))
+            if is_transaction and spans_delta > 0 and self.transport is not None:
+                self.transport.record_lost_event(
+                    "event_processor", data_category="span", quantity=spans_delta
+                )
+
         if (
             self.options["attach_stacktrace"]
             and "exception" not in event
@@ -541,14 +554,27 @@ class _Client(BaseClient):
             and event.get("type") == "transaction"
         ):
             new_event = None
+            spans_before = len(event.get("spans", []))
             with capture_internal_exceptions():
                 new_event = before_send_transaction(event, hint or {})
             if new_event is None:
                 logger.info("before send transaction dropped event")
                 if self.transport:
                     self.transport.record_lost_event(
-                        "before_send", data_category="transaction"
+                        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", []))
+                if spans_delta > 0 and self.transport is not None:
+                    self.transport.record_lost_event(
+                        reason="before_send", data_category="span", quantity=spans_delta
+                    )
+
             event = new_event  # type: ignore
 
         return event
diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py
index fe8293d6..43a13b52 100644
--- a/sentry_sdk/tracing.py
+++ b/sentry_sdk/tracing.py
@@ -119,11 +119,9 @@ if TYPE_CHECKING:
         },
     )
 
-
 BAGGAGE_HEADER_NAME = "baggage"
 SENTRY_TRACE_HEADER_NAME = "sentry-trace"
 
-
 # Transaction source
 # see https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations
 TRANSACTION_SOURCE_CUSTOM = "custom"
@@ -858,6 +856,8 @@ class Transaction(Span):
 
                 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:
diff --git a/sentry_sdk/transport.py b/sentry_sdk/transport.py
index 293dfc0e..63bd1d9f 100644
--- a/sentry_sdk/transport.py
+++ b/sentry_sdk/transport.py
@@ -133,10 +133,23 @@ class Transport(ABC):
         reason,  # type: str
         data_category=None,  # type: Optional[EventDataCategory]
         item=None,  # type: Optional[Item]
+        *,
+        quantity=1,  # type: int
     ):
         # type: (...) -> None
         """This increments a counter for event loss by reason and
-        data category.
+        data category by the given positive-int quantity (default 1).
+
+        If an item is provided, the data category and quantity are
+        extracted from the item, and the values passed for
+        data_category and quantity are ignored.
+
+        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
 
@@ -224,15 +237,26 @@ class HttpTransport(Transport):
         reason,  # type: str
         data_category=None,  # type: Optional[EventDataCategory]
         item=None,  # type: Optional[Item]
+        *,
+        quantity=1,  # type: int
     ):
         # type: (...) -> None
         if not self.options["send_client_reports"]:
             return
 
-        quantity = 1
         if item is not None:
             data_category = item.data_category
-            if data_category == "attachment":
+            quantity = 1  # If an item is provided, we always count it as 1 (except for attachments, handled below).
+
+            if data_category == "transaction":
+                # Also record the lost spans
+                event = item.get_transaction_event() or {}
+
+                # +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


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index b043a849..eada3bda 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -253,8 +253,8 @@ def capture_record_lost_event_calls(monkeypatch):
         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
diff --git a/tests/profiler/test_transaction_profiler.py b/tests/profiler/test_transaction_profiler.py
index b30faffc..ec506cfa 100644
--- a/tests/profiler/test_transaction_profiler.py
+++ b/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
diff --git a/tests/test_basics.py b/tests/test_basics.py
index 391c1c41..439215e0 100644
--- a/tests/test_basics.py
+++ b/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 test_event_processor_drop_records_client_report(
         # 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),
             ]
         )
 
diff --git a/tests/test_client.py b/tests/test_client.py
index a2fea562..3be8b1e6 100644
--- a/tests/test_client.py
+++ b/tests/test_client.py
@@ -3,6 +3,7 @@ import json
 import subprocess
 import sys
 import time
+from collections import Counter, defaultdict
 from collections.abc import Mapping
 from textwrap import dedent
 from unittest import mock
@@ -1214,3 +1215,159 @@ def test_uwsgi_warnings(sentry_init, recwarn, opt, missing_flags):
                 assert flag in str(record.message)
         else:
             assert not recwarn
+
+
+class TestSpanClientReports:
+    """
+    Tests for client reports related to spans.
+    """
+
+    @staticmethod
+    def span_dropper(spans_to_drop):
+        """
+        Returns a function that can be used to drop spans from an event.
+        """
+
+        def drop_spans(event, _):
+            event["spans"] = event["spans"][spans_to_drop:]
+            return event
+
+        return drop_spans
+
+    @staticmethod
+    def mock_transaction_event(span_count):
+        """
+        Returns a mock transaction event with the given number of spans.
+        """
+
+        return defaultdict(
+            mock.MagicMock,
+            type="transaction",
+            spans=[mock.MagicMock() for _ in range(span_count)],
+        )
+
+    def __init__(self, span_count):
+        """Configures a test case with the number of spans dropped and whether the transaction was dropped."""
+        self.span_count = span_count
+        self.expected_record_lost_event_calls = Counter()
+        self.before_send = lambda event, _: event
+        self.event_processor = lambda event, _: event
+
+    def _update_resulting_calls(self, reason, drops_transactions=0, drops_spans=0):
+        """
+        Updates the expected calls with the given resulting calls.
+        """
+        if drops_transactions > 0:
+            self.expected_record_lost_event_calls[
+                (reason, "transaction", None, drops_transactions)
+            ] += 1
+
+        if drops_spans > 0:
+            self.expected_record_lost_event_calls[
+                (reason, "span", None, drops_spans)
+            ] += 1
+
+    def with_before_send(
+        self,
+        before_send,
+        *,
+        drops_transactions=0,
+        drops_spans=0,
+    ):
+        self.before_send = before_send
+        self._update_resulting_calls(
+            "before_send",
+            drops_transactions,
+            drops_spans,
+        )
+
+        return self
+
+    def with_event_processor(
+        self,
+        event_processor,
+        *,
+        drops_transactions=0,
+        drops_spans=0,
+    ):
+        self.event_processor = event_processor
+        self._update_resulting_calls(
+            "event_processor",
+            drops_transactions,
+            drops_spans,
+        )
+
+        return self
+
+    def run(self, sentry_init, capture_record_lost_event_calls):
+        """Runs the test case with the configured parameters."""
+        sentry_init(before_send_transaction=self.before_send)
+        record_lost_event_calls = capture_record_lost_event_calls()
+
+        with sentry_sdk.isolation_scope() as scope:
+            scope.add_event_processor(self.event_processor)
+            event = self.mock_transaction_event(self.span_count)
+            sentry_sdk.get_client().capture_event(event, scope=scope)
+
+        # We use counters to ensure that the calls are made the expected number of times, disregarding order.
+        assert Counter(record_lost_event_calls) == self.expected_record_lost_event_calls
+
+
+@pytest.mark.parametrize(
+    "test_config",
+    (
+        TestSpanClientReports(span_count=10),  # No spans dropped
+        TestSpanClientReports(span_count=0).with_before_send(
+            lambda e, _: None,
+            drops_transactions=1,
+            drops_spans=1,
+        ),
+        TestSpanClientReports(span_count=10).with_before_send(
+            lambda e, _: None,
+            drops_transactions=1,
+            drops_spans=11,
+        ),
+        TestSpanClientReports(span_count=10).with_before_send(
+            TestSpanClientReports.span_dropper(3),
+            drops_spans=3,
+        ),
+        TestSpanClientReports(span_count=10).with_before_send(
+            TestSpanClientReports.span_dropper(10),
+            drops_spans=10,
+        ),
+        TestSpanClientReports(span_count=10).with_event_processor(
+            lambda e, _: None,
+            drops_transactions=1,
+            drops_spans=11,
+        ),
+        TestSpanClientReports(span_count=10).with_event_processor(
+            TestSpanClientReports.span_dropper(3),
+            drops_spans=3,
+        ),
+        TestSpanClientReports(span_count=10).with_event_processor(
+            TestSpanClientReports.span_dropper(10),
+            drops_spans=10,
+        ),
+        TestSpanClientReports(span_count=10)
+        .with_event_processor(
+            TestSpanClientReports.span_dropper(3),
+            drops_spans=3,
+        )
+        .with_before_send(
+            TestSpanClientReports.span_dropper(5),
+            drops_spans=5,
+        ),
+        TestSpanClientReports(10)
+        .with_event_processor(
+            TestSpanClientReports.span_dropper(3),
+            drops_spans=3,
+        )
+        .with_before_send(
+            lambda e, _: None,
+            drops_transactions=1,
+            drops_spans=8,  # 3 of the 11 (incl. transaction) spans already dropped
+        ),
+    ),
+)
+def test_dropped_transaction(sentry_init, capture_record_lost_event_calls, test_config):
+    test_config.run(sentry_init, capture_record_lost_event_calls)
diff --git a/tests/test_monitor.py b/tests/test_monitor.py
index e15b3a7d..03e415b5 100644
--- a/tests/test_monitor.py
+++ b/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):
diff --git a/tests/test_transport.py b/tests/test_transport.py
index 4ed95053..dc8e8073 100644
--- a/tests/test_transport.py
+++ b/tests/test_transport.py
@@ -86,6 +86,20 @@ def make_client(request, capturing_server):
     return inner
 
 
+def mock_transaction_envelope(span_count):
+    # type: (int) -> Envelope
+    event = defaultdict(
+        mock.MagicMock,
+        type="transaction",
+        spans=[mock.MagicMock() for _ in range(span_count)],
+    )
+
+    envelope = Envelope()
+    envelope.add_transaction(event)
+
+    return envelope
+
+
 @pytest.mark.forked
 @pytest.mark.parametrize("debug", (True, False))
 @pytest.mark.parametrize("client_flush_method", ["close", "flush"])
@@ -425,12 +439,17 @@ def test_data_category_limits_reporting(
 
     discarded_events = report["discarded_events"]
 
-    assert len(discarded_events) == 2
+    assert len(discarded_events) == 3
     assert {
         "category": "transaction",
         "reason": "ratelimit_backoff",
         "quantity": 2,
     } in discarded_events
+    assert {
+        "category": "span",
+        "reason": "ratelimit_backoff",
+        "quantity": 2,
+    } in discarded_events
     assert {
         "category": "attachment",
         "reason": "ratelimit_backoff",
@@ -454,9 +473,19 @@ def test_data_category_limits_reporting(
     envelope = capturing_server.captured[1].envelope
     assert envelope.items[0].type == "client_report"
     report = parse_json(envelope.items[0].get_bytes())
-    assert report["discarded_events"] == [
-        {"category": "transaction", "reason": "ratelimit_backoff", "quantity": 1},
-    ]
+
+    discarded_events = report["discarded_events"]
+    assert len(discarded_events) == 2
+    assert {
+        "category": "transaction",
+        "reason": "ratelimit_backoff",
+        "quantity": 1,
+    } in discarded_events
+    assert {
+        "category": "span",
+        "reason": "ratelimit_backoff",
+        "quantity": 1,
+    } in discarded_events
 
 
 @pytest.mark.parametrize("response_code", [200, 429])
@@ -613,3 +642,59 @@ def test_hub_cls_backwards_compat():
 
     with pytest.deprecated_call():
         assert transport.hub_cls is TestCustomHubClass
+
+
+@pytest.mark.parametrize("quantity", (1, 2, 10))
+def test_record_lost_event_quantity(capturing_server, make_client, quantity):
+    client = make_client()
+    transport = client.transport
+
+    transport.record_lost_event(reason="test", data_category="span", quantity=quantity)
+    client.flush()
+
+    (captured,) = capturing_server.captured  # Should only be one envelope
+    envelope = captured.envelope
+    (item,) = envelope.items  # Envelope should only have one item
+
+    assert item.type == "client_report"
+
+    report = parse_json(item.get_bytes())
+
+    assert report["discarded_events"] == [
+        {"category": "span", "reason": "test", "quantity": quantity}
+    ]
+
+
+@pytest.mark.parametrize("span_count", (0, 1, 2, 10))
+def test_record_lost_event_transaction_item(capturing_server, make_client, span_count):
+    client = make_client()
+    transport = client.transport
+
+    envelope = mock_transaction_envelope(span_count)
+    (transaction_item,) = envelope.items
+
+    transport.record_lost_event(reason="test", item=transaction_item)
+    client.flush()
+
+    (captured,) = capturing_server.captured  # Should only be one envelope
+    envelope = captured.envelope
+    (item,) = envelope.items  # Envelope should only have one item
+
+    assert item.type == "client_report"
+
+    report = parse_json(item.get_bytes())
+    discarded_events = report["discarded_events"]
+
+    assert len(discarded_events) == 2
+
+    assert {
+        "category": "transaction",
+        "reason": "test",
+        "quantity": 1,
+    } in discarded_events
+
+    assert {
+        "category": "span",
+        "reason": "test",
+        "quantity": span_count + 1,
+    } in discarded_events
diff --git a/tests/tracing/test_sampling.py b/tests/tracing/test_sampling.py
index d9bb6ef4..491281fa 100644
--- a/tests/tracing/test_sampling.py
+++ b/tests/tracing/test_sampling.py
@@ -264,7 +264,11 @@ def test_warns_and_sets_sampled_to_false_on_invalid_traces_sampler_return_value(
     "traces_sample_rate,sampled_output,expected_record_lost_event_calls",
     [
         (None, False, []),
-        (0.0, False, [("sample_rate", "transaction", None)]),
+        (
+            0.0,
+            False,
+            [("sample_rate", "transaction", None, 1), ("sample_rate", "span", None, 1)],
+        ),
         (1.0, True, []),
     ],
 )
@@ -290,7 +294,11 @@ def test_records_lost_event_only_if_traces_sample_rate_enabled(
     "traces_sampler,sampled_output,expected_record_lost_event_calls",
     [
         (None, False, []),
-        (lambda _x: 0.0, False, [("sample_rate", "transaction", None)]),
+        (
+            lambda _x: 0.0,
+            False,
+            [("sample_rate", "transaction", None, 1), ("sample_rate", "span", None, 1)],
+        ),
         (lambda _x: 1.0, True, []),
     ],
 )