diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index abad953de3..3a73a4d2ea 100644 --- a/sentry_sdk/client.py +++ b/sentry_sdk/client.py @@ -1244,15 +1244,31 @@ def _capture_telemetry( serialized = telemetry._to_json() # type: ignore[union-attr] if before_send is not None: - serialized = before_send(serialized, {}) # type: ignore[arg-type] + exception_raised_in_before_send_func = False + with capture_internal_exceptions(): + try: + serialized = before_send(serialized, {}) # type: ignore[arg-type] + except Exception: + exception_raised_in_before_send_func = True + raise if ty in ("log", "metric"): + # We are ok with dropping metrics and logs when an exception is raised + # because we allow users to drop them in their respect before_send_* + # functions. + if exception_raised_in_before_send_func: + return # Logs and metrics can be dropped in their respective # before_send, so if we get None, don't queue them for sending. if serialized is None: return elif ty == "span" and isinstance(telemetry, StreamedSpan): + # Reset the span to its original value before we attempted + # to call the `before_send_span` callback + if exception_raised_in_before_send_func: + serialized = telemetry._to_json() + # Spans can't be dropped in before_send_span by design. They can # be altered though (e.g. to sanitize). Only allow changes to # name and attributes. diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 1790e13fbf..5534d1f478 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -1200,16 +1200,25 @@ def _set_initial_sampling_decision( # we would have bailed already if neither `traces_sampler` nor # `traces_sample_rate` were defined, so one of these should work; prefer # the hook if so - sample_rate = ( - client.options["traces_sampler"](sampling_context) - if callable(client.options.get("traces_sampler")) - # default inheritance behavior - else ( + if callable(client.options.get("traces_sampler")): + try: + sample_rate = client.options["traces_sampler"](sampling_context) + except Exception: + logger.warning( + "[Tracing] traces_sampler raised; falling back to parent sample rate or traces_sample_rate", + exc_info=True, + ) + sample_rate = ( + sampling_context["parent_sampled"] + if sampling_context["parent_sampled"] is not None + else client.options["traces_sample_rate"] + ) + else: + sample_rate = ( sampling_context["parent_sampled"] if sampling_context["parent_sampled"] is not None else client.options["traces_sample_rate"] ) - ) # Since this is coming from the user (or from a function provided by the # user), who knows what we might get. (The only valid values are @@ -1235,7 +1244,7 @@ def _set_initial_sampling_decision( "[Tracing] Discarding {transaction_description} because {reason}".format( transaction_description=transaction_description, reason=( - "traces_sampler returned 0 or False" + "traces_sampler returned 0 or False, or is using a fallback sample rate that is 0 or False" if callable(client.options.get("traces_sampler")) else "traces_sample_rate is set to 0" ), diff --git a/sentry_sdk/tracing_utils.py b/sentry_sdk/tracing_utils.py index ff4c03829e..b1a9bec516 100644 --- a/sentry_sdk/tracing_utils.py +++ b/sentry_sdk/tracing_utils.py @@ -1575,6 +1575,20 @@ def add_sentry_baggage_to_headers( ) +def _get_effective_sample_rate( + client: "Any", propagation_context: "PropagationContext" +) -> "Union[float, bool]": + if propagation_context.parent_sampled is not None: + propagation_context_sample_rate = propagation_context._sample_rate() + + if propagation_context_sample_rate is not None: + return propagation_context_sample_rate + else: + return propagation_context.parent_sampled + else: + return client.options["traces_sample_rate"] + + def _make_sampling_decision( name: str, attributes: "Optional[Attributes]", @@ -1630,15 +1644,20 @@ def _make_sampling_decision( if propagation_context.custom_sampling_context: sampling_context.update(propagation_context.custom_sampling_context) - sample_rate = client.options["traces_sampler"](sampling_context) + try: + sample_rate = client.options["traces_sampler"](sampling_context) + except Exception: + logger.warning( + "[Tracing] traces_sampler raised; falling back to parent sample rate or traces_sample_rate", + exc_info=True, + ) + sample_rate = _get_effective_sample_rate( + client=client, propagation_context=propagation_context + ) else: - if propagation_context.parent_sampled is not None: - if propagation_context._sample_rate() is not None: - sample_rate = propagation_context._sample_rate() - else: - sample_rate = propagation_context.parent_sampled - else: - sample_rate = client.options["traces_sample_rate"] + sample_rate = _get_effective_sample_rate( + client=client, propagation_context=propagation_context + ) # Validate whether the sample_rate we got is actually valid. Since # traces_sampler is user-provided, it could return anything. @@ -1646,10 +1665,10 @@ def _make_sampling_decision( logger.warning(f"[Tracing] Discarding {name} because of invalid sample rate.") return False, None, None, "sample_rate" - sample_rate = float(sample_rate) # type: ignore[arg-type] + sample_rate = float(sample_rate) if not sample_rate: if traces_sampler_defined: - reason = "traces_sampler returned 0 or False" + reason = "traces_sampler returned 0 or False, or is using a fallback sample rate that is 0 or False" else: reason = "traces_sample_rate is set to 0" diff --git a/tests/test_logs.py b/tests/test_logs.py index ebd7e7f969..82b5072a01 100644 --- a/tests/test_logs.py +++ b/tests/test_logs.py @@ -162,6 +162,29 @@ def _before_log(record, hint): assert before_log_called is True +@minimum_python_37 +@pytest.mark.tests_internal_exceptions +def test_logs_before_send_log_raises_does_not_crash_application( + sentry_init, capture_items +): + def _before_log(record, hint): + raise ValueError("before_send_log error") + + sentry_init( + enable_logs=True, + before_send_log=_before_log, + ) + items = capture_items("log") + + sentry_sdk.logger.error("This is an error log...") + + get_client().flush() + logs = [item.payload for item in items] + + # The exception in before_send_log is swallowed and the log is dropped. + assert not logs + + @minimum_python_37 def test_logs_attributes(sentry_init, capture_items): """ diff --git a/tests/test_metrics.py b/tests/test_metrics.py index e62a868dbe..23e11f0135 100644 --- a/tests/test_metrics.py +++ b/tests/test_metrics.py @@ -247,6 +247,28 @@ def _before_metric(record, hint): assert before_metric_called +@pytest.mark.tests_internal_exceptions +def test_metrics_before_send_raises_does_not_crash_application( + sentry_init, capture_items +): + def _before_metric(record, hint): + raise ValueError("before_send_metric error") + + sentry_init( + before_send_metric=_before_metric, + ) + items = capture_items("trace_metric") + + sentry_sdk.metrics.count("test.keep", 1) + + get_client().flush() + + # The exception in before_send_metric is swallowed and the metric is + # dropped. + metrics = [item.payload for item in items] + assert not metrics + + def test_transport_format(sentry_init, capture_envelopes): sentry_init(server_name="test-server", release="1.0.0") diff --git a/tests/tracing/test_sampling.py b/tests/tracing/test_sampling.py index f528c15947..eb27a9e156 100644 --- a/tests/tracing/test_sampling.py +++ b/tests/tracing/test_sampling.py @@ -195,15 +195,124 @@ def test_tolerates_traces_sampler_returning_a_boolean_span_streaming( ): sentry_init( traces_sampler=mock.Mock(return_value=traces_sampler_return_value), - _experiments={ - "trace_lifecycle": "stream", - }, + trace_lifecycle="stream", ) with sentry_sdk.traces.start_span(name="dogpark") as span: assert span.sampled is traces_sampler_return_value +@pytest.mark.parametrize( + "traces_sample_rate,expected_decision", + [(0.0, False), (0.25, False), (0.75, True), (1.00, True)], +) +def test_traces_sampler_raising_falls_back_to_traces_sample_rate( + sentry_init, + traces_sample_rate, + expected_decision, +): + sentry_init( + traces_sampler=mock.Mock(side_effect=ValueError("boom")), + traces_sample_rate=traces_sample_rate, + ) + + baggage = Baggage(sentry_items={"sample_rand": "0.500000"}) + transaction = start_transaction(name="dogpark", baggage=baggage) + assert transaction.sampled is expected_decision + + +@pytest.mark.parametrize("parent_sampling_decision", [True, False]) +def test_traces_sampler_raising_falls_back_to_parent_sampling_decision( + sentry_init, parent_sampling_decision +): + # set traces_sample_rate to produce the opposite of the parent decision, + # to prove the parent's decision takes precedence in the fallback + sentry_init( + traces_sampler=mock.Mock(side_effect=ValueError("boom")), + traces_sample_rate=0.0 if parent_sampling_decision else 1.0, + ) + + baggage = Baggage(sentry_items={"sample_rand": "0.500000"}) + transaction = start_transaction( + name="dogpark", baggage=baggage, parent_sampled=parent_sampling_decision + ) + assert transaction.sampled is parent_sampling_decision + + +@pytest.mark.parametrize( + "traces_sample_rate,expected_decision", + [(0.0, False), (0.25, False), (0.75, True), (1.00, True)], +) +def test_traces_sampler_raising_falls_back_to_traces_sample_rate_span_streaming( + sentry_init, + traces_sample_rate, + expected_decision, +): + sentry_init( + traces_sampler=mock.Mock(side_effect=ValueError("boom")), + traces_sample_rate=traces_sample_rate, + trace_lifecycle="stream", + ) + + sentry_sdk.traces.continue_trace( + { + "sentry-trace": "0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331", + "baggage": "sentry-sample_rand=0.500000", + } + ) + + with sentry_sdk.traces.start_span(name="dogpark") as span: + assert span.sampled is expected_decision + + +@pytest.mark.parametrize( + "traces_sample_rate,expected_decision", + [(0.0, False), (0.25, False), (0.75, True), (1.00, True)], +) +def test_traces_sampler_raising_no_incoming_trace_falls_back_to_traces_sample_rate_span_streaming( + sentry_init, + traces_sample_rate, + expected_decision, + monkeypatch, +): + sentry_init( + traces_sampler=mock.Mock(side_effect=ValueError("boom")), + traces_sample_rate=traces_sample_rate, + trace_lifecycle="stream", + ) + + # no continue_trace, so no propagated sample_rand; make it deterministic + monkeypatch.setattr( + "sentry_sdk.tracing_utils._generate_sample_rand", lambda *a, **kw: 0.5 + ) + + with sentry_sdk.traces.start_span(name="dogpark") as span: + assert span.sampled is expected_decision + + +def test_traces_sampler_raising_no_incoming_trace_and_no_traces_sample_rate( + sentry_init, +): + sentry_init( + traces_sampler=mock.Mock(side_effect=ValueError("boom")), + ) + + transaction = start_transaction(name="dogpark") + assert transaction.sampled is False + + +def test_traces_sampler_raising_no_incoming_trace_and_no_traces_sample_rate_span_streaming( + sentry_init, +): + sentry_init( + traces_sampler=mock.Mock(side_effect=ValueError("boom")), + trace_lifecycle="stream", + ) + + with sentry_sdk.traces.start_span(name="dogpark") as span: + assert span.sampled is False + + @pytest.mark.parametrize("sampling_decision", [True, False]) def test_only_captures_transaction_when_sampled_is_true( sentry_init, sampling_decision, capture_events diff --git a/tests/tracing/test_span_streaming.py b/tests/tracing/test_span_streaming.py index 17a5288490..669d15deaf 100644 --- a/tests/tracing/test_span_streaming.py +++ b/tests/tracing/test_span_streaming.py @@ -359,6 +359,41 @@ def before_send_span(span, hint): assert not before_send_span_called +@pytest.mark.tests_internal_exceptions +def test_before_send_span_raises_does_not_crash_application(sentry_init, capture_items): + def before_send_span(span, hint): + # Mutate the span before raising to ensure the partial mutation + # is discarded when the exception is raised. + span["name"] = "Mutated span name" + span["attributes"]["mutated"] = True + raise ValueError("before_send_span error") + + sentry_init( + traces_sample_rate=1.0, + _experiments={ + "before_send_span": before_send_span, + "trace_lifecycle": "stream", + }, + ) + + items = capture_items("span") + + with sentry_sdk.traces.start_span(name="span", attributes={"original": "value"}): + ... + + sentry_sdk.get_client().flush() + spans = [item.payload for item in items] + + # The exception in before_send_span is swallowed and the original, + # unmodified span is sent. + assert len(spans) == 1 + (span,) = spans + + assert span["name"] == "span" + assert span["attributes"]["original"] == "value" + assert "mutated" not in span["attributes"] + + def test_span_attributes(sentry_init, capture_items): sentry_init( traces_sample_rate=1.0,